-
Notifications
You must be signed in to change notification settings - Fork 462
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 option to configure seLinux and runAs policies #538
Conversation
6a5ef04
to
6f256e1
Compare
Sorry for all the pushes but I had to fix the verification of the commit 😅 |
@stevehipwell @naseemkullah @edsiper could you please take a look? |
@AlbertoPimpo I'm not familiar enough with OpenShift to make a call on this change. FYI you need to rebase and bump your chart version. |
Maybe we need to ask to take a look this change @patrick-stephens ? |
6c8f1ae
to
23ef5c4
Compare
@stevehipwell done 👍 |
If it can help, in the PodSecurityPolicy you indeed allow to change the selinux context https://github.com/fluent/helm-charts/blob/main/charts/fluent-bit/templates/psp.yaml#L28 |
I would not be a fan of changing it from one hardcoded value to another, let's make it configurable with a default. |
@patrick-stephens I like the idea, maybe we should do the same for psp? I can take care of that. |
23ef5c4
to
2ac83e3
Compare
@patrick-stephens sorry for taking so long, it's done now. Feel free to merge it. |
@stevehipwell @naseemkullah @edsiper this is ready to be merged. |
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.
@AlbertoPimpo PSPs have not been supported in Kubernetes since 1.26
so I don't see any value in making changes there?
@stevehipwell It's a non-breaking change and a nice to have for the few users who are still stuck with old versions of kubernetes. Anyway, if this has to became a blocker for the PR, then I'll remove this part. |
@AlbertoPimpo could you please rebase, look at the comments I just made and also update the chart changelog annotations? As the PSP resource is still in the chart I can't see an issue with making this change. |
5964d9e
to
436f5b2
Compare
Signed-off-by: Alberto Pimpo <[email protected]>
436f5b2
to
a93369c
Compare
Thanks for the review @stevehipwell . I accepted your suggested changes, rebased and squashed everything in a single commit. We should be ready to merge. |
@AlbertoPimpo I can't see why the tests on your branch are failing. |
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.
LGTM
EDIT: after discussions, we decided to make seLinux and runAs policies configurable instead of changing the default.
When creating a securityContextContraint for Openshift, the helm chart currently set the selinux strategy to
MustRunAs
, but this might lead to permission errors if you are using selinux in the node and you want to mount an host folder with an HostPath.I think there is no reason for disallowing selinux context to be changed for the fluentbit deployment. Also, this is the same behavior used by red hat for their cluster logging operator that deploys fluentd.
https://github.com/openshift/cluster-logging-operator/blob/master/internal/auth/securitycontextconstraint.go#L46