Skip to content

Avoid RBAC errors when Operator can't list or watch Secrets#2793

Merged
levan-m merged 2 commits into
mainfrom
levan-m/secret-watch-without-rbac
Mar 23, 2026
Merged

Avoid RBAC errors when Operator can't list or watch Secrets#2793
levan-m merged 2 commits into
mainfrom
levan-m/secret-watch-without-rbac

Conversation

@levan-m

@levan-m levan-m commented Mar 20, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes bug reported in #2791.

  • Don't start secrets watch in helm metadata forwarder if Operator doesn't have list/watch RBAC.
  • Update CredentialManager to avoid unnecessary secrets informer initialization.

Motivation

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

  1. install Operator
❯ helm install operator datadog/datadog-operator \
--set datadogMonitor.enabled=true \
--set datadogAgent.enabled=false \
--set datadogAgentInternal.enabled=false
  1. Update rbac to drop secret watch, list permissions, restart operator pod.
  kubectl get clusterrole operator-datadog-operator -o json \
    | jq '
        .rules |= map(
          if (.resources // [] | index("secrets")) != null
          then .resources |= map(select(. != "secrets"))
          else .
          end
        )
        | .rules += [{"apiGroups":[""],"resources":["secrets"],"verbs":["create","delete","get","patch","update"]}]
      ' \
    | kubectl apply -f -

kubectl rollout restart deployment/operator-datadog-operator
  1. Observer errors
    ..."logger":"klog","msg":"Failed to watch","logger":"UnhandledError","reflector":"pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:285","type":"*v1.Secret","error":"failed to list *v1.Secret: secrets is forbidden: User \"system:serviceaccount:default:operator-datadog-operator\" cannot list resource \"secrets\" in API group \"\" in

  2. Update to fixed image; after restart Operator should log
    {"level":"INFO","ts":"2026-03-20T19:23:55.561Z","logger":"metadata.helm","msg":"No permission to list/watch Secrets, Helm metadata collection from Secrets will be disabled"}

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
  • All commits are signed (see: signing commits)

@levan-m levan-m added this to the v1.25.0 milestone Mar 20, 2026
@levan-m
levan-m requested a review from a team March 20, 2026 19:31
@levan-m levan-m added bug Something isn't working team/container-platform labels Mar 20, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 084333630d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +168 to +171
ResourceAttributes: &authorizationv1.ResourceAttributes{
Verb: verb,
Resource: "secrets",
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Scope Secret RBAC check to watched namespaces

canListWatchSecrets builds a SelfSubjectAccessReview without resourceAttributes.namespace, which Kubernetes interprets as an all-namespaces check for namespaced resources. In namespace-scoped deployments (for example when WATCH_NAMESPACE/DD_AGENT_WATCH_NAMESPACE limits cache scope and RBAC is granted per namespace), this returns denied even though list/watch is allowed in the watched namespace(s), so secretAccessEnabled is set to false and Secret-backed Helm releases are never processed. This is a functional regression for namespace-scoped RBAC setups.

Useful? React with 👍 / 👎.

@codecov-commenter

codecov-commenter commented Mar 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 2.12766% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.85%. Comparing base (d134930) to head (2403c14).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/utils/metadata/helm_metadata.go 0.00% 44 Missing ⚠️
cmd/main.go 0.00% 1 Missing ⚠️
pkg/config/creds.go 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2793      +/-   ##
==========================================
+ Coverage   38.78%   38.85%   +0.07%     
==========================================
  Files         309      309              
  Lines       26852    27080     +228     
==========================================
+ Hits        10414    10522     +108     
- Misses      15658    15776     +118     
- Partials      780      782       +2     
Flag Coverage Δ
unittests 38.85% <2.12%> (+0.07%) ⬆️

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

Files with missing lines Coverage Δ
cmd/main.go 6.64% <0.00%> (ø)
pkg/config/creds.go 62.06% <50.00%> (ø)
pkg/controller/utils/metadata/helm_metadata.go 26.68% <0.00%> (-1.66%) ⬇️

... and 3 files with indirect coverage changes


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 d134930...2403c14. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread cmd/main.go Outdated
credsManager := config.NewCredentialManagerWithDecryptor(mgr.GetClient(), secrets.NewSecretBackend())
// Get call on a cached client initializes informer which requires list and watch permissions.
// If RBAC restricts list and watch permissions, the informer will log errors and may cause crash loops.
// Passing Reader avoids informer initialization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This is not really true. What bypasses the initialization is the direct call using mgr.GetAPIReader() which returns a client.Reader instead of client.Client leading to these changes. Which is fine as we don't issue writes, only need read.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

updated comment.

@levan-m
levan-m merged commit 444f938 into main Mar 23, 2026
35 of 36 checks passed
@levan-m
levan-m deleted the levan-m/secret-watch-without-rbac branch March 23, 2026 18:16
dd-octo-sts Bot pushed a commit that referenced this pull request Mar 23, 2026
* Avoid RBAC errors when Operator can't list or watch Secrets

* improve comment

(cherry picked from commit 444f938)
tbavelier pushed a commit that referenced this pull request Mar 24, 2026
…2800)

* Avoid RBAC errors when Operator can't list or watch Secrets

* improve comment

(cherry picked from commit 444f938)

Co-authored-by: levan-m <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants