[v1.10.x] fix(generate): exclude ServiceAccounts in the CSV from generated bundle (#5124)

This is an automated cherry-pick of #5120

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

Author: openshift-cherrypick-robot

Makefile

@@ -162,7 +162,7 @@ test-e2e-teardown:
 # Repeated rules are executed in the order they appear.
 $(e2e_targets):: test-e2e-setup image/scorecard-test
 test-e2e:: $(e2e_tests) ## Run e2e tests
-	
+
 test-e2e-go:: image/custom-scorecard-tests ## Run Go e2e tests
 	go test ./test/e2e/go -v -ginkgo.v
 test-e2e-ansible:: image/ansible-operator ## Run Ansible e2e tests

changelog/fragments/fix-bundle-service-accounts.yaml

@@ -0,0 +1,4 @@
+entries:
+  - description: >
+      In `generate bundle`, exclude ServiceAccounts already in a CSV from generated bundle.
+    kind: bugfix

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

@@ -31,23 +31,6 @@ func GetManifestObjects(c *collector.Manifests, extraSAs []string) (objs []clien
 		objs = append(objs, &c.V1beta1CustomResourceDefinitions[i])
 	}
 
-	// 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 {
-		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.
 	for i := range c.Services {
 		objs = append(objs, &c.Services[i])
@@ -61,7 +44,7 @@ func GetManifestObjects(c *collector.Manifests, extraSAs []string) (objs []clien
 		}
 	}
 
-	// RBAC objects that are not a part of the CSV should be written.
+	// RBAC objects (including ServiceAccounts) that are not a part of the CSV should be written.
 	_, _, rbacObjs := c.SplitCSVPermissionsObjects(extraSAs)
 	objs = append(objs, rbacObjs...)
 

internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go

@@ -379,7 +379,7 @@ var _ = Describe("findMatchingDeploymentAndServiceForWebhook", func() {
 		port1 := new(int32)
 		*port1 = 2311
 		crdToConfigPath := map[string]apiextv1.WebhookConversion{
-			"crd-test-1": apiextv1.WebhookConversion{
+			"crd-test-1": {
 				ClientConfig: &apiextv1.WebhookClientConfig{
 					Service: &apiextv1.ServiceReference{
 						Path: &path1,
@@ -388,7 +388,7 @@ var _ = Describe("findMatchingDeploymentAndServiceForWebhook", func() {
 				},
 			},
 
-			"crd-test-2": apiextv1.WebhookConversion{
+			"crd-test-2": {
 				ClientConfig: &apiextv1.WebhookClientConfig{
 					Service: &apiextv1.ServiceReference{
 						Path: &path1,

internal/generate/collector/clusterserviceversion.go

@@ -38,16 +38,16 @@ const (
 // are added to out. Any bindings with some associations, but with non-associations, are added to out unmodified.
 func (c *Manifests) SplitCSVPermissionsObjects(extraSAs []string) (inPerms, inCPerms, out []client.Object) {
 	// Create a set of ServiceAccount names to match against below.
-	saNameSet := make(map[string]struct{})
+	csvSAs := make(map[string]struct{})
 	for _, dep := range c.Deployments {
 		saName := dep.Spec.Template.Spec.ServiceAccountName
 		if saName == "" {
 			saName = defaultServiceAccountName
 		}
-		saNameSet[saName] = struct{}{}
+		csvSAs[saName] = struct{}{}
 	}
 	for _, extraSA := range extraSAs {
-		saNameSet[extraSA] = struct{}{}
+		csvSAs[extraSA] = struct{}{}
 	}
 
 	// Construct sets for lookups.
@@ -86,9 +86,9 @@ func (c *Manifests) SplitCSVPermissionsObjects(extraSAs []string) (inPerms, inCP
 	// bound Subjects from partial bindings to easily find concrete bindings that bind non-CSV RBAC.
 	// Those with no Subjects left will be removed from the partial binding lists; their concrete counterparts should
 	// be added to out.
-	inRoleNames := pRoleBindings.getRolesBoundToPartialBindings(roleKind, roleNameMap, saNameSet)
-	inCRoleNamesNScope := pRoleBindings.getRolesBoundToPartialBindings(cRoleKind, cRoleNameMap, saNameSet)
-	inCRoleNamesCScope := pCRoleBindings.getRolesBoundToPartialBindings(cRoleKind, cRoleNameMap, saNameSet)
+	inRoleNames := pRoleBindings.getRolesBoundToPartialBindings(roleKind, roleNameMap, csvSAs)
+	inCRoleNamesNScope := pRoleBindings.getRolesBoundToPartialBindings(cRoleKind, cRoleNameMap, csvSAs)
+	inCRoleNamesCScope := pCRoleBindings.getRolesBoundToPartialBindings(cRoleKind, cRoleNameMap, csvSAs)
 
 	// Add {Cluster}Roles bound to a ServiceAccount to either namespace- or cluster-scoped permission sets.
 	for _, roleName := range inRoleNames {
@@ -120,6 +120,14 @@ func (c *Manifests) SplitCSVPermissionsObjects(extraSAs []string) (inPerms, inCP
 		out = append(out, cRoleBindingMap[pBinding.Name])
 	}
 
+	// All ServiceAccounts not in the CSV should be in out.
+	for i := range c.ServiceAccounts {
+		sa := c.ServiceAccounts[i]
+		if _, csvHasSA := csvSAs[sa.Name]; !csvHasSA {
+			out = append(out, &sa)
+		}
+	}
+
 	return inPerms, inCPerms, out
 }
 

internal/generate/collector/clusterserviceversion_test.go

@@ -21,6 +21,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -160,6 +161,10 @@ var _ = Describe("SplitCSVPermissionsObjects", func() {
 
 		complexTestSetup := func() {
 			c.Deployments = []appsv1.Deployment{newDeploymentWithServiceAccount(depSA)}
+			c.ServiceAccounts = []corev1.ServiceAccount{
+				newServiceAccount(depSA),
+				newServiceAccount(extraSA),
+			}
 			role1, role2 := newRole(roleName1), newRole(roleName2)
 			c.Roles = []rbacv1.Role{role1, role2}
 			// Use the same names as for Roles to make sure Kind is respected
@@ -198,12 +203,14 @@ var _ = Describe("SplitCSVPermissionsObjects", func() {
 			Expect(getClusterRoleNames(inPerm)).To(Equal([]string{roleName1}))
 			Expect(inCPerm).To(HaveLen(1))
 			Expect(getClusterRoleNames(inCPerm)).To(Equal([]string{roleName1}))
-			Expect(out).To(HaveLen(5))
+			Expect(out).To(HaveLen(6))
 			Expect(getRoleNames(out)).To(Equal([]string{roleName2}))
 			Expect(getClusterRoleNames(out)).To(Equal([]string{roleName2}))
 			Expect(getRoleBindingNames(out)).To(Equal([]string{bindingName1, bindingName2}))
 			Expect(getClusterRoleBindingNames(out)).To(Equal([]string{bindingName2}))
+			Expect(getServiceAccountNames(out)).To(Equal([]string{extraSA}))
 		})
+
 		It("contains a Deployment serviceAccountName and extra ServiceAccount", func() {
 			complexTestSetup()
 			inPerm, inCPerm, out = c.SplitCSVPermissionsObjects([]string{extraSA})
@@ -233,6 +240,10 @@ func getClusterRoleBindingNames(objs []client.Object) []string {
 	return getNamesForKind("ClusterRoleBinding", objs)
 }
 
+func getServiceAccountNames(objs []client.Object) []string {
+	return getNamesForKind("ServiceAccount", objs)
+}
+
 func getNamesForKind(kind string, objs []client.Object) (names []string) {
 	for _, obj := range objs {
 		if obj.GetObjectKind().GroupVersionKind().Kind == kind {
@@ -300,3 +311,9 @@ func newSubject(name, kind string) (s rbacv1.Subject) {
 func newServiceAccountSubject(name string) (s rbacv1.Subject) {
 	return newSubject(name, "ServiceAccount")
 }
+
+func newServiceAccount(name string) (sa corev1.ServiceAccount) {
+	sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount"))
+	sa.Name = name
+	return sa
+}