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

Add option to configure seLinux and runAs policies #538

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

AlbertoPimpo
Copy link
Contributor

@AlbertoPimpo AlbertoPimpo commented Aug 6, 2024

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

@AlbertoPimpo
Copy link
Contributor Author

Sorry for all the pushes but I had to fix the verification of the commit 😅

@AlbertoPimpo
Copy link
Contributor Author

@stevehipwell @naseemkullah @edsiper could you please take a look?

@stevehipwell
Copy link
Collaborator

@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.

@cosmo0920
Copy link

Maybe we need to ask to take a look this change @patrick-stephens ?
He sometimes do some stuffs of his technology interests of Openshift.

@AlbertoPimpo AlbertoPimpo force-pushed the patch-1 branch 2 times, most recently from 6c8f1ae to 23ef5c4 Compare August 15, 2024 12:32
@AlbertoPimpo
Copy link
Contributor Author

FYI you need to rebase and bump your chart version

@stevehipwell done 👍

@AlbertoPimpo
Copy link
Contributor Author

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

@patrick-stephens
Copy link
Contributor

I would not be a fan of changing it from one hardcoded value to another, let's make it configurable with a default.

@AlbertoPimpo
Copy link
Contributor Author

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.

@AlbertoPimpo
Copy link
Contributor Author

@patrick-stephens sorry for taking so long, it's done now. Feel free to merge it.

@AlbertoPimpo AlbertoPimpo changed the title Change Openshift scc seLinuxContext to RunAsAny Add option to configure seLinux and runAs policies Nov 10, 2024
@AlbertoPimpo
Copy link
Contributor Author

@stevehipwell @naseemkullah @edsiper this is ready to be merged.

Copy link
Collaborator

@stevehipwell stevehipwell left a 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?

@AlbertoPimpo
Copy link
Contributor Author

@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.

@stevehipwell
Copy link
Collaborator

@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.

@AlbertoPimpo AlbertoPimpo force-pushed the patch-1 branch 2 times, most recently from 5964d9e to 436f5b2 Compare November 23, 2024 14:36
@AlbertoPimpo
Copy link
Contributor Author

@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.

Thanks for the review @stevehipwell . I accepted your suggested changes, rebased and squashed everything in a single commit. We should be ready to merge.

@stevehipwell
Copy link
Collaborator

@AlbertoPimpo I can't see why the tests on your branch are failing.

Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehipwell stevehipwell merged commit 0451d01 into fluent:main Nov 25, 2024
2 checks passed
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