Merge pull request #260 from haiyanmeng/role

Add a new `namespace` field into the RBAC marker

Author: k8s-ci-robot

pkg/rbac/parser.go

@@ -52,6 +52,10 @@ type Rule struct {
 	Verbs []string
 	// URL specifies the non-resource URLs that this rule encompasses.
 	URLs []string `marker:"urls,optional"`
+	// Namespace specifies the scope of the Rule.
+	// If not set, the Rule belongs to the generated ClusterRole.
+	// If set, the Rule belongs to a Role, whose namespace is specified by this field.
+	Namespace string `marker:",optional"`
 }
 
 // ruleKey represents the resources and non-resources a Rule applies.
@@ -146,69 +150,109 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
 	return nil
 }
 
-// GenerateClusterRole generates a rbacv1.ClusterRole object
-func GenerateClusterRole(ctx *genall.GenerationContext, roleName string) (*rbacv1.ClusterRole, error) {
-	rules := make(map[ruleKey]*Rule)
+// 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)
 		}
 
-		// all the Rules having the same ruleKey will be merged into the first Rule
+		// 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 := rules[key]; !ok {
-				rules[key] = &rule
+			if _, ok := ruleMap[key]; !ok {
+				ruleMap[key] = rule
 				continue
 			}
-			rules[key].addVerbs(rule.Verbs)
+			ruleMap[key].addVerbs(rule.Verbs)
 		}
-	}
 
-	if len(rules) == 0 {
-		return nil, nil
-	}
+		// 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))
 
-	// sort the Rules in rules according to their ruleKeys
-	keys := make([]ruleKey, 0)
-	for key := range rules {
-		keys = append(keys, key)
-	}
-	sort.Sort(ruleKeys(keys))
+		var policyRules []rbacv1.PolicyRule
+		for _, key := range keys {
+			policyRules = append(policyRules, ruleMap[key].ToRule())
 
-	var policyRules []rbacv1.PolicyRule
-	for _, key := range keys {
-		policyRules = append(policyRules, rules[key].ToRule())
+		}
+		return policyRules
+	}
 
+	// collect all the namespaces and sort them
+	var namespaces []string
+	for ns := range rulesByNS {
+		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]
+		policyRules := NormalizeRules(rules)
+		if len(policyRules) == 0 {
+			continue
+		}
+		if ns == "" {
+			objs = append(objs, rbacv1.ClusterRole{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "ClusterRole",
+					APIVersion: rbacv1.SchemeGroupVersion.String(),
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: roleName,
+				},
+				Rules: policyRules,
+			})
+		} else {
+			objs = append(objs, rbacv1.Role{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Role",
+					APIVersion: rbacv1.SchemeGroupVersion.String(),
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      roleName,
+					Namespace: ns,
+				},
+				Rules: policyRules,
+			})
+		}
 	}
 
