Skip to content

Commit 7a61562

Browse files
p-sep-se
committed
Revert independent doNotDeploy behavior
1 parent fb3e87c commit 7a61562

4 files changed

Lines changed: 122 additions & 48 deletions

File tree

‎integrationtests/controller/bundle/bundle_targets_test.go‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -459,22 +459,23 @@ var _ = Describe("Bundle targets", Ordered, func() {
459459
})
460460
})
461461

462-
// Regression test for https://github.com/rancher/fleet/issues/3580:
462+
// With FirstMatch semantics (the default), only the first matching targetCustomization is applied.
463463
// When a broader-matching target appears before a doNotDeploy target in the list,
464-
// the doNotDeploy target was previously bypassed due to first-match semantics.
464+
// the broader match wins and the doNotDeploy entry is never evaluated.
465+
// Users must order their targetCustomizations from specific to general to use doNotDeploy effectively.
465466
When("a broader-matching targetCustomization appears before a doNotDeploy targetCustomization", func() {
466467
BeforeEach(func() {
467468
bundleName = "donot-deploy-after-broader-match"
468469
bdLabels = map[string]string{
469470
"fleet.cattle.io/bundle-name": bundleName,
470471
"fleet.cattle.io/bundle-namespace": namespace,
471472
}
472-
expectedNumberOfBundleDeployments = 0
473+
expectedNumberOfBundleDeployments = 1
473474
// simulate targets in fleet.yaml:
474475
// - first entry matches all clusters (broader match)
475476
// - second entry matches cluster "one" with doNotDeploy=true
476-
// With the old first-match logic, cluster "one" would match the first entry and
477-
// the doNotDeploy entry would never be evaluated.
477+
// With FirstMatch semantics, cluster "one" matches the first entry and gets deployed.
478+
// The doNotDeploy entry is never evaluated because the first match wins.
478479
targets = []v1alpha1.BundleTarget{
479480
{
480481
BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{
@@ -483,24 +484,74 @@ var _ = Describe("Bundle targets", Ordered, func() {
483484
},
484485
},
485486
ClusterGroup: "all",
487+
Source: "customization",
486488
},
487489
{
488490
ClusterGroup: "one",
489491
DoNotDeploy: true,
492+
Source: "customization",
490493
},
491494
}
492495
// simulate targets in GitRepo: only cluster group "one" is targeted
493496
targetsInGitRepo := []v1alpha1.BundleTarget{
494497
{
495498
ClusterGroup: "one",
499+
Source: "gitrepo",
496500
},
497501
}
498502
targetRestrictions = make([]v1alpha1.BundleTarget, len(targetsInGitRepo))
499503
copy(targetRestrictions, targetsInGitRepo)
500504
targets = append(targets, targetsInGitRepo...)
501505
})
502506

503-
It("no BundleDeployments are created", func() {
507+
It("creates a BundleDeployment because first match wins (using explicit Source field)", func() {
508+
waitForBundleToBeReady(bundleName)
509+
_ = verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName)
510+
})
511+
})
512+
513+
// Same scenario as above, but without Source fields to test backward compatibility
514+
// with bundles created before the Source field was added (position-based fallback).
515+
When("a broader-matching targetCustomization appears before a doNotDeploy targetCustomization (position-based detection)", func() {
516+
BeforeEach(func() {
517+
bundleName = "donot-deploy-after-broader-match-position-based"
518+
bdLabels = map[string]string{
519+
"fleet.cattle.io/bundle-name": bundleName,
520+
"fleet.cattle.io/bundle-namespace": namespace,
521+
}
522+
expectedNumberOfBundleDeployments = 1
523+
// simulate targets in fleet.yaml:
524+
// - first entry matches all clusters (broader match)
525+
// - second entry matches cluster "one" with doNotDeploy=true
526+
// With FirstMatch semantics, cluster "one" matches the first entry and gets deployed.
527+
// The doNotDeploy entry is never evaluated because the first match wins.
528+
// No Source fields are set, so position-based detection is used.
529+
targets = []v1alpha1.BundleTarget{
530+
{
531+
BundleDeploymentOptions: v1alpha1.BundleDeploymentOptions{
532+
Helm: &v1alpha1.HelmOptions{
533+
Values: &v1alpha1.GenericMap{Data: map[string]interface{}{"replicas": "1"}},
534+
},
535+
},
536+
ClusterGroup: "all",
537+
},
538+
{
539+
ClusterGroup: "one",
540+
DoNotDeploy: true,
541+
},
542+
}
543+
// simulate targets in GitRepo: only cluster group "one" is targeted
544+
targetsInGitRepo := []v1alpha1.BundleTarget{
545+
{
546+
ClusterGroup: "one",
547+
},
548+
}
549+
targetRestrictions = make([]v1alpha1.BundleTarget, len(targetsInGitRepo))
550+
copy(targetRestrictions, targetsInGitRepo)
551+
targets = append(targets, targetsInGitRepo...)
552+
})
553+
554+
It("creates a BundleDeployment because first match wins (using position-based fallback)", func() {
504555
waitForBundleToBeReady(bundleName)
505556
_ = verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName)
506557
})

‎internal/cmd/controller/target/builder.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ func (m *Manager) Targets(ctx context.Context, bundle *fleet.Bundle, manifestID
7272
if err != nil {
7373
return nil, false, err
7474
}
75+
clusterLoop:
7576
for _, cluster := range clusters.Items {
7677
logger.V(4).Info("Cluster has namespace?", "cluster", cluster.Name, "namespace", cluster.Status.Namespace)
7778
clusterGroups, err := m.clusterGroupsForCluster(ctx, &cluster)
@@ -85,28 +86,50 @@ func (m *Manager) Targets(ctx context.Context, bundle *fleet.Bundle, manifestID
8586
if target == nil {
8687
continue
8788
}
88-
// Check all matching targetCustomizations for doNotDeploy, not just the first match.
89-
// This ensures that a doNotDeploy entry is honoured even when a broader-matching
90-
// target appears before it in the target list (fixes first-match bypass).
91-
doNotDeploy := target.DoNotDeploy || bm.HasDoNotDeployTarget(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels)
92-
if doNotDeploy {
89+
// Check if the GitRepo target has doNotDeploy set
90+
if target.DoNotDeploy {
9391
logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.",
9492
"bundle", bundle.Name,
9593
"bundleNamespace", bundle.Namespace,
9694
"cluster", cluster.Name,
9795
"clusterNamespace", cluster.Namespace,
98-
"reason", "doNotDeploy",
96+
"reason", "doNotDeploy on GitRepo target",
9997
)
10098
continue
10199
}
102100
// check if there is any matching targetCustomization that should be applied
103101
targetOpts := target.BundleDeploymentOptions
104102
if bundle.Spec.TargetCustomizationMode == fleet.TargetCustomizationModeAllMatches {
105-
for _, tc := range bm.MatchAllTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels) {
103+
// AllMatches mode: merge all matching customizations
104+
// Check if any matching customization has doNotDeploy=true (OR logic)
105+
matchedCustomizations := bm.MatchAllTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels)
106+
for _, tc := range matchedCustomizations {
107+
if tc.DoNotDeploy {
108+
logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.",
109+
"bundle", bundle.Name,
110+
"bundleNamespace", bundle.Namespace,
111+
"cluster", cluster.Name,
112+
"clusterNamespace", cluster.Namespace,
113+
"reason", "doNotDeploy on targetCustomization (AllMatches mode)",
114+
)
115+
continue clusterLoop
116+
}
106117
targetOpts = options.Merge(targetOpts, tc.BundleDeploymentOptions)
107118
}
108119
} else {
120+
// FirstMatch mode: apply only the first matching customization
109121
if targetCustomized := bm.MatchTargetCustomizations(cluster.Name, clusterGroupsAsLabelMap, cluster.Labels); targetCustomized != nil {
122+
// Check if the first matching targetCustomization has doNotDeploy set
123+
if targetCustomized.DoNotDeploy {
124+
logger.V(1).Info("Skipping BundleDeployment creation because doNotDeploy is set to true.",
125+
"bundle", bundle.Name,
126+
"bundleNamespace", bundle.Namespace,
127+
"cluster", cluster.Name,
128+
"clusterNamespace", cluster.Namespace,
129+
"reason", "doNotDeploy on targetCustomization (FirstMatch mode)",
130+
)
131+
continue
132+
}
110133
targetOpts = targetCustomized.BundleDeploymentOptions
111134
}
112135
}

‎internal/cmd/controller/target/matcher/bundlematch.go‎

Lines changed: 15 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -75,30 +75,6 @@ func (a *BundleMatch) MatchAllTargetCustomizations(clusterName string, clusterGr
7575
return result
7676
}
7777

78-
// HasDoNotDeployTarget returns true if any bundle target matching the given cluster
79-
// has DoNotDeploy set to true. Unlike MatchTargetCustomizations, this scans all matching
80-
// bundle targets instead of stopping at the first match, so a doNotDeploy entry does not have to
81-
// appear before a broader-matching entry in the target list.
82-
func (a *BundleMatch) HasDoNotDeployTarget(clusterName string, clusterGroups map[string]map[string]string, clusterLabels map[string]string) bool {
83-
for _, tm := range a.matcher.matches {
84-
if !tm.bundleTarget.DoNotDeploy {
85-
continue
86-
}
87-
if len(clusterGroups) == 0 {
88-
if criteriaWithoutRestrictions(tm, clusterName, "", nil, clusterLabels) {
89-
return true
90-
}
91-
} else {
92-
for cg, cgLabels := range clusterGroups {
93-
if criteriaWithoutRestrictions(tm, clusterName, cg, cgLabels, clusterLabels) {
94-
return true
95-
}
96-
}
97-
}
98-
}
99-
return false
100-
}
101-
10278
type targetMatch struct {
10379
bundleTarget *fleet.BundleTarget
10480
criteria *ClusterMatcher
@@ -126,7 +102,7 @@ func (a *BundleMatch) initMatcher() error {
126102
t := targetMatch{
127103
bundleTarget: &a.bundle.Spec.Targets[i],
128104
criteria: clusterMatcher,
129-
isCustomization: determineIsCustomization(target, i, numCustomizations),
105+
isCustomization: determineIsCustomization(target, i, numCustomizations, numRestrictions),
130106
}
131107

132108
m.matches = append(m.matches, t)
@@ -146,7 +122,7 @@ func (a *BundleMatch) initMatcher() error {
146122

147123
// determineIsCustomization uses explicit Source field if present,
148124
// falls back to position-based detection for backward compatibility.
149-
func determineIsCustomization(target fleet.BundleTarget, index int, numCustomizations int) bool {
125+
func determineIsCustomization(target fleet.BundleTarget, index int, numCustomizations int, numRestrictions int) bool {
150126
// NEW BUNDLES: Source field is populated by bundlereader
151127
// This is the preferred method for long-term maintainability
152128
if target.Source != "" {
@@ -160,7 +136,15 @@ func determineIsCustomization(target fleet.BundleTarget, index int, numCustomiza
160136
// - Therefore: first N targets are customizations
161137
// where N = len(Targets) - len(TargetRestrictions)
162138
//
139+
// SPECIAL CASE: If there are no targetRestrictions, this bundle wasn't
140+
// created by a GitRepo (e.g., CLI-loaded bundles, standalone bundles).
141+
// In this case, treat all targets as regular bundle targets (not customizations)
142+
// to maintain backward compatibility with bundles that predate the Source field.
143+
//
163144
// This fixes the collision bug for old Bundles without requiring recreation
145+
if numRestrictions == 0 {
146+
return false
147+
}
164148
return index < numCustomizations
165149
}
166150

@@ -180,8 +164,12 @@ func (m *matcher) isRestricted(clusterName, clusterGroup string, clusterGroupLab
180164
}
181165

182166
// criteriaWithRestrictions checks that the cluster passes the restriction allowlist
183-
// and matches the target's cluster selector. Used for GitRepo targets.
167+
// and matches the target's cluster selector. Used for GitRepo targets only;
168+
// customization targets (from fleet.yaml) are excluded.
184169
func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool {
170+
if targetMatch.isCustomization {
171+
return false
172+
}
185173
if !m.isRestricted(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) &&
186174
targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels) {
187175
return true
@@ -190,13 +178,6 @@ func (m *matcher) criteriaWithRestrictions(targetMatch targetMatch, clusterName,
190178
return false
191179
}
192180

193-
// criteriaWithoutRestrictions checks targetMatch's criteria for a match on the specified cluster
194-
// name, group and labels, without checking if target is inside the targetRestrictions.
195-
// This is used for TargetCustomizations.
196-
func criteriaWithoutRestrictions(targetMatch targetMatch, clusterName, clusterGroup string, clusterGroupLabels, clusterLabels map[string]string) bool {
197-
return targetMatch.criteria.Match(clusterName, clusterGroup, clusterGroupLabels, clusterLabels)
198-
}
199-
200181
// 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.
201182
func (m *matcher) match(clusterName string, clusterLabels map[string]string, clusterGroups map[string]map[string]string, findCriteriaMatch findCriteriaMatch) *fleet.BundleTarget {
202183
for _, targetMatch := range m.matches {

‎internal/cmd/controller/target/matcher/bundlematch_test.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,83 +220,102 @@ func TestDetermineIsCustomization_Hybrid(t *testing.T) {
220220
target fleet.BundleTarget
221221
index int
222222
numCustomizations int
223+
numRestrictions int
223224
want bool
224225
}{
225226
{
226227
name: "new bundle - explicit customization source",
227228
target: fleet.BundleTarget{Name: "edge", Source: "customization"},
228229
index: 0,
229230
numCustomizations: 1,
231+
numRestrictions: 1,
230232
want: true,
231233
},
232234
{
233235
name: "new bundle - explicit gitrepo source",
234236
target: fleet.BundleTarget{Name: "prod", Source: "gitrepo"},
235237
index: 1,
236238
numCustomizations: 1,
239+
numRestrictions: 1,
237240
want: false,
238241
},
239242
{
240243
name: "old bundle - position-based customization",
241244
target: fleet.BundleTarget{Name: "edge", Source: ""}, // empty
242245
index: 0,
243246
numCustomizations: 1,
247+
numRestrictions: 1,
244248
want: true, // index < numCustomizations
245249
},
246250
{
247251
name: "old bundle - position-based gitrepo target",
248252
target: fleet.BundleTarget{Name: "prod", Source: ""}, // empty
249253
index: 1,
250254
numCustomizations: 1,
255+
numRestrictions: 1,
251256
want: false, // index >= numCustomizations
252257
},
253258
{
254259
name: "collision case - old bundle position fixes bug",
255260
target: fleet.BundleTarget{Name: "prod", ClusterSelector: labelSelector(map[string]string{"env": "prod"}), Source: ""},
256261
index: 0, // First in list
257262
numCustomizations: 1,
263+
numRestrictions: 1,
258264
want: true, // Treated as customization via position, bug fixed!
259265
},
260266
{
261267
name: "new bundle - explicit source overrides position",
262268
target: fleet.BundleTarget{Name: "test", Source: "gitrepo"},
263269
index: 0, // Position suggests customization
264270
numCustomizations: 1,
271+
numRestrictions: 1,
265272
want: false, // But Source field says gitrepo, so not a customization
266273
},
267274
{
268275
name: "old bundle - no customizations (all gitrepo)",
269276
target: fleet.BundleTarget{Name: "target", Source: ""},
270277
index: 0,
271278
numCustomizations: 0, // All targets are gitrepo targets
279+
numRestrictions: 1,
272280
want: false,
273281
},
282+
{
283+
name: "old bundle - standalone bundle (no restrictions)",
284+
target: fleet.BundleTarget{Name: "target", Source: ""},
285+
index: 0,
286+
numCustomizations: 1, // Would be calculated as 1 if there were restrictions
287+
numRestrictions: 0, // But no restrictions means not a GitRepo bundle
288+
want: false, // All targets treated as regular targets
289+
},
274290
{
275291
name: "old bundle - multiple customizations",
276292
target: fleet.BundleTarget{Name: "custom2", Source: ""},
277293
index: 2, // Third target (index 2)
278294
numCustomizations: 3, // First 3 are customizations
295+
numRestrictions: 2,
279296
want: true,
280297
},
281298
{
282299
name: "old bundle - boundary case (last customization)",
283300
target: fleet.BundleTarget{Name: "last-custom", Source: ""},
284301
index: 2, // Third target (index 2)
285302
numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2)
303+
numRestrictions: 1,
286304
want: true,
287305
},
288306
{
289307
name: "old bundle - boundary case (first gitrepo target)",
290308
target: fleet.BundleTarget{Name: "first-gitrepo", Source: ""},
291309
index: 3, // Fourth target (index 3)
292310
numCustomizations: 3, // First 3 are customizations (indices 0, 1, 2)
311+
numRestrictions: 1,
293312
want: false,
294313
},
295314
}
296315

297316
for _, tt := range tests {
298317
t.Run(tt.name, func(t *testing.T) {
299-
got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations)
318+
got := determineIsCustomization(tt.target, tt.index, tt.numCustomizations, tt.numRestrictions)
300319
assert.Equal(t, tt.want, got)
301320
})
302321
}

0 commit comments

Comments
 (0)