Merge pull request #937 from chrischdi/pr-deduplicate-by-verbs-and-group

✨ rbac: deduplicate having the same groups, resourceNames, URLs and Verbs

pkg/rbac/parser.go

@@ -93,6 +93,18 @@ func (r *Rule) key() ruleKey {
 	}
 }
 
+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) keyWithResourcesResourceNamesURLsVerbs() string {
+	key := r.key()
+	verbs := strings.Join(r.Verbs, "&")
+	return fmt.Sprintf("%s + %s + %s + %s", key.Resources, key.ResourceNames, key.URLs, 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) {
@@ -168,22 +180,28 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
 // GenerateRoles generate a slice of objs representing either a ClusterRole or a Role object
 // The order of the objs in the returned slice is stable and determined by their namespaces.
 func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{}, error) {
-	rulesByNS := make(map[string][]*Rule)
+	rulesByNSResource := make(map[string][]*Rule)
 	for _, root := range ctx.Roots {
 		markerSet, err := markers.PackageMarkers(ctx.Collector, root)
 		if err != nil {
 			root.AddError(err)
 		}
 
-		// group RBAC markers by namespace
+		// group RBAC markers by namespace and separate by resource
 		for _, markerValue := range markerSet[RuleDefinition.Name] {
 			rule := markerValue.(Rule)
-			namespace := rule.Namespace
-			if _, ok := rulesByNS[namespace]; !ok {
-				rules := make([]*Rule, 0)
-				rulesByNS[namespace] = rules
+			for _, resource := range rule.Resources {
+				r := Rule{
+					Groups:        rule.Groups,
+					Resources:     []string{resource},
+					ResourceNames: rule.ResourceNames,
+					URLs:          rule.URLs,
+					Namespace:     rule.Namespace,
+					Verbs:         rule.Verbs,
+				}
+				namespace := r.Namespace
+				rulesByNSResource[namespace] = append(rulesByNSResource[namespace], &r)
 			}
-			rulesByNS[namespace] = append(rulesByNS[namespace], &rule)
 		}
 	}
 
@@ -200,6 +218,45 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 			ruleMap[key].addVerbs(rule.Verbs)
 		}
 
+		// deduplicate resources
+		// 1. create map based on key without resources
+		ruleMapWithoutResources := make(map[string][]*Rule)
+		for _, rule := range ruleMap {
+			// get key without Resources
+			key := rule.keyWithGroupResourceNamesURLsVerbs()
+			ruleMapWithoutResources[key] = append(ruleMapWithoutResources[key], rule)
+		}
+		// 2. merge to ruleMap
+		ruleMap = make(map[ruleKey]*Rule)
+		for _, rules := range ruleMapWithoutResources {
+			rule := rules[0]
+			for _, mergeRule := range rules[1:] {
+				rule.Resources = append(rule.Resources, mergeRule.Resources...)
+			}
+
+			key := rule.key()
+			ruleMap[key] = rule
+		}
+
+		// deduplicate groups
+		// 1. create map based on key without group
+		ruleMapWithoutGroup := make(map[string][]*Rule)
+		for _, rule := range ruleMap {
+			// get key without Group
+			key := rule.keyWithResourcesResourceNamesURLsVerbs()
+			ruleMapWithoutGroup[key] = append(ruleMapWithoutGroup[key], rule)
+		}
+		// 2. merge to ruleMap
+		ruleMap = make(map[ruleKey]*Rule)
+		for _, rules := range ruleMapWithoutGroup {
+			rule := rules[0]
+			for _, mergeRule := range rules[1:] {
+				rule.Groups = append(rule.Groups, mergeRule.Groups...)
+			}
+			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 {
@@ -216,15 +273,15 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 
 	// collect all the namespaces and sort them
 	var namespaces []string
-	for ns := range rulesByNS {
+	for ns := range rulesByNSResource {
 		namespaces = append(namespaces, ns)
 	}
 	sort.Strings(namespaces)
 
 	// process the items in rulesByNS by the order specified in `namespaces` to make sure that the Role order is stable
 	var objs []interface{}
 	for _, ns := range namespaces {
-		rules := rulesByNS[ns]
+		rules := rulesByNSResource[ns]
 		policyRules := NormalizeRules(rules)
 		if len(policyRules) == 0 {
 			continue

pkg/rbac/parser_integration_test.go

@@ -52,7 +52,7 @@ var _ = Describe("ClusterRole generated by the RBAC Generator", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			By("parsing the desired YAML")
-			for i, expectedRoleBytes := range bytes.Split(expectedFile, []byte("\n---\n"))[1:] {
+			for i, expectedRoleBytes := range bytes.Split(expectedFile, []byte("\n---\n")) {
 				By(fmt.Sprintf("comparing the generated Role and expected Role (Pair %d)", i))
 				obj := objs[i]
 				switch obj := obj.(type) {

pkg/rbac/testdata/controller.go

@@ -11,3 +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-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

pkg/rbac/testdata/role.yaml

@@ -1,4 +1,3 @@
-
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -52,7 +51,57 @@ rules:
   - get
   - patch
   - update
-
+- apiGroups:
+  - 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
+  verbs:
+  - get
+- apiGroups:
+  - not-deduplicate-groups2
+  resources:
+  - some
+  verbs:
+  - list
+- apiGroups:
+  - not-deduplicate-resources
+  resources:
+  - another
+  verbs:
+  - list
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
@@ -66,7 +115,6 @@ rules:
   - jobs
   verbs:
   - get
-
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
@@ -76,11 +124,6 @@ metadata:
 rules:
 - apiGroups:
   - art
-  resources:
-  - jobs
-  verbs:
-  - get
-- apiGroups:
   - wave
   resources:
   - jobs