-
Notifications
You must be signed in to change notification settings - Fork 576
charts: Permit setting hostUsers for deployment #4371
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jcpunk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for configuring the Kubernetes hostUsers field in Headlamp pod specifications, allowing users to opt into user namespace isolation for enhanced security. The default value of true maintains backward compatibility by keeping the current behavior (user namespaces disabled).
Changes:
- Added
hostUsersconfiguration flag to values.yaml with a default value oftrue - Updated deployment template to include the hostUsers field in the pod spec
- Updated all 19 test expected output files to include the new hostUsers field
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| charts/headlamp/values.yaml | Adds new hostUsers configuration field with default value of true and minimal documentation |
| charts/headlamp/templates/deployment.yaml | Incorporates hostUsers value from configuration into pod spec |
| charts/headlamp/tests/expected_templates/*.yaml | Updates all test expected outputs to include hostUsers: true in pod specs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c8ba436 to
1b3df72
Compare
|
In theory, copilot's review has been addressed. |
illume
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some docs for this to the charts/README.md ?
I'm wondering what the impact will be on existing users of the charts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I've updated the documentation. Since it defaults to the current kubernetes default setting, I don't think existing users will notice any change. It will start letting folks opt-in to user namespaces. |
|
@jcpunk thanks for that. Are you able to see about the github check failure? make helm-template-test |
Signed-off-by: Pat Riehecky <[email protected]>
|
perhaps that is better |
Summary
Adds a flag for folks to opt into user namespaces.
Changes
Steps to Test
Notes for the Reviewer
https://kubernetes.io/docs/concepts/workloads/pods/user-namespaces/