Skip to content

[AGENTCFG-35] Trigger a secret refresh on expired api key - #41589

Merged
dd-mergequeue[bot] merged 140 commits into
mainfrom
saad/expired-apikey-trigger-secret-refresh
Nov 24, 2025
Merged

[AGENTCFG-35] Trigger a secret refresh on expired api key#41589
dd-mergequeue[bot] merged 140 commits into
mainfrom
saad/expired-apikey-trigger-secret-refresh

Conversation

@s-alad

@s-alad s-alad commented Oct 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Triggers a secret refresh whenever an API key expires. (whenver we get a 403 or the routine health probe checks for the api key validity)

Motivation

Allows for a more seamless way to refresh secrets and api-key without requiring manual editing

Describe how you validated your changes

manual testing

  1. create an API key
  2. create a secret backend that returns your api-key & add to datadog.yaml
    • secret_backend_command: <somepath>/secret.sh
  3. set the secret_refresh_on_api_key_failure_interval config to a value greater than 0
    • secret_refresh_on_api_key_failure_interval: 1
  4. lower forwarder validation interval for testing purposes
    • forwarder_apikey_validation_interval: 1
  5. build & run the agent
  6. revoke the original API key
  7. induce a 403 or wait a minuite for the health check to learn that the API key is invalid
  8. create a new API key
  9. update your secret backend executable
  10. you should see logs of the secrets being refreshed and the health check going back to valid

example output from logging

$ revoke api_key
[SECRET-REFRESH] api_key '***************************f66d3' for domain https://app.datadoghq.com. is invalid
$ update secrets backend with valid key
[SECRET-REFRESH] Refreshing secrets for 1 handles
[SECRET-REFRESH] Secret datadog_api_key has changed
[SECRET-REFRESH] api_key '***************************a1bdc' for domain https://7-73-0-app.agent.datadoghq.com. is valid
[SECRET-REFRESH] Rotating API key for 'api_key': ***************************f66d3 -> ***************************a1bdc
[SECRET-REFRESH] Updated config setting api_key for secret datadog_api_key
[SECRET-REFRESH] Updated 1 secrets
[SECRET-REFRESH] Secret refresh after invalid API key completed

Additional Notes

  • Currently there is NewDefaultForwarder and NewDefaultForwarderWithSecrets. Ideally there would only be one function NewDefaultForwarder which needs a secret component, however github.com/open-telemetry/opentelemetry-collector-contrib/pkg/datadog/agentcomponents depends on a NewDefaultForwarder that does not take in secrets so to maintain compatability, NewDefaultForwarder now calls NewDefaultForwarderWithSecrets with noop secrets.
  • the secrets Refresh() function now implements a rate-limit and takes in a boolean to disable the rate-limit

@github-actions github-actions Bot added medium review PR review might take time team/agent-configuration labels Oct 3, 2025
@github-actions github-actions Bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Oct 4, 2025
@s-alad s-alad added the qa/rc-required Only for a PR that requires validation on the Release Candidate label Oct 6, 2025
@s-alad s-alad changed the title expired apikey triggers a secret refresh [AGENTCFG-35] Trigger a secret refresh on expired apikey Oct 7, 2025
@s-alad
s-alad marked this pull request as ready for review October 7, 2025 20:14
@s-alad
s-alad requested review from a team as code owners October 7, 2025 20:14
@s-alad
s-alad requested review from dustmop and hush-hush October 7, 2025 20:14
@s-alad s-alad changed the title [AGENTCFG-35] Trigger a secret refresh on expired apikey [AGENTCFG-35] [WIP] Trigger a secret refresh on expired apikey Oct 7, 2025
@s-alad
s-alad requested a review from dustmop November 19, 2025 23:04
@s-alad
s-alad requested a review from a team as a code owner November 20, 2025 15:43
dustmop
dustmop approved these changes Nov 20, 2025
View reviewed changes

@dustmop dustmop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the fixes, LGTM!

@s-alad s-alad mentioned this pull request Nov 20, 2025
Merged
1 task
vickenty
vickenty approved these changes Nov 24, 2025
View reviewed changes

@vickenty vickenty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a small nit, but otherwise LGTM.

Comment thread comp/forwarder/defaultforwarder/default_forwarder.go Outdated
@s-alad

s-alad commented Nov 24, 2025

Copy link
Copy Markdown
Member Author

/merge

@dd-devflow-routing-codex

dd-devflow-routing-codex Bot commented Nov 24, 2025 •
edited
Loading

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-11-24 21:07:08 UTC ℹ️ Start processing command /merge

2025-11-24 21:07:13 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 42m (p90).

