From 35f8bca17800ca15501217f1c989610482214fb8 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Thu, 13 Jul 2023 16:31:52 +0200 Subject: [PATCH 1/2] Rename ClusterRoles created by OperatorGroups When an OperatorGroup creates a ClusterRole, it's based directly on the OG name with a suffix, this causes two issues: 1. same-named OGs in different namespaces overwrite each others CRs 2. there are some very important CRs that could be overwritten by OG Tests added. Signed-off-by: Per Goncalves da Silva Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator_test.go | 436 +++++++++++++++++- pkg/controller/operators/olm/operatorgroup.go | 128 +++-- pkg/lib/ownerutil/util.go | 5 + test/e2e/operator_groups_e2e_test.go | 40 +- 4 files changed, 551 insertions(+), 58 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index d4b0c0c3a4..4d69f0cdfe 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{} @@ -4521,6 +4523,431 @@ func TestSyncOperatorGroups(t *testing.T) { 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{ + 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, + }, + 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", + }, + }, + }, + &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", + }, + }, + }, + }, + }}, + }, + { + // 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/KeepOldClusterRoles", + 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: "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.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + 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.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", + }, + }, + }, + &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", + }, + }, + }, + }, + }}, + }, + { + // 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", + 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", + "olm.owner.kind": "OperatorGroup", + }, + }, + }, + &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", + }, + }, + }}, + }, + 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", + }, + }, + }, + &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", + }, + }, + }, + }, + }, + }, + }, { name: "MatchingNamespace/CSVPresent/Found", expectedEqual: true, @@ -5366,7 +5793,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) { diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 90d2cb8994..92c70c649f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -2,12 +2,15 @@ package olm import ( "context" + "crypto/sha256" "fmt" "hash/fnv" + "math/big" "reflect" "strings" - utillabels "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/labels" + "k8s.io/apimachinery/pkg/api/equality" + "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -18,20 +21,30 @@ 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 ( AdminSuffix = "admin" EditSuffix = "edit" ViewSuffix = "view" + + // kubeResourceNameLimit is the maximum length of a Kubernetes resource name + kubeResourceNameLimit = 253 + + // operatorGroupClusterRoleNameFmt template for ClusterRole names owned by OperatorGroups\ + // e.g. olm.og.my-group.admin- + operatorGroupClusterRoleNameFmt = "olm.og.%s.%s-%s" ) var ( @@ -66,6 +79,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 +996,88 @@ 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) 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 } - 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, - } + 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 + 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) + // 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 { + // 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 + } + + // ensure that the labels and aggregation rules are correct + if labels.Equals(existingRole.Labels, cp.Labels) && equality.Semantic.DeepEqual(existingRole.AggregationRule, aggregationRule) { return nil } - if !apierrors.IsAlreadyExists(err) { - a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole) - return err + + 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 } - if existingRole != nil && labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, clusterRole.AggregationRule) { - return nil + clusterRole = &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterRoleName, + }, + AggregationRule: aggregationRule, } - if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil { - a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole) + if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil { 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) { + 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) ensureOpGroupClusterRoles(op *operatorsv1.OperatorGroup, apis cache.APISet) error { @@ -1130,3 +1174,13 @@ 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()) } + +func hash(s string) string { + return toBase62(sha256.Sum224([]byte(s))) +} + +func toBase62(hash [28]byte) string { + var i big.Int + i.SetBytes(hash[:]) + return i.Text(62) +} diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index dafaeaf8b0..6bbd7eb5f0 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -271,6 +271,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 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 94f840b45c1b83cd49246d7f8555516ae027952a Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 18 Sep 2023 15:16:09 -0400 Subject: [PATCH 2/2] fixup! Rename ClusterRoles created by OperatorGroups Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator_test.go | 72 +++++++------------ pkg/lib/ownerutil/util.go | 2 +- test/e2e/operator_groups_e2e_test.go | 18 ++++- 3 files changed, 40 insertions(+), 52 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 4d69f0cdfe..99dca3fd98 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -4560,8 +4560,7 @@ func TestSyncOperatorGroups(t *testing.T) { "": { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4571,8 +4570,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4582,8 +4580,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4624,8 +4621,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-admin", + Name: "operator-group-1-admin", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4635,8 +4631,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-view", + Name: "operator-group-1-view", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4646,8 +4641,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-edit", + Name: "operator-group-1-edit", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4665,8 +4659,7 @@ func TestSyncOperatorGroups(t *testing.T) { "": { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4676,8 +4669,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4687,8 +4679,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4698,8 +4689,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-admin", + Name: "operator-group-1-admin", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4709,8 +4699,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-view", + Name: "operator-group-1-view", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4720,8 +4709,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "operator-group-1-edit", + Name: "operator-group-1-edit", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4762,8 +4750,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns-bob", @@ -4774,8 +4761,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-5", "olm.owner.namespace": "operator-ns", @@ -4787,8 +4773,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4806,8 +4791,7 @@ func TestSyncOperatorGroups(t *testing.T) { "": { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4818,8 +4802,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4829,8 +4812,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4873,8 +4855,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4884,8 +4865,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4895,8 +4875,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4913,8 +4892,7 @@ func TestSyncOperatorGroups(t *testing.T) { "": { &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", + Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4924,8 +4902,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", + Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", @@ -4935,8 +4912,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - ResourceVersion: "", - Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", + 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/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index 6bbd7eb5f0..352b2a8025 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -273,7 +273,7 @@ func CSVOwnerSelector(owner *operatorsv1alpha1.ClusterServiceVersion) labels.Sel // 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")) + return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1.OperatorGroupKind)) } // AddOwner adds an owner to the ownerref list. diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index 85b9cd85d3..a60a8ffcb4 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/test/e2e/operator_groups_e2e_test.go @@ -7,8 +7,10 @@ import ( "time" "github.com/blang/semver/v4" + "github.com/google/go-cmp/cmp" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" @@ -351,18 +353,28 @@ var _ = Describe("Operator Group", func() { adminPolicyRules := []rbacv1.PolicyRule{ {Verbs: []string{"*"}, APIGroups: []string{mainCRD.Spec.Group}, Resources: []string{mainCRDPlural}}, } - require.Equal(GinkgoT(), adminPolicyRules, role.Rules) + if assert.Equal(GinkgoT(), adminPolicyRules, role.Rules) == false { + fmt.Println(cmp.Diff(adminPolicyRules, role.Rules)) + GinkgoT().Fail() + } + } 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) + if assert.Equal(GinkgoT(), editPolicyRules, role.Rules) == false { + fmt.Println(cmp.Diff(editPolicyRules, role.Rules)) + GinkgoT().Fail() + } } 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) + if assert.Equal(GinkgoT(), viewPolicyRules, role.Rules) == false { + fmt.Println(cmp.Diff(viewPolicyRules, role.Rules)) + GinkgoT().Fail() + } } }