internal/generate/{collector/clusterserviceversion}: consider

(cluster) role bindings so CSV generator can assign the correct
service account names to roles

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

@@ -0,0 +1,5 @@
+entries:
+  - description: >
+      Fixed incorrect (cluster) role name assignments in generated CSVs
+      [#3600](https://github.com/operator-framework/operator-sdk/issues/3600).
+    kind: bugfix

internal/generate/clusterserviceversion/clusterserviceversion_test.go

@@ -16,7 +16,6 @@ package clusterserviceversion
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -32,7 +31,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"
 
@@ -46,16 +44,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.operatory.yaml")
 )
 
 var (
@@ -75,7 +69,7 @@ var (
 
 var _ = BeforeSuite(func() {
 	col = &collector.Manifests{}
-	Expect(col.UpdateFromDirs(goConfigDir, goCRDsDir)).ToNot(HaveOccurred())
+	collectManifestsFromFileHelper(col, goBasicOperatorPath)
 
 	cfg = readConfigHelper(goTestDataDir)
 
@@ -98,7 +92,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() {
 
@@ -282,7 +276,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)
@@ -312,42 +306,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")
-			})
-		})
 	})
 
 })
@@ -389,6 +347,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())

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

@@ -28,6 +28,7 @@ import (
 	admissionregv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/version"
 
 	"github.com/operator-framework/operator-sdk/internal/generate/collector"
@@ -39,7 +40,7 @@ import (
 func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion) error {
 	// Apply manifests to the CSV object.
 	if err := apply(c, csv); err != nil {
-		return fmt.Errorf("error updating ClusterServiceVersion: %v", err)
+		return err
 	}
 
 	// Set fields required by namespaced operators. This is a no-op for cluster-
@@ -65,7 +66,7 @@ func apply(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion)
 
 	applyCustomResourceDefinitions(c, csv)
 	if err := applyCustomResources(c, csv); err != nil {
-		return fmt.Errorf("error applying Custom Resource: %v", err)
+		return fmt.Errorf("error applying Custom Resource examples to CSV %s: %v", csv.GetName(), err)
 	}
 	applyWebhooks(c, csv)
 	return nil
@@ -80,29 +81,77 @@ func getCSVInstallStrategy(csv *operatorsv1alpha1.ClusterServiceVersion) operato
 	return csv.Spec.InstallStrategy
 }
 
-// applyRoles updates strategy's permissions with the Roles in the collector.
-func applyRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) {
+// applyRoles applies Roles to strategy's permissions field while respecting ServiceAccount names in RoleBindings.
+func applyRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) { //nolint:dupl
+	// Collect all role names by their corresponding service accounts via bindings. This lets us
+	// look up all service accounts a role is bound to and create one set of permissions per service account.
+	saNamesToRoleNames := make(map[string]map[string]struct{})
+	for _, binding := range c.RoleBindings {
+		roleRef := binding.RoleRef
+		if roleRef.Kind == "Role" && (roleRef.APIGroup == "" || roleRef.APIGroup == rbacv1.SchemeGroupVersion.Group) {
+			for _, name := range getSubjectServiceAccountNames(binding.Subjects) {
+				if saNamesToRoleNames[name] == nil {
+					saNamesToRoleNames[name] = make(map[string]struct{})
+				}
+				saNamesToRoleNames[name][roleRef.Name] = struct{}{}
+			}
+		}
+	}
+
+	// Apply relevant roles to each service account.
 	perms := []operatorsv1alpha1.StrategyDeploymentPermissions{}
-	for _, role := range c.Roles {
-		perms = append(perms, operatorsv1alpha1.StrategyDeploymentPermissions{
-			ServiceAccountName: role.GetName(),
-			Rules:              role.Rules,
-		})
+	for saName, roleNames := range saNamesToRoleNames {
+		p := operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: saName}
+		for _, role := range c.Roles {
+			if _, ok := roleNames[role.GetName()]; ok {
+				p.Rules = append(p.Rules, role.Rules...)
+			}
+		}
+		perms = append(perms, p)
 	}
 	strategy.Permissions = perms
 }
 
