nginx: Add container-based config discovery support - #24118
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2edb963 | Docs | Datadog PR Page | Give us feedback! |
9e08357 to
93d8da4
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8eba3f4 to
16c525a
Compare
There was a problem hiding this comment.
Pull request overview
Adds Autodiscovery “configuration discovery” support for the nginx integration so the Agent can auto-schedule the check for containers identified as nginx, without requiring a manual conf.yaml.
Changes:
- Add
auto_conf.yamlandspec.yamldiscovery stanza to enable container-based config discovery (port-hint 80,/nginx_status). - Add generated discovery plumbing (
config_models/discovery*.py) and an e2e discovery test path. - Add changelog entry documenting the new discovery support.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nginx/tests/test_e2e.py | Adds e2e tests for discovery-driven scheduling and candidate stability. |
| nginx/tests/docker/docker-compose.yaml | Adjusts nginx test container volume wiring. |
| nginx/tests/conftest.py | Returns e2e discovery metadata so discovery runs can mount auto_conf.yaml into the Agent. |
| nginx/datadog_checks/nginx/data/auto_conf.yaml | Adds AD template with ad_identifiers: [nginx] and discovery: {}. |
| nginx/datadog_checks/nginx/config_models/discovery.py | Generated candidate generator for discovery (from ports, builds nginx_status_url). |
| nginx/datadog_checks/nginx/config_models/discovery_strategies.py | Stub for custom local discovery strategies (generated). |
| nginx/datadog_checks/nginx/config_models/discovery_overrides.py | Stub hook for overriding/adjusting generated candidates. |
| nginx/changelog.d/24118.added | Changelog entry for the new discovery capability. |
| nginx/assets/configuration/spec.yaml | Adds discovery strategy + adds auto_conf.yaml to the integration config spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cb5dae9 to
af3841d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9c160ebcc
ℹ️ 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".
|
Putting this as draft for now until we can address the codex comment. |
…egeneration, and dependency floor - Tighten E2E discovery metric assertions from at_least=0 to at_least=1 - Add discovery_overrides.py/discovery_strategies.py stub files - Move ad_identifiers out of the spec discovery stanza and regenerate discovery.py/auto_conf.yaml - Add test_e2e_discovery_all_candidates and fix the changelog filename to match PR #24118 - Add NGINX_CONFIG_FOLDER fallback in docker-compose so docker compose ps works without the env var set - Bump datadog-checks-base floor to >=37.41.0, matching the version that introduced get_e2e_discovery_metadata, assert_all_discovery_candidates_stable, and candidate_ports
c9c160e to
aa22e70
Compare
…er plain HTTP Replace the from_ports strategy with a local:from_nginx_ports strategy that classifies each candidate port as HTTP or HTTPS (by number, 443/8443, or a https/ssl/tls port name) and generates a single scheme-matched candidate per port. This closes the gap Codex flagged: a TLS-only nginx on 443 no longer gets an unusable http:// candidate, and an HTTP candidate is never generated for a known HTTPS port. Add unit tests asserting the generator never produces http://...:443 and that port 80 is still tried before 443/8443.
Addresses review nits from sarah-witt on PR #24118.
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Adds container-based config discovery support to the nginx integration. When the Datadog Agent detects a container with the
nginxAD identifier, it probes the container's exposed ports — starting with port 80 if it is among them — until it finds the nginx status endpoint, then schedules a check against it automatically — no manualconf.yamlneeded. HTTPS candidates are generated for ports 443/8443 and for ports namedhttps/ssl/tls; everything else is probed over HTTP. This keeps the check's TLS verification defaults unchanged, so TLS-only nginx discovery is supported when the certificate is trusted by the Agent. Deployments using self-signed or otherwise untrusted certificates are not covered by this PR unless TLS verification is configured separately.Motivation
DSCVR-509 as part of DSCVR-485. Follows the same pattern established by the krakend integration.
Implementation note — custom discovery strategy: the framework's standard registry strategies (
from_ports,openmetrics_from_ports) generate candidates from a single fixed template, which can't vary thenginx_status_urlscheme (http/https) per port. Since that scheme depends on which port matched, this PR defines a small custom strategy,local:from_nginx_ports(datadog_checks/nginx/config_models/discovery_strategies.py), that classifies each candidate port as HTTP or HTTPS and yields the matching scheme alongside the port.Port selection rationale: Port 80 is nginx's default and documented HTTP port. 443/8443 are treated as HTTPS candidates so TLS-only deployments aren't left undiscoverable.
Endpoint:
/nginx_statusis the standard path for thengx_http_stub_status_module, used by the integration via thenginx_status_urlinstance parameter.Scope: this PR covers the open-source/standard nginx build only, discovered over HTTP or HTTPS depending on the port, and only when the Agent trusts the certificate (see the TLS note above). NGINX Plus is not covered — there's no public Docker image for it to validate against in this repo's e2e setup. Both NGINX Plus support and self-signed/untrusted-certificate discovery are left as possible follow-ups.
docker-compose change: the
${NGINX_CONFIG_FOLDER}mount now has a fallback (${NGINX_CONFIG_FOLDER:-/tmp}). Without one,docker compose ps(called internally by the newtest_e2e_discovery_all_candidatestest helper) fails when the env var isn't set. The fallback is/tmp(a directory, matching/etc/nginx) rather than/dev/null(a file) to keep the mount types consistent — it's never actually used to run the container, since the real config folder is always passed viaenv_vars=in the tests.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