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

Improves ClusterRole management in Datadog chart #12705

Merged

Conversation

clamoriniere
Copy link
Collaborator

@clamoriniere clamoriniere commented Apr 1, 2019

What this PR does / why we need it:

If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Which issue this PR fixes

Special notes for your reviewer:

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

@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 Apr 1, 2019
@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 Apr 1, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @clamoriniere. 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.

Copy link
Collaborator

@CharlyF CharlyF left a comment

Choose a reason for hiding this comment

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

I don't think the metrics provider should matter when creating the RBAC for the Cluster Check Worker.
Out of curiosity, why not putting the SA, CRB and Auth delegator in the same file ?

Lastly, why would the cluster check worker need RBACs ? It's not interacting with the APIServer as far as I know ? Just running check against endpoints provided by the Cluster Agent.

@@ -0,0 +1,19 @@
{{- if and .Values.rbac.create .Values.clusterAgent.enabled .Values.clusterAgent.clusterChecks.enabled .Values.clusterAgent.metricsProvider.enabled .Values.clusterchecksDeployment.enabled -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why checking Values.clusterAgent.metricsProvider.enabled ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can be removed I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact after looking again in the current RBAC file, the condition needs to check 'Values.clusterAgent.metricsProvider.enabled' to be align with the condition present in the equivalent file: https://github.com/helm/charts/blob/master/stable/datadog/templates/agent-clusterrolebinding-auth-delegator.yaml#L1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in fact it is a useless permission for the agent running clusterchecks. I will remove file this file.
the other ...auth-delegator.yaml file is for the cluster-agent and not the agent

@clamoriniere
Copy link
Collaborator Author

clamoriniere commented Apr 12, 2019

Hi @CharlyF,
Thanks for the review.
to answer your questions:

  • I decided to put in different files since it looks like to be the current "files organisation" style in this chart. But yes we can put all relative agent-clusterchecks RBAC resources in the same file. (and can be done for the other RBAC files also)
  • I will remove the metricsProvider.enabled for the condition.
  • ClusterCheck need RBACs thinks some checks like "kube_controller_manager", "kube_scheduler"... need to access the Kubernetes control plane components.

@clamoriniere clamoriniere force-pushed the feature/datadog-clusterchecks-rbac branch from d9ff46a to 7f1b13c Compare April 12, 2019 10:49
@helm-bot helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 12, 2019
@clamoriniere clamoriniere force-pushed the feature/datadog-clusterchecks-rbac branch from bb49b8d to d67e65b Compare April 12, 2019 11:21
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 12, 2019
@clamoriniere clamoriniere force-pushed the feature/datadog-clusterchecks-rbac branch from d67e65b to e3814ef Compare April 12, 2019 11:23
Copy link
Collaborator

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

LGTM, I think we still need rbac for things like nonResourceURL (in case users protect some endpoints with rbac), and checks that could target specific k8s components.

@hkaj
Copy link
Collaborator

hkaj commented Apr 12, 2019

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2019
@hkaj
Copy link
Collaborator

hkaj commented Apr 12, 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 Apr 12, 2019
@clamoriniere clamoriniere force-pushed the feature/datadog-clusterchecks-rbac branch from e3814ef to 76f6cdb Compare April 12, 2019 12:33
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
@clamoriniere clamoriniere force-pushed the feature/datadog-clusterchecks-rbac branch from 76f6cdb to 94673d2 Compare April 12, 2019 12:35
@CharlyF
Copy link
Collaborator

CharlyF commented Apr 12, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CharlyF, clamoriniere, hkaj

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

The pull request process is described 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 k8s-ci-robot merged commit 4226ebf into helm:master Apr 12, 2019
TsuyoshiUshio pushed a commit to TsuyoshiUshio/charts that referenced this pull request Apr 18, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
hajnej pushed a commit to hajnej/charts that referenced this pull request Apr 24, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
devnulled pushed a commit to devnulled/charts that referenced this pull request Apr 25, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
dermorz pushed a commit to dermorz/charts that referenced this pull request Apr 26, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
goshlanguage pushed a commit to goshlanguage/charts that referenced this pull request May 17, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
eyenx pushed a commit to eyenx/charts that referenced this pull request May 28, 2019
If a user choose to deploy a dedicated agent deployment for running
the cluster checks, the agent RBAC is updated in order to give only
ClusterRole permission to the Agent that need it.

Signed-off-by: cedric lamoriniere <cedric.lamoriniere@datadoghq.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. 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.

None yet

5 participants