-// applyClusterRoles updates strategy's cluserPermissions with the ClusterRoles
-// in the collector.
-func applyClusterRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) {
-	perms := []operatorsv1alpha1.StrategyDeploymentPermissions{}
-	for _, role := range c.ClusterRoles {
-		perms = append(perms, operatorsv1alpha1.StrategyDeploymentPermissions{
-			ServiceAccountName: role.GetName(),
-			Rules:              role.Rules,
-		})
+// applyClusterRoles applies ClusterRoles to strategy's clusterPermissions field while respecting
+// ServiceAccount names in ClusterRoleBindings.
+func applyClusterRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) { //nolint:dupl
+	// Collect all cluster role names by their corresponding service accounts via bindings. This lets us look up
+	// all service accounts a cluster role is bound to and create one set of clusterPermissions per service account.
+	saNamesToClusterRoleNames := make(map[string]map[string]struct{})
+	for _, binding := range c.ClusterRoleBindings {
+		roleRef := binding.RoleRef
+		if roleRef.Kind == "ClusterRole" && (roleRef.APIGroup == "" || roleRef.APIGroup == rbacv1.SchemeGroupVersion.Group) {
+			for _, name := range getSubjectServiceAccountNames(binding.Subjects) {
+				if saNamesToClusterRoleNames[name] == nil {
+					saNamesToClusterRoleNames[name] = make(map[string]struct{})
+				}
+				saNamesToClusterRoleNames[name][roleRef.Name] = struct{}{}
+			}
+		}
+	}
+
+	// Apply relevant cluster roles to each service account.
+	clusterPerms := []operatorsv1alpha1.StrategyDeploymentPermissions{}
+	for saName, clusterRoleNames := range saNamesToClusterRoleNames {
+		p := operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: saName}
+		for _, clusterRole := range c.ClusterRoles {
+			if _, ok := clusterRoleNames[clusterRole.GetName()]; ok {
+				p.Rules = append(p.Rules, clusterRole.Rules...)
+			}
+		}
+		clusterPerms = append(clusterPerms, p)
+	}
+	strategy.ClusterPermissions = clusterPerms
+}
+
+// getSubjectServiceAccountNames returns a list of all ServiceAccount subject names.
+func getSubjectServiceAccountNames(subjects []rbacv1.Subject) (saNames []string) {
+	for _, subject := range subjects {
+		if subject.Kind == "ServiceAccount" {
+			saNames = append(saNames, subject.Name)
+		}
 	}
-	strategy.ClusterPermissions = perms
+	return saNames
 }
 
 // applyDeployments updates strategy's deployments with the Deployments

internal/generate/collector/collect.go

@@ -40,6 +40,8 @@ import (
 type Manifests struct {
 	Roles                            []rbacv1.Role
 	ClusterRoles                     []rbacv1.ClusterRole
+	RoleBindings                     []rbacv1.RoleBinding
+	ClusterRoleBindings              []rbacv1.ClusterRoleBinding
 	Deployments                      []appsv1.Deployment
 	V1CustomResourceDefinitions      []apiextv1.CustomResourceDefinition
 	V1beta1CustomResourceDefinitions []apiextv1beta1.CustomResourceDefinition
@@ -54,6 +56,8 @@ type Manifests struct {
 var (
 	roleGK                 = rbacv1.SchemeGroupVersion.WithKind("Role").GroupKind()
 	clusterRoleGK          = rbacv1.SchemeGroupVersion.WithKind("ClusterRole").GroupKind()
+	roleBindingGK          = rbacv1.SchemeGroupVersion.WithKind("RoleBinding").GroupKind()
+	clusterRoleBindingGK   = rbacv1.SchemeGroupVersion.WithKind("ClusterRoleBinding").GroupKind()
 	deploymentGK           = appsv1.SchemeGroupVersion.WithKind("Deployment").GroupKind()
 	v1crdGK                = apiextv1.SchemeGroupVersion.WithKind("CustomResourceDefinition").GroupKind()
 	v1beta1crdGK           = apiextv1beta1.SchemeGroupVersion.WithKind("CustomResourceDefinition").GroupKind()
@@ -92,6 +96,10 @@ func (c *Manifests) UpdateFromDirs(deployDir, crdsDir string) error {
 				err = c.addRoles(manifest)
 			case clusterRoleGK:
 				err = c.addClusterRoles(manifest)
+			case roleBindingGK:
+				err = c.addRoleBindings(manifest)
+			case clusterRoleBindingGK:
+				err = c.addClusterRoleBindings(manifest)
 			case deploymentGK:
 				err = c.addDeployments(manifest)
 			case v1crdGK, v1beta1crdGK:
@@ -154,6 +162,10 @@ func (c *Manifests) UpdateFromReader(r io.Reader) error {
 			err = c.addRoles(manifest)
 		case clusterRoleGK:
 			err = c.addClusterRoles(manifest)
+		case roleBindingGK:
+			err = c.addRoleBindings(manifest)
+		case clusterRoleBindingGK:
+			err = c.addClusterRoleBindings(manifest)
 		case deploymentGK:
 			err = c.addDeployments(manifest)
 		case v1crdGK, v1beta1crdGK:
@@ -212,6 +224,30 @@ func (c *Manifests) addClusterRoles(rawManifests ...[]byte) error {
 	return nil
 }
 
+// addRoleBindings assumes all manifest data in rawManifests are RoleBindings and adds them to the collector.
+func (c *Manifests) addRoleBindings(rawManifests ...[]byte) error {
+	for _, rawManifest := range rawManifests {
+		binding := rbacv1.RoleBinding{}
+		if err := yaml.Unmarshal(rawManifest, &binding); err != nil {
+			return err
+		}
+		c.RoleBindings = append(c.RoleBindings, binding)
+	}
+	return nil
+}
+
+// addClusterRoleBindings assumes all manifest data in rawManifests are ClusterRoleBindings and adds them to the collector.
+func (c *Manifests) addClusterRoleBindings(rawManifests ...[]byte) error {
+	for _, rawManifest := range rawManifests {
+		binding := rbacv1.ClusterRoleBinding{}
+		if err := yaml.Unmarshal(rawManifest, &binding); err != nil {
+			return err
+		}
+		c.ClusterRoleBindings = append(c.ClusterRoleBindings, binding)
+	}
+	return nil
+}
+
 // addDeployments assumes all manifest data in rawManifests are Deployments
 // and adds them to the collector.
 func (c *Manifests) addDeployments(rawManifests ...[]byte) error {