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 security context with fallback #282

Closed
wants to merge 1 commit into from
Closed

Add security context with fallback #282

wants to merge 1 commit into from

Conversation

srudin
Copy link

@srudin srudin commented Jan 20, 2023

As discussed here: #281

This PR is adding the security context for all containers. It is possible to define the runAsUser / runAsGroup / fsGroup for every container individually. If nothing is set on a container a global value is used. If no global value is configured the root user is used (as is now).

I think this should be rather undisputed. ;-) Tested except I cannot set the root user in my environment so not sure if explicitely setting default 0 works (but would surprise me if not).

@srudin srudin mentioned this pull request Jan 20, 2023
@batpad
Copy link
Member

batpad commented Jan 20, 2023

This looks fantastic to me. Thank you @srudin .

Will give this a day to see if @Rub21 has any concerns. Also tagging in @yuvipanda since he knows more about these things than me in case there's any concerns with this approach.

@srudin let's give this a day for @Rub21 or @yuvipanda to leave any comments, else am good to approve and merge this 👍 thanks again!

@yuvipanda
Copy link

Instead of allowing overrides on a 'per-leaf' level, I would suggest allowing overrides of the entire securityContext object, like https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/23bf6a109fe9dd8fdabdbbaaa4c7d8abaf751951/jupyterhub/templates/scheduling/user-scheduler/deployment.yaml#L96 (or via | toJson on a single line to avoid having to count indents).

This allows admins to override any part of securityContext rather than just the 3 specified here, without requiring changes in the upstream code every time someone wants to customize something.

@srudin
Copy link
Author

srudin commented Jan 23, 2023

I see the benefit of that and I did it already at some other places. However I also think that the fallback per container -> full project is valuable as well and I didn't find a way to use the "with" syntax and the fallback "{{ value | default fallback }}" together. Do you know of a way to do that?

@srudin
Copy link
Author

srudin commented Jan 25, 2023

@yuvipanda Any feedback or examples regarding my question?

@batpad
Copy link
Member

batpad commented Jan 25, 2023

I didn't find a way to use the "with" syntax and the fallback "{{ value | default fallback }}" together.

@srudin do you have a bit more specific example of exactly what you were trying to do, what you tried, and what did not work?

@batpad
Copy link
Member

batpad commented Jan 25, 2023

It would be nice to do as @yuvipanda suggested, if that seems possible - allowing more flexible over-rides seems to make sense.

@srudin
Copy link
Author

srudin commented Jan 25, 2023

@batpad I didn't question that. But I think a fallback also makes sense and I only know how to do that with single values. So an example of the more flexible approach with a fallback would be appreciated.

@galewis2
Copy link

galewis2 commented Feb 6, 2023

Do we know which uids/gids work for Nominatim for example? With the current build, it looks like /app/ is owned by root, so for example, /app/address-levels.json cannot be downloaded

@yuvipanda
Copy link

@srudin if you are talking about how to set defaults, we put them in values.yaml like https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/46a632f5d07ba8520e5fb5e7539b3f7b89c5fcc7/jupyterhub/values.yaml#L88. But otherwise it's not entirely clear to me what the question is.

@srudin srudin closed this by deleting the head repository Sep 18, 2023
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