test(reconciler): refactor reconciler tests with k8s decorator

- Use fake k8s decorator for reconciler tests
- Consolidate fake reconciler factory constructors
- Add gRPC registry pod check unit tests
- Refactor gRPC registry pod reconcile unit tests

Author: njhale

pkg/controller/registry/reconciler/configmap.go

@@ -26,26 +26,36 @@ type configMapCatalogSourceDecorator struct {
 	*v1alpha1.CatalogSource
 }
 
+const (
+	// ConfigMapServerPostfix is a postfix appended to the names of resources generated for a ConfigMap server.
+	ConfigMapServerPostfix string = "-configmap-server"
+)
+
 func (s *configMapCatalogSourceDecorator) serviceAccountName() string {
-	return s.GetName() + "-configmap-server"
+	return s.GetName() + ConfigMapServerPostfix
 }
 
 func (s *configMapCatalogSourceDecorator) roleName() string {
-	return s.GetName() + "-configmap-reader"
+	return s.GetName() + ConfigMapServerPostfix
 }
 
 func (s *configMapCatalogSourceDecorator) Selector() map[string]string {
 	return map[string]string{
-		"olm.catalogSource": s.GetName(),
+		CatalogSourceLabelKey: s.GetName(),
 	}
 }
 
+const (
+	// ConfigMapRVLabelKey is the key for a label used to track the resource version of a related ConfigMap.
+	ConfigMapRVLabelKey string = "olm.configMapResourceVersion"
+)
+
 func (s *configMapCatalogSourceDecorator) Labels() map[string]string {
 	labels := map[string]string{
-		"olm.catalogSource": s.GetName(),
+		CatalogSourceLabelKey: s.GetName(),
 	}
 	if s.Spec.SourceType == v1alpha1.SourceTypeInternal || s.Spec.SourceType == v1alpha1.SourceTypeConfigmap {
-		labels["olm.configMapResourceVersion"] = s.Status.ConfigMapResource.ResourceVersion
+		labels[ConfigMapRVLabelKey] = s.Status.ConfigMapResource.ResourceVersion
 	}
 	return labels
 }
@@ -123,7 +133,7 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod {
 					Operator: v1.TolerationOpExists,
 				},
 			},
-			ServiceAccountName: s.GetName() + "-configmap-server",
+			ServiceAccountName: s.GetName() + ConfigMapServerPostfix,
 		},
 	}
 	ownerutil.AddOwner(pod, s.CatalogSource, false, false)
@@ -160,10 +170,15 @@ func (s *configMapCatalogSourceDecorator) Role() *rbacv1.Role {
 	return role
 }
 
