fix(olm): operator group annotations and listers

Author: Jeff Peeler

pkg/controller/operators/olm/operator.go

@@ -27,9 +27,6 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/informers"
-	cappsv1 "k8s.io/client-go/listers/apps/v1"
-	cv1 "k8s.io/client-go/listers/core/v1"
-	crbacv1 "k8s.io/client-go/listers/rbac/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
@@ -48,21 +45,15 @@ const (
 
 type Operator struct {
 	*queueinformer.Operator
-	csvQueue                 workqueue.RateLimitingInterface
-	client                   versioned.Interface
-	resolver                 install.StrategyResolverInterface
-	lister                   operatorlister.OperatorLister
-	roleLister               crbacv1.RoleLister
-	roleBindingLister        crbacv1.RoleBindingLister
-	clusterRoleLister        crbacv1.ClusterRoleLister
-	clusterRoleBindingLister crbacv1.ClusterRoleBindingLister
-	csvLister                map[string]csvlister.ClusterServiceVersionLister
-	operatorGroupLister      map[string]operatorgrouplister.OperatorGroupLister
-	deploymentLister         map[string]cappsv1.DeploymentLister
-	namespaceLister          cv1.NamespaceLister
-	annotator                *annotator.Annotator
-	recorder                 record.EventRecorder
-	cleanupFunc              func()
+	csvQueue            workqueue.RateLimitingInterface
+	client              versioned.Interface
+	resolver            install.StrategyResolverInterface
+	lister              operatorlister.OperatorLister
+	csvLister           map[string]csvlister.ClusterServiceVersionLister
+	operatorGroupLister map[string]operatorgrouplister.OperatorGroupLister
+	annotator           *annotator.Annotator
+	recorder            record.EventRecorder
+	cleanupFunc         func()
 }
 
 func NewOperator(crClient versioned.Interface, opClient operatorclient.ClientInterface, resolver install.StrategyResolverInterface, wakeupInterval time.Duration, annotations map[string]string, namespaces []string) (*Operator, error) {
@@ -210,13 +201,12 @@ func NewOperator(crClient versioned.Interface, opClient operatorclient.ClientInt
 
 	// set up watch on deployments
 	depInformers := []cache.SharedIndexInformer{}
-	op.deploymentLister = make(map[string]cappsv1.DeploymentLister, len(namespaces))
 	for _, namespace := range namespaces {
 		log.Debugf("watching deployments in namespace %s", namespace)
 		informerFactory := informers.NewSharedInformerFactoryWithOptions(opClient.KubernetesInterface(), wakeupInterval, informers.WithNamespace(namespace))
 		informer := informerFactory.Apps().V1().Deployments()
 		depInformers = append(depInformers, informer.Informer())
-		op.deploymentLister[namespace] = informer.Lister()
+		op.lister.AppsV1().RegisterDeploymentLister(namespace, informer.Lister())
 	}
 
 	depQueue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "csv-deployments")

pkg/controller/operators/olm/operator_test.go

@@ -1273,7 +1273,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 				},
 				LastUpdated: timeNow(),
 			},
-			expectedAnnotation: map[string]string{"olm.targetNamespaces": testNS},
+			expectedAnnotation: map[string]string{"olm.targetNamespaces": testNS, "olm.operatorGroup": "operator-group-1", "olm.operatorNamespace": testNS},
 		},
 	}
 
