Skip to content

Commit 21b16d0

Browse files
Per Goncalves da Silvaperdasilva
Per Goncalves da Silva
authored and
perdasilva
committed
refactor operator group cluster role name
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
1 parent 2c3434a commit 21b16d0

File tree

4 files changed

+99
-26
lines changed

4 files changed

+99
-26
lines changed

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ require (
1414
github.com/go-logr/logr v1.2.4
1515
github.com/golang/mock v1.6.0
1616
github.com/google/go-cmp v0.5.9
17+
github.com/google/uuid v1.3.0
1718
github.com/googleapis/gnostic v0.5.5
1819
github.com/itchyny/gojq v0.11.0
1920
github.com/maxbrunsfeld/counterfeiter/v6 v6.2.2
@@ -124,7 +125,6 @@ require (
124125
github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect
125126
github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2 // indirect
126127
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
127-
github.com/google/uuid v1.3.0 // indirect
128128
github.com/gorilla/mux v1.8.0 // indirect
129129
github.com/gosuri/uitable v0.0.4 // indirect
130130
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 // indirect

‎pkg/controller/operators/olm/operatorgroup.go‎

Lines changed: 89 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package olm
33
import (
44
"context"
55
"fmt"
6+
"github.com/google/uuid"
67
"hash/fnv"
78
"reflect"
89
"strings"
@@ -979,33 +980,19 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string,
979980
}
980981

981982
func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error {
982-
clusterRole := &rbacv1.ClusterRole{
983-
ObjectMeta: metav1.ObjectMeta{
984-
Name: strings.Join([]string{op.GetName(), suffix}, "-"),
985-
},
986-
}
987-
var selectors []metav1.LabelSelector
988-
for api := range apis {
989-
aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix)
990-
if err != nil {
991-
return err
992-
}
993-
selectors = append(selectors, metav1.LabelSelector{
994-
MatchLabels: map[string]string{
995-
aggregationLabel: "true",
996-
},
997-
})
998-
}
999-
if len(selectors) > 0 {
1000-
clusterRole.AggregationRule = &rbacv1.AggregationRule{
1001-
ClusterRoleSelectors: selectors,
1002-
}
983+
clusterRole, err := a.getOpGroupClusterRole(op, suffix)
984+
if err != nil {
985+
return err
1003986
}
1004-
err := ownerutil.AddOwnerLabels(clusterRole, op)
987+
clusterRole.AggregationRule, err = a.getClusterRoleAggregationRule(apis, suffix)
1005988
if err != nil {
1006989
return err
1007990
}
1008991

992+
if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil {
993+
return err
994+
}
995+
1009996
existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name)
1010997
if err != nil && !apierrors.IsNotFound(err) {
1011998
return err
@@ -1032,6 +1019,86 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi
10321019
return nil
10331020
}
10341021

1022+
func hash(str string) (string, error) {
1023+
// hash the string with some randomness to help avoid guessing attacks
1024+
h := fnv.New32a()
1025+
rnd := uuid.NewString()
1026+
if _, err := h.Write([]byte(fmt.Sprintf("%s.%s", rnd, str))); err != nil {
1027+
return "", err
1028+
}
1029+
return strings.Trim(fmt.Sprintf("%016x", h.Sum32()), "0"), nil
1030+
}
1031+
1032+
func (a *Operator) getOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string) (*rbacv1.ClusterRole, error) {
1033+
roleNameHash, err := hash(fmt.Sprintf("%s/%s", op.GetNamespace(), op.GetName()))
1034+
if err != nil {
1035+
return nil, err
1036+
}
1037+
roleName := fmt.Sprintf("olm.operator-group.role.%s.%s", roleNameHash, suffix)
1038+
1039+
clusterRole := &rbacv1.ClusterRole{
1040+
ObjectMeta: metav1.ObjectMeta{
1041+
Name: roleName,
1042+
},
1043+
}
1044+
return clusterRole, nil
1045+
}
1046+
1047+
func (a *Operator) getOldClusterRole(op *operatorsv1.OperatorGroup, suffix string) (*rbacv1.ClusterRole, error) {
1048+
// check if old role name exists
1049+
roles, err := a.lister.RbacV1().ClusterRoleLister().List(ownerutil.ClusterRoleSelector(op))
1050+
if err != nil && !apierrors.IsNotFound(err) {
1051+
return nil, err
1052+
}
1053+
var suffixRoles []*rbacv1.ClusterRole
1054+
for idx, _ := range roles {
1055+
role := roles[idx]
1056+
if strings.HasSuffix(role.GetName(), suffix) {
1057+
suffixRoles = append(suffixRoles, role)
1058+
}
1059+
}
1060+
// todo: finding multiple owned cluster roles with the same suffix is highly unlikely to happen. However,
1061+
// we need to test and document the manual clean up in case it ever happens
1062+
if len(suffixRoles) > 1 {
1063+
err := fmt.Errorf("found multiple cluster roles with suffix %s, please clean up manually", suffix)
1064+
a.logger.Error(err)
1065+
return nil, err
1066+
}
1067+
return suffixRoles[0].DeepCopy(), nil
1068+
}
1069+
1070+
func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) {
1071+
var selectors []metav1.LabelSelector
1072+
for api := range apis {
1073+
aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix)
1074+
if err != nil {
1075+
return nil, err
1076+
}
1077+
selectors = append(selectors, metav1.LabelSelector{
1078+
MatchLabels: map[string]string{
1079+
aggregationLabel: "true",
1080+
},
1081+
})
1082+
}
1083+
if len(selectors) > 0 {
1084+
return &rbacv1.AggregationRule{
1085+
ClusterRoleSelectors: selectors,
1086+
}, nil
1087+
}
1088+
return nil, nil
1089+
}
1090+
1091+
func (a *Operator) clusterRoleExistsAndIsOwnedBy(roleName string, owner ownerutil.Owner) (bool, error) {
1092+
role, err := a.lister.RbacV1().ClusterRoleLister().Get(roleName)
1093+
if err != nil && !apierrors.IsNotFound(err) {
1094+
return false, err
1095+
}
1096+
if apierrors.IsNotFound(err) {
1097+
return false, nil
1098+
}
1099+
return ownerutil.IsOwnedBy(role, owner), nil
1100+
}
1101+
10351102
func (a *Operator) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error {
10361103
for _, suffix := range Suffices {
10371104
if err := a.ensureOpGroupClusterRole(op, suffix, apis); err != nil {

‎pkg/lib/ownerutil/util.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,11 @@ func CSVOwnerSelector(owner *operatorsv1alpha1.ClusterServiceVersion) labels.Sel
272272
return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1alpha1.ClusterServiceVersionKind))
273273
}
274274

