add 'safe' slug scheme #744

minrk · 2023-06-08T11:01:04Z

closes #737
closes #498

implements scheme described here.

always produces valid values
uses names, etc. as-is, when valid (no more -2d)
uses stripped name---hash when needed
new behavior is enabled by default, opt-out for backward compatibility via KubeSpawner.slug_scheme = 'escape'

Potential issues to address:

escaping is applied to the final template result, rather than individual strings. This ensures correct values, but means the hash is a function of the full template string, not just the username (or servername). Dealing with length is tricky if we need to hash bits at a time.
Not everything is a kubernetes field, e.g. working directory should probably get special handling? It has different rules than the label fields
a breaking change for users to change schemes (just like changing the template config), but at least we don't make the change for them

- always produces valid values - uses names, etc. as-is, when valid - uses stripped name--hash when needed

minrk · 2023-06-08T11:04:23Z

kubespawner/slugs.py

+    return True
+
+
+def is_valid_object_name(s):


This function ended up not being used because instead of multiple is_valid args, is_valid_default is used, which validates the subset of object names and labels (same as object name with max length of 63 instead of 255)

Ph0tonic · 2024-01-16T14:06:11Z

Hi,
Any status on this PR ? It would be great to have this merged, can I help with it ?
Thanks

Ph0tonic

A few suggestions an questions.

kubespawner/spawner.py

Ph0tonic · 2024-04-09T12:05:03Z

Hi,
Any status update on this PR ? May I help with anything ?
\cc @minrk

consideRatio · 2024-05-04T20:17:47Z

@minrk I looked at this in order to help nudge it towards merge, but I see that its a very breaking PR to enable this for an existing hub as for example pvc_name is gets changed.

        if self.enable_user_namespaces:
            self.namespace = self._expand_user_properties(self.user_namespace_template)
        self.pod_name = self._expand_user_properties(self.pod_name_template)
        self.dns_name = self.dns_name_template.format(namespace=self.namespace, name=self.pod_name)
        self.secret_name = self._expand_user_properties(self.secret_name_template)
        self.pvc_name = self._expand_user_properties(self.pvc_name_template)
        if self.working_dir:
            self.working_dir = self._expand_user_properties(self.working_dir)

The changes to the labels are far less breaking as it won't affect kubespawner/z2jh/binderhub/grafana-dashboards etc - and only be breaking for external software considering the labels.

I'm not landing in a concrete action point here, just summarizing this realization.

minrk · 2024-05-06T13:55:46Z

@consideRatio yeah, I think that's exactly why I didn't finish this. I think the new scheme is fine and works better, but figuring out the transition is the hard part.

minrk · 2024-06-12T07:57:50Z

I'm going to try to pick this one up with #835 / #836 in mind as the transition path:

leave old labels as they are by default
make old labels opt-out
use safe scheme unconditionally in new labels
use new labels in all selectors, don't rely on old labels for anything

The hard part is still going to be transitioning things like PVC, pod name, etc. We need to make sure that once established, they don't change even if the scheme changes. I think this is true already for pod name. Persisting chosen values in Spawner state should do this for us, but will need to think it through and make sure everything is where it's supposed to be, and everything is persisted that should be. Looking at it, unfortunately pvc name is not persisted, so that's going to be hard.

We may have to keep both schemes alive and check at runtime:

if new name exists:
   use new name
else:
  if old name exists:
    use old name, warn
  else:
    use new name

It may turn out that the best we can do is a breaking change with a documented manual migration step.

PVCs can be renamed through the somewhat frightening process of:

get pv id from pvc
set volume retention to Retain
delete old pvc
patch claimRef on volume
create new pvc with specified volumeName

or use krew rename-pvc (which does the same thing, but has the benefit of being written by not us).

for more information, see https://pre-commit.ci

- safe scheme by default - preserve old scheme if found - persist pvc_name and attempt to handle lack of persistence from before - add handle_legacy_names and use_legacy_labels configuration for legacy labels

yuvipanda · 2024-06-12T22:04:38Z

