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 securityContext and podSecurityContext in Helm Chart #289

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

lepouletsuisse
Copy link
Contributor

Hey,
First, thank you for the fork, this tool is really useful for my company.

We are using Helm Chart at my company and we are currently improving the security in our Kubernetes cluster.
In order to reach our goal, we need to modify the securityContext of all pods and containers present in our cluster. Unfortunately, Elastalert2 didn't offer the possibility to set these values using Helm Chart. I did the modification on a fork and thought other people from the community could use this feature.
I think I changed everything I had to change according to your Contribution Guideline but if I forgot something, please let me know and I'll fix my PR.

@jertel
Copy link
Owner

jertel commented Jun 21, 2021

Hello! Please double check the chart readme -- there is an existing podSecurityPolicy.create setting that can be enabled. If this is insufficient please provide more information so we can improve the default policy for those that enable it.

@lepouletsuisse
Copy link
Contributor Author

lepouletsuisse commented Jun 21, 2021

Hello! Please double check the chart readme -- there is an existing podSecurityPolicy.create setting that can be enabled. If this is insufficient please provide more information so we can improve the default policy for those that enable it.

I don't think PodSecurityPolicy can be used to specify information in pods.

I may be wrong but according to this link, PSPs are cluster-level tool used as a gatekeeper to deny pods from being scheduled in case they don't fit the requirements in all the PSP on the cluster. Also, PSPs are not enabled by default in Kubernetes, you have to enable them manually through the kube-apiserver pod by enabling the admission controller for PSP, which I think confirm my understanding of PSP.

Also, even if this is possible to change a securityContext of a Pod using PSPs, here it is a quote from a official Kubernetes page:

A part of the Pod specification, PodSecurityContext (and its per-container counterpart SecurityContext) is the collection of fields that specify many of the security-relevant settings for a Pod. The security context dictates to the kubelet and container runtime how the Pod should actually be run. In contrast, the PodSecurityPolicy only constrains (or defaults) the values that may be set on the security context.

Also, PSP have been deprecated in Kubernetes 1.21 and are going to be removed in the future.

And finally, we are using Azure to deploy our Kubernetes cluster and Azure have their own way to handle policy through a custom Gatekeeper. There is an in-preview feature allowing to use PSPs but it's going to be removed as PSP are being deprecated.

However, I can be completely wrong in my understanding of PSPs. If this is the case, please give me resources to check and understand how I can use a PSP on a single deployment only.

@jertel
Copy link
Owner

jertel commented Jun 21, 2021

Your knowledge of Kubernetes is likely beyond mine at this point, so I appreciate you taking the time to clear up the current state of PSPs.

@etiennetremel - You added the podSecurityPolicy to the original (now obsolete) ElastAlert helm chart over a year ago via this commit:

helm/charts@25f7b33#diff-ce21e247f577481ff2b1fab295eb4f2d1b46574622260fa25461006045723cf6

If you're still using ElastAlert in Kubernetes could you take a look at this PR and provide your review? I can leave in the existing PSP for now for users that might be using it, and then remove it sometime in the future once people have had time to move away from that deprecated policy feature.

@etiennetremel
Copy link

Is there a specific reason to comment it out? I would enforce it instead. Otherwise looks good to me.

I have been using this chart https://github.com/jertel/elastalert-docker/tree/master/chart/elastalert
but looks like we will need a migration to the new one defined here, thanks for tagging me I wasn't aware :)

@jertel
Copy link
Owner

jertel commented Jun 22, 2021

@lepouletsuisse Will you please add a [deprecated] note next to the existing podSecuritypolicy.create setting in the README.md and values.yaml? And I agree with @etiennetremel that the new settings should be enabled by default which would mean uncommenting out your new lines in values.yaml and updating the default column of the README.md for those two new settings. I can't think of a downside to doing that.

With those changes I think we'll be ready to merge it in.

@lepouletsuisse
Copy link
Contributor Author

@jertel Done, I let you check the new commit and merge it if everything is alright 👍

Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

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

Thanks!

@jertel jertel merged commit 63c391f into jertel:master Jun 22, 2021
@lepouletsuisse lepouletsuisse deleted the f-add_chart_securityContext branch June 22, 2021 12:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants