Skip to content

feat(helm/provisioner): support deploying multiple provisioners in same namespace #15637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 25, 2024
Merged

feat(helm/provisioner): support deploying multiple provisioners in same namespace #15637

merged 3 commits into from
Nov 25, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Nov 24, 2024 •
edited
Loading

Fixes #15437

This allows us to support the following use-cases:

  1. Deploying multiple provisioner daemons with separate service accounts and roles: set nameOverride and coder.serviceAccount.Name
  2. Deploying a provisioner daemon that references an existing service account without creating a separate sa/role/rolebinding: set coder.serviceAccount.disableCreate=true, coder.serviceAccount.workspacePerms=false and coder.serviceAccount.name=<name of existing sa>.

Validated nameOverride using kustomize:

# Staged changes
--- a/kustomize/mycluster/coder/provisioner/kustomization.yaml
+++ b/kustomize/mycluster/coder/provisioner/kustomization.yaml
@@ -7,6 +7,12 @@ helmCharts:
     version: "2.17.2"
     namespace: coder
     valuesFile: provisioner-values.yaml
+  - name: coder-provisioner
+    releaseName: other-coder-provisioner
+    repo: https://helm.coder.com/v2
+    version: "2.17.2"
+    namespace: coder
+    valuesFile: other-provisioner-values.yaml

--- /dev/null
+++ b/kustomize/mycluster/coder/provisioner/other-provisioner-values.yaml
@@ -0,0 +1,7 @@
+coder:
+  serviceAccount:
+    name: "other-coder-provisioner"
+provisionerDaemon:
+  keySecretName: other-coder-provisioner-keys
+  keySecretKey: other-coder-onprem-1
+nameOverride: "other-coder-provisioner"

# Resulting new files (no modifications):

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	clusters/mycluster/coder/generated/deployment-other-coder-provisioner.yaml
	clusters/mycluster/coder/generated/role-other-coder-provisioner-workspace-perms.yaml
	clusters/mycluster/coder/generated/rolebinding-other-coder-provisioner.yaml
	clusters/mycluster/coder/generated/serviceaccount-other-coder-provisioner.yaml

johnstcn
johnstcn commented Nov 24, 2024
View reviewed changes
helm/coder/values.yaml
# coder.serviceAccount.name -- Whether to create the service account or use existing service account
# coder.serviceAccount.disableCreate -- Whether to create the service account or use existing service account.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review: drive-by typo fix

helm/provisioner/README.md
Comment on lines -35 to +36
pskSecretName: "coder-provisioner-psk"
keySecretName: "coder-provisionerd-key"
keySecretKey: "provisionerd-key"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: advertising provisioner keys as the preferred auth method

helm/provisioner/README.md
Comment on lines +105 to +113
extraTemplates:
- |
apiVersion: v1
kind: ConfigMap
metadata:
name: some-other-config
namespace: {{ .Release.Namespace }}
data:
key: some-other-value
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: this isn't strictly necessary, but I figured it could be a potential point of confusion, so elected to clarify it here.

helm/provisioner/README.md
```

## Specific Examples
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: these examples are essentially lifted from our tests

helm/provisioner/README.md
name: other-coder-provisioner
provisionerDaemon:
# ...
nameOverride: "other-coder-provisioner"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: this setting is essentially buried in libcoder, but it seems to be the best way to do this. Introducing a new separate variable here is just going to make things more complicated.

helm/provisioner/templates/coder.yaml
Comment on lines +2 to +4
{{- if not .Values.coder.serviceAccount.disableCreate }}
{{ include "libcoder.serviceaccount" (list . "coder.serviceaccount") }}
{{- end }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: this was added for helm/coder in #14817 but not ported over here.

I'm not sure if it would be better to do it in libcoder, folks don't seem to look in there much.

helm/provisioner/tests/testdata/name_override.golden
type: RuntimeDefault
volumeMounts: []
restartPolicy: Always
serviceAccountName: other-coder-provisioner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review: the fact that nameOverride has no bearing on serviceAccountName explicitly allows us to reference a pre-existing service account

@johnstcn johnstcn requested a review from spikecurtis November 24, 2024 17:06
@johnstcn johnstcn marked this pull request as ready for review November 24, 2024 17:06
ethanndickson
ethanndickson reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@ethanndickson ethanndickson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but will let someone with more helm experience approve

@johnstcn johnstcn removed the request for review from spikecurtis November 25, 2024 10:23
dannykopping
dannykopping approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnstcn johnstcn merged commit 7876dc5 into main Nov 25, 2024
30 checks passed
@johnstcn johnstcn deleted the cj/helm-provisioner-sa-disable-create branch November 25, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanndickson ethanndickson ethanndickson left review comments

@dannykopping dannykopping dannykopping approved these changes

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot deploy multiple provisioners on same namespace via Helm
3 participants
@johnstcn @dannykopping @ethanndickson