[v17] Fix teleport-cluster chart service account logic causing Argo…

…CD deployments to fail (#49068)

* teleport-cluster: Use separate SA in hooks when possible

* test that SAs are not created when hooks are disabled

* fix chart typos

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 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.
+
+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 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.
+
+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 -}}

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 }}

examples/chart/teleport-cluster/templates/auth/predeploy_serviceaccount.yaml

@@ -2,22 +2,21 @@
 # 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 }}
     {{- if $auth.extraLabels.serviceAccount }}
     {{- 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 }}

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 }}

examples/chart/teleport-cluster/templates/proxy/predeploy_serviceaccount.yaml

@@ -3,21 +3,20 @@
 # 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 }}
     {{- if $proxy.extraLabels.serviceAccount }}
     {{- 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 }}

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:
@@ -189,41 +192,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-hook
+
+  - 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: RELEASE-NAME
+          value: helm-test-sa-hook
 
-  - it: should use serviceAccount.name for auth predeploy job SA when set in values
+  - 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
       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 creating SAs
     template: proxy/predeploy_job.yaml
     set:
       clusterName: helm-lint
       serviceAccount:
         name: helm-test-sa
+        create: false
     asserts:
       - equal:
           path: spec.template.spec.serviceAccountName