275+
// ClusterRoleSelector returns a label selector to find cluster role objects owned by owner
276+
func ClusterRoleSelector(owner *operatorsv1.OperatorGroup) labels.Selector {
277+
return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1.OperatorGroupKind))
278+
}
279+
275280
// AddOwner adds an owner to the ownerref list.
276281
func AddOwner(object metav1.Object, owner Owner, blockOwnerDeletion, isController bool) {
277282
// 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‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -340,21 +340,22 @@ var _ = Describe("Operator Group", func() {
340340
})
341341

342342
// validate provided API clusterroles for the operatorgroup
343-
adminRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-admin", metav1.GetOptions{})
343+
roleNamePrefix := fmt.Sprintf("olm.%s.operator-group.%s.role.", opGroupNamespace, operatorGroup.Name)
344+
adminRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"admin", metav1.GetOptions{})
344345
require.NoError(GinkgoT(), err)
345346
adminPolicyRules := []rbacv1.PolicyRule{
346347
{Verbs: []string{"*"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}},
347348
}
348349
require.Equal(GinkgoT(), adminPolicyRules, adminRole.Rules)
349350

350-
editRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-edit", metav1.GetOptions{})
351+
editRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"edit", metav1.GetOptions{})
351352
require.NoError(GinkgoT(), err)
352353
editPolicyRules := []rbacv1.PolicyRule{
353354
{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}},
354355
}
355356
require.Equal(GinkgoT(), editPolicyRules, editRole.Rules)
356357

357-
viewRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-view", metav1.GetOptions{})
358+
viewRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), roleNamePrefix+"view", metav1.GetOptions{})
358359
require.NoError(GinkgoT(), err)
359360
viewPolicyRules := []rbacv1.PolicyRule{
360361
{Verbs: []string{"get"}, APIGroups: []string{"apiextensions.k8s.io"}, Resources: []string{"customresourcedefinitions"}, ResourceNames: []string{mainCRD.Name}},

0 commit comments

Comments
 (0)