diff --git a/manifest/generate.go b/manifest/generate.go index 23b8f6bd..9318fe4c 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -170,9 +170,70 @@ 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) - return patch, types.MergePatchType, err + // 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) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("creating patch from current to merged: %w", err) + } + return patch, types.MergePatchType, nil + } + + // 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) + if err != nil { + return nil, types.MergePatchType, fmt.Errorf("creating patch from current to desired: %w", err) + } + return patch, types.MergePatchType, nil } patchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject) @@ -184,6 +245,21 @@ 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 + } + if objMap == nil { + return []byte("null"), nil + } + 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") +} diff --git a/manifest/util.go b/manifest/util.go index 88b818af..3ee3bbc9 100644 --- a/manifest/util.go +++ b/manifest/util.go @@ -6,16 +6,23 @@ import ( jsoniter "github.com/json-iterator/go" ) -func deleteStatusAndTidyMetadata(obj []byte) (map[string]interface{}, error) { - var objectMap map[string]interface{} +func deleteStatusAndTidyMetadata(obj []byte) (map[string]any, error) { + var objectMap map[string]any err := jsoniter.Unmarshal(obj, &objectMap) if err != nil { return nil, fmt.Errorf("could not unmarshal byte sequence: %w", err) } + if objectMap == nil { + return nil, nil + } + delete(objectMap, "status") - metadata := objectMap["metadata"].(map[string]interface{}) + metadata, ok := objectMap["metadata"].(map[string]any) + if !ok { + return objectMap, nil + } delete(metadata, "managedFields") delete(metadata, "generation") @@ -23,16 +30,15 @@ func deleteStatusAndTidyMetadata(obj []byte) (map[string]interface{}, error) { delete(metadata, "resourceVersion") delete(metadata, "uid") - // See the below for the goal of this metadata tidy logic. - // https://github.com/databus23/helm-diff/issues/326#issuecomment-1008253274 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") - - if len(annotations) == 0 { - delete(metadata, "annotations") + if annotations, ok := a.(map[string]any); ok { + delete(annotations, "meta.helm.sh/release-name") + delete(annotations, "meta.helm.sh/release-namespace") + delete(annotations, "deployment.kubernetes.io/revision") + + if len(annotations) == 0 { + delete(metadata, "annotations") + } } } diff --git a/manifest/util_test.go b/manifest/util_test.go index 0f86223a..3b844a1d 100644 --- a/manifest/util_test.go +++ b/manifest/util_test.go @@ -10,7 +10,7 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { tests := []struct { name string obj []byte - want map[string]interface{} + want map[string]any wantErr bool }{ { @@ -19,6 +19,12 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { want: nil, wantErr: true, }, + { + name: "null json", + obj: []byte("null"), + want: nil, + wantErr: false, + }, { name: "valid json", obj: []byte(` @@ -57,21 +63,21 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { } } `), - want: map[string]interface{}{ + want: map[string]any{ "apiVersion": "apps/v1", "kind": "Deployment", - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ + "metadata": map[string]any{ + "annotations": map[string]any{ "other-annot": "value", }, "name": "nginx-deployment", "namespace": "test-ns", }, - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "containers": []interface{}{ - map[string]interface{}{ + "spec": map[string]any{ + "template": map[string]any{ + "spec": map[string]any{ + "containers": []any{ + map[string]any{ "image": "nginx:1.14.2", "imagePullPolicy": "IfNotPresent", "name": "nginx",