@@ -1298,7 +1298,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 			if tc.expectedAnnotation != nil {
 				// assuming CSVs are in correct namespace
 				for _, ns := range tc.namespaces {
-					deployments, err := op.deploymentLister[ns.Name].List(labels.Everything())
+					deployments, err := op.lister.AppsV1().DeploymentLister().Deployments(ns.GetName()).List(labels.Everything())
 					if err != nil {
 						t.Fatal(err)
 					}

pkg/controller/operators/olm/operatorgroup.go

@@ -2,6 +2,7 @@ package olm
 
 import (
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
@@ -42,7 +43,7 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	}
 	nsListJoined := strings.Join(nsList, ",")
 
-	if err := a.annotateDeployments(op.GetNamespace(), nsListJoined); err != nil {
+	if err := a.annotateDeployments(op, nsListJoined); err != nil {
 		log.Errorf("annotateDeployments error: %v", err)
 		return err
 	}
@@ -51,58 +52,19 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	// annotate csvs
 	csvsInNamespace := a.csvsInNamespace(op.Namespace)
 	for _, csv := range csvsInNamespace {
-		a.addAnnotationsToCSV(csv, op, nsListJoined)
-		// TODO: generate a patch
-		if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).Update(csv); err != nil {
-			log.Errorf("Update for existing CSV failed: %v", err)
-			return err
+		origCSVannotations := csv.GetAnnotations()
+		a.addAnnotationsToObjectMeta(&csv.ObjectMeta, op, nsListJoined)
+		if reflect.DeepEqual(origCSVannotations, csv.GetAnnotations()) == false {
+			// CRDs don't support strategic merge patching, but in the future if they do this should be updated to patch
+			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).Update(csv); err != nil {
+				log.Errorf("Update for existing CSV failed: %v", err)
+			}
 		}
 
 		if err := a.copyCsvToTargetNamespace(csv, op, targetedNamespaces); err != nil {
 			return err
 		}
 	}
-
-	//	// create new CSV instead of DeepCopy as namespace and resource version (and status) will be different
-	//	newCSV := v1alpha1.ClusterServiceVersion{
-	//		ObjectMeta: metav1.ObjectMeta{
-	//			Name: csv.Name,
-	//		},
-	//		Spec: *csv.Spec.DeepCopy(),
-	//		Status: v1alpha1.ClusterServiceVersionStatus{
-	//			Message:        "CSV copied to target namespace",
-	//			Reason:         v1alpha1.CSVReasonCopied,
-	//			LastUpdateTime: timeNow(),
-	//		},
-	//	}
-	//
-	//	a.addAnnotationsToCSV(&newCSV, op, nsListJoined)
-	//
-	//
-	//	for _, ns := range targetedNamespaces {
-	//		newCSV.SetNamespace(ns.Name)
-	//		if ns.Name != op.Namespace {
-	//			log.Debugf("Copying CSV %v to namespace %v", csv.GetName(), ns.GetName())
-	//			_, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.GetName()).Create(&newCSV)
-	//			if k8serrors.IsAlreadyExists(err) {
-	//				a.addAnnotationsToCSV(csv, op, nsListJoined)
-	//				if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.GetName()).Update(csv); err != nil {
-	//					log.Errorf("Update CSV in target namespace failed: %v", err)
-	//					return err
-	//				}
-	//			} else if err != nil {
-	//				log.Errorf("Create for new CSV failed: %v", err)
-	//				return err
-	//			}
-	//		} else {
-	//			a.addAnnotationsToCSV(csv, op, nsListJoined)
-	//			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.GetName()).Update(csv); err != nil {
-	//				log.Errorf("Update for existing CSV failed: %v", err)
-	//				return err
-	//			}
-	//		}
-	//	}
-	//}
 	log.Debug("CSV annotation completed")
 	//TODO: ensure RBAC on operator serviceaccount
 
@@ -142,11 +104,12 @@ func (a *Operator) copyCsvToTargetNamespace(csv *v1alpha1.ClusterServiceVersion,
 			if err != nil {
 				log.Errorf("Create failed, yet get failed: %v", err)
 			}
-			// update the potentially different annotations
-			fetchedCSV.Annotations = csv.Annotations
-			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.Name).Update(fetchedCSV); err != nil {
-				log.Errorf("Update CSV in target namespace failed: %v", err)
-				return err
+			if reflect.DeepEqual(fetchedCSV.Annotations, csv.Annotations) == false {
+				// CRDs don't support strategic merge patching, but in the future if they do this should be updated to patch
+				if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.Name).Update(fetchedCSV); err != nil {
+					log.Errorf("Update CSV in target namespace failed: %v", err)
+					return err
+				}
 			}
 		} else if err != nil {
 			log.Errorf("Create for new CSV failed: %v", err)
@@ -156,10 +119,10 @@ func (a *Operator) copyCsvToTargetNamespace(csv *v1alpha1.ClusterServiceVersion,
 	return nil
 }
 
-func (a *Operator) addAnnotationsToCSV(csv *v1alpha1.ClusterServiceVersion, op *v1alpha2.OperatorGroup, targetNamespaces string) {
-	metav1.SetMetaDataAnnotation(&csv.ObjectMeta, "olm.targetNamespaces", targetNamespaces)
-	metav1.SetMetaDataAnnotation(&csv.ObjectMeta, "olm.operatorNamespace", op.GetNamespace())
-	metav1.SetMetaDataAnnotation(&csv.ObjectMeta, "olm.operatorGroup", op.GetName())
+func (a *Operator) addAnnotationsToObjectMeta(obj *metav1.ObjectMeta, op *v1alpha2.OperatorGroup, targetNamespaces string) {
+	metav1.SetMetaDataAnnotation(obj, "olm.targetNamespaces", targetNamespaces)
+	metav1.SetMetaDataAnnotation(obj, "olm.operatorNamespace", op.GetNamespace())
+	metav1.SetMetaDataAnnotation(obj, "olm.operatorGroup", op.GetName())
 }
 
 func namespacesChanged(clusterNamespaces []*corev1.Namespace, statusNamespaces []*corev1.Namespace) bool {
@@ -232,10 +195,12 @@ func (a *Operator) ensureClusterRoles(op *v1alpha2.OperatorGroup) error {
 		}
 
 		clusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: fmt.Sprintf("owned-crd-manager-%s", csv.GetName()),
+			},
 			Rules: managerPolicyRules,
 		}
 		ownerutil.AddNonBlockingOwner(clusterRole, csv)
-		clusterRole.SetGenerateName(fmt.Sprintf("owned-crd-manager-%s-", csv.Spec.DisplayName))
 		_, err := a.OpClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
 		if k8serrors.IsAlreadyExists(err) {
 			if _, err = a.OpClient.UpdateClusterRole(clusterRole); err != nil {
@@ -284,9 +249,9 @@ func (a *Operator) ensureClusterRoles(op *v1alpha2.OperatorGroup) error {
 	return nil
 }
 
-func (a *Operator) annotateDeployments(operatorNamespace string, targetNamespaceString string) error {
+func (a *Operator) annotateDeployments(op *v1alpha2.OperatorGroup, targetNamespaceString string) error {
 	// write above namespaces to watch in every deployment in operator namespace
-	deploymentList, err := a.deploymentLister[operatorNamespace].List(labels.Everything())
+	deploymentList, err := a.lister.AppsV1().DeploymentLister().Deployments(op.GetNamespace()).List(labels.Everything())
 	if err != nil {
 		log.Errorf("deployment list failed: %v\n", err)
 		return err
@@ -308,12 +273,11 @@ func (a *Operator) annotateDeployments(operatorNamespace string, targetNamespace
 		}
 
 		originalDeploy := deploy.DeepCopy()
-		metav1.SetMetaDataAnnotation(&deploy.Spec.Template.ObjectMeta, "olm.targetNamespaces", targetNamespaceString)
+		a.addAnnotationsToObjectMeta(&deploy.Spec.Template.ObjectMeta, op, targetNamespaceString)
 		if _, _, err := a.OpClient.PatchDeployment(originalDeploy, deploy); err != nil {
 			log.Errorf("patch deployment failed: %v\n", err)
 			return err
 		}
-
 	}
 
 	return nil

test/e2e/operator_groups_e2e_test.go

@@ -182,7 +182,7 @@ func TestOperatorGroup(t *testing.T) {
 	require.Equal(t, createdCSV.Name, fetchedCSV.Name)
 	require.Equal(t, createdCSV.Spec, fetchedCSV.Spec)
 
-	log.Debug("Waiting on deployment to have correct annotation")
+	log.Debug("Waiting on deployment to have correct annotations")
 	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
 		createdDeployment, err := c.GetDeployment(testNamespace, "operator-deployment")
 		if err != nil {
@@ -191,8 +191,7 @@ func TestOperatorGroup(t *testing.T) {
 			}
 			return false, err
 		}
-		// TODO: verify operatorNamespace annotation, operatorGroup annotation
-		if createdDeployment.Spec.Template.Annotations["olm.targetNamespaces"] == otherNamespaceName {
+		if checkOperatorGroupAnnotations(&createdDeployment.Spec.Template, &operatorGroup, otherNamespaceName) == nil {
 			return true, nil
 		}
 		return false, nil