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
+532 −24
Open

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

Show file tree
Hide file tree
Changes from 2 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
26 changes: 24 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,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)
}
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.

// 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
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 Down
266 changes: 266 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,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) {
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: 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))
}
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 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.

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

Copilot uses AI. Check for mistakes.
}
})
}
}
Loading