- Notifications
You must be signed in to change notification settings - Fork925
fix(helm/provisioner): prefer provisioner key if both psk and key are set#15417
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -111,7 +111,7 @@ spec: | |||
| valueFrom: | |||
| secretKeyRef: | |||
| key: psk | |||
| name: coder-provisionerd-psk | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: renamed this to be clearer that it is not the default value
Uh oh!
There was an error while loading.Please reload this page.
| { | ||
| name: "provisionerd_key_psk_empty_workaround", | ||
| expectedError: "", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: adding an explicit test for the old workaround
6781b0f intomainUh oh!
There was an error while loading.Please reload this page.
… set (#15417)Relates to#15416This 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.(cherry picked from commit6781b0f)
Uh oh!
There was an error while loading.Please reload this page.
Relates to#15416
This PR modifies the provisioner helm chart logic when both PSK and key are set.
Previously, we would fail to install the chart.
Now we will set
CODER_PROVISIONER_DAEMON_KEYif bothkeySecretNameandpskSecretNameare set.Otherwise we will fall back to the existing behaviour of setting
CODER_PROVISIONER_DAEMON_PSK.This removes the requirement for a previous workaround of having to set
pskSecretNameto an empty string if usingkeySecretName/keySecretKey.UPDATE: after consulting with@spikecurtis I have modified the above logic slightly.
We will now prefer
CODER_PROVISIONER_DAEMON_KEYonly ifCODER_PROVISIONER_DAEMON_PSKis unchanged from its default value. Attempting to set both explicitly will still result in a failure to install the chart.Additionally, a separate 'gotcha' exists around
provisionerDaemon.tags-- specifying bothCODER_PROVISIONER_DAEMON_KEYandCODER_PROVISIONERD_TAGSwill result in the deployment failing to start. This is now detected upfront at helm install time.