jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
541 stars 303 forks source link

Escape pod label `hub.jupyter.org/servername` (pod annotation remains unescaped) #694

Closed yuvipanda closed 1 year ago

yuvipanda commented 1 year ago

Otherwise, server names with spaces in them get the following error message:

HTTP response body:
{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Pod
\"jupyter-user-2dname--server-202\" is invalid: metadata.labels:
Invalid value: \"server 2\": a valid label must be an empty string or
consist of alphanumeric characters, '-', '_' or '.', and must start
and end with an alphanumeric character (e.g. 'MyValue',  or
'my_value',  or '12345', regex used for validation is
'(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')","reason":"Invalid","details":{"name":"jupyter-user-2dname--server-202","kind":"Pod","causes":[{"reason":"FieldValueInvalid","message":"Invalid
value: \"server 2\": a valid label must be an empty string or consist
of alphanumeric characters, '-', '_' or '.', and must start and end
with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or
'12345', regex used for validation is
'(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')","field":"metadata.labels"}]},"code":422}
yuvipanda commented 1 year ago

I think it would be an implementation detail.

consideRatio commented 1 year ago

If and how this label used, if at all? I figure if its internal, its not breaking, but if its not, it is.

We have a breaking change lined up anyhow if i recall correctly, so it will still be a major version bump next i think.

manics commented 1 year ago

Follow-up in https://github.com/jupyterhub/kubespawner/issues/695

yuvipanda commented 1 year ago

Annotations do not have requirements on what their values can be, so it is very helpful for debugging to have them be unescaped.

minrk commented 1 year ago

I can imagine a hypothetical situation where someone uses this label for something (e.g. a network policy that only affected servers with a particular name and that name would change under the updated escaping), but I would be very surprised if any exist in the wild. but I agree it should be considered an implementation detail and how we connect things like services to pods here.

The main thing we need labels for is things like selectors and routing. Everything else should be in annotations.

consideRatio commented 1 year ago

I updated the title so the breaking changelog entry becomes quite self-evident.

Annotations do not have requirements on what their values can be, so it is very helpful for debugging to have them be unescaped.

Yes I agree!

I was thinking that we could choose a new name for either the label or annotation so that the name https://hub.jupyter.org/servername isn't having different value in the label vs annotation. So I proposed some names that we could change either the label or the annotation to.

Let's get this merged anyhow.