From 7fd88cf8302af3084fb398bbaf398195e33defe6 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Thu, 13 Jul 2023 16:31:52 +0200 Subject: [PATCH] refactor operator group cluster role name Signed-off-by: Per Goncalves da Silva --- go.mod | 2 +- pkg/controller/operators/olm/operatorgroup.go | 111 ++++++++++++++---- pkg/lib/ownerutil/util.go | 7 ++ test/e2e/operator_groups_e2e_test.go | 7 +- 4 files changed, 101 insertions(+), 26 deletions(-) diff --git a/go.mod b/go.mod index 4537e757dd0..5fb03887da2 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/go-logr/logr v1.2.3 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 @@ -122,7 +123,6 @@ require ( github.com/google/gofuzz v1.2.0 // indirect github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // 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 --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index cd135c8058f..5b5b4fcaeda 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/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 --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index 24ff1afe2e0..0bde9199444 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -17,6 +17,8 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" operatorsv2 "github.com/operator-framework/api/pkg/operators/v2" + + rbacv1 "k8s.io/api/rbac/v1" ) const ( @@ -272,6 +274,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 Owner) labels.Selector { + return labels.SelectorFromSet(OwnerLabel(owner, rbacv1.GroupKind)) +} + // 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 --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index 743f8ff37df..b24966250f5 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/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}},