Deduplicate appended slices in options.Merge (#5197)

options.Merge appended DownstreamResources, Helm.ValuesFrom, and
Diff.ComparePatches slices without checking for duplicates. When
both base and a per-target customization reference the same entry,
the same resource reference ended up in the merged list twice.

Extract a generic merge.ByKey helper to internal/merge. It iterates
base and custom once each (O(n) per slice), replaces a base entry
in-place when the custom entry shares its key, and appends otherwise.
The collision behaviour is caller-defined via an onCollide function.

- options.mergeUnique wraps merge.ByKey with a replace-with-custom
  collision handler, consistent with how the rest of options.Merge
  treats per-target customizations.
- bundlediff.mergeComparePatches is simplified to merge.ByKey with
  a union-operations collision handler: existing operations are kept
  and new ones not already present are appended. The result is then
  sorted for deterministic output.

Key functions:
- downstreamResourceKey: strings.ToLower(Kind)+"|"+Name (case-insensitive Kind)
- valuesFromKey: source-type+"|"+Namespace+"|"+Name+"|"+Key
- comparePatchKey (options) / patchIdentityKey (bundlediff):
  APIVersion+"|"+Kind+"|"+Namespace+"|"+Name

Author: thardeck

internal/cmd/cli/bundlediff.go

@@ -19,6 +19,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	command "github.com/rancher/fleet/internal/cmd"
+	"github.com/rancher/fleet/internal/merge"
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 )
 
@@ -249,54 +250,57 @@ func convertMergePatchToRemoveOps(patch map[string]any, basePath string) []patch
 	return ops
 }
 
