diff --git a/charts/fleet-crd/templates/crds.yaml b/charts/fleet-crd/templates/crds.yaml index 4280f0a2a2..6ca0f58c71 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,37 @@ 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) + + "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.' + enum: + - customization + - gitrepo + - helmop + - '' + type: string yaml: description: 'YAML options, if using raw YAML these are names that map to @@ -8708,6 +8752,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 +8772,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 +9502,37 @@ 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) + + "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.' + 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..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) }) }) @@ -588,6 +637,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 { @@ -608,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/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..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,25 +86,52 @@ 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 - targetCustomized := bm.MatchTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) - if targetCustomized != nil { - targetOpts = targetCustomized.BundleDeploymentOptions + if bundle.Spec.TargetCustomizationMode == fleet.TargetCustomizationModeAllMatches { + // 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 + } } 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..9ea195065a 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,40 +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 } -// 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 { +// 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.bundleTarget.DoNotDeploy { + if !tm.isCustomization { continue } if len(clusterGroups) == 0 { - if criteriaWithoutRestrictions(tm, clusterName, "", nil, clusterLabels) { - return true + if tm.criteria.Match(clusterName, "", nil, clusterLabels) { + result = append(result, tm.bundleTarget) } } else { for cg, cgLabels := range clusterGroups { - if criteriaWithoutRestrictions(tm, clusterName, cg, cgLabels, clusterLabels) { - return true + 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 false + return result } 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 +89,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, numRestrictions), } m.matches = append(m.matches, t) @@ -113,8 +120,36 @@ 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, numRestrictions 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) + // + // 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 +} + 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,13 +160,16 @@ 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 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 @@ -140,11 +178,6 @@ 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. -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 { @@ -160,6 +193,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..17477bcb19 --- /dev/null +++ b/internal/cmd/controller/target/matcher/bundlematch_test.go @@ -0,0 +1,322 @@ +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 + numRestrictions int + want bool + }{ + { + name: "new bundle - explicit customization source", + target: fleet.BundleTarget{Name: "edge", Source: "customization"}, + index: 0, + numCustomizations: 1, + numRestrictions: 1, + want: true, + }, + { + name: "new bundle - explicit gitrepo source", + target: fleet.BundleTarget{Name: "prod", Source: "gitrepo"}, + index: 1, + numCustomizations: 1, + numRestrictions: 1, + want: false, + }, + { + name: "old bundle - position-based customization", + target: fleet.BundleTarget{Name: "edge", Source: ""}, // empty + index: 0, + numCustomizations: 1, + numRestrictions: 1, + want: true, // index < numCustomizations + }, + { + name: "old bundle - position-based gitrepo target", + target: fleet.BundleTarget{Name: "prod", Source: ""}, // empty + index: 1, + numCustomizations: 1, + numRestrictions: 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, + numRestrictions: 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, + numRestrictions: 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 + 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, + }, + { + 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) + numRestrictions: 1, + 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) + numRestrictions: 1, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations, tt.numRestrictions) + assert.Equal(t, tt.want, got) + }) + } +} 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. 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.