feat: container level security context

[llaszkie]

Hi!

The feature differentiate securityContext on the pod and container level. It makes the 'k6-operator` capable to operate on K8S >= 1.25 when PSS (Pod Security Standards) the built-in Pod Security Admission controller is in place and level is set to Restricted.

Sample usage:

apiVersion: k6.io/v1alpha1
kind: TestRun
spec:
  runner:
   securityContext:
     runAsNonRoot: true
   containerSecurityContext:
     allowPrivilegeEscalation: false

[CLAassistant]

All committers have signed the CLA.

[llaszkie]

fixes #297

[yorugac]

Hi @llaszkie, thanks on working on this!
FYI, I'll be able to review this PR fully on the next week. For now, just one question: you chose to add a new field to Pod. Kubernetes does have 2 separate objects for pod's and container's security context respectfully, but since our TestRun API doesn't expose containers, a new field has to be added with a totally new name containerSecurityContext. Have you considered making a wrapper struct securityContext instead of a new field that will contain fields from both objects and then can be reduced to each with .ToPodSecurityContext() and .ToContainerSecurityContext() methods? This is the approach that I wanted to explore in context of this issue but didn't have time to dig into details yet: hence, the question 🙂

[llaszkie]

Hi @yorugac ,

Yes - I was considering the wrapper solution - as suggested in the #297. But it could bring more challenges than benefits. In the K8S definitions those contexts are defined separately: pod here and container here, but they overlap in some attributes (sic!), like seccompProfile. And it is a valid use case (for example for pod volumes) - the overlap handling is defined in spec too. How then you would distinguish the user intention to set the one on the Container, but not on the Pod in your API exposing only combined wrapper?

What could be done instead is to explicitly expose Container in your API. It would even more align both models (K8S and K6) and solution could be cleaner? Let me know how about that approach.

[yorugac]

What could be done instead is to explicitly expose Container in your API

WDYM by that?

After some looking into this, yes, I agree that there's obviously a goal of exposing a more granular control over security context in Kubernetes definitions, hence two objects. It'd be nice if we could avoid over-complicating our API. I think the main problem with the current implementation is that containerSecurityContext is being passed only to the main container and not to init containers. It probably should be passed everywhere if it is defined.

[yorugac]

init containers

I.e. here: https://github.com/grafana/k6-operator/blob/main/pkg/resources/jobs/helpers.go#L120-L147

[llaszkie]

What could be done instead is to explicitly expose Container in your API

I was thinking about extracting the container settings to a separate object embedded within the Pod. Probably too much overhead 😄

Thanks for the hint for the init containers - absolutely right. Fixed.

[yorugac]

@llaszkie, apologies for the delay. Thanks for the update; the changes seem to be fine 👍 But could you please rebase the branch against main? It can't be merged ATM.

[llaszkie]

Good morning! @yorugac: rebased. I suppose we are good to go now :-)

[yorugac]

@llaszkie, I have to ask for 2 more things though both should be pretty quick:

• please see my comment about Helm chart (I accidentally posted it as a comment instead of including into review)
• it'd be nice to add a sample of TestRun definition with a new option into config/samples.

[yorugac]

Thank you for the updates, @llaszkie!