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

move processing of labels and annotations as tags #1522

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

celenechang
Copy link
Contributor

@celenechang celenechang commented Nov 12, 2024

What does this PR do?

  • Fix issue where a Cluster Agent ClusterRole and ClusterRolebinding was not being cleaned up when DatadogAgent was deleted. This is because of the order of processing of overrides compared to features.
  • Changed behavior to only create ClusterRole if it will not be empty
  • Also noticed a small typo in one of the conditionals which is now fixed

Motivation

The presence of the ClusterRole and ClusterRoleBinding was causing a requeue of a nonexistant object

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Test according to instructions in #1379

In addition:

  • Verify that deletion of the DatadogAgent resource results in deletion of the ClusterRole and ClusterRoleBinding called datadog-cluster-agent-annotations-and-labels-as-tags
  • Verify that removal of both kubernetesResourcesLabelsAsTags and kubernetesResourcesAnnotationsAsTags from the DatadogAgent manifest results in the absence of the ClusterRole datadog-cluster-agent-annotations-and-labels-as-tags.

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@celenechang celenechang added the bug Something isn't working label Nov 12, 2024
@celenechang celenechang added this to the v1.11.0 milestone Nov 12, 2024
@celenechang celenechang requested a review from a team as a code owner November 12, 2024 19:00
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 62.79070% with 32 lines in your changes missing coverage. Please review.

Project coverage is 48.64%. Comparing base (9d37612) to head (6694f0f).

Files with missing lines Patch % Lines
...ller/datadogagent/feature/enabledefault/feature.go 14.28% 28 Missing and 2 partials ⚠️
...troller/datadogagent/feature/enabledefault/rbac.go 96.07% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1522      +/-   ##
==========================================
+ Coverage   48.63%   48.64%   +0.01%     
==========================================
  Files         227      226       -1     
  Lines       20227    20233       +6     
==========================================
+ Hits         9838     9843       +5     
- Misses       9872     9874       +2     
+ Partials      517      516       -1     
Flag Coverage Δ
unittests 48.64% <62.79%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
...nternal/controller/datadogagent/override/global.go 52.53% <ø> (+4.41%) ⬆️
...troller/datadogagent/feature/enabledefault/rbac.go 16.44% <96.07%> (+16.44%) ⬆️
...ller/datadogagent/feature/enabledefault/feature.go 36.47% <14.28%> (-2.75%) ⬇️

Continue to review full report in Codecov by Sentry.

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

---- 🚨 Try these New Features:

Copy link
Contributor

@adel121 adel121 left a comment

Choose a reason for hiding this comment

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

Thanks @celenechang , I've left a comment

Copy link
Contributor

@adel121 adel121 left a comment

Choose a reason for hiding this comment

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

Question:

kubernetesResourcesLabelsAsTags and podLabelsAsTags are very similar in the sense that they should be applied to all 3 agents (node, cluster and runner), and the cluster agent needs an rbac to watch pods.

I am curious why these 2 features are implemented differently in the operator?
If we had an rbac issue with kubernetesResourcesLabelsAsTags shouldn't we also have the same issue for podLabelsAsTags?

Copy link
Contributor

@adel121 adel121 left a comment

Choose a reason for hiding this comment

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

LGTM

@celenechang celenechang merged commit a20d854 into main Nov 25, 2024
20 checks passed
@celenechang celenechang deleted the celene/fix_dca_clusterrole branch November 25, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants