Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Add SkipNameValidation option #2918

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/config/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ import "time"

// Controller contains configuration options for a controller.
type Controller struct {
// SkipNameValidation allows skipping the name validation that ensures that every controller name is unique.
// Unique controller names are important to get unique metrics and logs for a controller.
// Can be overwritten for a controller via the SkipNameValidation setting on the controller.
// Defaults to false if SkipNameValidation setting on controller and Manager are unset.
SkipNameValidation *bool

// GroupKindConcurrency is a map from a Kind to the number of concurrent reconciliation
// allowed for that controller.
//
Expand All @@ -40,8 +46,8 @@ type Controller struct {
CacheSyncTimeout time.Duration

// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
// Defaults to the Controller.RecoverPanic setting from the Manager if unset.
// Defaults to true if Controller.RecoverPanic setting from the Manager is also unset.
// Can be overwritten for a controller via the RecoverPanic setting on the controller.
// Defaults to true if RecoverPanic setting on controller and Manager are unset.
RecoverPanic *bool

// NeedLeaderElection indicates whether the controller needs to use leader election.
Expand Down
16 changes: 14 additions & 2 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ type Options = TypedOptions[reconcile.Request]

// TypedOptions are the arguments for creating a new Controller.
type TypedOptions[request comparable] struct {
// SkipNameValidation allows skipping the name validation that ensures that every controller name is unique.
// Unique controller names are important to get unique metrics and logs for a controller.
// Defaults to the Controller.SkipNameValidation setting from the Manager if unset.
// Defaults to false if Controller.SkipNameValidation setting from the Manager is also unset.
SkipNameValidation *bool

// MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. Defaults to 1.
MaxConcurrentReconciles int

Expand Down Expand Up @@ -140,8 +146,14 @@ 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.SkipNameValidation == nil {
options.SkipNameValidation = mgr.GetControllerOptions().SkipNameValidation
}

if options.SkipNameValidation == nil || !*options.SkipNameValidation {
if err := checkName(name); err != nil {
return nil, err
}
}

if options.LogConstructor == nil {
Expand Down
48 changes: 48 additions & 0 deletions pkg/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,54 @@ var _ = Describe("controller.Controller", func() {
Expect(c2).To(BeNil())
})

It("should return an error if two controllers are registered with the same name and SkipNameValidation is set to false on the manager", func() {
m, err := manager.New(cfg, manager.Options{
Controller: config.Controller{
SkipNameValidation: ptr.To(false),
},
})
Expect(err).NotTo(HaveOccurred())

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

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

It("should not return an error if two controllers are registered with the same name and SkipNameValidation is set on the manager", func() {
m, err := manager.New(cfg, manager.Options{
Controller: config.Controller{
SkipNameValidation: ptr.To(true),
},
})
Expect(err).NotTo(HaveOccurred())

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

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

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

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

c2, err := controller.New("c6", m, controller.Options{Reconciler: rec, SkipNameValidation: ptr.To(true)})
Expect(err).NotTo(HaveOccurred())
Expect(c2).ToNot(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