2025-11-24 21:42:16 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue
dd-mergequeue Bot merged commit d4c488e into main Nov 24, 2025
313 of 314 checks passed
@dd-mergequeue
dd-mergequeue Bot deleted the saad/expired-apikey-trigger-secret-refresh branch November 24, 2025 21:42
@github-actions github-actions Bot added this to the 7.74.0 milestone Nov 24, 2025
@s-alad
s-alad restored the saad/expired-apikey-trigger-secret-refresh branch December 11, 2025 22:17
@ddrthall ddrthall mentioned this pull request Feb 23, 2026
Merged
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Apr 7, 2026
@ddrthall
[ AGNTLOG-552 ] Retry log payloads on HTTP 403 and trigger async API …
12b68d8 
…key refresh (#46704)

Instead of dropping log payloads on 403 Forbidden responses, treat them as retryable errors and trigger a throttled secrets refresh so the next retry attempt uses the updated API key. This new behavior only takes effect if a viable (non-nil) secrets component is supplied to Logs Pipeline, indicating a key refresh is possible to perform.

### What does this PR do?

**Core 403 retry logic**

- **`pkg/logs/client/http/destination.go`**: On HTTP 403, if a `secrets.Component` is present, call `secrets.Refresh(false)` (throttled async refresh) and return `RetryableError` instead of dropping the payload. Without secrets (`nil`), the existing drop behavior is preserved.
- **`pkg/logs/pipeline/provider.go`**: Add `NewProviderWithSecrets` constructor that accepts an optional `secrets.Component` and threads it down to `NewDestination`. The existing `NewProvider` delegates to this with `nil` for backward compatibility.

**Wiring secrets to callers**

- **Core logs agent** (`comp/logs/agent/agentimpl/`): Inject `secrets.Component` via fx and pass it through `NewProviderWithSecrets`.
- **Event platform forwarder** (`comp/forwarder/eventplatform/eventplatformimpl/`): Inject `secrets.Component` via fx and pass it through `NewHTTPSender` to `NewDestination`. The noop forwarder passes `nil`.
- **CWS and compliance reporters** (`pkg/security/reporter/`, `pkg/compliance/`): Accept `secrets.Component` and call `NewProviderWithSecrets`. Intermediate call chains in `pkg/security/module/` and `pkg/compliance/` updated to thread it through.

**Behavior on other Agent binaries**

- **security-agent** and **cluster-agent** already have a real `secrets.Component` in their fx graphs; updated their FX providers and call sites to pass it through to `StartRuntimeSecurity`, `StartCompliance`, and `runCompliance`.
- **system-probe** uses `secretsnoopfx` (no-op secrets); added `Secrets secrets.Component` to `FactoryDependencies` and passed it through to `NewCWSConsumer` and `StartCompliance` so the plumbing is consistent, but no actual refresh occurs.

**Test fixes**

- **`comp/networkpath/npcollector/npcollectorimpl/npcollector_testutils.go`**: Switched from `eventplatformimpl.Module` to `MockModule()` to avoid the real EP forwarder's new secrets dependency and a shutdown timeout caused by 403 retries during test teardown.
- New `TestForbiddenTriggersSecretsRefreshAndRetry` unit test in `destination_test.go`. Remaining test files updated to provide `secretsnoopfx.Module()` or pass `nil` for the new `secrets.Component` parameter.

### Motivation

This is the logs-agent counterpart to PR #41589, which added API key refresh on 403 to the core forwarder. The core forwarder's health check already handles 403s from its own HTTP calls, but the logs pipeline and event platform forwarder use a separate `pkg/logs/client/http/Destination` that previously dropped payloads on 403. This PR extends the same secret-refresh-on-403 pattern to those codepaths.

The secrets component is threaded into every process that has access to it via fx (core agent, security-agent, cluster-agent, system-probe), so that 403 retry + key refresh is available wherever the logs HTTP destination is used. Callers without the secrets component (serverless, OTel) retain the original drop behavior.

### Describe how you validated your changes

- **Unit tests**: New `TestForbiddenTriggersSecretsRefreshAndRetry` in `destination_test.go` using a mock secrets component that tracks `Refresh` calls.
- **Manual test**: Configured agent with a throwaway API key (`ENC[datadog_api_key]` + `secret_backend_command`), verified 403 retry + secret refresh + successful log delivery after key update via `dda inv agent.exec -- status`.

### Additional Notes

- Callers without the secrets component (connectivity checks, noop forwarder, sync destinations for serverless/OTel) pass `nil`, preserving the existing 403-drops-payload behavior.
- The EP forwarder has a pre-existing shutdown ordering issue (context canceled after pipeline stop) that becomes visible when 403s are retried. This is tracked by the existing TODO at `epforwarder.go:459` and is out of scope for this PR.

Co-authored-by: ryan.hall <ryan.hall@datadoghq.com>
@gh-worker-devtools-f43747
gh-worker-devtools-f43747 Bot deleted the saad/expired-apikey-trigger-secret-refresh branch May 27, 2026 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hush-hush hush-hush hush-hush approved these changes
@lebauce lebauce lebauce approved these changes
@truthbk truthbk truthbk approved these changes
@vickenty vickenty vickenty approved these changes
@fisherevans fisherevans fisherevans approved these changes
@ichinaski ichinaski ichinaski approved these changes
@dustmop dustmop dustmop approved these changes
@wdhif wdhif wdhif approved these changes
@songy23 songy23 songy23 approved these changes
@aureleoules aureleoules aureleoules approved these changes
@git-thuerk-done git-thuerk-done git-thuerk-done approved these changes
@daniel-taf daniel-taf daniel-taf approved these changes

Assignees

No one assigned

Labels

long review PR is complex, plan time to review it qa/rc-required Only for a PR that requires validation on the Release Candidate team/agent-configuration

Projects

None yet

Milestone

7.74.0

Development

Successfully merging this pull request may close these issues.