Handling of default values about dotnet-kube-client HOT 84 OPEN

I think the best we could do until Kubernetes exposes the default value through OpenAPI is to maintain a hand-maintained list of default values in a map in the generator script, and apply that default value in the property definition.

from dotnet-kube-client.

I wouldn't want to have PowerShell users needing to do that though...

I have this vision of cmdlets that can all take the same strongly typed Kubernetes model objects as input and all output the same strongly typed objects. Trying to find a solution that gets as close to that as possible 🤔

Seen this? https://github.com/wbish/jsondiffpatch.net

Yeah I've seen that, it's not JSON Patch (RFC 6902) though. Kubernetes API only supports RFC 6902 JSON Patch, RFC 7386 JSON Merge Patch and their custom Strategic Merge Patch. I therefor went with JSON Patch.

from dotnet-kube-client.

One big use case I have is actually loading YAML into an object, mutating it (in PowerShell), then applying it (sending a patch to the API).

For example this script in CI:

• build Docker images
• push them to a registry
• tag them with the currently built commit
• read all Kubernetes YAML (in the repo)
  Get-ChildItem -Recurse -File *.yml | ConvertFrom-KubeYaml
• for all Deployments, modify the image fields in the PodSpecs to point to the tag of the currently built commit
  | ForEach-Object { if ($_ is [DeploymentV1Beta]) { $_.Spec.Containers.Image = $commit } }
• apply all patches
  | Update-KubeResource