Thank you for picking this back up, @minrk!

consideRatio · 2024-06-13T07:46:54Z

Thank you for working this @minrk!!

These points may already be known and considered, but i wanted to highlight it when considering the paths onwards just in case.

A external resource complexity to be aware of

A key complexity for any change to username expansion is external resources, such as just creating a volume mount on the pod with subPath including username. We can't know if the external nfs volume includes the old username expansion or not, so we can't determine if its ok to switch over or not etc.

Kubespawner itself only use component label i think

I think the only use of labels currently by kubespawner itself is to reduce the set of resources to look at by the reflector using the component label. A performance aspect, because then the actual resource names are looked up among that set.

minrk · 2024-06-13T08:41:20Z

Thanks for the notes!

I think the only use of labels currently by kubespawner itself is to reduce the set of resources to look at by the reflector using the component label.

Close. I think there's one more place: when services are enabled, labels are also used to route service traffic to the pod, which also uses the full label set (I'm not sure that's the right choice, but it should at least include user and server names).

Now that this mostly works, I think there are some problems in the switch to handling escape of whole strings at a time:

variable expansion is done all over the place, with wildly different rules:
- pod/pvc_name use object naming rules
- labels have different rules (it's fine to use a strict subset of objects and labels for both)
- subdir mount paths (totally different)
- pod_connect_ip where hostname rules apply, the pod name may want to be used, but the rest of the string definitely must not be escaped
some strings should include other strings, but whole-string escaping makes that impossible unless all expanded results can also be referenced by name:
- pod_name_template "jupyter-{username}--{servername}" -> jupyter-escape--server--abc123
- pod_connect_ip "jupyter-{username}--{servername}.svc.local" -> jupyter-escape--serversvclocal--def456 which should be jupyter-escape--server--abc123.svc.local

That said, we cannot guarantee correct strings if we don't handle the full slug. I think it's simply not feasible to have one expand_user_properties that's used all over the place for pod names, URLs, templates, paths and have it produce correct, consistent results.

I don't really know how to address this. I think it only makes sense to apply the 'safe' slug scheme to kubernetes resource names and labels, and not in other contexts. It usually doesn't make sense to apply the escaping to mounts or subdirectories, but sometimes it will. The rules, however, are completely different and not really knowable. Applying the new scheme only to resource names

I'm going to give this a read through, and instead of changing what expand_user_properties does globally, I'm going to try to:

only apply the new scheme to k8s resource names and labels (drastically reduce potential impact of the change)
make pod_name (or perhaps dedicated {k8s_name}) available in the template namespace, so it can be referenced by other template strings without modification

yuvipanda · 2024-06-13T15:48:06Z

Thank you for diligently working through this hard problem, @minrk.

Let's bump a major version whenever we release this.

no need to make legacy component labels optional

minrk · 2024-06-17T08:40:47Z

Getting closer, but still tricky stuff to think about:

object names should be safe by default
it should be possible to force the old scheme for consistency (either a single global flag, or new template fields that keep the old value)
it should be possible to reference created resources where object rules may not apply (add pod_name, pvc_name, etc. to template namespace)
we should not lose track of pvcs across upgrade (pvc_name should have been in get_state all along)
applying appropriate context-dependent rules in blackboxes like in _expand_all is not feasible, so per-field escaping is really the only way to go

The {username}--{servername} collision problem changes slightly depending on how/when we apply the escape because e.g. username="üser", servername="" produces 'ser--73506260' which can also be produced with username="ser", servername="73506260", so we would need to pick a different delimiter and/or hash joiner to avoid that.

- add user_server to template namespace - add `safe_` prefixed fields to template namespace using new 'safe' slug scheme - add pod_name, namespace, pvc_name to template namespace

…-hash)

minrk · 2024-06-18T13:51:36Z

I think I'm narrowing it down to a scheme we can safely and comprehensibly switch to. I mainly need to hash out the transition process and write out some tests and docs for how that should work. In all cases, current behavior can be preserved unchanged explicitly by replacing {username} with {escaped_username} in templates, which will continue to be the old-style value with no plans to change. There is a new {safe_username} that is escaped with the new scheme, and a new {user_server} field that is the join of {username}--{servername} and can be used anywhere a unique slug for a given user+server should be used.

