From 6e1035954625ef92960d614e5bca46f648a92434 Mon Sep 17 00:00:00 2001 From: Craig Squire Date: Mon, 21 Nov 2022 04:40:00 -0600 Subject: [PATCH] Only create ServiceAccounts if existing ServiceAccount is not specified (#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. * Update bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml Co-authored-by: Ben B. * remove other invalid updates from make bundle Co-authored-by: Ben B. --- apis/v1alpha1/opentelemetrycollector_types.go | 6 +++-- ...ntelemetry.io_opentelemetrycollectors.yaml | 6 +++-- ...ntelemetry.io_opentelemetrycollectors.yaml | 6 +++-- docs/api.md | 4 +-- pkg/collector/reconcile/serviceaccount.go | 19 ++++++++----- .../reconcile/serviceaccount_test.go | 27 +++++++++++++++++++ .../e2e/smoke-targetallocator/00-assert.yaml | 5 ---- 7 files changed, 53 insertions(+), 20 deletions(-) diff --git a/apis/v1alpha1/opentelemetrycollector_types.go b/apis/v1alpha1/opentelemetrycollector_types.go index 4907f65173..6db0579bc9 100644 --- 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 index 87b2378322..21d8168cab 100644 --- 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 index c24a7fb8c7..30ed3d3fa0 100644 --- 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 index 6f03ab970a..2bbc23e0af 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1843,7 +1843,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector. serviceAccount string - 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.
false @@ -4578,7 +4578,7 @@ TargetAllocator indicates a value which determines whether to spawn a target all serviceAccount string - 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.
false diff --git a/pkg/collector/reconcile/serviceaccount.go b/pkg/collector/reconcile/serviceaccount.go index 2aa55e50d9..b19c40aa10 100644 --- 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 index db310237c0..df6d693971 100644 --- 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 index c180946c0e..355df73c4a 100644 --- 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