Add missing fields for Security context and secrets

[erikfuego]

Due to PodSecurity policies in place after CIS hardening my cluster, I had to add the following to the pod or container security context:

runAsNonRoot: true
seccompProfile:
  type: "RuntimeDefault"
allowPrivilegeEscalation: false
capabilities:
  drop:
  - ALL

With the current securityContext implementation, only securityContext.fsGroup and securityContext.runAsUser were included. This change adds the ability to include everything in the securityContext provided. It also removes the enabled parameter which is unnecessary to me because we can just check if anything in the securityContext exists.

I added a containerSecurityContext to help differentiate between adding these fields for a container or the service itself. If securityContext 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.

[tchinmai7]

+1 to this - would love to have the ability to pass securityContexts

[joshsizer]

+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

[Mercbot7]

This is a needed feature for us as well!

[erikfuego]

sorry everyone! i was on paternal leave. hoping this gets merged soon

[joshsizer]

LGTM

[joshsizer]

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

[joshsizer]

@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

[erikfuego]

@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

[erikfuego]

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

@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

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

[erikfuego]

@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

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

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

[erikfuego]

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.

[joshsizer]

The last change we need is to add to values.yaml:

containerSecurityContext:
  enabled: false

[erikfuego]

The last change we need is to add to values.yaml:

containerSecurityContext:
  enabled: false

that broke the CI

[joshsizer]

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

[joshsizer]

LGTM

@canterberry Can you review this as well?

[erikfuego]

LGTM

@canterberry Can you review this as well?

wanted to bumb this. Thanks!

[joshsizer]

@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

[vyas-n]

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

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