In general, we have:

username, servername, user_server - value determined by slug_scheme. Currently defaults to the new safe. Always match exactly one of:
ecaped_ variants - current scheme with escapism. Not guaranteed to produce valid values, especially for labels. {escaped_user_server} is identical to {escaped_username}--{escaped_servername} to preserve backward compatibility
safe_ variants - new, safe scheme that always produces a safe label. safe_user_server is not always identical to {safe_username}--{safe_servername}, but it is in the situations where neither needs any escaping, which is most of the time.
pod_name, pvc_name, and namespace are available to reference in templates (I'm mostly thinking of extra_ resources that may refer to them).

I've also removed the config to make the labels in #835 optional, which I don't think we need as covered by @yuvipanda's comments over there.

I feel like I am getting a good handle on resource names (pod_name, pvc_name), but figuring out the right thing to do for work_dir is a good deal more complex. Part of the trouble is that resource names are a safer transition because we can check if a resource at the old name exists and use it. We can also use state persistence to keep things sticky across config changes. We can't check if a filesystem path exists in time to make a choice.

Filesystems also tend to have very different, much more permissive, rules than k8s object names, but they can vary quite a bit! So what should we do for work_dir?

use new scheme and document how to keep old scheme with template config
continue to use old scheme by default, because it's much less likely to have caused any problems for paths
somehow try to identify if old scheme has been used, and use new if not

manics · 2024-06-18T15:02:46Z

If it helps we can potentially force admins to make a deliberate decision when upgrading Z2JH. If we add a new Z2JH config value to the schema and deliberately set it to an invalid value in values.yaml an admin must set the config. It's inconvenient for anyone expecting a seamless upgrade, but the schema validation should happen before anything is deployed so it shouldn't break anything.

- upgrade from 6.x after default slug scheme change preserves pvc attachment - old name check doesn't happen if loaded state from more kubespawner 7

minrk · 2024-06-26T13:31:57Z

I wouldn't call this "done" by any means, but think it's now at least "complete" in that I have a pretty good understanding of what's changed, what works on upgrade and what requires user intervention, it's documented more thoroughly than before, and the new upgrade logic is tested and explained.

So I think this is ready for review!

consideRatio · 2024-07-20T20:33:53Z

Amazing effort into this @minrk!!! I'm looking to help drive review effort and one of the key things I knew I wanted to look into was how this affected providing user storage via NFS, so this comment got dedicated to that as a first step.

NFS storage considerations

When using NFS storage, one may have a single PVC configured with a subPath referencing "{username}".

Click to see example z2jh / kubespawner config

This is z2jh Helm chart config:

jupyterhub:
  singleuser:
    storage:
      type: static
      static:
        pvcName: home-nfs
        subPath: "{username}"

Via a z2jh provided jupyterhub_config.py, this leads to KubeSpawner config:

c.KubeSpawner.volumes = [
    {
        "name": "home",
        "persistentVolumeClaim": {
            "claimName": "home-nfs",
        },
    },
],
c.KubeSpawner.volume_mounts = [
    {
        "mountPath": "/home/jovyan",
        "name": "home",
        "subPath": "{username}",  # this is whats important
    },
]

In such situations, {username} have been the same as {escaped_username}, but will transition to being {safe_username} KubeSpawner v7. This will lead to mounting a new folder for usernames with characters including for example: ., -, @, or capital letters.

A quick but essential config change before upgrading to z2jh v4 / kubespawner v7 would be to do a change like this:

jupyterhub:
  singleuser:
    storage:
      type: static
      static:
        pvcName: home-nfs
-        "subPath": "{username}"
+        "subPath": "{escaped_username}"

For new hubs though, it would make sense to do "subPath": "{username}". The z2jh default of singleuser.storage.static.subPath is "{username}", so for z2jh this is a key breaking change to point out.

Action points

z2jh v4 including kubespawner v7 need to highlight use of singleuser.storage.type=static need to adjust subPath to "{escaped_username}" for existing deployments to avoid re-creating users' home directories

docs/source/templates.md

consideRatio · 2024-07-20T21:19:58Z

Migration consideration notes

In case one wants to migrate some resource, for example NFS storage folder names, one may need to identify how {username} has changed for a set of users before and after. This comment explores some steps one could need to take in such migration.

Expand to read more

Detecting if `{username}` changed between v6 and v7 for specific usernames

import string

from escapism import escape
from kubespawner.slugs import safe_slug

test_username = "some-unescaped-username"

escaped_safe_chars = set(string.ascii_lowercase + string.digits)
escaped_username = escapism.escape(test_username, safe=escaped_safe_chars, escape_char='-').lower()
safe_username = safe_slug(test_username)
if safe_username != escaped_username:
    print(f"safe_username != escaped_username: {safe_username} != {escaped_username}")

Getting all usernames via JupyterHub API

import requests

def iter_usernames(hub_url, token):
    """generator yielding all usernames for a given hub

    requires a token with `list:users` scope.
    """
    next_url = hub_url + "/hub/api/users"
    s = requests.Session()
    s.headers["Authorization"] = f"Bearer {token}"
    s.headers["Accept"] = "application/jupyterhub-pagination+json"
    while next_url:
        print(f"Fetching {next_url}")
        r = s.get(next_url)
        r.raise_for_status()
        page = r.json()
        for user in page["items"]:
            yield user["name"]
        if page["_pagination"]["next"]:
            next_url = page["_pagination"]["next"]["url"]
        else:
            next_url = None

Creating a map of old -> new `{username}` expansion

TODO

consideRatio · 2024-07-21T11:44:19Z

kubespawner/spawner.py

+    handle_legacy_names = Bool(
+        True,
+        config=True,
+        help="""handle legacy names and labels
+
+        kubespawner 7 changed the scheme for computing names and labels to be more reliably valid.
+        In order to preserve backward compatibility, the old names must be handled in some places.
+
+        You can safely disable this if no PVCs were created or running servers were started
+        before upgrading to kubespawner 7.


The config handle_legacy_names is only used to handle a transition to safe name expansions for PVCs, not for anything else like the leading help text indicates with handle legacy names and labels.

I think updating this to describe its just about managing the PVC naming makes sense rather than trying to handle anything beyond the PVC naming.

Yes, I intended to have this for a catch-all config "check if the upgraded defaults might have detached something without user input," but PVCs were the only thing I found where there was something to be done. Maybe namespace should be handled as well?

namespaces don't need to be handled because they were persisted in 6.x, so I think this is okay as-is.

kubespawner/spawner.py

consideRatio · 2024-07-31T08:34:53Z

I chatted with @minrk about this just now, here is some planning made explicit:

@minrk does a final pass on this PR, possibly with no changes
We go for a kubespawner 7 beta release
We bump to kubespawner 7 beta in z2jh to get a dev release out
I'll do some test upgrades to verify things seem ok
- test the proposed migration in add 'safe' slug scheme #744 (comment)
- test a user with username something.something
- test a user with a pvc created already
  EDIT: I've not done this, and I'm giving up trying to get it done - but users have tested this it seems via z2jh beta with a kubespawner beta, and it seems OK! There were an issue for a user that had some custom logic in a hook, but besides that it no users have reported issues about this.
- test a user with a pvc not created already
  EDIT: Same note as the PVC test above.

Notes fot he future:

z2jh changelog/migration notes
- see a specific migration step in the end of add 'safe' slug scheme #744 (comment)
- script to look at usernames etc considered in add 'safe' slug scheme #744 (comment) considered

since it is an attempt to 'remember' an old name from kubespawner 6 which never remembers

minrk · 2024-07-31T13:44:00Z

I determined that handle_legacy_names should indeed be subordinate to remember_pvc_name, since it is itself an extended implementation of remembering the previously used pvc_name. Both are true by default.

minrk · 2024-08-01T06:53:00Z

Naturally, tests have started to fail for unrelated reasons on the last commit. Looking into it.

minrk · 2024-08-01T08:10:24Z

Fixed tests with #846

consideRatio · 2024-08-01T18:16:13Z

Thank you for amazing effort into this @minrk!!

I'm on vacation for two weeks now, but hope to find time to contribute towards release of beta and testing anyhow

add 'safe' slug scheme

7bac57c

- always produces valid values - uses names, etc. as-is, when valid - uses stripped name--hash when needed

minrk commented Jun 8, 2023

View reviewed changes

yuvipanda mentioned this pull request Sep 7, 2023

Server names starting with upper letter fail to start #737

Closed

Ph0tonic reviewed Jan 16, 2024

View reviewed changes

kubespawner/spawner.py Outdated Show resolved Hide resolved

kubespawner/spawner.py Outdated Show resolved Hide resolved

kubespawner/spawner.py Outdated Show resolved Hide resolved

kubespawner/spawner.py Show resolved Hide resolved

manics mentioned this pull request Apr 24, 2024

Add modern app.kubernetes.io labels alongside old #835

Merged

consideRatio mentioned this pull request Apr 28, 2024

Create new labels that we can switch over to later #836

Open

consideRatio added breaking new and removed breaking labels May 4, 2024

Ph0tonic mentioned this pull request May 6, 2024

Planning for release 4.0 with JupyterHub 5 jupyterhub/zero-to-jupyterhub-k8s#3406

Open

4 tasks

minrk and others added 2 commits June 12, 2024 09:58

Merge branch 'main' into wip-slug

d6be9f5

[pre-commit.ci] auto fixes from pre-commit.com hooks

bdf0f37

for more information, see https://pre-commit.ci

minrk mentioned this pull request Jun 12, 2024

Fix #761 expand pvc_name outside of init #820

Open

toward compatible transition for safe slugs

a1b524d

- safe scheme by default - preserve old scheme if found - persist pvc_name and attempt to handle lack of persistence from before - add handle_legacy_names and use_legacy_labels configuration for legacy labels

Merge from main

c0b79af

revert use_legacy_labels config

058afd5

no need to make legacy component labels optional

minrk added 2 commits June 17, 2024 11:17

let safe slug scheme live side-by-side with escape scheme

6968b68

- add user_server to template namespace - add `safe_` prefixed fields to template namespace using new 'safe' slug scheme - add pod_name, namespace, pvc_name to template namespace

add multi_slug mechanism for multi-word slugs (username--servername--…

45299ce

…-hash)

exercise pvc_name upgrade cases

f96ab38

- upgrade from 6.x after default slug scheme change preserves pvc attachment - old name check doesn't happen if loaded state from more kubespawner 7

minrk changed the title ~~[WIP] add 'safe' slug scheme~~ add 'safe' slug scheme Jun 26, 2024

minrk marked this pull request as ready for review June 26, 2024 12:51

Fix markdown table formatting

09ade1c

consideRatio reviewed Jul 20, 2024

View reviewed changes

docs/source/templates.md Outdated Show resolved Hide resolved

Document escaped_username and escaped_servername to be added in v7

10ba1fb

consideRatio reviewed Jul 21, 2024

View reviewed changes

kubespawner/spawner.py Outdated Show resolved Hide resolved

minrk added 3 commits July 30, 2024 14:07

clearer comment about values being loaded by get_state

d4c2308

Merge branch 'main' into wip-slug

f44b178

remove hardcoded safe slug scheme from namespace

97399f0

minrk commented Jul 31, 2024

View reviewed changes

kubespawner/spawner.py Show resolved Hide resolved

consideRatio mentioned this pull request Jul 31, 2024

KubeSpawner 7 beta review/release effort 2i2c-org/infrastructure#4539

Closed

only handle legacy pvc name when remember_pvc_name is true

6232c4b

since it is an attempt to 'remember' an old name from kubespawner 6 which never remembers

minrk mentioned this pull request Aug 1, 2024

use sync client in test_objects #846

Merged

Sync with main

845f3d8

consideRatio approved these changes Aug 1, 2024

View reviewed changes

consideRatio merged commit f77cfc3 into jupyterhub:main Aug 1, 2024
9 checks passed

minrk mentioned this pull request Sep 2, 2024

Updating KubeIngressProxy to new slug scheme (or not) #849

Open

consideRatio mentioned this pull request Sep 5, 2024

Planning release of v7 #850

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add 'safe' slug scheme #744

add 'safe' slug scheme #744

minrk commented Jun 8, 2023 •

edited

Loading

minrk Jun 8, 2023

Ph0tonic commented Jan 16, 2024

Ph0tonic left a comment

Ph0tonic commented Apr 9, 2024

consideRatio commented May 4, 2024 •

edited

Loading

minrk commented May 6, 2024

minrk commented Jun 12, 2024

yuvipanda commented Jun 12, 2024

consideRatio commented Jun 13, 2024

minrk commented Jun 13, 2024 •

edited

Loading

yuvipanda commented Jun 13, 2024

minrk commented Jun 17, 2024

minrk commented Jun 18, 2024 •

edited

Loading

manics commented Jun 18, 2024

minrk commented Jun 26, 2024

consideRatio commented Jul 20, 2024 •

edited

Loading

consideRatio commented Jul 20, 2024 •

edited by minrk

Loading

Detecting if `{username}` changed between v6 and v7 for specific usernames

Getting all usernames via JupyterHub API

Creating a map of old -> new `{username}` expansion

consideRatio Jul 21, 2024 •

edited

Loading

minrk Jul 30, 2024

minrk Jul 31, 2024

consideRatio commented Jul 31, 2024 •

edited

Loading

minrk commented Jul 31, 2024

minrk commented Aug 1, 2024

minrk commented Aug 1, 2024

consideRatio commented Aug 1, 2024

add 'safe' slug scheme #744

add 'safe' slug scheme #744

Conversation

minrk commented Jun 8, 2023 • edited Loading

minrk Jun 8, 2023

Choose a reason for hiding this comment

Ph0tonic commented Jan 16, 2024

Ph0tonic left a comment

Choose a reason for hiding this comment

Ph0tonic commented Apr 9, 2024

consideRatio commented May 4, 2024 • edited Loading

minrk commented May 6, 2024

minrk commented Jun 12, 2024

yuvipanda commented Jun 12, 2024

consideRatio commented Jun 13, 2024

minrk commented Jun 13, 2024 • edited Loading

yuvipanda commented Jun 13, 2024

minrk commented Jun 17, 2024

minrk commented Jun 18, 2024 • edited Loading

manics commented Jun 18, 2024

minrk commented Jun 26, 2024

consideRatio commented Jul 20, 2024 • edited Loading

NFS storage considerations

Action points

consideRatio commented Jul 20, 2024 • edited by minrk Loading

Migration consideration notes

Detecting if {username} changed between v6 and v7 for specific usernames

Getting all usernames via JupyterHub API

Creating a map of old -> new {username} expansion

consideRatio Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

minrk Jul 30, 2024

Choose a reason for hiding this comment

minrk Jul 31, 2024

Choose a reason for hiding this comment

consideRatio commented Jul 31, 2024 • edited Loading

minrk commented Jul 31, 2024

minrk commented Aug 1, 2024

minrk commented Aug 1, 2024

consideRatio commented Aug 1, 2024

minrk commented Jun 8, 2023 •

edited

Loading

consideRatio commented May 4, 2024 •

edited

Loading

minrk commented Jun 13, 2024 •

edited

Loading

minrk commented Jun 18, 2024 •

edited

Loading

consideRatio commented Jul 20, 2024 •

edited

Loading

consideRatio commented Jul 20, 2024 •

edited by minrk

Loading

Detecting if `{username}` changed between v6 and v7 for specific usernames

Creating a map of old -> new `{username}` expansion

consideRatio Jul 21, 2024 •

edited

Loading

consideRatio commented Jul 31, 2024 •

edited

Loading