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

Fix RBAC resource name too long #386

Merged
merged 3 commits into from Oct 4, 2021
Merged

Fix RBAC resource name too long #386

merged 3 commits into from Oct 4, 2021

Conversation

clamoriniere
Copy link
Collaborator

@clamoriniere clamoriniere commented Sep 30, 2021

What does this PR do?

We introduced the namespace as prefix of the clusterrole and
clusterrolebinding to allow multi DatadogAgent deployment in a cluster.
But if the DatadogAgent namespace/name is very long like:
datadog-agent/datadog-agent the final rbac name for orchestrator-explorer
RBAC was: datadog-agent-datadog-agent-orchestrator-explorer-cluster-agent
which is longer than the limit of 64 chars.

This PR reduce the size by renaming it to datadog-agent-datadog-agent-orch-exp-dca

This PR also update the value in app.kubernetes.io/instances for cluster scoped resources like:

  • clusterrole
  • clusterrolebinding

Motivation

Fix agent deployment

Additional Notes

Anything else we should know when reviewing?

Describe your test plan

try to deploy the datadogAgent named datadog-agent in the namespace: datadog-agent
the install should work.

We introduced the `namespace` as prefix of the clusterrole and
clusterrolebinding to allow multi DatadogAgent deployment in a cluster.
But if the DatadogAgent namespace/name is very long like:
datadog-agent/datadog-agent the final rbac name for `orchestrator-explorer`
RBAC was: `datadog-agent-datadog-agent-orchestrator-explorer-cluster-agent`
which is longer than the limit of 64 chars.

This PR reduce the size by renaming it to `datadog-agent-datadog-agent-orch-exp-dca`
@clamoriniere clamoriniere requested a review from a team as a code owner September 30, 2021 21:16
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, documentation

@clamoriniere clamoriniere added this to the v0.7 milestone Sep 30, 2021
@clamoriniere clamoriniere added bug Something isn't working component/controller labels Sep 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

Codecov Report

Merging #386 (fd0cf8a) into main (7cadb30) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #386   +/-   ##
=======================================
  Coverage   66.24%   66.24%           
=======================================
  Files          64       64           
  Lines        6933     6933           
=======================================
  Hits         4593     4593           
  Misses       2020     2020           
  Partials      320      320           
Flag Coverage Δ
unittests 66.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
controllers/datadogagent/kubestatemetrics.go 91.66% <ø> (ø)
controllers/datadogagent/orchestrator.go 79.16% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cadb30...fd0cf8a. Read the comment docs.

@clamoriniere clamoriniere modified the milestones: v0.7, v0.7.1 Sep 30, 2021
@davidor
Copy link
Member

davidor commented Oct 1, 2021

I think this only solves the issue in some cases. We can have the same issue if the DDA has a long name or is deployed in a namespace with a long name.

@@ -31,7 +31,7 @@ type roleBindingInfo struct {
func buildRoleBinding(dda *datadoghqv1alpha1.DatadogAgent, info roleBindingInfo, agentVersion string) *rbacv1.RoleBinding {
return &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Labels: getDefaultLabels(dda, info.name, agentVersion),
Labels: getDefaultLabels(dda, dda.Name, agentVersion),
Copy link
Member

@davidor davidor Oct 4, 2021

Choose a reason for hiding this comment

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

Shouldn't this one and the one in buildServiceAccount() be NewPartOfLabelValue(dda).String() like the rest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here it is a rolebinding not a clusterrolebinding.
So I was thinking we don't need to add the namespace

Copy link
Member

Choose a reason for hiding this comment

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

You're right, the namespace of the DDA is not needed in roles, but I wonder if we should just use the same format everywhere for consistency.

buildAgentClusterAgentRole (https://github.com/DataDog/datadog-operator/pull/386/files/d695397051a98d999df874dabfeba119ddfde1dd#diff-1ab2275c9afe43600275bda29e5e505bb4ecda385774675935de82f60e27a1b7R1363) is a role and uses NewPartOfLabelValue(dda).String(). That'd need to be changed if we keep dda.Name here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch,
PR updated with 85c0a1c

@clamoriniere clamoriniere merged commit 4bab124 into main Oct 4, 2021
@clamoriniere clamoriniere deleted the clamoriniere/fix-rbac branch October 4, 2021 18:15
celenechang pushed a commit that referenced this pull request Oct 5, 2021
We introduced the `namespace` as prefix of the clusterrole and
clusterrolebinding to allow multi DatadogAgent deployment in a cluster.
But if the DatadogAgent namespace/name is very long like:
datadog-agent/datadog-agent the final rbac name for `orchestrator-explorer`
RBAC was: `datadog-agent-datadog-agent-orchestrator-explorer-cluster-agent`
which is longer than the limit of 64 chars.
This PR reduce the size by renaming it to `datadog-agent-datadog-agent-orch-exp-dca`

Update app.kubernetes.io/instance label value for cluster resources
@khewonc khewonc mentioned this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/controller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants