Avoid reordering Rules

pkg/rbac/parser.go

@@ -23,6 +23,7 @@ limitations under the License.
 package rbac
 
 import (
+	"fmt"
 	"sort"
 	"strings"
 
@@ -54,6 +55,17 @@ type ruleKey struct {
 	URLs      string
 }
 
+func (key ruleKey) String() string {
+	return fmt.Sprintf("%s + %s + %s", key.Groups, key.Resources, key.URLs)
+}
+
+// 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()
@@ -121,14 +133,16 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
 	return into.Register(RuleDefinition)
 }
 
-func (g Generator) Generate(ctx *genall.GenerationContext) error {
+// GenerateClusterRole generates a rbacv1.ClusterRole object
+func GenerateClusterRole(ctx *genall.GenerationContext, roleName string) (*rbacv1.ClusterRole, error) {
 	rules := make(map[ruleKey]*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
 		for _, markerValue := range markerSet[RuleDefinition.Name] {
 			rule := markerValue.(Rule)
 			key := rule.key()
@@ -140,26 +154,46 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 		}
 	}
 
-	var policyRules []rbacv1.PolicyRule
-	for _, rule := range rules {
-		policyRules = append(policyRules, rule.ToRule())
+	if len(rules) == 0 {
+		return nil, nil
+	}
 
+	// 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, rules[key].ToRule())
 
-	if len(policyRules) == 0 {
-		return 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,61 @@
+package rbac_test
+
+import (
+	"io/ioutil"
+	"os"
+
+	"github.com/google/go-cmp/cmp"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	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"
+	"sigs.k8s.io/yaml"
+)
+
+var _ = Describe("ClusterRole generated by the RBAC Generator", func() {
+	// run this test multiple times to make sure the Rule order is stable.
+	const stableTestCount = 5
+	for i := 0; i < stableTestCount; i++ {
+		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:
+  - art
+  resources:
+  - jobs
+  verbs:
+  - get
+- apiGroups:
+  - batch
+  resources:
+  - jobs/status
+  verbs:
+  - watch
+- apiGroups:
+  - batch
+  - cron
+  resources:
+  - jobs/status
+  verbs:
+  - create
+  - get
+- apiGroups:
+  - batch.io
+  resources:
+  - cronjobs
+  verbs:
+  - create
+  - get
+  - watch
+- apiGroups:
+  - batch.io
+  resources:
+  - cronjobs/status
+  verbs:
+  - get
+  - patch
+  - update