PWB: Support defining sensitive config values via k8s secrets by pat-s · Pull Request #535 · rstudio/helm

pat-s · 2024-07-19T15:22:54Z

Applies to

launcherPem
secureCookieKey
userPassword
database.conf

When doing so, the rstudio-secret is skipped and individual mounts containing the above-mentioned secret values are injected.

This PR is breaking as the items listed are now maps and existing deployments would need to migrate from launcherPem: <value> to launcherPem.value: <value>.

I've tested this in a deployment of mine and it works as expected so far.

secureCookieKey:
  existingSecret: secure-cookie-key
launcherPem:
  existingSecret: launcher-pem
config:
  database:
    conf:
      existingSecret: database-conf
userPassword:
  existingSecret: workbench-user-password

The advantage of this approach is that it gives power to the users and makes use of "simple" k8s secrets instead of relying on some chart magic which puts together a bundled secret from plain config values.

related #520 #493

charts/rstudio-workbench/README.md

Co-authored-by: Graham Held <graham.held@gmail.com>

damienleger-cure51 · 2025-02-24T14:09:35Z

Hello, I would love to see this merged! 🙏

On my setup, helm chart is deployed through ArgoCD and it doesn't support helm lookup function. This lookup function is the one responsible for checking that launcher.pem+secureCookieKey exists, resulting of those secret values regenerated on every ArgoCD sync. Having those to be set only directly on values.yaml currently to fix this situation is a problem for sensitive value.

bschwedler · 2025-02-25T14:17:44Z

charts/rstudio-workbench/Chart.yaml

 name: rstudio-workbench
 description: Official Helm chart for Posit Workbench
-version: 0.8.7
+version: 1.0.0


@jforest We will probably want this to be a new minor version

Suggested change

version: 1.0.0

version: 0.9.0

@pat-s can you update this to 0.10.0 please?

jforest · 2025-02-25T15:19:33Z

hi @pat-s can you please merge a recent main into this? There have been a lot of changes since july, including the addition of helm unittests to run!

…ecret

samcofer · 2025-10-02T14:44:43Z

@jforest Can you take another look at this PR?

jforest

@pat-s You also need to update NEWS.md

We need to draw attention to the breaking changes. Things like global.secureCookieKey is no longer a string, and that string has moved to global.secureCookieKey.value

jforest · 2025-10-03T13:44:16Z

charts/rstudio-workbench/Chart.yaml

 name: rstudio-workbench
 description: Official Helm chart for Posit Workbench
-version: 0.8.7
+version: 1.0.0


@pat-s can you update this to 0.10.0 please?

charts/rstudio-workbench/templates/_helpers.tpl

jforest · 2025-10-03T14:12:22Z

charts/rstudio-workbench/templates/configmap-secret.yaml

+      {{- $launcherPem | nindent 6 }}
+      {{- end }}
+    {{- end }}
+    {{- if .Values.launcherPem.value }}


