generate: consider service accounts when generating a CSV (#3610)

This commit adds handling for extra RBAC objects present in `generate <bundle|packagemanifests>`
input. These objects will be written to the resulting bundle. For now, only Roles, RoleBindings,
their Cluster equivalents, and ServiceAccounts are written.

This PR also correctly names service account for (cluster) role permissions. These are currently
incorrect because the CSV generator is naively using (cluster) role names instead of actual service
account names. Previously this was ok because the names match the service account, but this is no
longer the case. See #3600.

Old test data has been removed, and a static `basic.operator.yaml` containing the output of
`kustomize build config/manifests` added; the static file's contents match a current project
manifest build.

internal/cmd/operator-sdk/generate: write RBAC objects to stdout or files named with object.Name +
GVK, and rename `--update-crds` to `--update-objects`

internal/generate/{collector/clusterserviceversion}: consider (cluster) role bindings so CSV
generator can assign the correct service account names to roles

Author: Eric Stroczynski

changelog/fragments/fix-csv-role-names.yaml

@@ -0,0 +1,19 @@
+entries:
+  - description: >
+      `generate <bundle|packagemanifests>` will write RBAC objects (Roles, RoleBindings, their Cluster equivalents,
+      and ServiceAccounts) not bound to CSV deployment service accounts
+      to the resulting manifests directory.
+    kind: addition
+  - description: >
+      The `--update-crds` flag has been renamed to `--update-objects` for the `generate packagemanifests` subcommand.
+    kind: change
+    breaking: true
+    migration:
+      header: Rename `--update-crds` flag to `--update-objects` in `generate packagemanifests` invocations
+      body: >
+        This flag has been renamed to account for all objects that can be written to the package directory,
+        ex. Roles.
+  - description: >
+      Fixed incorrect (cluster) role name assignments in generated CSVs
+      [#3600](https://github.com/operator-framework/operator-sdk/issues/3600).
+    kind: bugfix

internal/cmd/operator-sdk/generate/bundle/bundle.go

@@ -197,13 +197,7 @@ func (c bundleCmd) runManifests(cfg *config.Config) (err error) {
 		return fmt.Errorf("error generating ClusterServiceVersion: %v", err)
 	}
 
-	var objs []interface{}
-	for _, crd := range col.V1CustomResourceDefinitions {
-		objs = append(objs, crd)
-	}
-	for _, crd := range col.V1beta1CustomResourceDefinitions {
-		objs = append(objs, crd)
-	}
+	objs := genutil.GetManifestObjects(col)
 	if c.stdout {
 		if err := genutil.WriteObjects(stdout, objs...); err != nil {
 			return err

internal/cmd/operator-sdk/generate/internal/genutil.go

@@ -23,11 +23,11 @@ import (
 	"path/filepath"
 	"strings"
 
-	"k8s.io/apimachinery/pkg/util/validation"
-
 	"github.com/blang/semver"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/util/validation"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/kubebuilder/pkg/model/config"
 	"sigs.k8s.io/yaml"
 )
@@ -57,7 +57,7 @@ func IsPipeReader() bool {
 }
 
 // WriteObjects writes each object in objs to w.
-func WriteObjects(w io.Writer, objs ...interface{}) error {
+func WriteObjects(w io.Writer, objs ...controllerutil.Object) error {
 	for _, obj := range objs {
 		if err := writeObject(w, obj); err != nil {
 			return err
@@ -67,7 +67,7 @@ func WriteObjects(w io.Writer, objs ...interface{}) error {
 }
 
 // WriteObjectsToFiles creates dir then writes each object in objs to a file in dir.
-func WriteObjectsToFiles(dir string, objs ...interface{}) error {
+func WriteObjectsToFiles(dir string, objs ...controllerutil.Object) error {
 	if err := os.MkdirAll(dir, 0755); err != nil {
 		return err
 	}
@@ -76,12 +76,12 @@ func WriteObjectsToFiles(dir string, objs ...interface{}) error {
 	for _, obj := range objs {
 		var fileName string
 		switch t := obj.(type) {
-		case apiextv1.CustomResourceDefinition:
+		case *apiextv1.CustomResourceDefinition:
 			fileName = makeCRDFileName(t.Spec.Group, t.Spec.Names.Plural)
-		case apiextv1beta1.CustomResourceDefinition:
+		case *apiextv1beta1.CustomResourceDefinition:
 			fileName = makeCRDFileName(t.Spec.Group, t.Spec.Names.Plural)
 		default:
-			return fmt.Errorf("unknown object type: %T", t)
+			fileName = makeObjectFileName(t)
 		}
 
 		if _, hasFile := seenFiles[fileName]; hasFile {
@@ -99,6 +99,14 @@ func makeCRDFileName(group, resource string) string {
 	return fmt.Sprintf("%s_%s.yaml", group, resource)
 }
 
+func makeObjectFileName(obj controllerutil.Object) string {
+	gvk := obj.GetObjectKind().GroupVersionKind()
+	if gvk.Group == "" {
+		return fmt.Sprintf("%s_%s_%s.yaml", obj.GetName(), gvk.Version, strings.ToLower(gvk.Kind))
+	}
+	return fmt.Sprintf("%s_%s_%s_%s.yaml", obj.GetName(), gvk.Group, gvk.Version, strings.ToLower(gvk.Kind))
+}
+
 // writeObjectToFile marshals crd to bytes and writes them to dir in file.
 func writeObjectToFile(dir string, obj interface{}, fileName string) error {
 	f, err := os.Create(filepath.Join(dir, fileName))

internal/cmd/operator-sdk/generate/internal/manifests.go

@@ -0,0 +1,45 @@
+// Copyright 2020 The Operator-SDK 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 genutil
+
+import (
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	"github.com/operator-framework/operator-sdk/internal/generate/collector"
+)
+
+// GetManifestObjects returns all objects to be written to a manifests directory from collector.Manifests.
+func GetManifestObjects(c *collector.Manifests) (objs []controllerutil.Object) {
+	// All CRDs passed in should be written.
+	for i := range c.V1CustomResourceDefinitions {
+		objs = append(objs, &c.V1CustomResourceDefinitions[i])
+	}
+	for i := range c.V1beta1CustomResourceDefinitions {
+		objs = append(objs, &c.V1beta1CustomResourceDefinitions[i])
+	}
+
+	// All ServiceAccounts passed in should be written.
+	for i := range c.ServiceAccounts {
+		objs = append(objs, &c.ServiceAccounts[i])
+	}
+
+	// RBAC objects that are not a part of the CSV should be written.
+	_, roleObjs := c.SplitCSVPermissionsObjects()
+	objs = append(objs, roleObjs...)
+	_, clusterRoleObjs := c.SplitCSVClusterPermissionsObjects()
+	objs = append(objs, clusterRoleObjs...)
+
+	return objs
+}

internal/cmd/operator-sdk/generate/packagemanifests/cmd.go

@@ -28,17 +28,17 @@ import (
 //nolint:maligned
 type packagemanifestsCmd struct {
 	// Common options.
-	projectName  string
-	version      string
-	fromVersion  string
-	inputDir     string
-	outputDir    string
-	kustomizeDir string
-	deployDir    string
-	crdsDir      string
-	updateCRDs   bool
-	stdout       bool
-	quiet        bool
+	projectName   string
+	version       string
+	fromVersion   string
+	inputDir      string
+	outputDir     string
+	kustomizeDir  string
+	deployDir     string
+	crdsDir       string
+	updateObjects bool
+	stdout        bool
+	quiet         bool
 
 	// Package manifest options.
 	channelName      string
@@ -97,7 +97,8 @@ func (c *packagemanifestsCmd) addFlagsTo(fs *pflag.FlagSet) {
 	fs.StringVar(&c.channelName, "channel", "", "Channel name for the generated package")
 	fs.BoolVar(&c.isDefaultChannel, "default-channel", false, "Use the channel passed to --channel "+
 		"as the package manifest file's default channel")
-	fs.BoolVar(&c.updateCRDs, "update-crds", true, "Update CustomResoureDefinition manifests in this package")
+	fs.BoolVar(&c.updateObjects, "update-objects", true, "Update non-CSV objects in this package, "+
+		"ex. CustomResoureDefinitions, Roles")
 	fs.BoolVarP(&c.quiet, "quiet", "q", false, "Run in quiet mode")
 	fs.BoolVar(&c.stdout, "stdout", false, "Write package to stdout")
 }

internal/cmd/operator-sdk/generate/packagemanifests/packagemanifests.go

@@ -188,14 +188,8 @@ func (c packagemanifestsCmd) run(cfg *config.Config) error {
 		return fmt.Errorf("error generating ClusterServiceVersion: %v", err)
 	}
 
-	if c.updateCRDs {
-		var objs []interface{}
-		for _, crd := range col.V1CustomResourceDefinitions {
-			objs = append(objs, crd)
-		}
-		for _, crd := range col.V1beta1CustomResourceDefinitions {
-			objs = append(objs, crd)
-		}
+	if c.updateObjects {
+		objs := genutil.GetManifestObjects(col)
 		if c.stdout {
 			if err := genutil.WriteObjects(stdout, objs...); err != nil {
 				return err

internal/generate/clusterserviceversion/clusterserviceversion_test.go

@@ -16,7 +16,6 @@ package clusterserviceversion
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -31,7 +30,6 @@ import (
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-registry/pkg/lib/bundle"
 	appsv1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/kubebuilder/pkg/model/config"
 	"sigs.k8s.io/yaml"
 
@@ -45,16 +43,12 @@ var (
 	testDataDir           = filepath.Join("..", "testdata")
 	csvDir                = filepath.Join(testDataDir, "clusterserviceversions")
 	csvBasesDir           = filepath.Join(csvDir, "bases")
-	csvNewLayoutBundleDir = filepath.Join(csvDir, "newlayout", "manifests")
-
-	// TODO: create a new testdata dir (top level?) that has both a "config"
-	// dir and a "deploy" dir that contains `kustomize build config/default`
-	// output to simulate actual manifest collection behavior. Using "config"
-	// directly is not standard behavior.
-	goTestDataDir = filepath.Join(testDataDir, "non-standard-layout")
-	goAPIsDir     = filepath.Join(goTestDataDir, "api")
-	goConfigDir   = filepath.Join(goTestDataDir, "config")
-	goCRDsDir     = filepath.Join(goConfigDir, "crds")
+	csvNewLayoutBundleDir = filepath.Join(csvDir, "output")
+
+	goTestDataDir       = filepath.Join(testDataDir, "go")
+	goAPIsDir           = filepath.Join(goTestDataDir, "api")
+	goStaticDir         = filepath.Join(goTestDataDir, "static")
+	goBasicOperatorPath = filepath.Join(goStaticDir, "basic.operator.yaml")
 )
 
 var (
@@ -74,7 +68,7 @@ var (
 
 var _ = BeforeSuite(func() {
 	col = &collector.Manifests{}
-	Expect(col.UpdateFromDirs(goConfigDir, goCRDsDir)).ToNot(HaveOccurred())
+	collectManifestsFromFileHelper(col, goBasicOperatorPath)
 
 	cfg = readConfigHelper(goTestDataDir)
 
@@ -97,7 +91,7 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 		buf = &bytes.Buffer{}
 	})
 
-	Describe("for the new Go project layout", func() {
+	Describe("for a Go project", func() {
 
 		Context("with correct Options", func() {
 
@@ -281,7 +275,7 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 					getBase:      makeBaseGetter(newCSV),
 				}
 				// Update the input's and expected CSV's Deployment image.
-				Expect(g.Collector.UpdateFromDirs(goConfigDir, goCRDsDir)).ToNot(HaveOccurred())
+				collectManifestsFromFileHelper(g.Collector, goBasicOperatorPath)
 				Expect(len(g.Collector.Deployments)).To(BeNumerically(">=", 1))
 				imageTag := "controller:v" + g.Version
 				modifyDepImageHelper(&g.Collector.Deployments[0].Spec, imageTag)
@@ -311,42 +305,6 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 				Expect(csv).To(Equal(upgradeCSV(newCSV, g.OperatorName, g.Version)))
 			})
 		})
-
-		Context("generate ClusterServiceVersion", func() {
-			It("should handle CRDs with core type name", func() {
-				g = Generator{
-					OperatorName: operatorName,
-					OperatorType: operatorType,
-					Version:      version,
-					Collector:    &collector.Manifests{},
-					config:       cfg,
-					getBase:      makeBaseGetter(newCSV),
-				}
-				err := filepath.Walk(goConfigDir, func(path string, info os.FileInfo, err error) error {
-					if err != nil || info.IsDir() {
-						return err
-					}
-					file, err := os.OpenFile(path, os.O_RDONLY, 0)
-					if err != nil {
-						return err
-					}
-					defer file.Close()
-					return g.Collector.UpdateFromReader(file)
-				})
-				Expect(err).ShouldNot(HaveOccurred(), "failed to read manifests")
-				Expect(len(g.Collector.V1beta1CustomResourceDefinitions)).Should(BeEquivalentTo(2))
-				Expect(len(g.Collector.CustomResources)).Should(BeEquivalentTo(2))
-
-				csv, err := g.generate()
-				Expect(err).ToNot(HaveOccurred())
-				Expect(csv.Annotations["alm-examples"]).ShouldNot(BeEquivalentTo("[]"))
-
-				crs := []unstructured.Unstructured{}
-				err = json.Unmarshal([]byte(csv.Annotations["alm-examples"]), &crs)
-				Expect(err).ShouldNot(HaveOccurred(), "failed to parse 'alm-examples' annotations")
-				Expect(crs).Should(ConsistOf(g.Collector.CustomResources), "custom resources shall match with CSV annotations")
-			})
-		})
 	})
 
 })
@@ -388,6 +346,13 @@ var _ = Describe("Generation requires interaction", func() {
 	})
 })
 
+func collectManifestsFromFileHelper(col *collector.Manifests, path string) {
+	f, err := os.Open(path)
+	ExpectWithOffset(1, err).ToNot(HaveOccurred())
+	ExpectWithOffset(1, col.UpdateFromReader(f)).ToNot(HaveOccurred())
+	ExpectWithOffset(1, f.Close()).Should(Succeed())
+}
+
 func readConfigHelper(dir string) *config.Config {
 	wd, err := os.Getwd()
 	ExpectWithOffset(1, err).ToNot(HaveOccurred())