From 0ac4a86a696f4e37add38a8687b8914c272716de Mon Sep 17 00:00:00 2001 From: Maciej Zimnoch Date: Tue, 5 Aug 2025 18:44:44 +0200 Subject: [PATCH] Add support for ClusterRole AggregationRule during bundle generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClusterRoles that use an AggregationRule often do not have any rules defined directly. Instead, their rules are aggregated from other ClusterRoles that match the AggregationRule’s label selector. The existing generator logic only included rules from ClusterRoles that were explicitly bound via ClusterRoleBindings to the ServiceAccounts used by Deployments. Since ClusterRoles with an AggregationRule typically lack direct rule definitions, the resulting permission bundle ended up being empty. This update adds support for handling ClusterRoles that use an AggregationRule. Signed-off-by: Maciej Zimnoch --- .../clusterserviceversion_updaters.go | 80 +++++++++++++++++-- .../clusterserviceversion_updaters_test.go | 62 ++++++++++++-- 2 files changed, 130 insertions(+), 12 deletions(-) diff --git a/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go b/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go index 98d7540705d..25f19f0a1cf 100644 --- a/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go +++ b/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go @@ -18,6 +18,7 @@ import ( "encoding/json" "errors" "fmt" + "slices" "sort" "strings" "time" @@ -31,6 +32,9 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/operator-framework/operator-sdk/internal/generate/collector" @@ -57,8 +61,14 @@ func apply(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion, switch strategy.StrategyName { case operatorsv1alpha1.InstallStrategyNameDeployment: inPerms, inCPerms, _ := c.SplitCSVPermissionsObjects(extraSAs) - applyRoles(c, inPerms, &strategy.StrategySpec, extraSAs) - applyClusterRoles(c, inCPerms, &strategy.StrategySpec, extraSAs) + err := applyRoles(c, inPerms, &strategy.StrategySpec, extraSAs) + if err != nil { + return fmt.Errorf("can't apply roles: %w", err) + } + err = applyClusterRoles(c, inCPerms, &strategy.StrategySpec, extraSAs) + if err != nil { + return fmt.Errorf("can't apply cluster roles: %w", err) + } applyDeployments(c, &strategy.StrategySpec) } csv.Spec.InstallStrategy = strategy @@ -88,7 +98,7 @@ const defaultServiceAccountName = "default" // applyRoles applies Roles to strategy's permissions field by combining Roles bound to ServiceAccounts // into one set of permissions. -func applyRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) { //nolint:dupl +func applyRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) error { //nolint:dupl roleSet := make(map[string]rbacv1.Role) cRoleSet := make(map[string]rbacv1.ClusterRole) for i := range objs { @@ -120,8 +130,16 @@ func applyRoles(c *collector.Manifests, objs []client.Object, strategy *operator hasRole = has case "ClusterRole": role, has := cRoleSet[binding.RoleRef.Name] - rules = role.Rules + hasRole = has + if has { + var err error + rules, err = getClusterRoleRules(role, c.ClusterRoles) + if err != nil { + return fmt.Errorf("can't get ClusterRole rules: %w", err) + } + } + default: continue } @@ -143,11 +161,13 @@ func applyRoles(c *collector.Manifests, objs []client.Object, strategy *operator return perms[i].ServiceAccountName < perms[j].ServiceAccountName }) strategy.Permissions = perms + + return nil } // applyClusterRoles applies ClusterRoles to strategy's clusterPermissions field by combining ClusterRoles // bound to ServiceAccounts into one set of clusterPermissions. -func applyClusterRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) { //nolint:dupl +func applyClusterRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) error { //nolint:dupl roleSet := make(map[string]rbacv1.ClusterRole) for i := range objs { switch t := objs[i].(type) { @@ -166,7 +186,12 @@ func applyClusterRoles(c *collector.Manifests, objs []client.Object, strategy *o continue } if role, hasRole := roleSet[binding.RoleRef.Name]; hasRole { - perm.Rules = append(perm.Rules, role.Rules...) + rules, err := getClusterRoleRules(role, c.ClusterRoles) + if err != nil { + return fmt.Errorf("can't get ClusterRole rules: %w", err) + } + + perm.Rules = append(perm.Rules, rules...) saToPermissions[subject.Name] = perm } } @@ -183,6 +208,49 @@ func applyClusterRoles(c *collector.Manifests, objs []client.Object, strategy *o return perms[i].ServiceAccountName < perms[j].ServiceAccountName }) strategy.ClusterPermissions = perms + + return nil +} + +// getClusterRoleRules returns all PolicyRules for a given ClusterRole, including rules from aggregated ClusterRoles +// as specified by the AggregationRule. It recursively collects rules from other ClusterRoles that match the label selectors +// in the AggregationRule, ensuring no duplicate rules are added. +func getClusterRoleRules(clusterRole rbacv1.ClusterRole, clusterRoles []rbacv1.ClusterRole) ([]rbacv1.PolicyRule, error) { + rules := make([]rbacv1.PolicyRule, 0, len(clusterRole.Rules)) + rules = append(rules, clusterRole.Rules...) + + if clusterRole.AggregationRule == nil { + return rules, nil + } + + for _, crSelector := range clusterRole.AggregationRule.ClusterRoleSelectors { + labelSelector, err := metav1.LabelSelectorAsSelector(&crSelector) + if err != nil { + return nil, fmt.Errorf("can't create label selector from ClusterRole %q AggregationRule: %w", clusterRole.Name, err) + } + + for _, cr := range clusterRoles { + if cr.Name == clusterRole.Name { + continue + } + + if !labelSelector.Matches(labels.Set(cr.Labels)) { + continue + } + + for _, rule := range cr.Rules { + ruleExists := slices.ContainsFunc(rules, func(r rbacv1.PolicyRule) bool { + return equality.Semantic.DeepEqual(rule, r) + }) + + if !ruleExists { + rules = append(rules, rule) + } + } + } + } + + return rules, nil } // initPermissionSet initializes a map of ServiceAccount name to permissions, which are empty. diff --git a/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go b/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go index 212e32a6e0d..1e4f507b9e4 100644 --- a/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go +++ b/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go @@ -24,6 +24,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" @@ -65,6 +66,8 @@ var _ = Describe("apply functions", func() { saName1 = "service-account-1" roleName1 = "role-1" cRoleName1 = "cluster-role-1" + cRoleName2 = "cluster-role-2" + cRoleName3 = "cluster-role-3" ) BeforeEach(func() { @@ -79,7 +82,8 @@ var _ = Describe("apply functions", func() { rules := []rbacv1.PolicyRule{{Verbs: []string{"create"}}} perms := []client.Object{newRole(roleName1, rules...)} c.RoleBindings = []rbacv1.RoleBinding{newRoleBinding("role-binding", newRoleRef(roleName1), newServiceAccountSubject(saName1))} - applyRoles(c, perms, strategy, nil) + err := applyRoles(c, perms, strategy, nil) + Expect(err).NotTo(HaveOccurred()) Expect(strategy.Permissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{ {ServiceAccountName: saName1, Rules: rules}, })) @@ -90,7 +94,49 @@ var _ = Describe("apply functions", func() { rules := []rbacv1.PolicyRule{{Verbs: []string{"create"}}} perms := []client.Object{newClusterRole(cRoleName1, rules...)} c.ClusterRoleBindings = []rbacv1.ClusterRoleBinding{newClusterRoleBinding("cluster-role-binding", newClusterRoleRef(cRoleName1), newServiceAccountSubject(saName1))} - applyClusterRoles(c, perms, strategy, nil) + err := applyClusterRoles(c, perms, strategy, nil) + Expect(err).NotTo(HaveOccurred()) + Expect(strategy.ClusterPermissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{ + {ServiceAccountName: saName1, Rules: rules}, + })) + }) + It("adds rules from aggregated ClusterRoles eliminating duplicates to the CSV deployment strategy", func() { + c.Deployments = []appsv1.Deployment{newDeploymentWithServiceAccount(depName1, saName1)} + c.ServiceAccounts = []corev1.ServiceAccount{newServiceAccount(saName1)} + rules := []rbacv1.PolicyRule{{Verbs: []string{"create"}}} + var emptyRules []rbacv1.PolicyRule + perms := []client.Object{ + func() *rbacv1.ClusterRole { + cr := newClusterRole(cRoleName1, emptyRules...) + cr.AggregationRule = &rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + { + MatchLabels: map[string]string{ + "aggregate-to-cluster-role-1": "true", + }, + }, + }, + } + return cr + }(), + func() *rbacv1.ClusterRole { + cr := newClusterRole(cRoleName2, rules...) + cr.Labels = map[string]string{ + "aggregate-to-cluster-role-1": "true", + } + return cr + }(), + func() *rbacv1.ClusterRole { + cr := newClusterRole(cRoleName3, rules...) + cr.Labels = map[string]string{ + "aggregate-to-cluster-role-1": "true", + } + return cr + }(), + } + c.ClusterRoleBindings = []rbacv1.ClusterRoleBinding{newClusterRoleBinding("cluster-role-binding", newClusterRoleRef(cRoleName1), newServiceAccountSubject(saName1))} + err := applyClusterRoles(c, perms, strategy, nil) + Expect(err).NotTo(HaveOccurred()) Expect(strategy.ClusterPermissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{ {ServiceAccountName: saName1, Rules: rules}, })) @@ -128,8 +174,10 @@ var _ = Describe("apply functions", func() { newClusterRoleBinding("cluster-role-binding-2", newClusterRoleRef(cRoleName2), newServiceAccountSubject(extraSAName)), newClusterRoleBinding("cluster-role-binding-3", newClusterRoleRef(cRoleName3), newServiceAccountSubject(extraSAName)), } - applyRoles(c, perms, strategy, []string{extraSAName}) - applyClusterRoles(c, cperms, strategy, []string{extraSAName}) + err := applyRoles(c, perms, strategy, []string{extraSAName}) + Expect(err).NotTo(HaveOccurred()) + err = applyClusterRoles(c, cperms, strategy, []string{extraSAName}) + Expect(err).NotTo(HaveOccurred()) Expect(strategy.Permissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{ {ServiceAccountName: saName1, Rules: rules}, {ServiceAccountName: extraSAName, Rules: rules}, @@ -146,14 +194,16 @@ var _ = Describe("apply functions", func() { c.Deployments = []appsv1.Deployment{newDeploymentWithServiceAccount(depName1, saName1)} c.ServiceAccounts = []corev1.ServiceAccount{newServiceAccount(saName1)} c.RoleBindings = []rbacv1.RoleBinding{newRoleBinding("role-binding", newRoleRef(roleName1), newServiceAccountSubject(saName1))} - applyRoles(c, nil, strategy, nil) + err := applyRoles(c, nil, strategy, nil) + Expect(err).NotTo(HaveOccurred()) Expect(strategy.Permissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{})) }) It("adds no ClusterPermissions to the CSV deployment strategy", func() { c.Deployments = []appsv1.Deployment{newDeploymentWithServiceAccount(depName1, saName1)} c.ServiceAccounts = []corev1.ServiceAccount{newServiceAccount(saName1)} c.ClusterRoleBindings = []rbacv1.ClusterRoleBinding{newClusterRoleBinding("cluster-role-binding", newClusterRoleRef(cRoleName1), newServiceAccountSubject(saName1))} - applyClusterRoles(c, nil, strategy, nil) + err := applyClusterRoles(c, nil, strategy, nil) + Expect(err).NotTo(HaveOccurred()) Expect(strategy.ClusterPermissions).To(Equal([]operatorsv1alpha1.StrategyDeploymentPermissions{})) }) })