Kong / charts

Helm chart for Kong
Apache License 2.0
249 stars 480 forks source link

Custom Container SecurityContext fields #373

Closed ttony closed 3 years ago

ttony commented 3 years ago

Hi,

I found that there is no place to include a security context in init-containers (wait-for-db) and containers. recently, our security scan tools pointed that such securityContext is mandatory:

securityContext:
  readOnlyRootFilesystem: false
  allowPrivilegeEscalation: false

Currently, kong-helm supported securityContext for kong pods. for this request, we can use containers.securityContext? let me know if what's the suitable name for this.

rainest commented 3 years ago

We basically dump the raw YAML from the current (Pod-level) security context into Pod templates:

https://github.com/Kong/charts/blob/bd8e8600f708a956e4041ec84d6a0814e1fbcfd2/charts/kong/values.yaml#L582-L583 https://github.com/Kong/charts/blob/bd8e8600f708a956e4041ec84d6a0814e1fbcfd2/charts/kong/templates/deployment.yaml#L240-L241 https://github.com/Kong/charts/blob/bd8e8600f708a956e4041ec84d6a0814e1fbcfd2/charts/kong/templates/_helpers.tpl#L559-L564

That helper function is probably unnecessary and obfuscates what actually happens when looking at the templates, but whatever.

IMO this would read cleaner in values.yaml to support both:

securityContext:
  pod: {}
  container: {}

But it'd break existing configs and probably isn't worth it given that there's only two types of security context.

Given that, I think it's appropriate to add a new top-level value, containerSecurityContext, which functions the same way (defaults to an empty object, inserts whatever YAML object you provide into the appropriate location within the container specs). Doesn't need a helper though, you can just stick both the toYaml and nindent directly into the templates.

There is a complicating factor where we'd potentially want different security contexts for the various containers, e.g. Kong containers get one context, controller containers another, and user sidecars yet another. Making separate sections for each seems like it'd be annoying though, since you'd need to copy-paste contexts if you don't actually need that level of granularity. As such, I'd recommend just adding a single containerSecurityContext and using it for all containers--if we do find that users need more control later on we can add new settings, using containerSecurityContext as the default and overriding it if a specific container's context setting is present.

@ttony does that design sound clear? Are you up for submitting a PR to add it?

ttony commented 3 years ago

@rainest , sound clear. will take this soon.