rbac: more test cases and review fixes

kubernetes-sigs · May 14, 2024 · 73b2aad · 73b2aad
1 parent e0fa97d
commit 73b2aad
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 24 deletions.
diff --git a/pkg/rbac/parser.go b/pkg/rbac/parser.go
@@ -93,13 +93,13 @@ func (r *Rule) key() ruleKey {
 	}
 }
 
-func (r *Rule) KeyWithoutResources() string {
+func (r *Rule) keyWithGroupResourceNamesURLsVerbs() string {
 	key := r.key()
 	verbs := strings.Join(r.Verbs, "&")
 	return fmt.Sprintf("%s + %s + %s + %s", key.Groups, key.ResourceNames, key.URLs, verbs)
 }
 
-func (r *Rule) KeyWithoutGroups() string {
+func (r *Rule) keyWithResourcesResourceNamesURLsVerbs() string {
 	key := r.key()
 	verbs := strings.Join(r.Verbs, "&")
 	return fmt.Sprintf("%s + %s + %s + %s", key.Resources, key.ResourceNames, key.URLs, verbs)
@@ -187,7 +187,7 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 			root.AddError(err)
 		}
 
-		// group RBAC markers by namespace and resource
+		// group RBAC markers by namespace and separate by resource
 		for _, markerValue := range markerSet[RuleDefinition.Name] {
 			rule := markerValue.(Rule)
 			for _, resource := range rule.Resources {
@@ -200,10 +200,6 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 					Verbs:         rule.Verbs,
 				}
 				namespace := r.Namespace
-				if _, ok := rulesByNSResource[namespace]; !ok {
-					rules := make([]*Rule, 0)
-					rulesByNSResource[namespace] = rules
-				}
 				rulesByNSResource[namespace] = append(rulesByNSResource[namespace], &r)
 			}
 		}
@@ -226,12 +222,8 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 		// 1. create map based on key without resources
 		ruleMapWithoutResources := make(map[string][]*Rule)
 		for _, rule := range ruleMap {
-			// unset Resources on the key
-			key := rule.KeyWithoutResources()
-			if _, ok := ruleMapWithoutResources[key]; !ok {
-				rules := make([]*Rule, 0)
-				ruleMapWithoutResources[key] = rules
-			}
+			// get key without Resources
+			key := rule.keyWithGroupResourceNamesURLsVerbs()
 			ruleMapWithoutResources[key] = append(ruleMapWithoutResources[key], rule)
 		}
 		// 2. merge to ruleMap
@@ -250,12 +242,8 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 		// 1. create map based on key without group
 		ruleMapWithoutGroup := make(map[string][]*Rule)
 		for _, rule := range ruleMap {
-			// unset Resources on the key
-			key := rule.KeyWithoutGroups()
-			if _, ok := ruleMapWithoutGroup[key]; !ok {
-				rules := make([]*Rule, 0)
-				ruleMapWithoutGroup[key] = rules
-			}
+			// get key without Group
+			key := rule.keyWithResourcesResourceNamesURLsVerbs()
 			ruleMapWithoutGroup[key] = append(ruleMapWithoutGroup[key], rule)
 		}
 		// 2. merge to ruleMap

diff --git a/pkg/rbac/testdata/controller.go b/pkg/rbac/testdata/controller.go
@@ -11,6 +11,20 @@ package controller
 // +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=watch;watch
 // +kubebuilder:rbac:groups=art,resources=jobs,verbs=get,namespace=park
 // +kubebuilder:rbac:groups=batch.io,resources=cronjobs,resourceNames=foo;bar;baz,verbs=get;watch
-// +kubebuilder:rbac:groups=deduplicate,resources=some;some/status,verbs=get;list
-// +kubebuilder:rbac:groups=deduplicate,resources=some,verbs=get
-// +kubebuilder:rbac:groups=deduplicate,resources=some/status,verbs=list
+// +kubebuilder:rbac:groups=deduplicate-verbs,resources=some,verbs=get;list
+// +kubebuilder:rbac:groups=deduplicate-verbs,resources=some,verbs=get
+// +kubebuilder:rbac:groups=deduplicate-verbs,resources=some,verbs=list
+// +kubebuilder:rbac:groups=deduplicate-resources,resources=one,verbs=create
+// +kubebuilder:rbac:groups=deduplicate-resources,resources=two,verbs=create
+// +kubebuilder:rbac:groups=deduplicate-resources,resources=three,verbs=create
+// +kubebuilder:rbac:groups=deduplicate-groups1,resources=foo,verbs=patch
+// +kubebuilder:rbac:groups=deduplicate-groups2,resources=foo,verbs=patch
+// +kubebuilder:rbac:groups=deduplicate-groups3,resources=foo,verbs=patch
+// +kubebuilder:rbac:groups=deduplicate-all,resources=foo;bar,verbs=get;list
+// +kubebuilder:rbac:groups=deduplicate-all,resources=foo,verbs=get
+// +kubebuilder:rbac:groups=deduplicate-all,resources=bar,verbs=list
+// +kubebuilder:rbac:groups=deduplicate-all-group,resources=foo;bar,verbs=get;list
+// +kubebuilder:rbac:groups=not-deduplicate-resources,resources=some,verbs=get
+// +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
diff --git a/pkg/rbac/testdata/role.yaml b/pkg/rbac/testdata/role.yaml
@@ -52,12 +52,55 @@ rules:
   - patch
   - update
 - apiGroups:
-  - deduplicate
+  - deduplicate-all
+  - deduplicate-all-group
+  resources:
+  - bar
+  - foo
+  verbs:
+  - get
+  - list
+- apiGroups:
+  - deduplicate-groups1
+  - deduplicate-groups2
+  - deduplicate-groups3
+  resources:
+  - foo
+  verbs:
+  - patch
+- apiGroups:
+  - deduplicate-resources
+  resources:
+  - one
+  - three
+  - two
+  verbs:
+  - create
+- apiGroups:
+  - deduplicate-verbs
+  resources:
+  - some
+  verbs:
+  - get
+  - list
+- apiGroups:
+  - not-deduplicate-groups1
+  - not-deduplicate-resources
   resources:
   - some
-  - some/status
   verbs:
   - get
+- apiGroups:
+  - not-deduplicate-groups2
+  resources:
+  - some
+  verbs:
+  - list
+- apiGroups:
+  - not-deduplicate-resources
+  resources:
+  - another
+  verbs:
   - list
 ---
 apiVersion: rbac.authorization.k8s.io/v1