+const (
+	// ConfigMapReaderPostfix is the postfix applied to the name of reader RoleBinding generated for a ConfigMap server.
+	ConfigMapReaderPostfix string = ConfigMapServerPostfix + "-reader"
+)
+
 func (s *configMapCatalogSourceDecorator) RoleBinding() *rbacv1.RoleBinding {
 	rb := &rbacv1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      s.GetName() + "-server-configmap-reader",
+			Name:      s.GetName() + ConfigMapReaderPostfix,
 			Namespace: s.GetNamespace(),
 		},
 		Subjects: []rbacv1.Subject{
@@ -442,7 +457,7 @@ func (c *ConfigMapRegistryReconciler) CheckRegistryServer(catalogSource *v1alpha
 		c.currentRole(source) == nil ||
 		c.currentRoleBinding(source) == nil ||
 		c.currentService(source) == nil ||
-		len(c.currentPods(source, c.Image)) != 1 {
+		len(c.currentPods(source, c.Image)) < 1 {
 		healthy = false
 		return
 	}

pkg/controller/registry/reconciler/configmap_test.go

@@ -2,32 +2,73 @@ package reconciler
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
 	"github.com/ghodss/yaml"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/informers"
-	k8sfake "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/tools/cache"
+	k8slabels "k8s.io/kubernetes/pkg/util/labels"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
 )
 
 const (
 	registryImageName = "test:image"
 	testNamespace     = "testns"
 )
 
-func cmReconciler(t *testing.T, k8sObjs []runtime.Object, stopc <-chan struct{}) (*ConfigMapRegistryReconciler, operatorclient.ClientInterface) {
-	opClientFake := operatorclient.NewClient(k8sfake.NewSimpleClientset(k8sObjs...), nil, nil)
+type fakeReconcilerConfig struct {
+	k8sObjs              []runtime.Object
+	k8sClientOptions     []clientfake.Option
+	configMapServerImage string
+}
+
+type fakeReconcilerOption func(*fakeReconcilerConfig)
+
+func withK8sObjs(k8sObjs ...runtime.Object) fakeReconcilerOption {
+	return func(config *fakeReconcilerConfig) {
+		config.k8sObjs = k8sObjs
+	}
+}
+
+func withK8sClientOptions(options ...clientfake.Option) fakeReconcilerOption {
+	return func(config *fakeReconcilerConfig) {
+		config.k8sClientOptions = options
+	}
+}
+
+func withConfigMapServerImage(configMapServerImage string) fakeReconcilerOption {
+	return func(config *fakeReconcilerConfig) {
+		config.configMapServerImage = configMapServerImage
+	}
+}
+
+func fakeReconcilerFactory(t *testing.T, stopc <-chan struct{}, options ...fakeReconcilerOption) (RegistryReconcilerFactory, operatorclient.ClientInterface) {
+	config := &fakeReconcilerConfig{
+		configMapServerImage: registryImageName,
+	}
+
+	// Apply all config options
+	for _, option := range options {
+		option(config)
+	}
+
+	opClientFake := operatorclient.NewClient(clientfake.NewReactionForwardingClientsetDecorator(config.k8sObjs, config.k8sClientOptions...), nil, nil)
 
 	// Creates registry pods in response to configmaps
 	informerFactory := informers.NewSharedInformerFactory(opClientFake.KubernetesInterface(), 5*time.Second)
@@ -55,10 +96,10 @@ func cmReconciler(t *testing.T, k8sObjs []runtime.Object, stopc <-chan struct{})
 	lister.CoreV1().RegisterPodLister(testNamespace, podInformer.Lister())
 	lister.CoreV1().RegisterConfigMapLister(testNamespace, configMapInformer.Lister())
 
-	rec := &ConfigMapRegistryReconciler{
-		Image:    registryImageName,
-		OpClient: opClientFake,
-		Lister:   lister,
+	rec := &registryReconcilerFactory{
+		OpClient:             opClientFake,
+		Lister:               lister,
+		ConfigMapServerImage: config.configMapServerImage,
 	}
 
 	var hasSyncedCheckFns []cache.InformerSynced
@@ -137,14 +178,27 @@ func validConfigMapCatalogSource(configMap *corev1.ConfigMap) *v1alpha1.CatalogS
 }
 
 func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object {
-	decorated := configMapCatalogSourceDecorator{catsrc}
-	objs := []runtime.Object{
-		decorated.Pod(registryImageName),
-		decorated.Service(),
-		decorated.ServiceAccount(),
-		decorated.Role(),
-		decorated.RoleBinding(),
+	var objs []runtime.Object
+	switch catsrc.Spec.SourceType {
+	case v1alpha1.SourceTypeInternal, v1alpha1.SourceTypeConfigmap:
+		decorated := configMapCatalogSourceDecorator{catsrc}
+		objs = clientfake.AddSimpleGeneratedNames(
+			clientfake.AddSimpleGeneratedName(decorated.Pod(registryImageName)),
+			decorated.Service(),
+			decorated.ServiceAccount(),
+			decorated.Role(),
+			decorated.RoleBinding(),
+		)
+	case v1alpha1.SourceTypeGrpc:
+		if catsrc.Spec.Image != "" {
+			decorated := grpcCatalogSourceDecorator{catsrc}
+			objs = clientfake.AddSimpleGeneratedNames(
+				decorated.Pod(),
+				decorated.Service(),
+			)
+		}
 	}
+
 	blockOwnerDeletion := false
 	isController := false
 	for _, o := range objs {
@@ -161,14 +215,30 @@ func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object {
 	return objs
 }
 
-func modifyObjName(objs []runtime.Object, kind, newName string) []runtime.Object {
+func modifyObjName(objs []runtime.Object, kind runtime.Object, newName string) []runtime.Object {
 	out := []runtime.Object{}
-	for _, o := range objs {
-		if o.GetObjectKind().GroupVersionKind().Kind == kind {
-			mo := o.(metav1.Object)
-			mo.SetName(newName)
-			out = append(out, mo.(runtime.Object))
-			continue
+	t := reflect.TypeOf(kind)
+	for _, obj := range objs {
+		o := obj.DeepCopyObject()
+		if reflect.TypeOf(o) == t {
+			if accessor, err := meta.Accessor(o); err == nil {
+				accessor.SetName(newName)
+			}
+		}
+		out = append(out, o)
+	}
+	return out
+}
+
+func setLabel(objs []runtime.Object, kind runtime.Object, label, value string) []runtime.Object {
+	out := []runtime.Object{}
+	t := reflect.TypeOf(kind)
+	for _, obj := range objs {
+		o := obj.DeepCopyObject()
+		if reflect.TypeOf(o) == t {
+			if accessor, err := meta.Accessor(o); err == nil {
+				k8slabels.AddLabel(accessor.GetLabels(), label, value)
+			}
 		}
 		out = append(out, o)
 	}
@@ -235,7 +305,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			testName: "ExistingRegistry/BadServiceAccount",
 			in: in{
 				cluster: cluster{
-					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "ServiceAccount", "badName"), validConfigMap),
+					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &corev1.ServiceAccount{}, "badName"), validConfigMap),
 				},
 				catsrc: validCatalogSource,
 			},
@@ -253,7 +323,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			testName: "ExistingRegistry/BadService",
 			in: in{
 				cluster: cluster{
-					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Service", "badName"), validConfigMap),
+					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &corev1.Service{}, "badName"), validConfigMap),
 				},
 				catsrc: validCatalogSource,
 			},
@@ -271,7 +341,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			testName: "ExistingRegistry/BadPod",
 			in: in{
 				cluster: cluster{
-					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Pod", "badName"), validConfigMap),
+					k8sObjs: append(setLabel(objectsForCatalogSource(validCatalogSource), &corev1.Pod{}, CatalogSourceLabelKey, "badValue"), validConfigMap),
 				},
 				catsrc: validCatalogSource,
 			},
@@ -289,7 +359,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			testName: "ExistingRegistry/BadRole",
 			in: in{
 				cluster: cluster{
-					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Role", "badName"), validConfigMap),
+					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &rbacv1.Role{}, "badName"), validConfigMap),
 				},
 				catsrc: validCatalogSource,
 			},
@@ -307,7 +377,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			testName: "ExistingRegistry/BadRoleBinding",
 			in: in{
 				cluster: cluster{
-					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "RoleBinding", "badName"), validConfigMap),
+					k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &rbacv1.RoleBinding{}, "badName"), validConfigMap),
 				},
 				catsrc: validCatalogSource,
 			},
