From 62726d1cf4a855c4a404036050d5655f282c2a00 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Fri, 15 Nov 2024 11:27:02 -0500 Subject: [PATCH 1/3] teleport-cluster: Use separate SA in hooks when possible --- .../teleport-cluster/templates/_helpers.tpl | 38 +++++++++++++++++++ .../templates/auth/predeploy_job.yaml | 2 +- .../auth/predeploy_serviceaccount.yaml | 8 ++-- .../templates/proxy/predeploy_job.yaml | 2 +- .../proxy/predeploy_serviceaccount.yaml | 8 ++-- .../tests/predeploy_test.yaml | 36 +++++++++++++++--- 6 files changed, 78 insertions(+), 16 deletions(-) diff --git a/examples/chart/teleport-cluster/templates/_helpers.tpl b/examples/chart/teleport-cluster/templates/_helpers.tpl index 1edf2404f4368..785ef0c2065e8 100644 --- a/examples/chart/teleport-cluster/templates/_helpers.tpl +++ b/examples/chart/teleport-cluster/templates/_helpers.tpl @@ -6,10 +6,48 @@ if serviceAccount is not defined or serviceAccount.name is empty, use .Release.N {{- coalesce .Values.serviceAccount.name .Release.Name -}} {{- end -}} +{{/* +Create the name of the service account to use in the auth config check hook. + +If the chart is creating service accounts, we know we can create new arbitrary service accounts. +We cannot reuse the same name as the deployment SA because the non-hook sevrice account might +not exist yet. We tried being smart with hooks but ArgoCD doesn't differentiate between install +and upgrade, causing various issues on update and eventually forcing us to use a separate SA. + +If the chart is not creating service accounts, for backward compatibility we don't want +to force new service account names to existing chart users. We know the SA should already exist, +so we can use the same SA for deployments and hooks. +*/}} +{{- define "teleport-cluster.auth.hookServiceAccountName" -}} +{{- include "teleport-cluster.auth.serviceAccountName" . -}} +{{- if .Values.serviceAccount.create -}} +-hook +{{- end}} +{{- end -}} + {{- define "teleport-cluster.proxy.serviceAccountName" -}} {{- coalesce .Values.serviceAccount.name .Release.Name -}}-proxy {{- end -}} +{{/* +Create the name of the service account to use in the proxy config check hook. + +If the chart is creating service accounts, we know we can create new arbitrary service accounts. +We cannot reuse the same name as the deployment SA because the non-hook sevrice account might +not exist yet. We tried being smart with hooks but ArgoCD doesn't differentiate between install +and upgrade, causing various issues on update and eventually forcing us to use a separate SA. + +If the chart is not creating service accounts, for backward compatibility we don't want +to force new service account names to existing chart users. We know the SA should already exist, +so we can use the same SA for deployments and hooks. +*/}} +{{- define "teleport-cluster.proxy.hookServiceAccountName" -}} +{{- include "teleport-cluster.proxy.serviceAccountName" . -}} +{{- if .Values.serviceAccount.create -}} +-hook +{{- end}} +{{- end -}} + {{- define "teleport-cluster.version" -}} {{- coalesce .Values.teleportVersionOverride .Chart.Version }} {{- end -}} diff --git a/examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml b/examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml index e75c0f20a55e8..d5a38e93ead74 100644 --- a/examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml +++ b/examples/chart/teleport-cluster/templates/auth/predeploy_job.yaml @@ -104,5 +104,5 @@ spec: {{- if .Values.extraVolumes }} {{- toYaml .Values.extraVolumes | nindent 6 }} {{- end }} - serviceAccountName: {{ include "teleport-cluster.auth.serviceAccountName" . }} + serviceAccountName: {{ include "teleport-cluster.auth.hookServiceAccountName" . }} {{- end }} diff --git a/examples/chart/teleport-cluster/templates/auth/predeploy_serviceaccount.yaml b/examples/chart/teleport-cluster/templates/auth/predeploy_serviceaccount.yaml index 8510f30145fb0..893078f9a902d 100644 --- a/examples/chart/teleport-cluster/templates/auth/predeploy_serviceaccount.yaml +++ b/examples/chart/teleport-cluster/templates/auth/predeploy_serviceaccount.yaml @@ -2,12 +2,13 @@ # upon first install of the chart. it will be deleted by Helm after the pre-deploy hooks run, then the # regular serviceAccount is created with the same name and exists for the lifetime of the release. {{- $auth := mustMergeOverwrite (mustDeepCopy .Values) .Values.auth -}} +{{- if $auth.validateConfigOnDeploy }} {{- $projectedServiceAccountToken := semverCompare ">=1.20.0-0" .Capabilities.KubeVersion.Version }} {{- if $auth.serviceAccount.create }} apiVersion: v1 kind: ServiceAccount metadata: - name: {{ template "teleport-cluster.auth.serviceAccountName" . }} + name: {{ template "teleport-cluster.auth.hookServiceAccountName" . }} namespace: {{ .Release.Namespace }} labels: {{- include "teleport-cluster.auth.labels" . | nindent 4 }} @@ -15,9 +16,7 @@ metadata: {{- toYaml $auth.extraLabels.serviceAccount | nindent 4 }} {{- end }} annotations: - # this ServiceAccount resource MUST only be hooked on pre-install, as it would conflict - # with the existing ServiceAccount if hooked on pre-upgrade. - "helm.sh/hook": pre-install + "helm.sh/hook": pre-install,pre-upgrade "helm.sh/hook-weight": "3" "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded {{- if or $auth.annotations.serviceAccount $auth.azure.clientID }} @@ -32,3 +31,4 @@ metadata: automountServiceAccountToken: false {{- end }} {{- end }} +{{- end }} diff --git a/examples/chart/teleport-cluster/templates/proxy/predeploy_job.yaml b/examples/chart/teleport-cluster/templates/proxy/predeploy_job.yaml index 627aa4c626fcc..0f4ddb4f7fff4 100644 --- a/examples/chart/teleport-cluster/templates/proxy/predeploy_job.yaml +++ b/examples/chart/teleport-cluster/templates/proxy/predeploy_job.yaml @@ -100,5 +100,5 @@ spec: {{- if $proxy.extraVolumes }} {{- toYaml $proxy.extraVolumes | nindent 6 }} {{- end }} - serviceAccountName: {{ include "teleport-cluster.proxy.serviceAccountName" . }} + serviceAccountName: {{ include "teleport-cluster.proxy.hookServiceAccountName" . }} {{- end }} diff --git a/examples/chart/teleport-cluster/templates/proxy/predeploy_serviceaccount.yaml b/examples/chart/teleport-cluster/templates/proxy/predeploy_serviceaccount.yaml index 89a056ceed58e..6c5b9a4a1476c 100644 --- a/examples/chart/teleport-cluster/templates/proxy/predeploy_serviceaccount.yaml +++ b/examples/chart/teleport-cluster/templates/proxy/predeploy_serviceaccount.yaml @@ -3,11 +3,12 @@ # regular serviceAccount is created with the same name and exists for the lifetime of the release. {{- $proxy := mustMergeOverwrite (mustDeepCopy .Values) .Values.proxy -}} {{- $projectedServiceAccountToken := semverCompare ">=1.20.0-0" .Capabilities.KubeVersion.Version }} +{{- if $proxy.validateConfigOnDeploy }} {{- if $proxy.serviceAccount.create }} apiVersion: v1 kind: ServiceAccount metadata: - name: {{ include "teleport-cluster.proxy.serviceAccountName" . }} + name: {{ include "teleport-cluster.proxy.hookServiceAccountName" . }} namespace: {{ .Release.Namespace }} labels: {{- include "teleport-cluster.proxy.labels" . | nindent 4 }} @@ -15,9 +16,7 @@ metadata: {{- toYaml $proxy.extraLabels.serviceAccount | nindent 4 }} {{- end }} annotations: - # this ServiceAccount resource MUST only be hooked on pre-install, as it would conflict - # with the existing ServiceAccount if hooked on pre-upgrade. - "helm.sh/hook": pre-install + "helm.sh/hook": pre-install,pre-upgrade "helm.sh/hook-weight": "3" "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded {{- if $proxy.annotations.serviceAccount }} @@ -27,3 +26,4 @@ metadata: automountServiceAccountToken: false {{- end }} {{- end }} +{{- end }} diff --git a/examples/chart/teleport-cluster/tests/predeploy_test.yaml b/examples/chart/teleport-cluster/tests/predeploy_test.yaml index c9de357957901..44674a74ee656 100644 --- a/examples/chart/teleport-cluster/tests/predeploy_test.yaml +++ b/examples/chart/teleport-cluster/tests/predeploy_test.yaml @@ -189,41 +189,65 @@ tests: path: metadata.labels.baz value: overridden - - it: should use default serviceAccount name for auth predeploy job SA when not set in values + - it: should use default serviceAccount name suffixed with -hook for auth predeploy job SA when not set in values and we're creating SAs template: auth/predeploy_job.yaml set: clusterName: helm-lint asserts: - equal: path: spec.template.spec.serviceAccountName - value: RELEASE-NAME + value: RELEASE-NAME-hook - - it: should use serviceAccount.name for auth predeploy job SA when set in values + - it: should use serviceAccount.name suffixed with -hook for auth predeploy job SA when set in values and we're creating SAs template: auth/predeploy_job.yaml set: clusterName: helm-lint serviceAccount: name: helm-test-sa + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: helm-test-sa-hook + + - it: should use serviceAccount.name for auth predeploy job SA when set in values and we're not cretaing SAs + template: auth/predeploy_job.yaml + set: + clusterName: helm-lint + serviceAccount: + name: helm-test-sa + create: false asserts: - equal: path: spec.template.spec.serviceAccountName value: helm-test-sa - - it: should use default serviceAccount name for proxy predeploy job SA when not set in values + - it: should use default serviceAccount name suffixed with -hook for proxy predeploy job SA when not set in values and we're creating SAs template: proxy/predeploy_job.yaml set: clusterName: helm-lint asserts: - equal: path: spec.template.spec.serviceAccountName - value: RELEASE-NAME-proxy + value: RELEASE-NAME-proxy-hook + + - it: should use serviceAccount.name suffixed with -hook for proxy predeploy job SA when set in values and we're creating SAs + template: proxy/predeploy_job.yaml + set: + clusterName: helm-lint + serviceAccount: + name: helm-test-sa + asserts: + - equal: + path: spec.template.spec.serviceAccountName + value: helm-test-sa-proxy-hook - - it: should use serviceAccount.name for proxy predeploy job SA when set in values + - it: should use serviceAccount.name for proxy predeploy job SA when set in values and we're not cretaing SAs template: proxy/predeploy_job.yaml set: clusterName: helm-lint serviceAccount: name: helm-test-sa + create: false asserts: - equal: path: spec.template.spec.serviceAccountName From 358c40510cf93362c8889a6761b4da349642c910 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Fri, 15 Nov 2024 11:28:52 -0500 Subject: [PATCH 2/3] test that SAs are not created when hooks are disabled --- examples/chart/teleport-cluster/tests/predeploy_test.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples/chart/teleport-cluster/tests/predeploy_test.yaml b/examples/chart/teleport-cluster/tests/predeploy_test.yaml index 44674a74ee656..c55b225c5acc3 100644 --- a/examples/chart/teleport-cluster/tests/predeploy_test.yaml +++ b/examples/chart/teleport-cluster/tests/predeploy_test.yaml @@ -2,8 +2,10 @@ suite: Pre-Deploy Config Test Hooks templates: - auth/predeploy_job.yaml - auth/predeploy_config.yaml + - auth/predeploy_serviceaccount.yaml - proxy/predeploy_job.yaml - proxy/predeploy_config.yaml + - proxy/predeploy_serviceaccount.yaml tests: - it: Deploys the auth-test config template: auth/predeploy_config.yaml @@ -53,6 +55,7 @@ tests: asserts: - hasDocuments: count: 0 + - it: should set resources on auth predeploy job when set in values template: auth/predeploy_job.yaml values: From 0d511a66d113b976f3a44f6ada23e7423517c1ed Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Fri, 15 Nov 2024 12:31:53 -0500 Subject: [PATCH 3/3] fix chart typos --- examples/chart/teleport-cluster/templates/_helpers.tpl | 8 ++++---- examples/chart/teleport-cluster/tests/predeploy_test.yaml | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/examples/chart/teleport-cluster/templates/_helpers.tpl b/examples/chart/teleport-cluster/templates/_helpers.tpl index 785ef0c2065e8..668e51092ca2a 100644 --- a/examples/chart/teleport-cluster/templates/_helpers.tpl +++ b/examples/chart/teleport-cluster/templates/_helpers.tpl @@ -10,7 +10,7 @@ if serviceAccount is not defined or serviceAccount.name is empty, use .Release.N Create the name of the service account to use in the auth config check hook. If the chart is creating service accounts, we know we can create new arbitrary service accounts. -We cannot reuse the same name as the deployment SA because the non-hook sevrice account might +We cannot reuse the same name as the deployment SA because the non-hook service account might not exist yet. We tried being smart with hooks but ArgoCD doesn't differentiate between install and upgrade, causing various issues on update and eventually forcing us to use a separate SA. @@ -22,7 +22,7 @@ so we can use the same SA for deployments and hooks. {{- include "teleport-cluster.auth.serviceAccountName" . -}} {{- if .Values.serviceAccount.create -}} -hook -{{- end}} +{{- end -}} {{- end -}} {{- define "teleport-cluster.proxy.serviceAccountName" -}} @@ -33,7 +33,7 @@ so we can use the same SA for deployments and hooks. Create the name of the service account to use in the proxy config check hook. If the chart is creating service accounts, we know we can create new arbitrary service accounts. -We cannot reuse the same name as the deployment SA because the non-hook sevrice account might +We cannot reuse the same name as the deployment SA because the non-hook service account might not exist yet. We tried being smart with hooks but ArgoCD doesn't differentiate between install and upgrade, causing various issues on update and eventually forcing us to use a separate SA. @@ -45,7 +45,7 @@ so we can use the same SA for deployments and hooks. {{- include "teleport-cluster.proxy.serviceAccountName" . -}} {{- if .Values.serviceAccount.create -}} -hook -{{- end}} +{{- end -}} {{- end -}} {{- define "teleport-cluster.version" -}} diff --git a/examples/chart/teleport-cluster/tests/predeploy_test.yaml b/examples/chart/teleport-cluster/tests/predeploy_test.yaml index c55b225c5acc3..3ab3ad799e99c 100644 --- a/examples/chart/teleport-cluster/tests/predeploy_test.yaml +++ b/examples/chart/teleport-cluster/tests/predeploy_test.yaml @@ -212,7 +212,7 @@ tests: path: spec.template.spec.serviceAccountName value: helm-test-sa-hook - - it: should use serviceAccount.name for auth predeploy job SA when set in values and we're not cretaing SAs + - it: should use serviceAccount.name for auth predeploy job SA when set in values and we're not creating SAs template: auth/predeploy_job.yaml set: clusterName: helm-lint @@ -244,7 +244,7 @@ tests: path: spec.template.spec.serviceAccountName value: helm-test-sa-proxy-hook - - it: should use serviceAccount.name for proxy predeploy job SA when set in values and we're not cretaing SAs + - it: should use serviceAccount.name for proxy predeploy job SA when set in values and we're not creating SAs template: proxy/predeploy_job.yaml set: clusterName: helm-lint