Release prep 1.2.4: avoid DNS for disabled ADP bootstrap - #2058
Conversation
- Gate remote-agent bootstrap on `data_plane.enabled` so default-disabled ADP exits before creating the Core Agent IPC client. - Add an explicit `HttpsCapableConnectorBuilder::without_dns_resolution()` mode. - Use no-DNS connector construction for Core Agent IPC, which targets `https://127.0.0.1:<cmd_port>` and does not require hostname resolution. - Avoid system DNS resolver construction for DNS-free connector transports such as Unix sockets and vsock. On hosts without nameservers in `/etc/resolv.conf`, ADP could fail during startup while constructing the HTTP connector for Core Agent IPC. This happened before ADP reached the disabled-exit path, creating noisy crash loops even when `data_plane.enabled` was false. unit tests, integration test. Co-authored-by: andrew.qian <andrew.qian@datadoghq.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
dd-octo-sts
Bot
added
area/io
![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: 8882b51695
ℹ️ 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".
| exposed_ports: | ||
| - "58125/udp" | ||
|
|
||
| procedure: |
There was a problem hiding this comment.
This new case is not loadable by Panoramic: IntegrationConfig requires a top-level assertions list, and assertion entries are tagged with type, while this file uses procedure and nested assertion keys. When make test-integration discovers this directory, deserialization fails with the missing assertions field and the discovery path panics, so the integration suite cannot start while this config is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5be14cda44. The test now uses the required top-level assertions field and type-tagged assertion entries, so Panoramic can discover it.
| let ipc_cert_file_path = config.auth().ipc_cert_file_path(); | ||
| let client_tls_config = build_ipc_client_ipc_tls_config(ipc_cert_file_path).await?; | ||
| let connector_builder = HttpsCapableConnectorBuilder::default(); | ||
| let connector_builder = HttpsCapableConnectorBuilder::default().without_dns_resolution(); |
There was a problem hiding this comment.
This disables DNS for every remote-agent IPC connector, but RemoteAgentClientConfiguration still supports agent_ipc_endpoint when cmd_port is absent, including non-loopback URIs such as https://agent.example:5001. In that supported configuration the connector is built with the noop resolver, so hostname endpoints fail before they can connect; only disable DNS when the selected endpoint is a literal IP or when a non-DNS transport such as vsock is used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5be14cda44. RemoteAgentClient now preserves DNS resolution for hostname endpoints and disables it only for literal IPv4/IPv6 endpoints; vsock continues to bypass DNS.
There was a problem hiding this comment.
Pull request overview
Release-prep for ADP 1.2.4 on releases/1.2.x, backporting the “avoid DNS during IPC bootstrap” fix to prevent startup failures on hosts without usable DNS configuration.
Changes:
- Add a “no DNS resolution” mode to the HTTPS-capable HTTP connector builder and unit-test it.
- Use the no-DNS connector for Core Agent IPC client construction, and add an integration test case for empty
/etc/resolv.conf. - Update container CA-certs install behavior and bump ADP version to
1.2.4.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/integration/cases/adp-ipc-no-dns/empty-resolv-conf.sh |
Adds init script to blank /etc/resolv.conf in the integration container. |
test/integration/cases/adp-ipc-no-dns/config.yaml |
Adds an integration test intended to validate ADP startup without DNS. |
lib/saluki-io/src/net/client/http/conn.rs |
Adds without_dns_resolution() to build connectors without constructing a system DNS resolver. |
lib/datadog-agent/commons/src/ipc/client/mod.rs |
Switches IPC client to use the no-DNS connector builder. |
docker/scripts/agent-data-plane/app/00-install-ca-certs.sh |
Adds a script intended to install CA certs only when missing. |
docker/Dockerfile.proxy-dumper |
Removes ca-certificates version pin and adjusts CA-certs install comment. |
Cargo.lock |
Bumps agent-data-plane package version to 1.2.4. |
bin/agent-data-plane/Cargo.toml |
Bumps ADP crate version to 1.2.4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RUN test -d /usr/local/share/ca-certificates || apt-get update && \ | ||
| apt-get install -y --no-install-recommends ca-certificates=20240203 && \ | ||
| apt-get install -y --no-install-recommends ca-certificates && \ | ||
| apt-get clean |
There was a problem hiding this comment.
Addressed in fe6fc2e1c7. The condition now checks /usr/share/ca-certificates and groups the update, install, and cleanup commands so they run only when the directory is absent.
| let auth_interceptor = BearerAuthInterceptor::from_file(&config.auth().auth_token_file_path()).await?; | ||
| let ipc_cert_file_path = config.auth().ipc_cert_file_path(); | ||
| let client_tls_config = build_ipc_client_ipc_tls_config(ipc_cert_file_path).await?; | ||
| let connector_builder = HttpsCapableConnectorBuilder::default(); | ||
| let connector_builder = HttpsCapableConnectorBuilder::default().without_dns_resolution(); | ||
| #[cfg(target_os = "linux")] |
There was a problem hiding this comment.
Addressed in 5be14cda44. DNS resolution is retained for hostname-based agent_ipc_endpoint values and disabled only for literal IPv4/IPv6 endpoints.
| /// DNS, such as Unix sockets, vsock, or literal-IP TCP endpoints. Hostname-based TCP | ||
| /// destinations will fail to resolve when this is enabled. | ||
| /// | ||
| /// Defaults to enabled. |
There was a problem hiding this comment.
Addressed in 5be14cda44. The documentation now states explicitly that DNS resolution is enabled by default.
| #!/usr/bin/env sh | ||
| # | ||
| # Ensures CA certificates are present in the final image. | ||
| # | ||
| # We only install them if they're missing. In CI, the application base image already ships CA |
There was a problem hiding this comment.
Addressed in fe6fc2e1c7. The unused CA certificate installation script was removed.
| procedure: | ||
| - parallel: | ||
| - assertion: process_stable_for | ||
| duration: 10s | ||
| - assertion: log_contains | ||
| pattern: "Topology healthy" | ||
| timeout: 60s | ||
| - assertion: log_not_contains | ||
| pattern: "Failed to load system DNS configuration when creating DNS resolver for HTTP client" | ||
| during: 10s | ||
| - assertion: log_not_contains | ||
| pattern: "panic|PANIC" | ||
| regex: true | ||
| during: 10s |
There was a problem hiding this comment.
Addressed in 5be14cda44. The test now uses assertions and type, matching the Panoramic integration-test schema.
|
Binary Size Analysis (Agent Data Plane)Baseline: b02c083 · Comparison: 8882b51 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ❌ 1 regression detected
Fine details of change detection per experiment (34)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
## Summary Fixes issues found while reviewing the ADP backport in #2058. - Keeps DNS enabled for hostname IPC endpoints. - Disables DNS only for literal-IP and vsock endpoints. - Fixes the proxy-dumper CA certificate condition. ## Validation - `make check-fmt` - `cargo nextest run -p datadog-agent-commons ipc::client::tests` - `target/debug/panoramic list -d test/integration/cases --runtime linux` Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>
… backport) (#2080) ## Summary Backports #2060 onto the ADP 1.2.4 backport in #2058. - Keeps DNS enabled for hostname IPC endpoints. - Disables DNS only for literal-IP and vsock endpoints. - Fixes the 1.2.x integration test schema. - Fixes the proxy-dumper CA certificate condition and removes the unused certificate script. ## Validation Unit tests and Panoramic discovery. Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>
… backport) (#2080) ## Summary Backports #2060 onto the ADP 1.2.4 backport in #2058. - Keeps DNS enabled for hostname IPC endpoints. - Disables DNS only for literal-IP and vsock endpoints. - Fixes the 1.2.x integration test schema. - Fixes the proxy-dumper CA certificate condition and removes the unused certificate script. ## Validation Unit tests and Panoramic discovery. Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>
Summary
Release-prep PR for ADP 1.2.4 on
releases/1.2.x. Cherry-pick of #2041 plus a version bump.Backports the fix that avoids DNS lookups during ADP bootstrap when IPC is disabled, preventing startup failures in environments where DNS resolution isn't available.
Commits
c8a82cb75a(fix(agent-data-plane): avoid DNS for disabled ADP bootstrap #2041) —fix(agent-data-plane): avoid DNS for disabled ADP bootstrapchore(dev): Bump ADP to 1.2.4Test plan
cargo checkpasses for the affected crates on the 1.2.x toolchain.🤖 Generated with Claude Code