Merge pull request #182 from droot/verify-rbac-annotations

Verify rbac annotations

Author: Phillip Wittrock

cmd/internal/codegen/parse/parser.go

@@ -17,15 +17,17 @@ limitations under the License.
 package parse
 
 import (
+	"bufio"
+	"fmt"
 	"go/build"
+	"log"
+	"os"
 	"path/filepath"
 	"strings"
-	"os"
-	"bufio"
 
 	"github.com/golang/glog"
-
 	"github.com/kubernetes-sigs/kubebuilder/cmd/internal/codegen"
+	"github.com/markbates/inflect"
 	"github.com/pkg/errors"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -68,11 +70,90 @@ func NewAPIs(context *generator.Context, arguments *args.GeneratorArgs) *APIs {
 	b.parseControllers()
 	b.parseRBAC()
 	b.parseInformers()
+	b.verifyRBACAnnotations()
 	b.parseAPIs()
 	b.parseCRDs()
 	return b
 }
 
+// verifyRBACAnnotations verifies that there are corresponding RBAC annotations for
+// each informer annotation.
+// e.g. if there is an // +kubebuilder:informer annotation for Pods, then there
+// should also be a // +kubebuilder:rbac annotation for Pods
+func (b *APIs) verifyRBACAnnotations() {
+	parseOption := b.arguments.CustomArgs.(*ParseOptions)
+	if parseOption.SkipRBACValidation {
+		log.Println("skipping RBAC validations")
+		return
+	}
+	err := rbacMatchesInformers(b.Informers, b.Rules)
+	if err != nil {
+		log.Fatal(err)
+	}
+}
+
+func rbacMatchesInformers(informers map[v1.GroupVersionKind]bool, rbacRules []rbacv1.PolicyRule) error {
+	rs := inflect.NewDefaultRuleset()
+
+	// For each informer, look for the RBAC annotation
+	for gvk := range informers {
+		found := false
+
+		// Search all RBAC rules for one that matches the informer group and resource
+		for _, rule := range rbacRules {
+
+			// Check if the group matches the informer group
+			groupFound := false
+			for _, g := range rule.APIGroups {
+				// RBAC has the full group with domain, whereas informers do not.  Strip the domain
+				// from the group before comparing.
+				parts := strings.Split(g, ".")
+				group := parts[len(parts)-1]
+
+				// If the RBAC group is wildcard or matches, it is a match
+				if g == "*" || group == gvk.Group {
+					groupFound = true
+					break
+				}
+				// Edge case where "core" and "" are equivalent
+				if (group == "core" || group == "") && (gvk.Group == "core" || gvk.Group == "") {
+					groupFound = true
+					break
+				}
+			}
+			if !groupFound {
+				continue
+			}
+
+			// The resource name is the lower-plural of the Kind
+			resource := rs.Pluralize(strings.ToLower(gvk.Kind))
+			// Check if the resource matches the informer resource
+			resourceFound := false
+			for _, k := range rule.Resources {
+				// If the RBAC resource is a wildcard or matches the informer resource, it is a match
+				if k == "*" || k == resource {
+					resourceFound = true
+					break
+				}
+			}
+			if !resourceFound {
+				continue
+			}
+
+			// Found a matching RBAC rule
+			found = true
+			break
+		}
+		if !found {
+			return fmt.Errorf("Missing rbac rule for %s.%s.  Add with // +kubebuilder:rbac:groups=%s,"+
+				"resources=%s,verbs=get;list;watch comment on controller struct "+
+				"or run the command with '--skip-rbac-validation' arg", gvk.Group, gvk.Kind, gvk.Group,
+				inflect.NewDefaultRuleset().Pluralize(strings.ToLower(gvk.Kind)))
+		}
+	}
+	return nil
+}
+
 // parseGroupNames initializes b.GroupNames with the set of all groups
 func (b *APIs) parseGroupNames() {
 	b.GroupNames = sets.String{}
@@ -129,10 +210,10 @@ func (b *APIs) parseDomain() {
 
 func parseDomainFromFiles(paths []string) string {
 	var domain string
-    for _, path := range paths {
-    	if strings.HasSuffix(path, "pkg/apis") {
-    		filePath := strings.Join([]string{build.Default.GOPATH, "src", path, "doc.go"}, "/")
-            lines := []string{}
+	for _, path := range paths {
+		if strings.HasSuffix(path, "pkg/apis") {
+			filePath := strings.Join([]string{build.Default.GOPATH, "src", path, "doc.go"}, "/")
+			lines := []string{}
 
 			file, err := os.Open(filePath)
 			if err != nil {

cmd/internal/codegen/parse/parser_test.go

@@ -0,0 +1,96 @@
+/*
+Copyright 2018 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 parse
+
+import (
+	"testing"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestrbacMatchesInformers(t *testing.T) {
+	tests := []struct {
+		informers map[v1.GroupVersionKind]bool
+		rbacRules []rbacv1.PolicyRule
+		expErr    bool
+	}{
+		{
+			// informer resource matches the RBAC rule
+			informers: map[v1.GroupVersionKind]bool{
+				v1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}: true,
+			},
+			rbacRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{"apps"}, Resources: []string{"deployments"}},
+			},
+			expErr: false,
+		},
+		{
+			// RBAC rule does not match the informer resource because of missing pluralization in RBAC rules
+			informers: map[v1.GroupVersionKind]bool{
+				v1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}: true,
+			},
+			rbacRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{"apps"}, Resources: []string{"Deployment"}},
+			},
+			expErr: true,
+		},
+		{
+			// wild-card RBAC rule should match any resource in the group
+			informers: map[v1.GroupVersionKind]bool{
+				v1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "StatefulSet"}: true,
+				v1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}:  true,
+			},
+			rbacRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{"apps"}, Resources: []string{"*"}},
+			},
+			expErr: false,
+		},
+		{
+			// empty group name is normalized to "core"
+			informers: map[v1.GroupVersionKind]bool{
+				v1.GroupVersionKind{Group: "core", Version: "v1", Kind: "Pod"}: true,
+			},
+			rbacRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{""}, Resources: []string{"pods"}},
+			},
+			expErr: false,
+		},
+		{
+			// empty group name is normalized to "core"
+			informers: map[v1.GroupVersionKind]bool{
+				v1.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}: true,
+			},
+			rbacRules: []rbacv1.PolicyRule{
+				{APIGroups: []string{"core"}, Resources: []string{"pods"}},
+			},
+			expErr: false,
+		},
+	}
+
+	for _, test := range tests {
+		err := checkRBACMatchesInformers(test.informers, test.rbacRules)
+		if test.expErr {
+			if err == nil {
+				t.Errorf("RBAC rules %+v shouldn't match with informers %+v", test.rbacRules, test.informers)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("RBAC rules %+v should match informers %+v, but got a mismatch error: %v", test.rbacRules, test.informers, err)
+			}
+		}
+	}
+}

cmd/internal/codegen/parse/rbac.go

@@ -64,7 +64,15 @@ func parseRBACTag(tag string) rbacv1.PolicyRule {
 		values = strings.Split(value, ";")
 		switch kv[0] {
 		case "groups":
-			result.APIGroups = values
+			normalized := []string{}
+			for _, v := range values {
+				if v == "core" {
+					normalized = append(normalized, "")
+				} else {
+					normalized = append(normalized, v)
+				}
+			}
+			result.APIGroups = normalized
 		case "resources":
 			result.Resources = values
 		case "verbs":
@@ -112,6 +120,9 @@ func parseInformerTag(tag string) v1.GroupVersionKind {
 		value := kv[1]
 		switch kv[0] {
 		case "group":
+			if value == "" {
+				value = "core"
+			}
 			result.Group = value
 		case "version":
 			result.Version = value

cmd/internal/codegen/parse/util.go

@@ -27,7 +27,11 @@ import (
 )
 
 type ParseOptions struct {
-    SkipMapValidation bool
+	SkipMapValidation bool
+
+	// SkipRBACValidation flag determines whether to check RBAC annotations
+	// for the controller or not at parse stage.
+	SkipRBACValidation bool
 }
 
 // IsAPIResource returns true if t has a +resource/+kubebuilder:resource comment tag

cmd/kubebuilder-gen/codegen/run/generator.go

@@ -20,9 +20,9 @@ import (
 	"github.com/golang/glog"
 	"github.com/kubernetes-sigs/kubebuilder/cmd/internal/codegen/parse"
 	"github.com/kubernetes-sigs/kubebuilder/cmd/kubebuilder-gen/codegen"
+	"github.com/spf13/pflag"
 	"k8s.io/gengo/args"
 	"k8s.io/gengo/generator"
-	"github.com/spf13/pflag"
 )
 
 // CodeGenerator generates code for Kubernetes resources and controllers
@@ -54,6 +54,8 @@ func (g *CodeGenerator) Execute() error {
 	customArgs := &parse.ParseOptions{}
 	pflag.CommandLine.BoolVar(&customArgs.SkipMapValidation, "skip-map-validation", true,
 		"if set to true, skip generating validation schema for map type in CRD.")
+	pflag.CommandLine.BoolVar(&customArgs.SkipRBACValidation, "skip-rbac-validation", false,
+		"if set to true, skip validation for RBAC annotations for the controller.")
 	arguments.CustomArgs = customArgs
 
 	arguments.OutputFileBaseName = g.OutputFileBaseName

cmd/kubebuilder/create/controller/controller.go

@@ -79,6 +79,7 @@ func (bc *{{.Kind}}Controller) Reconcile(k types.ReconcileKey) error {
     return nil
 }
 {{if .CoreType}}// +kubebuilder:informers:group={{ .Group }},version={{ .Version }},kind={{ .Kind }}{{end}}
+{{if .CoreType}}// +kubebuilder:rbac:groups={{ .Group }},resources={{ .Resource }},verbs=get;watch;list{{end}}
 // +kubebuilder:controller:group={{ .Group }},version={{ .Version }},kind={{ .Kind}},resource={{ .Resource }}
 type {{.Kind}}Controller struct {
     // INSERT ADDITIONAL FIELDS HERE

cmd/kubebuilder/create/controller/run.go

@@ -21,18 +21,20 @@ import (
 	"log"
 	"os"
 
+	"strings"
+
 	createutil "github.com/kubernetes-sigs/kubebuilder/cmd/kubebuilder/create/util"
 	generatecmd "github.com/kubernetes-sigs/kubebuilder/cmd/kubebuilder/generate"
 	"github.com/kubernetes-sigs/kubebuilder/cmd/kubebuilder/util"
 	"github.com/markbates/inflect"
 	"github.com/spf13/cobra"
-	"strings"
 )
 
 type ControllerArguments struct {
-	nonNamespacedKind bool
-	generate          bool
-	CoreType          bool
+	nonNamespacedKind  bool
+	generate           bool
+	CoreType           bool
+	SkipRBACValidation bool
 }
 
 func AddCreateController(cmd *cobra.Command) {
@@ -59,6 +61,7 @@ kubebuilder create controller --group apps --version v1beta2 --kind Deployment -
 	createControllerCmd.Flags().BoolVar(&c.nonNamespacedKind, "non-namespaced", false, "if set, the API kind will be non namespaced")
 	createControllerCmd.Flags().BoolVar(&c.generate, "generate", true, "generate controller code")
 	createControllerCmd.Flags().BoolVar(&c.CoreType, "core-type", false, "generate controller for core type")
+	createControllerCmd.Flags().BoolVar(&c.SkipRBACValidation, "skip-rbac-validation", false, "if set to true, skip validation for RBAC annotations for the controller.")
 	cmd.AddCommand(createControllerCmd)
 }
 
@@ -77,6 +80,9 @@ func (c *ControllerArguments) RunCreateController(cmd *cobra.Command, args []str
 	if c.generate {
 		fmt.Printf("Generating code for new controller... " +
 			"Regenerate after editing controller files by running `kubebuilder generate clean; kubebuilder generate`.\n")
+		if c.SkipRBACValidation {
+			args = append(args, "--skip-rbac-validation")
+		}
 		generatecmd.RunGenerate(cmd, args)
 	}
 	fmt.Printf("Next: Run the controller and create an instance with:\n" +

cmd/kubebuilder/create/resource/run.go

@@ -30,9 +30,12 @@ import (
 	"github.com/spf13/cobra"
 )
 
-var nonNamespacedKind bool
-var controller bool
-var generate bool
+var (
+	nonNamespacedKind  bool
+	controller         bool
+	generate           bool
+	skipRBACValidation bool
+)
 
 var createResourceCmd = &cobra.Command{
 	Use:   "resource",
@@ -63,6 +66,7 @@ func AddCreateResource(cmd *cobra.Command) {
 	createResourceCmd.Flags().BoolVar(&controller, "controller", true, "if true, generate the controller code for the resource")
 	createResourceCmd.Flags().BoolVar(&generate, "generate", true, "generate source code")
 	createResourceCmd.Flags().BoolVar(&createutil.AllowPluralKind, "plural-kind", false, "allow the kind to be plural")
+	createResourceCmd.Flags().BoolVar(&skipRBACValidation, "skip-rbac-validation", false, "if set to true, skip validation for RBAC annotations")
 	cmd.AddCommand(createResourceCmd)
 }
 
@@ -83,6 +87,9 @@ func RunCreateResource(cmd *cobra.Command, args []string) {
 	if generate {
 		fmt.Printf("Generating code for new resource...  " +
 			"Regenerate after editing resources files by running `kubebuilder build generated`.\n")
+		if skipRBACValidation {
+			args = append(args, "--skip-rbac-validation")
+		}
 		generatecmd.RunGenerate(cmd, args)
 	}
 	fmt.Printf("Next: Install the API, run the controller and create an instance with:\n" +
@@ -115,7 +122,10 @@ func createResource(boilerplate string) {
 
 	if controller {
 		fmt.Printf("Creating controller ...\n")
-		c := controllerct.ControllerArguments{CoreType: false}
+		c := controllerct.ControllerArguments{
+			CoreType:           false,
+			SkipRBACValidation: skipRBACValidation,
+		}
 		c.CreateController(boilerplate)
 	}