From 6781b0fc87128999e943d2520b0925caf7f613fb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Nov 2024 12:33:58 +0000 Subject: [PATCH] fix(helm/provisioner): prefer provisioner key if both psk and key are set (#15417) Relates to https://github.com/coder/coder/pull/15416 This PR modifies the provisioner helm chart logic: - Previously, when both provisionerDaemon.keySecretName and provisionerDaemon.pskSecretName were both set, we would fail to install the chart. This required users to have an obnoxious workaround in place where setting provisionerDaemon.pskSecretName="" was required in order to use provisioner keys. We now check for pskSecretName being set to the default value when keySecretName is also specified, and switch to provisioner key authentication instead of PSK. The previous workaround is still supported. - We also had omitted to check for provisionerd.Tags being set along with provisionerDaemon.keySecretName. This would result in a crashlooping provisioner deployment, as setting both of these configuration options is not allowed. We now fast-fail the Helm deployment if we detect this scenario. --- helm/provisioner/templates/_coder.tpl | 23 +-- helm/provisioner/tests/chart_test.go | 10 ++ .../tests/testdata/provisionerd_key.golden | 2 - .../tests/testdata/provisionerd_key.yaml | 4 - ...ovisionerd_key_psk_empty_workaround.golden | 135 ++++++++++++++++++ ...provisionerd_key_psk_empty_workaround.yaml | 7 + .../tests/testdata/provisionerd_key_tags.yaml | 9 ++ .../testdata/provisionerd_no_psk_or_key.yaml | 3 - .../tests/testdata/provisionerd_psk.golden | 2 +- .../tests/testdata/provisionerd_psk.yaml | 2 +- .../testdata/provisionerd_psk_and_key.yaml | 2 +- helm/provisioner/values.yaml | 11 +- 12 files changed, 186 insertions(+), 24 deletions(-) create mode 100644 helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.golden create mode 100644 helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.yaml create mode 100644 helm/provisioner/tests/testdata/provisionerd_key_tags.yaml diff --git a/helm/provisioner/templates/_coder.tpl b/helm/provisioner/templates/_coder.tpl index 9c2b2dece1..585393a6bf 100644 --- a/helm/provisioner/templates/_coder.tpl +++ b/helm/provisioner/templates/_coder.tpl @@ -34,22 +34,23 @@ env: value: "0.0.0.0:2112" {{- if and (empty .Values.provisionerDaemon.pskSecretName) (empty .Values.provisionerDaemon.keySecretName) }} {{ fail "Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified." }} -{{- else if and (.Values.provisionerDaemon.pskSecretName) (.Values.provisionerDaemon.keySecretName) }} -{{ fail "Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified, but not both." }} -{{- end }} -{{- if .Values.provisionerDaemon.pskSecretName }} -- name: CODER_PROVISIONER_DAEMON_PSK - valueFrom: - secretKeyRef: - name: {{ .Values.provisionerDaemon.pskSecretName | quote }} - key: psk -{{- end }} -{{- if and .Values.provisionerDaemon.keySecretName .Values.provisionerDaemon.keySecretKey }} +{{- else if and .Values.provisionerDaemon.keySecretName .Values.provisionerDaemon.keySecretKey }} + {{- if and (not (empty .Values.provisionerDaemon.pskSecretName)) (ne .Values.provisionerDaemon.pskSecretName "coder-provisioner-psk") }} + {{ fail "Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified, but not both." }} + {{- else if .Values.provisionerDaemon.tags }} + {{ fail "provisionerDaemon.tags may not be specified with provisionerDaemon.keySecretName." }} + {{- end }} - name: CODER_PROVISIONER_DAEMON_KEY valueFrom: secretKeyRef: name: {{ .Values.provisionerDaemon.keySecretName | quote }} key: {{ .Values.provisionerDaemon.keySecretKey | quote }} +{{- else }} +- name: CODER_PROVISIONER_DAEMON_PSK + valueFrom: + secretKeyRef: + name: {{ .Values.provisionerDaemon.pskSecretName | quote }} + key: psk {{- end }} {{- if include "provisioner.tags" . }} - name: CODER_PROVISIONERD_TAGS diff --git a/helm/provisioner/tests/chart_test.go b/helm/provisioner/tests/chart_test.go index ab6d8445e8..4bb54e2d78 100644 --- a/helm/provisioner/tests/chart_test.go +++ b/helm/provisioner/tests/chart_test.go @@ -56,6 +56,12 @@ var testCases = []testCase{ name: "provisionerd_key", expectedError: "", }, + // Test explicitly for the workaround where setting provisionerDaemon.pskSecretName="" + // was required to use provisioner keys. + { + name: "provisionerd_key_psk_empty_workaround", + expectedError: "", + }, { name: "provisionerd_psk_and_key", expectedError: `Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified, but not both.`, @@ -64,6 +70,10 @@ var testCases = []testCase{ name: "provisionerd_no_psk_or_key", expectedError: `Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified.`, }, + { + name: "provisionerd_key_tags", + expectedError: `provisionerDaemon.tags may not be specified with provisionerDaemon.keySecretName.`, + }, { name: "extra_templates", expectedError: "", diff --git a/helm/provisioner/tests/testdata/provisionerd_key.golden b/helm/provisioner/tests/testdata/provisionerd_key.golden index c4f33f766d..c4c23ec6da 100644 --- a/helm/provisioner/tests/testdata/provisionerd_key.golden +++ b/helm/provisioner/tests/testdata/provisionerd_key.golden @@ -112,8 +112,6 @@ spec: secretKeyRef: key: provisionerd-key name: coder-provisionerd-key - - name: CODER_PROVISIONERD_TAGS - value: clusterType=k8s,location=auh - name: CODER_URL value: http://coder.default.svc.cluster.local image: ghcr.io/coder/coder:latest diff --git a/helm/provisioner/tests/testdata/provisionerd_key.yaml b/helm/provisioner/tests/testdata/provisionerd_key.yaml index c5ab331a45..82f786637e 100644 --- a/helm/provisioner/tests/testdata/provisionerd_key.yaml +++ b/helm/provisioner/tests/testdata/provisionerd_key.yaml @@ -2,9 +2,5 @@ coder: image: tag: latest provisionerDaemon: - pskSecretName: "" keySecretName: "coder-provisionerd-key" keySecretKey: "provisionerd-key" - tags: - location: auh - clusterType: k8s diff --git a/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.golden b/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.golden new file mode 100644 index 0000000000..c4c23ec6da --- /dev/null +++ b/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.golden @@ -0,0 +1,135 @@ +--- +# Source: coder-provisioner/templates/coder.yaml +apiVersion: v1 +kind: ServiceAccount +metadata: + annotations: {} + labels: + app.kubernetes.io/instance: release-name + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: coder-provisioner + app.kubernetes.io/part-of: coder-provisioner + app.kubernetes.io/version: 0.1.0 + helm.sh/chart: coder-provisioner-0.1.0 + name: coder-provisioner +--- +# Source: coder-provisioner/templates/rbac.yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: coder-provisioner-workspace-perms +rules: + - apiGroups: [""] + resources: ["pods"] + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch + - apiGroups: [""] + resources: ["persistentvolumeclaims"] + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch + - apiGroups: + - apps + resources: + - deployments + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch +--- +# Source: coder-provisioner/templates/rbac.yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: "coder-provisioner" +subjects: + - kind: ServiceAccount + name: "coder-provisioner" +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: coder-provisioner-workspace-perms +--- +# Source: coder-provisioner/templates/coder.yaml +apiVersion: apps/v1 +kind: Deployment +metadata: + annotations: {} + labels: + app.kubernetes.io/instance: release-name + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: coder-provisioner + app.kubernetes.io/part-of: coder-provisioner + app.kubernetes.io/version: 0.1.0 + helm.sh/chart: coder-provisioner-0.1.0 + name: coder-provisioner +spec: + replicas: 1 + selector: + matchLabels: + app.kubernetes.io/instance: release-name + app.kubernetes.io/name: coder-provisioner + template: + metadata: + annotations: {} + labels: + app.kubernetes.io/instance: release-name + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: coder-provisioner + app.kubernetes.io/part-of: coder-provisioner + app.kubernetes.io/version: 0.1.0 + helm.sh/chart: coder-provisioner-0.1.0 + spec: + containers: + - args: + - provisionerd + - start + command: + - /opt/coder + env: + - name: CODER_PROMETHEUS_ADDRESS + value: 0.0.0.0:2112 + - name: CODER_PROVISIONER_DAEMON_KEY + valueFrom: + secretKeyRef: + key: provisionerd-key + name: coder-provisionerd-key + - name: CODER_URL + value: http://coder.default.svc.cluster.local + image: ghcr.io/coder/coder:latest + imagePullPolicy: IfNotPresent + lifecycle: {} + name: coder + ports: null + resources: {} + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: null + runAsGroup: 1000 + runAsNonRoot: true + runAsUser: 1000 + seccompProfile: + type: RuntimeDefault + volumeMounts: [] + restartPolicy: Always + serviceAccountName: coder-provisioner + terminationGracePeriodSeconds: 600 + volumes: [] diff --git a/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.yaml b/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.yaml new file mode 100644 index 0000000000..cfa46974c3 --- /dev/null +++ b/helm/provisioner/tests/testdata/provisionerd_key_psk_empty_workaround.yaml @@ -0,0 +1,7 @@ +coder: + image: + tag: latest +provisionerDaemon: + pskSecretName: "" + keySecretName: "coder-provisionerd-key" + keySecretKey: "provisionerd-key" diff --git a/helm/provisioner/tests/testdata/provisionerd_key_tags.yaml b/helm/provisioner/tests/testdata/provisionerd_key_tags.yaml new file mode 100644 index 0000000000..7cb35f0052 --- /dev/null +++ b/helm/provisioner/tests/testdata/provisionerd_key_tags.yaml @@ -0,0 +1,9 @@ +coder: + image: + tag: latest +provisionerDaemon: + keySecretName: "coder-provisionerd-key" + keySecretKey: "provisionerd-key" + tags: + location: auh + clusterType: k8s diff --git a/helm/provisioner/tests/testdata/provisionerd_no_psk_or_key.yaml b/helm/provisioner/tests/testdata/provisionerd_no_psk_or_key.yaml index dbb0eca812..4d883a59fc 100644 --- a/helm/provisioner/tests/testdata/provisionerd_no_psk_or_key.yaml +++ b/helm/provisioner/tests/testdata/provisionerd_no_psk_or_key.yaml @@ -4,6 +4,3 @@ coder: provisionerDaemon: pskSecretName: "" keySecretName: "" - tags: - location: auh - clusterType: k8s diff --git a/helm/provisioner/tests/testdata/provisionerd_psk.golden b/helm/provisioner/tests/testdata/provisionerd_psk.golden index b641ee0db3..c1d9421c3c 100644 --- a/helm/provisioner/tests/testdata/provisionerd_psk.golden +++ b/helm/provisioner/tests/testdata/provisionerd_psk.golden @@ -111,7 +111,7 @@ spec: valueFrom: secretKeyRef: key: psk - name: coder-provisionerd-psk + name: not-the-default-coder-provisioner-psk - name: CODER_PROVISIONERD_TAGS value: clusterType=k8s,location=auh - name: CODER_URL diff --git a/helm/provisioner/tests/testdata/provisionerd_psk.yaml b/helm/provisioner/tests/testdata/provisionerd_psk.yaml index f891b007db..c53958d4b8 100644 --- a/helm/provisioner/tests/testdata/provisionerd_psk.yaml +++ b/helm/provisioner/tests/testdata/provisionerd_psk.yaml @@ -2,7 +2,7 @@ coder: image: tag: latest provisionerDaemon: - pskSecretName: "coder-provisionerd-psk" + pskSecretName: "not-the-default-coder-provisioner-psk" tags: location: auh clusterType: k8s diff --git a/helm/provisioner/tests/testdata/provisionerd_psk_and_key.yaml b/helm/provisioner/tests/testdata/provisionerd_psk_and_key.yaml index 530f48807e..d2da1c370d 100644 --- a/helm/provisioner/tests/testdata/provisionerd_psk_and_key.yaml +++ b/helm/provisioner/tests/testdata/provisionerd_psk_and_key.yaml @@ -2,7 +2,7 @@ coder: image: tag: latest provisionerDaemon: - pskSecretName: "coder-provisionerd-psk" + pskSecretName: "not-the-default-coder-provisioner-psk" keySecretName: "coder-provisionerd-key" keySecretKey: "provisionerd-key" tags: diff --git a/helm/provisioner/values.yaml b/helm/provisioner/values.yaml index 446a4605db..869ddc876c 100644 --- a/helm/provisioner/values.yaml +++ b/helm/provisioner/values.yaml @@ -204,14 +204,23 @@ provisionerDaemon: # provisionerDaemon.keySecretName -- The name of the Kubernetes # secret that contains a provisioner key to use to authenticate with Coder. # See: https://coder.com/docs/admin/provisioners#authentication + # NOTE: it is not permitted to specify both provisionerDaemon.keySecretName + # and provisionerDaemon.pskSecretName. An exception is made for the purposes + # of backwards-compatibility: if provisionerDaemon.pskSecretName is unchanged + # from the default value and provisionerDaemon.keySecretName is set, then + # provisionerDaemon.keySecretName and provisionerDaemon.keySecretKey will take + # precedence over provisionerDaemon.pskSecretName. keySecretName: "" # provisionerDaemon.keySecretKey -- The key of the Kubernetes # secret specified in provisionerDaemon.keySecretName that contains # the provisioner key. Defaults to "key". keySecretKey: "key" - # provisionerDaemon.tags -- Tags to filter provisioner jobs by. + # provisionerDaemon.tags -- If using a PSK, specify the set of provisioner + # job tags for which this provisioner daemon is responsible. # See: https://coder.com/docs/admin/provisioners#provisioner-tags + # NOTE: it is not permitted to specify both provisionerDaemon.tags and + # provsionerDaemon.keySecretName. tags: {} # location: usa