search

trinodb / charts

Apache License 2.0
151 stars 173 forks source link

Not possible to clear securityContext.runAsUser or securityContext.runAsGroup which prevents using the Chart out-of-the-box with OpenShift #181

Closed joshuagrisham-karolinska closed 1 month ago

joshuagrisham-karolinska commented 5 months ago

Due to how OpenShift handles UIDs for containers by default, it is best if we do not actually set anything for securityContext.runAsUser or securityContext.runAsGroup since the assigned number range can be dynamic per namespace and you will get a lot of errors unless you jump through all of the hoops to create a custom Security Context Constraint.

Unfortunately due to a bug with Helm (https://github.com/helm/helm/issues/12488), if your deployment process uses the -f values.yaml kind of syntax (such as like with ArgoCD) then it is not possible to wipe out these values with null -- they will always be set as the default after the values merge.

Example values file in my own deployment (assuming the dependent trino chart has a local alias of trino in my chart)

trino:
  securityContext:
    runAsUser: null # purposefully empty
    runAsGroup: null # purposefully empty

After applying my values file, the resulting Deployments will still carry the default from the Trino Chart's default values.yaml like this:

...
      securityContext:
        runAsUser: 1000
        runAsGroup: 1000
...

Then, when this is deployed to OpenShift, the Deployments are blocked from being created and generate loads of spam events that runAsUser etc is not within the allowed range 😨

I am not sure what the "right" approach is for this, and a bit of a shame about the bug in Helm. For now my work-around is that I have manually downloaded a specific version of the chart into my own repository under charts/trino/ and just commented out both runAsUser and runAsGroup from the default values file.

Maybe it is a good compromise to allow for mapping in the entire securityContext from a user's own values file (i.e. the entire object that is in the user's values file will just get passed in), but not set any properties underneath it? The downside is that this might be a breaking change for some users if/when they take the version of the chart that implements this change (that they might need to specifically set these values to 1000 in their own values files after that?). Or if you want to avoid a breaking change, add some kind of true/false flag that blocks them that is enabled by default ? (though this sounds a bit messy and less fun to have lingering around...)

nineinchnick commented 5 months ago

Thanks for the detailed description!

Maybe it is a good compromise to allow for mapping in the entire securityContext

That sounds reasonable. We already do this in a few sections. We surely can try to handle backward compatibility, detecting when both runAsUser as the whole securityContext are set and raising an error.

chgl commented 5 months ago

@joshuagrisham-karolinska I think

securityContext: ~

should do the trick to remove the securityContext entirely from the pod spec.

A solution that is backwards-compatible and maybe even nicer since it allows setting all securityContext options is instead of:

https://github.com/trinodb/charts/blob/main/charts/trino/templates/deployment-coordinator.yaml#L34-L38

doing the same thing already done for the container-level securityContext:

https://github.com/trinodb/charts/blob/main/charts/trino/templates/deployment-coordinator.yaml#L115-L116