Deployment spec properties are inadvertently removed when serializing

[whatsacomputertho]

Summary

Various valid deployment spec properties are seemingly wiped out when I load it into a V1DeploymentSpec object, make a small change, then re-serialize to a dict.

Details

I am extracting a V1DeploymentSpec from a ClusterServiceVersion manifest, making a small mutation to it, then re-serializing it and adding it back into the ClusterServiceVersion. This leads to multiple mutations that were not intended. See the following sequence of diffs (none of which were intended) for reference:

@@ -356,287 +355,126 @@

-          selector:
-            matchLabels:
-              name: my-operator
+          selector: {}

...

             spec:
-              affinity:
-                nodeAffinity:
-                  requiredDuringSchedulingIgnoredDuringExecution:
-                    nodeSelectorTerms:
-                    - matchExpressions:
-                      - key: kubernetes.io/arch
-                        operator: In
-                        values:
-                        - s390x
-                        - amd64
+              affinity: {}

...

                 - name: WATCH_NAMESPACE
-                  valueFrom:
-                    fieldRef:
-                      fieldPath: metadata.annotations['olm.targetNamespaces']
                 - name: POD_NAME
-                  valueFrom:
-                    fieldRef:
-                      fieldPath: metadata.name

...

-                imagePullPolicy: Always
-                livenessProbe:
-                  failureThreshold: 4
-                  httpGet:
-                    path: /healthz
-                    port: 8081
-                  initialDelaySeconds: 15
-                  periodSeconds: 15
-                  successThreshold: 1
-                  timeoutSeconds: 3
-                readinessProbe:
-                  failureThreshold: 3
-                  httpGet:
-                    path: /healthz
-                    port: 8081
-                  initialDelaySeconds: 15
-                  periodSeconds: 15
-                  successThreshold: 1
-                  timeoutSeconds: 3

[roycaihw]

/help

[k8s-ci-robot]

@roycaihw:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

Details

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[RehanK-123]

I would like to take up this issue.

[RehanK-123]

@whatsacomputertho
What i understand from the description is that you are trying to load the specs from ClusterServiceVersion manifest and making small mutations, but with those mutations other unexpected mutations happen, could it be due to you loading the mutations to another V1DeploymentSpec object, which then reinitializes the other states of the objects to the state that they were defined as? such as with selector : {}?

[LeonxLJX]

Hi! I'd like to work on this issue. Please let me know if I can be assigned. Thanks!

[LeonxLJX]

Hi! I'd like to fix Deployment spec properties being inadvertently removed when serializing. Plan: reproduce, fix the serialization, add a test. May I be assigned?

[chala2001]

I tried to reproduce this on master (36.0.0+snapshot, models generated from release-1.36) and the properties survive the round trip.

The manifest and the code aren't in the description, so I rebuilt the spec from the diff above, keeping the fields that were wiped there: selector.matchLabels, affinity.nodeAffinity, env[].valueFrom, imagePullPolicy and livenessProbe.

import json
from kubernetes.client import ApiClient
from kubernetes.client.models.v1_deployment_spec import V1DeploymentSpec

spec = {
    "selector": {"matchLabels": {"name": "my-operator"}},
    "template": {
        "spec": {
            "affinity": {"nodeAffinity": {"requiredDuringSchedulingIgnoredDuringExecution": {
                "nodeSelectorTerms": [{"matchExpressions": [
                    {"key": "kubernetes.io/arch", "operator": "In", "values": ["s390x", "amd64"]}]}]}}},
            "containers": [{
                "name": "op",
                "image": "img:1",
                "imagePullPolicy": "Always",
                "env": [{"name": "WATCH_NAMESPACE", "valueFrom": {
                    "fieldRef": {"fieldPath": "metadata.annotations['olm.targetNamespaces']"}}}],
                "livenessProbe": {"httpGet": {"path": "/healthz", "port": 8081},
                                  "initialDelaySeconds": 15, "failureThreshold": 4},
            }],
        }
    },
}

obj = V1DeploymentSpec.from_dict(spec)
print(ApiClient().sanitize_for_serialization(obj) == spec)

That prints True. I get the same result loading through ApiClient().deserialize(json.dumps(spec), "V1DeploymentSpec", "application/json") and through to_dict() fed back into from_dict(). I also checked 35.0.0 from PyPI, where the equivalent load is ApiClient()._ApiClient__deserialize(spec, "V1DeploymentSpec"), and nothing is dropped there either.

@whatsacomputertho which version were you on, and how were you serializing the spec back into the CSV? One thing worth ruling out: to_dict() returns snake_case keys (image_pull_policy, match_labels) and keeps unset fields as None, so it is not the right thing to write back into a manifest, but it would not drop those properties either. If you still have the CSV and the load/mutate/dump snippet, that would pin it down. Otherwise this may already be fixed by the move to the pydantic models, where each model's from_dict reads the camelCase names.

@LeonxLJX not trying to take this off you, just posting what I found while reading the issue.

[Yash121l]

/assign

[Yash121l]

I reproduced this. The trigger is to_dict(): it returns snake_case keys by default (match_labels, image_pull_policy, value_from), while ApiClient deserialization matches only the camelCase names in attribute_map and silently ignores anything else. So any load, to_dict(), reload cycle drops exactly the multi-word fields and keeps the single-word ones, which is the diff in the description: selector: {}, affinity: {}, no valueFrom, no imagePullPolicy, no livenessProbe.

On 36.0.3:

ac = ApiClient()
obj = ac._ApiClient__deserialize(spec, "V1DeploymentSpec")
back = ac._ApiClient__deserialize(obj.to_dict(), "V1DeploymentSpec")
ac.sanitize_for_serialization(back) == spec  # False, selector == {}

Using obj.to_dict(serialize=True) or ApiClient().sanitize_for_serialization(obj) instead round-trips cleanly on 36.0.3.

This is already fixed on master by #2631: the pydantic models use validation_alias=AliasChoices("matchLabels", "match_labels"), so both spellings deserialize. The same script returns True on 37.0.0a1 and on master, and False on 34.1.0, 35.0.0, 36.0.0 and 36.0.3. That also explains why @chala2001 could not reproduce with camelCase input, which was never broken.

I would suggest closing this once 37.0.0 ships. If you want a regression test pinned for the snake_case round trip, I will add one under kubernetes/base, since kubernetes/test is generator output.