Skip to content

Commit 334f3d7

Browse files
alvaroalemanalvaroaleman
committed
⚠️ Validate controller names are unique
When re-using the same name among multiple controllers, they will report to the same metrics and use the same logger. This can be very confusing, might not be obvious and can happen accidentally. Validate controller names are unique at runtime to avoid all of this.
1 parent abb2d86 commit 334f3d7

File tree

4 files changed

+95
-22
lines changed

4 files changed

+95
-22
lines changed

‎pkg/builder/controller_test.go

+18-6
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ var _ = Describe("application", func() {
142142
Expect(err).NotTo(HaveOccurred())
143143

144144
instance, err := ControllerManagedBy(m).
145-
Named("my_controller").
145+
Named("my_new_controller").
146146
Build(noop)
147147
Expect(err).To(MatchError(ContainSubstring("there are no watches configured, controller will never get triggered. Use For(), Owns(), Watches() or WatchesRawSource() to set them up")))
148148
Expect(instance).To(BeNil())
@@ -154,7 +154,7 @@ var _ = Describe("application", func() {
154154
Expect(err).NotTo(HaveOccurred())
155155

156156
instance, err := ControllerManagedBy(m).
157-
Named("my_controller").
157+
Named("my_other_controller").
158158
Watches(&appsv1.ReplicaSet{}, &handler.EnqueueRequestForObject{}).
159159
Build(noop)
160160
Expect(err).NotTo(HaveOccurred())
@@ -186,6 +186,7 @@ var _ = Describe("application", func() {
186186

187187
instance, err := TypedControllerManagedBy[empty](m).
188188
For(&appsv1.ReplicaSet{}).
189+
Named("last_controller").
189190
Build(typedNoop)
190191
Expect(err).To(MatchError(ContainSubstring("For() can only be used with reconcile.Request, got builder.empty")))
191192
Expect(instance).To(BeNil())
@@ -197,7 +198,7 @@ var _ = Describe("application", func() {
197198
Expect(err).NotTo(HaveOccurred())
198199

199200
instance, err := TypedControllerManagedBy[empty](m).
200-
Named("my_controller").
201+
Named("my_controller-0").
201202
Owns(&appsv1.ReplicaSet{}).
202203
Build(typedNoop)
203204
// If we ever allow Owns() without For() we need to update the code to error
@@ -213,7 +214,7 @@ var _ = Describe("application", func() {
213214
Expect(err).NotTo(HaveOccurred())
214215

215216
instance, err := TypedControllerManagedBy[empty](m).
216-
Named("my_controller").
217+
Named("my_controller-1").
217218
WatchesRawSource(
218219
source.TypedKind(
219220
m.GetCache(),
@@ -263,6 +264,7 @@ var _ = Describe("application", func() {
263264

264265
builder := ControllerManagedBy(m).
265266
For(&appsv1.ReplicaSet{}).
267+
Named("replicaset-4").
266268
Owns(&appsv1.ReplicaSet{}).
267269
WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles})
268270
builder.newController = newController
@@ -294,6 +296,7 @@ var _ = Describe("application", func() {
294296

295297
builder := ControllerManagedBy(m).
296298
For(&appsv1.ReplicaSet{}).
299+
Named("replicaset-3").
297300
Owns(&appsv1.ReplicaSet{})
298301
builder.newController = newController
299302

@@ -317,6 +320,7 @@ var _ = Describe("application", func() {
317320

318321
builder := ControllerManagedBy(m).
319322
For(&appsv1.ReplicaSet{}).
323+
Named("replicaset-2").
320324
Owns(&appsv1.ReplicaSet{}).
321325
WithOptions(controller.Options{RateLimiter: rateLimiter})
322326
builder.newController = newController
@@ -341,6 +345,7 @@ var _ = Describe("application", func() {
341345

342346
builder := ControllerManagedBy(m).
343347
For(&appsv1.ReplicaSet{}).
348+
Named("replicaset-0").
344349
Owns(&appsv1.ReplicaSet{}).
345350
WithLogConstructor(func(request *reconcile.Request) logr.Logger {
346351
return logr.New(logger)
@@ -358,6 +363,7 @@ var _ = Describe("application", func() {
358363

359364
builder := ControllerManagedBy(m).
360365
For(&appsv1.ReplicaSet{}).
366+
Named("replicaset-1").
361367
Owns(&appsv1.ReplicaSet{}).
362368
WithOptions(controller.Options{Reconciler: noop})
363369
instance, err := builder.Build(noop)
@@ -387,6 +393,7 @@ var _ = Describe("application", func() {
387393
By("creating the 2nd controller")
388394
ctrl2, err := ControllerManagedBy(m).
389395
For(&TestDefaultValidator{}).
396+
Named("test-default-validator-1").
390397
Owns(&appsv1.ReplicaSet{}).
391398
Build(noop)
392399
Expect(err).NotTo(HaveOccurred())
@@ -401,6 +408,7 @@ var _ = Describe("application", func() {
401408

402409
bldr := ControllerManagedBy(m).
403410
For(&appsv1.Deployment{}).
411+
Named("deployment-0").
404412
Owns(&appsv1.ReplicaSet{})
405413

406414
ctx, cancel := context.WithCancel(context.Background())
@@ -414,6 +422,7 @@ var _ = Describe("application", func() {
414422

415423
bldr := ControllerManagedBy(m).
416424
For(&appsv1.Deployment{}).
425+
Named("deployment-1").
417426
Owns(&appsv1.ReplicaSet{}, MatchEveryOwner)
418427

419428
ctx, cancel := context.WithCancel(context.Background())
@@ -443,6 +452,7 @@ var _ = Describe("application", func() {
443452

444453
bldr := ControllerManagedBy(m).
445454
Named("Deployment").
455+
Named("deployment-2").
446456
Watches( // Equivalent of For
447457
&appsv1.Deployment{}, &handler.EnqueueRequestForObject{}).
448458
Watches( // Equivalent of Owns
@@ -503,6 +513,7 @@ var _ = Describe("application", func() {
503513

504514
bldr := ControllerManagedBy(m).
505515
For(&appsv1.Deployment{}, WithPredicates(deployPrct)).
516+
Named("deployment-3").
506517
Owns(&appsv1.ReplicaSet{}, WithPredicates(replicaSetPrct)).
507518
WithEventFilter(allPrct)
508519

@@ -527,8 +538,8 @@ var _ = Describe("application", func() {
527538
})
528539

529540
It("should support multiple controllers watching the same metadata kind", func() {
530-
bldr1 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata)
531-
bldr2 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata)
541+
bldr1 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata).Named("deployment-4")
542+
bldr2 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata).Named("deployment-5")
532543

533544
ctx, cancel := context.WithCancel(context.Background())
534545
defer cancel()
@@ -541,6 +552,7 @@ var _ = Describe("application", func() {
541552

542553
bldr := ControllerManagedBy(mgr).
543554
For(&appsv1.Deployment{}, OnlyMetadata).
555+
Named("deployment-6").
544556
Owns(&appsv1.ReplicaSet{}, OnlyMetadata).
545557
Watches(&appsv1.StatefulSet{},
546558
handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []reconcile.Request {

‎pkg/controller/controller.go

+4
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt
131131
return nil, fmt.Errorf("must specify Name for Controller")
132132
}
133133

134+
if err := checkName(name); err != nil {
135+
return nil, err
136+
}
137+
134138
if options.LogConstructor == nil {
135139
log := mgr.GetLogger().WithValues(
136140
"controller", name,

‎pkg/controller/controller_test.go

+30-16
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,20 @@ var _ = Describe("controller.Controller", func() {
6060
Expect(err.Error()).To(ContainSubstring("must specify Reconciler"))
6161
})
6262

63+
It("should return an error if two controllers are registered with the same name", func() {
64+
m, err := manager.New(cfg, manager.Options{})
65+
Expect(err).NotTo(HaveOccurred())
66+
67+
c1, err := controller.New("c3", m, controller.Options{Reconciler: rec})
68+
Expect(err).NotTo(HaveOccurred())
69+
Expect(c1).ToNot(BeNil())
70+
71+
c2, err := controller.New("c3", m, controller.Options{Reconciler: rec})
72+
Expect(err).To(HaveOccurred())
73+
Expect(err.Error()).To(ContainSubstring("controller with name c3 already exists"))
74+
Expect(c2).To(BeNil())
75+
})
76+
6377
It("should not return an error if two controllers are registered with different names", func() {
6478
m, err := manager.New(cfg, manager.Options{})
6579
Expect(err).NotTo(HaveOccurred())
@@ -99,7 +113,7 @@ var _ = Describe("controller.Controller", func() {
99113
m, err := manager.New(cfg, manager.Options{})
100114
Expect(err).NotTo(HaveOccurred())
101115

102-
c, err := controller.New("new-controller", m, controller.Options{Reconciler: rec})
116+
c, err := controller.New("new-controller-0", m, controller.Options{Reconciler: rec})
103117
Expect(c.Watch(watch)).To(Succeed())
104118
Expect(err).NotTo(HaveOccurred())
105119

@@ -125,7 +139,7 @@ var _ = Describe("controller.Controller", func() {
125139
m, err := manager.New(cfg, manager.Options{})
126140
Expect(err).NotTo(HaveOccurred())
127141

128-
_, err = controller.New("new-controller", m, controller.Options{Reconciler: rec})
142+
_, err = controller.New("new-controller-1", m, controller.Options{Reconciler: rec})
129143
Expect(err).NotTo(HaveOccurred())
130144

131145
// force-close keep-alive connections. These'll time anyway (after
@@ -138,7 +152,7 @@ var _ = Describe("controller.Controller", func() {
138152
m, err := manager.New(cfg, manager.Options{})
139153
Expect(err).NotTo(HaveOccurred())
140154

141-
c, err := controller.New("new-controller", m, controller.Options{
155+
c, err := controller.New("new-controller-2", m, controller.Options{
142156
Reconciler: reconcile.Func(nil),
143157
})
144158
Expect(err).NotTo(HaveOccurred())
@@ -161,7 +175,7 @@ var _ = Describe("controller.Controller", func() {
161175
return nil
162176
}
163177

164-
c, err := controller.New("new-controller", m, controller.Options{
178+
c, err := controller.New("new-controller-3", m, controller.Options{
165179
Reconciler: reconcile.Func(nil),
166180
RateLimiter: customRateLimiter,
167181
NewQueue: customNewQueue,
@@ -180,7 +194,7 @@ var _ = Describe("controller.Controller", func() {
180194
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{RecoverPanic: ptr.To(true)}})
181195
Expect(err).NotTo(HaveOccurred())
182196

183-
c, err := controller.New("new-controller", m, controller.Options{
197+
c, err := controller.New("new-controller-4", m, controller.Options{
184198
Reconciler: reconcile.Func(nil),
185199
})
186200
Expect(err).NotTo(HaveOccurred())
@@ -213,7 +227,7 @@ var _ = Describe("controller.Controller", func() {
213227
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: ptr.To(true)}})
214228
Expect(err).NotTo(HaveOccurred())
215229

216-
c, err := controller.New("new-controller", m, controller.Options{
230+
c, err := controller.New("new-controller-5", m, controller.Options{
217231
Reconciler: reconcile.Func(nil),
218232
})
219233
Expect(err).NotTo(HaveOccurred())
@@ -228,7 +242,7 @@ var _ = Describe("controller.Controller", func() {
228242
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: ptr.To(true)}})
229243
Expect(err).NotTo(HaveOccurred())
230244

231-
c, err := controller.New("new-controller", m, controller.Options{
245+
c, err := controller.New("new-controller-6", m, controller.Options{
232246
NeedLeaderElection: ptr.To(false),
233247
Reconciler: reconcile.Func(nil),
234248
})
@@ -244,7 +258,7 @@ var _ = Describe("controller.Controller", func() {
244258
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{MaxConcurrentReconciles: 5}})
245259
Expect(err).NotTo(HaveOccurred())
246260

247-
c, err := controller.New("new-controller", m, controller.Options{
261+
c, err := controller.New("new-controller-7", m, controller.Options{
248262
Reconciler: reconcile.Func(nil),
249263
})
250264
Expect(err).NotTo(HaveOccurred())
@@ -259,7 +273,7 @@ var _ = Describe("controller.Controller", func() {
259273
m, err := manager.New(cfg, manager.Options{})
260274
Expect(err).NotTo(HaveOccurred())
261275

262-
c, err := controller.New("new-controller", m, controller.Options{
276+
c, err := controller.New("new-controller-8", m, controller.Options{
263277
Reconciler: reconcile.Func(nil),
264278
})
265279
Expect(err).NotTo(HaveOccurred())
@@ -274,7 +288,7 @@ var _ = Describe("controller.Controller", func() {
274288
m, err := manager.New(cfg, manager.Options{})
275289
Expect(err).NotTo(HaveOccurred())
276290

277-
c, err := controller.New("new-controller", m, controller.Options{
291+
c, err := controller.New("new-controller-9", m, controller.Options{
278292
Reconciler: reconcile.Func(nil),
279293
MaxConcurrentReconciles: 5,
280294
})
@@ -290,7 +304,7 @@ var _ = Describe("controller.Controller", func() {
290304
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{CacheSyncTimeout: 5}})
291305
Expect(err).NotTo(HaveOccurred())
292306

293-
c, err := controller.New("new-controller", m, controller.Options{
307+
c, err := controller.New("new-controller-10", m, controller.Options{
294308
Reconciler: reconcile.Func(nil),
295309
})
296310
Expect(err).NotTo(HaveOccurred())
@@ -305,7 +319,7 @@ var _ = Describe("controller.Controller", func() {
305319
m, err := manager.New(cfg, manager.Options{})
306320
Expect(err).NotTo(HaveOccurred())
307321

308-
c, err := controller.New("new-controller", m, controller.Options{
322+
c, err := controller.New("new-controller-11", m, controller.Options{
309323
Reconciler: reconcile.Func(nil),
310324
})
311325
Expect(err).NotTo(HaveOccurred())
@@ -320,7 +334,7 @@ var _ = Describe("controller.Controller", func() {
320334
m, err := manager.New(cfg, manager.Options{})
321335
Expect(err).NotTo(HaveOccurred())
322336

323-
c, err := controller.New("new-controller", m, controller.Options{
337+
c, err := controller.New("new-controller-12", m, controller.Options{
324338
Reconciler: reconcile.Func(nil),
325339
CacheSyncTimeout: 5,
326340
})
@@ -336,7 +350,7 @@ var _ = Describe("controller.Controller", func() {
336350
m, err := manager.New(cfg, manager.Options{})
337351
Expect(err).NotTo(HaveOccurred())
338352

339-
c, err := controller.New("new-controller", m, controller.Options{
353+
c, err := controller.New("new-controller-13", m, controller.Options{
340354
Reconciler: rec,
341355
})
342356
Expect(err).NotTo(HaveOccurred())
@@ -351,7 +365,7 @@ var _ = Describe("controller.Controller", func() {
351365
m, err := manager.New(cfg, manager.Options{})
352366
Expect(err).NotTo(HaveOccurred())
353367

354-
c, err := controller.New("new-controller", m, controller.Options{
368+
c, err := controller.New("new-controller-14", m, controller.Options{
355369
NeedLeaderElection: ptr.To(false),
356370
Reconciler: rec,
357371
})
@@ -367,7 +381,7 @@ var _ = Describe("controller.Controller", func() {
367381
m, err := manager.New(cfg, manager.Options{})
368382
Expect(err).NotTo(HaveOccurred())
369383

370-
c, err := controller.New("new-controller", m, controller.Options{
384+
c, err := controller.New("new-controller-15", m, controller.Options{
371385
Reconciler: rec,
372386
})
373387
Expect(err).NotTo(HaveOccurred())

‎pkg/controller/name.go

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/*
2+
Copyright 2020 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package controller
18+
19+
import (
20+
"fmt"
21+
"sync"
22+
23+
"k8s.io/apimachinery/pkg/util/sets"
24+
)
25+
26+
var nameLock sync.Mutex
27+
var usedNamed sets.Set[string]
28+
29+
func checkName(name string) error {
30+
nameLock.Lock()
31+
defer nameLock.Unlock()
32+
if usedNamed == nil {
33+
usedNamed = sets.Set[string]{}
34+
}
35+
36+
if usedNamed.Has(name) {
37+
return fmt.Errorf("controller with name %s already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric", name)
38+
}
39+
40+
usedNamed.Insert(name)
41+
42+
return nil
43+
}

0 commit comments

Comments
 (0)