Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/datadog] Add options to set pod and container securityContext #17274

Closed
wants to merge 4 commits into from

Conversation

coreypobrien
Copy link
Contributor

Is this a new chart: Nope

What this PR does / why we need it: Adds options to specify securityContext settings for both pods and containers to handle special capabilities or more stringent security policies.

Which issue this PR fixes

Special notes for your reviewer: There is kind of a mixed nomenclature for the settings for various containers and pods because some are reused and some settings aren't mirrored everywhere. I tried to follow the existing naming instead of renaming everything.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/chart])

Signed-off-by: Corey O'Brien <corey@fairwinds.com>
@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: coreypobrien
To complete the pull request process, please assign clamoriniere
You can assign the PR to them by writing /assign @clamoriniere in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @coreypobrien. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2019
@coreypobrien
Copy link
Contributor Author

/assign @clamoriniere @xlucas

Signed-off-by: Corey O'Brien <corey@fairwinds.com>
Signed-off-by: Corey O'Brien <corey@fairwinds.com>
@coreypobrien
Copy link
Contributor Author

@hkaj @irabinovitch @CharlyF @mfpierre @clamoriniere @xlucas
Anyone available to take a look at this? Thanks

Signed-off-by: Corey O'Brien <corey@fairwinds.com>
@CharlyF
Copy link
Collaborator

CharlyF commented Oct 1, 2019

Hey @coreypobrien - Sorry for the delay, and thanks for the PR!
We are currently working on a major revamp of the chart but I will sync with the team asap to see how we can include your suggestion.

Best,
.C

@coreypobrien
Copy link
Contributor Author

Thanks for the update @CharlyF . Does that mean this won't be merged?

@CharlyF
Copy link
Collaborator

CharlyF commented Oct 1, 2019

This seems like a reasonable implementation, I have to double check with the team to confirm how to best integrate your change in this chart (or the new one).

@coreypobrien
Copy link
Contributor Author

Sounds good. Is there a branch somewhere you're working on where I could take a look and help with anything?

@CharlyF
Copy link
Collaborator

CharlyF commented Oct 2, 2019

Hey @coreypobrien - I wanted to circle back on this.
I think my colleague has not pushed his branch yet so we can still get changes in.
I was reviewing your PR and per the original issue it seems like the need was solely for the system-probe container (which indeed requires extra capabilities given that it runs bpf code).
Could you let me know why you want to add the security context option to all containers ?
I am afraid that this might be error prone in some cases as we can't easily run with other users than the one configured in the Dockerfile for the agent process (and the other agents').

Best,
.C

@coreypobrien
Copy link
Contributor Author

The original use case was that, but when I started down the path I realized there were a lot of other security settings that users focused on that would want to use like allowPrivilegeEscalation that are going to be useful on more than just the daemonset nodes and on both the pod and container contexts. So instead of trying to solve all of them ideally, I went with allowing the possibility of someone solving their specific problems.

My specific desires to to reduce privileges for places that the agent needs less permissions like when it is in cluster-agent or cluster-check or apm-only mode.

@CharlyF
Copy link
Collaborator

CharlyF commented Oct 7, 2019

@coreypobrien I see, thank you for clarifying.
We are ok with merging your change - However could you please add a disclaimer in the README for these options ? We want to be extra transparent as to when and how to use them, to avoid users configuring them thinking the agent can run with a different UID or with a limited context.

@zanhsieh
Copy link
Collaborator

zanhsieh commented Oct 8, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 8, 2019
@k8s-ci-robot
Copy link
Contributor

@coreypobrien: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e ffbdea9 link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@stale
Copy link

stale bot commented Nov 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2019
@zanhsieh
Copy link
Collaborator

zanhsieh commented Nov 7, 2019

@coreypobrien
Would you mind to rebase and resolve conflict files please? Thank you.
stable/datadog/Chart.yaml
stable/datadog/README.md
stable/datadog/templates/cluster-agent-deployment.yaml

@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2019
@CharlyF
Copy link
Collaborator

CharlyF commented Nov 7, 2019

Also @coreypobrien if you can please address my comment: #17274 (comment)
that would be great

@stale
Copy link

stale bot commented Dec 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 7, 2019
@clamoriniere
Copy link
Collaborator

/remove-lifecycle stale

@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 9, 2019
@stale
Copy link

stale bot commented Jan 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 8, 2020
@stale
Copy link

stale bot commented Jan 22, 2020

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Jan 22, 2020
@clamoriniere
Copy link
Collaborator

Hi @coreypobrien
May I ask you to rebase your PR, and look at the @CharlyF comment #17274 (comment) if you are still interested to have the feature you have implemented.

@nexuhan
Copy link
Contributor

nexuhan commented Nov 4, 2020

Merged in DataDog/helm-charts#83.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stable/datadog] container security context
8 participants