Skip to content

Commit

Permalink
Merge pull request #2918 from sbueringer/pr-add-skip-name-validation
Browse files Browse the repository at this point in the history
🌱 Add SkipNameValidation option
  • Loading branch information
k8s-ci-robot authored Aug 13, 2024
2 parents 96e8152 + 89bebe3 commit 9f5afec
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 4 deletions.
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

0 comments on commit 9f5afec

Please sign in to comment.