diff --git a/pkg/rbac/parser.go b/pkg/rbac/parser.go index 97128603d..5603a8315 100644 --- a/pkg/rbac/parser.go +++ b/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,7 +133,8 @@ 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) @@ -129,6 +142,7 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error { 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 } diff --git a/pkg/rbac/parser_integration_test.go b/pkg/rbac/parser_integration_test.go new file mode 100644 index 000000000..240bbb526 --- /dev/null +++ b/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)) + }) + } +}) diff --git a/pkg/rbac/rbac_suite_test.go b/pkg/rbac/rbac_suite_test.go new file mode 100644 index 000000000..a355b2f54 --- /dev/null +++ b/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") +} diff --git a/pkg/rbac/testdata/README.md b/pkg/rbac/testdata/README.md new file mode 100644 index 000000000..3e0bfdeec --- /dev/null +++ b/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! diff --git a/pkg/rbac/testdata/controller.go b/pkg/rbac/testdata/controller.go new file mode 100644 index 000000000..98ed3d5d9 --- /dev/null +++ b/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 diff --git a/pkg/rbac/testdata/role.yaml b/pkg/rbac/testdata/role.yaml new file mode 100644 index 000000000..01fc94f1e --- /dev/null +++ b/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