WIF-48: credential Provider in delegatedauth + DELA directive discovery - #55479
WIF-48: credential Provider in delegatedauth + DELA directive discovery#55479wynbennett wants to merge 16 commits into
Conversation
…covery This PR adds a credential Provider interface to the delegatedauth component and discovers DELA directives, handing them to consumers as providers. Changes: - Add credential Provider to delegatedauth (provider.go, provider_test.go) - Extract writeback.go and status.go from delegatedauth.go - Consolidate CredentialProvider redeclarations and document InstanceParams - Add delegated_auth_endpoints.go for DELA directive discovery - Wire delegated auth config in pkg/config/setup/config.go - Add delegated_auth.go to pkg/config/model - Update endpoints.go in pkg/config/utils - Update noop-impl, mock, and def for new Provider interface
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 219c7a086f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Directive: directiveText, | ||
| TargetSite: destination, | ||
| FallbackAPIKey: resolveFallbackAPIKey(secretResolver, directive.params["fallback"], configKey), | ||
| SkipConfigWriteback: true, |
There was a problem hiding this comment.
With SkipConfigWriteback: true, a successful exchange only updates the provider registry, but none of the three consumers registered above uses that registry: the default forwarder builds authorizers solely from APIKeys.Keys, trace appendEndpoints still copies the configured string into Endpoint.APIKey, and logs endpoints still use the configured api_key; a repository-wide search finds no production call to ProvidersFor or ProviderForDirective. Consequently, an additional_endpoints directive leaves the infrastructure endpoint with zero authorizers, while the APM and logs paths send the literal DELA(...) value, even after the real key resolves. Wire provider lookup into all three send paths before suppressing writeback, or retain the existing writeback delivery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is by design — this PR is the foundation layer only. The three consumers (forwarder, trace writers, logs agent) are wired to call ProvidersFor / ProviderForDirective on the send path in PR #55480. SkipConfigWriteback: true is set only for additional_endpoints directives, where the resolved credential reaches the consumer through the Provider registry rather than the config tree. Without PR #55480 the providers are registered but not yet consumed.
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change can use the wrong AWS region for one directive. It can also keep an old provider or an old fallback key.
🤖 Datadog Autotest · Commit 219c7a0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| if err != nil { | ||
| return "", err | ||
| } | ||
| resolved, err := secretResolver.Resolve(wrapped, origin, "", "", false) |
There was a problem hiding this comment.
The Agent can keep an expired fallback key and stop sending data when delegated authentication is unavailable.
Assertion details
- Input: A DELA directive uses fallback=ENC[...] and secret refresh changes that secret.
- Expected:
A secret refresh must update the instance fallback key and its credential provider. - Actual:
The one-time resolution records the secret under a synthetic origin. The existing configuration subscriber ignores updates for that origin. The instance keeps the first fallback key after the secret changes.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
Documented as a known limitation with a TODO on resolveFallbackAPIKey in commit 546adfc. The resolved value is a snapshot at config-load time. A proper fix requires passing the raw ENC[...] handle and the secrets resolver to the component so it can re-resolve on secret refresh, or re-running discovery when the secrets subscriber fires. This is tracked for a follow-up.
Go Package Import DifferencesBaseline: d725d90
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: cfd8b79 | Docs | View more details | Give us feedback! |
Files inventory check summaryFile checks results against ancestor c35c49a9: Results for datadog-agent_7.84.0~devel.git.592.bb228e6.pipeline.134336743-1_amd64.deb:Detected file changes:
|
Static quality checks❌ Please find below the results from static quality gates Error
Gate failure full details
Static quality gate failures prevent this PR from merging! Successful checksInfo
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: d725d90 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | +0.83 | [+0.62, +1.05] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.79 | [+0.56, +1.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.51 | [+0.47, +0.55] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.38 | [+0.29, +0.46] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | +0.29 | [-0.60, +1.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.25 | [+0.22, +0.28] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.04 | [+0.01, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.01 | [-0.04, +0.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.19 | [-0.30, -0.07] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | -0.23 | [-0.54, +0.07] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 173.19MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 748.40KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 526.51MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 211.83MiB ≤ 228MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.54MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 366.06 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 21 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 424.93MiB ≤ 455MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.73MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.39 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 324.60MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 70.12 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 302.75MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.30 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 312.55MiB ≤ 345MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
Fix three findings from the Codex/Autotest review: 1. (P1) Per-directive regions leak into shared initialization initializeIfNeeded could store the first directive's ProviderConfig (e.g. region=us-east-1) as the component-wide default. Later directives with no overrides would inherit that region instead of auto-detecting. Fix: always auto-detect in initializeIfNeeded; per-instance config is applied separately by providerConfigForInstance. 2. (P2) Provider replacement keeps old provider registerProvider appended without removing the old entry for the same directive. Lookup could return the cancelled provider. Fix: remove any existing entry for the same directive before appending. 3. (P2) Secret refresh does not update fallback key The fallback ENC[...] handle is resolved once at config-load time and never re-resolved. Documented as a known limitation with a TODO.
Review findings addressed in commit
|
…nges Add Refresh() bool to the Provider interface so the forwarder can nudge a re-exchange on 403, matching the secrets.Refresh() contract. The implementation resets the credential to buffering (Authorize returns false, transaction requeues) and sends a non-blocking signal to a capacity-1 channel — a burst of 403s coalesces into a single refresh, no storm. Add a credential cache keyed by (orgUUID, targetSite) so two DELA directives for the same org against the same site share one instanceProvider and one background refresh goroutine instead of each making a redundant WIF exchange. Dedup applies only in directive mode (SkipConfigWriteback); the flat-key path still needs one authInstance per config slot for write-back. Update all Provider implementations (mock, noop, test stubs) with Refresh(). WIF-48
Consolidates the per-package stubProvider copies duplicated across the resolver, logs, trace writer, and trace API tests into one shared test helper in comp/core/delegatedauth/mock. WIF-48
…import _test.go files are only visible within their own package. Rename to stub_provider.go so consumer test packages can import StubProvider from delegatedauth/mock. Update BUILD.bazel via gazelle. WIF-48
Create pkg/credential as the canonical home for the Provider interface, IsDirective, StampAuth, and Lookup types. This eliminates duplication of the CredentialProvider type and provider-resolution logic across 6 consumer subsystems. - pkg/credential/provider.go: Provider interface (Authorize + Refresh) - pkg/credential/directive.go: DirectivePrefix const, IsDirective func - pkg/credential/authorize.go: StampAuth helper (provider-or-static-key) - pkg/credential/lookup.go: Lookup func type - comp/core/delegatedauth/def: Provider is now an alias for credential.Provider - Add pkg/credential/go.mod and replace directives to all submodule go.mod files WIF-48
Merge main into foundation branch. The merge conflicted on comp/otelcol/ddflareextension/impl/go.mod where our branch added pkg/credential as an indirect require and main added golang.org/x/sys; both are kept. Register pkg/credential in modules.yml so modules.add-all-replace auto-detects it and generates correct relative replace paths in all sub-module go.mod files (previously the replace directives were manually added with incorrect relative paths). WIF-48
Without this entry golangci-lint fails with: typechecking error: pattern ./pkg/credential/...: main module does not contain package github.com/DataDog/datadog-agent/pkg/credential WIF-48
bazel mod tidy removes this entry because local modules are resolved via replace directives, not go_deps. No other datadog_agent_pkg_* module appears in deps/go.MODULE.bazel. WIF-48
Same owners as comp/core/delegatedauth. WIF-48
Required because test/fakeintake/go.mod changed (pkg/credential replace directive). WIF-48
BenchmarksBenchmark execution time: 2026-09-01 22:00:39 Comparing candidate commit cfd8b79 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics.
|
1 participant
Summary
This PR adds the foundation for delegated authentication (WIF-48): a credential
Providerinterface on thedelegatedauthcomponent and a DELA directive discovery mechanism that scansadditional_endpointsconfig keys forDELA(...)directives and registers a credential provider for each one.What is a DELA directive?
A DELA directive is a string of the form
DELA(<org_uuid>, aws)orDELA(<org_uuid>, aws, region=us-east-1, fallback=ENC[...])that an operator places in anadditional_endpointsapi_key field instead of a static API key. The agent exchanges a cloud auth proof (e.g. an AWS STS token) for a Datadog API key at the target site, and hands the resolved credential to consumers through theProviderinterface. This keeps the resolved key out of the config tree entirely — the directive stays in place and the credential flows through the provider registry.Provider lifecycle
A fallback is only ever used after an attempt has actually failed. While the very first exchange is still in flight the provider reports "not yet", so callers hold their payloads instead of shipping them under a key the operator only meant as a safety net.
What this PR provides
Providerinterface (def/component.go):Authorize(http.Header) bool— consumers call this on every request; returnsfalsewhile resolving (buffer),trueonce a key is available (stamps the header).instanceProvider(impl/provider.go): the concrete provider for one delegated-auth instance, managing the three-state lifecycle above using atomic swaps.pkg/config/setup/delegated_auth_endpoints.go): parses directives from both map-shape (additional_endpoints,apm_config.additional_endpoints) and list-shape (logs_config.additional_endpoints) config keys, resolvesfallback=ENC[...]handles through the secrets backend, and registers an instance per directive withSkipConfigWriteback: trueso the resolved key never enters the config tree.pkg/config/model/delegated_auth.go): theDelaDirectivePrefixconstant shared between setup and utils.delegatedauth.gosplit —writeback.goandstatus.goextracted as separate files. Noop-impl, mock, and def updated for the newProviderinterface.Review findings addressed (commit
546adfc)initializeIfNeedednow always auto-detects; per-instanceProviderConfigapplied separately byproviderConfigForInstance.registerProviderremoves the old entry for the same directive before appending.What this PR does NOT do
The three data-plane consumers (forwarder, trace writers, logs agent) are not yet wired to call
ProvidersFor/ProviderForDirectiveon the send path. That wiring is in PR #55480.Architecture doc
See Delegated Authentication in the Agent: Architecture and Path Forward for the full design, including the previous approach vs. the new Provider interface, locking, and future work.
Stacked PRs
WIF-48