Make use of hub.existingSecret sustainable #2042

consideRatio · 2021-02-19T03:43:08Z

Reimplementation of hub.existingSecret

Use of hub.existingSecret hasn't been a good experience for users because they end up needing to manually update it whenever they change something passed to the hub pod through values.yaml to be read in jupyterhub_config.py.

This PR is solving this by allowing hub.existingSecret be mounted in parallel to the chart managed k8s Secret resource we make use of and then merging values.

Closes #2009.

Documentation preview

Available here.

Breaking changes

Users of hub.existingSecrets need to recreate their secrets and read the docs on the changes.
Those using hub.services and referencing the api token from the chart managed k8s secret, need to reference hub.services.<service-name>.api_token going onwards instead of service.token.<service-name>

Implementation challenges

lookup of hub.existingSecret?
There are two exceptions to the general rule of merging the config from hub.existingSecret. These exceptions are for hub.db.password and hub.config.JupyterHub.proxy_auth_token, or hub.config.ConfigurableHTTPProxy.auth_token as the non-deprecated value is called.

These specific keys are exceptions as we need to know if they are declared in the chart managed or self managed k8s Secret during template rendering. This is because we reference one k8s Secret or the other as a source of an environment variable on the hub and/or proxy pod during template rendering. And, deciding this is a problem as I don't think we can or should rely on a lookup of a non-chart managed k8s resource. I'm actually not sure if this works or not, I just recall this to fail, but even if this recollection is wrong and it would work, it would be good to minimize lookup calls in general I think due to the unknown performance or ability to do so in all k8s clusters independent on various k8s Secret mechanisms in play.

I consider this challenge managed:

We document that the user need to specify hub.db.password within the user managed k8s Secret if they use an external database and hub.existingSecret.
We document that the user should kubectl edit the chart managed k8s secret in place if they want to update ConfigurableHTTPProxy.auth_token.

Implementation steps

Let hub.existingSecret mount in parallel to the defaultSecret in the hub pod.
Merge in existingSecret values with defaultSecret values, and let top level keys override defaultSecret values if they are defined. This can be handled in z2jh.py.
Create three separate _helpers-names.tpl templates and make use of them where appropriate.
- hub used for: volume declaration hub pod, mounting env var from JupyterHub.proxy_auth_token
- hub-existing-secret used for: volume declaration hub pod (conditional)
- hub-existing-secret-or-default used for: mounting of hub.db.password env var
Update configuration reference entry on hub.existingSecret
Changelog strategy defined: we simply reference docs in the configuration reference for whoever use hub.existingSecret.

Unrelated but part of PR

Replace references to JupyterHub.proxy_auth_token with ConfigurableHTTPProxy.auth_token as the previous is deprecated by JupyterHub. This won't matter but feels nice to do. This configuration value is indirectly set using an environment variable, both on the hub and proxy pod though, so perhaps it wouldn't even be a warning about it before or after.

meeseeksmachine · 2021-02-19T03:58:15Z

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-am-i-supposed-to-use-existingsecret/5097/5

minrk · 2021-02-19T15:51:06Z

jupyterhub/files/hub/jupyterhub_config.py

-c.JupyterHub.proxy_auth_token = get_secret_value("JupyterHub.proxy_auth_token")
+c.ConfigurableHTTPProxy.auth_token = get_secret_value(
+    "ConfigurableHTTPProxy.auth_token"
+)
 c.JupyterHub.cookie_secret = a2b_hex(get_secret_value("JupyterHub.cookie_secret"))
 c.CryptKeeper.keys = get_secret_value("CryptKeeper.keys").split(";")

 # load hub.config values, except potentially seeded secrets
 for section, sub_cfg in get_config("hub.config", {}).items():
    if section == "JupyterHub" and sub_cfg in ["proxy_auth_token", "cookie_secret"]:


What does this disallowed list do? To make sense, sub_config must always be a dict, so these sub_cfg in [list] conditions will always be False. Or am I misunderstanding something?

Ah yes this logic is just malfunctional because I did the mental mistake of thinking sub_cfg was string representing the sub config key name ("proxy_auth_token").

Resolved by fcea63f I think.

jupyterhub/templates/_helpers-names.tpl

minrk · 2021-02-19T15:54:06Z

jupyterhub/templates/hub/deployment.yaml

            {{- if .Values.hub.db.password }}
            {{- if eq .Values.hub.db.type "mysql" }}
            - name: MYSQL_PWD
              valueFrom:
                secretKeyRef:
