diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..d785798d 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -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) + } + + // 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) + } + + // 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 } @@ -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) +} + 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) diff --git a/manifest/generate_test.go b/manifest/generate_test.go new file mode 100644 index 00000000..d5c4611a --- /dev/null +++ b/manifest/generate_test.go @@ -0,0 +1,420 @@ +package manifest + +import ( + "testing" + + "github.com/stretchr/testify/require" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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) { + tests := []struct { + name string + original runtime.Object + current runtime.Object + target *resource.Info + 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 not present in chart should be preserved; no effective change expected from three-way merge", + }, + { + 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)) + } + }) + } +} + +// TestCreatePatchForCRD tests the three-way merge implementation for CRD objects. +// This ensures the isCRD branch in createPatch is properly covered. +func TestCreatePatchForCRD(t *testing.T) { + originalCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + currentCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + targetCRD := &apiextv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "crds.example.com", + }, + Spec: apiextv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextv1.CustomResourceDefinitionNames{ + Plural: "crds", + Singular: "crd", + Kind: "Crd", + }, + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } + + target := &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "apiextensions.k8s.io", + Version: "v1", + Kind: "CustomResourceDefinition", + }, + }, + Object: targetCRD, + } + + patch, patchType, err := createPatch(originalCRD, currentCRD, target) + require.NoError(t, err, "createPatch should not return an error for CRD") + require.Equal(t, types.MergePatchType, patchType, "patch type should be MergePatchType for CRD objects") + + t.Logf("CRD Patch result: %s", string(patch)) + + require.Equal(t, "{}", string(patch), "CRD with no changes should result in empty patch") +}