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

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/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/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{

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) {

test/e2e/operator_groups_e2e_test.go

@@ -8,12 +8,14 @@ import (
 	"time"
 
 	"github.com/coreos/go-semver/semver"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	"k8s.io/apimachinery/pkg/api/errors"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/informers"
@@ -1215,12 +1217,12 @@ func TestCSVCopyWatchingAllNamespaces(t *testing.T) {
 	defer func() {
 		c.DeleteServiceAccount(serviceAccount.GetNamespace(), serviceAccount.GetName(), metav1.NewDeleteOptions(0))
 	}()
-	_, err = c.CreateRole(role)
+	createdRole, err := c.CreateRole(role)
 	require.NoError(t, err)
 	defer func() {
 		c.DeleteRole(role.GetNamespace(), role.GetName(), metav1.NewDeleteOptions(0))
 	}()
-	_, err = c.CreateRoleBinding(roleBinding)
+	createdRoleBinding, err := c.CreateRoleBinding(roleBinding)
 	require.NoError(t, err)
 	defer func() {
 		c.DeleteRoleBinding(roleBinding.GetNamespace(), roleBinding.GetName(), metav1.NewDeleteOptions(0))
@@ -1234,10 +1236,47 @@ func TestCSVCopyWatchingAllNamespaces(t *testing.T) {
 	createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Create(&aCSV)
 	require.NoError(t, err)
 
+	ownerutil.AddOwnerLabels(createdRole, createdCSV)
+	_, err = c.UpdateRole(createdRole)
+	require.NoError(t, err)
+
+	ownerutil.AddOwnerLabels(createdRoleBinding, createdCSV)
+	_, err = c.UpdateRoleBinding(createdRoleBinding)
+	require.NoError(t, err)
+
 	t.Log("wait for CSV to succeed")
 	_, err = fetchCSV(t, crc, createdCSV.GetName(), opGroupNamespace, csvSucceededChecker)
 	require.NoError(t, err)
 
+	t.Log("wait for roles to be promoted to clusterroles")
+	var fetchedRole *rbacv1.ClusterRole
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedRole, err = c.GetClusterRole(role.GetName())
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return false, nil
+			}
+			return false, err
+		}
+		return true, nil
+	})
+	require.EqualValues(t, role.Rules, fetchedRole.Rules)
+	var fetchedRoleBinding *rbacv1.ClusterRoleBinding
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedRoleBinding, err = c.GetClusterRoleBinding(roleBinding.GetName())
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return false, nil
+			}
+			return false, err
+		}
+		return true, nil
+	})
+	require.EqualValues(t, roleBinding.Subjects, fetchedRoleBinding.Subjects)
+	require.EqualValues(t, roleBinding.RoleRef.Name, fetchedRoleBinding.RoleRef.Name)
+	require.EqualValues(t, "rbac.authorization.k8s.io", fetchedRoleBinding.RoleRef.APIGroup)
+	require.EqualValues(t, "ClusterRole", fetchedRoleBinding.RoleRef.Kind)
+
 	t.Log("Waiting for operator namespace csv to have annotations")
 	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
 		fetchedCSV, fetchErr := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Get(csvName, metav1.GetOptions{})