diff --git a/pkg/rbac/parser.go b/pkg/rbac/parser.go index 50bdf322c..51b4c043f 100644 --- a/pkg/rbac/parser.go +++ b/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,7 +273,7 @@ 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) @@ -224,7 +281,7 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{ // 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 diff --git a/pkg/rbac/parser_integration_test.go b/pkg/rbac/parser_integration_test.go index f4f7bf45a..74f8a7b56 100644 --- a/pkg/rbac/parser_integration_test.go +++ b/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) { diff --git a/pkg/rbac/testdata/controller.go b/pkg/rbac/testdata/controller.go index 27800d729..82125e015 100644 --- a/pkg/rbac/testdata/controller.go +++ b/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 diff --git a/pkg/rbac/testdata/role.yaml b/pkg/rbac/testdata/role.yaml index 3c3d01a81..33dfc8052 100644 --- a/pkg/rbac/testdata/role.yaml +++ b/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