For writing this script, it would be tremendously helpful if I had IntelliSense on $_.Spec.Containers.Image (i.e. it's strongly typed).

I couldn't find good examples of how to use PSObject in C# to add the note property and also read it again, while still declaring types for parameters and output...

from dotnet-kube-client.

Sorry, separation of concerns. I.e. not have one cmdlet that is solely responsible for parsing, diffing and querying the server. Because there may be use cases that we are not thinking of where one wants to intercept these steps.

from dotnet-kube-client.

I implemented serialising into PSObject and diffing PSObject and it works well. I am setting type names so output formatting is preserved. I think this is the best approach, so we can abandon the tracked models.

> Get-Content -Raw ./deploy_orig.yml | ConvertFrom-KubeYaml | Get-Member


   TypeName: KubeClient.Models.DeploymentV1Beta1

Name        MemberType     Definition
----        ----------     ----------
Equals      Method         bool Equals(System.Object obj)
GetHashCode Method         int GetHashCode()
GetType     Method         type GetType()
ToString    Method         string ToString()
ApiVersion  NoteProperty   string ApiVersion=apps/v1beta1
Kind        NoteProperty   string Kind=Deployment
Metadata    NoteProperty   KubeClient.Models.ObjectMetaV1 Metadata=@{Annotations=System.Collections.Generic.Dictionary`2[System.String,System.Object]; Name=sourcegraph-frontend; Namespace=prod}
Spec        NoteProperty   KubeClient.Models.DeploymentSpecV1Beta1 Spec=@{MinReadySeconds=10; Replicas=3; RevisionHistoryLimit=10; Strategy=; Template=}

from dotnet-kube-client.

Interesting! I wonder how kubectl / client-go handle it. I take it the OpenAPI / Swagger document doesn't list these defaults? I'll take a look at both of these ideas this morning if I get time...

from dotnet-kube-client.

kubectl apply circumvents the problem by only diffing maps instead of the struct types. I thought about this but I think it would severely impact the UX so I would rather hardcode defaults.
The default value is not exposed through OpenAPI but usually mentioned in the field description. I suggested adding this in the comment I linked above.

I don't know how client-go handles this

from dotnet-kube-client.

I like your idea - let's do that.

from dotnet-kube-client.

So even if we have DefaultValue attributes on everything and initialisers, it would still mean that if a YAML does not declare a field and the value is different than the default on the server, it will reset the value to the default value. I wonder if that is acceptable behaviour or not.
If we always ignore default values, it would be impossible to reset a value back to the default value.

Maybe we do need to make everything with a default nullable? So that null means "not specified"/"ignore" while the actual default value means "reset". The OpenAPI spec is inconsistent about this too - some scalars are nullable in addition to having a default value, while others are not.

The question is just how inconvenient this would make using the API client, if even fields are nullable that are guaranteed to be set to a default value by the server.

from dotnet-kube-client.

Maybe we need to generate two sets of classes for API responses and for inputs? Where inputs just has more optional fields, so the API responses can extend the input classes and override the fields with default values to make them non-optional.

What do you usually do in C# in such cases? In TypeScript I can distinguish between null and undefined and scalars don't have zero values (undefined is the zero value for everything).

from dotnet-kube-client.

Hmm, not sure about yamldotnet but JSON.NET has DefaultValueHandling so if the value is some well-known scalar value (or null) the member won't be serialised. Perhaps yamldotnet has something similar?

from dotnet-kube-client.

aaubry/YamlDotNet#21

from dotnet-kube-client.

Serialisation is not the problem, it's generating the patch (diffing yaml vs server state)

from dotnet-kube-client.

Ah. Yeah I could apply the DefaultValue attribute and you could look for that?

from dotnet-kube-client.

You could treat that default value as "missing" or "unspecified" perhaps?

from dotnet-kube-client.

If the attribute is.missing th

from dotnet-kube-client.

Man, I hate doing GitHub on my phone :)

from dotnet-kube-client.

If attribute is missing there is a way to determine default value for type but I'd have to look it up.

from dotnet-kube-client.

Here you go:

https://stackoverflow.com/a/353073/885866

from dotnet-kube-client.

Yeah, but if you always ignore the default value, then it would become impossible to set the field to the default value again after it was modified on the server...

from dotnet-kube-client.

Here's another solution I just came up with:

Generate setters for every field (and backing properties) that will mark the property as dirty when set in a HashSet that can be queried publicly. E.g.

class KubeModel {
	protected HashSet<string> dirtyProperties = new HashSet<string>();
	public IImmutableSet<string> DirtyProperties {
		get { return dirtyProperties; }
	}
}
class DeploymentSpec extends KubeModel {
 	private int progressDeadlineSeconds;
    public int ProgressDeadlineSeconds {
   		get { return progressDeadlineSeconds; }
 		set {
  			progressDeadlineSeconds = value; 	
			dirtyProperties.Add("ProgressDeadlineSeconds");
		}
    }
}

It would be more code but it's generated, so doesn't really matter.
This would not require changing types to nullable and also remove the need for maintaining a map of default values and even non-updateable properties (#29), because those properties would never get set by the YAML deserialisation.
The diff implementation would ignore any properties that are not dirty.

The only other solution I can think of is to deserialise to Dictionaries, then build PSObjects and return those, but "lie" about the OutputType in the Cmdlet to make autocompletion still work. While building PSObjects, the "virtual" type needs to be added as a TypeName to make output formatting work.
Then make the patch cmdlet convert back to dictionaries first, and make the diff implementation "track" the "virtual" model type of the dictionaries by starting at KubeResource and traversing through the Type as its traversing through the dictionaries, so it can retrieve the patch strategies etc from attributes.
The disadvantage is that strictly Get-Kube* cmdlets now return a completely different type than ConvertFrom-KubeYaml.

What do you think?

from dotnet-kube-client.

Interesting - what about just using the YAML document as-is; it only specifies fields that are specified after all :)

This is starting to get a little complex but I'm not against modifying the models in principle (or creating some sort of helper that can do the same). Let me have a think and see what I can do...

from dotnet-kube-client.

What do you mean with as-is? Dictionary? We need to give it some type to serialize into

from dotnet-kube-client.

What if we generate a second set of models in parallel to the first (in a namespace like KubeClient.Models.Tracked) which have the tracking behaviour and implicit / explicit cast operators (or AsTracked() / AsNonTracked()) to convert back and forth? The actual code for tracking changes can be improved using the [CallerMemberName] attribute (or whatever it's called).

from dotnet-kube-client.

Not for the purposes of diff perhaps? Can't remember for yamldotnet but JSON.NET has JObject / Jtoken (a DOM for JSON, effectively). I imagine yamldotnet must have something similar? So I mean don't deserialise at all if you don't need to...

from dotnet-kube-client.

Yep, they have a DOM for YAML:

https://github.com/aaubry/YamlDotNet/wiki/Samples.LoadingAYamlStream

from dotnet-kube-client.

Yeah, but we can't operate on the YAML string ;)
I think you mean what I wrote in my second idea:

The only other solution I can think of is to deserialise to Dictionaries, then build PSObjects and return those, but "lie" about the OutputType in the Cmdlet to make autocompletion still work. While building PSObjects, the "virtual" type needs to be added as a TypeName to make output formatting work.
Then make the patch cmdlet convert back to dictionaries first, and make the diff implementation "track" the "virtual" model type of the dictionaries by starting at KubeResource and traversing through the Type as its traversing through the dictionaries, so it can retrieve the patch strategies etc from attributes.
The disadvantage is that strictly Get-Kube* cmdlets now return a completely different type than ConvertFrom-KubeYaml.

If we go with separate class hierarchies, we'll have the same problem of fragmented types...

from dotnet-kube-client.

Yep, they have a DOM for YAML

I believe the patch implementation shouldn't rely on the encoding being YAML though, so Dictionaries would be the better choice.

from dotnet-kube-client.

If we go with separate class hierarchies, we'll have the same problem of fragmented types

A fair point.

We could, however, change the client so that its methods take / return a JObject/JToken
(or a YamlDocument) instead of specific model types (i.e. no serialisation / deserialisation at all). Clients can then just use JObject.ToObject<TModel>()/JObject.ToObject(modelType) if required.

from dotnet-kube-client.

I'm pretty comfortable with JToken and friends being passed around (it's a reasonably common way to pass untyped or semi-typed data around when that data was received in JSON format in the first place but the schema is not fixed); there's even reasonably high-fidelity conversion between YamlDocument and JSON:

https://github.com/aaubry/YamlDotNet/wiki/Samples.ConvertYamlToJson

from dotnet-kube-client.

Seen this? https://github.com/wbish/jsondiffpatch.net

from dotnet-kube-client.

I agree that's the ideal way to go - but perhaps we might be able to simplify things a bit by keeping the 2 scenarios separate?

1. Consumer wants to load objects, modify them, and then patch.
2. Consumer wants to apply Kube YAML from a file.

For 1, this is a moot point because all fields are already populated; by definition, only fields they change will be changed (and need patch operations generated for them).

For 2, we have a set of keys from the YAML so we know what patch operations are required.

Have I missed anything, or does that more or less cover both scenarios?

from dotnet-kube-client.

(so for 1, just do a property-level diff; all properties will have been populated before modifying)

from dotnet-kube-client.

(as for 2, I know JSON.NET has an operation called Populate but I don't know if YamlDotNet does - populate simply deserialises on top of an existing object instance, only setting members for which serialised properties are present in the JSON - it's super useful)

from dotnet-kube-client.

It's also possible to wrap a given model instance in a PSObject and augment / replace members that way (e.g. by adding change-tracking behaviour) although this may not be the simplest option...

from dotnet-kube-client.

What if ConvertFrom-KubeYaml wrapped each deserialised resource model in a PSObject with an added PSNoteProperty called something like YamlKeys or LoadedYamlMembers? That way, you know which values were present in the Yaml to use as a hint for patching :-)

Mind you, if they load from YAML and then modify the resources before passing to Update-KubeResource then that might not help so much :-(

from dotnet-kube-client.

(with the option above, the consumer always still gets real, strongly-typed models; it's just that ConvertFrom-Yaml adds extra metadata to those deserialised instances to help guide Update-KubeResource)

from dotnet-kube-client.

Here's a quick example (from memory; pretty sure it'll compile but not 100% certain):

class MyModel
{
    public string Foo { get; set; }
}

class MyCmdlet
{
    public override void ProcessRecord()
    {
        MyModel model = new MyModel
        {
            Foo = "bar"
        };

        PSObject result = new PSObject(model);
        result.Properties.Add(
            new PSNoteProperty("Name", "MyModel1")
        );

        WriteObject(result);
    }
}

from dotnet-kube-client.

Intellisense will work on the result of the Cmdlet because it really is a MyModel instance, just wrapped in a PSObject (in fact, from memory all objects are wrapped in PSObject under the covers but I might be misremembering that).

Additionally, if you decorate your Cmdlet class with[OutputType(typeof(MyModel))] or [OutputType(typeof(MyModel), ParameterSet="MyParameterSet")] then consumers will get intellisense before it's even run (e.g. when writing a pipeline).

from dotnet-kube-client.

I have an idea for attaching properties to arbitrary objects (CLR-level, not PSObject-level) - give me an hour or so and I'll post some code :)

from dotnet-kube-client.

Ok, forget that but how about this:

When deserialising from YAML, clone the deserialised model and store it as a PSNoteProperty (called __YamlSource or something like that) on the PSObject-wrapped model written to the pipeline. That way you can compare the model's properties with the original to see which ones have been changed! Not certain but you might even be able to mark that property as hidden...

from dotnet-kube-client.

Can do this in C# to hide that property by default:
http://ramblingcookiemonster.github.io/Decorating-Objects/

from dotnet-kube-client.

Yeah I think adding the noteproperty would work, but how would you read it from a cmdlet that expects KubeResouce as a parameter?

from dotnet-kube-client.

Bah, didn't think of that :)

Maybe have it take a PSObject and use a custom validation attribute to verify the type?

from dotnet-kube-client.

Maybe add a parameter set that takes a different parameter of type PSObject from the pipeline?

from dotnet-kube-client.

You can still get at the wrapped model via the BaseObject property.

from dotnet-kube-client.

That could work, we would just lose the type declaration in Get-Help output with validation at runtime. That might be the trade-off we have to do.

from dotnet-kube-client.

So just to make sure I understand how this works:

1. Get resource spec from YAML.
2. Consumer may modify that resource spec.
3. Get resource spec from K8s.
4. Decide what to do?

What if when we loaded the resource spec from YAML we deserialised it on top of the spec loaded from K8s? Then comparing the 2 might be easier...

from dotnet-kube-client.

(props missing from YAML will be filled in from K8s and they make their changes to that model and when updating K8s all we need to do is load from K8s and compare).

Maybe if ConvertFrom-KubeYaml had a switch to enable filling in of undeclared properties from current K8s state?)

from dotnet-kube-client.

Like this?

Get-Content -Raw deploy.yml | ConvertFrom-KubeYaml -OnTopOfServerState

So that would require ConvertFrom-KubeYaml to have API access, and essentially would take the job of diffing and patching, because to deserialise "on top of" the server object it has to consider all the aspects of merge strategies etc. So really it just means merging cmdlets into one. It would be nice if we could keep some SoC

from dotnet-kube-client.

SoC?

from dotnet-kube-client.

How about a separate Cmdlet to do merge into model from K8s state?

from dotnet-kube-client.

How about a separate Cmdlet to do merge into model from K8s state?

That's Compare-KubeResource. This whole problem is basically "what type should Compare-KubeResource take as a parameter".

from dotnet-kube-client.

Update-KubeModel perhaps?

from dotnet-kube-client.

Ergonomics would suffer if they had to explicitly load from YAML, load from K8s, compare, and update. But if the was another Cmdlet or function that wrapped up using the 3 operations together...

from dotnet-kube-client.

That way you cater to the most common use case but preserve composability.

from dotnet-kube-client.

Ah, I was thinking of segregation of responsibility but yeah now I get you.

from dotnet-kube-client.

Yes, my plan is to have "shortcuts" that compose these together, but still have separate cmdlets under the hood.

Update-KubeModel already exists as (Compare-KubeResource $a $b).ApplyTo($a). But that doesn't solve the problem, because there still needs to be a well-typed way to output the data from ConvertFrom-KubeYaml to Compare-KubeResource.

from dotnet-kube-client.

I think where I am coming from is that if the model they go to modify was already populated from K8s and then yaml then you don't need to care which properties they changed - just diff and go :)

from dotnet-kube-client.

Yes, but unless I'm missing something to "populate it from K8S" you need to diff it

from dotnet-kube-client.

Maybe not? Simply replace all property values with what is specified in the yaml. Then diff and merge only happens when you want to apply...

from dotnet-kube-client.

If you only do that shallow (for top-level properties) then you will e.g. override the whole DeploymentSpec for a Deployment. So you would still have all the problems for nested objects. To do it deep, you need to traverse the object, which can contain lists, like PodSpec.Containers. To merge those, you need to consider the merge strategy, etc. So it's basically the full patch algorithm.

from dotnet-kube-client.

I.e. after you did that, there is no point in then diffing that object again to generate the patch and send it to the server, you could have just generated a patch in the first place instead of merging the objects on the client.

from dotnet-kube-client.

Oh, I was just thinking in terms of SoC. The later Cmdlets still have to deal with merge but at least the load / consumer-modifications steps don't have to care. But perhaps I'm just misunderstanding the problem (it's been a long day 😁)

from dotnet-kube-client.

@felixfbecker - ok, had a think about it overnight and this will be the easiest way to attach arbitrary properties (e.g. Original) to models without having to use PSObject:

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace AttachArbitraryProperties
{
    static class Program
    {
        static void Main()
        {
            Model current;

            current.SetOriginal(
                Clone(current) // e.g. serialise and deserialise, or deserialise twice
            );

            // Later:

            Model original = current.GetOriginal();

            // TODO: Diff
        }
    }

    abstract class Model
    {
        // Whatever
    }

    static class ModelCustomProperties
    {
        static readonly ConditionalWeakTable<object, object> _originals = new ConditionalWeakTable<object, object>();

        public static object GetOriginal(this Model model)
        {
            if (model == null)
                throw new ArgumentNullException(nameof(model));

            if (_originals.TryGetValue(model, out object original))
                return original;

            return null;
        }

        public static void SetOriginal(this Model model, object original)
        {
            if (model == null)
                throw new ArgumentNullException(nameof(model));

            _originals.AddOrUpdate(model, original);
        }
    }
}

Will work as long as it's in the same AppDomain, and the ConditionalWeakTable won't prevent models from being garbage-collected if nothing else is holding on to them.

Docs here:

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2?view=netcore-2.1

from dotnet-kube-client.

Hmm, that'll also not work over remote sessions, right?

from dotnet-kube-client.

No, but it seems (to me at least) to be a sufficiently edge-case scenario that docs may be good enough to indicate that it isn't supported if you load the YAML on a remote system but do the diff on a local system. And you could always add a Cmdlet (e.g. Update-KubeModelSnapshot) that could do the augmentation locally before you do any modifications (with docs indicating that this is unnecessary unless you're doing load remotely but compare locally).

BTW, have you seen platyPS? Pretty easy way to author help for binary Powershell modules :)

from dotnet-kube-client.

I just thought about the approach of saving the raw YAML dictionaries in a PSNoteProperty again, but I realised that won't work - the user may add a field dynamically that was not in the original YAML, but then the new field would still be ignored (unless the user also updates the dictionary, which would not be good UX).

What if we took a look at the separate Models.Tracked namespace approach? I think this could work - for parsing from YAML, I would always use that, when querying from the server, it's okay if the result is not tracked, because those objects always have all default values filled out. I would propose we make the tracked models all inherit from the non-tracked models.

from dotnet-kube-client.

Ok yeah can do - will make properties virtual and override them on tracked models.

from dotnet-kube-client.

Do you need collections tracked too?

from dotnet-kube-client.

No, only model properties.

from dotnet-kube-client.

Ok, easy enough :)

I can get that done sometime on Saturday - will post here when it's ready to try out.

from dotnet-kube-client.

Each tracked model will have a hashset of field names that have been modified. It will be reset during serialisation. Ok?

from dotnet-kube-client.

Sorry, deserialisation

from dotnet-kube-client.

The hashset could also never be reset for the lifetime of the object.

from dotnet-kube-client.

Ok, made a first attempt at this:

https://github.com/tintoy/dotnet-kube-client/tree/feature/tracked-models

CI build in progress so you can try out the packages.

https://travis-ci.org/tintoy/dotnet-kube-client/builds/422831113

Each model in KubeClient.Models.Tracked (providing it has any non-collection properties) has a new property called __ModifiedProperties__. This is a HashSet<string> containing the CLR name of any properties that were modified. I was going to use the JSON / YAML property name but that makes reflection harder than it needs to be.

from dotnet-kube-client.

For example:

https://github.com/tintoy/dotnet-kube-client/blob/feature/tracked-models/src/KubeClient/Models/Tracked/generated/PodV1.cs

from dotnet-kube-client.

Still trying to decide whether to move the tracked models to a separate assembly; this would improve ergonomics for existing users since they won't be confused by 2 models with the same name in different namespaces.

from dotnet-kube-client.

Awesome! What's missing it seems is a way to get the right tracked model type like I can for non-tracked through ModelMetadata

from dotnet-kube-client.

I added that myself on the feature branch.
What I couldn't figure out how to fix is that tracked models are not tracked "deep", i.e. KubeClient.Models.Tracked.DeploymentV1Beta1#Spec is of type KubeClient.Models.DeploymentSpecV1Beta1, not KubeClient.Models.Tracked.DeploymentSpecV1Beta1 as it would need to be.

from dotnet-kube-client.

I can fix that - will have a go sometime today :)

from dotnet-kube-client.

I'll also do a try of serialising into dynamic and see how that approach plays out.

from dotnet-kube-client.

Ok - so if I remember correctly the work for this has been happening in the feature/dynamic-client branch? I'll work on getting that merged so we can start closing off these issues :)

from dotnet-kube-client.

@felixfbecker I think this may have been dealt with as part of the work for #43. If you get any spare time, can you have a look and let me know if it works for you?

from dotnet-kube-client.