-                  name: {{ include "jupyterhub.hub-secret.fullname" . }}
+                  name: {{ include "jupyterhub.hub-existing-secret-or-default.fullname" . }}


What if we set this env in juptyerhub_config instead of the pod? Could we get rid of this exceptional behavior?

Absolutely, If you think we can avoid that, it would be great! I has been assuming that we couldn't because it would be consumed by various tools outside our control.

Do you know how we would configure this in a way that covers all situations properly? Manipulating the connection string?

We can set os.environ in jupyterhub_config.py and it should work the same as setting it at the pod level. That will occur soon enough that it will affect the relevant targets (postgres/mysql imports later on in the same process), and also subprocesses, if there are any (there shouldn't be). Only init containers wouldn't get this env, but I don't think that's an issue, and they can always access the same secret if they need to.

So this should work in jupyterhub_config.py (with the appropriate ifs):

os.environ["MYSQL_PWD"] = values.db.password

Haha doh oh yes of course!!

Resolved in 3a78453!

manics · 2021-02-20T17:19:18Z

It sounds like there's loads of expertise hidden in here 😄

Is there somewhere we can document this knowledge (not necessarily in this PR)? The comments about supporting the use of Z2JH in other Helm charts, and how to configure it, sound particularly valuable to me.

consideRatio · 2021-02-20T23:53:07Z

The comments about supporting the use of Z2JH in other Helm charts, and how to configure it, sound particularly valuable to me.

@manics I created #2057!

consideRatio · 2021-02-21T04:29:55Z

Tests added that works! This may be ready for merge now,thank you @minrk and @manics for your review efforts!

Test shortcomings

We don't verify hub.db.password / setting of environment variables as we now set from jupyterhub_config.py. This is because changing hub.db.type which the logic relies on would interfere with hub function unless we also have such database server running.
Ignore the dev upgrade test failing, it will only do so for this PR but not for future PRs.

It seems that we error with 403 when accessing the jupyterhub api unless our hub.services.test.apiToken is set to something with at least 8 characters. No clue why. Perhaps 7 would be sufficient but 6 characters wasn't.

consideRatio · 2021-02-21T12:16:45Z

I made sure that anyone using hub.services together with hub.existingToken wouldn't experience anything unexpected. The k8s Secrets values are now the source of how JupyterHub configures the services api_tokens instead of the config values passed, that enables hub.existingSecret's value to matter.
I renamed various fields in the k8s secret to consistently use the chart configuration names the fields associate with.
- Motivation: to improve readability and avoid enable a future refactor where anything can be set directly based on the field name as a standalone key.
- Breaking change: anyone using hub.services and referencing the api token from the chart managed k8s secret, now need to reference hub.services.<service-name>.api_token going onwards instead of service.token.<service-name> as the key in the k8s secret is renamed.

Done!!!

jupyterhub/files/hub/jupyterhub_config.py

dev-config.yaml

minrk · 2021-02-25T08:34:41Z

Wonderful!

jupyterhub/zero-to-jupyterhub-k8s#2042 Merge pull request #2042 from consideRatio/pr/existing-secret-reimplemented

consideRatio · 2021-02-25T08:39:24Z

Thanks @minrk! https://readthedocs.org/ seems to be down btw if you also got an error message.

consideRatio added the enhancement label Feb 19, 2021

minrk reviewed Feb 19, 2021

View reviewed changes

consideRatio force-pushed the pr/existing-secret-reimplemented branch from d49b3e9 to 95408de Compare February 20, 2021 23:45

consideRatio mentioned this pull request Feb 20, 2021

docs: add section on using jupyterhub as a subchart #2057

Open

consideRatio force-pushed the pr/existing-secret-reimplemented branch 6 times, most recently from 2d535db to 3987f1e Compare February 21, 2021 04:16

consideRatio added 8 commits February 21, 2021 05:26

Make hub.existingSecret implementation sustainable

e3988ff

Fix deprecation of JupyterHub.proxy_auth_token

d91191d

fix: wrongly implemented logic to not load already loaded config

4a90cfa

Set SQL clients' env vars on hub startup (not through pod spec)

3a78453

refactor: extract a separate test_hub.py file from test_spawn.py

a96539c

ci: tweak user-scheduler's logLevel to avoid spamming logs

96d5eaa

ci: add foundation for hub.existingSecret test

17384de

ci: complete implementation of hub.existingSecret test

030f8ad

consideRatio force-pushed the pr/existing-secret-reimplemented branch from 3987f1e to 030f8ad Compare February 21, 2021 04:28

consideRatio requested a review from minrk February 21, 2021 05:27

Let JupyterHub services api_tokens rely on the k8s Secret's values

b739f95

consideRatio force-pushed the pr/existing-secret-reimplemented branch from 3ed6a6d to 1c721bb Compare February 21, 2021 09:18

consideRatio marked this pull request as draft February 21, 2021 09:20

consideRatio force-pushed the pr/existing-secret-reimplemented branch 2 times, most recently from 111f9d7 to 2cadaae Compare February 21, 2021 09:38

consideRatio force-pushed the pr/existing-secret-reimplemented branch from aa87a81 to 404921f Compare February 21, 2021 09:50

Let k8s Secret keys get the name matching the chart config name

6d1693b

consideRatio force-pushed the pr/existing-secret-reimplemented branch 2 times, most recently from d2a6fd8 to bc0a69c Compare February 21, 2021 11:27

ci: use 8+ characters for hub.services api_token!?

802a41b

It seems that we error with 403 when accessing the jupyterhub api unless our hub.services.test.apiToken is set to something with at least 8 characters. No clue why. Perhaps 7 would be sufficient but 6 characters wasn't.

consideRatio force-pushed the pr/existing-secret-reimplemented branch from bc0a69c to 802a41b Compare February 21, 2021 11:49

consideRatio added the breaking label Feb 21, 2021

consideRatio marked this pull request as ready for review February 21, 2021 12:11

minrk reviewed Feb 22, 2021

View reviewed changes

jupyterhub/files/hub/jupyterhub_config.py Outdated Show resolved Hide resolved

jupyterhub/files/hub/jupyterhub_config.py Outdated Show resolved Hide resolved

consideRatio added 2 commits February 22, 2021 16:16

Apply code review suggestion to correctly set db url

5f8fac2

Apply code review suggestion to use camelCase for consistency

b0716dd

minrk reviewed Feb 23, 2021

View reviewed changes

dev-config.yaml Show resolved Hide resolved

minrk merged commit 74abbb8 into jupyterhub:master Feb 25, 2021

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Feb 25, 2021

[jupyterhub] Automatic update for commit 0.11.1-n259.h74abbb81

922523d

jupyterhub/zero-to-jupyterhub-k8s#2042 Merge pull request #2042 from consideRatio/pr/existing-secret-reimplemented

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make use of hub.existingSecret sustainable #2042

Make use of hub.existingSecret sustainable #2042

consideRatio commented Feb 19, 2021 •

edited

Loading

meeseeksmachine commented Feb 19, 2021

minrk Feb 19, 2021

consideRatio Feb 20, 2021

minrk Feb 19, 2021

consideRatio Feb 20, 2021 •

edited

Loading

minrk Feb 20, 2021

consideRatio Feb 20, 2021

consideRatio Feb 21, 2021 •

edited

Loading

manics commented Feb 20, 2021

consideRatio commented Feb 20, 2021

consideRatio commented Feb 21, 2021 •

edited

Loading

consideRatio commented Feb 21, 2021 •

edited

Loading

minrk commented Feb 25, 2021

consideRatio commented Feb 25, 2021

Make use of hub.existingSecret sustainable #2042

Make use of hub.existingSecret sustainable #2042

Conversation

consideRatio commented Feb 19, 2021 • edited Loading

Reimplementation of hub.existingSecret

Documentation preview

Breaking changes

Implementation challenges

Implementation steps

Unrelated but part of PR

meeseeksmachine commented Feb 19, 2021

minrk Feb 19, 2021

Choose a reason for hiding this comment

consideRatio Feb 20, 2021

Choose a reason for hiding this comment

minrk Feb 19, 2021

Choose a reason for hiding this comment

consideRatio Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

minrk Feb 20, 2021

Choose a reason for hiding this comment

consideRatio Feb 20, 2021

Choose a reason for hiding this comment

consideRatio Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

manics commented Feb 20, 2021

consideRatio commented Feb 20, 2021

consideRatio commented Feb 21, 2021 • edited Loading

consideRatio commented Feb 21, 2021 • edited Loading

minrk commented Feb 25, 2021

consideRatio commented Feb 25, 2021

consideRatio commented Feb 19, 2021 •

edited

Loading

consideRatio Feb 20, 2021 •

edited

Loading

consideRatio Feb 21, 2021 •

edited

Loading

consideRatio commented Feb 21, 2021 •

edited

Loading

consideRatio commented Feb 21, 2021 •

edited

Loading