Serialization sets invalid default values

[bastianeicher]

Assume the following deployment:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test
spec:
  selector:
    matchLabels:
      app: test
  template:
    metadata:
      labels:
        app: test
    spec:
      containers:
        - name: test
          image: nginx

Creating this with kubectl apply succeeds.

Trying to do the same with KubeClient fails with an "unprocessable entity" error:

var deployment = new DeploymentV1Beta1
{
    Metadata = new ObjectMetaV1 {Name = "test"},
    Spec = new DeploymentSpecV1Beta1
    {
        Selector = new LabelSelectorV1
        {
            MatchLabels = {["app"] = "test"}
        },
        Template = new PodTemplateSpecV1
        {
            Metadata = new ObjectMetaV1
            {
                Labels = {["app"] = "test"}
            },
            Spec = new PodSpecV1
            {
                Containers =
                {
                    new ContainerV1
                    {
                        Name = "test",
                        Image = "nginx"
                    }
                }
            }
        }
    }
}));
client.Create(deployment);

Serializing the deployment object to JSON with JsonConvert.SerializeObject(deployment) and then converting it to YAML for better readability gives the following (slightly abbreviated):

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test
  uid: 
  clusterName: 
  generateName: 
  # ...
spec:
  paused: false
  selector:
    matchExpressions: []
    matchLabels:
      app: test
  template:
    metadata:
      labels:
        app: test
      uid: 
      clusterName: 
      generateName: 
      name: 
      # ...
    spec:
      hostIPC: false
      hostPID: false
      hostname: 
      nodeName: 
      # ...
      containers:
      - name: test
        image: nginx
        command: []
        lifecycle: 
        livenessProbe:        
        readinessProbe: 
        # ...
      hostAliases: []
      imagePullSecrets: []
      initContainers: []
      readinessGates: []
      # ...
  rollbackTo: 
  minReadySeconds: 0
  progressDeadlineSeconds: 0
  replicas: 0
  revisionHistoryLimit: 0
  strategy: 
status: 

Trying this with kubectl apply fails with: unknown field "readinessGates" in io.k8s.api.core.v1.PodSpec After removing that field it fails with: spec.progressDeadlineSeconds: Invalid value: 0: must be greater than minReadySeconds.

I suppose the underlying problem is, that fields like DeploymentV1.ProgressDeadlineSeconds have documentation like Defaults to 600s. but these default values are not represented in code and the fields in question are not nullable.

[tintoy]

Yeah I'm thinking these should be nullable :)

Let me see what I can do.

[tintoy]

What version of the K8s are you using, by the way? The field readinessGates does appear in the YAML for PodSpecV1:

io.k8s.api.core.v1.PodSpec:
  description: PodSpec is a description of a pod.
  properties:
    # ...<snip>...
    containers:
      description: List of containers belonging to the pod. Containers cannot currently
        be added or removed. There must be at least one container in a Pod. Cannot
        be updated.
      items:
        $ref: '#/definitions/io.k8s.api.core.v1.Container'
      type: array
      x-kubernetes-patch-merge-key: name
      x-kubernetes-patch-strategy: merge
    readinessGates:
      description: 'If specified, all readiness gates will be evaluated for pod
        readiness. A pod is ready when all its containers are ready AND all conditions
        specified in the readiness gates have status equal to "True" More info:
        https://github.com/kubernetes/community/blob/master/keps/sig-network/0007-pod-ready%2B%2B.md'
      items:
        $ref: '#/definitions/io.k8s.api.core.v1.PodReadinessGate'
      type: array
    # ...<snip>...
    terminationGracePeriodSeconds:
      description: Optional duration in seconds the pod needs to terminate gracefully.
        May be decreased in delete request. Value must be non-negative integer.
        The value zero indicates delete immediately. If this value is nil, the default
        grace period will be used instead. The grace period is the duration in seconds
        after the processes running in the pod are sent a termination signal and
        the time when the processes are forcibly halted with a kill signal. Set
        this value longer than the expected cleanup time for your process. Defaults
        to 30 seconds.
      format: int64
      type: integer
    tolerations:
      description: If specified, the pod's tolerations.
      items:
        $ref: '#/definitions/io.k8s.api.core.v1.Toleration'
      type: array
    volumes:
      description: 'List of volumes that can be mounted by containers belonging
        to the pod. More info: https://kubernetes.io/docs/concepts/storage/volumes'
      items:
        $ref: '#/definitions/io.k8s.api.core.v1.Volume'
      type: array
      x-kubernetes-patch-merge-key: name
      x-kubernetes-patch-strategy: merge,retainKeys
  required:
  - containers

[tintoy]

I think the broader problem is that we don't currently respect the required property in the OpenAPI YAML which would tell us which properties can be null (i.e. any not specified in required). But that's fixable, I think.

[tintoy]

One way to fix this is detailed in https://github.com/tintoy/dotnet-kube-client/commit/0e9021d741169948762db4e1e1831ec45c695824 and https://github.com/tintoy/dotnet-kube-client/commit/2eb875683c23d59786e46acfc33d28f05a5c8be5, but this would be a breaking change for existing consumers. Will have a think about this overnight to see if I can come up with a less disruptive option :)

[tintoy]

Still trying out ideas (sorry for the delay).

[bastianeicher]

No worries, better to get it right than a quick-fix that breaks stuff. 👍

[tintoy]

I am thinking that, for list (array) properties, they'll be read-only if they're mandatory, but read/write (and null by default) if they're optional. The ergonomics are a bit...meh... but it's a compromise. What do you think?

CC: @felixfbecker - is this likely to mess you up at all? I imagine it might affect resource diffs, but at least you'd know if something is null then they probably didn't specify it...

[tintoy]

For list / dict properties, an alternative approach would be to use a custom JsonConverter that treats an empty collection as null (and, consequently, omits it from the resulting JSON).

[tintoy]

(that way the collection properties themselves are never null, preserving the current client-side behaviour)

[tintoy]

Ok, we now generate a ShouldSerializeXXX method for all optional collection properties which ensures that they are not serialised if the collection is empty (see https://github.com/tintoy/dotnet-kube-client/commit/99af4b729b2692cc84ee791092bd3a7931db150a for details).

This does make it hard to empty out a collection property while updating a resource but, given that the K8s API is kinda braindead anyway when it comes to updates (you more or less have to use JSON-patch rather than a PUT to accomplish most useful modifications) I think we could live with this behaviour.

What do you think?

[bastianeicher]

Sounds good to me. I also much prefer not having to initialize collection properties.

Would it maybe make sense to pre-initialize required, non-collection properties (such as Spec) as well? This could enable nice and concise code like this:

var deployment = new DeploymentV1
{
    Metadata = {Name = "test"},
    Spec =
    {
        Template =
        {
            Metadata =
            {
                Labels = {["app"] = "test"}
            },
            Spec =
            {
                Containers =
                {
                    new ContainerV1
                    {
                        Name = "test",
                        Image = "nginx"
                    }
                }
            }
        }
    }
}));

[tintoy]

For sure - all collection properties are read-only now. What I also wound up doing is also generating a ShouldSerializeXXX() method for optional collection properties - if collection is empty, it's omitted :)

[tintoy]

Ok, this should be resolved in the latest release @bastianeicher :)

Would you mind giving it a try to see if it produces the results you'd expect?

[bastianeicher]

Jep, works perfectly now. Thanks. :)