Avoid reordering Rules

Currently, the RBAC generator may change the order of Rules defined by
the "kubebuilder:rbac" markers, which may break the RBAC and should be
avoided. This commit stops reordering Rules for the correctness of RBAC.

Two notes:
1) each field of a given Rule will still be ordered for the purpose of
deduplicating Rules;
2) all the Rules having the same ruleKey will be merged into the first
Rule

Signed-off-by: Haiyan Meng <[email protected]>

Author: haiyanmeng

pkg/rbac/parser.go

@@ -121,45 +121,64 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
 	return into.Register(RuleDefinition)
 }
 
-func (g Generator) Generate(ctx *genall.GenerationContext) error {
-	rules := make(map[ruleKey]*Rule)
+// GenerateClusterRole generates a rbacv1.ClusterRole object
+func GenerateClusterRole(ctx *genall.GenerationContext, roleName string) (*rbacv1.ClusterRole, error) {
+	// Using a slice here to preserve the order of Rules
+	var ruleSlice []Rule
+	// ruleKeyToIndexMap maps a ruleKey to its index in ruleSlice
+	ruleKeyToIndexMap := make(map[ruleKey]int)
 	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
 		for _, markerValue := range markerSet[RuleDefinition.Name] {
 			rule := markerValue.(Rule)
 			key := rule.key()
-			if _, ok := rules[key]; !ok {
-				rules[key] = &rule
+			if _, ok := ruleKeyToIndexMap[key]; !ok {
+				ruleKeyToIndexMap[key] = len(ruleSlice)
+				ruleSlice = append(ruleSlice, rule)
 				continue
 			}
-			rules[key].addVerbs(rule.Verbs)
+			ruleSlice[ruleKeyToIndexMap[key]].addVerbs(rule.Verbs)
 		}
 	}
 
 	var policyRules []rbacv1.PolicyRule
-	for _, rule := range rules {
+	for _, rule := range ruleSlice {
 		policyRules = append(policyRules, rule.ToRule())
 
 	}
 
 	if len(policyRules) == 0 {
-		return nil
+		return nil, nil
 	}
 
-	if err := ctx.WriteYAML("role.yaml", rbacv1.ClusterRole{
+	return &rbacv1.ClusterRole{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "ClusterRole",
 			APIVersion: rbacv1.SchemeGroupVersion.String(),
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name: g.RoleName,
+			Name: roleName,
 		},
 		Rules: policyRules,
-	}); err != nil {
+	}, nil
+}
+
+func (g Generator) Generate(ctx *genall.GenerationContext) error {
+	clusterRole, err := GenerateClusterRole(ctx, g.RoleName)
+	if err != nil {
+		return err
+	}
+
+	if clusterRole == nil {
+		return nil
+	}
+
+	if err := ctx.WriteYAML("role.yaml", *clusterRole); err != nil {
 		return err
 	}
 

pkg/rbac/parser_integration_test.go

@@ -0,0 +1,57 @@
+package rbac_test
+
+import (
+	"io/ioutil"
+	"os"
+
+	"github.com/google/go-cmp/cmp"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"sigs.k8s.io/yaml"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+
+	"sigs.k8s.io/controller-tools/pkg/genall"
+	"sigs.k8s.io/controller-tools/pkg/loader"
+	"sigs.k8s.io/controller-tools/pkg/markers"
+	"sigs.k8s.io/controller-tools/pkg/rbac"
+)
+
+var _ = Describe("ClusterRole generated by the RBAC Generator", func() {
+	It("should match the expected result", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata")).To(Succeed()) // go modules are directory-sensitive
+		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
+
+		By("loading the roots")
+		pkgs, err := loader.LoadRoots(".")
+		Expect(err).NotTo(HaveOccurred())
+
+		By("registering RBAC rule marker")
+		reg := &markers.Registry{}
+		Expect(reg.Register(rbac.RuleDefinition)).To(Succeed())
+
+		By("creating GenerationContext")
+		ctx := &genall.GenerationContext{
+			Collector: &markers.Collector{Registry: reg},
+			Roots:     pkgs,
+		}
+
+		By("generating a ClusterRole")
+		clusterRole, err := rbac.GenerateClusterRole(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())
+
+		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/rbac_suite_test.go

@@ -0,0 +1,29 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rbac_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestCRDGeneration(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "RBAC Generation Suite")
+}

pkg/rbac/testdata/README.md

@@ -0,0 +1,18 @@
+# RBAC Integration Test testdata
+
+This contains a tiny module used for testdata for the RBAC integration
+test.  The directory should always be called testdata, so Go treats it
+specially.
+
+If you add a new marker, re-generate the golden output file,
+`role.yaml`, with:
+
+```bash
+$ /path/to/current/build/of/controller-gen rbac:roleName=manager-role paths=. output:dir=.
+```
+
+Make sure you review the diff to ensure that it only contains the desired
+changes!
+
+If you didn't add a new marker and this output changes, make sure you have
+a good explanation for why generated output needs to change!

pkg/rbac/testdata/controller.go

@@ -0,0 +1,9 @@
+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=batch;batch;batch,resources=jobs/status,verbs=watch
+// +kubebuilder:rbac:groups=batch;cron,resources=jobs/status,verbs=create;get
+// +kubebuilder:rbac:groups=cron;batch,resources=jobs/status,verbs=get;create
+// +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=watch;watch

pkg/rbac/testdata/role.yaml

@@ -0,0 +1,44 @@
+
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  creationTimestamp: null
+  name: manager-role
+rules:
+- apiGroups:
+  - batch.io
+  resources:
+  - cronjobs
+  verbs:
+  - create
+  - get
+  - watch
+- apiGroups:
+  - batch.io
+  resources:
+  - cronjobs/status
+  verbs:
+  - get
+  - patch
+  - update
+- apiGroups:
+  - art
+  resources:
+  - jobs
+  verbs:
+  - get
+- apiGroups:
+  - batch
+  resources:
+  - jobs/status
+  verbs:
+  - watch
+- apiGroups:
+  - batch
+  - cron
+  resources:
+  - jobs/status
+  verbs:
+  - create
+  - get