Refactor controllers registration

internal/reconciler/reconciler_suite_test.go → internal/controller/controller_suite_test.go

@@ -1,4 +1,4 @@
-package reconciler_test
+package controller_test
 
 import (
 	"testing"

internal/controller/doc.go

@@ -0,0 +1,11 @@
+/*
+Package controller is responsible for creating and registering controllers for
+sigs.k8s.io/controller-runtime/pkg/manager.Manager.
+
+A controller is responsible for watching for updates to the resource of a desired type and propagating those updates
+as events through the event channel.
+
+The reconciliation part of a controller -- reacting on a resource change -- is implemented by the Reconciler type,
+which in turn implements sigs.k8s.io/controller-runtime/pkg/reconcile.Reconciler.
+*/
+package controller

internal/manager/fakes.go → internal/controller/fakes.go

@@ -1,4 +1,4 @@
-package manager
+package controller
 
 import (
 	_ "sigs.k8s.io/controller-runtime/pkg/client"  // used below to generate a fake

internal/reconciler/getter.go → internal/controller/getter.go

@@ -1,4 +1,4 @@
-package reconciler
+package controller
 
 import (
 	"context"

internal/reconciler/implementation.go → internal/controller/reconciler.go

@@ -1,4 +1,4 @@
-package reconciler
+package controller
 
 import (
 	"context"
@@ -18,8 +18,8 @@ import (
 // If the function returns false, the reconciler will log the returned string.
 type NamespacedNameFilterFunc func(nsname types.NamespacedName) (bool, string)
 
-// Config contains the configuration for the Implementation.
-type Config struct {
+// ReconcilerConfig is the configuration for the reconciler.
+type ReconcilerConfig struct {
 	// Getter gets a resource from the k8s API.
 	Getter Getter
 	// ObjectType is the type of the resource that the reconciler will reconcile.
@@ -30,21 +30,21 @@ type Config struct {
 	NamespacedNameFilter NamespacedNameFilterFunc
 }
 
-// Implementation is a reconciler for Kubernetes resources.
+// Reconciler reconciles Kubernetes resources of a specific type.
 // It implements the reconcile.Reconciler interface.
 // A successful reconciliation of a resource has the two possible outcomes:
 // (1) If the resource is deleted, the Implementation will send a DeleteEvent to the event channel.
 // (2) If the resource is upserted (created or updated), the Implementation will send an UpsertEvent
 // to the event channel.
-type Implementation struct {
-	cfg Config
+type Reconciler struct {
+	cfg ReconcilerConfig
 }
 
-var _ reconcile.Reconciler = &Implementation{}
+var _ reconcile.Reconciler = &Reconciler{}
 
-// NewImplementation creates a new Implementation.
-func NewImplementation(cfg Config) *Implementation {
-	return &Implementation{
+// NewReconciler creates a new reconciler.
+func NewReconciler(cfg ReconcilerConfig) *Reconciler {
+	return &Reconciler{
 		cfg: cfg,
 	}
 }
@@ -59,7 +59,7 @@ func newObject(objectType client.Object) client.Object {
 }
 
 // Reconcile implements the reconcile.Reconciler Reconcile method.
-func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
+func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
 	logger := log.FromContext(ctx)
 	// The controller runtime has set the logger with the group, kind, namespace and name of the resource,
 	// and a few other key/value pairs. So we don't need to set them here.

internal/reconciler/implementation_test.go → internal/controller/reconciler_test.go

@@ -1,4 +1,4 @@
-package reconciler_test
+package controller_test
 
 import (
 	"context"
@@ -14,10 +14,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/gateway-api/apis/v1beta1"
 
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler/reconcilerfakes"
-
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/controller"
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/controller/controllerfakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
 )
 
 type getFunc func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error
@@ -29,8 +28,8 @@ type result struct {
 
 var _ = Describe("Reconciler", func() {
 	var (
-		rec        *reconciler.Implementation
-		fakeGetter *reconcilerfakes.FakeGetter
+		rec        *controller.Reconciler
+		fakeGetter *controllerfakes.FakeGetter
 		eventCh    chan interface{}
 
 		hr1NsName = types.NamespacedName{
@@ -108,7 +107,7 @@ var _ = Describe("Reconciler", func() {
 	}
 
 	BeforeEach(func() {
-		fakeGetter = &reconcilerfakes.FakeGetter{}
+		fakeGetter = &controllerfakes.FakeGetter{}
 		eventCh = make(chan interface{})
 	})
 
@@ -136,7 +135,7 @@ var _ = Describe("Reconciler", func() {
 
 		When("Reconciler doesn't have a filter", func() {
 			BeforeEach(func() {
-				rec = reconciler.NewImplementation(reconciler.Config{
+				rec = controller.NewReconciler(controller.ReconcilerConfig{
 					Getter:     fakeGetter,
 					ObjectType: &v1beta1.HTTPRoute{},
 					EventCh:    eventCh,
@@ -161,7 +160,7 @@ var _ = Describe("Reconciler", func() {
 					return true, ""
 				}
 
-				rec = reconciler.NewImplementation(reconciler.Config{
+				rec = controller.NewReconciler(controller.ReconcilerConfig{
 					Getter:               fakeGetter,
 					ObjectType:           &v1beta1.HTTPRoute{},
 					EventCh:              eventCh,
@@ -203,7 +202,7 @@ var _ = Describe("Reconciler", func() {
 
 	Describe("Edge cases", func() {
 		BeforeEach(func() {
-			rec = reconciler.NewImplementation(reconciler.Config{
+			rec = controller.NewReconciler(controller.ReconcilerConfig{
 				Getter:     fakeGetter,
 				ObjectType: &v1beta1.HTTPRoute{},
 				EventCh:    eventCh,
@@ -221,7 +220,7 @@ var _ = Describe("Reconciler", func() {
 		})
 
 		DescribeTable("Reconciler should not block when ctx is done",
-			func(get getFunc, invalidResourceEventCount int, nsname types.NamespacedName) {
+			func(get getFunc, nsname types.NamespacedName) {
 				fakeGetter.GetCalls(get)
 
 				ctx, cancel := context.WithCancel(context.Background())
@@ -232,9 +231,8 @@ var _ = Describe("Reconciler", func() {
 				Consistently(eventCh).ShouldNot(Receive())
 				Expect(resultCh).To(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}})))
 			},
-			Entry("Upserting valid HTTPRoute", getReturnsHRForHR(hr1), 0, hr1NsName),
-			Entry("Deleting valid HTTPRoute", getReturnsNotFoundErrorForHR(hr1), 0, hr1NsName),
-			Entry("Upserting invalid HTTPRoute", getReturnsHRForHR(hr2), 1, hr2NsName),
+			Entry("Upserting HTTPRoute", getReturnsHRForHR(hr1), hr1NsName),
+			Entry("Deleting HTTPRoute", getReturnsNotFoundErrorForHR(hr1), hr1NsName),
 		)
 	})
 })

internal/manager/controllers.go → internal/controller/register.go

@@ -1,4 +1,4 @@
-package manager
+package controller
 
 import (
 	"context"
@@ -11,64 +11,71 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/index"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
 )
 
 const (
 	// addIndexFieldTimeout is the timeout used for adding an Index Field to a cache.
 	addIndexFieldTimeout = 2 * time.Minute
 )
 
-type newReconcilerFunc func(cfg reconciler.Config) *reconciler.Implementation
-
-type controllerConfig struct {
-	namespacedNameFilter reconciler.NamespacedNameFilterFunc
+type config struct {
+	namespacedNameFilter NamespacedNameFilterFunc
 	k8sPredicate         predicate.Predicate
 	fieldIndices         index.FieldIndices
-	newReconciler        newReconcilerFunc
+	newReconciler        NewReconcilerFunc
 }
 
-type controllerOption func(*controllerConfig)
+// NewReconcilerFunc defines a function that creates a new Reconciler. Used for unit-testing.
+type NewReconcilerFunc func(cfg ReconcilerConfig) *Reconciler
+
+// Option defines configuration options for registering a controller.
+type Option func(*config)
 
-func withNamespacedNameFilter(filter reconciler.NamespacedNameFilterFunc) controllerOption {
-	return func(cfg *controllerConfig) {
+// WithNamespacedNameFilter enables filtering of objects by NamespacedName by the controller.
+func WithNamespacedNameFilter(filter NamespacedNameFilterFunc) Option {
+	return func(cfg *config) {
 		cfg.namespacedNameFilter = filter
 	}
 }
 
-func withK8sPredicate(p predicate.Predicate) controllerOption {
-	return func(cfg *controllerConfig) {
+// WithK8sPredicate enables filtering of events before they are sent to the controller.
+func WithK8sPredicate(p predicate.Predicate) Option {
+	return func(cfg *config) {
 		cfg.k8sPredicate = p
 	}
 }
 
-func withFieldIndices(fieldIndices index.FieldIndices) controllerOption {
-	return func(cfg *controllerConfig) {
+// WithFieldIndices adds indices to the FieldIndexer of the manager.
+func WithFieldIndices(fieldIndices index.FieldIndices) Option {
+	return func(cfg *config) {
 		cfg.fieldIndices = fieldIndices
 	}
 }
 
-// withNewReconciler allows us to mock reconciler creation in the unit tests.
-func withNewReconciler(newReconciler newReconcilerFunc) controllerOption {
-	return func(cfg *controllerConfig) {
+// WithNewReconciler allows us to mock reconciler creation in the unit tests.
+func WithNewReconciler(newReconciler NewReconcilerFunc) Option {
+	return func(cfg *config) {
 		cfg.newReconciler = newReconciler
 	}
 }
 
-func defaultControllerConfig() controllerConfig {
-	return controllerConfig{
-		newReconciler: reconciler.NewImplementation,
+func defaultConfig() config {
+	return config{
+		newReconciler: NewReconciler,
 	}
 }
 
-func registerController(
+// Register registers a new controller for the object type in the manager and configure it with the provided options.
+// If the options include WithFieldIndices, it will add the specified indices to FieldIndexer of the manager.
+// The registered controller will send events to the provided channel.
+func Register(
 	ctx context.Context,
 	objectType client.Object,
 	mgr manager.Manager,
 	eventCh chan<- interface{},
-	options ...controllerOption,
+	options ...Option,
 ) error {
-	cfg := defaultControllerConfig()
+	cfg := defaultConfig()
 
 	for _, opt := range options {
 		opt(&cfg)
@@ -87,7 +94,7 @@ func registerController(
 		builder = builder.WithEventFilter(cfg.k8sPredicate)
 	}
 
-	recCfg := reconciler.Config{
+	recCfg := ReconcilerConfig{
 		Getter:               mgr.GetClient(),
 		ObjectType:           objectType,
 		EventCh:              eventCh,

internal/manager/controllers_test.go → internal/controller/register_test.go

@@ -1,4 +1,4 @@
-package manager
+package controller_test
 
 import (
 	"context"
@@ -15,26 +15,26 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/gateway-api/apis/v1beta1"
 
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/controller"
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/controller/controllerfakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/filter"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/index"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/managerfakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
 )
 
-func TestRegisterController(t *testing.T) {
+func TestRegister(t *testing.T) {
 	type fakes struct {
-		mgr     *managerfakes.FakeManager
-		indexer *managerfakes.FakeFieldIndexer
+		mgr     *controllerfakes.FakeManager
+		indexer *controllerfakes.FakeFieldIndexer
 	}
 
 	getDefaultFakes := func() fakes {
-		scheme = runtime.NewScheme()
+		scheme := runtime.NewScheme()
 		utilruntime.Must(v1beta1.AddToScheme(scheme))
 
-		indexer := &managerfakes.FakeFieldIndexer{}
+		indexer := &controllerfakes.FakeFieldIndexer{}
 
-		mgr := &managerfakes.FakeManager{}
+		mgr := &controllerfakes.FakeManager{}
 		mgr.GetClientReturns(fake.NewClientBuilder().Build())
 		mgr.GetSchemeReturns(scheme)
 		mgr.GetLoggerReturns(zap.New())
@@ -97,24 +97,24 @@ func TestRegisterController(t *testing.T) {
 		t.Run(test.msg, func(t *testing.T) {
 			g := NewGomegaWithT(t)
 
-			newReconciler := func(c reconciler.Config) *reconciler.Implementation {
+			newReconciler := func(c controller.ReconcilerConfig) *controller.Reconciler {
 				g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient()))
 				g.Expect(c.ObjectType).To(BeIdenticalTo(objectType))
 				g.Expect(c.EventCh).To(BeIdenticalTo(eventCh))
 				g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(namespacedNameFilter))
 
-				return reconciler.NewImplementation(c)
+				return controller.NewReconciler(c)
 			}
 
-			err := registerController(
+			err := controller.Register(
 				context.Background(),
 				objectType,
 				test.fakes.mgr,
 				eventCh,
-				withNamespacedNameFilter(namespacedNameFilter),
-				withK8sPredicate(predicate.ServicePortsChangedPredicate{}),
-				withFieldIndices(fieldIndexes),
-				withNewReconciler(newReconciler),
+				controller.WithNamespacedNameFilter(namespacedNameFilter),
+				controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}),
+				controller.WithFieldIndices(fieldIndexes),
+				controller.WithNewReconciler(newReconciler),
 			)
 
 			if test.expectedErr == nil {