diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..ad495f15 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -170,8 +170,44 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) _, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition) if isUnstructured || isCRD { - // fall back to generic JSON merge patch - patch, err := jsonpatch.CreateMergePatch(oldData, newData) + // For unstructured objects (CRDs, CRs), we need to perform a three-way merge + // to detect manual changes made in the cluster. + // + // The approach is: + // 1. Create a patch from old -> new (chart changes) + // 2. Apply this patch to current (live state with manual changes) + // 3. Create a patch from current -> merged result + // 4. If chart changed (old != new), return step 3's patch + // 5. If chart unchanged (old == new) but current != new, return current->new patch applied to current to restore chart state + // (i.e., detect and revert manual changes by restoring chart-owned fields to the chart's desired state) + + // Step 1: Create patch from old -> new (what the chart wants to change) + chartChanges, err := jsonpatch.CreateMergePatch(oldData, newData) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err) + } + + // Check if chart actually changed anything + chartChanged := !isPatchEmpty(chartChanges) + + if chartChanged { + // Step 2: Apply chart changes to current (merge chart changes with live state) + mergedData, err := jsonpatch.MergePatch(currentData, chartChanges) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("applying chart changes to current: %w", err) + } + + // Step 3: Create patch from current -> merged (what to apply to current) + // This patch, when applied to current, will produce the merged result + patch, err := jsonpatch.CreateMergePatch(currentData, mergedData) + return patch, types.MergePatchType, err + } + + // Chart didn't change (old == new), but we need to detect if current diverges + // from the chart state. This is the case where manual changes were made to + // chart-owned fields. + // Create a patch from current -> new to detect and restore drift + patch, err := jsonpatch.CreateMergePatch(currentData, newData) return patch, types.MergePatchType, err } @@ -184,6 +220,10 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return patch, types.StrategicMergePatchType, err } +func isPatchEmpty(patch []byte) bool { + return len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" +} + func objectKey(r *resource.Info) string { gvk := r.Object.GetObjectKind().GroupVersionKind() return fmt.Sprintf("%s/%s/%s/%s", gvk.GroupVersion().String(), gvk.Kind, r.Namespace, r.Name) diff --git a/manifest/generate_test.go b/manifest/generate_test.go new file mode 100644 index 00000000..7f7e09bb --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,320 @@ +package manifest + +import ( + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/cli-runtime/pkg/resource" +) + +// TestCreatePatchForUnstructured tests the three-way merge implementation for unstructured objects (CRDs, CRs). +// This tests the fix for issue #917 where manual changes to CRs were not being detected. +func TestCreatePatchForUnstructured(t *testing.T) { + tests := []struct { + name string + original runtime.Object + current runtime.Object + target *resource.Info + expectedChange bool + description string + }{ + { + name: "CR with manual annotation in current (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, + }, + }, + }, + current: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": "test-so", + "namespace": "default", + "annotations": map[string]interface{}{ + "manual-change": "true", + }, + }, + "spec": map[string]interface{}{ + "maxReplicaCount": 10, + }, + }, + }, + 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, + }, + }, + }, + }, + expectedChange: true, + description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)", + }, + { + name: "CR with no manual changes", + 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, + }, + }, + }, + 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, + }, + }, + }, + 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, + }, + }, + }, + }, + expectedChange: false, + description: "No changes should result in empty patch", + }, + { + name: "CR with chart change and manual change on different fields", + 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, + "minReplicaCount": 1, + }, + }, + }, + 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": 30, + "minReplicaCount": 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": 20, + "minReplicaCount": 1, + }, + }, + }, + }, + expectedChange: true, + description: "Chart changes maxReplicaCount, manual changes minReplicaCount - should merge", + }, + { + name: "CR with field unchanged in chart but modified in live state (issue #917)", + 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, + }, + }, + }, + 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": 30, + }, + }, + }, + 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, + }, + }, + }, + }, + expectedChange: true, + description: "Field maxReplicaCount is 10 in both original and target, but 30 in current - should detect drift", + }, + { + name: "CR with chart overriding manual change", + 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, + }, + }, + }, + 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": 30, + }, + }, + }, + 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": 20, + }, + }, + }, + }, + expectedChange: true, + description: "Chart explicitly changes maxReplicaCount from 10 to 20, overriding manual value of 30", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + patch, patchType, err := createPatch(tt.original, tt.current, tt.target) + require.NoError(t, err, "createPatch should not return an error") + require.Equal(t, types.MergePatchType, patchType, "patch type should be MergePatchType for unstructured objects") + + t.Logf("Patch result: %s", string(patch)) + + if tt.expectedChange { + require.NotEmpty(t, patch, tt.description+": expected patch to detect changes, got empty patch") + require.NotEqual(t, []byte("{}"), patch, tt.description+": expected patch to detect changes, got empty patch") + require.NotEqual(t, []byte("null"), patch, tt.description+": expected patch to detect changes, got null patch") + } else { + // No changes expected: patch must be empty or effectively empty ("{}" or "null") + if len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" { + return + } + require.Failf(t, tt.description+": expected no changes, got unexpected patch", "unexpected patch: %s", string(patch)) + } + }) + } +}