[Helm] Expose security context in helm chart.

[DmitriGekhtman]

Signed-off-by: Dmitri Gekhtman [email protected]

Why are these changes needed?

Exposes securityContext in Helm charts.
See #750.

TODO: Test with the workflows in #750

Related issue number

Checks

• I've made sure the tests are passing.
• Testing Strategy
  • Unit tests
  • Manual tests
  • This PR is not tested :(

[DmitriGekhtman]

cc @juliusvonkohout

[juliusvonkohout]

I only took a glimpse, but is the naming really consistent?

[DmitriGekhtman]

I only took a glimpse, but is the naming really consistent?

I'm open to suggestions!

My main consideration when making changes to these charts is backwards compatibility.
I'm not 100% pleased with the previous decisions that were made, but for now I'm just trying to expose the necessary configuration while keeping things consistent with what's already there.

[DmitriGekhtman]

I only took a glimpse, but is the naming really consistent?

Basically, I'm saying
"No, the naming is not completely consistent. But it's the best I can do at the moment."

[juliusvonkohout]

Alright, then why do we not take the values from the other PR instead of SecurityContext: {}?

[DmitriGekhtman]

Alright, then why do we not take the values from the other PR instead of SecurityContext: {}?

I'd prefer to simply expose the configuration rather than copying it everywhere.

[DmitriGekhtman]

Alright, then why do we not take the values from the other PR instead of SecurityContext: {}?

I'd prefer to simply expose the configuration rather than copying it everywhere.

Actually, sure -- I'll set the defaults to the recommended securityContext and then let users override as needed.

[DmitriGekhtman]

Alright, then why do we not take the values from the other PR instead of SecurityContext: {}?

I'd prefer to simply expose the configuration rather than copying it everywhere.

Actually, sure -- I'll set the defaults to the recommended securityContext and then let users override as needed.

Will repeat the same treatment for the auto-configured autoscaler container.

[DmitriGekhtman]

Alright, then why do we not take the values from the other PR instead of SecurityContext: {}?

Actually, never mind.

The reason for not doing this by default is

1. It makes debugging more difficult for infra developers.
2. It appears at the moment to interact poorly with OpenShift.

Thus, it's better to clearly document the security context recommendation, but not to copy the more restrictive configuration everywhere.

[DmitriGekhtman]

Thanks, @juliusvonkohout for prodding us to ensure that Ray can run on Kubernetes with proper security configuration!

[DmitriGekhtman]

@kevin85421 this works exactly as expected when the relevant securityContext configuration is added.
It fails exactly as expected when the relevant securityContext configuration is removed.

[kevin85421]

LGTM