generate bundle: add --extra-service-accounts flag to consider extra-Deployment SAs (#4826)

Signed-off-by: Eric Stroczynski <[email protected]>

Author: Eric Stroczynski

changelog/fragments/extra-service-accounts.yaml

@@ -0,0 +1,10 @@
+entries:
+  - description: >
+      Added --extra-service-accounts flag to `generate bundle` to consider roles bound to
+      service accounts not specified in the operator's Deployment.
+    kind: addition
+  - description: >
+      `generate bundle` adds ClusterRoles bound by RoleBindings to a CSV's `.spec.permissions`,
+      since these become namespace-scoped at runtime. They will also be added to `.spec.clusterPermissions`
+      if bound by a ClusterRoleBinding.
+    kind: change

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

@@ -197,16 +197,17 @@ func (c bundleCmd) runManifests() (err error) {
 	}
 
 	csvGen := gencsv.Generator{
-		OperatorName: c.packageName,
-		Version:      c.version,
-		Collector:    col,
-		Annotations:  metricsannotations.MakeBundleObjectAnnotations(c.layout),
+		OperatorName:         c.packageName,
+		Version:              c.version,
+		Collector:            col,
+		Annotations:          metricsannotations.MakeBundleObjectAnnotations(c.layout),
+		ExtraServiceAccounts: c.extraServiceAccounts,
 	}
 	if err := csvGen.Generate(opts...); err != nil {
 		return fmt.Errorf("error generating ClusterServiceVersion: %v", err)
 	}
 
-	objs := genutil.GetManifestObjects(col)
+	objs := genutil.GetManifestObjects(col, c.extraServiceAccounts)
 	if c.stdout {
 		if err := genutil.WriteObjects(stdout, objs...); err != nil {
 			return err

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

@@ -38,6 +38,8 @@ type bundleCmd struct {
 	crdsDir      string
 	stdout       bool
 	quiet        bool
+	// ServiceAccount names to consider outside of the operator's service account.
+	extraServiceAccounts []string
 
 	// Metadata options.
 	channels       string
@@ -129,6 +131,9 @@ func (c *bundleCmd) addFlagsTo(fs *pflag.FlagSet) {
 		"Directory containing kustomize bases in a \"bases\" dir and a kustomization.yaml for operator-framework manifests")
 	fs.StringVar(&c.channels, "channels", "alpha", "A comma-separated list of channels the bundle belongs to")
 	fs.StringVar(&c.defaultChannel, "default-channel", "", "The default channel for the bundle")
+	fs.StringSliceVar(&c.extraServiceAccounts, "extra-service-accounts", nil,
+		"Names of service accounts, outside of the operator's Deployment account, "+
+			"that have bindings to {Cluster}Roles that should be added to the CSV")
 	fs.BoolVar(&c.overwrite, "overwrite", true, "Overwrite the bundle's metadata and Dockerfile if they exist")
 	fs.BoolVarP(&c.quiet, "quiet", "q", false, "Run in quiet mode")
 	fs.BoolVar(&c.stdout, "stdout", false, "Write bundle manifest to stdout")

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

@@ -22,7 +22,7 @@ import (
 )
 
 // GetManifestObjects returns all objects to be written to a manifests directory from collector.Manifests.
-func GetManifestObjects(c *collector.Manifests) (objs []client.Object) {
+func GetManifestObjects(c *collector.Manifests, extraSAs []string) (objs []client.Object) {
 	// All CRDs passed in should be written.
 	for i := range c.V1CustomResourceDefinitions {
 		objs = append(objs, &c.V1CustomResourceDefinitions[i])
@@ -32,8 +32,20 @@ func GetManifestObjects(c *collector.Manifests) (objs []client.Object) {
 	}
 
 	// All ServiceAccounts passed in should be written.
+	saSet := make(map[string]struct{}, len(extraSAs))
+	for _, saName := range extraSAs {
+		saSet[saName] = struct{}{}
+	}
 	for i := range c.ServiceAccounts {
-		objs = append(objs, &c.ServiceAccounts[i])
+		sa := c.ServiceAccounts[i]
+		saSet[sa.GetName()] = struct{}{}
+		objs = append(objs, &sa)
+	}
+	extraSAs = make([]string, len(saSet))
+	i := 0
+	for saName := range saSet {
+		extraSAs[i] = saName
+		i++
 	}
 
 	// All Services passed in should be written.
@@ -50,10 +62,8 @@ func GetManifestObjects(c *collector.Manifests) (objs []client.Object) {
 	}
 
 	// 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...)
+	_, _, rbacObjs := c.SplitCSVPermissionsObjects(extraSAs)
+	objs = append(objs, rbacObjs...)
 
 	removeNamespace(objs)
 	return objs

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

@@ -47,7 +47,7 @@ var _ = Describe("GetManifestObjects", func() {
 				{ObjectMeta: metav1.ObjectMeta{Namespace: "bar"}},
 			},
 		}
-		objs := GetManifestObjects(&m)
+		objs := GetManifestObjects(&m, nil)
 		Expect(objs).To(HaveLen(len(m.Roles) + len(m.ClusterRoles) + len(m.ServiceAccounts) + len(m.V1CustomResourceDefinitions) + len(m.V1beta1CustomResourceDefinitions)))
 		for _, obj := range objs {
 			Expect(obj.GetNamespace()).To(BeEmpty())

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

@@ -207,7 +207,8 @@ func (c packagemanifestsCmd) run() error {
 	}
 
 	if c.updateObjects {
-		objs := genutil.GetManifestObjects(col)
+		// Extra ServiceAccounts not supported by this command.
+		objs := genutil.GetManifestObjects(col, nil)
 		if c.stdout {
 			if err := genutil.WriteObjects(stdout, objs...); err != nil {
 				return err

internal/generate/clusterserviceversion/clusterserviceversion.go

@@ -53,6 +53,9 @@ type Generator struct {
 	Collector *collector.Manifests
 	// Annotations are applied to the resulting CSV.
 	Annotations map[string]string
+	// ExtraServiceAccounts are ServiceAccount names to consider when matching
+	// {Cluster}Roles to include in a CSV via their Bindings.
+	ExtraServiceAccounts []string
 
 	// Func that returns the writer the generated CSV's bytes are written to.
 	getWriter func() (io.Writer, error)
@@ -163,7 +166,7 @@ func (g *Generator) generate() (base *operatorsv1alpha1.ClusterServiceVersion, e
 		base.Spec.Replaces = genutil.MakeCSVName(g.OperatorName, g.FromVersion)
 	}
 
-	if err := ApplyTo(g.Collector, base); err != nil {
+	if err := ApplyTo(g.Collector, base, g.ExtraServiceAccounts); err != nil {
 		return nil, err
 	}
 

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

@@ -30,16 +30,17 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/operator-framework/operator-sdk/internal/generate/collector"
 	"github.com/operator-framework/operator-sdk/internal/util/k8sutil"
 )
 
 // ApplyTo applies relevant manifests in c to csv, sorts the applied updates,
 // and validates the result.
-func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion) error {
+func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion, extraSAs []string) error {
 	// Apply manifests to the CSV object.
-	if err := apply(c, csv); err != nil {
+	if err := apply(c, csv, extraSAs); err != nil {
 		return err
 	}
 
@@ -50,12 +51,13 @@ func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersio
 }
 
 // apply applies relevant manifests in c to csv.
-func apply(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion) error {
+func apply(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion, extraSAs []string) error {
 	strategy := getCSVInstallStrategy(csv)
 	switch strategy.StrategyName {
 	case operatorsv1alpha1.InstallStrategyNameDeployment:
-		applyRoles(c, &strategy.StrategySpec)
-		applyClusterRoles(c, &strategy.StrategySpec)
+		inPerms, inCPerms, _ := c.SplitCSVPermissionsObjects(extraSAs)
+		applyRoles(c, inPerms, &strategy.StrategySpec, extraSAs)
+		applyClusterRoles(c, inCPerms, &strategy.StrategySpec, extraSAs)
 		applyDeployments(c, &strategy.StrategySpec)
 	}
 	csv.Spec.InstallStrategy = strategy
@@ -82,34 +84,46 @@ const defaultServiceAccountName = "default"
 
 // applyRoles applies Roles to strategy's permissions field by combining Roles bound to ServiceAccounts
 // into one set of permissions.
-func applyRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) { //nolint:dupl
-	objs, _ := c.SplitCSVPermissionsObjects()
-	roleSet := make(map[string]*rbacv1.Role)
+func applyRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) { //nolint:dupl
+	roleSet := make(map[string]rbacv1.Role)
+	cRoleSet := make(map[string]rbacv1.ClusterRole)
 	for i := range objs {
 		switch t := objs[i].(type) {
 		case *rbacv1.Role:
-			roleSet[t.GetName()] = t
-		}
-	}
-
-	saToPermissions := make(map[string]operatorsv1alpha1.StrategyDeploymentPermissions)
-	for _, dep := range c.Deployments {
-		saName := dep.Spec.Template.Spec.ServiceAccountName
-		if saName == "" {
-			saName = defaultServiceAccountName
+			roleSet[t.GetName()] = *t
+		case *rbacv1.ClusterRole:
+			cRoleSet[t.GetName()] = *t
 		}
-		saToPermissions[saName] = operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: saName}
 	}
 
-	// Collect all role names by their corresponding service accounts via bindings. This lets us
+	// Collect all role and cluster 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.
+	saToPermissions := initPermissionSet(c.Deployments, extraSAs)
 	for _, binding := range c.RoleBindings {
-		if role, hasRole := roleSet[binding.RoleRef.Name]; hasRole {
-			for _, subject := range binding.Subjects {
-				if perm, hasSA := saToPermissions[subject.Name]; hasSA && subject.Kind == "ServiceAccount" {
-					perm.Rules = append(perm.Rules, role.Rules...)
-					saToPermissions[subject.Name] = perm
-				}
+		for _, subject := range binding.Subjects {
+			perm, hasSA := saToPermissions[subject.Name]
+			if subject.Kind != "ServiceAccount" || !hasSA {
+				continue
+			}
+			var (
+				rules   []rbacv1.PolicyRule
+				hasRole bool
+			)
+			switch binding.RoleRef.Kind {
+			case "Role":
+				role, has := roleSet[binding.RoleRef.Name]
+				rules = role.Rules
+				hasRole = has
+			case "ClusterRole":
+				role, has := cRoleSet[binding.RoleRef.Name]
+				rules = role.Rules
+				hasRole = has
+			default:
+				continue
+			}
+			if hasRole {
+				perm.Rules = append(perm.Rules, rules...)
+				saToPermissions[subject.Name] = perm
 			}
 		}
 	}
@@ -121,39 +135,35 @@ func applyRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDeta
 			perms = append(perms, perm)
 		}
 	}
+	sort.Slice(perms, func(i, j int) bool {
+		return perms[i].ServiceAccountName < perms[j].ServiceAccountName
+	})
 	strategy.Permissions = perms
 }
 
 // applyClusterRoles applies ClusterRoles to strategy's clusterPermissions field by combining ClusterRoles
 // bound to ServiceAccounts into one set of clusterPermissions.
-func applyClusterRoles(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) { //nolint:dupl
-	objs, _ := c.SplitCSVClusterPermissionsObjects()
-	roleSet := make(map[string]*rbacv1.ClusterRole)
+func applyClusterRoles(c *collector.Manifests, objs []client.Object, strategy *operatorsv1alpha1.StrategyDetailsDeployment, extraSAs []string) { //nolint:dupl
+	roleSet := make(map[string]rbacv1.ClusterRole)
 	for i := range objs {
 		switch t := objs[i].(type) {
 		case *rbacv1.ClusterRole:
-			roleSet[t.GetName()] = t
+			roleSet[t.GetName()] = *t
 		}
 	}
 
-	saToPermissions := make(map[string]operatorsv1alpha1.StrategyDeploymentPermissions)
-	for _, dep := range c.Deployments {
-		saName := dep.Spec.Template.Spec.ServiceAccountName
-		if saName == "" {
-			saName = defaultServiceAccountName
-		}
-		saToPermissions[saName] = operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: saName}
-	}
-
 	// 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.
+	saToPermissions := initPermissionSet(c.Deployments, extraSAs)
 	for _, binding := range c.ClusterRoleBindings {
-		if role, hasRole := roleSet[binding.RoleRef.Name]; hasRole {
-			for _, subject := range binding.Subjects {
-				if perm, hasSA := saToPermissions[subject.Name]; hasSA && subject.Kind == "ServiceAccount" {
-					perm.Rules = append(perm.Rules, role.Rules...)
-					saToPermissions[subject.Name] = perm
-				}
+		for _, subject := range binding.Subjects {
+			perm, hasSA := saToPermissions[subject.Name]
+			if !hasSA || subject.Kind != "ServiceAccount" {
+				continue
+			}
+			if role, hasRole := roleSet[binding.RoleRef.Name]; hasRole {
+				perm.Rules = append(perm.Rules, role.Rules...)
+				saToPermissions[subject.Name] = perm
 			}
 		}
 	}
@@ -165,9 +175,28 @@ func applyClusterRoles(c *collector.Manifests, strategy *operatorsv1alpha1.Strat
 			perms = append(perms, perm)
 		}
 	}
+	sort.Slice(perms, func(i, j int) bool {
+		return perms[i].ServiceAccountName < perms[j].ServiceAccountName
+	})
 	strategy.ClusterPermissions = perms
 }
 
+// initPermissionSet initializes a map of ServiceAccount name to permissions, which are empty.
+func initPermissionSet(deps []appsv1.Deployment, extraSAs []string) map[string]operatorsv1alpha1.StrategyDeploymentPermissions {
+	saToPermissions := make(map[string]operatorsv1alpha1.StrategyDeploymentPermissions)
+	for _, dep := range deps {
+		saName := dep.Spec.Template.Spec.ServiceAccountName
+		if saName == "" {
+			saName = defaultServiceAccountName
+		}
+		saToPermissions[saName] = operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: saName}
+	}
+	for _, extraSA := range extraSAs {
+		saToPermissions[extraSA] = operatorsv1alpha1.StrategyDeploymentPermissions{ServiceAccountName: extraSA}
+	}
+	return saToPermissions
+}
+
 // applyDeployments updates strategy's deployments with the Deployments
 // in the collector.
 func applyDeployments(c *collector.Manifests, strategy *operatorsv1alpha1.StrategyDetailsDeployment) {