From 115630c49f31954bc273bf92e8b0de551d7e2c49 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 1 Feb 2026 12:18:50 +0800 Subject: [PATCH 01/13] 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 02/13] 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 03/13] 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 04/13] 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 05/13] 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 06/13] 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 07/13] 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") +} From 2ff4b31f7bf93c9f5414ca7f2fd4f10be664cf93 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 15 Feb 2026 16:52:07 +0800 Subject: [PATCH 08/13] fix: handle null input in deleteStatusAndTidyMetadata - Return nil without panic when JSON input is null - Safely handle missing metadata field with type assertion - Add test case for null JSON input Signed-off-by: yxxhero --- manifest/generate.go | 3 +++ manifest/util.go | 17 +++++++++++------ manifest/util_test.go | 24 +++++++++++++++--------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index d785798d..add0a7a5 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -248,6 +248,9 @@ func cleanMetadataForPatch(data []byte) ([]byte, error) { if err != nil { return nil, err } + if objMap == nil { + return []byte("null"), nil + } return json.Marshal(objMap) } diff --git a/manifest/util.go b/manifest/util.go index 88b818af..9817349a 100644 --- a/manifest/util.go +++ b/manifest/util.go @@ -6,16 +6,23 @@ import ( jsoniter "github.com/json-iterator/go" ) -func deleteStatusAndTidyMetadata(obj []byte) (map[string]interface{}, error) { - var objectMap map[string]interface{} +func deleteStatusAndTidyMetadata(obj []byte) (map[string]any, error) { + var objectMap map[string]any err := jsoniter.Unmarshal(obj, &objectMap) if err != nil { return nil, fmt.Errorf("could not unmarshal byte sequence: %w", err) } + if objectMap == nil { + return nil, nil + } + delete(objectMap, "status") - metadata := objectMap["metadata"].(map[string]interface{}) + metadata, ok := objectMap["metadata"].(map[string]any) + if !ok { + return objectMap, nil + } delete(metadata, "managedFields") delete(metadata, "generation") @@ -23,10 +30,8 @@ func deleteStatusAndTidyMetadata(obj []byte) (map[string]interface{}, error) { delete(metadata, "resourceVersion") delete(metadata, "uid") - // See the below for the goal of this metadata tidy logic. - // https://github.com/databus23/helm-diff/issues/326#issuecomment-1008253274 if a := metadata["annotations"]; a != nil { - annotations := a.(map[string]interface{}) + annotations := a.(map[string]any) delete(annotations, "meta.helm.sh/release-name") delete(annotations, "meta.helm.sh/release-namespace") delete(annotations, "deployment.kubernetes.io/revision") diff --git a/manifest/util_test.go b/manifest/util_test.go index 0f86223a..3b844a1d 100644 --- a/manifest/util_test.go +++ b/manifest/util_test.go @@ -10,7 +10,7 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { tests := []struct { name string obj []byte - want map[string]interface{} + want map[string]any wantErr bool }{ { @@ -19,6 +19,12 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { want: nil, wantErr: true, }, + { + name: "null json", + obj: []byte("null"), + want: nil, + wantErr: false, + }, { name: "valid json", obj: []byte(` @@ -57,21 +63,21 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { } } `), - want: map[string]interface{}{ + want: map[string]any{ "apiVersion": "apps/v1", "kind": "Deployment", - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ + "metadata": map[string]any{ + "annotations": map[string]any{ "other-annot": "value", }, "name": "nginx-deployment", "namespace": "test-ns", }, - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "containers": []interface{}{ - map[string]interface{}{ + "spec": map[string]any{ + "template": map[string]any{ + "spec": map[string]any{ + "containers": []any{ + map[string]any{ "image": "nginx:1.14.2", "imagePullPolicy": "IfNotPresent", "name": "nginx", From 1076d7d1b26e958329e689d4bf23bd9c6a76dd1c Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 15 Feb 2026 17:10:38 +0800 Subject: [PATCH 09/13] fix: improve error handling and type safety - Wrap errors with context in createPatch for better debugging - Use safe type assertion for annotations in deleteStatusAndTidyMetadata Signed-off-by: yxxhero --- manifest/generate.go | 10 ++++++++-- manifest/util.go | 15 ++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/manifest/generate.go b/manifest/generate.go index add0a7a5..9318fe4c 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -215,7 +215,10 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) // 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(cleanedCurrentData, mergedData) - return patch, types.MergePatchType, err + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("creating patch from current to merged: %w", err) + } + return patch, types.MergePatchType, nil } // Chart didn't change (old == new), but we need to detect if current diverges @@ -227,7 +230,10 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return nil, types.MergePatchType, fmt.Errorf("building desired state: %w", err) } patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, desiredData) - return patch, types.MergePatchType, err + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("creating patch from current to desired: %w", err) + } + return patch, types.MergePatchType, nil } patchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject) diff --git a/manifest/util.go b/manifest/util.go index 9817349a..3ee3bbc9 100644 --- a/manifest/util.go +++ b/manifest/util.go @@ -31,13 +31,14 @@ func deleteStatusAndTidyMetadata(obj []byte) (map[string]any, error) { delete(metadata, "uid") if a := metadata["annotations"]; a != nil { - annotations := a.(map[string]any) - 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") + if annotations, ok := a.(map[string]any); ok { + 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") + } } } From 548fcc6813debd271ae994270649916669a20df1 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Fri, 25 Sep 2026 13:45:05 +0800 Subject: [PATCH 10/13] test: add e2e regression test for CR/CRD drift detection (#917) Add scripts/issues/917.sh, picked up automatically by the integration-tests CI job (kind cluster, Helm v3.18.6/v3.22.0/v4.3.0 matrix) through scripts/issues/run.sh. Scenarios: - A: --three-way-merge of a clean unchanged release reports no drift - B: manual change on a CR field the chart owns is detected, also with --three-way-merge-mode client (local merge, no patch permissions), and stays invisible to a plain diff - C: a chart change is merged with the manual change, not masked by it - D: manual change on the CRD itself is detected - all: no error output and no resourceVersion diff entries (the dry-run used to fail with 'metadata.resourceVersion: Invalid value: 0') Signed-off-by: yxxhero Signed-off-by: yxxhero --- scripts/issues/917.sh | 160 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100755 scripts/issues/917.sh diff --git a/scripts/issues/917.sh b/scripts/issues/917.sh new file mode 100755 index 00000000..5bd0d714 --- /dev/null +++ b/scripts/issues/917.sh @@ -0,0 +1,160 @@ +#!/usr/bin/env bash +# Reproduction test for https://github.com/databus23/helm-diff/issues/917 +# +# Bug: "helm diff --three-way-merge does not detect manual changes made to +# Custom Resources (CRDs and CRs) in the cluster". +# +# Unstructured objects fell back to a JSON merge patch built from the old and +# the new release manifest only, so the live object was ignored and manual +# changes (drift) never showed up in the diff. +# +# Scenarios (each prints its full diff output to the CI log): +# A. --three-way-merge of a clean, unchanged release reports no drift +# B. manual change on a CR field the chart owns is detected +# (also without patch permissions, via --three-way-merge-mode client, +# and NOT detected by a plain diff, which never looks at the cluster) +# C. a chart change is merged with the manual change, not masked by it +# D. manual change on the CRD itself is detected +# +# In addition, no scenario may print an error or a diff entry for +# server-managed fields (resourceVersion), which used to break the dry-run. + +set -euo pipefail + +ISSUE=917 +# shellcheck source=lib.sh +source "$(dirname "$0")/lib.sh" + +# expect +expect() { + local scenario="$1" desc="$2" pattern="$3" want="$4" got + local out="$WORK/${scenario// /_}.out" + strip_ansi < "$out" > "$WORK/stripped.out" + if grep -qE "$pattern" "$WORK/stripped.out"; then + got=present + else + got=absent + fi + if [ "$got" = "$want" ]; then + echo "OK: [$scenario] $desc is $want" + else + echo "FAIL: [$scenario] expected $desc to be $want (#${ISSUE})" + FAIL=1 + fi +} + +kubectl create namespace "$NS" 2>/dev/null || true + +CHART='apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: widgets.example.com +spec: + group: example.com + names: + plural: widgets + singular: widget + kind: Widget + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + x-kubernetes-preserve-unknown-fields: true +--- +apiVersion: example.com/v1 +kind: Widget +metadata: + name: w +spec: + size: {{ .Values.size | default "small" }} +' + +chart "$WORK/a" "$CHART" + +# Helm applies the CRD before the CR but does not wait for it to become +# established, so the very first install can lose a race. Retry. +ok=0 +for attempt in 1 2 3; do + if helm upgrade -i rel "$WORK/a" -n "$NS" > "$WORK/install.out" 2>&1; then + ok=1 + break + fi + echo "===== helm install attempt ${attempt} failed =====" + cat "$WORK/install.out" + kubectl wait --for=condition=Established crd/widgets.example.com --timeout=30s >/dev/null 2>&1 || true + sleep 2 +done +if [ "$ok" -ne 1 ]; then + echo "FAIL: could not install the release (#${ISSUE})" + exit 1 +fi +kubectl wait --for=condition=Established crd/widgets.example.com --timeout=60s >/dev/null +kubectl get widget w -n "$NS" -o jsonpath='{.spec.size}'; echo " <- live CR size" + +############################################################################### +# Variant A: clean release, unchanged chart -> no drift, no errors +############################################################################### +run_diff "A clean" noassert rel "$WORK/a" -n "$NS" --three-way-merge +expect "A clean" "no diff entry for spec.size" '^[+-][[:space:]]*size:' absent +expect "A clean" "no error" '^Error:' absent + +############################################################################### +# Variant B: manual change on a CR field the chart owns must be detected +############################################################################### +kubectl patch widget w -n "$NS" --type=merge -p '{"spec":{"size":"large"}}' >/dev/null + +run_diff "B drift" noassert rel "$WORK/a" -n "$NS" --three-way-merge +expect "B drift" "diff entry for the live value (size: large)" '^[+-][[:space:]]*size: large' present +expect "B drift" "diff entry for the chart value (size: small)" '^[+-][[:space:]]*size: small' present +expect "B drift" "no error" '^Error:' absent + +# A plain diff never looks at the cluster, so it must stay silent about the +# drift (documents the difference the --three-way-merge flag makes). +run_diff "B plain" noassert rel "$WORK/a" -n "$NS" +expect "B plain" "no drift entry without --three-way-merge" '^[+-][[:space:]]*size:' absent + +# The local merge (no patch permissions needed) must detect the drift as well. +run_diff "B client" noassert rel "$WORK/a" -n "$NS" --three-way-merge --three-way-merge-mode client +expect "B client" "diff entry for the live value (size: large)" '^[+-][[:space:]]*size: large' present +expect "B client" "diff entry for the chart value (size: small)" '^[+-][[:space:]]*size: small' present +expect "B client" "no error" '^Error:' absent + +############################################################################### +# Variant C: chart change merged with the manual change +############################################################################### +run_diff "C chart-change" noassert rel "$WORK/a" -n "$NS" --three-way-merge --set size=medium +expect "C chart-change" "diff entry for the new chart value (size: medium)" '^[+-][[:space:]]*size: medium' present +expect "C chart-change" "no error" '^Error:' absent + +############################################################################### +# Variant D: manual change on the CRD itself must be detected +############################################################################### +kubectl patch crd widgets.example.com --type=merge -p '{"spec":{"versions":[{"name":"v1","served":false,"storage":true,"schema":{"openAPIV3Schema":{"type":"object","x-kubernetes-preserve-unknown-fields":true}}}]}}' >/dev/null + +run_diff "D crd-drift" noassert rel "$WORK/a" -n "$NS" --three-way-merge +expect "D crd-drift" "diff entry for the live value (served: false)" '^[+-][[:space:]]*served: false' present +expect "D crd-drift" "diff entry for the chart value (served: true)" '^[+-][[:space:]]*served: true' present +expect "D crd-drift" "no error" '^Error:' absent + +############################################################################### +# Server-managed fields must never show up as diff entries (the dry-run patch +# used to fail with "metadata.resourceVersion: Invalid value: 0"). +############################################################################### +echo "" +for scenario in "A clean" "B drift" "B plain" "B client" "C chart-change" "D crd-drift"; do + out="$WORK/${scenario// /_}.out" + strip_ansi < "$out" > "$WORK/stripped.out" + if grep -qE '^[+-][[:space:]]*resourceVersion:' "$WORK/stripped.out"; then + echo "FAIL: [$scenario] shows a resourceVersion diff entry (#${ISSUE})" + FAIL=1 + fi +done +if [ "$FAIL" -eq 0 ]; then + echo "OK: no resourceVersion diff entries in any scenario" +fi + +finish From a0a4264b625871a000056c9d8126985991efbf1d Mon Sep 17 00:00:00 2001 From: yxxhero Date: Fri, 25 Sep 2026 14:02:24 +0800 Subject: [PATCH 11/13] fix(test): pre-install the CRD before helm install in #917 e2e test Helm fails to build the release manifest when a chart's templates contain both a CRD and a CR of that kind ('no matches for kind Widget ... ensure CRDs are installed first'), so the CRD is applied with kubectl and waited for before the install. The CRD stays in the release manifest via templates/, keeping the CRD drift scenario meaningful. Replaces the retry loop with a linear flow now that the precondition is guaranteed. Signed-off-by: yxxhero Signed-off-by: yxxhero --- scripts/issues/917.sh | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/scripts/issues/917.sh b/scripts/issues/917.sh index 5bd0d714..5f9de6a2 100755 --- a/scripts/issues/917.sh +++ b/scripts/issues/917.sh @@ -45,7 +45,7 @@ expect() { kubectl create namespace "$NS" 2>/dev/null || true -CHART='apiVersion: apiextensions.k8s.io/v1 +CRD_YAML='apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: name: widgets.example.com @@ -64,8 +64,9 @@ spec: openAPIV3Schema: type: object x-kubernetes-preserve-unknown-fields: true ---- -apiVersion: example.com/v1 +' + +CR_TMPL='apiVersion: example.com/v1 kind: Widget metadata: name: w @@ -73,26 +74,17 @@ spec: size: {{ .Values.size | default "small" }} ' -chart "$WORK/a" "$CHART" - -# Helm applies the CRD before the CR but does not wait for it to become -# established, so the very first install can lose a race. Retry. -ok=0 -for attempt in 1 2 3; do - if helm upgrade -i rel "$WORK/a" -n "$NS" > "$WORK/install.out" 2>&1; then - ok=1 - break - fi - echo "===== helm install attempt ${attempt} failed =====" - cat "$WORK/install.out" - kubectl wait --for=condition=Established crd/widgets.example.com --timeout=30s >/dev/null 2>&1 || true - sleep 2 -done -if [ "$ok" -ne 1 ]; then - echo "FAIL: could not install the release (#${ISSUE})" - exit 1 -fi +# Helm cannot build a manifest whose CR refers to a CRD that is only defined +# in the same chart ("no matches for kind Widget ... ensure CRDs are installed +# first"), so the CRD has to exist in the cluster before the install. It is +# still part of the release manifest via templates/, which is what scenario D +# relies on. +kubectl apply -f <(printf '%s\n' "$CRD_YAML") >/dev/null kubectl wait --for=condition=Established crd/widgets.example.com --timeout=60s >/dev/null + +chart "$WORK/a" "${CRD_YAML}--- +${CR_TMPL}" +helm upgrade -i rel "$WORK/a" -n "$NS" >/dev/null kubectl get widget w -n "$NS" -o jsonpath='{.spec.size}'; echo " <- live CR size" ############################################################################### From 06fc4a16ba56afd478aaa469e01d54a3574dd802 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Fri, 25 Sep 2026 14:16:23 +0800 Subject: [PATCH 12/13] fix(test): adopt the pre-installed CRD into the release in #917 e2e test Helm (>=3.13) refuses to install a release whose manifest contains an already existing resource without ownership metadata ('exists and cannot be imported into the current release: invalid ownership metadata'). Label the kubectl-created CRD with app.kubernetes.io/managed-by=Helm and annotate it with the release name/namespace so Helm adopts it. Signed-off-by: yxxhero Signed-off-by: yxxhero --- scripts/issues/917.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/issues/917.sh b/scripts/issues/917.sh index 5f9de6a2..fb86b994 100755 --- a/scripts/issues/917.sh +++ b/scripts/issues/917.sh @@ -78,9 +78,12 @@ spec: # in the same chart ("no matches for kind Widget ... ensure CRDs are installed # first"), so the CRD has to exist in the cluster before the install. It is # still part of the release manifest via templates/, which is what scenario D -# relies on. +# relies on. Because the CRD then already exists when Helm installs the +# release, it must carry the ownership metadata Helm requires to adopt it. kubectl apply -f <(printf '%s\n' "$CRD_YAML") >/dev/null kubectl wait --for=condition=Established crd/widgets.example.com --timeout=60s >/dev/null +kubectl label crd widgets.example.com app.kubernetes.io/managed-by=Helm --overwrite >/dev/null +kubectl annotate crd widgets.example.com meta.helm.sh/release-name=rel meta.helm.sh/release-namespace="$NS" --overwrite >/dev/null chart "$WORK/a" "${CRD_YAML}--- ${CR_TMPL}" From bafc14241a2c5c72510fd5242a13345f5764a42c Mon Sep 17 00:00:00 2001 From: yxxhero Date: Fri, 25 Sep 2026 14:31:07 +0800 Subject: [PATCH 13/13] fix(test): drift a names field instead of served in #917 CRD scenario Setting served: false on the CRD version stops it from being served, the REST mapper can no longer resolve kind Widget and helm diff fails to build the new manifest ('no matches for kind Widget ... ensure CRDs are installed first') before any diff is computed. Drift spec.names.singular instead: the chart owns it, the version stays served and the drift is still detected through the same code path. Signed-off-by: yxxhero Signed-off-by: yxxhero --- scripts/issues/917.sh | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/scripts/issues/917.sh b/scripts/issues/917.sh index fb86b994..da8c30b6 100755 --- a/scripts/issues/917.sh +++ b/scripts/issues/917.sh @@ -127,12 +127,16 @@ expect "C chart-change" "no error" '^Error:' absent ############################################################################### # Variant D: manual change on the CRD itself must be detected +# +# The drift must not stop the version from being served ("served: false" +# makes the REST mapper unable to resolve kind Widget, so the new manifest +# cannot even be built), so a chart-owned names field is drifted instead. ############################################################################### -kubectl patch crd widgets.example.com --type=merge -p '{"spec":{"versions":[{"name":"v1","served":false,"storage":true,"schema":{"openAPIV3Schema":{"type":"object","x-kubernetes-preserve-unknown-fields":true}}}]}}' >/dev/null +kubectl patch crd widgets.example.com --type=merge -p '{"spec":{"names":{"singular":"widg"}}}' >/dev/null run_diff "D crd-drift" noassert rel "$WORK/a" -n "$NS" --three-way-merge -expect "D crd-drift" "diff entry for the live value (served: false)" '^[+-][[:space:]]*served: false' present -expect "D crd-drift" "diff entry for the chart value (served: true)" '^[+-][[:space:]]*served: true' present +expect "D crd-drift" "diff entry for the live value (singular: widg)" '^[+-][[:space:]]*singular: widg' present +expect "D crd-drift" "diff entry for the chart value (singular: widget)" '^[+-][[:space:]]*singular: widget' present expect "D crd-drift" "no error" '^Error:' absent ###############################################################################