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 container security context #411

Merged

Conversation

gonmmarques
Copy link
Contributor

@gonmmarques gonmmarques commented Jun 5, 2024

Hello there,

Saw this issue #402 marked as good first issue and decided to give it a try.

Tried to follow a contributing guide, but only found #330.

Anyways, let me know if I missed something or is something wrong.

The way it is now added, by default the fields are optional for both of the containers in the deployment, but someone can change it in the Helm installation by setting the values e.g for manager container below:

manager:
  # ...
  # manager.containerSecurityContext -- A security context defines privileges and access control settings for the container.
  containerSecurityContext: {
    "runAsNonRoot": true,
    "runAsUser": 1000,
    "runAsGroup": 1000,
    "allowPrivilegeEscalation": false,
    "seccompProfile": {
      "type": "RuntimeDefault"
    },
    "capabilities": {
      "drop": [
        "ALL"
      ]
    }
  }

Checked it by running locally the helm template and it rendered

    spec:
      containers:
        - name: manager
          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            runAsGroup: 1000
            runAsNonRoot: true
            runAsUser: 1000
            seccompProfile:
              type: RuntimeDefault

Closes: #402

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

@yorugac
Copy link
Collaborator

yorugac commented Jun 7, 2024

Hi @gonmmarques, thanks for working on this!
The PR LGTM, the only thing left is to run helm-docs (that will be in contributing guide for Helm additions).

However, I've just noticed that the issue didn't specify whether it was about pod's securityContext or about container's securityContext 😕 For example seccompProfile is present in both.
I.e. it could be that container's one will not be sufficient for that case. @MohanedSaad, could you please clarify that bit?

@MohanedSaad
Copy link

container's

Hi, thanks for picking this up. The container one should be enough for our case 😊

@gonmmarques
Copy link
Contributor Author

gonmmarques commented Jun 7, 2024

Hello @yorugac

I will run the tool. Given the @MohanedSaad feedback, is there anything still missing?

@yorugac
Copy link
Collaborator

yorugac commented Jun 10, 2024

Thanks for the update @gonmmarques and for feedback, @MohanedSaad!

@yorugac yorugac merged commit be7b9e0 into grafana:main Jun 10, 2024
1 check passed
@gonmmarques gonmmarques deleted the feat/helm-add-security-context-config branch July 5, 2024 14:41
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.

Helm: add configuring securityContext for the operator deployment
4 participants