Closed erikfuego closed 7 months ago
+1 to this - would love to have the ability to pass securityContexts
+1 this is a crucial feature
I need this change so that I can set runAsNonRoot: true
so that my docker registry deployment passes my Kyverno runAsNonRoot policies
This is a needed feature for us as well!
sorry everyone! i was on paternal leave. hoping this gets merged soon
@canterberry Could we get an Approving review
?
This MR looks good to me, and it's backwards compatible. Also, happy to become a maintainer for this repo if you need some help going through these issues.
@erikfuego I realize this is not backwards compatible as of now, if people have securityContext disabled, this update will now automatically enable it which may be undesired and breaking (since there are default fields for securityContext set).
In the case that someone already has it enabled, you receive the following warning when installing:
W0201 10:03:19.051240 7376 warnings.go:70] unknown field "spec.template.spec.securityContext.enabled"
I've suggested some ways to work around this by keeping the enabled field, and omitting it when populating the securityContext sections.
I was using the Bitnami MongoDB chart as a basis
Yeah thats a good point. Can you confirm that all the changes you suggested are in? The CI tests are failing. Im also going to test this locally with the values I set for the chart
@joshsizer I reverted the changes you suggested for containerSecurityContext
because thats a new field I added to make the securityContext work for containers. And accepted your changes to keep the enabled
field for securityContext and omit it when populating securityContext
@joshsizer I reverted the changes you suggested for
containerSecurityContext
because thats a new field I added to make the securityContext work for containers. And accepted your changes to keep theenabled
field for securityContext and omit it when populating securityContext
Awesome! I wonder, would it be good to keep the changes for containerSecurityContext, so that the schemas for each section are similar? I see either option as viable, but lean towards keeping them similar. I think lint was failing because we would have to add in to values.yaml
containerSecurityContext:
enabled: false
@joshsizer I reverted the changes you suggested for
containerSecurityContext
because thats a new field I added to make the securityContext work for containers. And accepted your changes to keep theenabled
field for securityContext and omit it when populating securityContextAwesome! I wonder, would it be good to keep the changes for containerSecurityContext, so that the schemas for each section are similar? I see either option as viable, but lean towards keeping them similar. I think lint was failing because we would have to add in to
values.yaml
containerSecurityContext: enabled: false
I prefer the pattern where enabled
is not used because otherwise its an extra value to worry about. But if you want to use enabled
, feel free to suggest the changes and I can apply them
sorry about that weird merge, I merged this branch into a colleagues forked repo to test these changes, and for some reason his changes were merged into my branch.
The last change we need is to add to values.yaml:
containerSecurityContext: enabled: false
that broke the CI
The last change we need is to add to values.yaml:
containerSecurityContext: enabled: false
that broke the CI
Could you add into values.yaml
containerSecurityContext:
enabled: false
That will fix the CI.
I'm not sure I can push to this branch
LGTM
@canterberry Can you review this as well?
wanted to bumb this. Thanks!
@erikfuego Would you be able to recommit with signed commits? I am now a maintainer, and can approve and merge, but this repository is setup to only allow signed commits
@joshsizer
Not sure what the protocol should be moving forward, but I'm also a maintainer and I can now merge if you'd like.
@joshsizer
Not sure what the protocol should be moving forward, but I'm also a maintainer and I can now merge if you'd like.
Let's do it!
Due to PodSecurity policies in place after CIS hardening my cluster, I had to add the following to the pod or container security context:
With the current securityContext implementation, only
securityContext.fsGroup
andsecurityContext.runAsUser
were included. This change adds the ability to include everything in thesecurityContext
provided. It also removes theenabled
parameter which is unnecessary to me because we can just check if anything in thesecurityContext
exists.I added a
containerSecurityContext
to help differentiate between adding these fields for a container or the service itself. IfsecurityContext
is added for the service, PodSecurity fails because it specifically asks to be added to the container level.This also adds a missing field for secrets called
secretRef
for s3.