CAP-3059 consolidate RBAC creation logic for custom resources (#2262)

Author: kangyili

internal/controller/datadogagent/feature/kubernetesstatecore/rbac.go

@@ -6,13 +6,12 @@
 package kubernetesstatecore
 
 import (
-	"maps"
-	"slices"
 	"strings"
 
 	"github.com/gobuffalo/flect"
 	rbacv1 "k8s.io/api/rbac/v1"
 
+	"github.com/DataDog/datadog-operator/internal/controller/datadogagent/feature/utils"
 	"github.com/DataDog/datadog-operator/pkg/kubernetes/rbac"
 )
 
@@ -136,33 +135,16 @@ func getRBACPolicyRules(collectorOpts collectorOptions) []rbacv1.PolicyRule {
 
 	// Add permissions for custom resources
 	if len(collectorOpts.customResources) > 0 {
-		// Group custom resources by API group using sets to avoid duplicates
-		groupedResources := make(map[string]map[string]struct{})
+		rbacBuilder := utils.NewRBACBuilder(commonVerbs...)
 		for _, cr := range collectorOpts.customResources {
-			apiGroup := cr.GroupVersionKind.Group
 			// Use the resource plural if specified, otherwise derive it from the Kind
 			resourceName := cr.ResourcePlural
 			if resourceName == "" {
 				resourceName = strings.ToLower(flect.Pluralize(cr.GroupVersionKind.Kind))
 			}
-
-			if _, exists := groupedResources[apiGroup]; !exists {
-				groupedResources[apiGroup] = make(map[string]struct{})
-			}
-			groupedResources[apiGroup][resourceName] = struct{}{}
-		}
-
-		// Create RBAC rules for each API group
-		for apiGroup, resourceSet := range groupedResources {
-			// Convert set to sorted slice for deterministic output
-			resources := slices.Sorted(maps.Keys(resourceSet))
-
-			rbacRules = append(rbacRules, rbacv1.PolicyRule{
-				APIGroups: []string{apiGroup},
-				Resources: resources,
-				Verbs:     commonVerbs,
-			})
+			rbacBuilder.AddGroupKind(cr.GroupVersionKind.Group, resourceName)
 		}
+		rbacRules = append(rbacRules, rbacBuilder.Build()...)
 	}
 
 	return rbacRules

internal/controller/datadogagent/feature/orchestratorexplorer/rbac.go

@@ -14,6 +14,7 @@ import (
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
 	"github.com/DataDog/datadog-operator/internal/controller/datadogagent/common"
+	"github.com/DataDog/datadog-operator/internal/controller/datadogagent/feature/utils"
 	"github.com/DataDog/datadog-operator/pkg/kubernetes/rbac"
 )
 
@@ -125,12 +126,17 @@ func getRBACPolicyRules(logger logr.Logger, crs []string) []rbacv1.PolicyRule {
 		},
 	}
 
-	groupResources := mapAPIGroupsResources(logger, crs)
-	for group, resources := range groupResources {
-		rbacRules = append(rbacRules, rbacv1.PolicyRule{
-			APIGroups: []string{group},
-			Resources: append([]string{}, resources...),
-		})
+	if len(crs) > 0 {
+		rbacBuilder := utils.NewRBACBuilder()
+		for _, cr := range crs {
+			crSplit := strings.Split(cr, "/")
+			if len(crSplit) == 3 {
+				rbacBuilder.AddGroupKind(crSplit[0], crSplit[2])
+			} else {
+				logger.Error(fmt.Errorf("unable to create cluster role for %s, skipping", cr), "correct format should be group/version/kind")
+			}
+		}
+		rbacRules = append(rbacRules, rbacBuilder.Build()...)
 	}
 
 	defaultVerbs := []string{
@@ -147,19 +153,3 @@ func getRBACPolicyRules(logger logr.Logger, crs []string) []rbacv1.PolicyRule {
 
 	return rbacRules
 }
-
-func mapAPIGroupsResources(logger logr.Logger, customResources []string) map[string][]string {
-	groupResources := make(map[string][]string, len(customResources))
-	for _, cr := range customResources {
-		crSplit := strings.Split(cr, "/")
-		if len(crSplit) == 3 {
-			if _, ok := groupResources[crSplit[0]]; !ok {
-				groupResources[crSplit[0]] = make([]string, 0, len(customResources))
-			}
-			groupResources[crSplit[0]] = append(groupResources[crSplit[0]], crSplit[2])
-		} else {
-			logger.Error(fmt.Errorf("unable to create cluster role for %s, skipping", cr), "correct format should be group/version/kind")
-		}
-	}
-	return groupResources
-}

internal/controller/datadogagent/feature/orchestratorexplorer/rbac_test.go

@@ -6,42 +6,59 @@
 package orchestratorexplorer
 
 import (
+	"strings"
 	"testing"
 
-	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/assert"
-)
+	rbacv1 "k8s.io/api/rbac/v1"
 
-func TestMapAPIGroupsResources(t *testing.T) {
+	"github.com/DataDog/datadog-operator/internal/controller/datadogagent/feature/utils"
+)
 
+func TestRBACBuilderFromCustomResourceStrings(t *testing.T) {
 	for _, tt := range []struct {
 		name            string
 		customResources []string
-		expected        map[string][]string
+		expectedRules   []rbacv1.PolicyRule
 	}{
 		{
 			name:            "empty crs",
 			customResources: []string{},
-			expected:        map[string][]string{},
+			expectedRules:   nil,
 		},
 		{
 			name:            "two crs, same group",
 			customResources: []string{"datadoghq.com/v1alpha1/datadogmetrics", "datadoghq.com/v1alpha1/watermarkpodautoscalers"},
-			expected: map[string][]string{
-				"datadoghq.com": {"datadogmetrics", "watermarkpodautoscalers"},
+			expectedRules: []rbacv1.PolicyRule{
+				{
+					APIGroups: []string{"datadoghq.com"},
+					Resources: []string{"datadogmetrics", "watermarkpodautoscalers"},
+				},
 			},
 		},
 		{
 			name:            "three crs, different groups",
 			customResources: []string{"datadoghq.com/v1alpha1/datadogmetrics", "datadoghq.com/v1alpha1/watermarkpodautoscalers", "cilium.io/v1/ciliumendpoints"},
-			expected: map[string][]string{
-				"datadoghq.com": {"datadogmetrics", "watermarkpodautoscalers"},
-				"cilium.io":     {"ciliumendpoints"},
+			expectedRules: []rbacv1.PolicyRule{
+				{
+					APIGroups: []string{"cilium.io"},
+					Resources: []string{"ciliumendpoints"},
+				},
+				{
+					APIGroups: []string{"datadoghq.com"},
+					Resources: []string{"datadogmetrics", "watermarkpodautoscalers"},
+				},
 			},
 		},
 	} {
-		actualGroupsResources := mapAPIGroupsResources(logr.Logger{}, tt.customResources)
-		assert.Equal(t, tt.expected, actualGroupsResources)
+		t.Run(tt.name, func(t *testing.T) {
+			rbacBuilder := utils.NewRBACBuilder()
+			for _, cr := range tt.customResources {
+				crSplit := strings.Split(cr, "/")
+				rbacBuilder.AddGroupKind(crSplit[0], crSplit[2])
+			}
+			actualRules := rbacBuilder.Build()
+			assert.Equal(t, tt.expectedRules, actualRules)
+		})
 	}
-
 }

internal/controller/datadogagent/feature/utils/utils.go

@@ -6,8 +6,12 @@
 package utils
 
 import (
+	"maps"
+	"slices"
 	"strconv"
+	"strings"
 
+	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/DataDog/datadog-operator/api/datadoghq/v2alpha1"
@@ -67,3 +71,98 @@ func HasAgentDataPlaneAnnotation(dda metav1.Object) bool {
 func HasFineGrainedKubeletAuthz(dda metav1.Object) bool {
 	return hasFeatureEnableAnnotation(dda, EnableFineGrainedKubeletAuthz)
 }
+
+// resourceSet represents a set of resources with their verbs
+type resourceSet map[string][]string
+
+// groupedResources maps API groups to their resource sets
+type groupedResources map[string]resourceSet
+
+// RBACBuilder provides a simple builder for creating RBAC policy rules for custom resources
+type RBACBuilder struct {
+	// groupedResources stores resources grouped by API group
+	groupedResources groupedResources
+	// verbs stores the verbs to apply to all rules
+	verbs []string
+}
+
+// NewRBACBuilder creates a new RBACBuilder instance with the specified verbs
+func NewRBACBuilder(verbs ...string) *RBACBuilder {
+	return &RBACBuilder{
+		groupedResources: make(groupedResources),
+		verbs:            verbs,
+	}
+}
+
+// AddGroupKind adds a custom resource by group and resource name with optional verbs
+// If no verbs are provided, uses the default verbs from NewRBACBuilder
+func (rb *RBACBuilder) AddGroupKind(group, resource string, verbs ...string) *RBACBuilder {
+	if _, exists := rb.groupedResources[group]; !exists {
+		rb.groupedResources[group] = make(resourceSet)
+	}
+
+	// Use provided verbs or fall back to default verbs
+	resourceVerbs := verbs
+	if len(verbs) == 0 {
+		resourceVerbs = rb.verbs
+	}
+
+	existingVerbs := rb.groupedResources[group][resource]
+	allVerbs := append(existingVerbs, resourceVerbs...)
+	verbSet := make(map[string]struct{})
+	var uniqueVerbs []string
+	for _, verb := range allVerbs {
+		if _, exists := verbSet[verb]; !exists {
+			verbSet[verb] = struct{}{}
+			uniqueVerbs = append(uniqueVerbs, verb)
+		}
+	}
+
+	rb.groupedResources[group][resource] = uniqueVerbs
+	return rb
+}
+
+// Build creates the final RBAC policy rules
+func (rb *RBACBuilder) Build() []rbacv1.PolicyRule {
+	if len(rb.groupedResources) == 0 {
+		return nil
+	}
+
+	var rbacRules []rbacv1.PolicyRule
+
+	// Sort API groups for deterministic output
+	apiGroups := slices.Sorted(maps.Keys(rb.groupedResources))
+
+	// Create RBAC rules for each API group
+	for _, apiGroup := range apiGroups {
+		resourceSet := rb.groupedResources[apiGroup]
+
+		// Group resources by their verbs to minimize the number of rules
+		verbToResources := make(map[string][]string)
+		verbsKeyToActualVerbs := make(map[string][]string)
+
+		for resource, verbs := range resourceSet {
+			verbsKey := strings.Join(verbs, ",")
+			verbToResources[verbsKey] = append(verbToResources[verbsKey], resource)
+			verbsKeyToActualVerbs[verbsKey] = verbs
+		}
+
+		// Create one rule per verb combination
+		// Sort verbsKeys for deterministic output
+		verbsKeys := slices.Sorted(maps.Keys(verbToResources))
+		for _, verbsKey := range verbsKeys {
+			resources := verbToResources[verbsKey]
+			slices.Sort(resources)
+
+			rule := rbacv1.PolicyRule{
+				APIGroups: []string{apiGroup},
+				Resources: resources,
+				Verbs:     verbsKeyToActualVerbs[verbsKey],
+			}
+
+			rbacRules = append(rbacRules, rule)
+		}
+	}
+
+	return rbacRules
+}