From cb999bcb7c48c26cd39ed840fdbaf59ed62ee751 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Thu, 13 Jul 2023 16:31:52 +0200 Subject: [PATCH 1/6] refactor operator group cluster role name Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operator_test.go | 363 ++++++++++++++++-- pkg/controller/operators/olm/operatorgroup.go | 118 ++++-- test/e2e/operator_groups_e2e_test.go | 40 +- 3 files changed, 438 insertions(+), 83 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index d4b0c0c3a4..a82f7b2da0 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -8,6 +8,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "errors" "fmt" "math" "math/big" @@ -51,6 +52,9 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" + opregistry "github.com/operator-framework/operator-registry/pkg/registry" + clienttesting "k8s.io/client-go/testing" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs" @@ -65,8 +69,6 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/scoped" - opregistry "github.com/operator-framework/operator-registry/pkg/registry" - clienttesting "k8s.io/client-go/testing" ) type TestStrategy struct{} @@ -933,8 +935,7 @@ func TestTransitionCSV(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "default", - Namespace: namespace, - }, + Namespace: namespace}, Spec: operatorsv1.OperatorGroupSpec{}, Status: operatorsv1.OperatorGroupStatus{ Namespaces: []string{namespace}, @@ -2224,8 +2225,7 @@ func TestTransitionCSV(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "default-2", - Namespace: namespace, - }, + Namespace: namespace}, Spec: operatorsv1.OperatorGroupSpec{}, Status: operatorsv1.OperatorGroupStatus{ Namespaces: []string{namespace}, @@ -2272,8 +2272,7 @@ func TestTransitionCSV(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "default", - Namespace: namespace, - }, + Namespace: namespace}, Spec: operatorsv1.OperatorGroupSpec{}, Status: operatorsv1.OperatorGroupStatus{ Namespaces: []string{namespace, "new-namespace"}, @@ -3731,8 +3730,7 @@ func TestUpdates(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "default", - Namespace: namespace, - }, + Namespace: namespace}, Spec: operatorsv1.OperatorGroupSpec{ TargetNamespaces: []string{namespace}, }, @@ -4422,8 +4420,7 @@ func TestSyncOperatorGroups(t *testing.T) { operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, - }, + Namespace: operatorNamespace}, Spec: operatorsv1.OperatorGroupSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "app-a"}, @@ -4452,8 +4449,7 @@ func TestSyncOperatorGroups(t *testing.T) { operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, - }, + Namespace: operatorNamespace}, Spec: operatorsv1.OperatorGroupSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "app-a"}, @@ -4494,7 +4490,40 @@ func TestSyncOperatorGroups(t *testing.T) { operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, + Namespace: operatorNamespace}, + Spec: operatorsv1.OperatorGroupSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "app-a"}, + }, + }, + }, + k8sObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorNamespace, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: targetNamespace, + Labels: map[string]string{"app": "app-a"}, + }, + }, + }, + }, + expectedStatus: operatorsv1.OperatorGroupStatus{ + Namespaces: []string{targetNamespace}, + LastUpdated: &now, + }, + }, + { + name: "MatchingNamespace/NoCSVs/CreatesClusterRoles", + expectedEqual: true, + initial: initial{ + operatorGroup: &operatorsv1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operator-group-1", + Namespace: operatorNamespace, Labels: map[string]string{"app": "app-a"}, }, Spec: operatorsv1.OperatorGroupSpec{ Selector: &metav1.LabelSelector{ @@ -4520,16 +4549,293 @@ func TestSyncOperatorGroups(t *testing.T) { Namespaces: []string{targetNamespace}, LastUpdated: &now, }, + final: final{objects: map[string][]runtime.Object{ + "": { + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.admin-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.edit-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.view-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + }, + }}, }, { - name: "MatchingNamespace/CSVPresent/Found", + // check that even if cluster roles exist without the naming convention, we create the new ones and leave the old ones unchanged + name: "MatchingNamespace/NoCSVs/UpdatesOldClusterRoles", expectedEqual: true, initial: initial{ operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, + Namespace: operatorNamespace}, + Spec: operatorsv1.OperatorGroupSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "app-a"}, + }, }, + }, + k8sObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorNamespace, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: targetNamespace, + Labels: map[string]string{"app": "app-a"}, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-admin", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-view", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-edit", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + }, + }, + expectedStatus: operatorsv1.OperatorGroupStatus{ + Namespaces: []string{targetNamespace}, + LastUpdated: &now, + }, + final: final{objects: map[string][]runtime.Object{ + "": { + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.admin-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.edit-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.view-d2ededg", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-admin", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-view", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "operator-group-1-edit", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + }, + }}, + }, + { + // if a cluster role exists with the correct name, use that + name: "MatchingNamespace/NoCSVs/DoesNotUpdateClusterRoles", + expectedEqual: true, + initial: initial{ + operatorGroup: &operatorsv1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operator-group-1", + Namespace: operatorNamespace}, + Spec: operatorsv1.OperatorGroupSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "app-a"}, + }, + }, + }, + k8sObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorNamespace, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: targetNamespace, + Labels: map[string]string{"app": "app-a"}, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.admin-xxxxx", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.edit-yyyyy", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.view-zzzzz", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }}, + }, + expectedStatus: operatorsv1.OperatorGroupStatus{ + Namespaces: []string{targetNamespace}, + LastUpdated: &now, + }, + final: final{objects: map[string][]runtime.Object{ + "": { + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.admin-xxxxx", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.edit-yyyyy", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.operatorgroup.view-zzzzz", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + }, + }, + }, + }, + { + name: "MatchingNamespace/CSVPresent/Found", + expectedEqual: true, + initial: initial{ + operatorGroup: &operatorsv1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operator-group-1", + Namespace: operatorNamespace}, Spec: operatorsv1.OperatorGroupSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{"app": "app-a"}, @@ -4635,8 +4941,7 @@ func TestSyncOperatorGroups(t *testing.T) { operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, - }, + Namespace: operatorNamespace}, Spec: operatorsv1.OperatorGroupSpec{ TargetNamespaces: []string{operatorNamespace, targetNamespace}, }, @@ -4739,8 +5044,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", Namespace: operatorNamespace, - Labels: map[string]string{"app": "app-a"}, - }, + Labels: map[string]string{"app": "app-a"}}, Spec: operatorsv1.OperatorGroupSpec{}, }, clientObjs: []runtime.Object{}, @@ -4842,8 +5146,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, - Labels: map[string]string{"app": "app-a"}, + Namespace: operatorNamespace, Labels: map[string]string{"app": "app-a"}, Annotations: map[string]string{ operatorsv1.OperatorGroupProvidedAPIsAnnotationKey: "missing.fake.api.group", }, @@ -4879,8 +5182,7 @@ func TestSyncOperatorGroups(t *testing.T) { Labels: map[string]string{"app": "app-a"}, Annotations: map[string]string{ operatorsv1.OperatorGroupProvidedAPIsAnnotationKey: "missing.fake.api.group", - }, - }, + }}, Spec: operatorsv1.OperatorGroupSpec{ StaticProvidedAPIs: true, }, @@ -4899,8 +5201,7 @@ func TestSyncOperatorGroups(t *testing.T) { operatorGroup: &operatorsv1.OperatorGroup{ ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1", - Namespace: operatorNamespace, - }, + Namespace: operatorNamespace}, Spec: operatorsv1.OperatorGroupSpec{}, }, clientObjs: []runtime.Object{}, @@ -5366,7 +5667,10 @@ func RequireObjectsInCache(t *testing.T, lister operatorlister.OperatorLister, n require.Failf(t, "couldn't find expected object", "%#v", object) } if err != nil { - return fmt.Errorf("namespace: %v, error: %v", namespace, err) + if apierrors.IsNotFound(err) { + return err + } + return errors.Join(err, fmt.Errorf("namespace: %v, error: %v", namespace, err)) } if doCompare { if !reflect.DeepEqual(object, fetched) { @@ -5440,8 +5744,7 @@ func TestCARotation(t *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "default", - Namespace: namespace, - }, + Namespace: namespace}, Spec: operatorsv1.OperatorGroupSpec{}, Status: operatorsv1.OperatorGroupStatus{ Namespaces: []string{namespace}, diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 90d2cb8994..6958c2ceac 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -4,10 +4,10 @@ import ( "context" "fmt" "hash/fnv" + "math/rand" "reflect" "strings" - utillabels "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/labels" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -18,14 +18,17 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/errors" + utillabels "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/labels" + operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" + opregistry "github.com/operator-framework/operator-registry/pkg/registry" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" - opregistry "github.com/operator-framework/operator-registry/pkg/registry" ) const ( @@ -66,6 +69,8 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error { "namespace": op.GetNamespace(), }) + logger.Infof("syncing OperatorGroup %s/%s", op.GetNamespace(), op.GetName()) + // Query OG in this namespace groups, err := a.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(op.GetNamespace()).List(labels.Everything()) if err != nil { @@ -981,59 +986,102 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string, return namespaceList, nil } -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}, "-"), - Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, - }, +func (a *Operator) stableRand(op *operatorsv1.OperatorGroup) (string, error) { + key := fmt.Sprintf("%s/%s", op.GetNamespace(), op.GetName()) + h := fnv.New64a() + _, err := h.Write([]byte(key)) + if err != nil { + return "", err } - 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", - }, - }) + rnd := rand.NewSource(int64(h.Sum64())) + const alphabet = "abcdefghijklmnopqrstuvwxyz0123456789" + + b := make([]byte, 7) + for i := range b { + b[i] = alphabet[rnd.Int63()%int64(len(alphabet))] } - if len(selectors) > 0 { - clusterRole.AggregationRule = &rbacv1.AggregationRule{ - ClusterRoleSelectors: selectors, - } + return string(b), nil +} + +func (a *Operator) getClusterRoleName(op *operatorsv1.OperatorGroup, roleType string) (string, error) { + roleSuffix, err := a.stableRand(op) + if err != nil { + return "", err + } + return fmt.Sprintf("olm.operatorgroup.%s-%s", roleType, roleSuffix), nil +} + +func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error { + // create target cluster role spec + var clusterRole *rbacv1.ClusterRole + clusterRoleName, err := a.getClusterRoleName(op, suffix) + if err != nil { + return err } - err := ownerutil.AddOwnerLabels(clusterRole, op) + aggregationRule, err := a.getClusterRoleAggregationRule(apis, suffix) if err != nil { return err } - existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name) + clusterRole = &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterRoleName, + }, + AggregationRule: aggregationRule, + } + + if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil { + return err + } + + // get existing cluster role for this level (suffix: admin, edit, view)) + existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRoleName) if err != nil && !apierrors.IsNotFound(err) { return err } - if apierrors.IsNotFound(err) { - existingRole, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) - if err == nil { + if existingRole != nil && existingRole.Name == clusterRoleName { + // if the existing role conforms to the naming convention, check for skew + if labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { return nil } - if !apierrors.IsAlreadyExists(err) { - a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) + // if skew was found, correct it + clusterRole.Name = existingRole.Name + if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { + a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) return err } + return nil } - if existingRole != nil && labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, clusterRole.AggregationRule) { + a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) + _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) + if err == nil { return nil } + // name collision, try again with a new name in the next reconcile + a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) + return err +} - if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { - a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) - return err +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", + }, + }) } - return nil + if len(selectors) > 0 { + return &rbacv1.AggregationRule{ + ClusterRoleSelectors: selectors, + }, nil + } + return nil, nil } func (a *Operator) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error { diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index feebdc0675..85b9cd85d3 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/test/e2e/operator_groups_e2e_test.go @@ -340,27 +340,31 @@ 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{}) - 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{}) + existingClusterRoleList, err := c.KubernetesInterface().RbacV1().ClusterRoles().List(context.TODO(), metav1.ListOptions{ + LabelSelector: labels.SelectorFromSet(ownerutil.OwnerLabel(&operatorGroup, "OperatorGroup")).String(), + }) 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) + require.Len(GinkgoT(), existingClusterRoleList.Items, 3) - viewRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(context.TODO(), operatorGroup.Name+"-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}}, - {Verbs: []string{"get", "list", "watch"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}}, + for _, role := range existingClusterRoleList.Items { + if strings.HasSuffix(role.Name, "admin") { + adminPolicyRules := []rbacv1.PolicyRule{ + {Verbs: []string{"*"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}}, + } + require.Equal(GinkgoT(), adminPolicyRules, role.Rules) + } else if strings.HasSuffix(role.Name, "edit") { + editPolicyRules := []rbacv1.PolicyRule{ + {Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}}, + } + require.Equal(GinkgoT(), editPolicyRules, role.Rules) + } else if strings.HasSuffix(role.Name, "view") { + viewPolicyRules := []rbacv1.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{"apiextensions.k8s.io"}, Resources: []string{"customresourcedefinitions"}, ResourceNames: []string{mainCRD.Name}}, + {Verbs: []string{"get", "list", "watch"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}}, + } + require.Equal(GinkgoT(), viewPolicyRules, role.Rules) + } } - require.Equal(GinkgoT(), viewPolicyRules, viewRole.Rules) // Unsupport all InstallModes log("unsupporting all csv installmodes") From e13d65cd16418cde65f115b98a117776277eaf1b Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Wed, 13 Sep 2023 13:58:44 +0200 Subject: [PATCH 2/6] nit fixes Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operatorgroup.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 6958c2ceac..4a239343a3 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -1040,26 +1040,22 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi return err } if existingRole != nil && existingRole.Name == clusterRoleName { - // if the existing role conforms to the naming convention, check for skew + // if the cluster role already exists, check if it needs to be updated if labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { return nil } // if skew was found, correct it - clusterRole.Name = existingRole.Name if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) - return err } - return nil + return err } a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) - _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) - if err == nil { - return nil + if _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil { + // name collision, try again with a new name in the next reconcile + a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) } - // name collision, try again with a new name in the next reconcile - a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) return err } From 7e71999060ed33a087d15b27a16faa3aa5f6504b Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Thu, 14 Sep 2023 13:21:54 +0200 Subject: [PATCH 3/6] update naming format Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operator_test.go | 38 +++-- pkg/controller/operators/olm/operatorgroup.go | 145 ++++++++++++------ pkg/lib/ownerutil/util.go | 6 +- 3 files changed, 127 insertions(+), 62 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index a82f7b2da0..9c9d57fe33 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -298,6 +298,18 @@ func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Opera k8sClientFake.Resources = apiResourcesForObjects(append(config.extObjs, config.regObjs...)) k8sClientFake.PrependReactor("*", "*", clienttesting.ReactionFunc(func(action clienttesting.Action) (bool, runtime.Object, error) { *config.actionLog = append(*config.actionLog, action) + switch action.GetVerb() { + case "create": + a := action.(clienttesting.CreateAction) + m := a.GetObject().(metav1.Object) + + // create a name if generateName is set + if m.GetGenerateName() != "" { + m.SetName(m.GetGenerateName() + "xxxxx") + m.SetGenerateName("") + return false, a.GetObject(), nil + } + } return false, nil, nil })) apiextensionsFake := apiextensionsfake.NewSimpleClientset(config.extObjs...) @@ -4554,7 +4566,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.admin-d2ededg", + Name: "olm.og.operator-group-1.admin-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4565,7 +4577,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.edit-d2ededg", + Name: "olm.og.operator-group-1.edit-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4576,7 +4588,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.view-d2ededg", + Name: "olm.og.operator-group-1.view-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4589,7 +4601,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, { // check that even if cluster roles exist without the naming convention, we create the new ones and leave the old ones unchanged - name: "MatchingNamespace/NoCSVs/UpdatesOldClusterRoles", + name: "MatchingNamespace/NoCSVs/KeepOldClusterRoles", expectedEqual: true, initial: initial{ operatorGroup: &operatorsv1.OperatorGroup{ @@ -4658,7 +4670,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.admin-d2ededg", + Name: "olm.og.operator-group-1.admin-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4669,7 +4681,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.edit-d2ededg", + Name: "olm.og.operator-group-1.edit-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4680,7 +4692,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.view-d2ededg", + Name: "olm.og.operator-group-1.view-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4754,7 +4766,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4765,7 +4777,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.edit-yyyyy", + Name: "olm.og.operator-group-1.edit-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4776,7 +4788,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.view-zzzzz", + Name: "olm.og.operator-group-1.view-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4794,7 +4806,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4805,7 +4817,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.edit-yyyyy", + Name: "olm.og.operator-group-1.edit-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4816,7 +4828,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.operatorgroup.view-zzzzz", + Name: "olm.og.operator-group-1.view-xxxxx", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 4a239343a3..2b00cf80c6 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -4,8 +4,10 @@ import ( "context" "fmt" "hash/fnv" - "math/rand" + "math" "reflect" + "regexp" + "sort" "strings" "github.com/sirupsen/logrus" @@ -35,6 +37,12 @@ const ( AdminSuffix = "admin" EditSuffix = "edit" ViewSuffix = "view" + + // kubeResourceNameLimit is the maximum length of a Kubernetes resource name + kubeResourceNameLimit = 253 + + // resourceRandomPrefixLengh is the length of the random prefix added to the resource name + resourceRandomPrefixLength = 5 ) var ( @@ -986,46 +994,30 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string, return namespaceList, nil } -func (a *Operator) stableRand(op *operatorsv1.OperatorGroup) (string, error) { - key := fmt.Sprintf("%s/%s", op.GetNamespace(), op.GetName()) - h := fnv.New64a() - _, err := h.Write([]byte(key)) - if err != nil { - return "", err - } - rnd := rand.NewSource(int64(h.Sum64())) - const alphabet = "abcdefghijklmnopqrstuvwxyz0123456789" +func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, roleType string, apis cache.APISet) error { + // create target cluster role spec + var clusterRole *rbacv1.ClusterRole - b := make([]byte, 7) - for i := range b { - b[i] = alphabet[rnd.Int63()%int64(len(alphabet))] - } - return string(b), nil -} + // to respect kubernetes resource length limitations we need to truncate the operator group name + template := "olm.og.%s.%s-" // final name will look like, e.g. olm.og.my-og.admin-pll5s + numTemplateChars := len(strings.Replace(template, "%s", "", -1)) + roleTypeLength := len(roleType) -func (a *Operator) getClusterRoleName(op *operatorsv1.OperatorGroup, roleType string) (string, error) { - roleSuffix, err := a.stableRand(op) - if err != nil { - return "", err - } - return fmt.Sprintf("olm.operatorgroup.%s-%s", roleType, roleSuffix), nil -} + // the operator group component of the name must be limited by the resource name length limit + // minus the number of characters used up by the other components of the name + nameLimit := kubeResourceNameLimit - numTemplateChars - roleTypeLength - resourceRandomPrefixLength + operatorGroupName := op.GetName()[:int(math.Min(float64(nameLimit), float64(len(op.GetName()))))] -func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error { - // create target cluster role spec - var clusterRole *rbacv1.ClusterRole - clusterRoleName, err := a.getClusterRoleName(op, suffix) - if err != nil { - return err - } - aggregationRule, err := a.getClusterRoleAggregationRule(apis, suffix) + clusterRoleName := fmt.Sprintf(template, operatorGroupName, roleType) + aggregationRule, err := a.getClusterRoleAggregationRule(apis, roleType) if err != nil { return err } + // if we'll be creating a fresh cluster role, use generateName to avoid name collisions clusterRole = &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: clusterRoleName, + GenerateName: clusterRoleName, }, AggregationRule: aggregationRule, } @@ -1034,29 +1026,74 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi return err } - // get existing cluster role for this level (suffix: admin, edit, view)) - existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRoleName) - if err != nil && !apierrors.IsNotFound(err) { + // list current cluster roles owned by this operator group + clusterRoles, err := a.lister.RbacV1().ClusterRoleLister().List(ownerutil.OperatorGroupOwnerSelector(op)) + if err != nil { return err } - if existingRole != nil && existingRole.Name == clusterRoleName { - // if the cluster role already exists, check if it needs to be updated - if labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { - return nil + + // find the cluster role that matches the template and is of the correct type + var existingClusterRoles []*rbacv1.ClusterRole + re, ok := ogClusterRoleNameRegExMap[roleType] + if !ok { + return fmt.Errorf("no regex found for role type: %s", roleType) + } + for _, cr := range clusterRoles { + if re.FindStringSubmatch(cr.GetName()) != nil { + existingClusterRoles = append(existingClusterRoles, cr) } - // if skew was found, correct it - if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { - a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) + } + + switch len(existingClusterRoles) { + // if no cluster role exists, create one + case 0: + a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetGenerateName(), op.GetNamespace(), op.GetName()) + if _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil { + // name collision, try again with a new name in the next reconcile + a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) } return err - } + // if an existing cluster role resource is found, update it if necessary + case 1: + existingClusterRole := existingClusterRoles[0].DeepCopy() + // the cluster role will need to be updated if the aggregation rules have changed - otherwise, nothing to do + // the resource is guaranteed to have the correct ownership labels because we reached this part of the code + if !reflect.DeepEqual(existingClusterRole.AggregationRule, aggregationRule) { + if _, err := a.opClient.UpdateClusterRole(existingClusterRole); err != nil { + a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) + } + return err + } + // the inherent race condition created by listing to check if a resource exists and then creating it + // and the fact that we are using generateName means that it is possible for multiple cluster roles of the same + // role type to be created for the same operator group. In this case, we'll disambiguate by keeping the oldest + // cluster role (by creation timestamp - tie-breaking by name) and deleting the others + default: + a.logger.Warnf("multiple (%d) cluster roles of type %s owned by operator group %s/%s found", len(existingClusterRoles), roleType, op.GetNamespace(), op.GetName()) + // sort by creation timestamp and tie-break by name + sort.Slice(existingClusterRoles, func(i, j int) bool { + creationTimeI := existingClusterRoles[i].GetCreationTimestamp() + creationTimeJ := existingClusterRoles[j].GetCreationTimestamp() + if creationTimeI.Equal(&creationTimeJ) { + return strings.Compare(existingClusterRoles[i].GetName(), existingClusterRoles[j].GetName()) < 0 + } + return creationTimeI.Before(&creationTimeJ) + }) + + // delete all but the oldest cluster role + a.logger.Infof("keeping cluster role: %s owned by operator group: %s/%s and deleting %d others", existingClusterRoles[0].GetName(), op.GetNamespace(), op.GetName(), len(existingClusterRoles)-1) + for _, cr := range existingClusterRoles[1:] { + a.logger.Infof("deleting cluster role: %s owned by operator group: %s/%s - there may be only one", cr.GetName(), op.GetNamespace(), op.GetName()) + if err := a.opClient.DeleteClusterRole(cr.GetName(), &metav1.DeleteOptions{}); err != nil { + a.logger.WithError(err).Errorf("Delete cluster role failed: %v", cr) + return err + } + } - a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) - if _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil { - // name collision, try again with a new name in the next reconcile - a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) + // now that we're (hopefully) down to one cluster role, re-reconcile + return fmt.Errorf("multiple cluster roles of type %s owned by operator group %s/%s found", roleType, op.GetNamespace(), op.GetName()) } - return err + return nil } func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) { @@ -1174,3 +1211,15 @@ func csvCopyPrototype(src, dst *v1alpha1.ClusterServiceVersion) { dst.Status.Reason = v1alpha1.CSVReasonCopied dst.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", src.GetNamespace()) } + +// ogClusterRoleNameRegEx returns a regexp for the naming format used for cluster roles owned by operator groups +// for a particular role type e.g. olm.og.my-og.admin-pll2k (where pll2k is a random suffix and admin is the role type) +var ogClusterRoleNameRegExMap = map[string]*regexp.Regexp{ + "admin": ogClusterRoleNameRegEx("admin"), + "edit": ogClusterRoleNameRegEx("edit"), + "view": ogClusterRoleNameRegEx("view"), +} + +func ogClusterRoleNameRegEx(roleType string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`^olm\.og\.[^\.]+\.%s-[a-z0-9]{5}$`, roleType)) +} diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index dafaeaf8b0..a07c4f019e 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -2,7 +2,6 @@ package ownerutil import ( "fmt" - log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" rbac "k8s.io/api/rbac/v1" @@ -271,6 +270,11 @@ func CSVOwnerSelector(owner *operatorsv1alpha1.ClusterServiceVersion) labels.Sel return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1alpha1.ClusterServiceVersionKind)) } +// OperatorGroupOwnerSelector returns a label selector to find generated objects owned by owner +func OperatorGroupOwnerSelector(owner *operatorsv1.OperatorGroup) labels.Selector { + return labels.SelectorFromSet(OwnerLabel(owner, "OperatorGroup")) +} + // 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 From 0b9d7fb44222a30b37ebc9b617fa6ae54de9e415 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 15 Sep 2023 10:13:22 +0200 Subject: [PATCH 4/6] revert to predictable hashes Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operator_test.go | 36 ++--- pkg/controller/operators/olm/operatorgroup.go | 135 ++++++------------ 2 files changed, 58 insertions(+), 113 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 9c9d57fe33..e45f233b9b 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -298,18 +298,6 @@ func NewFakeOperator(ctx context.Context, options ...fakeOperatorOption) (*Opera k8sClientFake.Resources = apiResourcesForObjects(append(config.extObjs, config.regObjs...)) k8sClientFake.PrependReactor("*", "*", clienttesting.ReactionFunc(func(action clienttesting.Action) (bool, runtime.Object, error) { *config.actionLog = append(*config.actionLog, action) - switch action.GetVerb() { - case "create": - a := action.(clienttesting.CreateAction) - m := a.GetObject().(metav1.Object) - - // create a name if generateName is set - if m.GetGenerateName() != "" { - m.SetName(m.GetGenerateName() + "xxxxx") - m.SetGenerateName("") - return false, a.GetObject(), nil - } - } return false, nil, nil })) apiextensionsFake := apiextensionsfake.NewSimpleClientset(config.extObjs...) @@ -4566,7 +4554,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4577,7 +4565,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-xxxxx", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4588,7 +4576,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.view-xxxxx", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4670,7 +4658,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4681,7 +4669,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-xxxxx", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4692,7 +4680,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.view-xxxxx", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4766,7 +4754,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4777,7 +4765,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-xxxxx", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4788,7 +4776,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.view-xxxxx", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4806,7 +4794,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-xxxxx", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4817,7 +4805,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-xxxxx", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4828,7 +4816,7 @@ func TestSyncOperatorGroups(t *testing.T) { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ ResourceVersion: "", - Name: "olm.og.operator-group-1.view-xxxxx", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 2b00cf80c6..3007c2aace 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -2,12 +2,11 @@ package olm import ( "context" + "crypto/sha256" "fmt" "hash/fnv" - "math" + "math/big" "reflect" - "regexp" - "sort" "strings" "github.com/sirupsen/logrus" @@ -41,8 +40,9 @@ const ( // kubeResourceNameLimit is the maximum length of a Kubernetes resource name kubeResourceNameLimit = 253 - // resourceRandomPrefixLengh is the length of the random prefix added to the resource name - resourceRandomPrefixLength = 5 + // operatorGroupClusterRoleNameFmt template for ClusterRole names owned by OperatorGroups\ + // e.g. olm.og.my-group.admin- + operatorGroupClusterRoleNameFmt = "olm.og.%s.%s-%s" ) var ( @@ -994,30 +994,31 @@ func (a *Operator) updateNamespaceList(op *operatorsv1.OperatorGroup) ([]string, return namespaceList, nil } -func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, roleType string, apis cache.APISet) error { +func (a *Operator) getClusterRoleName(op *operatorsv1.OperatorGroup, roleType string) (string, error) { + roleSuffix := hash(fmt.Sprintf("%s/%s/%s", op.GetNamespace(), op.GetName(), roleType)) + // calculate how many characters are left for the operator group name + nameLimit := kubeResourceNameLimit - len(strings.Replace(operatorGroupClusterRoleNameFmt, "%s", "", -1)) - len(roleType) - len(roleSuffix) + if len(op.GetName()) < nameLimit { + return fmt.Sprintf(operatorGroupClusterRoleNameFmt, op.GetName(), roleType, roleSuffix), nil + } + return fmt.Sprintf(operatorGroupClusterRoleNameFmt, op.GetName()[:nameLimit], roleType, roleSuffix), nil +} + +func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffix string, apis cache.APISet) error { // create target cluster role spec var clusterRole *rbacv1.ClusterRole - - // to respect kubernetes resource length limitations we need to truncate the operator group name - template := "olm.og.%s.%s-" // final name will look like, e.g. olm.og.my-og.admin-pll5s - numTemplateChars := len(strings.Replace(template, "%s", "", -1)) - roleTypeLength := len(roleType) - - // the operator group component of the name must be limited by the resource name length limit - // minus the number of characters used up by the other components of the name - nameLimit := kubeResourceNameLimit - numTemplateChars - roleTypeLength - resourceRandomPrefixLength - operatorGroupName := op.GetName()[:int(math.Min(float64(nameLimit), float64(len(op.GetName()))))] - - clusterRoleName := fmt.Sprintf(template, operatorGroupName, roleType) - aggregationRule, err := a.getClusterRoleAggregationRule(apis, roleType) + clusterRoleName, err := a.getClusterRoleName(op, suffix) + if err != nil { + return err + } + aggregationRule, err := a.getClusterRoleAggregationRule(apis, suffix) if err != nil { return err } - // if we'll be creating a fresh cluster role, use generateName to avoid name collisions clusterRole = &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: clusterRoleName, + Name: clusterRoleName, }, AggregationRule: aggregationRule, } @@ -1026,74 +1027,32 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, roleT return err } - // list current cluster roles owned by this operator group - clusterRoles, err := a.lister.RbacV1().ClusterRoleLister().List(ownerutil.OperatorGroupOwnerSelector(op)) - if err != nil { + // get existing cluster role for this level (suffix: admin, edit, view)) + existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRoleName) + if err != nil && !apierrors.IsNotFound(err) { return err } - // find the cluster role that matches the template and is of the correct type - var existingClusterRoles []*rbacv1.ClusterRole - re, ok := ogClusterRoleNameRegExMap[roleType] - if !ok { - return fmt.Errorf("no regex found for role type: %s", roleType) - } - for _, cr := range clusterRoles { - if re.FindStringSubmatch(cr.GetName()) != nil { - existingClusterRoles = append(existingClusterRoles, cr) - } - } - - switch len(existingClusterRoles) { - // if no cluster role exists, create one - case 0: - a.logger.Infof("Creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetGenerateName(), op.GetNamespace(), op.GetName()) - if _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil { - // name collision, try again with a new name in the next reconcile - a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) - } - return err - // if an existing cluster role resource is found, update it if necessary - case 1: - existingClusterRole := existingClusterRoles[0].DeepCopy() - // the cluster role will need to be updated if the aggregation rules have changed - otherwise, nothing to do - // the resource is guaranteed to have the correct ownership labels because we reached this part of the code - if !reflect.DeepEqual(existingClusterRole.AggregationRule, aggregationRule) { - if _, err := a.opClient.UpdateClusterRole(existingClusterRole); err != nil { - a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) - } + if existingRole != nil { + // if the existing role conforms to the naming convention, check for skew + cp := existingRole.DeepCopy() + if err := ownerutil.AddOwnerLabels(cp, op); err != nil { return err } - // the inherent race condition created by listing to check if a resource exists and then creating it - // and the fact that we are using generateName means that it is possible for multiple cluster roles of the same - // role type to be created for the same operator group. In this case, we'll disambiguate by keeping the oldest - // cluster role (by creation timestamp - tie-breaking by name) and deleting the others - default: - a.logger.Warnf("multiple (%d) cluster roles of type %s owned by operator group %s/%s found", len(existingClusterRoles), roleType, op.GetNamespace(), op.GetName()) - // sort by creation timestamp and tie-break by name - sort.Slice(existingClusterRoles, func(i, j int) bool { - creationTimeI := existingClusterRoles[i].GetCreationTimestamp() - creationTimeJ := existingClusterRoles[j].GetCreationTimestamp() - if creationTimeI.Equal(&creationTimeJ) { - return strings.Compare(existingClusterRoles[i].GetName(), existingClusterRoles[j].GetName()) < 0 - } - return creationTimeI.Before(&creationTimeJ) - }) - // delete all but the oldest cluster role - a.logger.Infof("keeping cluster role: %s owned by operator group: %s/%s and deleting %d others", existingClusterRoles[0].GetName(), op.GetNamespace(), op.GetName(), len(existingClusterRoles)-1) - for _, cr := range existingClusterRoles[1:] { - a.logger.Infof("deleting cluster role: %s owned by operator group: %s/%s - there may be only one", cr.GetName(), op.GetNamespace(), op.GetName()) - if err := a.opClient.DeleteClusterRole(cr.GetName(), &metav1.DeleteOptions{}); err != nil { - a.logger.WithError(err).Errorf("Delete cluster role failed: %v", cr) - return err - } + // ensure that the labels and aggregation rules are correct + if labels.Equals(existingRole.Labels, cp.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { + return nil } - - // now that we're (hopefully) down to one cluster role, re-reconcile - return fmt.Errorf("multiple cluster roles of type %s owned by operator group %s/%s found", roleType, op.GetNamespace(), op.GetName()) + if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { + a.logger.WithError(err).Errorf("update existing cluster role failed: %v", clusterRole) + } + return err } - return nil + + a.logger.Infof("creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) + _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) + return err } func (a *Operator) getClusterRoleAggregationRule(apis cache.APISet, suffix string) (*rbacv1.AggregationRule, error) { @@ -1212,14 +1171,12 @@ func csvCopyPrototype(src, dst *v1alpha1.ClusterServiceVersion) { dst.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", src.GetNamespace()) } -// ogClusterRoleNameRegEx returns a regexp for the naming format used for cluster roles owned by operator groups -// for a particular role type e.g. olm.og.my-og.admin-pll2k (where pll2k is a random suffix and admin is the role type) -var ogClusterRoleNameRegExMap = map[string]*regexp.Regexp{ - "admin": ogClusterRoleNameRegEx("admin"), - "edit": ogClusterRoleNameRegEx("edit"), - "view": ogClusterRoleNameRegEx("view"), +func hash(s string) string { + return toBase62(sha256.Sum224([]byte(s))) } -func ogClusterRoleNameRegEx(roleType string) *regexp.Regexp { - return regexp.MustCompile(fmt.Sprintf(`^olm\.og\.[^\.]+\.%s-[a-z0-9]{5}$`, roleType)) +func toBase62(hash [28]byte) string { + var i big.Int + i.SetBytes(hash[:]) + return i.Text(62) } From dbacd7ab43e94c853ad08083053358edf995230b Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 15 Sep 2023 10:32:21 +0200 Subject: [PATCH 5/6] add regression test for custom labels no longer getting removed from the cluster role Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operator_test.go | 110 ++++++++++++++++++ pkg/controller/operators/olm/operatorgroup.go | 26 +++-- 2 files changed, 124 insertions(+), 12 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index e45f233b9b..d8003c95d4 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -4724,6 +4724,116 @@ func TestSyncOperatorGroups(t *testing.T) { }, }}, }, + { + // ensure that ownership labels are fixed but user labels are preserved + name: "MatchingNamespace/NoCSVs/ClusterRoleOwnershipLabels", + expectedEqual: true, + initial: initial{ + operatorGroup: &operatorsv1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operator-group-1", + Namespace: operatorNamespace}, + Spec: operatorsv1.OperatorGroupSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "app-a"}, + }, + }, + }, + k8sObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: operatorNamespace, + }, + }, + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: targetNamespace, + Labels: map[string]string{"app": "app-a"}, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns-bob", + "olm.owner.kind": "OperatorGroup", + "not.an.olm.label": "true", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Labels: map[string]string{ + "olm.owner": "operator-group-5", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + "not.an.olm.label": "false", + "another.olm.label": "or maybe not", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroupKind", + }, + }, + }, + }, + }, + expectedStatus: operatorsv1.OperatorGroupStatus{ + Namespaces: []string{targetNamespace}, + LastUpdated: &now, + }, + final: final{objects: map[string][]runtime.Object{ + "": { + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + "not.an.olm.label": "true", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + ResourceVersion: "", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Labels: map[string]string{ + "olm.owner": "operator-group-1", + "olm.owner.namespace": "operator-ns", + "olm.owner.kind": "OperatorGroup", + "not.an.olm.label": "false", + "another.olm.label": "or maybe not", + }, + }, + }, + }, + }}, + }, { // if a cluster role exists with the correct name, use that name: "MatchingNamespace/NoCSVs/DoesNotUpdateClusterRoles", diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 3007c2aace..34b5925761 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -1016,17 +1016,6 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi return err } - clusterRole = &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: clusterRoleName, - }, - AggregationRule: aggregationRule, - } - - if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil { - return err - } - // get existing cluster role for this level (suffix: admin, edit, view)) existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRoleName) if err != nil && !apierrors.IsNotFound(err) { @@ -1044,12 +1033,25 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi if labels.Equals(existingRole.Labels, cp.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { return nil } - if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { + + cp.AggregationRule = aggregationRule + if _, err := a.opClient.UpdateClusterRole(cp); err != nil { a.logger.WithError(err).Errorf("update existing cluster role failed: %v", clusterRole) } return err } + clusterRole = &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterRoleName, + }, + AggregationRule: aggregationRule, + } + + if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil { + return err + } + a.logger.Infof("creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) return err From e3b7132711f65d95c046886f3a568ee92302c7d5 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Mon, 18 Sep 2023 16:36:47 +0200 Subject: [PATCH 6/6] move to semantic DeepEquals Signed-off-by: Per Goncalves da Silva --- pkg/controller/operators/olm/operatorgroup.go | 4 +++- pkg/lib/ownerutil/util.go | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 34b5925761..92c70c649f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -9,6 +9,8 @@ import ( "reflect" "strings" + "k8s.io/apimachinery/pkg/api/equality" + "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -1030,7 +1032,7 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi } // ensure that the labels and aggregation rules are correct - if labels.Equals(existingRole.Labels, cp.Labels) && reflect.DeepEqual(existingRole.AggregationRule, aggregationRule) { + if labels.Equals(existingRole.Labels, cp.Labels) && equality.Semantic.DeepEqual(existingRole.AggregationRule, aggregationRule) { return nil } diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index a07c4f019e..6bbd7eb5f0 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -2,6 +2,7 @@ package ownerutil import ( "fmt" + log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" rbac "k8s.io/api/rbac/v1"