🐛 Further deduplication of RBAC rules

pkg/rbac/parser.go

@@ -23,22 +23,20 @@ limitations under the License.
 package rbac
 
 import (
-	"fmt"
 	"sort"
 	"strings"
 
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	sets "k8s.io/apimachinery/pkg/util/sets"
 
 	"sigs.k8s.io/controller-tools/pkg/genall"
 	"sigs.k8s.io/controller-tools/pkg/markers"
 )
 
-var (
-	// RuleDefinition is a marker for defining RBAC rules.
-	// Call ToRule on the value to get a Kubernetes RBAC policy rule.
-	RuleDefinition = markers.Must(markers.MakeDefinition("kubebuilder:rbac", markers.DescribesPackage, Rule{}))
-)
+// RuleDefinition is a marker for defining RBAC rules.
+// Call ToRule on the value to get a Kubernetes RBAC policy rule.
+var RuleDefinition = markers.Must(markers.MakeDefinition("kubebuilder:rbac", markers.DescribesPackage, Rule{}))
 
 // +controllertools:marker:generateHelp:category=RBAC
 
@@ -63,83 +61,107 @@ type Rule struct {
 	Namespace string `marker:",optional"`
 }
 
-// ruleKey represents the resources and non-resources a Rule applies.
-type ruleKey struct {
-	Groups        string
-	Resources     string
-	ResourceNames string
-	URLs          string
-}
+func newSet(strings []string) sets.String {
+	if strings == nil { // preserve nils for back-compat with existing code
+		return nil
+	}
 
-func (key ruleKey) String() string {
-	return fmt.Sprintf("%s + %s + %s + %s", key.Groups, key.Resources, key.ResourceNames, key.URLs)
+	return sets.NewString(strings...)
 }
 
-// ruleKeys implements sort.Interface
-type ruleKeys []ruleKey
-
-func (keys ruleKeys) Len() int           { return len(keys) }
-func (keys ruleKeys) Swap(i, j int)      { keys[i], keys[j] = keys[j], keys[i] }
-func (keys ruleKeys) Less(i, j int) bool { return keys[i].String() < keys[j].String() }
-
-// key normalizes the Rule and returns a ruleKey object.
-func (r *Rule) key() ruleKey {
-	r.normalize()
-	return ruleKey{
-		Groups:        strings.Join(r.Groups, "&"),
-		Resources:     strings.Join(r.Resources, "&"),
-		ResourceNames: strings.Join(r.ResourceNames, "&"),
-		URLs:          strings.Join(r.URLs, "&"),
+func (r *Rule) Normalize() *NormalizedRule {
+	result := &NormalizedRule{
+		Groups:        newSet(r.Groups),
+		Resources:     newSet(r.Resources),
+		ResourceNames: newSet(r.ResourceNames),
+		Verbs:         newSet(r.Verbs),
+		URLs:          newSet(r.URLs),
+		Namespace:     r.Namespace,
+	}
+
+	// simplify Resources and Verbs which both support special "*" to mean 'any'
+	// if this is specified, remove all other names
+	if result.Resources.Has("*") {
+		result.Resources = sets.NewString("*")
 	}
+
+	if result.Verbs.Has("*") {
+		result.Verbs = sets.NewString("*")
+	}
+
+	// fix the group names, since letting people type "core" is nice
+	if result.Groups.Has("core") {
+		result.Groups.Delete("core")
+		result.Groups.Insert("")
+	}
+
+	result.GenerateComparisonKey()
+
+	return result
 }
 
-// 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) {
-	r.Verbs = removeDupAndSort(append(r.Verbs, verbs...))
+type NormalizedRule struct {
+	// if two different rules have the same comparison key then they can have their verbs merged
+	// this key should comprise all the fields below except Verbs and Namespace (since rules
+	// are partitioned by namespace before merging)
+	ComparisonKey string
+
+	Namespace     string
+	Groups        sets.String
+	Resources     sets.String
+	ResourceNames sets.String
+	URLs          sets.String
+
+	Verbs sets.String
 }
 
-// normalize removes duplicates from each field of a Rule, and sorts each field.
-func (r *Rule) normalize() {
-	r.Groups = removeDupAndSort(r.Groups)
-	r.Resources = removeDupAndSort(r.Resources)
-	r.ResourceNames = removeDupAndSort(r.ResourceNames)
-	r.Verbs = removeDupAndSort(r.Verbs)
-	r.URLs = removeDupAndSort(r.URLs)
+// GenerateComparisonKey generates the ComparisonKey
+func (nr *NormalizedRule) GenerateComparisonKey() {
+	nr.ComparisonKey = strings.Join(
+		[]string{
+			strings.Join(setToSorted(nr.Groups), "&"),
+			strings.Join(setToSorted(nr.Resources), "&"),
+			strings.Join(setToSorted(nr.ResourceNames), "&"),
+			strings.Join(setToSorted(nr.URLs), "&"),
+		},
+		" + ")
 }
 
-// removeDupAndSort removes duplicates in strs, sorts the items, and returns a
-// new slice of strings.
-func removeDupAndSort(strs []string) []string {
-	set := make(map[string]bool)
-	for _, str := range strs {
-		if _, ok := set[str]; !ok {
-			set[str] = true
-		}
+func setToSorted(set sets.String) []string {
+	if set == nil { // preserve nils
+		return nil
 	}
 
-	var result []string
-	for str := range set {
-		result = append(result, str)
-	}
+	result := set.UnsortedList()
 	sort.Strings(result)
 	return result
 }
 
+// Subsumes indicates if one rule entirely determines another,
+// meaning that the other is unnecessary.
+// Remember that Kubernetes RBAC rules are purely additive, there
+// are no deny rules.
+func (nr *NormalizedRule) Subsumes(other *NormalizedRule) bool {
+	// See the code for documentation of these fields: https://github.com/kubernetes/api/blob/v0.23.6/rbac/v1/types.go#L49
+	return nr.Namespace == other.Namespace &&
+		(nr.Groups.IsSuperset(other.Groups)) &&
+		// Resources supports special "*" to mean "any"
+		(nr.Resources.Has("*") || nr.Resources.IsSuperset(other.Resources)) &&
+		// Empty ResourceNames means "any"
+		(len(nr.ResourceNames) == 0 || nr.ResourceNames.IsSuperset(other.ResourceNames)) &&
+		nr.URLs.IsSuperset(other.URLs) && // TODO: check? also URLs can have "*" at specific locations
+		// Verbs support special "*" to mean "any"
+		(nr.Verbs.Has("*") || nr.Verbs.IsSuperset(other.Verbs))
+}
+
 // ToRule converts this rule to its Kubernetes API form.
-func (r *Rule) ToRule() rbacv1.PolicyRule {
-	// fix the group names first, since letting people type "core" is nice
-	for i, group := range r.Groups {
-		if group == "core" {
-			r.Groups[i] = ""
-		}
-	}
+func (nr *NormalizedRule) ToRule() rbacv1.PolicyRule {
 	return rbacv1.PolicyRule{
-		APIGroups:       r.Groups,
-		Verbs:           r.Verbs,
-		Resources:       r.Resources,
-		ResourceNames:   r.ResourceNames,
-		NonResourceURLs: r.URLs,
+		APIGroups:       setToSorted(nr.Groups),
+		Verbs:           setToSorted(nr.Verbs),
+		Resources:       setToSorted(nr.Resources),
+		ResourceNames:   setToSorted(nr.ResourceNames),
+		NonResourceURLs: setToSorted(nr.URLs),
 	}
 }
 
@@ -162,52 +184,7 @@ 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)
-	for _, root := range ctx.Roots {
-		markerSet, err := markers.PackageMarkers(ctx.Collector, root)
-		if err != nil {
-			root.AddError(err)
-		}
-
-		// group RBAC markers by namespace
-		for _, markerValue := range markerSet[RuleDefinition.Name] {
-			rule := markerValue.(Rule)
-			namespace := rule.Namespace
-			if _, ok := rulesByNS[namespace]; !ok {
-				rules := make([]*Rule, 0)
-				rulesByNS[namespace] = rules
-			}
-			rulesByNS[namespace] = append(rulesByNS[namespace], &rule)
-		}
-	}
-
-	// NormalizeRules merge Rule with the same ruleKey and sort the Rules
-	NormalizeRules := func(rules []*Rule) []rbacv1.PolicyRule {
-		ruleMap := make(map[ruleKey]*Rule)
-		// all the Rules having the same ruleKey will be merged into the first Rule
-		for _, rule := range rules {
-			key := rule.key()
-			if _, ok := ruleMap[key]; !ok {
-				ruleMap[key] = rule
-				continue
-			}
-			ruleMap[key].addVerbs(rule.Verbs)
-		}
-
-		// sort the Rules in rules according to their ruleKeys
-		keys := make([]ruleKey, 0, len(ruleMap))
-		for key := range ruleMap {
-			keys = append(keys, key)
-		}
-		sort.Sort(ruleKeys(keys))
-
-		var policyRules []rbacv1.PolicyRule
-		for _, key := range keys {
-			policyRules = append(policyRules, ruleMap[key].ToRule())
-
-		}
-		return policyRules
-	}
+	rulesByNS := GroupRulesByNamespace(ctx)
 
 	// collect all the namespaces and sort them
 	var namespaces []string
@@ -253,6 +230,101 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
 	return objs, nil
 }
 
+func GroupRulesByNamespace(ctx *genall.GenerationContext) map[string][]*Rule {
+	rulesByNS := 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
+		for _, markerValue := range markerSet[RuleDefinition.Name] {
+			rule := markerValue.(Rule)
+			namespace := rule.Namespace
+			rulesByNS[namespace] = append(rulesByNS[namespace], &rule)
+		}
+	}
+
+	return rulesByNS
+}
+
+// insertRule inserts a rule into a destination slice, deduplicating via the
+// Subsumes function or merging verbs if appropriate
+func insertRule(dest []*NormalizedRule, it *NormalizedRule) []*NormalizedRule {
+	// this is not going to be very fast but the set of rules should always be small
+	mergeWith := -1
+	for ix, other := range dest {
+		if other.Subsumes(it) {
+			// not needed; another rule handles this case
+			return dest
+		}
+
+		if it.Subsumes(other) {
+			// rebuild whole list:
+			// the 'it' rule subsumes the 'other' rule;
+			// but it might also subsume other rules in the list, so we
+			// need to go through and check them as well.
+			//
+			// redoing the insertion with `it` first handles this:
+			result := []*NormalizedRule{it}
+			for _, d := range dest {
+				result = insertRule(result, d)
+			}
+
+			return result
+		}
+
+		if it.ComparisonKey == other.ComparisonKey {
+			// match the same things, can merge their
+			// verbs (if no better match)
+			mergeWith = ix
+		}
+	}
+
+	if mergeWith >= 0 {
+		// we found one to merge with
+		insertAll(dest[mergeWith].Verbs, it.Verbs)
+		return dest
+	}
+
+	// otherwise, insert it
+	return append(dest, it)
+}
+
+func insertAll(set sets.String, other sets.String) {
+	for value := range other {
+		set.Insert(value)
+	}
+}
+
+// Sorts the rules for deterministic output:
+type ruleSorter []*NormalizedRule
+
+// ruleSorter implements sort.Interface
+var _ sort.Interface = ruleSorter{}
+
+func (keys ruleSorter) Len() int           { return len(keys) }
+func (keys ruleSorter) Swap(i, j int)      { keys[i], keys[j] = keys[j], keys[i] }
+func (keys ruleSorter) Less(i, j int) bool { return keys[i].ComparisonKey < keys[j].ComparisonKey }
+
+// NormalizeRules merges Rules that can be merged, and sorts the Rules
+func NormalizeRules(rules []*Rule) []rbacv1.PolicyRule {
+	var simplified []*NormalizedRule
+	for _, rule := range rules {
+		simplified = insertRule(simplified, rule.Normalize())
+	}
+
+	sort.Sort(ruleSorter(simplified))
+
+	result := make([]rbacv1.PolicyRule, len(simplified))
+	for i := range simplified {
+		result[i] = simplified[i].ToRule()
+	}
+
+	return result
+}
+
 func (g Generator) Generate(ctx *genall.GenerationContext) error {
 	objs, err := GenerateRoles(ctx, g.RoleName)
 	if err != nil {