From 97708aa2c496b5c6cebf922c0031aaf6f46ddf3e Mon Sep 17 00:00:00 2001
From: Christian Schlotter <christian.schlotter@broadcom.com>
Date: Thu, 29 Aug 2024 13:30:50 +0200
Subject: [PATCH] rbac: fix adding nonResourceURLs including normalisation

---
 pkg/rbac/parser.go              | 39 +++++++++++++++++++++++++++++++++
 pkg/rbac/testdata/controller.go |  2 ++
 pkg/rbac/testdata/role.yaml     |  5 +++++
 3 files changed, 46 insertions(+)

diff --git a/pkg/rbac/parser.go b/pkg/rbac/parser.go
index 51b4c043f..d9f377b2d 100644
--- a/pkg/rbac/parser.go
+++ b/pkg/rbac/parser.go
@@ -105,6 +105,12 @@ func (r *Rule) keyWithResourcesResourceNamesURLsVerbs() string {
 	return fmt.Sprintf("%s + %s + %s + %s", key.Resources, key.ResourceNames, key.URLs, verbs)
 }
 
+func (r *Rule) keyWitGroupResourcesResourceNamesVerbs() string {
+	key := r.key()
+	verbs := strings.Join(r.Verbs, "&")
+	return fmt.Sprintf("%s + %s + %s + %s", key.Groups, key.Resources, key.ResourceNames, verbs)
+}
+
 // addVerbs adds new verbs into a Rule.
 // The duplicates in `r.Verbs` will be removed, and then `r.Verbs` will be sorted.
 func (r *Rule) addVerbs(verbs []string) {
@@ -190,6 +196,20 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 		// group RBAC markers by namespace and separate by resource
 		for _, markerValue := range markerSet[RuleDefinition.Name] {
 			rule := markerValue.(Rule)
+			if len(rule.Resources) == 0 {
+				// Add a rule without any resource if Resources is empty.
+				r := Rule{
+					Groups:        rule.Groups,
+					Resources:     []string{},
+					ResourceNames: rule.ResourceNames,
+					URLs:          rule.URLs,
+					Namespace:     rule.Namespace,
+					Verbs:         rule.Verbs,
+				}
+				namespace := r.Namespace
+				rulesByNSResource[namespace] = append(rulesByNSResource[namespace], &r)
+				continue
+			}
 			for _, resource := range rule.Resources {
 				r := Rule{
 					Groups:        rule.Groups,
@@ -257,6 +277,25 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 			ruleMap[key] = rule
 		}
 
+		// deduplicate URLs
+		// 1. create map based on key without URLs
+		ruleMapWithoutURLs := make(map[string][]*Rule)
+		for _, rule := range ruleMap {
+			// get key without Group
+			key := rule.keyWitGroupResourcesResourceNamesVerbs()
+			ruleMapWithoutURLs[key] = append(ruleMapWithoutURLs[key], rule)
+		}
+		// 2. merge to ruleMap
+		ruleMap = make(map[ruleKey]*Rule)
+		for _, rules := range ruleMapWithoutURLs {
+			rule := rules[0]
+			for _, mergeRule := range rules[1:] {
+				rule.URLs = append(rule.URLs, mergeRule.URLs...)
+			}
+			key := rule.key()
+			ruleMap[key] = rule
+		}
+
 		// sort the Rules in rules according to their ruleKeys
 		keys := make([]ruleKey, 0, len(ruleMap))
 		for key := range ruleMap {
diff --git a/pkg/rbac/testdata/controller.go b/pkg/rbac/testdata/controller.go
index 82125e015..9a8f5d256 100644
--- a/pkg/rbac/testdata/controller.go
+++ b/pkg/rbac/testdata/controller.go
@@ -28,3 +28,5 @@ package controller
 // +kubebuilder:rbac:groups=not-deduplicate-resources,resources=another,verbs=list
 // +kubebuilder:rbac:groups=not-deduplicate-groups1,resources=some,verbs=get
 // +kubebuilder:rbac:groups=not-deduplicate-groups2,resources=some,verbs=list
+// +kubebuilder:rbac:urls=/url-to-duplicate,verbs=get
+// +kubebuilder:rbac:urls=/another/url-to-duplicate,verbs=get
diff --git a/pkg/rbac/testdata/role.yaml b/pkg/rbac/testdata/role.yaml
index 33dfc8052..44ac99ce0 100644
--- a/pkg/rbac/testdata/role.yaml
+++ b/pkg/rbac/testdata/role.yaml
@@ -4,6 +4,11 @@ kind: ClusterRole
 metadata:
   name: manager-role
 rules:
+- nonResourceURLs:
+  - /another/url-to-duplicate
+  - /url-to-duplicate
+  verbs:
+  - get
 - apiGroups:
   - art
   resources: