refactor operator group cluster role name

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

Diff for: go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/go-logr/logr v1.2.4
 	github.com/golang/mock v1.6.0
 	github.com/google/go-cmp v0.5.9
+	github.com/google/uuid v1.3.0
 	github.com/googleapis/gnostic v0.5.5
 	github.com/itchyny/gojq v0.11.0
 	github.com/maxbrunsfeld/counterfeiter/v6 v6.2.2
@@ -124,7 +125,6 @@ require (
 	github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect
 	github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2 // indirect
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
-	github.com/google/uuid v1.3.0 // indirect
 	github.com/gorilla/mux v1.8.0 // indirect
 	github.com/gosuri/uitable v0.0.4 // indirect
 	github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 // indirect

Diff for: pkg/controller/operators/olm/operatorgroup.go

@@ -3,6 +3,7 @@ package olm
 import (
 	"context"
 	"fmt"
+	"github.com/google/uuid"
 	"hash/fnv"
 	"reflect"
 	"strings"
@@ -978,32 +979,18 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string,
 }
 
 func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error {
-	clusterRole := &rbacv1.ClusterRole{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: strings.Join([]string{op.GetName(), suffix}, "-"),
-		},
-	}
-	var selectors []metav1.LabelSelector
-	for api := range apis {
-		aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix)
-		if err != nil {
-			return err
-		}
-		selectors = append(selectors, metav1.LabelSelector{
-			MatchLabels: map[string]string{
-				aggregationLabel: "true",
-			},
-		})
-	}
-	if len(selectors) > 0 {
-		clusterRole.AggregationRule = &rbacv1.AggregationRule{
-			ClusterRoleSelectors: selectors,
-		}
+	clusterRole, err := a.getOpGroupClusterRole(op, suffix, apis)
+	if err != nil {
+		return err
 	}
-	err := ownerutil.AddOwnerLabels(clusterRole, op)
+	aggregationRule, err := a.getClusterRoleAggregationRule(apis, suffix)
 	if err != nil {
 		return err
 	}
+	clusterRole.AggregationRule = aggregationRule
+	if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil {
+		return err
+	}
 
 	existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name)
 	if err != nil && !apierrors.IsNotFound(err) {
@@ -1031,6 +1018,86 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi
 	return nil
 }
 
+func hash(str string) (string, error) {
+	// hash the string with some randomness to help avoid guessing attacks
+	h := fnv.New32a()
+	rnd := uuid.NewString()
+	if _, err := h.Write([]byte(fmt.Sprintf("%s.%s", rnd, str))); err != nil {
+		return "", err
+	}
+	return strings.Trim(fmt.Sprintf("%016x", h.Sum32()), "0"), nil
+}
+
+func (a *Operator) getOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) (*rbacv1.ClusterRole, error) {
+	// oldRoleName := strings.Join([]string{op.GetName(), suffix}, "-")
+	roleNameHash, err := hash(fmt.Sprintf("%s/%s", op.GetNamespace(), op.GetName()))
+	if err != nil {
+		return nil, err
+	}
+	roleName := fmt.Sprintf("olm.operator-group.role.%s.%s", roleNameHash, suffix)
+
+	clusterRole := &rbacv1.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: roleName,
+		},
+	}
+
+	// check if old role name exists
+	roles, err := a.lister.RbacV1().ClusterRoleLister().List(ownerutil.ClusterRoleSelector(op))
+	if err != nil && !apierrors.IsNotFound(err) {
+		return nil, err
+	}
+	var suffixRoles []*rbacv1.ClusterRole
+	for idx, _ := range roles {
+		role := roles[idx]
+		if strings.HasSuffix(role.GetName(), suffix) {
+			suffixRoles = append(suffixRoles, role)
+		}
+	}
+	// todo: finding multiple owned cluster roles with the same suffix is highly unlikely to happen. However,
+	//       we need to test and document the manual clean up in case it ever happens
+	if len(suffixRoles) > 1 {
+		err := fmt.Errorf("found multiple cluster roles with suffix %s, please clean up manually", suffix)
+		a.logger.Error(err)
+		return nil, err
+	} else if len(suffixRoles) == 1 {
+		return suffixRoles[0], nil
+	}
+	return clusterRole, nil
+}
+
+func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) {
+	var selectors []metav1.LabelSelector
+	for api := range apis {
+		aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix)
+		if err != nil {
+			return nil, err
+		}
+		selectors = append(selectors, metav1.LabelSelector{
+			MatchLabels: map[string]string{
+				aggregationLabel: "true",
+			},
+		})
+	}
+	if len(selectors) > 0 {
+		return &rbacv1.AggregationRule{
+			ClusterRoleSelectors: selectors,
+		}, nil
+	}
+	return nil, nil
+}
+
+func (a *Operator) clusterRoleExistsAndIsOwnedBy(roleName string, owner ownerutil.Owner) (bool, error) {
+	role, err := a.lister.RbacV1().ClusterRoleLister().Get(roleName)
+	if err != nil && !apierrors.IsNotFound(err) {
+		return false, err
+	}
+	if apierrors.IsNotFound(err) {
+		return false, nil
+	}
+	return ownerutil.IsOwnedBy(role, owner), nil
+}
+
 func (a *Operator) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error {
 	for _, suffix := range Suffices {
 		if err := a.ensureOpGroupClusterRole(op, suffix, apis); err != nil {

Diff for: pkg/lib/ownerutil/util.go

@@ -272,6 +272,11 @@ func CSVOwnerSelector(owner *operatorsv1alpha1.ClusterServiceVersion) labels.Sel
 	return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1alpha1.ClusterServiceVersionKind))
 }
 
+// ClusterRoleSelector returns a label selector to find cluster role objects owned by owner
+func ClusterRoleSelector(owner *operatorsv1.OperatorGroup) labels.Selector {
+	return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1.OperatorGroupKind))
+}
+
 // AddOwner adds an owner to the ownerref list.
 func AddOwner(object metav1.Object, owner Owner, blockOwnerDeletion, isController bool) {
 	// Most of the time we won't have TypeMeta on the object, so we infer it for types we know about

Diff for: test/e2e/operator_groups_e2e_test.go

@@ -340,21 +340,22 @@ var _ = Describe("Operator Group", func() {
 		})
 
 		// validate provided API clusterroles for the operatorgroup
-		adminRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-admin", metav1.GetOptions{})
+		roleNamePrefix := fmt.Sprintf("olm.%s.operator-group.%s.role.", opGroupNamespace, operatorGroup.Name)
+		adminRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"admin", metav1.GetOptions{})
 		require.NoError(GinkgoT(), err)
 		adminPolicyRules := []rbacv1.PolicyRule{
 			{Verbs: []string{"*"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}},
 		}
 		require.Equal(GinkgoT(), adminPolicyRules, adminRole.Rules)
 
-		editRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-edit", metav1.GetOptions{})
+		editRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"edit", metav1.GetOptions{})
 		require.NoError(GinkgoT(), err)
 		editPolicyRules := []rbacv1.PolicyRule{
 			{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}},
 		}
 		require.Equal(GinkgoT(), editPolicyRules, editRole.Rules)
 
-		viewRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-view", metav1.GetOptions{})
+		viewRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"view", metav1.GetOptions{})
 		require.NoError(GinkgoT(), err)
 		viewPolicyRules := []rbacv1.PolicyRule{
 			{Verbs: []string{"get"}, APIGroups: []string{"apiextensions.k8s.io"}, Resources: []string{"customresourcedefinitions"}, ResourceNames: []string{mainCRD.Name}},