Only create ServiceAccounts if existing ServiceAccount is not specifi…

…ed (open-telemetry#1246) * Only create SAs if existing SA is not specified * Add note to types about ServiceAccount creation * Remove ServiceAccount from TA e2e test assertion, generate to update docs * make bundle * Update bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml Co-authored-by: Ben B. <[email protected]> * Update bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml Co-authored-by: Ben B. <[email protected]> * remove other invalid updates from make bundle Co-authored-by: Ben B. <[email protected]>
ItielOlenick · Nov 21, 2022 · 84e04c4 · 84e04c4
1 parent b646aa4
commit 84e04c4
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 20 deletions.
diff --git a/apis/v1alpha1/opentelemetrycollector_types.go b/apis/v1alpha1/opentelemetrycollector_types.go
@@ -84,7 +84,8 @@ type OpenTelemetryCollectorSpec struct {
 	// Mode represents how the collector should be deployed (deployment, daemonset, statefulset or sidecar)
 	// +optional
 	Mode Mode `json:"mode,omitempty"`
-	// ServiceAccount indicates the name of an existing service account to use with this instance.
+	// ServiceAccount indicates the name of an existing service account to use with this instance. When set,
+	// the operator will not automatically create a ServiceAccount for the collector.
 	// +optional
 	ServiceAccount string `json:"serviceAccount,omitempty"`
 	// Image indicates the container image to use for the OpenTelemetry Collector.
@@ -164,7 +165,8 @@ type OpenTelemetryTargetAllocator struct {
 	// Filtering is disabled by default.
 	// +optional
 	FilterStrategy string `json:"filterStrategy,omitempty"`
-	// ServiceAccount indicates the name of an existing service account to use with this instance.
+	// ServiceAccount indicates the name of an existing service account to use with this instance. When set,
+	// the operator will not automatically create a ServiceAccount for the TargetAllocator.
 	// +optional
 	ServiceAccount string `json:"serviceAccount,omitempty"`
 	// Image indicates the container image to use for the OpenTelemetry TargetAllocator.

diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
@@ -1685,7 +1685,8 @@ spec:
                 type: object
               serviceAccount:
                 description: ServiceAccount indicates the name of an existing service
-                  account to use with this instance.
+                  account to use with this instance. When set, the operator will not
+                  automatically create a ServiceAccount for the collector.
                 type: string
               targetAllocator:
                 description: TargetAllocator indicates a value which determines whether
@@ -1752,7 +1753,8 @@ spec:
                     type: integer
                   serviceAccount:
                     description: ServiceAccount indicates the name of an existing
-                      service account to use with this instance.
+                      service account to use with this instance. When set, the operator
+                      will not automatically create a ServiceAccount for the TargetAllocator.
                     type: string
                 type: object
               tolerations:

diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
@@ -1683,7 +1683,8 @@ spec:
                 type: object
               serviceAccount:
                 description: ServiceAccount indicates the name of an existing service
-                  account to use with this instance.
+                  account to use with this instance. When set, the operator will not
+                  automatically create a ServiceAccount for the collector.
                 type: string
               targetAllocator:
                 description: TargetAllocator indicates a value which determines whether
@@ -1750,7 +1751,8 @@ spec:
                     type: integer
                   serviceAccount:
                     description: ServiceAccount indicates the name of an existing
-                      service account to use with this instance.
+                      service account to use with this instance. When set, the operator
+                      will not automatically create a ServiceAccount for the TargetAllocator.
                     type: string
                 type: object
               tolerations:

diff --git a/docs/api.md b/docs/api.md
@@ -1843,7 +1843,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector.
         <td><b>serviceAccount</b></td>
         <td>string</td>
         <td>
-          ServiceAccount indicates the name of an existing service account to use with this instance.<br/>
+          ServiceAccount indicates the name of an existing service account to use with this instance. When set, the operator will not automatically create a ServiceAccount for the collector.<br/>
         </td>
         <td>false</td>
       </tr><tr>
@@ -4578,7 +4578,7 @@ TargetAllocator indicates a value which determines whether to spawn a target all
         <td><b>serviceAccount</b></td>
         <td>string</td>
         <td>
-          ServiceAccount indicates the name of an existing service account to use with this instance.<br/>
+          ServiceAccount indicates the name of an existing service account to use with this instance. When set, the operator will not automatically create a ServiceAccount for the TargetAllocator.<br/>
         </td>
         <td>false</td>
       </tr></tbody>

diff --git a/pkg/collector/reconcile/serviceaccount.go b/pkg/collector/reconcile/serviceaccount.go
@@ -33,13 +33,7 @@ import (
 
 // ServiceAccounts reconciles the service account(s) required for the instance in the current context.
 func ServiceAccounts(ctx context.Context, params Params) error {
-	desired := []corev1.ServiceAccount{}
-	if params.Instance.Spec.Mode != v1alpha1.ModeSidecar {
-		desired = append(desired, collector.ServiceAccount(params.Instance))
-	}
-	if params.Instance.Spec.TargetAllocator.Enabled {
-		desired = append(desired, targetallocator.ServiceAccount(params.Instance))
-	}
+	desired := desiredServiceAccounts(params)
 
 	// first, handle the create/update parts
 	if err := expectedServiceAccounts(ctx, params, desired); err != nil {
@@ -54,6 +48,17 @@ func ServiceAccounts(ctx context.Context, params Params) error {
 	return nil
 }
 
+func desiredServiceAccounts(params Params) []corev1.ServiceAccount {
+	desired := []corev1.ServiceAccount{}
+	if params.Instance.Spec.Mode != v1alpha1.ModeSidecar && len(params.Instance.Spec.ServiceAccount) == 0 {
+		desired = append(desired, collector.ServiceAccount(params.Instance))
+	}
+	if params.Instance.Spec.TargetAllocator.Enabled && len(params.Instance.Spec.TargetAllocator.ServiceAccount) == 0 {
+		desired = append(desired, targetallocator.ServiceAccount(params.Instance))
+	}
+	return desired
+}
+
 func expectedServiceAccounts(ctx context.Context, params Params, expected []corev1.ServiceAccount) error {
 	for _, obj := range expected {
 		desired := obj

diff --git a/pkg/collector/reconcile/serviceaccount_test.go b/pkg/collector/reconcile/serviceaccount_test.go
@@ -117,3 +117,30 @@ func TestDeleteServiceAccounts(t *testing.T) {
 	})
 
 }
+
+func TestDesiredServiceAccounts(t *testing.T) {
+	t.Run("should not create any service account", func(t *testing.T) {
+		params := params()
+		params.Instance.Spec.ServiceAccount = "existing-collector-sa"
+		params.Instance.Spec.TargetAllocator.Enabled = true
+		params.Instance.Spec.TargetAllocator.ServiceAccount = "existing-allocator-sa"
+		desired := desiredServiceAccounts(params)
+		assert.Len(t, desired, 0)
+	})
+
+	t.Run("should create collector service account", func(t *testing.T) {
+		params := params()
+		desired := desiredServiceAccounts(params)
+		assert.Len(t, desired, 1)
+		assert.Equal(t, collector.ServiceAccount(params.Instance), desired[0])
+	})
+
+	t.Run("should create targetallocator service account", func(t *testing.T) {
+		params := params()
+		params.Instance.Spec.ServiceAccount = "existing-collector-sa"
+		params.Instance.Spec.TargetAllocator.Enabled = true
+		desired := desiredServiceAccounts(params)
+		assert.Len(t, desired, 1)
+		assert.Equal(t, targetallocator.ServiceAccount(params.Instance), desired[0])
+	})
+}
diff --git a/tests/e2e/smoke-targetallocator/00-assert.yaml b/tests/e2e/smoke-targetallocator/00-assert.yaml
@@ -19,11 +19,6 @@ kind: ConfigMap
 metadata:
   name: stateful-targetallocator
 ---
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: stateful-targetallocator
----
 # Print TA pod logs if test fails
 apiVersion: kuttl.dev/v1beta1
 kind: TestAssert