Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Helm configuration for pod security context #467

Merged

Conversation

gonmmarques
Copy link
Contributor

@gonmmarques gonmmarques commented Oct 2, 2024

Hello there,

Saw this issue #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 #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: #466

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@gonmmarques gonmmarques force-pushed the feat/add-security-context-to-pod-spec branch 3 times, most recently from 8be7340 to 50883c3 Compare November 4, 2024 17:12
@gonmmarques gonmmarques force-pushed the feat/add-security-context-to-pod-spec branch from 50883c3 to b3c81a0 Compare November 4, 2024 17:24
@gonmmarques
Copy link
Contributor Author

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.

@gonmmarques gonmmarques requested a review from yorugac November 4, 2024 17:27
Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙌 Thanks for the update, @gonmmarques!

@yorugac yorugac merged commit 36bcf0d into grafana:main Nov 5, 2024
8 checks passed
@gonmmarques gonmmarques deleted the feat/add-security-context-to-pod-spec branch November 5, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add securityContext to pod spec in Helm chart
2 participants