refactor operator group cluster role name

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

Author: Per Goncalves da Silva

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

pkg/controller/operators/olm/operatorgroup.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"hash/fnv"
+	"k8s.io/apiserver/pkg/storage/names"
 	"reflect"
 	"strings"
 
@@ -978,38 +979,34 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string,
 }
 
 func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error {
+	// create target cluster role spec
 	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",
+			Name: names.SimpleNameGenerator.GenerateName(fmt.Sprintf("olm.operatorgroup.%s-", suffix)),
+			Labels: map[string]string{
+				ownerutil.OlmOperatorGroupClusterRoleLevel: suffix,
 			},
-		})
-	}
-	if len(selectors) > 0 {
-		clusterRole.AggregationRule = &rbacv1.AggregationRule{
-			ClusterRoleSelectors: selectors,
-		}
+		},
 	}
-	err := ownerutil.AddOwnerLabels(clusterRole, op)
+
+	var err error
+	clusterRole.AggregationRule, err = a.getClusterRoleAggregationRule(apis, suffix)
 	if err != nil {
 		return err
 	}
 
-	existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name)
-	if err != nil && !apierrors.IsNotFound(err) {
+	if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil {
 		return err
 	}
-	if apierrors.IsNotFound(err) {
+
+	// get existing cluster role for this level (suffix: admin, edit, view))
+	existingClusterRoleList, err := a.lister.RbacV1().ClusterRoleLister().List(labels.SelectorFromSet(ownerutil.ClusterRoleByOwnerAndLevel(op, suffix)))
+	if err != nil {
+		return err
+	}
+
+	var existingRole *rbacv1.ClusterRole
+	if len(existingClusterRoleList) == 0 {
 		existingRole, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{})
 		if err == nil {
 			return nil
@@ -1018,6 +1015,10 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi
 			a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
 			return err
 		}
+	} else if len(existingClusterRoleList) == 1 {
+		existingRole = existingClusterRoleList[0].DeepCopy()
+	} else {
+		return fmt.Errorf("found multiple cluster roles at level '%s' for operator group: '%s'", suffix, op.Name)
 	}
 
 	if existingRole != nil && labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, clusterRole.AggregationRule) {
@@ -1031,6 +1032,38 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi
 	return 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 {

pkg/lib/ownerutil/util.go

@@ -20,10 +20,14 @@ import (
 )
 
 const (
-	OwnerKey           = "olm.owner"
-	OwnerNamespaceKey  = "olm.owner.namespace"
-	OwnerKind          = "olm.owner.kind"
-	OwnerPackageServer = "packageserver"
+	OwnerKey                         = "olm.owner"
+	OwnerNamespaceKey                = "olm.owner.namespace"
+	OwnerKind                        = "olm.owner.kind"
+	OlmOperatorGroupClusterRoleLevel = "olm.operatorgroup.rolelevel"
+	ClusterRoleLevelAdmin            = "admin"
+	ClusterRoleLevelEdit             = "edit"
+	ClusterRoleLevelView             = "view"
+	OwnerPackageServer               = "packageserver"
 )
 
 var (
@@ -208,12 +212,18 @@ func NonBlockingOwner(owner Owner) metav1.OwnerReference {
 // OwnerLabel returns a label added to generated objects for later querying
 func OwnerLabel(owner Owner, kind string) map[string]string {
 	return map[string]string{
-		OwnerKey:          owner.GetName(),
+		OwnerKey:          string(owner.GetUID()),
 		OwnerNamespaceKey: owner.GetNamespace(),
 		OwnerKind:         kind,
 	}
 }
 
+func ClusterRoleByOwnerAndLevel(owner Owner, level string) map[string]string {
+	labels := OwnerLabel(owner, "OperatorGroup")
+	labels[OlmOperatorGroupClusterRoleLevel] = level
+	return labels
+}
+
 // AddOwnerLabels adds ownerref-like labels to an object by inferring the owner kind
 func AddOwnerLabels(object metav1.Object, owner Owner) error {
 	err := InferGroupVersionKind(owner)
@@ -272,6 +282,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

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}},