Skip to content

Commit 22d2b35

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 22d2b35

File tree

3 files changed

+77
-16
lines changed

3 files changed

+77
-16
lines changed

‎pkg/controller/controller.go

Lines changed: 4 additions & 0 deletions
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

Lines changed: 30 additions & 16 deletions
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

Lines changed: 43 additions & 0 deletions
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)