diff --git a/manifest/generate.go b/manifest/generate.go index e6e8ab8a..cf3731a7 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -639,7 +639,15 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // from the chart state on chart-owned fields. // Build desired state by applying new onto current (preserves live-only additions), // then diff current -> desired to detect drift on chart-owned fields. - desiredData, err := jsonpatch.MergePatch(cleanedCurrentData, cleanedNewData) + // A merge patch reads a null as "delete this key", but a null the chart + // renders here is in both manifests, so the upgrade sends nothing for it + // and leaves the live value as it is. Drop the chart's nulls first, so + // they do not read as drift. + desiredPatch, err := dropNulls(cleanedNewData) + if err != nil { + return nil, fmt.Errorf("dropping nulls from new: %w", err) + } + desiredData, err := jsonpatch.MergePatch(cleanedCurrentData, desiredPatch) if err != nil { return nil, fmt.Errorf("building desired state: %w", err) } @@ -665,6 +673,31 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return patch, nil } +// dropNulls removes every null-valued key from the JSON object in data, at any +// depth of nested objects. A list is left as it is, since a merge patch +// replaces a list whole. +func dropNulls(data []byte) ([]byte, error) { + var obj interface{} + if err := json.Unmarshal(data, &obj); err != nil { + return nil, err + } + return json.Marshal(dropNullValues(obj)) +} + +func dropNullValues(v interface{}) interface{} { + switch t := v.(type) { + case map[string]interface{}: + for k, val := range t { + if val == nil { + delete(t, k) + continue + } + t[k] = dropNullValues(val) + } + } + return v +} + func isPatchEmpty(patch []byte) bool { return len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" } diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 6d55930d..99367087 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -1235,6 +1235,118 @@ func TestCreatePatchForUnstructured(t *testing.T) { expectedChange: true, description: "Chart explicitly changes maxReplicaCount from 10 to 20, overriding manual value of 30", }, + { + name: "CR whose chart renders explicit nulls the live object stores (chart doesn't change anything)", + original: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": nil, + }, + }, + }, + current: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": nil, + }, + }, + }, + target: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "keda.sh", + Version: "v1alpha1", + Kind: "ScaledObject", + }, + }, + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": nil, + }, + }, + }, + }, + expectedChange: false, + description: "A null in both manifests, also stored live, is not drift", + }, + { + name: "CR whose chart renders a null where the live object has a value (chart doesn't change anything)", + original: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": nil, + }, + }, + }, + current: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": map[string]interface{}{"replicas": 2}, + }, + }, + }, + target: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "keda.sh", + Version: "v1alpha1", + Kind: "ScaledObject", + }, + }, + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + "fallback": nil, + }, + }, + }, + }, + expectedChange: false, + description: "A null the chart renders leaves the field unset, so the upgrade keeps the live value and there is no drift", + }, } for _, tt := range tests {