-// mergeComparePatches merges new comparePatches with existing ones, avoiding duplicates.
-// This preserves any user-configured comparePatches while adding new ones from drift detection.
-func mergeComparePatches(existing, new []fleet.ComparePatch) []fleet.ComparePatch {
-	patchMap := make(map[string]fleet.ComparePatch)
+// patchIdentityKey returns the key that identifies a ComparePatch resource.
+func patchIdentityKey(p fleet.ComparePatch) string {
+	return p.APIVersion + "|" + p.Kind + "|" + p.Namespace + "|" + p.Name
+}
 
-	// Key function for resource identity
-	resourceKey := func(p fleet.ComparePatch) string {
-		return fmt.Sprintf("%s|%s|%s|%s", p.APIVersion, p.Kind, p.Namespace, p.Name)
+// unionComparePatchOps merges two ComparePatch entries for the same resource by
+// unioning their operations: base operations are kept and custom operations not
+// already present in base are appended.
+func unionComparePatchOps(base, custom fleet.ComparePatch) fleet.ComparePatch {
+	opSet := make(map[string]struct{}, len(base.Operations))
+	for _, op := range base.Operations {
+		opSet[op.Op+"|"+op.Path+"|"+op.Value] = struct{}{}
+	}
+	result := base
+	for _, op := range custom.Operations {
+		if _, exists := opSet[op.Op+"|"+op.Path+"|"+op.Value]; !exists {
+			result.Operations = append(result.Operations, op)
+		}
 	}
+	return result
+}
 
-	for _, patch := range existing {
-		key := resourceKey(patch)
-		patchMap[key] = patch
-	}
+// mergeComparePatches merges new comparePatches with existing ones.
+// For the same resource identity, operations are unioned — existing operations
+// are kept and new operations not already present are appended. The result is
+// sorted by resource identity for deterministic output.
+func mergeComparePatches(existing, new []fleet.ComparePatch) []fleet.ComparePatch {
+	// Consolidate duplicate resource identities within new by unioning their
+	// operations before merging with existing, so no operations are dropped.
+	consolidated := consolidatePatches(new)
+	merged := merge.ByKey(existing, consolidated, patchIdentityKey, unionComparePatchOps)
+	sort.Slice(merged, func(i, j int) bool {
+		return patchIdentityKey(merged[i]) < patchIdentityKey(merged[j])
+	})
+	return merged
+}
 
-	// Merge new patches, combining operations for same resource
-	for _, newPatch := range new {
-		key := resourceKey(newPatch)
-		if existingPatch, found := patchMap[key]; found {
-			merged := existingPatch
-			opSet := make(map[string]bool)
-			for _, op := range existingPatch.Operations {
-				opKey := fmt.Sprintf("%s|%s|%s", op.Op, op.Path, op.Value)
-				opSet[opKey] = true
-			}
-			for _, op := range newPatch.Operations {
-				opKey := fmt.Sprintf("%s|%s|%s", op.Op, op.Path, op.Value)
-				if !opSet[opKey] {
-					merged.Operations = append(merged.Operations, op)
-				}
-			}
-			patchMap[key] = merged
+// consolidatePatches folds entries with the same resource identity by unioning
+// their operations, preserving the order of first occurrence.
+func consolidatePatches(patches []fleet.ComparePatch) []fleet.ComparePatch {
+	seen := make(map[string]int, len(patches))
+	result := make([]fleet.ComparePatch, 0, len(patches))
+	for _, p := range patches {
+		k := patchIdentityKey(p)
+		if idx, ok := seen[k]; ok {
+			result[idx] = unionComparePatchOps(result[idx], p)
 		} else {
-			patchMap[key] = newPatch
+			seen[k] = len(result)
+			result = append(result, p)
 		}
 	}
-
-	// Convert map back to slice, sorted for deterministic output
-	var result []fleet.ComparePatch
-	keys := make([]string, 0, len(patchMap))
-	for k := range patchMap {
-		keys = append(keys, k)
-	}
-	sort.Strings(keys)
-	for _, k := range keys {
-		result = append(result, patchMap[k])
-	}
-
 	return result
 }
 

internal/cmd/cli/bundlediff_test.go

@@ -63,6 +63,46 @@ func TestMergeComparePatches(t *testing.T) {
 	}
 }
 
+func TestMergeComparePatches_DuplicateNewEntries(t *testing.T) {
+	// Two new entries for the same resource identity must have their operations
+	// unioned, not dropped.
+	newPatches := []fleet.ComparePatch{
+		{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+			Name:       "app-config",
+			Namespace:  "default",
+			Operations: []fleet.Operation{{Op: "remove", Path: "/data/a"}},
+		},
+		{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+			Name:       "app-config",
+			Namespace:  "default",
+			Operations: []fleet.Operation{{Op: "remove", Path: "/data/b"}},
+		},
+	}
+
+	expected := []fleet.ComparePatch{
+		{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+			Name:       "app-config",
+			Namespace:  "default",
+			Operations: []fleet.Operation{
+				{Op: "remove", Path: "/data/a"},
+				{Op: "remove", Path: "/data/b"},
+			},
+		},
+	}
+
+	merged := mergeComparePatches(nil, newPatches)
+
+	if diff := cmp.Diff(expected, merged); diff != "" {
+		t.Errorf("mergeComparePatches() mismatch (-want +got):\n%s", diff)
+	}
+}
+
 func TestConvertMergePatchToRemoveOps(t *testing.T) {
 	tests := []struct {
 		name     string

internal/cmd/controller/options/calculate.go

@@ -6,11 +6,37 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"maps"
+	"strings"
 
+	"github.com/rancher/fleet/internal/merge"
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 	"github.com/rancher/wrangler/v3/pkg/data"
 )
 
+// mergeUnique merges two slices by key, with custom entries taking precedence
+// over base entries for the same key. See merge.ByKey for full semantics.
+func mergeUnique[T any](base, custom []T, keyFn func(T) string) []T {
+	return merge.ByKey(base, custom, keyFn, func(_ T, c T) T { return c })
+}
+
+func downstreamResourceKey(r fleet.DownstreamResource) string {
+	return strings.ToLower(r.Kind) + "|" + r.Name
+}
+
+func valuesFromKey(v fleet.ValuesFrom) string {
+	if v.ConfigMapKeyRef != nil {
+		return "configmap|" + v.ConfigMapKeyRef.Namespace + "|" + v.ConfigMapKeyRef.Name + "|" + v.ConfigMapKeyRef.Key
+	}
+	if v.SecretKeyRef != nil {
+		return "secret|" + v.SecretKeyRef.Namespace + "|" + v.SecretKeyRef.Name + "|" + v.SecretKeyRef.Key
+	}
+	return "" // no stable identity; always append
+}
+
+func comparePatchKey(p fleet.ComparePatch) string {
+	return p.APIVersion + "|" + p.Kind + "|" + p.Namespace + "|" + p.Name
+}
+
 // DeploymentID hashes the options to a string
 func DeploymentID(manifestID string, opts fleet.BundleDeploymentOptions) (string, error) {
 	h := sha256.New()
@@ -64,8 +90,8 @@ func Merge(base, custom fleet.BundleDeploymentOptions) fleet.BundleDeploymentOpt
 		} else if custom.Helm.TemplateValues != nil {
 			maps.Copy(result.Helm.TemplateValues, custom.Helm.TemplateValues)
 		}
-		if custom.Helm.ValuesFrom != nil {
-			result.Helm.ValuesFrom = append(result.Helm.ValuesFrom, custom.Helm.ValuesFrom...)
+		if len(custom.Helm.ValuesFrom) > 0 {
+			result.Helm.ValuesFrom = mergeUnique(result.Helm.ValuesFrom, custom.Helm.ValuesFrom, valuesFromKey)
 		}
 		if custom.Helm.Repo != "" {
 			result.Helm.Repo = custom.Helm.Repo
@@ -98,7 +124,7 @@ func Merge(base, custom fleet.BundleDeploymentOptions) fleet.BundleDeploymentOpt
 		if result.Diff == nil {
 			result.Diff = &fleet.DiffOptions{}
 		}
-		result.Diff.ComparePatches = append(result.Diff.ComparePatches, custom.Diff.ComparePatches...)
+		result.Diff.ComparePatches = mergeUnique(result.Diff.ComparePatches, custom.Diff.ComparePatches, comparePatchKey)
 	}
 	if custom.YAML != nil {
 		if result.YAML == nil {
@@ -114,7 +140,7 @@ func Merge(base, custom fleet.BundleDeploymentOptions) fleet.BundleDeploymentOpt
 		result.CorrectDrift = custom.CorrectDrift
 	}
 	if len(custom.DownstreamResources) > 0 {
-		result.DownstreamResources = append(result.DownstreamResources, custom.DownstreamResources...)
+		result.DownstreamResources = mergeUnique(result.DownstreamResources, custom.DownstreamResources, downstreamResourceKey)
 	}
 
 	return result