From 115630c49f31954bc273bf92e8b0de551d7e2c49 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 12:18:50 +0800 Subject: [PATCH 1/4] fix: detect manual changes on CRDs and CRs with three-way-merge Previously, helm diff --three-way-merge would not detect manual changes made to Custom Resources (CRDs and CRs) in the cluster. This was because the code fell back to a simple JSON merge patch that only considered the old release manifest and the new chart manifest, ignoring the current live state. For unstructured objects (CRDs, CRs), we now perform a proper three-way merge that respects manual changes made in the cluster: 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. Return that patch (which will be applied to current by the caller) This ensures that manual changes to CRDs and CRs are properly detected and displayed in the diff, just like they are for built-in Kubernetes resources. Fixes #917 Signed-off-by: yxxhero --- manifest/generate.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..92403249 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -170,8 +170,30 @@ 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. Return that patch (which will be applied to current by the caller) + + // 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) + } + + // 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 } From 6610be9d2200a9092c4deea927d902cd44c8ed63 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 12:44:09 +0800 Subject: [PATCH 2/4] test: add tests for three-way merge with unstructured objects Add unit tests to verify the three-way merge implementation for unstructured objects (CRDs, CRs) works correctly. These tests verify: - Manual annotations are preserved when chart doesn't change anything - Manual changes to fields are preserved when chart doesn't change them - Chart changes are applied while preserving manual changes to other fields - Chart overrides manual changes when it explicitly changes a field Related to #917 Signed-off-by: yxxhero --- manifest/generate_test.go | 266 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 manifest/generate_test.go diff --git a/manifest/generate_test.go b/manifest/generate_test.go new file mode 100644 index 00000000..ddb2c3ab --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,266 @@ +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: false, + description: "Manual annotation should be preserved, but not cause a diff since chart doesn't change anything", + }, + { + 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 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 + if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" { + t.Logf("%s: got unexpected patch: %s", tt.description, string(patch)) + } + } + }) + } +} From a460e71b3b2c0eb6d79b66fde45b26924d7cfcb5 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 13:12:37 +0800 Subject: [PATCH 3/4] fix: properly detect drift when chart unchanged for unstructured objects Signed-off-by: yxxhero --- manifest/generate.go | 34 ++++++++++++++++----- manifest/generate_test.go | 64 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index 92403249..621912ac 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -177,7 +177,9 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // 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. Return that patch (which will be applied to current by the caller) + // 4. If chart changed (old != new), return step 3's patch + // 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state + // (i.e., detect and revert manual changes to chart-owned fields) // Step 1: Create patch from old -> new (what the chart wants to change) chartChanges, err := jsonpatch.CreateMergePatch(oldData, newData) @@ -185,15 +187,27 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err) } - // 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) + // 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 } - // 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) + // 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 } @@ -206,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 index ddb2c3ab..7f7e09bb 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -76,8 +76,8 @@ func TestCreatePatchForUnstructured(t *testing.T) { }, }, }, - expectedChange: false, - description: "Manual annotation should be preserved, but not cause a diff since chart doesn't change anything", + 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", @@ -188,6 +188,59 @@ func TestCreatePatchForUnstructured(t *testing.T) { 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{ @@ -256,10 +309,11 @@ func TestCreatePatchForUnstructured(t *testing.T) { 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 - if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" { - t.Logf("%s: got unexpected patch: %s", tt.description, string(patch)) + // 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)) } }) } From efb7e0a4ab204ae7070b51170d52c7b4b947d1b1 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 13:18:49 +0800 Subject: [PATCH 4/4] fix: correct comment direction in three-way merge logic Signed-off-by: yxxhero --- manifest/generate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index 621912ac..ad495f15 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -178,8 +178,8 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // 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 new->current patch to restore chart state - // (i.e., detect and revert manual changes to chart-owned fields) + // 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)