Add Config Discovery support for Vault - #24851
Draft
martavicentenavarro wants to merge 5 commits into
Draft
Conversation
Adds a `discovery:` strategy block to Vault's spec.yaml targeting port 8200, with candidates covering the legacy/OpenMetrics x token/no-token dispatch matrix in priority order (rich OpenMetrics, safe OpenMetrics, rich legacy, safe legacy). Wires `get_e2e_discovery_metadata()` into the E2E docker environment and adds unit and E2E coverage for the generated candidates. Known gap: `test_e2e_discovery`'s `no_token` candidate never authenticates against Vault, so 24 metrics that only appear once a request passes through Vault's authenticated logical-request pipeline (audit logging, ACL/token checks, policy lookups, lease issuance) are excluded from that test's assertions. These metrics remain fully covered by the existing `test_e2e` suite. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
martavicentenavarro
added
the
qa/skip-qa
Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
evalya-impact-summaryevalya impact analysis |
Contributor
|
Contributor
Author
![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: 4ef4aae0b7
ℹ️ 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".
| - 8200 | ||
| candidates: | ||
| - use_openmetrics: "true" | ||
| api_url: "http://{service.host}:{port.number}/v1" |
There was a problem hiding this comment.
When a Vault listener has TLS enabled, every generated candidate uses plaintext http://, so each probe fails before it can collect the leader/health metric and discovery returns no configuration. This excludes secure production Vault deployments even though the check's api_url supports HTTPS; include HTTPS candidates or derive the listener scheme rather than only probing HTTP.
Useful? React with 👍 / 👎.
Vault's check only skips its metrics scrape when neither a token nor `no_token` is configured (see `VaultCheckV2.metric_collection_enabled` and the equivalent legacy-check gating), so the two `no_token: false` candidates never attempt to collect metrics at all — they trivially "succeed" on the always-unauthenticated `is_leader`/health output alone. With the previous ordering (openmetrics+no_token, openmetrics, legacy+no_token, legacy), a failed openmetrics scrape on the first candidate would let the second, health-only candidate win immediately, permanently starving out the third candidate even though it could still collect the full metric set. Reorder so both `no_token: true` candidates are tried, in either mode, before either health-only fallback. Flagged by Codex review on PR #24851. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The discovery templates rely on datadog-checks-base bracketing IPv6-literal
hosts when interpolating `{service.host}` into a candidate URL. That fix
landed in datadog-checks-base 38.0.0 (37.41.0 predates it), so the
minimum-base-package CI job fails test_ipv6_host_is_bracketed_in_generated_api_url
with the previously declared 37.41.0 floor.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The two "with token" candidates omitted `no_token` without configuring `client_token`/`client_token_path`. `VaultCheckV2.metric_collection_enabled` (and the equivalent legacy-check gating) skips the metrics scrape entirely in that case, so those candidates could only ever emit the always-unauthenticated leader/health metrics. Discovery accepts the first candidate whose check run collects at least one metric with no error, so a health-only candidate would trivially "succeed" and get locked in permanently -- a degraded config masquerading as working, with no core metrics. Discovery has no way to synthesize a token on its own, so `no_token: true` is the only signal it can produce that guarantees a real metrics scrape. Remove the two credential-less candidates, keeping only the OpenMetrics and legacy `no_token: true` candidates. Updates tests/test_discovery.py for the two-candidate expectation and adds an explicit assertion that every generated candidate enables metric collection (`no_token` true, since no credential is ever synthesized). Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds Config Discovery support to the Vault integration.
assets/configuration/spec.yamlnow declares adiscovery:strategy block that matches on port8200and thevaultcontainer image, generating two candidates in priority order, both withno_token: true:This covers Vault's
use_openmetricsdispatch (OpenMetrics vs. legacy collection). Both candidates setno_token: truedeliberately: discovery has no way to obtain aclient_token/client_token_pathon its own, and without one ofno_token,client_token, orclient_token_pathset,VaultCheckV2.metric_collection_enabled(and the equivalent legacy-check gating) disables the metrics scrape entirely — the check would only ever reach Vault's always-unauthenticated/sys/healthand/sys/leaderendpoints. Since discovery accepts the first candidate whose run completes without error and collects at least one metric, a token-less candidate withoutno_tokencould still "succeed" on health/leader data alone and get locked in permanently as a degraded config with no real metrics. Earlier candidates that combineduse_openmetrics/legacy mode without any credential signal were removed for this reason; only the twono_token: truecandidates remain.Also:
get_e2e_discovery_metadata()into the E2E docker environment (tests/conftest.py) so the Agent container running E2E discovery tests can read container labels.datadog-checks-basefloor to>=38.0.0— the version that ships both the discovery runtime (Service/candidate_ports) the generateddiscovery.pyimports, and a fix for bracketing IPv6 hosts in discovery-generated URLs.tests/test_discovery.py(unit coverage for the generated candidate list, API URL construction, and an explicit assertion that every candidate enables metric collection) andtest_e2e_discovery/test_e2e_discovery_all_candidates(E2E coverage for the generated candidates against a real Vault container).Known gap: HTTP only. Generated candidates use
http://exclusively; there is no auto-generated HTTPS/TLS candidate. The check already supports TLS today via manual configuration (api_url: https://...plus the existingtls_ca_cert,tls_cert,tls_verify, and related options), but discovery has no per-deployment field to supply a trusted CA bundle, and defaulting an auto-generated HTTPS candidate totls_verify: falsewould be unsafe. This PR intentionally scopes discovery to HTTP listeners; users running Vault with TLS should continue to configure the integration manually.Known gap:
test_e2e_discoveryexercises theno_tokendiscovery candidate, which only ever hits Vault's unauthenticated status endpoints (/sys/health,/sys/leader, metrics scrape). 24 metrics that are only emitted once a request passes through Vault's authenticated logical-request pipeline (audit logging of the request/response, ACL/token checks, policy lookups, lease issuance) can't be produced by that candidate, so they're excluded from this test's assertions (see theexcludelist and comment intests/test_e2e.py). These metrics remain fully covered by the existingtest_e2esuite, which authenticates.Motivation
Config Discovery lets the Agent automatically detect and configure integrations from running containers, without requiring a hand-written Autodiscovery template. Vault didn't yet support this; this PR brings it in line with other integrations that have already adopted the discovery framework (krakend, n8n, ray, pulsar, temporal).
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged