From fb3e87c9c14f9bd2a2758b1d7e6da7ad7e2371d3 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Thu, 2 Apr 2026 08:58:04 +0200 Subject: [PATCH 1/3] feat: add TargetCustomizationMode with FirstMatch and AllMatches options Introduces TargetCustomizationMode on BundleSpec and fleet.yaml. The default "FirstMatch" preserves existing behaviour. "AllMatches" applies all matching targetCustomizations in order, merging their options into the base deployment options. Customization targets are now identified via hasMatchingRestriction() rather than relying on their position in the Targets slice. --- charts/fleet-crd/templates/crds.yaml | 96 +++++- .../cli/apply/apply_online_test.go | 1 + .../cli/apply/targetsfile_test.go | 4 +- .../controller/bundle/bundle_targets_test.go | 67 ++++ .../helmops/controller/controller_test.go | 14 +- internal/bundlereader/read.go | 14 +- internal/bundlereader/read_test.go | 48 +++ .../helmops/reconciler/helmop_controller.go | 1 + .../cmd/controller/options/calculate_test.go | 125 ++++++++ internal/cmd/controller/target/builder.go | 11 +- .../controller/target/matcher/bundlematch.go | 98 +++++- .../target/matcher/bundlematch_test.go | 303 ++++++++++++++++++ .../fleet.cattle.io/v1alpha1/bundle_types.go | 38 ++- .../fleet.cattle.io/v1alpha1/fleetyaml.go | 6 +- 14 files changed, 794 insertions(+), 32 deletions(-) create mode 100644 internal/bundlereader/read_test.go create mode 100644 internal/cmd/controller/options/calculate_test.go create mode 100644 internal/cmd/controller/target/matcher/bundlematch_test.go diff --git a/charts/fleet-crd/templates/crds.yaml b/charts/fleet-crd/templates/crds.yaml index 4280f0a2a2..ffd9e33155 100644 --- a/charts/fleet-crd/templates/crds.yaml +++ b/charts/fleet-crd/templates/crds.yaml @@ -2461,6 +2461,19 @@ spec: description: ServiceAccount which will be used to perform this deployment. nullable: true type: string + targetCustomizationMode: + default: FirstMatch + description: 'TargetCustomizationMode controls how targetCustomizations + from fleet.yaml + + are evaluated. "FirstMatch" (default) stops at the first matching + entry. + + "AllMatches" applies all matching entries in order, merging them.' + enum: + - FirstMatch + - AllMatches + type: string targetRestrictions: description: TargetRestrictions is an allow list, which controls if a bundledeployment is created for a target. @@ -2468,10 +2481,10 @@ spec: description: 'BundleTargetRestriction is used internally by Fleet and should not be modified. - It acts as an allow list, to prevent the creation of BundleDeployments - from + It acts as an allow list, restricting BundleDeployment creation + to only those clusters - Targets created by TargetCustomizations in fleet.yaml.' + that are explicitly listed in the GitRepo targets.' properties: clusterGroup: nullable: true @@ -3198,6 +3211,35 @@ spec: this deployment. nullable: true type: string + source: + description: 'Source indicates the origin of this target. + + "customization" - target comes from fleet.yaml targetCustomizations + + "gitrepo" - target comes from GitRepo.Spec.Targets (via + targets file) + + + If empty, provenance is determined by position in the Targets + array: + + - First N targets are customizations where N = len(Targets) + - len(TargetRestrictions) + + - Remaining targets are GitRepo targets + + + This field enables explicit provenance tracking for better + maintainability + + while maintaining backward compatibility with Bundles created + before this field existed.' + enum: + - customization + - gitrepo + - helmop + - '' + type: string yaml: description: 'YAML options, if using raw YAML these are names that map to @@ -8708,6 +8750,19 @@ spec: description: ServiceAccount which will be used to perform this deployment. nullable: true type: string + targetCustomizationMode: + default: FirstMatch + description: 'TargetCustomizationMode controls how targetCustomizations + from fleet.yaml + + are evaluated. "FirstMatch" (default) stops at the first matching + entry. + + "AllMatches" applies all matching entries in order, merging them.' + enum: + - FirstMatch + - AllMatches + type: string targetRestrictions: description: TargetRestrictions is an allow list, which controls if a bundledeployment is created for a target. @@ -8715,10 +8770,10 @@ spec: description: 'BundleTargetRestriction is used internally by Fleet and should not be modified. - It acts as an allow list, to prevent the creation of BundleDeployments - from + It acts as an allow list, restricting BundleDeployment creation + to only those clusters - Targets created by TargetCustomizations in fleet.yaml.' + that are explicitly listed in the GitRepo targets.' properties: clusterGroup: nullable: true @@ -9445,6 +9500,35 @@ spec: this deployment. nullable: true type: string + source: + description: 'Source indicates the origin of this target. + + "customization" - target comes from fleet.yaml targetCustomizations + + "gitrepo" - target comes from GitRepo.Spec.Targets (via + targets file) + + + If empty, provenance is determined by position in the Targets + array: + + - First N targets are customizations where N = len(Targets) + - len(TargetRestrictions) + + - Remaining targets are GitRepo targets + + + This field enables explicit provenance tracking for better + maintainability + + while maintaining backward compatibility with Bundles created + before this field existed.' + enum: + - customization + - gitrepo + - helmop + - '' + type: string yaml: description: 'YAML options, if using raw YAML these are names that map to diff --git a/integrationtests/cli/apply/apply_online_test.go b/integrationtests/cli/apply/apply_online_test.go index 32eca9ca85..7bea26c59d 100644 --- a/integrationtests/cli/apply/apply_online_test.go +++ b/integrationtests/cli/apply/apply_online_test.go @@ -105,6 +105,7 @@ data: { Name: "default", ClusterGroup: "default", + Source: "gitrepo", }, }, }, diff --git a/integrationtests/cli/apply/targetsfile_test.go b/integrationtests/cli/apply/targetsfile_test.go index 281c000000..6b8752c738 100644 --- a/integrationtests/cli/apply/targetsfile_test.go +++ b/integrationtests/cli/apply/targetsfile_test.go @@ -59,7 +59,7 @@ var _ = Describe("Fleet apply targets", func() { ) BeforeEach(func() { - targets = []fleet.BundleTarget{{Name: "target1", ClusterName: "test1"}} + targets = []fleet.BundleTarget{{Name: "target1", ClusterName: "test1", Source: "gitrepo"}} targetRestrictions = []fleet.BundleTargetRestriction{{Name: "target1", ClusterName: "test1"}} file := createTargetsFile(targets, targetRestrictions) options = apply.Options{TargetsFile: file.Name()} @@ -81,7 +81,7 @@ var _ = Describe("Fleet apply targets", func() { ) BeforeEach(func() { - targets = []fleet.BundleTarget{{Name: "target1", ClusterName: "test1"}} + targets = []fleet.BundleTarget{{Name: "target1", ClusterName: "test1", Source: "gitrepo"}} targetRestrictions = []fleet.BundleTargetRestriction{{Name: "target1", ClusterName: "test1"}} file := createTargetsFile(targets, targetRestrictions) options = apply.Options{TargetsFile: file.Name()} diff --git a/integrationtests/controller/bundle/bundle_targets_test.go b/integrationtests/controller/bundle/bundle_targets_test.go index a7cae80ed0..b5f63c3855 100644 --- a/integrationtests/controller/bundle/bundle_targets_test.go +++ b/integrationtests/controller/bundle/bundle_targets_test.go @@ -588,6 +588,73 @@ var _ = Describe("Bundle targets", Ordered, func() { } }) }) + + // AllMatches mode: every matching targetCustomization is applied in order, + // so a cluster that matches two customizations receives the merged options + // from both, whereas FirstMatch would stop at the first. + When("TargetCustomizationMode is AllMatches and two customizations match cluster one", func() { + BeforeEach(func() { + bundleName = "all-matches-mode" + bdLabels = map[string]string{ + "fleet.cattle.io/bundle-name": bundleName, + "fleet.cattle.io/bundle-namespace": namespace, + } + expectedNumberOfBundleDeployments = 3 + + // Customization 1: only cluster group "one" → sets region + // Customization 2: all clusters → sets env + // GitRepo target: all clusters (no extra values) + targetsInGitRepo := []v1alpha1.BundleTarget{ + {ClusterGroup: "all"}, + } + targets = []v1alpha1.BundleTarget{ + { + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + Helm: &v1alpha1.HelmOptions{ + Values: &v1alpha1.GenericMap{Data: map[string]interface{}{"region": "us-west"}}, + }, + }, + ClusterGroup: "one", + }, + { + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + Helm: &v1alpha1.HelmOptions{ + Values: &v1alpha1.GenericMap{Data: map[string]interface{}{"env": "prod"}}, + }, + }, + ClusterGroup: "all", + }, + } + targetRestrictions = make([]v1alpha1.BundleTarget, len(targetsInGitRepo)) + copy(targetRestrictions, targetsInGitRepo) + targets = append(targets, targetsInGitRepo...) + }) + + JustBeforeEach(func() { + // The outer JustBeforeEach already created the bundle with FirstMatch + // (the default). Patch it to AllMatches so the reconciler re-evaluates. + mod := bundle.DeepCopy() + mod.Spec.TargetCustomizationMode = v1alpha1.TargetCustomizationModeAllMatches + Expect(k8sClient.Patch(ctx, mod, client.MergeFrom(bundle))).ToNot(HaveOccurred()) + bundle = mod + }) + + It("merges all matching customizations into cluster one's BundleDeployment", func() { + bdList := verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName) + for _, bd := range bdList.Items { + values, _ := loadValues(bd) + if strings.Contains(bd.Namespace, "cluster-one") { + // cluster "one" matches both customizations: both keys must be present + Expect(values).To(HaveKeyWithValue("region", "us-west"), "cluster-one should have region from cust1") + Expect(values).To(HaveKeyWithValue("env", "prod"), "cluster-one should have env from cust2") + } else { + // other clusters match only cust2 + Expect(values).ToNot(HaveKey("region"), "non-one clusters should not have region") + Expect(values).To(HaveKeyWithValue("env", "prod"), "non-one clusters should have env from cust2") + } + } + }) + }) }) func verifyBundlesDeploymentsAreCreated(numBundleDeployments int, bdLabels map[string]string, bundleName string) *v1alpha1.BundleDeploymentList { diff --git a/integrationtests/helmops/controller/controller_test.go b/integrationtests/helmops/controller/controller_test.go index 15e761b490..a53ae817c0 100644 --- a/integrationtests/helmops/controller/controller_test.go +++ b/integrationtests/helmops/controller/controller_test.go @@ -406,6 +406,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } @@ -455,6 +456,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } @@ -485,6 +487,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } checkBundleIsAsExpected(g, *bundle, helmop, t) @@ -587,6 +590,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } @@ -652,6 +656,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -676,6 +681,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -704,6 +710,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -1023,6 +1030,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -1202,6 +1210,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -1276,6 +1285,7 @@ var _ = Describe("HelmOps controller", func() { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } // the original helmop has no version defined. @@ -1316,7 +1326,7 @@ var _ = Describe("HelmOps controller", func() { ns := types.NamespacedName{Name: helmop.Name, Namespace: helmop.Namespace} err := k8sClient.Get(ctx, ns, bundle) g.Expect(err).ToNot(HaveOccurred()) - t := []fleet.BundleTarget{{Name: "default", ClusterGroup: "default"}} + t := []fleet.BundleTarget{{Name: "default", ClusterGroup: "default", Source: "helmop"}} helmop.Spec.Helm.Version = "0.2.0" checkBundleIsAsExpected(g, *bundle, helmop, t) }).Should(Succeed()) @@ -1350,7 +1360,7 @@ var _ = Describe("HelmOps controller", func() { ns := types.NamespacedName{Name: helmop.Name, Namespace: helmop.Namespace} err := k8sClient.Get(ctx, ns, bundle) g.Expect(err).ToNot(HaveOccurred()) - t := []fleet.BundleTarget{{Name: "default", ClusterGroup: "default"}} + t := []fleet.BundleTarget{{Name: "default", ClusterGroup: "default", Source: "helmop"}} helmop.Spec.Helm.Version = "0.2.0" checkBundleIsAsExpected(g, *bundle, helmop, t) }).Should(Succeed()) diff --git a/internal/bundlereader/read.go b/internal/bundlereader/read.go index 00c082e0d4..33daf4bdb4 100644 --- a/internal/bundlereader/read.go +++ b/internal/bundlereader/read.go @@ -184,7 +184,11 @@ func bundleFromDir(ctx context.Context, name, baseDir string, bundleData []byte, }) } - fy.Targets = append(fy.Targets, fy.TargetCustomizations...) + // Mark targetCustomizations with explicit source + for _, tc := range fy.TargetCustomizations { + tc.Source = "customization" + fy.Targets = append(fy.Targets, tc) + } meta, err := readMetadata(bundleData) if err != nil { @@ -244,6 +248,7 @@ func bundleFromDir(ctx context.Context, name, baseDir string, bundleData []byte, ClusterSelector: target.ClusterSelector, ClusterGroup: target.ClusterGroup, ClusterGroupSelector: target.ClusterGroupSelector, + Source: "gitrepo", // OverrideTargets replace GitRepo targets }) bundle.Spec.TargetRestrictions = append(bundle.Spec.TargetRestrictions, fleet.BundleTargetRestriction(target)) } @@ -259,6 +264,7 @@ func bundleFromDir(ctx context.Context, name, baseDir string, bundleData []byte, { Name: "default", ClusterGroup: "default", + Source: "gitrepo", }, } } @@ -328,7 +334,11 @@ func appendTargets(def *fleet.Bundle, targetsFile string) (*fleet.Bundle, error) return nil, err } - def.Spec.Targets = append(def.Spec.Targets, spec.Targets...) + // Mark GitRepo targets with explicit source + for _, target := range spec.Targets { + target.Source = "gitrepo" + def.Spec.Targets = append(def.Spec.Targets, target) + } def.Spec.TargetRestrictions = append(def.Spec.TargetRestrictions, spec.TargetRestrictions...) return def, nil diff --git a/internal/bundlereader/read_test.go b/internal/bundlereader/read_test.go new file mode 100644 index 0000000000..fd49f1d22e --- /dev/null +++ b/internal/bundlereader/read_test.go @@ -0,0 +1,48 @@ +package bundlereader + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +// TestBundleFromDir_TargetCustomizationModePropagate verifies that the +// targetCustomizationMode field from fleet.yaml is correctly unmarshalled into +// bundle.Spec.TargetCustomizationMode via the embedded BundleSpec. +func TestBundleFromDir_TargetCustomizationModePropagate(t *testing.T) { + dir := t.TempDir() + + tests := []struct { + name string + yaml string + wantMode fleet.TargetCustomizationMode + }{ + { + name: "AllMatches is propagated to BundleSpec", + yaml: `targetCustomizationMode: AllMatches`, + wantMode: fleet.TargetCustomizationModeAllMatches, + }, + { + name: "FirstMatch is propagated to BundleSpec", + yaml: `targetCustomizationMode: FirstMatch`, + wantMode: fleet.TargetCustomizationModeFirstMatch, + }, + { + name: "omitted mode propagates as empty string (controller uses default)", + yaml: `namespace: test`, + wantMode: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + bundle, _, err := bundleFromDir(context.Background(), "test", dir, []byte(tt.yaml), nil) + require.NoError(t, err) + assert.Equal(t, tt.wantMode, bundle.Spec.TargetCustomizationMode) + }) + } +} diff --git a/internal/cmd/controller/helmops/reconciler/helmop_controller.go b/internal/cmd/controller/helmops/reconciler/helmop_controller.go index 59b6ef8724..feb0f600ec 100644 --- a/internal/cmd/controller/helmops/reconciler/helmop_controller.go +++ b/internal/cmd/controller/helmops/reconciler/helmop_controller.go @@ -227,6 +227,7 @@ func (r *HelmOpReconciler) calculateBundle(helmop *fleet.HelmOp) *fleet.Bundle { { Name: "default", ClusterGroup: "default", + Source: "helmop", }, } } diff --git a/internal/cmd/controller/options/calculate_test.go b/internal/cmd/controller/options/calculate_test.go new file mode 100644 index 0000000000..cba74237ca --- /dev/null +++ b/internal/cmd/controller/options/calculate_test.go @@ -0,0 +1,125 @@ +package options + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +// TestMergeChain verifies that chaining Merge calls (as done in AllMatches mode) +// correctly accumulates values from multiple customizations. +func TestMergeChain(t *testing.T) { + base := fleet.BundleDeploymentOptions{ + DefaultNamespace: "base-ns", + Helm: &fleet.HelmOptions{ + ReleaseName: "base-release", + Values: &fleet.GenericMap{ + Data: map[string]any{ + "source": "base", + "edge": "false", + "extra": "false", + }, + }, + }, + } + + edgeCustomization := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{ + Values: &fleet.GenericMap{ + Data: map[string]any{ + "source": "edge", + "edge": "true", + }, + }, + }, + } + + extraCustomization := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{ + Values: &fleet.GenericMap{ + Data: map[string]any{ + "source": "extra", + "extra": "true", + }, + }, + }, + } + + // Simulate AllMatches: base -> edge -> extra + result := Merge(base, edgeCustomization) + result = Merge(result, extraCustomization) + + assert.Equal(t, "base-ns", result.DefaultNamespace, "namespace from base should be preserved") + assert.Equal(t, "base-release", result.Helm.ReleaseName, "releaseName from base should be preserved") + assert.Equal(t, "extra", result.Helm.Values.Data["source"], "last matching customization wins for scalar values") + assert.Equal(t, "true", result.Helm.Values.Data["edge"], "edge value set by first customization should be present") + assert.Equal(t, "true", result.Helm.Values.Data["extra"], "extra value set by second customization should be present") +} + +// TestMergeChain_ValuesFrom verifies that ValuesFrom entries are accumulated +// across multiple customizations. +func TestMergeChain_ValuesFrom(t *testing.T) { + base := fleet.BundleDeploymentOptions{} + + first := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{ + ValuesFrom: []fleet.ValuesFrom{ + {ConfigMapKeyRef: &fleet.ConfigMapKeySelector{LocalObjectReference: fleet.LocalObjectReference{Name: "cm-edge"}}}, + }, + }, + } + + second := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{ + ValuesFrom: []fleet.ValuesFrom{ + {ConfigMapKeyRef: &fleet.ConfigMapKeySelector{LocalObjectReference: fleet.LocalObjectReference{Name: "cm-extra"}}}, + }, + }, + } + + result := Merge(base, first) + result = Merge(result, second) + + assert.Len(t, result.Helm.ValuesFrom, 2, "ValuesFrom entries should be accumulated") + assert.Equal(t, "cm-edge", result.Helm.ValuesFrom[0].ConfigMapKeyRef.Name) + assert.Equal(t, "cm-extra", result.Helm.ValuesFrom[1].ConfigMapKeyRef.Name) +} + +// TestMergeChain_ScalarLastWins verifies that for scalar fields (namespace, +// releaseName) the last non-empty customization wins. +func TestMergeChain_ScalarLastWins(t *testing.T) { + base := fleet.BundleDeploymentOptions{ + DefaultNamespace: "base-ns", + } + first := fleet.BundleDeploymentOptions{ + DefaultNamespace: "first-ns", + } + second := fleet.BundleDeploymentOptions{ + DefaultNamespace: "second-ns", + } + + result := Merge(base, first) + result = Merge(result, second) + assert.Equal(t, "second-ns", result.DefaultNamespace, "last non-empty namespace wins") +} + +// TestMergeChain_BoolSticky verifies that OR'd booleans are sticky once set true. +// +// TODO we might want to rethink that approach, but for now this verifies that +// if one customization sets Force to true, it cannot be unset by a later +// customization. +func TestMergeChain_BoolSticky(t *testing.T) { + base := fleet.BundleDeploymentOptions{} + setForce := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{Force: true}, + } + clearForce := fleet.BundleDeploymentOptions{ + Helm: &fleet.HelmOptions{Force: false}, + } + + result := Merge(base, setForce) + result = Merge(result, clearForce) + assert.True(t, result.Helm.Force, "Force cannot be unset once true -- OR semantics") +} diff --git a/internal/cmd/controller/target/builder.go b/internal/cmd/controller/target/builder.go index 2be607c75b..53bfcd7dc3 100644 --- a/internal/cmd/controller/target/builder.go +++ b/internal/cmd/controller/target/builder.go @@ -101,9 +101,14 @@ func (m *Manager) Targets(ctx context.Context, bundle *fleet.Bundle, manifestID } // check if there is any matching targetCustomization that should be applied targetOpts := target.BundleDeploymentOptions - targetCustomized := bm.MatchTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) - if targetCustomized != nil { - targetOpts = targetCustomized.BundleDeploymentOptions + if bundle.Spec.TargetCustomizationMode == fleet.TargetCustomizationModeAllMatches { + for _, tc := range bm.MatchAllTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) { + targetOpts = options.Merge(targetOpts, tc.BundleDeploymentOptions) + } + } else { + if targetCustomized := bm.MatchTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels); targetCustomized != nil { + targetOpts = targetCustomized.BundleDeploymentOptions + } } opts := options.Merge(bundle.Spec.BundleDeploymentOptions, targetOpts) diff --git a/internal/cmd/controller/target/matcher/bundlematch.go b/internal/cmd/controller/target/matcher/bundlematch.go index 89b6f6e3e7..ddcea66898 100644 --- a/internal/cmd/controller/target/matcher/bundlematch.go +++ b/internal/cmd/controller/target/matcher/bundlematch.go @@ -30,8 +30,8 @@ func (a *BundleMatch) MatchForTarget(name string) *fleet.BundleTarget { } // Match returns the first BundleTarget that matches the target criteria. Targets are evaluated in order. -// It checks for restrictions, which means that just targets included in the GitRepo can be returned. TargetCustomizations -// described in the fleet.yaml will be ignored. +// It checks for restrictions, which means that just targets included in the GitRepo can be returned. +// TargetCustomizations described in the fleet.yaml will be ignored. // All GitRepo targets are added as TargetRestrictions, which acts as a whitelist. func (a *BundleMatch) Match(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) *fleet.BundleTarget { if m := a.matcher.match(clusterName, clusterLabels, clusterGroups, a.matcher.criteriaWithRestrictions); m != nil { @@ -44,17 +44,41 @@ func (a *BundleMatch) Match(clusterName string, clusterGroups map[string]map[str // MatchTargetCustomizations returns the first BundleTarget that matches the target criteria. Targets are evaluated in order. // It doesn't check for restrictions, which means TargetCustomizations described in the fleet.yaml are considered. func (a *BundleMatch) MatchTargetCustomizations(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) *fleet.BundleTarget { - if m := a.matcher.match(clusterName, clusterLabels, clusterGroups, criteriaWithoutRestrictions); m != nil { + if m := a.matcher.matchCustomization(clusterName, clusterLabels, clusterGroups); m != nil { return m } return nil } +// MatchAllTargetCustomizations returns all BundleTargets marked as customizations that match the target criteria, in list order. +// Used when TargetCustomizationMode is AllMatches. +func (a *BundleMatch) MatchAllTargetCustomizations(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) []*fleet.BundleTarget { + var result []*fleet.BundleTarget + for _, tm := range a.matcher.matches { + if !tm.isCustomization { + continue + } + if len(clusterGroups) == 0 { + if tm.criteria.Match(clusterName, "", nil, clusterLabels) { + result = append(result, tm.bundleTarget) + } + } else { + for cg, cgLabels := range clusterGroups { + if tm.criteria.Match(clusterName, cg, cgLabels, clusterLabels) { + result = append(result, tm.bundleTarget) + break // matched via one group — don't add the same target multiple times + } + } + } + } + return result +} + // HasDoNotDeployTarget returns true if any bundle target matching the given cluster // has DoNotDeploy set to true. Unlike MatchTargetCustomizations, this scans all matching // bundle targets instead of stopping at the first match, so a doNotDeploy entry does not have to -// appear before a broader matching entry in the target list. +// appear before a broader-matching entry in the target list. func (a *BundleMatch) HasDoNotDeployTarget(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) bool { for _, tm := range a.matcher.matches { if !tm.bundleTarget.DoNotDeploy { @@ -76,8 +100,9 @@ func (a *BundleMatch) HasDoNotDeployTarget(clusterName string, clusterGroups map } type targetMatch struct { - bundleTarget *fleet.BundleTarget - criteria *ClusterMatcher + bundleTarget *fleet.BundleTarget + criteria *ClusterMatcher + isCustomization bool // true when this target comes from fleet.yaml targetCustomizations, not a GitRepo target } type matcher struct { @@ -88,14 +113,20 @@ type matcher struct { func (a *BundleMatch) initMatcher() error { m := &matcher{} + // Calculate position-based split point for fallback + // The first N targets are customizations, where N = total - restrictions + numRestrictions := len(a.bundle.Spec.TargetRestrictions) + numCustomizations := len(a.bundle.Spec.Targets) - numRestrictions + for i, target := range a.bundle.Spec.Targets { clusterMatcher, err := NewClusterMatcher(target.ClusterName, target.ClusterGroup, target.ClusterGroupSelector, target.ClusterSelector) if err != nil { return err } t := targetMatch{ - bundleTarget: &a.bundle.Spec.Targets[i], - criteria: clusterMatcher, + bundleTarget: &a.bundle.Spec.Targets[i], + criteria: clusterMatcher, + isCustomization: determineIsCustomization(target, i, numCustomizations), } m.matches = append(m.matches, t) @@ -113,8 +144,28 @@ func (a *BundleMatch) initMatcher() error { return nil } +// determineIsCustomization uses explicit Source field if present, +// falls back to position-based detection for backward compatibility. +func determineIsCustomization(target fleet.BundleTarget, index int, numCustomizations int) bool { + // NEW BUNDLES: Source field is populated by bundlereader + // This is the preferred method for long-term maintainability + if target.Source != "" { + return target.Source == "customization" + } + + // OLD BUNDLES: Source field is empty (created before this field existed) + // Use position-based detection as fallback: + // - bundlereader appends targetCustomizations first (read.go:187) + // - Then appends GitRepo targets from targets file (read.go:332) + // - Therefore: first N targets are customizations + // where N = len(Targets) - len(TargetRestrictions) + // + // This fixes the collision bug for old Bundles without requiring recreation + return index < numCustomizations +} + func (m *matcher) isRestricted(clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool { - // There are no restrictions. That means this Bundle was not created by a GitRepo, and there are no targetCustomizations + // There are no restrictions. That means this Bundle was not created by a GitRepo. if len(m.restrictions) == 0 { return false } @@ -125,12 +176,11 @@ func (m *matcher) isRestricted(clusterName, clusterGroup string, clusterGroupLab } } - // This target is a targetCustomization from a fleet.yaml return true } -// checks if criteria is matched just if the target is inside the targetRestrictions. This is used for Targets defined -// in the GitRepo, since these targets are also added as targetRestrictions. +// criteriaWithRestrictions checks that the cluster passes the restriction allowlist +// and matches the target's cluster selector. Used for GitRepo targets. func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool { if !m.isRestricted(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) && targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) { @@ -140,7 +190,9 @@ func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName, return false } -// Checks targetMatch's criteria for a match on the specified cluster name, group and labels, without checking if target is inside the targetRestrictions. This is used for TargetCustomizations. +// criteriaWithoutRestrictions checks targetMatch's criteria for a match on the specified cluster +// name, group and labels, without checking if target is inside the targetRestrictions. +// This is used for TargetCustomizations. func criteriaWithoutRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool { return targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) } @@ -160,6 +212,26 @@ func (m *matcher) match(clusterName string, clusterLabels map[string]string, clu } } } + return nil +} +// matchCustomization returns the first customization target that matches the cluster. +func (m *matcher) matchCustomization(clusterName string, clusterLabels map[string]string, clusterGroups map[string]map[string]string) *fleet.BundleTarget { + for _, tm := range m.matches { + if !tm.isCustomization { + continue + } + if len(clusterGroups) == 0 { + if tm.criteria.Match(clusterName, "", nil, clusterLabels) { + return tm.bundleTarget + } + } else { + for clusterGroup, clusterGroupLabels := range clusterGroups { + if tm.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) { + return tm.bundleTarget + } + } + } + } return nil } diff --git a/internal/cmd/controller/target/matcher/bundlematch_test.go b/internal/cmd/controller/target/matcher/bundlematch_test.go new file mode 100644 index 0000000000..d941561d81 --- /dev/null +++ b/internal/cmd/controller/target/matcher/bundlematch_test.go @@ -0,0 +1,303 @@ +package matcher + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +// makeBundle builds a minimal Bundle reflecting the order produced by bundlereader: +// targetCustomizations come first in Targets, followed by the GitRepo target. +// The GitRepo target is also added as a TargetRestriction. +func makeBundle(gitRepoTarget fleet.BundleTarget, customizations []fleet.BundleTarget) *fleet.Bundle { + targets := make([]fleet.BundleTarget, 0, len(customizations)+1) + targets = append(targets, customizations...) + targets = append(targets, gitRepoTarget) + b := &fleet.Bundle{ + Spec: fleet.BundleSpec{ + Targets: targets, + TargetRestrictions: []fleet.BundleTargetRestriction{ + { + Name: gitRepoTarget.Name, + ClusterSelector: gitRepoTarget.ClusterSelector, + }, + }, + }, + } + return b +} + +func labelSelector(labels map[string]string) *metav1.LabelSelector { + return &metav1.LabelSelector{MatchLabels: labels} +} + +// TestMatchAllTargetCustomizations_NoGroups verifies that MatchAllTargetCustomizations +// returns all matching customizations (without restrictions) in order. +func TestMatchAllTargetCustomizations_NoGroups(t *testing.T) { + // gitRepoTarget matches all clusters (catch-all selector), simulating a GitRepo target. + gitRepoTarget := fleet.BundleTarget{Name: "gitrepo", ClusterSelector: &metav1.LabelSelector{}} + + tests := []struct { + name string + customizationTargets []fleet.BundleTarget + clusterLabels map[string]string + wantNames []string + }{ + { + name: "no customizations match", + customizationTargets: []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + }, + clusterLabels: map[string]string{}, + wantNames: nil, + }, + { + name: "one customization matches", + customizationTargets: []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + {Name: "extra", ClusterSelector: labelSelector(map[string]string{"extra": "true"})}, + }, + clusterLabels: map[string]string{"edge": "true"}, + wantNames: []string{"edge"}, + }, + { + name: "both customizations match", + customizationTargets: []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + {Name: "extra", ClusterSelector: labelSelector(map[string]string{"extra": "true"})}, + }, + clusterLabels: map[string]string{"edge": "true", "extra": "true"}, + wantNames: []string{"edge", "extra"}, + }, + { + name: "order is preserved", + customizationTargets: []fleet.BundleTarget{ + {Name: "extra", ClusterSelector: labelSelector(map[string]string{"extra": "true"})}, + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + }, + clusterLabels: map[string]string{"edge": "true", "extra": "true"}, + wantNames: []string{"extra", "edge"}, + }, + { + name: "catch-all customization matches everything", + customizationTargets: []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + {Name: "all", ClusterSelector: &metav1.LabelSelector{}}, + }, + clusterLabels: map[string]string{"edge": "true"}, + wantNames: []string{"edge", "all"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + bm, err := New(makeBundle(gitRepoTarget, tt.customizationTargets)) + require.NoError(t, err) + + got := bm.MatchAllTargetCustomizations("local", nil, tt.clusterLabels) + + var gotNames []string + for _, bt := range got { + gotNames = append(gotNames, bt.Name) + } + assert.Equal(t, tt.wantNames, gotNames) + }) + } +} + +// TestMatchAllTargetCustomizations_DoNotDeployIncluded verifies that customization targets +// with DoNotDeploy set are still returned by MatchAllTargetCustomizations — the caller +// (builder.go) is responsible for handling doNotDeploy via HasDoNotDeployTarget. +// The GitRepo target is excluded because it matches both with and without restrictions. +func TestMatchAllTargetCustomizations_DoNotDeployIncluded(t *testing.T) { + gitRepoTarget := fleet.BundleTarget{Name: "gitrepo", ClusterSelector: &metav1.LabelSelector{}} + customizations := []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + {Name: "stopper", DoNotDeploy: true, ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + } + bm, err := New(makeBundle(gitRepoTarget, customizations)) + require.NoError(t, err) + + got := bm.MatchAllTargetCustomizations("local", nil, map[string]string{"edge": "true"}) + var names []string + for _, g := range got { + names = append(names, g.Name) + } + // Both "edge" and "stopper" match as customizations; gitrepo is excluded. + // HasDoNotDeployTarget is what gates actual deployment. + assert.Equal(t, []string{"edge", "stopper"}, names) +} + +// TestMatchTargetCustomizations_StillFirstMatch confirms the original method is unchanged: +// with the correct ordering (customizations first, gitrepo last), it returns the first +// matching customization — stopping there and never reaching the second. +func TestMatchTargetCustomizations_StillFirstMatch(t *testing.T) { + gitRepoTarget := fleet.BundleTarget{Name: "gitrepo", ClusterSelector: &metav1.LabelSelector{}} + customizations := []fleet.BundleTarget{ + {Name: "edge", ClusterSelector: labelSelector(map[string]string{"edge": "true"})}, + {Name: "extra", ClusterSelector: labelSelector(map[string]string{"extra": "true"})}, + } + bm, err := New(makeBundle(gitRepoTarget, customizations)) + require.NoError(t, err) + + got := bm.MatchTargetCustomizations("local", nil, map[string]string{"edge": "true", "extra": "true"}) + require.NotNil(t, got) + // "edge" is the first customization in the list and matches — "extra" is never reached. + assert.Equal(t, "edge", got.Name, "MatchTargetCustomizations returns first match without restrictions") +} + +// TestCustomizationWithSameSelectorsAsGitRepoTarget verifies the fix for the bug where +// a fleet.yaml targetCustomization that shares the same (Name, ClusterSelector, etc.) +// with a GitRepo target was incorrectly classified as a GitRepo target and skipped. +// +// This test verifies that customizations are applied based on their provenance +// (fleet.yaml vs GitRepo), not just selector equality. The hybrid approach uses +// position-based detection for old Bundles (without Source field), correctly +// identifying the first target as a customization even when selectors match. +func TestCustomizationWithSameSelectorsAsGitRepoTarget(t *testing.T) { + // GitRepo target: deploys to clusters with env=prod + gitRepoTarget := fleet.BundleTarget{ + Name: "production", + ClusterSelector: labelSelector(map[string]string{"env": "prod"}), + // Source field not set, simulating an old Bundle + } + + // fleet.yaml targetCustomization: has IDENTICAL selectors to gitRepoTarget, + // but contains BundleDeploymentOptions (e.g., different Helm values). + // Even though selectors match, this should still be treated as a customization. + customization := fleet.BundleTarget{ + Name: "production", // same name + ClusterSelector: labelSelector(map[string]string{"env": "prod"}), // same selector + // Source field not set, simulating an old Bundle + // Position-based fallback: index 0 < numCustomizations (1), so this is a customization + } + + // Build bundle with customization first (as bundlereader does), then GitRepo target + bundle := &fleet.Bundle{ + Spec: fleet.BundleSpec{ + Targets: []fleet.BundleTarget{customization, gitRepoTarget}, + TargetRestrictions: []fleet.BundleTargetRestriction{ + { + Name: gitRepoTarget.Name, + ClusterSelector: gitRepoTarget.ClusterSelector, + }, + }, + }, + } + + bm, err := New(bundle) + require.NoError(t, err) + + clusterLabels := map[string]string{"env": "prod"} + + t.Run("MatchTargetCustomizations should return the customization", func(t *testing.T) { + // Position-based detection correctly identifies index 0 as a customization + // even though it has identical selectors to the GitRepo target + got := bm.MatchTargetCustomizations("cluster1", nil, clusterLabels) + require.NotNil(t, got, "customization should match even when it has same selectors as GitRepo target") + assert.Equal(t, "production", got.Name) + }) + + t.Run("MatchAllTargetCustomizations should include the customization", func(t *testing.T) { + // Position-based detection correctly includes the customization at index 0 + got := bm.MatchAllTargetCustomizations("cluster1", nil, clusterLabels) + require.Len(t, got, 1, "should return the customization even when selectors match a GitRepo target") + assert.Equal(t, "production", got[0].Name) + }) +} + +func TestDetermineIsCustomization_Hybrid(t *testing.T) { + labelSelector := func(m map[string]string) *metav1.LabelSelector { + return &metav1.LabelSelector{MatchLabels: m} + } + + tests := []struct { + name string + target fleet.BundleTarget + index int + numCustomizations int + want bool + }{ + { + name: "new bundle - explicit customization source", + target: fleet.BundleTarget{Name: "edge", Source: "customization"}, + index: 0, + numCustomizations: 1, + want: true, + }, + { + name: "new bundle - explicit gitrepo source", + target: fleet.BundleTarget{Name: "prod", Source: "gitrepo"}, + index: 1, + numCustomizations: 1, + want: false, + }, + { + name: "old bundle - position-based customization", + target: fleet.BundleTarget{Name: "edge", Source: ""}, // empty + index: 0, + numCustomizations: 1, + want: true, // index < numCustomizations + }, + { + name: "old bundle - position-based gitrepo target", + target: fleet.BundleTarget{Name: "prod", Source: ""}, // empty + index: 1, + numCustomizations: 1, + want: false, // index >= numCustomizations + }, + { + name: "collision case - old bundle position fixes bug", + target: fleet.BundleTarget{Name: "prod", ClusterSelector: labelSelector(map[string]string{"env": "prod"}), Source: ""}, + index: 0, // First in list + numCustomizations: 1, + want: true, // Treated as customization via position, bug fixed! + }, + { + name: "new bundle - explicit source overrides position", + target: fleet.BundleTarget{Name: "test", Source: "gitrepo"}, + index: 0, // Position suggests customization + numCustomizations: 1, + want: false, // But Source field says gitrepo, so not a customization + }, + { + name: "old bundle - no customizations (all gitrepo)", + target: fleet.BundleTarget{Name: "target", Source: ""}, + index: 0, + numCustomizations: 0, // All targets are gitrepo targets + want: false, + }, + { + name: "old bundle - multiple customizations", + target: fleet.BundleTarget{Name: "custom2", Source: ""}, + index: 2, // Third target (index 2) + numCustomizations: 3, // First 3 are customizations + want: true, + }, + { + name: "old bundle - boundary case (last customization)", + target: fleet.BundleTarget{Name: "last-custom", Source: ""}, + index: 2, // Third target (index 2) + numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2) + want: true, + }, + { + name: "old bundle - boundary case (first gitrepo target)", + target: fleet.BundleTarget{Name: "first-gitrepo", Source: ""}, + index: 3, // Fourth target (index 3) + numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2) + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go index 9418224bad..63a39d2f0c 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go @@ -112,6 +112,13 @@ type BundleSpec struct { // +nullable Resources []BundleResource `json:"resources,omitempty"` + // TargetCustomizationMode controls how targetCustomizations from fleet.yaml + // are evaluated. "FirstMatch" (default) stops at the first matching entry. + // "AllMatches" applies all matching entries in order, merging them. + // +kubebuilder:default=FirstMatch + // +kubebuilder:validation:Enum=FirstMatch;AllMatches + TargetCustomizationMode TargetCustomizationMode `json:"targetCustomizationMode,omitempty"` + // Targets refer to the clusters which will be deployed to. // Targets are evaluated in order and the first one to match is used. Targets []BundleTarget `json:"targets,omitempty"` @@ -234,8 +241,8 @@ type Partition struct { } // BundleTargetRestriction is used internally by Fleet and should not be modified. -// It acts as an allow list, to prevent the creation of BundleDeployments from -// Targets created by TargetCustomizations in fleet.yaml. +// It acts as an allow list, restricting BundleDeployment creation to only those clusters +// that are explicitly listed in the GitRepo targets. type BundleTargetRestriction struct { // +nullable Name string `json:"name,omitempty"` @@ -281,6 +288,21 @@ type BundleTarget struct { // NamespaceAnnotations are annotations that will be appended to the namespace created by Fleet. // +nullable NamespaceAnnotations map[string]string `json:"namespaceAnnotations,omitempty"` + + // Source indicates the origin of this target. + // "customization" - target comes from fleet.yaml targetCustomizations + // "gitrepo" - target comes from GitRepo.Spec.Targets (via targets file) + // "helmop" - target comes from a HelmOp resource + // + // If empty, provenance is determined by position in the Targets array: + // - First N targets are customizations where N = len(Targets) - len(TargetRestrictions) + // - Remaining targets are GitRepo targets + // + // This field enables explicit provenance tracking for better maintainability + // while maintaining backward compatibility with Bundles created before this field existed. + // +optional + // +kubebuilder:validation:Enum=customization;gitrepo;helmop;"" + Source string `json:"source,omitempty"` } // BundleSummary contains the number of bundle deployments in each state and a @@ -343,6 +365,18 @@ type NonReadyResource struct { NonReadyStatus []NonReadyStatus `json:"nonReadyStatus,omitempty"` } +// TargetCustomizationMode controls how targetCustomizations from fleet.yaml are evaluated. +type TargetCustomizationMode string + +const ( + // TargetCustomizationModeFirstMatch stops at the first matching targetCustomization entry. + // This is the default behaviour. + TargetCustomizationModeFirstMatch TargetCustomizationMode = "FirstMatch" + // TargetCustomizationModeAllMatches applies all matching targetCustomization entries + // in order, merging their options. + TargetCustomizationModeAllMatches TargetCustomizationMode = "AllMatches" +) + const ( // BundleConditionReady is unused. A "Ready" condition on a bundle // indicates that its resources are ready and the dependencies are diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/fleetyaml.go b/pkg/apis/fleet.cattle.io/v1alpha1/fleetyaml.go index 932fcd3846..0920995a16 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/fleetyaml.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/fleetyaml.go @@ -11,8 +11,10 @@ type FleetYAML struct { Labels map[string]string `json:"labels,omitempty"` BundleSpec // TargetCustomizations are used to determine how resources should be - // modified per target. Targets are evaluated in order and the first - // one to match a cluster is used for that cluster. + // modified per target. Targets are evaluated in order. By default + // (FirstMatch) the first one to match a cluster is used for that cluster. + // If targetCustomizationMode is set to AllMatches, all matching entries + // are merged in order. TargetCustomizations []BundleTarget `json:"targetCustomizations,omitempty"` // ImageScans are optional and used to update container image // references in the git repo. From 086336598ee4b4435ab36b93beffabf53099eb65 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Mon, 13 Apr 2026 14:15:57 +0200 Subject: [PATCH 2/3] Revert independent doNotDeploy behavior --- .../controller/bundle/bundle_targets_test.go | 64 +++++++++++++++++-- internal/cmd/controller/target/builder.go | 37 +++++++++-- .../controller/target/matcher/bundlematch.go | 49 +++++--------- .../target/matcher/bundlematch_test.go | 21 +++++- 4 files changed, 122 insertions(+), 49 deletions(-) diff --git a/integrationtests/controller/bundle/bundle_targets_test.go b/integrationtests/controller/bundle/bundle_targets_test.go index b5f63c3855..4f4ba0aab1 100644 --- a/integrationtests/controller/bundle/bundle_targets_test.go +++ b/integrationtests/controller/bundle/bundle_targets_test.go @@ -459,9 +459,10 @@ var _ = Describe("Bundle targets", Ordered, func() { }) }) - // Regression test for https://github.com/rancher/fleet/issues/3580: + // With FirstMatch semantics (the default), only the first matching targetCustomization is applied. // When a broader-matching target appears before a doNotDeploy target in the list, - // the doNotDeploy target was previously bypassed due to first-match semantics. + // the broader match wins and the doNotDeploy entry is never evaluated. + // Users must order their targetCustomizations from specific to general to use doNotDeploy effectively. When("a broader-matching targetCustomization appears before a doNotDeploy targetCustomization", func() { BeforeEach(func() { bundleName = "donot-deploy-after-broader-match" @@ -469,12 +470,12 @@ var _ = Describe("Bundle targets", Ordered, func() { "fleet.cattle.io/bundle-name": bundleName, "fleet.cattle.io/bundle-namespace": namespace, } - expectedNumberOfBundleDeployments = 0 + expectedNumberOfBundleDeployments = 1 // simulate targets in fleet.yaml: // - first entry matches all clusters (broader match) // - second entry matches cluster "one" with doNotDeploy=true - // With the old first-match logic, cluster "one" would match the first entry and - // the doNotDeploy entry would never be evaluated. + // With FirstMatch semantics, cluster "one" matches the first entry and gets deployed. + // The doNotDeploy entry is never evaluated because the first match wins. targets = []v1alpha1.BundleTarget{ { BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ @@ -483,16 +484,19 @@ var _ = Describe("Bundle targets", Ordered, func() { }, }, ClusterGroup: "all", + Source: "customization", }, { ClusterGroup: "one", DoNotDeploy: true, + Source: "customization", }, } // simulate targets in GitRepo: only cluster group "one" is targeted targetsInGitRepo := []v1alpha1.BundleTarget{ { ClusterGroup: "one", + Source: "gitrepo", }, } targetRestrictions = make([]v1alpha1.BundleTarget, len(targetsInGitRepo)) @@ -500,8 +504,53 @@ var _ = Describe("Bundle targets", Ordered, func() { targets = append(targets, targetsInGitRepo...) }) - It("no BundleDeployments are created", func() { - waitForBundleToBeReady(bundleName) + It("creates a BundleDeployment because first match wins (using explicit Source field)", func() { + _ = verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName) + }) + }) + + // Same scenario as above, but without Source fields to test backward compatibility + // with bundles created before the Source field was added (position-based fallback). + When("a broader-matching targetCustomization appears before a doNotDeploy targetCustomization (position-based detection)", func() { + BeforeEach(func() { + bundleName = "donot-deploy-after-broader-match-position-based" + bdLabels = map[string]string{ + "fleet.cattle.io/bundle-name": bundleName, + "fleet.cattle.io/bundle-namespace": namespace, + } + expectedNumberOfBundleDeployments = 1 + // simulate targets in fleet.yaml: + // - first entry matches all clusters (broader match) + // - second entry matches cluster "one" with doNotDeploy=true + // With FirstMatch semantics, cluster "one" matches the first entry and gets deployed. + // The doNotDeploy entry is never evaluated because the first match wins. + // No Source fields are set, so position-based detection is used. + targets = []v1alpha1.BundleTarget{ + { + BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{ + Helm: &v1alpha1.HelmOptions{ + Values: &v1alpha1.GenericMap{Data: map[string]interface{}{"replicas": "1"}}, + }, + }, + ClusterGroup: "all", + }, + { + ClusterGroup: "one", + DoNotDeploy: true, + }, + } + // simulate targets in GitRepo: only cluster group "one" is targeted + targetsInGitRepo := []v1alpha1.BundleTarget{ + { + ClusterGroup: "one", + }, + } + targetRestrictions = make([]v1alpha1.BundleTarget, len(targetsInGitRepo)) + copy(targetRestrictions, targetsInGitRepo) + targets = append(targets, targetsInGitRepo...) + }) + + It("creates a BundleDeployment because first match wins (using position-based fallback)", func() { _ = verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName) }) }) @@ -675,6 +724,7 @@ func verifyBundlesDeploymentsAreCreated(numBundleDeployments int, bdLabels map[s } func waitForBundleToBeReady(bundleName string) { + GinkgoHelper() Eventually(func() bool { bundle := &v1alpha1.Bundle{} err := k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: bundleName}, bundle) diff --git a/internal/cmd/controller/target/builder.go b/internal/cmd/controller/target/builder.go index 53bfcd7dc3..1e4f9eecc5 100644 --- a/internal/cmd/controller/target/builder.go +++ b/internal/cmd/controller/target/builder.go @@ -72,6 +72,7 @@ func (m *Manager) Targets(ctx context.Context, bundle *fleet.Bundle, manifestID if err != nil { return nil, false, err } + clusterLoop: for _, cluster := range clusters.Items { logger.V(4).Info("Cluster has namespace?", "cluster", cluster.Name, "namespace", cluster.Status.Namespace) clusterGroups, err := m.clusterGroupsForCluster(ctx, &cluster) @@ -85,28 +86,50 @@ func (m *Manager) Targets(ctx context.Context, bundle *fleet.Bundle, manifestID if target == nil { continue } - // Check all matching targetCustomizations for doNotDeploy, not just the first match. - // This ensures that a doNotDeploy entry is honoured even when a broader-matching - // target appears before it in the target list (fixes first-match bypass). - doNotDeploy := target.DoNotDeploy || bm.HasDoNotDeployTarget(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) - if doNotDeploy { + // Check if the GitRepo target has doNotDeploy set + if target.DoNotDeploy { logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.", "bundle", bundle.Name, "bundleNamespace", bundle.Namespace, "cluster", cluster.Name, "clusterNamespace", cluster.Namespace, - "reason", "doNotDeploy", + "reason", "doNotDeploy on GitRepo target", ) continue } // check if there is any matching targetCustomization that should be applied targetOpts := target.BundleDeploymentOptions if bundle.Spec.TargetCustomizationMode == fleet.TargetCustomizationModeAllMatches { - for _, tc := range bm.MatchAllTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) { + // AllMatches mode: merge all matching customizations + // Check if any matching customization has doNotDeploy=true (OR logic) + matchedCustomizations := bm.MatchAllTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) + for _, tc := range matchedCustomizations { + if tc.DoNotDeploy { + logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.", + "bundle", bundle.Name, + "bundleNamespace", bundle.Namespace, + "cluster", cluster.Name, + "clusterNamespace", cluster.Namespace, + "reason", "doNotDeploy on targetCustomization (AllMatches mode)", + ) + continue clusterLoop + } targetOpts = options.Merge(targetOpts, tc.BundleDeploymentOptions) } } else { + // FirstMatch mode: apply only the first matching customization if targetCustomized := bm.MatchTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels); targetCustomized != nil { + // Check if the first matching targetCustomization has doNotDeploy set + if targetCustomized.DoNotDeploy { + logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.", + "bundle", bundle.Name, + "bundleNamespace", bundle.Namespace, + "cluster", cluster.Name, + "clusterNamespace", cluster.Namespace, + "reason", "doNotDeploy on targetCustomization (FirstMatch mode)", + ) + continue + } targetOpts = targetCustomized.BundleDeploymentOptions } } diff --git a/internal/cmd/controller/target/matcher/bundlematch.go b/internal/cmd/controller/target/matcher/bundlematch.go index ddcea66898..9ea195065a 100644 --- a/internal/cmd/controller/target/matcher/bundlematch.go +++ b/internal/cmd/controller/target/matcher/bundlematch.go @@ -75,30 +75,6 @@ func (a *BundleMatch) MatchAllTargetCustomizations(clusterName string, clusterGr return result } -// HasDoNotDeployTarget returns true if any bundle target matching the given cluster -// has DoNotDeploy set to true. Unlike MatchTargetCustomizations, this scans all matching -// bundle targets instead of stopping at the first match, so a doNotDeploy entry does not have to -// appear before a broader-matching entry in the target list. -func (a *BundleMatch) HasDoNotDeployTarget(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) bool { - for _, tm := range a.matcher.matches { - if !tm.bundleTarget.DoNotDeploy { - continue - } - if len(clusterGroups) == 0 { - if criteriaWithoutRestrictions(tm, clusterName, "", nil, clusterLabels) { - return true - } - } else { - for cg, cgLabels := range clusterGroups { - if criteriaWithoutRestrictions(tm, clusterName, cg, cgLabels, clusterLabels) { - return true - } - } - } - } - return false -} - type targetMatch struct { bundleTarget *fleet.BundleTarget criteria *ClusterMatcher @@ -126,7 +102,7 @@ func (a *BundleMatch) initMatcher() error { t := targetMatch{ bundleTarget: &a.bundle.Spec.Targets[i], criteria: clusterMatcher, - isCustomization: determineIsCustomization(target, i, numCustomizations), + isCustomization: determineIsCustomization(target, i, numCustomizations, numRestrictions), } m.matches = append(m.matches, t) @@ -146,7 +122,7 @@ func (a *BundleMatch) initMatcher() error { // determineIsCustomization uses explicit Source field if present, // falls back to position-based detection for backward compatibility. -func determineIsCustomization(target fleet.BundleTarget, index int, numCustomizations int) bool { +func determineIsCustomization(target fleet.BundleTarget, index int, numCustomizations int, numRestrictions int) bool { // NEW BUNDLES: Source field is populated by bundlereader // This is the preferred method for long-term maintainability if target.Source != "" { @@ -160,7 +136,15 @@ func determineIsCustomization(target fleet.BundleTarget, index int, numCustomiza // - Therefore: first N targets are customizations // where N = len(Targets) - len(TargetRestrictions) // + // SPECIAL CASE: If there are no targetRestrictions, this bundle wasn't + // created by a GitRepo (e.g., CLI-loaded bundles, standalone bundles). + // In this case, treat all targets as regular bundle targets (not customizations) + // to maintain backward compatibility with bundles that predate the Source field. + // // This fixes the collision bug for old Bundles without requiring recreation + if numRestrictions == 0 { + return false + } return index < numCustomizations } @@ -180,8 +164,12 @@ func (m *matcher) isRestricted(clusterName, clusterGroup string, clusterGroupLab } // criteriaWithRestrictions checks that the cluster passes the restriction allowlist -// and matches the target's cluster selector. Used for GitRepo targets. +// and matches the target's cluster selector. Used for GitRepo targets only; +// customization targets (from fleet.yaml) are excluded. func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool { + if targetMatch.isCustomization { + return false + } if !m.isRestricted(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) && targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) { return true @@ -190,13 +178,6 @@ func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName, return false } -// criteriaWithoutRestrictions checks targetMatch's criteria for a match on the specified cluster -// name, group and labels, without checking if target is inside the targetRestrictions. -// This is used for TargetCustomizations. -func criteriaWithoutRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool { - return targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) -} - // match returns the first BundleTarget, from the matcher's target matches, which matches the specified cluster name, groups and labels, using matching logic implemented via findCriteriaMatch. func (m *matcher) match(clusterName string, clusterLabels map[string]string, clusterGroups map[string]map[string]string, findCriteriaMatch findCriteriaMatch) *fleet.BundleTarget { for _, targetMatch := range m.matches { diff --git a/internal/cmd/controller/target/matcher/bundlematch_test.go b/internal/cmd/controller/target/matcher/bundlematch_test.go index d941561d81..17477bcb19 100644 --- a/internal/cmd/controller/target/matcher/bundlematch_test.go +++ b/internal/cmd/controller/target/matcher/bundlematch_test.go @@ -220,6 +220,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target fleet.BundleTarget index int numCustomizations int + numRestrictions int want bool }{ { @@ -227,6 +228,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "edge", Source: "customization"}, index: 0, numCustomizations: 1, + numRestrictions: 1, want: true, }, { @@ -234,6 +236,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "prod", Source: "gitrepo"}, index: 1, numCustomizations: 1, + numRestrictions: 1, want: false, }, { @@ -241,6 +244,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "edge", Source: ""}, // empty index: 0, numCustomizations: 1, + numRestrictions: 1, want: true, // index < numCustomizations }, { @@ -248,6 +252,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "prod", Source: ""}, // empty index: 1, numCustomizations: 1, + numRestrictions: 1, want: false, // index >= numCustomizations }, { @@ -255,6 +260,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "prod", ClusterSelector: labelSelector(map[string]string{"env": "prod"}), Source: ""}, index: 0, // First in list numCustomizations: 1, + numRestrictions: 1, want: true, // Treated as customization via position, bug fixed! }, { @@ -262,6 +268,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "test", Source: "gitrepo"}, index: 0, // Position suggests customization numCustomizations: 1, + numRestrictions: 1, want: false, // But Source field says gitrepo, so not a customization }, { @@ -269,13 +276,23 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "target", Source: ""}, index: 0, numCustomizations: 0, // All targets are gitrepo targets + numRestrictions: 1, want: false, }, + { + name: "old bundle - standalone bundle (no restrictions)", + target: fleet.BundleTarget{Name: "target", Source: ""}, + index: 0, + numCustomizations: 1, // Would be calculated as 1 if there were restrictions + numRestrictions: 0, // But no restrictions means not a GitRepo bundle + want: false, // All targets treated as regular targets + }, { name: "old bundle - multiple customizations", target: fleet.BundleTarget{Name: "custom2", Source: ""}, index: 2, // Third target (index 2) numCustomizations: 3, // First 3 are customizations + numRestrictions: 2, want: true, }, { @@ -283,6 +300,7 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "last-custom", Source: ""}, index: 2, // Third target (index 2) numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2) + numRestrictions: 1, want: true, }, { @@ -290,13 +308,14 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) { target: fleet.BundleTarget{Name: "first-gitrepo", Source: ""}, index: 3, // Fourth target (index 3) numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2) + numRestrictions: 1, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations) + got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations, tt.numRestrictions) assert.Equal(t, tt.want, got) }) } From 3da7a0b3c92438ee25a90e7ddbf24a07a7570e30 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Tue, 14 Apr 2026 12:37:17 +0200 Subject: [PATCH 3/3] chore: update generated files --- charts/fleet-crd/templates/crds.yaml | 4 ++++ internal/mocks/git_fetcher_mock.go | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/charts/fleet-crd/templates/crds.yaml b/charts/fleet-crd/templates/crds.yaml index ffd9e33155..6ca0f58c71 100644 --- a/charts/fleet-crd/templates/crds.yaml +++ b/charts/fleet-crd/templates/crds.yaml @@ -3219,6 +3219,8 @@ spec: "gitrepo" - target comes from GitRepo.Spec.Targets (via targets file) + "helmop" - target comes from a HelmOp resource + If empty, provenance is determined by position in the Targets array: @@ -9508,6 +9510,8 @@ spec: "gitrepo" - target comes from GitRepo.Spec.Targets (via targets file) + "helmop" - target comes from a HelmOp resource + If empty, provenance is determined by position in the Targets array: diff --git a/internal/mocks/git_fetcher_mock.go b/internal/mocks/git_fetcher_mock.go index 4227c38084..7b65444562 100644 --- a/internal/mocks/git_fetcher_mock.go +++ b/internal/mocks/git_fetcher_mock.go @@ -45,17 +45,17 @@ func (m *MockStrategy) EXPECT() *MockStrategyMockRecorder { } // Execute mocks base method. -func (m *MockStrategy) Execute(ctx context.Context, r *git.Repository, CommitHash plumbing.Hash) error { +func (m *MockStrategy) Execute(ctx context.Context, r *git.Repository, commitHash plumbing.Hash) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Execute", ctx, r, CommitHash) + ret := m.ctrl.Call(m, "Execute", ctx, r, commitHash) ret0, _ := ret[0].(error) return ret0 } // Execute indicates an expected call of Execute. -func (mr *MockStrategyMockRecorder) Execute(ctx, r, CommitHash any) *gomock.Call { +func (mr *MockStrategyMockRecorder) Execute(ctx, r, commitHash any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Execute", reflect.TypeOf((*MockStrategy)(nil).Execute), ctx, r, CommitHash) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Execute", reflect.TypeOf((*MockStrategy)(nil).Execute), ctx, r, commitHash) } // Type mocks base method.