Skip to content

Commit

Permalink
Merge pull request #2902 from alvaroaleman/ensure
Browse files Browse the repository at this point in the history 
⚠️ Validate controller names are unique
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Aug 5, 2024
2 parents 242abfb + 2b94165 commit 3efd16a
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 22 deletions.
2 changes: 2 additions & 0 deletions pkg/builder/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,8 @@ func (blder *TypedBuilder[request]) WithLogConstructor(logConstructor func(*requ
// (underscores and alphanumeric characters only).
//
// By default, controllers are named using the lowercase version of their kind.
//
// The name must be unique as it is used to identify the controller in metrics and logs.
func (blder *TypedBuilder[request]) Named(name string) *TypedBuilder[request] {
blder.name = name
return blder
Expand Down
24 changes: 18 additions & 6 deletions pkg/builder/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ var _ = Describe("application", func() {
Expect(err).NotTo(HaveOccurred())

instance, err := ControllerManagedBy(m).
Named("my_controller").
Named("my_new_controller").
Build(noop)
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")))
Expect(instance).To(BeNil())
Expand All @@ -154,7 +154,7 @@ var _ = Describe("application", func() {
Expect(err).NotTo(HaveOccurred())

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

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

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

instance, err := TypedControllerManagedBy[empty](m).
Named("my_controller").
Named("my_controller-1").
WatchesRawSource(
source.TypedKind(
m.GetCache(),
Expand Down Expand Up @@ -263,6 +264,7 @@ var _ = Describe("application", func() {

builder := ControllerManagedBy(m).
For(&appsv1.ReplicaSet{}).
Named("replicaset-4").
Owns(&appsv1.ReplicaSet{}).
WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles})
builder.newController = newController
Expand Down Expand Up @@ -294,6 +296,7 @@ var _ = Describe("application", func() {

builder := ControllerManagedBy(m).
For(&appsv1.ReplicaSet{}).
Named("replicaset-3").
Owns(&appsv1.ReplicaSet{})
builder.newController = newController

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

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

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

builder := ControllerManagedBy(m).
For(&appsv1.ReplicaSet{}).
Named("replicaset-1").
Owns(&appsv1.ReplicaSet{}).
WithOptions(controller.Options{Reconciler: noop})
instance, err := builder.Build(noop)
Expand Down Expand Up @@ -387,6 +393,7 @@ var _ = Describe("application", func() {
By("creating the 2nd controller")
ctrl2, err := ControllerManagedBy(m).
For(&TestDefaultValidator{}).
Named("test-default-validator-1").
Owns(&appsv1.ReplicaSet{}).
Build(noop)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -401,6 +408,7 @@ var _ = Describe("application", func() {

bldr := ControllerManagedBy(m).
For(&appsv1.Deployment{}).
Named("deployment-0").
Owns(&appsv1.ReplicaSet{})

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

bldr := ControllerManagedBy(m).
For(&appsv1.Deployment{}).
Named("deployment-1").
Owns(&appsv1.ReplicaSet{}, MatchEveryOwner)

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

bldr := ControllerManagedBy(m).
Named("Deployment").
Named("deployment-2").
Watches( // Equivalent of For
&appsv1.Deployment{}, &handler.EnqueueRequestForObject{}).
Watches( // Equivalent of Owns
Expand Down Expand Up @@ -503,6 +513,7 @@ var _ = Describe("application", func() {

bldr := ControllerManagedBy(m).
For(&appsv1.Deployment{}, WithPredicates(deployPrct)).
Named("deployment-3").
Owns(&appsv1.ReplicaSet{}, WithPredicates(replicaSetPrct)).
WithEventFilter(allPrct)

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

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

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

bldr := ControllerManagedBy(mgr).
For(&appsv1.Deployment{}, OnlyMetadata).
Named("deployment-6").
Owns(&appsv1.ReplicaSet{}, OnlyMetadata).
Watches(&appsv1.StatefulSet{},
handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []reconcile.Request {
Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,15 @@ type TypedController[request comparable] interface {

// New returns a new Controller registered with the Manager. The Manager will ensure that shared Caches have
// been synced before the Controller is Started.
//
// The name must be unique as it is used to identify the controller in metrics and logs.
func New(name string, mgr manager.Manager, options Options) (Controller, error) {
return NewTyped(name, mgr, options)
}

// NewTyped returns a new typed controller registered with the Manager,
//
// The name must be unique as it is used to identify the controller in metrics and logs.
func NewTyped[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) {
c, err := NewTypedUnmanaged(name, mgr, options)
if err != nil {
Expand All @@ -117,11 +121,15 @@ func NewTyped[request comparable](name string, mgr manager.Manager, options Type

// NewUnmanaged returns a new controller without adding it to the manager. The
// caller is responsible for starting the returned controller.
//
// The name must be unique as it is used to identify the controller in metrics and logs.
func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller, error) {
return NewTypedUnmanaged(name, mgr, options)
}

// NewTypedUnmanaged returns a new typed controller without adding it to the manager.
//
// The name must be unique as it is used to identify the controller in metrics and logs.
func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) {
if options.Reconciler == nil {
return nil, fmt.Errorf("must specify Reconciler")
Expand All @@ -131,6 +139,10 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt
return nil, fmt.Errorf("must specify Name for Controller")
}

if err := checkName(name); err != nil {
return nil, err
}

if options.LogConstructor == nil {
log := mgr.GetLogger().WithValues(
"controller", name,
Expand Down
46 changes: 30 additions & 16 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,20 @@ var _ = Describe("controller.Controller", func() {
Expect(err.Error()).To(ContainSubstring("must specify Reconciler"))
})

It("should return an error if two controllers are registered with the same name", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c1, err := controller.New("c3", m, controller.Options{Reconciler: rec})
Expect(err).NotTo(HaveOccurred())
Expect(c1).ToNot(BeNil())

c2, err := controller.New("c3", m, controller.Options{Reconciler: rec})
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("controller with name c3 already exists"))
Expect(c2).To(BeNil())
})

It("should not return an error if two controllers are registered with different names", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -99,7 +113,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

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

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

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

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

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-2", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -161,7 +175,7 @@ var _ = Describe("controller.Controller", func() {
return nil
}

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

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-4", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -213,7 +227,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: ptr.To(true)}})
Expect(err).NotTo(HaveOccurred())

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

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

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-7", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -259,7 +273,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-8", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -274,7 +288,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

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

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-10", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -305,7 +319,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-11", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -320,7 +334,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-12", m, controller.Options{
Reconciler: reconcile.Func(nil),
CacheSyncTimeout: 5,
})
Expand All @@ -336,7 +350,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-13", m, controller.Options{
Reconciler: rec,
})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -351,7 +365,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-14", m, controller.Options{
NeedLeaderElection: ptr.To(false),
Reconciler: rec,
})
Expand All @@ -367,7 +381,7 @@ var _ = Describe("controller.Controller", func() {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
c, err := controller.New("new-controller-15", m, controller.Options{
Reconciler: rec,
})
Expect(err).NotTo(HaveOccurred())
Expand Down
Loading

0 comments on commit 3efd16a

Please sign in to comment.