From 37c8aa19e2151efacd43d00f95fdd285026eaea5 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Fri, 26 Jul 2019 11:33:14 -0400 Subject: [PATCH] Return controller from builder.Build Return controller from builder.Build to allow developers to use it after it's been created by the builder; e.g., to add watches dynamically from a reconciler. Remove deprecated SimpleController() and Builder.WithManager(). Signed-off-by: Andy Goldstein --- pkg/builder/controller.go | 60 ++++------------------ pkg/builder/controller_test.go | 88 +++++++++------------------------ pkg/builder/webhook.go | 15 ++---- pkg/builder/webhook_test.go | 3 -- pkg/patterns/application/doc.go | 2 +- 5 files changed, 37 insertions(+), 131 deletions(-) diff --git a/pkg/builder/controller.go b/pkg/builder/controller.go index d89f3696ff..4f7a1ffc3e 100644 --- a/pkg/builder/controller.go +++ b/pkg/builder/controller.go @@ -23,7 +23,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" - "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -33,9 +32,7 @@ import ( ) // Supporting mocking out functions for testing -var getConfig = config.GetConfig var newController = controller.New -var newManager = manager.New var getGvk = apiutil.GVKForObject // Builder builds a Controller. @@ -50,16 +47,9 @@ type Builder struct { name string } -// SimpleController returns a new Builder. -// -// Deprecated: Use ControllerManagedBy(Manager) instead. -func SimpleController() *Builder { - return &Builder{} -} - // ControllerManagedBy returns a new controller builder that will be started by the provided Manager func ControllerManagedBy(m manager.Manager) *Builder { - return SimpleController().WithManager(m) + return &Builder{mgr: m} } // ForType defines the type of Object being *reconciled*, and configures the ControllerManagedBy to respond to create / delete / @@ -109,14 +99,6 @@ func (blder *Builder) WithConfig(config *rest.Config) *Builder { return blder } -// WithManager sets the Manager to use for registering the ControllerManagedBy. Defaults to a new manager.Manager. -// -// Deprecated: Use ControllerManagedBy(Manager) and this isn't needed. -func (blder *Builder) WithManager(m manager.Manager) *Builder { - blder.mgr = m - return blder -} - // WithEventFilter sets the event filters, to filter which create/update/delete/generic events eventually // trigger reconciliations. For example, filtering on whether the resource version has changed. // Defaults to the empty list. @@ -141,23 +123,17 @@ func (blder *Builder) Complete(r reconcile.Reconciler) error { return err } -// Build builds the Application ControllerManagedBy and returns the Manager used to start it. -// -// Deprecated: Use Complete -func (blder *Builder) Build(r reconcile.Reconciler) (manager.Manager, error) { +// Build builds the Application ControllerManagedBy and returns the Controller it created. +func (blder *Builder) Build(r reconcile.Reconciler) (controller.Controller, error) { if r == nil { return nil, fmt.Errorf("must provide a non-nil Reconciler") } - - // Set the Config - if err := blder.loadRestConfig(); err != nil { - return nil, err + if blder.mgr == nil { + return nil, fmt.Errorf("must provide a non-nil Manager") } - // Set the Manager - if err := blder.doManager(); err != nil { - return nil, err - } + // Set the Config + blder.loadRestConfig() // Set the ControllerManagedBy if err := blder.doController(r); err != nil { @@ -169,7 +145,7 @@ func (blder *Builder) Build(r reconcile.Reconciler) (manager.Manager, error) { return nil, err } - return blder.mgr, nil + return blder.ctrl, nil } func (blder *Builder) doWatch() error { @@ -203,26 +179,10 @@ func (blder *Builder) doWatch() error { return nil } -func (blder *Builder) loadRestConfig() error { - if blder.config != nil { - return nil - } - if blder.mgr != nil { +func (blder *Builder) loadRestConfig() { + if blder.config == nil { blder.config = blder.mgr.GetConfig() - return nil } - var err error - blder.config, err = getConfig() - return err -} - -func (blder *Builder) doManager() error { - if blder.mgr != nil { - return nil - } - var err error - blder.mgr, err = newManager(blder.config, manager.Options{}) - return err } func (blder *Builder) getControllerName() (string, error) { diff --git a/pkg/builder/controller_test.go b/pkg/builder/controller_test.go index 29917edbe5..18c9dba047 100644 --- a/pkg/builder/controller_test.go +++ b/pkg/builder/controller_test.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -44,9 +43,7 @@ var _ = Describe("application", func() { BeforeEach(func() { stop = make(chan struct{}) - getConfig = func() (*rest.Config, error) { return cfg, nil } newController = controller.New - newManager = manager.New }) AfterEach(func() { @@ -57,27 +54,24 @@ var _ = Describe("application", func() { Describe("New", func() { It("should return success if given valid objects", func() { - instance, err := SimpleController(). - For(&appsv1.ReplicaSet{}). - Owns(&appsv1.ReplicaSet{}). - Build(noop) + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) Expect(err).NotTo(HaveOccurred()) - Expect(instance).NotTo(BeNil()) - }) - It("should return an error if the Config is invalid", func() { - getConfig = func() (*rest.Config, error) { return cfg, fmt.Errorf("expected error") } - instance, err := SimpleController(). + instance, err := ControllerManagedBy(m). For(&appsv1.ReplicaSet{}). Owns(&appsv1.ReplicaSet{}). Build(noop) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("expected error")) - Expect(instance).To(BeNil()) + Expect(err).NotTo(HaveOccurred()) + Expect(instance).NotTo(BeNil()) }) It("should return an error if there is no GVK for an object", func() { - instance, err := SimpleController(). + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + instance, err := ControllerManagedBy(m). For(&fakeType{}). Owns(&appsv1.ReplicaSet{}). Build(noop) @@ -85,7 +79,7 @@ var _ = Describe("application", func() { Expect(err.Error()).To(ContainSubstring("no kind is registered for the type builder.fakeType")) Expect(instance).To(BeNil()) - instance, err = SimpleController(). + instance, err = ControllerManagedBy(m). For(&appsv1.ReplicaSet{}). Owns(&fakeType{}). Build(noop) @@ -94,25 +88,17 @@ var _ = Describe("application", func() { Expect(instance).To(BeNil()) }) - It("should return an error if it cannot create the manager", func() { - newManager = func(config *rest.Config, options manager.Options) (manager.Manager, error) { - return nil, fmt.Errorf("expected error") - } - instance, err := SimpleController(). - For(&appsv1.ReplicaSet{}). - Owns(&appsv1.ReplicaSet{}). - Build(noop) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("expected error")) - Expect(instance).To(BeNil()) - }) - It("should return an error if it cannot create the controller", func() { newController = func(name string, mgr manager.Manager, options controller.Options) ( controller.Controller, error) { return nil, fmt.Errorf("expected error") } - instance, err := SimpleController(). + + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + instance, err := ControllerManagedBy(m). For(&appsv1.ReplicaSet{}). Owns(&appsv1.ReplicaSet{}). Build(noop) @@ -150,30 +136,6 @@ var _ = Describe("application", func() { }) }) - Describe("Start with SimpleController", func() { - It("should Reconcile Owns objects", func(done Done) { - bldr := SimpleController(). - ForType(&appsv1.Deployment{}). - WithConfig(cfg). - Owns(&appsv1.ReplicaSet{}) - doReconcileTest("1", stop, bldr, nil, false) - - close(done) - }, 10) - - It("should Reconcile Owns objects with a Manager", func(done Done) { - m, err := manager.New(cfg, manager.Options{}) - Expect(err).NotTo(HaveOccurred()) - - bldr := SimpleController(). - WithManager(m). - For(&appsv1.Deployment{}). - Owns(&appsv1.ReplicaSet{}) - doReconcileTest("2", stop, bldr, m, false) - close(done) - }, 10) - }) - Describe("Start with ControllerManagedBy", func() { It("should Reconcile Owns objects", func(done Done) { m, err := manager.New(cfg, manager.Options{}) @@ -217,25 +179,21 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr return reconcile.Result{}, nil }) - instance := mgr if complete { err := blder.Complete(fn) Expect(err).NotTo(HaveOccurred()) } else { var err error - instance, err = blder.Build(fn) + var c controller.Controller + c, err = blder.Build(fn) Expect(err).NotTo(HaveOccurred()) - } - - // Manager should match - if mgr != nil { - Expect(instance).To(Equal(mgr)) + Expect(c).NotTo(BeNil()) } By("Starting the application") go func() { defer GinkgoRecover() - Expect(instance.Start(stop)).NotTo(HaveOccurred()) + Expect(mgr.Start(stop)).NotTo(HaveOccurred()) By("Stopping the application") }() @@ -263,7 +221,7 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr }, }, } - err := instance.GetClient().Create(context.TODO(), dep) + err := mgr.GetClient().Create(context.TODO(), dep) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Deployment Reconcile") @@ -293,7 +251,7 @@ func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr Template: dep.Spec.Template, }, } - err = instance.GetClient().Create(context.TODO(), rs) + err = mgr.GetClient().Create(context.TODO(), rs) Expect(err).NotTo(HaveOccurred()) By("Waiting for the ReplicaSet Reconcile") diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 89fb62c71b..ada4ddd239 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -55,25 +55,16 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { // Complete builds the webhook. func (blder *WebhookBuilder) Complete() error { // Set the Config - if err := blder.loadRestConfig(); err != nil { - return err - } + blder.loadRestConfig() // Set the Webhook if needed return blder.registerWebhooks() } -func (blder *WebhookBuilder) loadRestConfig() error { - if blder.config != nil { - return nil - } - if blder.mgr != nil { +func (blder *WebhookBuilder) loadRestConfig() { + if blder.config == nil { blder.config = blder.mgr.GetConfig() - return nil } - var err error - blder.config, err = getConfig() - return err } func (blder *WebhookBuilder) registerWebhooks() error { diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 47dd101ce1..d0987d5f80 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -28,7 +28,6 @@ import ( . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/scheme" @@ -40,9 +39,7 @@ var _ = Describe("application", func() { BeforeEach(func() { stop = make(chan struct{}) - getConfig = func() (*rest.Config, error) { return cfg, nil } newController = controller.New - newManager = manager.New }) AfterEach(func() { diff --git a/pkg/patterns/application/doc.go b/pkg/patterns/application/doc.go index ea2bdd219d..5784051b96 100644 --- a/pkg/patterns/application/doc.go +++ b/pkg/patterns/application/doc.go @@ -20,7 +20,7 @@ limitations under the License. // An application is a Controller and Resource that together implement the operational logic for an application. // They are often used to take off-the-shelf OSS applications, and make them Kubernetes native. // -// A typical application Controller may use a new builder.SimpleController() to create a Controller +// A typical application Controller may use builder.ControllerManagedBy() to create a Controller // for a single API type that manages other objects it creates. // // Application Controllers are most useful for stateful applications such as Cassandra, Etcd and MySQL