From 875aeb3f3ed2bc768839bbfc23ad40fe2c8b08e7 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 92b8fc0578fbb..f3e3aec14b50c 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 c2e43190ba8069634c6d5fa353e0b793563d340b 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 6e1acdb168f5a8825ab9c0038f51358e7595fd65 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 f3e3aec14b50c..7e2f4de3e2c04 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