Skip to content

feat(helm/provisioner): add support for provisioner keys, add note re psk #15122

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 4 commits into from
Oct 18, 2024
Merged

feat(helm/provisioner): add support for provisioner keys, add note re psk #15122

merged 4 commits into from
Oct 18, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Oct 17, 2024 •
edited
Loading

Relates to #14985

  • Adds provisionerDaemon.keySecretName and provisionerDaemon.keySecretKey
  • Omitting provisionerDaemon.pskSecretName will now cause the PSK secret to no longer be created.
  • Adds a note in NOTES.txt regarding provisioner PSKs.
  • Adds validation that either provisionerDaemon.keySecretName or provisionerDaemon.pskSecretName is specified, and will fail the install in this case.

Manual smoke-testing:

  1. With defaults:
helm install --namespace=tmp test-provisionerd . --set coder.image.tag=latest --dry-run 

NAME: test-provisionerd
LAST DEPLOYED: Thu Oct 17 14:45:55 2024
NAMESPACE: tmp
STATUS: pending-install
REVISION: 1
TEST SUITE: None
HOOKS:
MANIFEST: <yaml>

NOTES:
* Provisioner Daemon PSKs are no longer recommended for use with external
  provisioners. Consider migrating to scoped provisioner keys instead. For more
  information, see: https://coder.com/docs/admin/provisioners#authentication

Enjoy Coder! Please create an issue at https://github.com/coder/coder if you run
into any problems! :)
  1. With explicitly specified pskSecretName:
helm install --namespace=tmp test-provisionerd . --set coder.image.tag=latest --set provisionerDaemon.pskSecretName='foobar' --dry-run

NAME: test-provisionerd
LAST DEPLOYED: Thu Oct 17 14:43:08 2024
NAMESPACE: tmp
STATUS: pending-install
REVISION: 1
TEST SUITE: None
HOOKS:
MANIFEST: <yaml>
NOTES:
* Provisioner Daemon PSKs are no longer recommended for use with external
  provisioners. Consider migrating to scoped provisioner keys instead. For more
  information, see: https://coder.com/docs/admin/provisioners#authentication

Enjoy Coder! Please create an issue at https://github.com/coder/coder if you run
into any problems! :)
  1. With pskSecretName empty and keySecretName specified:
helm install --namespace=tmp test-provisionerd . --set coder.image.tag=latest --set provisionerDaemon.pskSecretName='' --set provisionerDaemon.keySecretName='foobar' --dry-run 

NAME: test-provisionerd
LAST DEPLOYED: Thu Oct 17 14:45:00 2024
NAMESPACE: tmp
STATUS: pending-install
REVISION: 1
TEST SUITE: None
HOOKS:
MANIFEST: <yaml>

NOTES:
Enjoy Coder! Please create an issue at https://github.com/coder/coder if you run
into any problems! :)
  1. With pskSecretName empty and keySecretName empty:
helm install --namespace=tmp test-provisionerd . --set coder.image.tag=latest --set provisionerDaemon.pskSecretName='' --set provisionerDaemon.keySecretName='' --dry-run

Error: INSTALLATION FAILED: execution error at (coder-provisioner/templates/coder.yaml:5:3): Either provisionerDaemon.pskSecretName or provisionerDaemon.keySecretName must be specified.

@johnstcn johnstcn self-assigned this Oct 17, 2024
matifali
matifali reviewed Oct 18, 2024
View reviewed changes
Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

LGTM.

mtojek
mtojek approved these changes Oct 18, 2024
View reviewed changes
helm/provisioner/tests/testdata/provisionerd_psk_and_key.golden
- name: CODER_PROVISIONER_DAEMON_PSK
valueFrom:
secretKeyRef:
key: psk
Copy link
Member

Choose a reason for hiding this comment

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

nit: any reason why this is called psk instead of provisionerd-psk?

Copy link
Member Author

@johnstcn johnstcn Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The key name is currently hard-coded in _coder.tpl:

- name: CODER_PROVISIONER_DAEMON_PSK
  valueFrom:
    secretKeyRef:
      name: {{ .Values.provisionerDaemon.pskSecretName | quote }}
      key: psk
{{- if include "provisioner.tags" . }}

This wasn't changed as part of this PR. I can add a separate PR to allow customizing the key name, if required.

@johnstcn johnstcn merged commit 413928b into main Oct 18, 2024
29 checks passed
@johnstcn johnstcn deleted the cj/helm-provisionerd-provisionerkey branch October 18, 2024 10:33
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2024
spikecurtis
spikecurtis reviewed Oct 21, 2024
View reviewed changes
helm/provisioner/tests/chart_test.go
expectedError: "",
},
{
name: "provisionerd_psk_and_key",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ever sensible to accept both? I think we can only accept one or other as the authentication credential

Copy link
Member Author

@johnstcn johnstcn Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, you're correct:

error: cannot provide both provisioner key --key and pre-shared key --psk

I can remove this test and add a check in the Helm chart, but I'd worry about logic drift.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@spikecurtis spikecurtis spikecurtis left review comments

@matifali matifali matifali approved these changes

@mtojek mtojek mtojek approved these changes

@ericpaulsen ericpaulsen Awaiting requested review from ericpaulsen

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnstcn @spikecurtis @matifali @mtojek