Skip to content

fix: detect manual changes on CRDs and CRs with three-way-merge #923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yxxhero wants to merge 7 commits into
base: master
Choose a base branch
from
+489 −2
Open

fix: detect manual changes on CRDs and CRs with three-way-merge #923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
115630c
fix: detect manual changes on CRDs and CRs with three-way-merge
yxxhero Feb 1, 2026
6610be9
test: add tests for three-way merge with unstructured objects
yxxhero Feb 1, 2026
a460e71
fix: properly detect drift when chart unchanged for unstructured objects
yxxhero Feb 1, 2026
efb7e0a
fix: correct comment direction in three-way merge logic
yxxhero Feb 1, 2026
a59ce03
fix: clean metadata fields to avoid resourceVersion patch errors
yxxhero Feb 14, 2026
5d14d97
fix: preserve live-only fields in three-way merge for unstructured ob…
yxxhero Feb 14, 2026
f5b13c2
refactor: reuse deleteStatusAndTidyMetadata in cleanMetadataForPatch
yxxhero Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions manifest/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,63 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info)
_, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition)

if isUnstructured || isCRD {
// fall back to generic JSON merge patch
patch, err := jsonpatch.CreateMergePatch(oldData, newData)
// For unstructured objects (CRDs, CRs), we need to perform a three-way merge
// to detect manual changes made in the cluster.
//
// The approach is:
// 1. Create a patch from old -> new (chart changes)
// 2. Apply this patch to current (live state with manual changes)
// 3. Create a patch from current -> merged result
// 4. If chart changed (old != new), return step 3's patch
// 5. If chart unchanged (old == new), 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
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)
Comment on lines +185 to +196
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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)
}

Copilot uses AI. Check for mistakes.
}

// Step 1: Create patch from old -> new (what the chart wants to change)
chartChanges, err := jsonpatch.CreateMergePatch(cleanedOldData, cleanedNewData)
if err != nil {
return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err)
}
Comment on lines 199 to 203
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// 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(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(cleanedCurrentData, mergedData)
return patch, types.MergePatchType, err
}

// Chart didn't change (old == new), but we need to detect if current diverges
// 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
Comment on lines 221 to 230
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
}

Expand All @@ -184,6 +239,18 @@ 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 cleanMetadataForPatch(data []byte) ([]byte, error) {
objMap, err := deleteStatusAndTidyMetadata(data)
if err != nil {
return nil, err
}
return json.Marshal(objMap)
}
Comment on lines 246 to 252
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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)
Expand Down
Loading
Loading