fix(catalog): kind was omitted from ownerlabels

ownerlabel for roles/rolebindings were missing the kind label,
which is used in the selector to determine which roles/bindings are
owned by the operator.

this fixes a bug where roles/rolebindings would not be lifted to
clusterroles/bindings when installed into a global operatorgroup

Author: ecordell

Makefile

@@ -98,6 +98,7 @@ container:
 
 clean-e2e:
 	kubectl delete crds --all
+	kubectl delete apiservices.apiregistration.k8s.io v1alpha1.packages.apps.redhat.com || true
 	kubectl delete -f test/e2e/resources/0000_50_olm_00-namespace.yaml
 
 clean:

pkg/controller/install/deployment.go

@@ -75,7 +75,9 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeployme
 		dep.Spec.Template.SetAnnotations(annotations)
 
 		ownerutil.AddNonBlockingOwner(dep, i.owner)
-		ownerutil.AddOwnerLabels(dep, i.owner)
+		if err := ownerutil.AddOwnerLabels(dep, i.owner); err != nil {
+			return err
+		}
 		if _, err := i.strategyClient.CreateOrUpdateDeployment(dep); err != nil {
 			return err
 		}

pkg/controller/operators/catalog/operator.go

@@ -388,7 +388,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
 
 			return nil
 		}
-		
+
 		logger.Debug("catsrc configmap state good, checking registry pod")
 	}
 
@@ -398,7 +398,6 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
 		return fmt.Errorf("no reconciler for source type %s", catsrc.Spec.SourceType)
 	}
 
-
 	// if registry pod hasn't been created or hasn't been updated since the last configmap update, recreate it
 	if catsrc.Status.RegistryServiceStatus == nil || catsrc.Status.RegistryServiceStatus.CreatedAt.Before(&catsrc.Status.LastSync) {
 		logger.Debug("registry server scheduled recheck")
@@ -545,7 +544,7 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 
 	subs, err := o.lister.OperatorsV1alpha1().SubscriptionLister().Subscriptions(namespace).List(labels.Everything())
 	if err != nil {
-		logger.WithError(err).Debug("couldn't list subscriptions")	
+		logger.WithError(err).Debug("couldn't list subscriptions")
 		return err
 	}
 
@@ -752,7 +751,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
 		} else {
 			out.Status.State = v1alpha1.SubscriptionStateAtLatest
 		}
-		
+
 		out.Status.InstalledCSV = sub.Status.CurrentCSV
 	}
 

pkg/controller/operators/olm/apiservices.go

@@ -93,6 +93,7 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
 
 		// Check if the APIService is adoptable
 		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
+			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
 			err := olmerrors.NewUnadoptableError("", apiServiceName)
 			logger.WithError(err).Warn("found unadoptable apiservice")
 			errs = append(errs, err)
@@ -554,7 +555,10 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 	if err == nil {
 		// Check if the only owners are this CSV or in this CSV's replacement chain.
 		if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, csv) {
-			ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv)
+			logger.WithFields(log.Fields{"obj": "authDelegatorCRB", "labels": existingAuthDelegatorClusterRoleBinding.GetLabels()}).Debug("adopting")
+			if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv); err != nil {
+				return nil, err
+			}
 		}
 
 		// Attempt an update.
@@ -564,7 +568,9 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role.
-		ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv)
+		if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv); err != nil {
+			return nil, err
+		}
 		_, err = a.OpClient.CreateClusterRoleBinding(authDelegatorClusterRoleBinding)
 		if err != nil {
 			log.Warnf("could not create auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
@@ -597,7 +603,10 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 	if err == nil {
 		// Check if the only owners are this CSV or in this CSV's replacement chain.
 		if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, csv) {
-			ownerutil.AddOwnerLabels(authReaderRoleBinding, csv)
+			logger.WithFields(log.Fields{"obj": "existingAuthReaderRB", "labels": existingAuthReaderRoleBinding.GetLabels()}).Debug("adopting")
+			if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, csv); err != nil {
+				return nil, err
+			}
 		}
 		// Attempt an update.
 		if _, err := a.OpClient.UpdateRoleBinding(authReaderRoleBinding); err != nil {
@@ -606,7 +615,9 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 		}
 	} else if k8serrors.IsNotFound(err) {
 		// Create the role.
-		ownerutil.AddOwnerLabels(authReaderRoleBinding, csv)
+		if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, csv); err != nil {
+			return nil, err
+		}
 		_, err = a.OpClient.CreateRoleBinding(authReaderRoleBinding)
 		if err != nil {
 			log.Warnf("could not create auth reader role binding %s", authReaderRoleBinding.GetName())
@@ -706,12 +717,15 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 
 		// check if the APIService is adoptable
 		if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
+			logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
 			return nil, fmt.Errorf("pre-existing APIService %s is not adoptable", apiServiceName)
 		}
 	}
 
 	// Add the CSV as an owner
