Add support for ClusterRole AggregationRule during bundle generation

zimnx · zimnx · commit 27659b859caf · 2025-08-29T11:17:16.000+02:00
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 &lt;maciej@scylladb.com&gt;
diff --git a/changelog/fragments/01-support-aggregated-rules-of-clusterroles.yaml b/changelog/fragments/01-support-aggregated-rules-of-clusterroles.yaml
@@ -0,0 +1,41 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      The bundle generator now includes policy rules from ClusterRoles originating through AggregationRule.
+      Previously, only policy rules of ClusterRoles explicitly bound to ServiceAccounts via ClusterRoleBindings were included.
+      With this update, the generator also aggregates and applies rules from matching ClusterRoles, 
+      ensuring proper RBAC coverage.
+    
+
+    # kind is one of:
+    # - addition
+    # - change
+    # - deprecation
+    # - removal
+    # - bugfix
+    kind: addition
+
+    # Is this a breaking change?
+    breaking: false
+
+    # NOTE: ONLY USE `pull_request_override` WHEN ADDING THIS
+    # FILE FOR A PREVIOUSLY MERGED PULL_REQUEST!
+    #
+    # The generator auto-detects the PR number from the commit
+    # message in which this file was originally added.
+    #
+    # What is the pull request number (without the "#")?
+    # pull_request_override: 0
+
+
+    # Migration can be defined to automatically add a section to
+    # the migration guide. This is required for breaking changes.
+#    migration:
+#      header: Header text for the migration section
+#      body: |
+#        Body of the migration section. This should be formatted as markdown and can
+#        span multiple lines.
+#
+#        Using the YAML string '|' operator means that newlines in this string will
+#        be honored and interpretted as newlines in the rendered markdown.
diff --git 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,48 @@ 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, 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
@@ -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,9 @@ var _ = Describe("apply functions", func() {
 			saName1    = "service-account-1"
 			roleName1  = "role-1"
 			cRoleName1 = "cluster-role-1"
+			cRoleName2 = "cluster-role-2"
+			cRoleName3 = "cluster-role-3"
+			cRoleName4 = "cluster-role-4"
 		)
 
 		BeforeEach(func() {
@@ -79,7 +83,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 +95,55 @@ 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"}}, {Verbs: []string{"update"}}}
+				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
+					}(),
+					// ClusterRole not bound to any ServiceAccount, nor matching any ClusterRule AggregationRule,
+					// it shouldn't land in strategy ClusterPermissions.
+					newClusterRole(cRoleName4, []rbacv1.PolicyRule{{Verbs: []string{"delete"}}}...),
+				}
+				for _, cr := range perms {
+					c.ClusterRoles = append(c.ClusterRoles, *cr.(*rbacv1.ClusterRole))
+				}
+				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 +181,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 +201,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{}))
 			})
 		})
