Merge pull request #835 from njhale/fix-registry-recreation

Fix gRPC registry pod recreation

pkg/api/client/clientset/versioned/fake/decorator.go

@@ -18,6 +18,8 @@ package fake
 import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/testing"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake"
 )
 
 // ClientsetDecorator defines decorator methods for a Clientset.
@@ -38,14 +40,19 @@ type ReactionForwardingClientsetDecorator struct {
 
 // NewReactionForwardingClientsetDecorator returns the ReactionForwardingClientsetDecorator wrapped Clientset result
 // of calling NewSimpleClientset with the given objects.
-func NewReactionForwardingClientsetDecorator(objects ...runtime.Object) *ReactionForwardingClientsetDecorator {
+func NewReactionForwardingClientsetDecorator(objects []runtime.Object, options ...clientfake.Option) *ReactionForwardingClientsetDecorator {
 	decorator := &ReactionForwardingClientsetDecorator{
 		Clientset: *NewSimpleClientset(objects...),
 	}
 
 	// Swap out the embedded ReactionChain with a Reactor that reduces over the decorator's ReactionChain.
 	decorator.ReactionChain = decorator.Clientset.ReactionChain
 	decorator.Clientset.ReactionChain = []testing.Reactor{&testing.SimpleReactor{"*", "*", decorator.reduceReactions}}
+
+	// Apply options
+	for _, option := range options {
+		option(decorator)
+	}
 
 	return decorator
 }

pkg/controller/operators/catalog/operator_test.go

@@ -16,13 +16,11 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
-	clitesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
 	apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
@@ -34,6 +32,7 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/fakes"
+	"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"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
@@ -379,8 +378,6 @@ func TestSyncCatalogSources(t *testing.T) {
 	}
 }
 
-// TODO: CatalogSource tests for RegistryServiceStatus
-
 func TestCompetingCRDOwnersExist(t *testing.T) {
 
 	testNamespace := "default"
@@ -465,43 +462,13 @@ func fakeConfigMapData() map[string]string {
 	return data
 }
 
-// fakeClientOption configures a fake option
-type fakeClientOption func(fake.ClientsetDecorator)
-
-// withSelfLinks returns a fakeClientOption that configures a ClientsetDecorator to write selfLinks to all OLM types on create.
-func withSelfLinks(t *testing.T) fakeClientOption {
-	return func(c fake.ClientsetDecorator) {
-		c.PrependReactor("create", "*", func(a clitesting.Action) (bool, runtime.Object, error) {
-			ca, ok := a.(clitesting.CreateAction)
-			if !ok {
-				t.Fatalf("expected CreateAction")
-			}
-
-			obj := ca.GetObject()
-			accessor, err := meta.Accessor(obj)
-			if err != nil {
-				return false, nil, err
-			}
-			if accessor.GetSelfLink() != "" {
-				// SelfLink is already set
-				return false, nil, nil
-			}
-
-			gvr := ca.GetResource()
-			accessor.SetSelfLink(buildSelfLink(gvr.GroupVersion().String(), gvr.Resource, accessor.GetNamespace(), accessor.GetName()))
-
-			return false, obj, nil
-		})
-	}
-}
-
 // fakeOperatorConfig is the configuration for a fake operator.
 type fakeOperatorConfig struct {
-	clientObjs        []runtime.Object
-	k8sObjs           []runtime.Object
-	extObjs           []runtime.Object
-	regObjs           []runtime.Object
-	fakeClientOptions []fakeClientOption
+	clientObjs    []runtime.Object
+	k8sObjs       []runtime.Object
+	extObjs       []runtime.Object
+	regObjs       []runtime.Object
+	clientOptions []clientfake.Option
 }
 
 // fakeOperatorOption applies an option to the given fake operator configuration.
@@ -525,9 +492,9 @@ func extObjs(extObjs ...runtime.Object) fakeOperatorOption {
 	}
 }
 
-func withFakeClientOptions(options ...fakeClientOption) fakeOperatorOption {
+func withFakeClientOptions(options ...clientfake.Option) fakeOperatorOption {
 	return func(config *fakeOperatorConfig) {
-		config.fakeClientOptions = options
+		config.clientOptions = options
 	}
 }
 
@@ -540,11 +507,7 @@ func NewFakeOperator(namespace string, watchedNamespaces []string, stopCh <-chan
 	}
 
 	// Create client fakes
-	clientFake := fake.NewReactionForwardingClientsetDecorator(config.clientObjs...)
-	for _, option := range config.fakeClientOptions {
-		option(clientFake)
-	}
-
+	clientFake := fake.NewReactionForwardingClientsetDecorator(config.clientObjs, config.clientOptions...)
 	opClientFake := operatorclient.NewClient(k8sfake.NewSimpleClientset(config.k8sObjs...), apiextensionsfake.NewSimpleClientset(config.extObjs...), apiregistrationfake.NewSimpleClientset(config.regObjs...))
 
 	// Create operator namespace
@@ -707,11 +670,3 @@ func toManifest(obj runtime.Object) string {
 	raw, _ := json.Marshal(obj)
 	return string(raw)
 }
-
-// selfLink returns a selfLink.
-func buildSelfLink(groupVersion, plural, namespace, name string) string {
-	if namespace == metav1.NamespaceAll {
-		return fmt.Sprintf("/apis/%s/%s/%s", groupVersion, plural, name)
-	}
-	return fmt.Sprintf("/apis/%s/namespaces/%s/%s/%s", groupVersion, namespace, plural, name)
-}

pkg/controller/operators/catalog/subscriptions_test.go

@@ -14,15 +14,16 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/fakes"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake"
 )
 
 func TestSyncSubscriptions(t *testing.T) {
 	now := metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC)
 	timeNow = func() metav1.Time { return now }
-
 	testNamespace := "testNamespace"
+
 	type fields struct {
-		fakeClientOptions []fakeClientOption
+		clientOptions     []clientfake.Option
 		sourcesLastUpdate metav1.Time
 		resolveSteps      []*v1alpha1.Step
 		resolveSubs       []*v1alpha1.Subscription
@@ -51,7 +52,7 @@ func TestSyncSubscriptions(t *testing.T) {
 		{
 			name: "NoStatus/NoCurrentCSV/FoundInCatalog",
 			fields: fields{
-				fakeClientOptions: []fakeClientOption{withSelfLinks(t)},
+				clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)},
 				existingOLMObjs: []runtime.Object{
 					&v1alpha1.Subscription{
 						ObjectMeta: metav1.ObjectMeta{
@@ -182,7 +183,7 @@ func TestSyncSubscriptions(t *testing.T) {
 		{
 			name: "Status/HaveCurrentCSV/UpdateFoundInCatalog",
 			fields: fields{
-				fakeClientOptions: []fakeClientOption{withSelfLinks(t)},
+				clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)},
 				existingOLMObjs: []runtime.Object{
 					&v1alpha1.ClusterServiceVersion{
 						ObjectMeta: metav1.ObjectMeta{
@@ -322,7 +323,7 @@ func TestSyncSubscriptions(t *testing.T) {
 		{
 			name: "Status/HaveCurrentCSV/UpdateFoundInCatalog/UpdateRequiresDependency",
 			fields: fields{
-				fakeClientOptions: []fakeClientOption{withSelfLinks(t)},
+				clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)},
 				existingOLMObjs: []runtime.Object{
 					&v1alpha1.ClusterServiceVersion{
 						ObjectMeta: metav1.ObjectMeta{
@@ -514,7 +515,7 @@ func TestSyncSubscriptions(t *testing.T) {
 			// Create test operator
 			stopCh := make(chan struct{})
 			defer func() { stopCh <- struct{}{} }()
-			o, err := NewFakeOperator(testNamespace, []string{testNamespace}, stopCh, withClientObjs(tt.fields.existingOLMObjs...), withK8sObjs(tt.fields.existingObjects...), withFakeClientOptions(tt.fields.fakeClientOptions...))
+			o, err := NewFakeOperator(testNamespace, []string{testNamespace}, stopCh, withClientObjs(tt.fields.existingOLMObjs...), withK8sObjs(tt.fields.existingObjects...), withFakeClientOptions(tt.fields.clientOptions...))
 			require.NoError(t, err)
 
 			o.reconciler = &fakes.FakeRegistryReconcilerFactory{

pkg/controller/registry/reconciler/configmap.go

@@ -26,8 +26,13 @@ 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 {
@@ -36,16 +41,21 @@ func (s *configMapCatalogSourceDecorator) roleName() string {
 
 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)
@@ -442,7 +452,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
 	}