Skip to content

Commit fec5a69

Browse files
mogitamogita
authored
fix(rollout): stop matching canary templates from other rollouts (#10230)
1 parent 8fb47cc commit fec5a69

3 files changed

Lines changed: 114 additions & 6 deletions

File tree

‎controllers/apps/rollout/transformer_rollout_replace.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -652,8 +652,14 @@ func buildReplaceInstanceTemplateName(tpl appsv1.InstanceTemplate, suffix string
652652
}
653653

654654
func isRolloutManagedInstanceTemplate(rollout *appsv1alpha1.Rollout, tpl appsv1.InstanceTemplate) bool {
655-
return isRolloutManagedInstanceTemplateName(rollout, tpl.Name) ||
656-
hasInstanceTemplateCreatedByAnnotation(tpl)
655+
// Only match templates whose name carries this rollout's UID. The
656+
// "created-by-rollout" annotation alone is not enough: it is set by every
657+
// Rollout that creates a canary template, so an OR-match would treat
658+
// leftover canary templates from previous rollouts as managed by the
659+
// current one. That misclassification breaks setup (no fresh canary is
660+
// created) and panics teardown (the resolved template map has no default
661+
// entry; tpls[""].DeepCopy() dereferences nil).
662+
return isRolloutManagedInstanceTemplateName(rollout, tpl.Name)
657663
}
658664

659665
func isRolloutManagedInstanceTemplateName(rollout *appsv1alpha1.Rollout, tplName string) bool {

‎controllers/apps/rollout/transformer_rollout_replace_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,3 +100,84 @@ func TestIsRolloutManagedInstanceTemplateNameSupportsLegacyAndNewNames(t *testin
100100
}
101101
}
102102
}
103+
104+
// TestIsRolloutManagedInstanceTemplateRejectsForeignRolloutCanaries verifies
105+
// that templates created by a different (previous) Rollout — which still carry
106+
// the "created-by-rollout" annotation but whose names reference that previous
107+
// Rollout's UID — are not treated as managed by the current Rollout.
108+
//
109+
// Prior to the fix, isRolloutManagedInstanceTemplate also matched on the
110+
// annotation alone, so leftover canary templates from a completed rollout were
111+
// misclassified as managed by every subsequent rollout. That misclassification
112+
// caused replaceShardingInstanceTemplatesFromSpec to return a non-empty map
113+
// without a default entry, which made tpls[""].DeepCopy() in the teardown
114+
// transformer dereference a nil pointer and panic the controller manager.
115+
func TestIsRolloutManagedInstanceTemplateRejectsForeignRolloutCanaries(t *testing.T) {
116+
previous := &appsv1alpha1.Rollout{}
117+
previous.UID = types.UID("11111111-1111-1111-1111-111111111111")
118+
current := &appsv1alpha1.Rollout{}
119+
current.UID = types.UID("22222222-2222-2222-2222-222222222222")
120+
121+
previousCanary := appsv1.InstanceTemplate{
122+
Name: replaceInstanceTemplateNameSuffix(previous),
123+
Annotations: map[string]string{
124+
instanceTemplateCreatedByAnnotationKey: "yes",
125+
},
126+
}
127+
if !isRolloutManagedInstanceTemplate(previous, previousCanary) {
128+
t.Fatalf("previous rollout should manage its own canary")
129+
}
130+
if isRolloutManagedInstanceTemplate(current, previousCanary) {
131+
t.Fatalf("current rollout must not claim a canary created by a previous rollout")
132+
}
133+
}
134+
135+
// TestReplaceShardingInstanceTemplatesAlwaysProvidesDefaultForFreshRollout
136+
// reproduces the consecutive-rollout panic. The cluster sharding spec contains
137+
// a canary template left behind by a successful prior rollout (named with the
138+
// previous rollout's UID, annotation set). A fresh rollout then asks for its
139+
// template map. Before the fix, replaceShardingInstanceTemplatesFromSpec
140+
// matched the leftover via the annotation, the early-return path fired with
141+
// only a non-default entry, and the caller saw tpls[""] == nil — exactly the
142+
// state that crashed the teardown transformer at tpls[""].DeepCopy().
143+
func TestReplaceShardingInstanceTemplatesAlwaysProvidesDefaultForFreshRollout(t *testing.T) {
144+
previous := &appsv1alpha1.Rollout{}
145+
previous.UID = types.UID("11111111-1111-1111-1111-111111111111")
146+
current := &appsv1alpha1.Rollout{}
147+
current.UID = types.UID("22222222-2222-2222-2222-222222222222")
148+
current.Spec.Shardings = []appsv1alpha1.RolloutSharding{{
149+
Name: "shard",
150+
Strategy: appsv1alpha1.RolloutStrategy{
151+
Replace: &appsv1alpha1.RolloutStrategyReplace{},
152+
},
153+
}}
154+
155+
replicas := int32(3)
156+
spec := &appsv1.ClusterSharding{
157+
Name: "shard",
158+
Template: appsv1.ClusterComponentSpec{
159+
Replicas: replicas,
160+
FlatInstanceOrdinal: true,
161+
Instances: []appsv1.InstanceTemplate{
162+
{
163+
Name: replaceInstanceTemplateNameSuffix(previous),
164+
Replicas: &replicas,
165+
Annotations: map[string]string{
166+
instanceTemplateCreatedByAnnotationKey: "yes",
167+
},
168+
},
169+
},
170+
},
171+
}
172+
173+
tpls, exist, err := replaceShardingInstanceTemplates(current, current.Spec.Shardings[0], spec)
174+
if err != nil {
175+
t.Fatalf("unexpected error: %v", err)
176+
}
177+
if exist {
178+
t.Fatalf("expected exist=false (no template managed by current rollout), got true")
179+
}
180+
if tpls[""] == nil {
181+
t.Fatalf("expected a default template entry for the current rollout, got nil — this is the regression that panics teardown")
182+
}
183+
}