-	ownerutil.AddOwnerLabels(apiService, csv)
+	if err := ownerutil.AddOwnerLabels(apiService, csv); err != nil {
+		return nil, err
+	}
 
 	// update the ServiceReference
 	apiService.Spec.Service = &apiregistrationv1.ServiceReference{

pkg/controller/operators/olm/operator_test.go

@@ -2987,7 +2987,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            "csv-role",
 			Namespace:       operatorNamespace,
-			Labels:          ownerutil.OwnerLabel(operatorCSV),
+			Labels:          ownerutil.OwnerLabel(operatorCSV, v1alpha1.ClusterServiceVersionKind),
 			OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(operatorCSV)},
 		},
 		Rules: permissions[0].Rules,
@@ -3001,7 +3001,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            "csv-rolebinding",
 			Namespace:       operatorNamespace,
-			Labels:          ownerutil.OwnerLabel(operatorCSV),
+			Labels:          ownerutil.OwnerLabel(operatorCSV, v1alpha1.ClusterServiceVersionKind),
 			OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(operatorCSV)},
 		},
 		Subjects: []rbacv1.Subject{

pkg/controller/operators/olm/operatorgroup.go

@@ -249,30 +249,21 @@ func (a *Operator) ensureClusterRolesForCSV(csv *v1alpha1.ClusterServiceVersion,
 		group := nameGroupPair[1]
 		namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
 
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, AdminSuffix, VerbsForSuffix[AdminSuffix], group, plural, nil); err != nil {
-			return err
-		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, EditSuffix, VerbsForSuffix[EditSuffix], group, plural, nil); err != nil {
-			return err
-		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, ViewSuffix, VerbsForSuffix[ViewSuffix], group, plural, nil); err != nil {
-			return err
+		for suffix, verbs := range VerbsForSuffix {
+			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, group, plural, nil); err != nil {
+				return err
+			}
 		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"-crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
+		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
 			return err
 		}
 	}
 	for _, owned := range csv.Spec.APIServiceDefinitions.Owned {
 		namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
-
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, AdminSuffix, VerbsForSuffix[AdminSuffix], owned.Group, owned.Name, nil); err != nil {
-			return err
-		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, EditSuffix, VerbsForSuffix[EditSuffix], owned.Group, owned.Name, nil); err != nil {
-			return err
-		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, ViewSuffix, VerbsForSuffix[ViewSuffix], owned.Group, owned.Name, nil); err != nil {
-			return err
+		for suffix, verbs := range VerbsForSuffix {
+			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, owned.Group, owned.Name, nil); err != nil {
+				return err
+			}
 		}
 	}
 	return nil
@@ -351,6 +342,9 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
 	if err != nil {
 		return err
 	}
+	if len(ownedRoles) == 0 {
+		return fmt.Errorf("no owned roles found")
+	}
 
 	for _, r := range ownedRoles {
 		a.Log.Debug("processing role")
@@ -363,7 +357,7 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
 				},
 				ObjectMeta: metav1.ObjectMeta{
 					Name:   r.GetName(),
-					Labels: ownerutil.OwnerLabel(csv),
+					Labels: r.GetLabels(),
 				},
 				Rules: r.Rules,
 			}
@@ -378,6 +372,9 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
 	if err != nil {
 		return err
 	}
