Get Manager tests to 100% coverage

Author: pwittrock

pkg/controller/controller.go

@@ -109,6 +109,9 @@ type controller struct {
 	// defaults to cache.WaitForCacheSync
 	waitForCache func(stopCh <-chan struct{}, cacheSyncs ...cache.InformerSynced) bool
 
+	// started is true if the Controller has been started
+	started bool
+
 	// TODO(pwittrock): Consider initializing a logger with the controller name as the tag
 }
 
@@ -178,6 +181,8 @@ func (c *controller) Start(stop <-chan struct{}) error {
 		}, c.jitterPeriod, stop)
 	}
 
+	c.started = true
+
 	<-stop
 	log.Info("Stopping workers", "controller", c.name)
 	return nil

pkg/controller/controller_suite_integration_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/kubernetes-sigs/controller-runtime/pkg/controller"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/internal/informer"
 	logf "github.com/kubernetes-sigs/controller-runtime/pkg/runtime/log"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/test"
@@ -46,6 +47,7 @@ var _ = BeforeSuite(func() {
 
 	var err error
 	cfg, err = testenv.Start()
+	controller.TestConfig = cfg
 	Expect(err).NotTo(HaveOccurred())
 
 	time.Sleep(1 * time.Second)

pkg/controller/manager.go

@@ -21,12 +21,12 @@ import (
 	"sync"
 
 	"github.com/kubernetes-sigs/controller-runtime/pkg/client"
-	"github.com/kubernetes-sigs/controller-runtime/pkg/client/config"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/controller/reconcile"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/internal/apiutil"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/internal/informer"
 	"github.com/kubernetes-sigs/controller-runtime/pkg/runtime/inject"
 	"k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
@@ -92,8 +92,12 @@ func (cm *controllerManager) NewController(ca Args, r reconcile.Reconcile) (Cont
 	cm.mu.Lock()
 	defer cm.mu.Unlock()
 
+	if r == nil {
+		return nil, fmt.Errorf("must specify Reconcile")
+	}
+
 	if len(ca.Name) == 0 {
-		return nil, fmt.Errorf("must specify name for Controller")
+		return nil, fmt.Errorf("must specify Name for Controller")
 	}
 
 	if ca.MaxConcurrentReconciles <= 0 {
@@ -196,6 +200,9 @@ type ManagerArgs struct {
 	// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources
 	// Defaults to the kubernetes/client-go scheme.Scheme
 	Scheme *runtime.Scheme
+
+	// Mapper is the rest mapper used to map go types to Kubernetes APIs
+	MapperProvider func(c *rest.Config) (meta.RESTMapper, error)
 }
 
 // NewManager returns a new fully initialized Manager.
@@ -204,11 +211,7 @@ func NewManager(args ManagerArgs) (Manager, error) {
 
 	// Initialize a rest.config if none was specified
 	if cm.config == nil {
-		var err error
-		cm.config, err = config.GetConfig()
-		if err != nil {
-			return nil, err
-		}
+		return nil, fmt.Errorf("must specify Config")
 	}
 
 	// Use the Kubernetes client-go scheme if none is specified
@@ -228,7 +231,10 @@ func NewManager(args ManagerArgs) (Manager, error) {
 	objCache := client.NewObjectCache(spi.KnownInformersByType(), cm.scheme)
 	spi.Callbacks = append(spi.Callbacks, objCache)
 
-	mapper, err := apiutil.NewDiscoveryRESTMapper(cm.config)
+	if args.MapperProvider == nil {
+		args.MapperProvider = apiutil.NewDiscoveryRESTMapper
+	}
+	mapper, err := args.MapperProvider(cm.config)
 	if err != nil {
 		log.Error(err, "Failed to get API Group-Resources")
 		return nil, err

pkg/controller/manager_test.go

@@ -17,91 +17,260 @@ limitations under the License.
 package controller
 
 import (
+	"fmt"
+
+	"github.com/kubernetes-sigs/controller-runtime/pkg/client"
+	"github.com/kubernetes-sigs/controller-runtime/pkg/controller/reconcile"
+	"github.com/kubernetes-sigs/controller-runtime/pkg/internal/informer"
+	"github.com/kubernetes-sigs/controller-runtime/pkg/internal/informer/informertest"
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/cache"
 )
 
+var TestConfig *rest.Config
+
 var _ = Describe("controller", func() {
+	var stop chan struct{}
+
+	rec := reconcile.Func(func(reconcile.Request) (reconcile.Result, error) {
+		return reconcile.Result{}, nil
+	})
 	BeforeEach(func() {
+		stop = make(chan struct{})
 	})
 
 	AfterEach(func() {
+		close(stop)
 	})
 
 	Describe("Creating a Manager", func() {
 
-		It("should return an error if Name is not Specified", func() {
+		It("should return an error if there is no Config", func() {
+			m, err := NewManager(ManagerArgs{})
+			Expect(m).To(BeNil())
+			Expect(err.Error()).To(ContainSubstring("must specify Config"))
 
 		})
 
 		It("should return an error if it can't create a RestMapper", func() {
-
-		})
-
-		It("should return an error if it cannot get a Config", func() {
-
-		})
-
-		It("should default the Config if none is specified", func() {
+			expected := fmt.Errorf("expected error: RestMapper")
+			m, err := NewManager(ManagerArgs{
+				Config:         TestConfig,
+				MapperProvider: func(c *rest.Config) (meta.RESTMapper, error) { return nil, expected },
+			})
+			Expect(m).To(BeNil())
+			Expect(err).To(Equal(expected))
 
 		})
 	})
 
 	Describe("Staring a Manager", func() {
 
 		It("should Start each Controller", func() {
-
+			// TODO(community): write this
 		})
 
-		It("should return an error if any Controllers fail to stop", func() {
-
+		It("should return an error if any Controllers fail to stop", func(done Done) {
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			c, err := m.NewController(Args{Name: "foo"}, rec)
+			Expect(err).NotTo(HaveOccurred())
+			ctrl, ok := c.(*controller)
+			Expect(ok).To(BeTrue())
+
+			// Make Controller startup fail
+			ctrl.waitForCache = func(stopCh <-chan struct{}, cacheSyncs ...cache.InformerSynced) bool { return false }
+			err = m.Start(stop)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("caches to sync"))
+
+			close(done)
 		})
 	})
 
 	Describe("Manager", func() {
 		It("should provide a function to get the Config", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+			Expect(m.GetConfig()).To(Equal(mrg.config))
 		})
 
 		It("should provide a function to get the Client", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+			Expect(m.GetClient()).To(Equal(mrg.client))
 		})
 
 		It("should provide a function to get the Scheme", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+			Expect(m.GetScheme()).To(Equal(mrg.scheme))
 		})
 
 		It("should provide a function to get the FieldIndexer", func() {
-
-		})
-	})
-
-	Describe("Creating a Controller", func() {
-
-		It("should immediately start the Controller if the ControllerManager has already Started", func() {
-
-		})
-
-		It("should provide an inject function for providing dependencies", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+			Expect(m.GetFieldIndexer()).To(Equal(mrg.fieldIndexes))
 		})
 	})
 
 	Describe("Creating a Controller", func() {
 		It("should return an error if Name is not Specified", func() {
-
-		})
-
-		It("should return an error if it cannot get a Config", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			c, err := m.NewController(Args{}, rec)
+			Expect(c).To(BeNil())
+			Expect(err.Error()).To(ContainSubstring("must specify Name for Controller"))
 		})
 
-		It("should default the Config if none is specified", func() {
+		It("should return an error if Reconcile is not Specified", func() {
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			c, err := m.NewController(Args{Name: "foo"}, nil)
+			Expect(c).To(BeNil())
+			Expect(err.Error()).To(ContainSubstring("must specify Reconcile"))
 
 		})
 
 		It("should immediately start the Controller if the ControllerManager has already Started", func() {
-
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+
+			// Make Controller startup fail
+			go func() {
+				defer GinkgoRecover()
+				Expect(m.Start(stop)).NotTo(HaveOccurred())
+			}()
+			Eventually(func() bool { return mrg.started }).Should(BeTrue())
+
+			c, err := m.NewController(Args{Name: "Foo"}, rec)
+			Expect(err).NotTo(HaveOccurred())
+			ctrl, ok := c.(*controller)
+			Expect(ok).To(BeTrue())
+
+			// Wait for Controller to start
+			Eventually(func() bool { return ctrl.started }).Should(BeTrue())
 		})
 
+		It("should provide an inject function for providing dependencies", func(done Done) {
+			m, err := NewManager(ManagerArgs{Config: TestConfig})
+			Expect(err).NotTo(HaveOccurred())
+			mrg, ok := m.(*controllerManager)
+			Expect(ok).To(BeTrue())
+
+			mrg.informers = &informertest.FakeInformers{}
+
+			c, err := m.NewController(Args{Name: "foo"}, rec)
+			Expect(err).NotTo(HaveOccurred())
+			ctrl, ok := c.(*controller)
+			Expect(ok).To(BeTrue())
+
+			By("Injecting the dependencies")
+			err = ctrl.inject(&injectable{
+				scheme: func(scheme *runtime.Scheme) error {
+					defer GinkgoRecover()
+					Expect(scheme).To(Equal(mrg.scheme))
+					return nil
+				},
+				config: func(config *rest.Config) error {
+					defer GinkgoRecover()
+					Expect(config).To(Equal(mrg.config))
+					return nil
+				},
+				client: func(client client.Interface) error {
+					defer GinkgoRecover()
+					Expect(client).To(Equal(mrg.client))
+					return nil
+				},
+				informers: func(informers informer.Informers) error {
+					defer GinkgoRecover()
+					Expect(informers).To(Equal(mrg.informers))
+					return nil
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Returning an error if dependency injection fails")
+
+			expected := fmt.Errorf("expected error")
+			err = ctrl.inject(&injectable{
+				client: func(client client.Interface) error {
+					return expected
+				},
+			})
+			Expect(err).To(Equal(expected))
+
+			err = ctrl.inject(&injectable{
+				scheme: func(scheme *runtime.Scheme) error {
+					return expected
+				},
+			})
+			Expect(err).To(Equal(expected))
+
+			err = ctrl.inject(&injectable{
+				config: func(config *rest.Config) error {
+					return expected
+				},
+			})
+			Expect(err).To(Equal(expected))
+
+			err = ctrl.inject(&injectable{
+				informers: func(informers informer.Informers) error {
+					return expected
+				},
+			})
+			Expect(err).To(Equal(expected))
+
+			close(done)
+		})
 	})
 })
+
+type injectable struct {
+	scheme    func(scheme *runtime.Scheme) error
+	client    func(client.Interface) error
+	config    func(config *rest.Config) error
+	informers func(informer.Informers) error
+}
+
+func (i *injectable) InjectInformers(informers informer.Informers) error {
+	if i.informers == nil {
+		return nil
+	}
+	return i.informers(informers)
+}
+
+func (i *injectable) InjectConfig(config *rest.Config) error {
+	if i.config == nil {
+		return nil
+	}
+	return i.config(config)
+}
+
+func (i *injectable) InjectClient(c client.Interface) error {
+	if i.client == nil {
+		return nil
+	}
+	return i.client(c)
+}
+
+func (i *injectable) InjectScheme(scheme *runtime.Scheme) error {
+	if i.scheme == nil {
+		return nil
+	}
+	return i.scheme(scheme)
+}