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 3 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
44 changes: 42 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,44 @@ 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) but current != new, return new->current patch to restore chart state
// (i.e., detect and revert manual changes to chart-owned fields)
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.

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

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

Copilot uses AI. Check for mistakes.

// 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)
}
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(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
}

// 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
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 +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)
Expand Down
320 changes: 320 additions & 0 deletions manifest/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,320 @@
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) {
Comment on lines +17 to +19
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.

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.

Copilot uses AI. Check for mistakes.
tests := []struct {
name string
original runtime.Object
current runtime.Object
target *resource.Info
Comment on lines +20 to +24
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 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.

Copilot uses AI. Check for mistakes.
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: true,
description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)",
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.

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

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

Copilot uses AI. Check for mistakes.
},
{
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 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{
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: 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))
}
})
}
}
Loading