-	return &rbacv1.ClusterRole{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "ClusterRole",
-			APIVersion: rbacv1.SchemeGroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name: roleName,
-		},
-		Rules: policyRules,
-	}, nil
+	return objs, nil
 }
 
 func (g Generator) Generate(ctx *genall.GenerationContext) error {
-	clusterRole, err := GenerateClusterRole(ctx, g.RoleName)
+	objs, err := GenerateRoles(ctx, g.RoleName)
 	if err != nil {
 		return err
 	}
 
-	if clusterRole == nil {
+	if len(objs) == 0 {
 		return nil
 	}
 
-	if err := ctx.WriteYAML("role.yaml", *clusterRole); err != nil {
-		return err
-	}
-
-	return nil
+	return ctx.WriteYAML("role.yaml", objs...)
 }

pkg/rbac/parser_integration_test.go

@@ -1,6 +1,8 @@
 package rbac_test
 
 import (
+	"bytes"
+	"fmt"
 	"io/ioutil"
 	"os"
 
@@ -43,19 +45,29 @@ var _ = Describe("ClusterRole generated by the RBAC Generator", func() {
 			}
 
 			By("generating a ClusterRole")
-			clusterRole, err := rbac.GenerateClusterRole(ctx, "manager-role")
+			objs, err := rbac.GenerateRoles(ctx, "manager-role")
 			Expect(err).NotTo(HaveOccurred())
 
 			By("loading the desired YAML")
 			expectedFile, err := ioutil.ReadFile("role.yaml")
 			Expect(err).NotTo(HaveOccurred())
 
 			By("parsing the desired YAML")
-			var role rbacv1.ClusterRole
-			Expect(yaml.Unmarshal(expectedFile, &role)).To(Succeed())
+			for i, expectedRoleBytes := range bytes.Split(expectedFile, []byte("\n---\n"))[1:] {
+				By(fmt.Sprintf("comparing the generated Role and expected Role (Pair %d)", i))
+				obj := objs[i]
+				switch obj := obj.(type) {
+				case rbacv1.ClusterRole:
+					var expectedClusterRole rbacv1.ClusterRole
+					Expect(yaml.Unmarshal(expectedRoleBytes, &expectedClusterRole)).To(Succeed())
+					Expect(obj).To(Equal(expectedClusterRole), "type not as expected, check pkg/rbac/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(obj, expectedClusterRole))
+				default:
+					var expectedRole rbacv1.Role
+					Expect(yaml.Unmarshal(expectedRoleBytes, &expectedRole)).To(Succeed())
+					Expect(obj).To(Equal(expectedRole), "type not as expected, check pkg/rbac/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(obj, expectedRole))
+				}
+			}
 
-			By("comparing the two")
-			Expect(*clusterRole).To(Equal(role), "type not as expected, check pkg/rbac/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(*clusterRole, role))
 		})
 	}
 })

pkg/rbac/testdata/controller.go

@@ -3,7 +3,10 @@ package controller
 // +kubebuilder:rbac:groups=batch.io,resources=cronjobs,verbs=get;watch;create
 // +kubebuilder:rbac:groups=batch.io,resources=cronjobs/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=art,resources=jobs,verbs=get
+// +kubebuilder:rbac:groups=wave,resources=jobs,verbs=get,namespace=zoo
 // +kubebuilder:rbac:groups=batch;batch;batch,resources=jobs/status,verbs=watch
 // +kubebuilder:rbac:groups=batch;cron,resources=jobs/status,verbs=create;get
+// +kubebuilder:rbac:groups=art,resources=jobs,verbs=get,namespace=zoo
 // +kubebuilder:rbac:groups=cron;batch,resources=jobs/status,verbs=get;create
 // +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=watch;watch
+// +kubebuilder:rbac:groups=art,resources=jobs,verbs=get,namespace=park

pkg/rbac/testdata/role.yaml

@@ -42,3 +42,39 @@ rules:
   - get
   - patch
   - update
+
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  creationTimestamp: null
+  name: manager-role
+  namespace: park
+rules:
+- apiGroups:
+  - art
+  resources:
+  - jobs
+  verbs:
+  - get
+
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  creationTimestamp: null
+  name: manager-role
+  namespace: zoo
+rules:
+- apiGroups:
+  - art
+  resources:
+  - jobs
+  verbs:
+  - get
+- apiGroups:
+  - wave
+  resources:
+  - jobs
+  verbs:
+  - get

pkg/rbac/zz_generated.markerhelp.go

@@ -64,6 +64,10 @@ func (Rule) Help() *markers.DefinitionHelp {
 				Summary: "URL specifies the non-resource URLs that this rule encompasses.",
 				Details: "",
 			},
+			"Namespace": markers.DetailedHelp{
+				Summary: "specifies the scope of the Rule. If not set, the Rule belongs to the generated ClusterRole. If set, the Rule belongs to a Role, whose namespace is specified by this field.",
+				Details: "",
+			},
 		},
 	}
 }

test.sh

@@ -123,6 +123,7 @@ golangci-lint run --disable-all \
     --enable=misspell \
     --enable=gocyclo \
     --enable=gosec \
+    --deadline=5m \
     ./pkg/... ./cmd/...
 
 # --enable=structcheck \  # doesn't understand embedded structs