grafana / k6-operator

An operator for running distributed k6 tests.
Apache License 2.0
592 stars 166 forks source link

feat: Add Helm configuration for pod security context #467

Closed gonmmarques closed 2 weeks ago

gonmmarques commented 1 month ago

Hello there,

Saw this issue https://github.com/grafana/k6-operator/issues/466, and actually was also looking into having that possibility. decided to give it a try.

Tried to follow a contributing guide, but only found https://github.com/grafana/k6-operator/issues/330.

The way it is now added, by default the field is optional. Let me know also if it makes sense the way it was added (name of the key and location) and the schema definition.

I also ran helm-docs to update the README of the Helm Chart.

I ran locally helm template and was having issues when setting containerSecurityContext, when checking the charts/k6-operator/values.schema.json I noticed those fields for the manager and proxy their values changed on the should have the additionalProperties to true right? See Here and Here

Example

global:
  podSecurityContext: {
    "runAsNonRoot": true,
    "runAsUser": 1000,
    "runAsGroup": 1000,
    "fsGroup": 1000,
    "allowPrivilegeEscalation": false,
    "seccompProfile": {
      "type": "RuntimeDefault"
    }
  }  

Checked it by running locally the helm template and it rendered

  spec:
      securityContext:
        allowPrivilegeEscalation: false
        fsGroup: 1000
        runAsGroup: 1000
        runAsNonRoot: true
        runAsUser: 1000
        seccompProfile:
          type: RuntimeDefault

Anyways, let me know if I missed something or is something wrong. Closes: https://github.com/grafana/k6-operator/issues/466

gonmmarques commented 2 weeks ago

Hi @gonmmarques, apologies for the delay! I've finally gotten around to going through the PRs 😅

We've recently had a bug with values schema in the chart: it was resolved just last week. So this PR needs rebasing to the latest main.

Also, one change request: as you can see in the issue's discussion, there is a chance of misunderstanding how security contexts for test runs are configured (only via TestRun definition ATM). So I think its best to move the podSecurityContext in Helm values to manager object instead of global: to make it clearer that it impacts only the k6-operator app. WDYT?

Hello @yorugac, no worries. Sure that makes sense. I have updated the PR, let me know if there is anything missing.