I think it makes more sense to remove the {{- end }} line above and make this into an {{- else if ... we don't want to end up with 2 launcher.pem items

jforest · 2025-10-03T14:14:55Z

charts/rstudio-workbench/templates/_helpers.tpl

+        key: password
+        name: {{ .Values.userPassword.existingSecret }}
+  {{- end }}
+  {{- if .Values.userPassword.value }}


I think you should also remove the {{- end }} line above here, and make this line {{- else if ... so we don't end up duplicating the env var if both are defined

jforest · 2025-10-03T14:15:28Z

charts/rstudio-workbench/templates/_helpers.tpl

    - name: rstudio-session-secret
      mountPath: {{ .Values.session.defaultSecretMountPath }}
    {{- end }}
+    {{- if or (not .Values.launcherPem.existingSecret) (not .Values.secureCookieKey.existingSecret) }}


this should probably match line 335

jforest · 2025-10-03T14:29:56Z

charts/rstudio-workbench/README.md

 | chronicleAgent.workbenchApiKey.valueFrom | object | `{}` | Workbench API key as a `valueFrom` reference (ex. a Kubernetes Secret reference) to set as the    `CHRONICLE_WORKBENCH_APIKEY` environment variable (recommended) |
 | command | list | `[]` | command is the pod container's run command. By default, it uses the container's default. However, the chart expects a container using `supervisord` for startup |
+| config.database.conf.value | string | 0644 | Database connection config |
+| config.database.conf.existingSecret | string | `""` | Secret for database connection config |


We should call out here that this is an either/or situation. That both will not be used, and that if both are defined we will ONLY use the existingSecret.

jforest · 2025-10-03T14:30:07Z

charts/rstudio-workbench/README.md

 | fullnameOverride | string | `""` | the full name of the release (can be overridden) |
-| global.secureCookieKey | string | `""` |  |
+| global.secureCookieKey.value | string | `""` |  |
+| global.secureCookieKey.existingSecret | string | `""` | Secret containing secureCookieKey |


We should call out here that this is an either/or situation. That both will not be used, and that if both are defined we will ONLY use the existingSecret.

jforest · 2025-10-03T14:30:12Z

charts/rstudio-workbench/README.md

-| launcherPem | string | `""` | An inline launcher.pem key. If not provided, one will be auto-generated. See README for more details. |
+| launcher.useTemplates | bool | `false` | whether to render and use templates in the job launching process |
+| launcherPem.value | string | `""` | An inline launcher.pem key. If not provided, one will be auto-generated. See README for more details. |
+| launcherPem.existingSecret | string | `""` | Existing secret containing launcherPem contents |


We should call out here that this is an either/or situation. That both will not be used, and that if both are defined we will ONLY use the existingSecret.

jforest · 2025-10-03T14:30:17Z

charts/rstudio-workbench/README.md

 | sealedSecret.enabled | bool | `false` | use SealedSecret instead of Secret to deploy secrets |
-| secureCookieKey | string | `""` |  |
+| secureCookieKey.value | string | `""` |  |
+| secureCookieKey.existingSecret | string | `""` | Secret containing secureCookieKey |


We should call out here that this is an either/or situation. That both will not be used, and that if both are defined we will ONLY use the existingSecret.

charts/rstudio-workbench/templates/_helpers.tpl

kaypeter87 · 2025-10-09T19:44:53Z

@jforest since the original author of this PR is inactive, I have created a new PR with the suggestions here: #723

pat-s · 2025-10-09T19:52:55Z

@kaypeter87 Not sure I understand, my latest commit here was 8 days ago 👀

kaypeter87 · 2025-10-10T14:30:16Z

@kaypeter87 Not sure I understand, my latest commit here was 8 days ago 👀

Apologies 😬 Misread the activity on the PR and didn't notice your commits last week.

This PR has been open over a year and I would love to see this merged so I created the PR. @jforest Let me know how I should proceed with my PR.

…cation

pat-s · 2025-10-10T15:17:05Z

@pat-s can you update this to 0.10.0 please?

Did so but it doesn't make sense from a semver-perspective. This change is clearly breaking and I don't know why it shouldn't result in a major version bump.

fh-albert-lee · 2025-10-15T15:40:34Z

Did so but it doesn't make sense from a semver-perspective. This change is clearly breaking and I don't know why it shouldn't result in a major version bump.

This project is still pre-1.0.0 so technically considered under "initial development". See https://semver.org/#spec-item-4:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Many projects in pre-1.0.0 follow this pattern (example, external-dns where the 0.19.0 release includes breaking changes).

pat-s · 2025-10-15T16:03:21Z

This project is still pre-1.0.0 so technically considered under "initial development". See semver.org#spec-item-4:

To me this general argumentation doesn't make sense as it just undermines all ideas of semver when doing so over many years - especially when a project/repo is in "active" use for many years already. Even more if its an enterprise product that should not be in "initial development" ever arguably. But I am not a maintainer, just a person sharing his thoughts.

(not meant offensive against you @fh-albert-lee, just took your comment as informative)

Anyhow, hope we can get this merged soon. Maintainers: feel free to adjust versions and changelog/README entries to your liking, this doesn't need to come from my side and there's no value in me solving Chart.yaml conflicts repeatedly.

jforest · 2025-10-27T13:53:59Z

In order for all the tests to run properly, we'll need to move this code into a different PR, one created in the repo. #726 is that PR. I'll close this one out to remove confusion

pat-s requested review from atheriel and colearendt as code owners July 19, 2024 15:22

pat-s added 6 commits November 7, 2024 19:45

allow setting launcherPem via secret

8649b5f

use individual secrets for sensitive vars

805edcb

also add secret for userPassword

c36a73c

document

248f6cd

clean

de66db1

bump chart version

a60473d

pat-s force-pushed the existingSecret branch from 7a7e480 to a60473d Compare November 7, 2024 18:49

pat-s requested review from a team, GCRev and zachhannum as code owners November 7, 2024 18:49

GCRev reviewed Nov 8, 2024

View reviewed changes

charts/rstudio-workbench/README.md Outdated Show resolved Hide resolved

Update charts/rstudio-workbench/README.md

d2254ac

Co-authored-by: Graham Held <graham.held@gmail.com>

jforest removed the request for review from colearendt February 24, 2025 17:49

bschwedler reviewed Feb 25, 2025

View reviewed changes

pat-s added 4 commits October 1, 2025 09:10

Merge branch 'main' into existingSecret

c0301e8

Merge branch 'existingSecret' of github.com:pat-s/helm into existingS…

e86f4f5

…ecret

adjust test

2a01d83

adjust deployment test

df98f0d

jforest requested changes Oct 3, 2025

View reviewed changes

kaypeter87 mentioned this pull request Oct 9, 2025

Posit Workbench: Support defining sensitive config values via k8s secrets #723

Closed

Merge branch 'main' into existingSecret

760c971

pat-s added 6 commits October 10, 2025 16:58

turn RSW_TESTUSER_PASSWD section into if else to avoid possible dupli…

216938d

…cation

align conditions for volumes with volumeMounts

081ae0b

update comment with new syntax

6ab8fbc

turn launcher.pem into if else and add missing .value

cf502e3

use ifelse for secureCookieKey secret definition

839ad2f

update value doc in readme

26829c5

jforest mentioned this pull request Oct 27, 2025

Copy of the PR from pat-s:existingSecret so it can run tests properly #726

Merged

jforest closed this Oct 27, 2025

Comments

Conversation

pat-s commented Jul 19, 2024

Uh oh!

Uh oh!

damienleger-cure51 commented Feb 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforest commented Feb 25, 2025

Uh oh!

samcofer commented Oct 2, 2025

Uh oh!

jforest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaypeter87 commented Oct 9, 2025

Uh oh!

pat-s commented Oct 9, 2025

Uh oh!

kaypeter87 commented Oct 10, 2025

Uh oh!

pat-s commented Oct 10, 2025

Uh oh!

fh-albert-lee commented Oct 15, 2025

Uh oh!

pat-s commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jforest commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pat-s commented Oct 15, 2025 •

edited

Loading