+	if len(ownedRoleBindings) == 0 {
+		return fmt.Errorf("no owned rolebindings found")
+	}
 
 	for _, r := range ownedRoleBindings {
 		_, err := a.lister.RbacV1().ClusterRoleBindingLister().Get(r.GetName())
@@ -389,7 +386,7 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
 				},
 				ObjectMeta: metav1.ObjectMeta{
 					Name:   r.GetName(),
-					Labels: ownerutil.OwnerLabel(csv),
+					Labels: r.GetLabels(),
 				},
 				Subjects: r.Subjects,
 				RoleRef: rbacv1.RoleRef{
@@ -449,7 +446,9 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
 		// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
 		ownedRole.SetNamespace(targetNamespace)
 		ownedRole.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
-		ownerutil.AddOwnerLabels(ownedRole, targetCSV)
+		if err := ownerutil.AddOwnerLabels(ownedRole, targetCSV); err != nil {
+			return err
+		}
 		ownedRole.SetLabels(utillabels.AddLabel(ownedRole.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
 		if _, err := a.OpClient.CreateRole(ownedRole); err != nil {
 			return err
@@ -489,7 +488,9 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
 		// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
 		ownedRoleBinding.SetNamespace(targetNamespace)
 		ownedRoleBinding.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
-		ownerutil.AddOwnerLabels(ownedRoleBinding, targetCSV)
+		if err := ownerutil.AddOwnerLabels(ownedRoleBinding, targetCSV); err != nil {
+			return err
+		}
 		ownedRoleBinding.SetLabels(utillabels.AddLabel(ownedRoleBinding.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
 		if _, err := a.OpClient.CreateRoleBinding(ownedRoleBinding); err != nil {
 			return err

pkg/controller/registry/resolver/rbac.go

@@ -85,7 +85,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 				Name:            generateName(csv.GetName()),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
-				Labels:          ownerutil.OwnerLabel(csv),
+				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},
 			Rules: permission.Rules,
 		}
@@ -97,7 +97,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 				Name:            generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
-				Labels:          ownerutil.OwnerLabel(csv),
+				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},
 			RoleRef: rbacv1.RoleRef{
 				Kind:     "Role",
@@ -128,7 +128,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 			ObjectMeta: metav1.ObjectMeta{
 				Name:            generateName(csv.GetName()),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
-				Labels:          ownerutil.OwnerLabel(csv),
+				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},
 			Rules: permission.Rules,
 		}
@@ -140,7 +140,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 				Name:            generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
-				Labels:          ownerutil.OwnerLabel(csv),
+				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},
 			RoleRef: rbacv1.RoleRef{
 				Kind:     "ClusterRole",

pkg/lib/ownerutil/util.go

@@ -187,25 +187,29 @@ func NonBlockingOwner(owner Owner) metav1.OwnerReference {
 }
 
 // OwnerLabel returns a label added to generated objects for later querying
-func OwnerLabel(owner Owner) map[string]string {
+func OwnerLabel(owner Owner, kind string) map[string]string {
 	return map[string]string{
 		OwnerKey:          owner.GetName(),
 		OwnerNamespaceKey: owner.GetNamespace(),
-		OwnerKind:         owner.GetObjectKind().GroupVersionKind().Kind,
+		OwnerKind:         kind,
 	}
 }
 
 // AddOwnerLabels adds ownerref-like labels to an object
-func AddOwnerLabels(object metav1.Object, owner Owner) {
+func AddOwnerLabels(object metav1.Object, owner Owner) error {
+	err := InferGroupVersionKind(owner)
+	if err != nil {
+		return err
+	}
 	labels := object.GetLabels()
 	if labels == nil {
 		labels = map[string]string{}
 	}
-	for key, val := range OwnerLabel(owner) {
+	for key, val := range OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) {
 		labels[key] = val
 	}
 	object.SetLabels(labels)
-	return
+	return nil
 }
 
 // IsOwnedByKindLabel returns whether or not a label exists on the object pointing to an owner of a particular kind
@@ -241,7 +245,7 @@ func AdoptableLabels(labels map[string]string, checkName bool, targets ...Owner)
 
 // CSVOwnerSelector returns a label selector to find generated objects owned by owner
 func CSVOwnerSelector(owner *v1alpha1.ClusterServiceVersion) labels.Selector {
-	return labels.SelectorFromSet(OwnerLabel(owner))
+	return labels.SelectorFromSet(OwnerLabel(owner, v1alpha1.ClusterServiceVersionKind))
 }
 
 // AddOwner adds an owner to the ownerref list.

test/e2e/installplan_e2e_test.go

@@ -12,6 +12,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/apis/rbac"
@@ -711,11 +712,36 @@ func TestCreateInstallPlanWithPermissions(t *testing.T) {
 				t.Logf("%v, %v: %v && %v", key, expected, strings.HasPrefix(key.Name, expected.Name), key.Kind == expected.Kind)
 			}
 		}
+
+		// This operator was installed into a global operator group, so the roles should have been lifted to clusterroles
+		if step.Resource.Kind == "Role" {
+			err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+				_, err = c.GetClusterRole(step.Resource.Name)
+				if err != nil {
+					if k8serrors.IsNotFound(err) {
+						return false, nil
+					}
+					return false, err
+				}
+				return true, nil
+			})
+		}
+		if step.Resource.Kind == "RoleBinding" {
+			err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+				_, err = c.GetClusterRoleBinding(step.Resource.Name)
+				if err != nil {
+					if k8serrors.IsNotFound(err) {
+						return false, nil
+					}
+					return false, err
+				}
+				return true, nil
+			})
+		}
 	}
 
 	// Should have removed every matching step
 	require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected: %#v", expectedSteps)
-
 }
 
 func TestInstallPlanCRDValidation(t *testing.T) {