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 9 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 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
2ff4b31
fix: handle null input in deleteStatusAndTidyMetadata
yxxhero Feb 15, 2026
1076d7d
fix: improve error handling and type safety
yxxhero Feb 15, 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
94 changes: 92 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
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

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

The error returned here should be wrapped with context for better debugging. Consider wrapping it like the other error returns in this block:

patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, mergedData)
if err != nil {
    return nil, types.MergePatchType, fmt.Errorf("creating patch from current to merged: %w", err)
}
return patch, types.MergePatchType, nil

This ensures consistent error handling and provides clearer error messages when issues occur.

Copilot uses AI. Check for mistakes.
}

// 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
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

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

The error returned here should be wrapped with context for better debugging. Consider wrapping it like the other error returns in this block:

patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, desiredData)
if err != nil {
    return nil, types.MergePatchType, fmt.Errorf("creating patch from current to desired: %w", err)
}
return patch, types.MergePatchType, nil

This ensures consistent error handling within this function and provides clearer error messages when issues occur.

Suggested change
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

Copilot uses AI. Check for mistakes.
}

Expand All @@ -184,6 +239,41 @@ 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) {
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")
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 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.

Copilot uses AI. Check for mistakes.

if len(annotations) == 0 {
delete(metadata, "annotations")
}
}
}

return json.Marshal(objMap)
}
Comment on lines +252 to +261
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