helm-charts icon indicating copy to clipboard operation
helm-charts copied to clipboard

[kubernetes-ingress] Add configs for `SecurityContext`

Open tbnguyen1407 opened this issue 3 years ago • 5 comments

problem

Current chart has no config for SecurityContext.

suggestion

Add support for configuring SecurityContext in values.yaml

~ values.yaml ~
podSecurityContext: {}
containerSecurityContext: {}

consideration

Need to ensure no conflict with existing hardcoded securityContext set on main container when running unprivileged

tbnguyen1407 avatar May 09 '22 05:05 tbnguyen1407

Reason why IC chart doesn't have the ability to modify securityContext is exactly because we want to avoid users accidentally breaking unprivileged operation, which is now a default mode. Helm logic doesn't have the ability to do sanity checking of provided values in securityContext against our provided values, so for now we have decided to keep things as is. Can you elaborate what your plans were, especially in regards to current default values?

dkorunic avatar May 09 '22 06:05 dkorunic

Thanks for quick response. Currently we use a post-renderer to patch the deployment/daemonset’s securityContext.

I think a possible (though not ideal) way is to completely expose securityContext to values.yaml with defaults set same as current unprivileged config. Users then can add more fields if required (e.g. readOnlyRootFilesystem: true).

Edit:

Another reason you may not want to hardcode securityContext.runAsUser is that for certain environment (e.g. OpenShift) the user is always random. Setting securityContext.runAsNonRoot: true may be a better approach

tbnguyen1407 avatar May 09 '22 06:05 tbnguyen1407

@tbnguyen1407 There are few problems with completely opening/exposing securityContext values in values.yaml as opposed to hardcoding them:

  • As soon as UID/GID are mismatched with file/folder permissions that were used while creating the container (1000:1000 at the moment of writing this response), extraVolumes and extraVolumeMounts have to be used to override all invalid permissions and as a consequence, some default files such as HAProxy error files will get lost. We don't map and mount those folders by default yet, as mismatched UID/GIDs also bring some additional complexity in handling S6 overlay as well.

  • Same goes with readOnlyRootFilesystem feature. While we have prepared S6 overlay defaults (again, these have to be hardcoded) to make sure container can work in readonly mode, there is some additional complication on how this affects container and required volumes and their mounts and we in general avoid making this a default.

If we would have to expose securityContext fully, then we would have to make empty volumes mounting through extraVolumes and extraVolumeMounts a new default to avoid container being unable to start on UID/GID changes. I would rather go about this by adding a readOnlyRootFilesystem toggle that also enables required extraVolumes and extraVolumeMounts only when needed and thus avoid fully exposing securityContext.

dkorunic avatar May 09 '22 08:05 dkorunic

Thanks for detailed explanations. I understand there are considerations in place, hence this is just a suggestion. Current workaround with post-renderer is still working for now. I am sure the chart will improve and we will be able to use it in production, especially after recent removal of privileged initContainer (though the latest commit seems to have problem and not yet merged).

tbnguyen1407 avatar May 09 '22 09:05 tbnguyen1407

Also see #150 - some of which may apply here as well:

kustomize build https://github.com/kyverno/policies//pod-security | \
  kyverno apply -r \
  <(helm template --repo https://haproxytech.github.io/helm-charts kubernetes-ingress) \
  -

joebowbeer avatar Jul 10 '22 08:07 joebowbeer

This can be safely closed due to ffbf1a74eacdfa105a4d39015ca33cd37cade4f6 and dd4f8d1e566b34a7ffe59865fd389160134e5310.

dkorunic avatar Aug 22 '22 09:08 dkorunic