From 115630c49f31954bc273bf92e8b0de551d7e2c49 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 12:18:50 +0800 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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) From a59ce034b4d475711939d396d3bf79778adb9ab5 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sat, 14 Feb 2026 14:43:34 +0800 Subject: [PATCH 5/7] fix: clean metadata fields to avoid resourceVersion patch errors Add cleanMetadataForPatch function to remove server-managed metadata fields (resourceVersion, generation, uid, etc.) before creating merge patches. This prevents 'resourceVersion: Invalid value: 0' errors when dry-running patches against CRs/CRDs. Signed-off-by: yxxhero --- manifest/generate.go | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index ad495f15..06364511 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -181,8 +181,23 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // 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) + // Clean metadata fields that shouldn't be compared (they're server-managed) + // This prevents "resourceVersion: Invalid value: 0" errors when dry-running patches + cleanedOldData, err := cleanMetadataForPatch(oldData) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("cleaning old metadata: %w", err) + } + cleanedNewData, err := cleanMetadataForPatch(newData) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("cleaning new metadata: %w", err) + } + cleanedCurrentData, err := cleanMetadataForPatch(currentData) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("cleaning current metadata: %w", err) + } + // Step 1: Create patch from old -> new (what the chart wants to change) - chartChanges, err := jsonpatch.CreateMergePatch(oldData, newData) + chartChanges, err := jsonpatch.CreateMergePatch(cleanedOldData, cleanedNewData) if err != nil { return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err) } @@ -192,14 +207,14 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) if chartChanged { // Step 2: Apply chart changes to current (merge chart changes with live state) - mergedData, err := jsonpatch.MergePatch(currentData, chartChanges) + mergedData, err := jsonpatch.MergePatch(cleanedCurrentData, 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) + patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, mergedData) return patch, types.MergePatchType, err } @@ -207,7 +222,7 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // 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) + patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, cleanedNewData) return patch, types.MergePatchType, err } @@ -224,6 +239,26 @@ func isPatchEmpty(patch []byte) bool { return len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" } +func cleanMetadataForPatch(data []byte) ([]byte, error) { + var objMap map[string]interface{} + if err := json.Unmarshal(data, &objMap); err != nil { + return nil, err + } + + if metadata, ok := objMap["metadata"].(map[string]interface{}); ok { + delete(metadata, "resourceVersion") + delete(metadata, "generation") + delete(metadata, "creationTimestamp") + delete(metadata, "uid") + delete(metadata, "managedFields") + delete(metadata, "selfLink") + } + + delete(objMap, "status") + + return json.Marshal(objMap) +} + 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) From 5d14d97ad64f31c46512ad1b79eb451d7c9a8310 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sat, 14 Feb 2026 15:01:21 +0800 Subject: [PATCH 6/7] fix: preserve live-only fields in three-way merge for unstructured objects - Change drift detection to apply new onto current (preserves live-only fields) - Align cleanMetadataForPatch with deleteStatusAndTidyMetadata (add helm annotations) - Update test case: manual annotations should be preserved, not cause a diff This ensures three-way merge behavior matches strategic merge patch: - Live-only fields (e.g., manual annotations) are preserved - Drift on chart-owned fields (e.g., spec fields) is still detected Signed-off-by: yxxhero --- manifest/generate.go | 37 ++++++++++++++++++++++++++----------- manifest/generate_test.go | 4 ++-- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index 06364511..a7c5fcfd 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 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) + // 5. If chart unchanged (old == new), build desired state by applying new onto current + // (preserving live-only fields), then diff current -> desired to detect drift // Clean metadata fields that shouldn't be compared (they're server-managed) // This prevents "resourceVersion: Invalid value: 0" errors when dry-running patches @@ -219,10 +219,14 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) } // 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(cleanedCurrentData, cleanedNewData) + // 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) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("building desired state: %w", err) + } + patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, desiredData) return patch, types.MergePatchType, err } @@ -245,16 +249,27 @@ func cleanMetadataForPatch(data []byte) ([]byte, error) { return nil, err } + delete(objMap, "status") + if metadata, ok := objMap["metadata"].(map[string]interface{}); ok { - delete(metadata, "resourceVersion") + delete(metadata, "managedFields") delete(metadata, "generation") delete(metadata, "creationTimestamp") + delete(metadata, "resourceVersion") delete(metadata, "uid") - delete(metadata, "managedFields") - delete(metadata, "selfLink") - } - delete(objMap, "status") + // Remove helm-related and k8s annotations that shouldn't be compared + if a := metadata["annotations"]; a != nil { + annotations := a.(map[string]interface{}) + delete(annotations, "meta.helm.sh/release-name") + delete(annotations, "meta.helm.sh/release-namespace") + delete(annotations, "deployment.kubernetes.io/revision") + + if len(annotations) == 0 { + delete(metadata, "annotations") + } + } + } return json.Marshal(objMap) } diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 7f7e09bb..82bf9726 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -76,8 +76,8 @@ func TestCreatePatchForUnstructured(t *testing.T) { }, }, }, - expectedChange: true, - description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)", + expectedChange: false, + description: "Manual annotation not present in chart should be preserved; no effective change expected from three-way merge", }, { name: "CR with no manual changes", From f5b13c210517e9f509751569767ecc50549b5048 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sat, 14 Feb 2026 15:21:56 +0800 Subject: [PATCH 7/7] refactor: reuse deleteStatusAndTidyMetadata in cleanMetadataForPatch - Refactor cleanMetadataForPatch to call deleteStatusAndTidyMetadata to avoid code duplication and ensure consistent metadata handling - Add TestCreatePatchForCRD test case to cover the CRD type branch (apiextv1.CustomResourceDefinition path) Signed-off-by: yxxhero --- manifest/generate.go | 27 +--------- manifest/generate_test.go | 100 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index a7c5fcfd..d785798d 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -244,33 +244,10 @@ func isPatchEmpty(patch []byte) bool { } func cleanMetadataForPatch(data []byte) ([]byte, error) { - var objMap map[string]interface{} - if err := json.Unmarshal(data, &objMap); err != nil { + objMap, err := deleteStatusAndTidyMetadata(data) + if err != nil { return nil, err } - - delete(objMap, "status") - - if metadata, ok := objMap["metadata"].(map[string]interface{}); ok { - delete(metadata, "managedFields") - delete(metadata, "generation") - delete(metadata, "creationTimestamp") - delete(metadata, "resourceVersion") - delete(metadata, "uid") - - // Remove helm-related and k8s annotations that shouldn't be compared - if a := metadata["annotations"]; a != nil { - annotations := a.(map[string]interface{}) - delete(annotations, "meta.helm.sh/release-name") - delete(annotations, "meta.helm.sh/release-namespace") - delete(annotations, "deployment.kubernetes.io/revision") - - if len(annotations) == 0 { - delete(metadata, "annotations") - } - } - } - return json.Marshal(objMap) } diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 82bf9726..d5c4611a 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -4,7 +4,9 @@ import ( "testing" "github.com/stretchr/testify/require" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -318,3 +320,101 @@ func TestCreatePatchForUnstructured(t *testing.T) { }) } } + +// TestCreatePatchForCRD tests the three-way merge implementation for CRD objects. +// This ensures the isCRD branch in createPatch is properly covered. +func TestCreatePatchForCRD(t *testing.T) { + originalCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + currentCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + targetCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + target := &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "apiextensions.k8s.io", + Version: "v1", + Kind: "CustomResourceDefinition", + }, + }, + Object: targetCRD, + } + + patch, patchType, err := createPatch(originalCRD, currentCRD, target) + require.NoError(t, err, "createPatch should not return an error for CRD") + require.Equal(t, types.MergePatchType, patchType, "patch type should be MergePatchType for CRD objects") + + t.Logf("CRD Patch result: %s", string(patch)) + + require.Equal(t, "{}", string(patch), "CRD with no changes should result in empty patch") +}