‎controllers/apps/rollout/transformer_rollout_teardown.go‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
2020
package rollout
2121

2222
import (
23+
"fmt"
2324
"slices"
2425

2526
"k8s.io/utils/ptr"
@@ -89,7 +90,15 @@ func (t *rolloutTearDownTransformer) compReplace(transCtx *rolloutTransformConte
8990
}
9091
newReplicas := replaceInstanceTemplateReplicas(tpls)
9192
if newReplicas == replicas && spec.Replicas == replicas && checkClusterNCompRunning(transCtx, comp.Name) {
92-
tpl := tpls[""].DeepCopy() // use the default template, use DeepCopy to avoid it been removed
93+
defaultTpl := tpls[""]
94+
if defaultTpl == nil {
95+
// The replace transformer is expected to have produced a default
96+
// canary template before teardown can be reached. A missing entry
97+
// here indicates an inconsistent rollout state; surface it instead
98+
// of silently no-op'ing (and instead of dereferencing nil).
99+
return fmt.Errorf("component %s: no default rollout-managed template found in cluster spec during teardown", comp.Name)
100+
}
101+
tpl := defaultTpl.DeepCopy() // use the default template, use DeepCopy to avoid it been removed
93102
spec.ServiceVersion = tpl.ServiceVersion
94103
spec.ComponentDef = tpl.CompDef
95104
spec.OfflineInstances = slices.DeleteFunc(spec.OfflineInstances, func(instance string) bool {
@@ -104,7 +113,9 @@ func (t *rolloutTearDownTransformer) compReplace(transCtx *rolloutTransformConte
104113
if ptr.Deref(tpl.Replicas, 0) > 0 {
105114
return false
106115
}
107-
return isRolloutManagedInstanceTemplate(rollout, tpl)
116+
// Drop drained canaries from this rollout AND any drained canary
117+
// left over from previous rollouts (they no longer back pods).
118+
return isRolloutManagedInstanceTemplate(rollout, tpl) || hasInstanceTemplateCreatedByAnnotation(tpl)
108119
})
109120
for i, inst := range spec.Instances {
110121
if len(inst.ServiceVersion) > 0 {
@@ -175,7 +186,15 @@ func (t *rolloutTearDownTransformer) shardingReplace(transCtx *rolloutTransformC
175186
}
176187
newReplicas := replaceInstanceTemplateReplicas(tpls)
177188
if newReplicas == replicas && spec.Template.Replicas == replicas && checkClusterNShardingRunning(transCtx, sharding.Name) {
178-
tpl := tpls[""].DeepCopy() // use the default template, use DeepCopy to avoid it been removed
189+
defaultTpl := tpls[""]
190+
if defaultTpl == nil {
191+
// The replace transformer is expected to have produced a default
192+
// canary template before teardown can be reached. A missing entry
193+
// here indicates an inconsistent rollout state; surface it instead
194+
// of silently no-op'ing (and instead of dereferencing nil).
195+
return fmt.Errorf("sharding %s: no default rollout-managed template found in cluster spec during teardown", sharding.Name)
196+
}
197+
tpl := defaultTpl.DeepCopy() // use the default template, use DeepCopy to avoid it been removed
179198
spec.Template.ServiceVersion = tpl.ServiceVersion
180199
spec.Template.ComponentDef = tpl.CompDef
181200
spec.Template.OfflineInstances = slices.DeleteFunc(spec.Template.OfflineInstances, func(instance string) bool {
@@ -190,7 +209,9 @@ func (t *rolloutTearDownTransformer) shardingReplace(transCtx *rolloutTransformC
190209
if ptr.Deref(tpl.Replicas, 0) > 0 {
191210
return false
192211
}
193-
return isRolloutManagedInstanceTemplate(rollout, tpl)
212+
// Drop drained canaries from this rollout AND any drained canary
213+
// left over from previous rollouts (they no longer back pods).
214+
return isRolloutManagedInstanceTemplate(rollout, tpl) || hasInstanceTemplateCreatedByAnnotation(tpl)
194215
})
195216
for i, inst := range spec.Template.Instances {
196217
if len(inst.ServiceVersion) > 0 {

0 commit comments

Comments
 (0)