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

Remove IPC_LOCK capability #198

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

pabrahamsson
Copy link
Contributor

Since we know we're deploying to k8s and are already disabling mlock (also see #80), is there a need for IPC_LOCK?
This also helps with deploying on for example Openshift as we now only need --set server.uid and --set server.gid for the ranges allowed by the restricted SCC. No more mucking around with SCCs and capabilities.

@tongpu
Copy link
Contributor

tongpu commented Feb 18, 2020

Always great to see that someone already created the PR I just wanted to start working on. 👍

@jasonodonnell The only way that I can envision that this could break existing deployments is if someone would have explicitly defined disable_mlock = false in either server.standalone.config or server.ha.config.

@tongpu
Copy link
Contributor

tongpu commented Feb 18, 2020

@pabrahamsson You probably should also remove the two test cases in test/acceptance/server-ha.bats and test/acceptance/server.bats

@pabrahamsson
Copy link
Contributor Author

pabrahamsson commented Feb 18, 2020

Thank you @tongpu, updated.
@jasonodonnell let me know if I should rebase

@tvoran tvoran added chart Area: helm chart enhancement New feature or request labels Mar 5, 2020
@karabijavad
Copy link
Contributor

this would also be nice for allowing deployment on eks + fargate

@pabrahamsson
Copy link
Contributor Author

@jasonodonnell is there anything I can do to help getting this PR merged?

@karabijavad
Copy link
Contributor

@pabrahamsson you would probably want to conditionally have the securityContext capability exist based on some .Values parameter

some people may want that security context field there

perhaps a .Values.mlock_enabled bool, defaulting to true, which adds the securityContext.capabilities element?

@pabrahamsson
Copy link
Contributor Author

@karabijavad while I can do what you suggest I'd like to understand the reasoning. The conditionals you're mentioning were already removed as part of #80. mlock is disabled by default (and can't be enabled through values.yaml) hence what's the purpose of IPC_LOCK? It sounds to me like either both mlock and IPC_LOCK are supported (through values.yaml) or both should be removed. Am I missing something?

@karabijavad
Copy link
Contributor

@pabrahamsson my apologies. i had no idea about that. youre right.

@spangenberg spangenberg self-requested a review April 2, 2020 12:44
@tvoran tvoran self-requested a review April 3, 2020 15:48
@spangenberg spangenberg removed their request for review April 7, 2020 15:10
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jasonodonnell jasonodonnell merged commit 497daa5 into hashicorp:master Apr 9, 2020
@lucaspouzac
Copy link

Hello, could you release new version with this fix please ?

Thanks!

radudd pushed a commit to radudd/vault-helm that referenced this pull request Jun 5, 2020
* Remove IPC_LOCK capability

* Remove tests for IPC_LOCK
amouat added a commit to amouat/wolfi-os that referenced this pull request Jun 2, 2023
Having setcap set on the binary broke upstream Helm chart compatability
due to hashicorp/vault-helm#198.

Signed-off-by: Adrian Mouat <adrian@chainguard.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart Area: helm chart enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants