fix: do not read a CR's chart-rendered nulls as drift on the three-way merge

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"
 }

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 {