@@ -345,7 +415,8 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			stopc := make(chan struct{})
 			defer close(stopc)
 
-			rec, client := cmReconciler(t, tt.in.cluster.k8sObjs, stopc)
+			factory, client := fakeReconcilerFactory(t, stopc, withK8sObjs(tt.in.cluster.k8sObjs...), withK8sClientOptions(clientfake.WithNameGeneration(t)))
+			rec := factory.ReconcilerForSource(tt.in.catsrc)
 
 			err := rec.EnsureRegistryServer(tt.in.catsrc)
 
@@ -360,9 +431,14 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			decorated := configMapCatalogSourceDecorator{tt.in.catsrc}
 
 			pod := decorated.Pod(registryImageName)
-			outPod, err := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).Get(pod.GetName(), metav1.GetOptions{})
+			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
+			outPods, err := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(listOptions)
 			require.NoError(t, err)
-			require.Equal(t, pod, outPod)
+			require.Len(t, outPods.Items, 1)
+			outPod := outPods.Items[0]
+			require.Equal(t, pod.GetGenerateName(), outPod.GetGenerateName())
+			require.Equal(t, pod.GetLabels(), outPod.GetLabels())
+			require.Equal(t, pod.Spec, outPod.Spec)
 
 			service := decorated.Service()
 			outService, err := client.KubernetesInterface().CoreV1().Services(service.GetNamespace()).Get(service.GetName(), metav1.GetOptions{})

pkg/controller/registry/reconciler/grpc.go

@@ -3,16 +3,17 @@ package reconciler
 import (
 	"fmt"
 
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/intstr"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 // grpcCatalogSourceDecorator wraps CatalogSource to add additional methods
@@ -22,13 +23,13 @@ type grpcCatalogSourceDecorator struct {
 
 func (s *grpcCatalogSourceDecorator) Selector() labels.Selector {
 	return labels.SelectorFromValidatedSet(map[string]string{
-		"olm.catalogSource": s.GetName(),
+		CatalogSourceLabelKey: s.GetName(),
 	})
 }
 
 func (s *grpcCatalogSourceDecorator) Labels() map[string]string {
 	return map[string]string{
-		"olm.catalogSource": s.GetName(),
+		CatalogSourceLabelKey: s.GetName(),
 	}
 }