diff --git a/Makefile b/Makefile index de6e426f00f..670356d2e21 100644 --- a/Makefile +++ b/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 diff --git a/changelog/fragments/fix-bundle-service-accounts.yaml b/changelog/fragments/fix-bundle-service-accounts.yaml new file mode 100644 index 00000000000..e434966cdc8 --- /dev/null +++ b/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 diff --git a/internal/cmd/operator-sdk/generate/internal/manifests.go b/internal/cmd/operator-sdk/generate/internal/manifests.go index 7e9ffb3a99e..5834684f02c 100644 --- a/internal/cmd/operator-sdk/generate/internal/manifests.go +++ b/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...) diff --git a/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go b/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go index 55fda9d33cc..5f20b476685 100644 --- a/internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go +++ b/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, diff --git a/internal/generate/collector/clusterserviceversion.go b/internal/generate/collector/clusterserviceversion.go index 4643623599c..c658aeeafa3 100644 --- a/internal/generate/collector/clusterserviceversion.go +++ b/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 } diff --git a/internal/generate/collector/clusterserviceversion_test.go b/internal/generate/collector/clusterserviceversion_test.go index 8604c8361c8..94a476f8275 100644 --- a/internal/generate/collector/clusterserviceversion_test.go +++ b/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 +} diff --git a/testdata/ansible/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml b/testdata/ansible/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml deleted file mode 100644 index adc7cf0c639..00000000000 --- a/testdata/ansible/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml +++ /dev/null @@ -1,5 +0,0 @@ -apiVersion: v1 -kind: ServiceAccount -metadata: - creationTimestamp: null - name: memcached-operator-controller-manager diff --git a/testdata/go/v3/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml b/testdata/go/v3/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml deleted file mode 100644 index adc7cf0c639..00000000000 --- a/testdata/go/v3/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml +++ /dev/null @@ -1,5 +0,0 @@ -apiVersion: v1 -kind: ServiceAccount -metadata: - creationTimestamp: null - name: memcached-operator-controller-manager diff --git a/testdata/helm/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml b/testdata/helm/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml deleted file mode 100644 index adc7cf0c639..00000000000 --- a/testdata/helm/memcached-operator/bundle/manifests/memcached-operator-controller-manager_v1_serviceaccount.yaml +++ /dev/null @@ -1,5 +0,0 @@ -apiVersion: v1 -kind: ServiceAccount -metadata: - creationTimestamp: null - name: memcached-operator-controller-manager