fix: detect manual changes on CRDs and CRs with three-way-merge#923
fix: detect manual changes on CRDs and CRs with three-way-merge#923
Conversation
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 <aiopsclub@163.com>
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 <aiopsclub@163.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the manifest generation logic to perform a more sophisticated three-way merge for unstructured resources (CRDs/CRs) so that live state is incorporated into the patch calculation, and adds focused unit tests around this behavior.
Changes:
- Modified
createPatchinmanifest/generate.goso that for unstructured/CRD objects it computes a merge patch by deriving chart changes from the previous release to the new manifest and then applying those changes onto the live object before generating the final patch. - Added
manifest/generate_test.gowith table-driven tests that exercisecreatePatchfor several CR/CRD scenarios, verifying patch type and whether a patch is (non-)empty. - Improved error context messages in the unstructured/CRD path of
createPatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| manifest/generate.go | Adjusts createPatch to use a custom three-way merge flow for unstructured/CRD objects instead of a simple old→new JSON merge patch. |
| manifest/generate_test.go | Introduces unit tests asserting patch type and presence/absence of diffs for various CR/CRD combinations of original, current, and target manifests. |
| tests := []struct { | ||
| name string | ||
| original runtime.Object | ||
| current runtime.Object | ||
| target *resource.Info |
There was a problem hiding this comment.
The table-driven tests cover several useful combinations of original/current/target, but they do not include the core scenario from issue #917 where a field that is explicitly defined in the chart remains unchanged between original and target while being modified only in the live object (e.g., spec.maxReplicaCount is 10 in both original and target, but 30 in current). Without a test for this pattern, it is easy for the unstructured/CRD three-way-merge logic to regress (or remain incorrect) while all tests here still pass. Please add a case that matches the reported bug scenario to assert that a non-empty patch is produced when live state diverges from the chart for a chart-owned field, even if the chart did not change that field between releases.
manifest/generate_test.go
Outdated
| // No changes expected | ||
| if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" { | ||
| t.Logf("%s: got unexpected patch: %s", tt.description, string(patch)) | ||
| } |
There was a problem hiding this comment.
In the expectedChange == false branch, the test only logs an unexpected non-empty patch instead of failing the test when a non-trivial patch is returned. For cases like "CR with no manual changes", an empty (or effectively empty) patch is the expected behavior, so silently accepting a non-empty patch here would hide regressions in the three-way-merge logic. Consider making this branch assert that the patch is empty (allowing only {} or "null" as appropriate) so that any future behavior change that introduces unintended diffs will cause the test to fail.
| // 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)) |
| // 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) | ||
| } |
There was a problem hiding this comment.
The three-way merge implementation for unstructured/CRD objects here will still fail to detect drift when a field is unchanged between oldData and newData but modified only in the live object (the scenario described in #917 for values explicitly set in the chart). In that case jsonpatch.CreateMergePatch(oldData, newData) returns an empty patch, mergedData ends up identical to currentData, and CreateMergePatch(currentData, mergedData) also returns an empty patch, so no diff is produced even though the live state diverges from the chart. This contradicts the PR description that manual changes to CRDs/CRs are now detected "just like" built-in resources, and it means the fix does not yet cover the core bug scenario. To align behavior with the strategic three-way merge path used for built-in types, the unstructured/CRD branch should compute a patch that also captures differences between currentData and newData for chart-owned fields, even when newData equals oldData, rather than only replaying old -> new changes onto currentData.
Signed-off-by: yxxhero <aiopsclub@163.com>
manifest/generate.go
Outdated
| // 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) |
There was a problem hiding this comment.
In the step-by-step comment for the unstructured/CRD three-way merge, step 5 says "return new->current patch to restore chart state", but the implementation below calls jsonpatch.CreateMergePatch(currentData, newData), i.e. it creates a patch from current -> new. To avoid confusion for future readers, the comment should be updated to describe the same direction as the code (patch from current to new, applied to the current/live object).
| // 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) |
Signed-off-by: yxxhero <aiopsclub@163.com>
|
When running with this change against an Istio |
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 <aiopsclub@163.com>
| // 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) | ||
| return patch, types.MergePatchType, err |
There was a problem hiding this comment.
In the chartChanged == false branch, CreateMergePatch(cleanedCurrentData, cleanedNewData) will generate deletions for any fields that exist only in the live object (e.g., manual annotations/labels, controllers’ injected fields, and potentially Helm’s own meta.helm.sh/* annotations if they’re not present in the rendered manifest). That makes the server dry-run “target” diverge from what a three-way merge should do and can produce noisy/incorrect diffs by removing user-owned fields when the chart didn’t change. Consider instead building a desired object by overlaying the chart state onto the live state (preserving unknown additions), then diffing current -> desired (e.g., apply a merge-patch of cleanedNewData onto cleanedCurrentData and compute a patch from current to that result).
| 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) | ||
| } |
There was a problem hiding this comment.
cleanMetadataForPatch largely overlaps with deleteStatusAndTidyMetadata (manifest/util.go) but removes a different set of metadata fields/annotations (e.g., meta.helm.sh/* and deployment.kubernetes.io/revision are handled in the other helper but not here). Having two slightly different “tidy metadata” implementations risks inconsistent behavior between patch generation and manifest rendering. Suggest reusing/extracting the existing helper (or at least aligning the exact fields removed) so the same metadata is ignored everywhere.
manifest/generate_test.go
Outdated
| expectedChange: true, | ||
| description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)", |
There was a problem hiding this comment.
This case asserts a non-empty patch when the only live change is an extra annotation not present in the chart. That implies the diff will attempt to remove live-only fields, which is typically not what three-way merge behavior should do (it should preserve unknown additions and only reconcile chart-owned fields). If the implementation is changed to preserve live-only fields (recommended), update this expectation to assert no effective change / that the extra annotation is preserved, and focus drift detection on chart fields (e.g. the issue #917 spec field).
| 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", |
…jects - 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 <aiopsclub@163.com>
manifest/generate.go
Outdated
| func cleanMetadataForPatch(data []byte) ([]byte, error) { | ||
| var objMap map[string]interface{} | ||
| if err := json.Unmarshal(data, &objMap); 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") |
There was a problem hiding this comment.
cleanMetadataForPatch largely duplicates deleteStatusAndTidyMetadata (same metadata/status stripping + annotation cleanup) but now exists as a separate implementation with different JSON codec and no direct unit tests. To avoid the two clean-up paths drifting over time, consider extracting a shared helper (or reusing deleteStatusAndTidyMetadata and re-marshalling) so metadata stripping behavior is defined in one place and covered by the existing util tests.
| // 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) { |
There was a problem hiding this comment.
The new unstructured three-way-merge tests exercise the runtime.Unstructured branch, but the dedicated CRD type branch (isCRD == true) isn’t covered. Since the PR claims to fix CRDs and CRs, add at least one test case that uses an apiextensions/v1 CustomResourceDefinition object to ensure createPatch takes the CRD path and produces the expected patch behavior.
- 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 <aiopsclub@163.com>
| // 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) |
There was a problem hiding this comment.
cleanMetadataForPatch calls deleteStatusAndTidyMetadata, which assumes the JSON decodes to a map containing a metadata object. If currentObj is ever nil (the function comment above still claims this is possible), json.Marshal(currentObj) becomes null, which will cause a panic when deleteStatusAndTidyMetadata does objectMap["metadata"].(map[string]interface{}). Consider explicitly handling null/empty input in cleanMetadataForPatch (e.g., treat it as {} or skip cleaning) and/or update the comment about currentObj potentially being nil so behavior stays consistent and panic-free.
| // 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) | |
| // This prevents "resourceVersion: Invalid value: 0" errors when dry-running patches. | |
| // If the JSON represents a null/empty object, skip cleaning to avoid panics in | |
| // downstream metadata processing that expects a full object with metadata. | |
| var cleanedOldData []byte | |
| trimmedOld := bytes.TrimSpace(oldData) | |
| if len(trimmedOld) == 0 || bytes.Equal(trimmedOld, []byte("null")) { | |
| cleanedOldData = oldData | |
| } else { | |
| var err error | |
| cleanedOldData, err := cleanMetadataForPatch(oldData) | |
| if err != nil { | |
| return nil, types.MergePatchType, fmt.Errorf("cleaning old metadata: %w", err) | |
| } | |
| } | |
| var cleanedNewData []byte | |
| trimmedNew := bytes.TrimSpace(newData) | |
| if len(trimmedNew) == 0 || bytes.Equal(trimmedNew, []byte("null")) { | |
| cleanedNewData = newData | |
| } else { | |
| var err error | |
| cleanedNewData, err := cleanMetadataForPatch(newData) | |
| if err != nil { | |
| return nil, types.MergePatchType, fmt.Errorf("cleaning new metadata: %w", err) | |
| } | |
| } | |
| var cleanedCurrentData []byte | |
| trimmedCurrent := bytes.TrimSpace(currentData) | |
| if len(trimmedCurrent) == 0 || bytes.Equal(trimmedCurrent, []byte("null")) { | |
| cleanedCurrentData = currentData | |
| } else { | |
| var err error | |
| cleanedCurrentData, err := cleanMetadataForPatch(currentData) | |
| if err != nil { | |
| return nil, types.MergePatchType, fmt.Errorf("cleaning current metadata: %w", err) | |
| } |
2 participants
Summary
Fixes #917
Previously,
helm diff --three-way-mergewould 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:
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.
Changes
manifest/generate.goto implement three-way merge for unstructured objects (CRDs, CRs)Testing