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

Add gid to worker, make uid/gid int #80

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Add gid to worker, make uid/gid int #80

merged 2 commits into from
Apr 24, 2024

Conversation

athornton
Copy link
Member

@athornton athornton commented Apr 24, 2024

If the auth system doesn't provide uid/gid for users (roughly speaking, if the auth system isn't LDAP-backed) the noteburst system needs to provide both: the Nublado controller needs both a uid and a gid in order to spawn a user.

At some time, uid became a string; according to the (unused, presumably) values-minikube.yaml for Noteburst it used to be an int. Since uid and gid are Unix account concepts, they're both properly ints rather than strings.

This PR implements that change in the code and test harness.

A different approach would be to drop those fields entirely, and just insist that the auth system do the work for us. This will work at IDF and USDF, anyway, which is probably all we really care about.

@athornton athornton requested a review from jonathansick April 24, 2024 15:23
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, thanks. I'm curious to know from @rra what might have changed in the system so that both uid and gid are necessary in non-LDAP systems when it wasn't before when we used GitHub auth?

@jonathansick
Copy link
Member

Please add a change log entry with scriv before merging.

@rra
Copy link
Member

rra commented Apr 24, 2024

Nublado v2 had a fallback where it blindly assumed the GID was the same as the UID if the Gafaelfawr data didn't contain a GID. When we rewrote that code for Nublado v3, I made the GID required to avoid baking that assumption into a layer of the stack that you wouldn't expect to be allocating GIDs.

I've been thinking about making Gafaelfawr require all users have primary GIDs so that the user creation would cleanly fail in Gafaelfawr, but have been holding off just because Nublado is the only service that cares and if people deploy Phalanx somewhere without a POSIX file system, they may not want to go to the work of creating a GID. Probably the right answer is to add a configuration parameter to Gafaelfawr to say whether a GID is required for a given environment.

@athornton athornton merged commit 8d11db5 into main Apr 24, 2024
4 checks passed
@athornton athornton deleted the tickets/DM-43454 branch April 24, 2024 22:43
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.

3 participants