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

[YUNIKORN-2990] Add support for securityContext in the helm charts #184

Closed
wants to merge 2 commits into from

Conversation

pbacsko
Copy link
Contributor

@pbacsko pbacsko commented Nov 28, 2024

What is this PR for?

Add support for various securityContext settings in the template.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2990

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@pbacsko pbacsko self-assigned this Nov 28, 2024
@wilfred-s wilfred-s requested a review from manirajv06 December 4, 2024 05:15
Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

We should also add the same change to the admission controller deployment.

@pbacsko pbacsko requested a review from wilfred-s December 4, 2024 11:39
Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM +1
leaving open for others to comment on the changes

@pbacsko pbacsko closed this in 9ef8317 Dec 6, 2024
@josqu4red
Copy link

Could defaults be set to match the container images ?

@craigcondit
Copy link
Contributor

craigcondit commented Dec 20, 2024

The values are meant for overrides. If blank they will already default to the container settings. As YuniKorn doesn't read or write any files, the actual values don't really matter much. However, setting security context is convenient in environments where this is required for policy reasons.

@josqu4red
Copy link

josqu4red commented Dec 20, 2024

Right. I planned to submit this change for policy reasons (but stopped at the Jira step). Good to have it now 👍

@craigcondit
Copy link
Contributor

I personally would rather not encode this (or any other specific values) into the default helm chart. Security context is something each environment policy differs on. It's not possible to construct one that will satisfy every policy. The default values are already secure; this PR simply allows setting them explicitly if required in certain environments.

@josqu4red
Copy link

josqu4red commented Dec 20, 2024

Yes that makes sense.
Aside from this it would have been nice to reuse the values hierarchy (e.g .Values.admissionController.podSecurityContext instead of .Values.admissionPodSecurityContext etc.) don't you think?

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