Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

K8 dot unames #854

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

K8 dot unames #854

wants to merge 21 commits into from

Conversation

yhal-nesi
Copy link

Resolves the following issue #852 by sanitizing k8 namespace name. Deals with ".", "@", unicode, and converts upper case to lower case. Slug generator is based on JupyterHub code https://github.com/jupyterhub/kubespawner/blob/main/kubespawner/slugs.py and can also be used to sanitize other types of k8 objects.

The bootstrap script would also need to be modified to take advantage of that. https://github.com/OSC/ondemand/blob/master/hooks/k8s-bootstrap/k8s-bootstrap-ondemand.sh
Otherwise the change is not compatible.

Most of the work done by my colleague @DininduSenanayake.

@johrstrom
Copy link
Contributor

I think we're overthinking this. Why not just gsub replace the bad characters?

The bootstrap script would also need to be modified to take advantage of that. https://github.com/OSC/ondemand/blob/master/hooks/k8s-bootstrap/k8s-bootstrap-ondemand.sh

Another reason to simplify this. Can this script compute the same hash/various other calculations? No, but it could tr or sed search and replace.

@yhal-nesi
Copy link
Author

yhal-nesi commented Sep 4, 2024

I guess I am just trying to make it work in all cases. Let's say we replace "." by "-", and then firstname.lastname is going to become firstname-lastname. But what if there is already a user with that username? Same with upper case to lower case conversion etc. It is a bit contrived and you could say that if someone has a directory like that it is their problem.

Another reason to simplify this. Can this script compute the same hash/various other calculations? No, but it could tr or sed search and replace.

The way I was testing this is to call ruby inside a script. This needs rb file in the same directory, but there might be a way to call a library too? Not very familiar with ruby.

export SANITIZED_USERNAME=$(ruby -r /opt/ood-hooks/k8s-bootstrap/slug_generator.rb -e "puts(SlugGenerator.safe_slug '$ONDEMAND_USERNAME')")
export NAMESPACE="${NAMESPACE_PREFIX}${SANITIZED_USERNAME}"

@johrstrom
Copy link
Contributor

I guess I am just trying to make it work in all cases.

I'd rather have something simple that solves most cases we actually see in the wild. I think a simple gsub and tr would do this. Maybe we can query Etc for collisions?

For example, while can be a legitimate username on some systems, we'd never actually see it in practice.

I commend you on your diligence to cover all cases, but as a maintainer I need to balance simplicity with the solutions the code provides. Which is to say, I'd rather cover most cases we'll actually see with simple solutions than to have a complex one that covers a lot of cases we'll never actually encounter. (not to mention the distribution and upgrade problems this would bring)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants