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.

Sign up for GitHub

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

Jump to bottom

Add pod_security_context and container_security_context config #480

Merged
merged 12 commits into from Feb 11, 2021
Merged

Add pod_security_context and container_security_context config #480

merged 12 commits into from Feb 11, 2021

Conversation

cyrilcros
Copy link

@cyrilcros cyrilcros commented Feb 3, 2021 •
edited by consideRatio

This PR adds container_security_context and pod_security_context configuration options.

Old vs new options
The new options container_security_context and pod_security_context updates the partial security contexts built from the following older options:

  • supplemental_gids (pod)
  • fs_gid (pod)
  • privileged (container)
  • allow_privilege_escalation (container)
  • uid (container)
  • gid (container).

Related
Fixes #454 - pod/container security contexts can now be configured
Related to #478 - exposes fsGroupChangePolicy through pod security context, but doesn't provide a default value.

@cyrilcros cyrilcros force-pushed the security_context_dict branch 3 times, most recently from 558a802 to 74ad78b Compare February 3, 2021 16:27
@cyrilcros
Copy link
Author

@consideRatio OK this should be ready for review.

@cyrilcros cyrilcros mentioned this pull request Feb 3, 2021
Closed
@consideRatio
Copy link
Member

consideRatio commented Feb 3, 2021 •
edited

@cyrilcros I just saw your edit of splitting into pod_security_context and container_security_context and I like it!

I suggest you let whatever is set there override for example fs_gid if there is a conflict as well. I'm not 100% my motivation for this, but I think it makes sense related to us not having defaults here yet, and if someone specifies this it is with intent.

Overview of securityContext configuration and what KubeSpawner has exposed before this.

Pod specific

  • supplementalGroups (supplemental_gids)
  • fsGroup (fs_gid)
  • fsGroupChangePolicy
  • sysctls

Container specific

  • privileged (privileged)
  • allowPrivilegeEscalation (allow_privilege_escalation)
  • capabilities
  • procMount
  • readOnlyRootFilesystem

Common

  • runAsUser (uid)
  • runAsGroup (gid)
  • runAsNonRoot
  • seLinuxOptions
  • seccompProfile
  • windowsOptions

I hope to find time to review this within a week, but my attention is too split at the moment. I should really be doing something else right now as well. Thanks for your work @cyrilcros! ❤️

@cyrilcros
Copy link
Author

cyrilcros commented Feb 3, 2021 •
edited

  • I now add the old options like run_as_uid into a dict, update it with the new appropriate security context, and generate a kubernetes-client security context from it. None values are ignored as required.
  • At this point you will get a TypeError if you supply an illegal argument, which could be also due to something like fsGroupChangePolicy running on an earlier Kubernetes version (<1.18 for alpha). I added tests for that.

@cyrilcros cyrilcros force-pushed the security_context_dict branch 2 times, most recently from b9f9786 to c8246f8 Compare February 3, 2021 19:47
@consideRatio
Copy link
Member

consideRatio commented Feb 3, 2021 •
edited

@cyrilcros thank you for your work on this!!! :Tada:! 🥳

I'll find time to review/merge next week the latest.

@consideRatio consideRatio mentioned this pull request Feb 8, 2021
Merged
@consideRatio
Copy link
Member

@cyrilcros I'm actively working to get this PR to get merged but I've decided to wait for #483 to get merged before I finish some parts of this PR. Is it okay that I push some commits here?

@cyrilcros
Copy link
Author

@consideRatio Sure thing.

cyril.cros and others added 10 commits February 9, 2021 11:20
@consideRatio
Container and pod security contexts
f436450 
They have higher priority than existing options like run_as_uid.
TypeError happens if they are invalid. This can depend on current Kubernetes
version installed + kubernetes-client lib support.
@consideRatio
correct syntax
f5a721b
@consideRatio
Update inline comment about security context config
de77aca
@consideRatio
Code refactoring
5d1a7ef
@consideRatio
Made docstrings more explicit
b445dd8
@consideRatio
Pass configured security contexts to make_pod
2c858e8
@consideRatio
Accept k8s native configuration
b8dd426 
Instead of accepting the Python kubernetes-client's configuration, we
should accept it just as if it was k8s native config.
@consideRatio
Adjust to new make_pod param names
16e1747
@consideRatio
Refactoring / black autoformatting
1ca1d7d
@consideRatio
Tolerate anything in pod and container security context
2a0ffed 
The kubernetes-client can't be used to validate the security context
resources without compromising significantly on the allowed values to be
passed as the Python client is too outdated to know about modern
options.
@consideRatio consideRatio mentioned this pull request Feb 9, 2021
Closed
@consideRatio consideRatio changed the title Support for setting a security_context Add pod_security_context and container_security_context config Feb 9, 2021
@consideRatio
Copy link
Member

@cyrilcros thanks for your work on this 🎉 ❤️!

I consider this ready for review/merge @minrk @manics if you have time!

minrk
minrk reviewed Feb 11, 2021
View reviewed changes
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Some minor feedback on improving the error messages, but otherwise looks great to me. Thanks!

kubespawner/objects.py Outdated Show resolved Hide resolved
kubespawner/objects.py Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member

Thanks for the review @minrk! Resolved by 4661af8

minrk
minrk approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice!

@minrk minrk merged commit 941585f into jupyterhub:master Feb 11, 2021
@welcome
Copy link

welcome bot commented Feb 11, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@consideRatio
Copy link
Member

@cyrilcros thank you so much for your work on this!!! ❤️ 🎉

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/singleuser-pod-fails-after-1-0-0-upgrade-on-supplementarygroups-with-psp/9660/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minrk minrk minrk approved these changes

@consideRatio consideRatio Awaiting requested review from consideRatio

@manics manics Awaiting requested review from manics

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose traitlets to configure the pod's and the container's securityContext
4 participants
@cyrilcros @consideRatio @meeseeksmachine @minrk