-
Notifications
You must be signed in to change notification settings - Fork 11
Release prep 1.2.4: avoid DNS for disabled ADP bootstrap #2058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #!/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 | ||
|
Comment on lines
+1
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in fe6fc2e1c7. The unused CA certificate installation script was removed. |
||
| # certificates and may run as a non-root user, so we skip the install (and avoid needing root just to | ||
| # run apt-get). For local builds the plain Ubuntu base lacks them but runs as root, so the install | ||
| # succeeds. Use the repository's available version because Ubuntu repositories do not retain old | ||
| # exact package versions indefinitely. | ||
|
|
||
| set -eu | ||
|
|
||
| if [ -d /usr/share/ca-certificates ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| apt-get update | ||
| apt-get install --no-install-recommends -y ca-certificates | ||
| apt-get clean | ||
| rm -rf /var/lib/apt/lists | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,7 +59,7 @@ impl RemoteAgentClient { | |
| 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(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This disables DNS for every remote-agent IPC connector, but Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5be14cda44. |
||
| #[cfg(target_os = "linux")] | ||
|
Comment on lines
59
to
63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5be14cda44. DNS resolution is retained for hostname-based |
||
| let connector_builder = if let Some(addr) = config.vsock_addr()? { | ||
| connector_builder.with_vsock_addr(addr) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,6 +421,7 @@ pub struct HttpsCapableConnectorBuilder { | |
| bytes_sent: Option<Counter>, | ||
| error_telemetry: Option<HttpTransactionErrorTelemetry>, | ||
| conn_age_limit: Option<Duration>, | ||
| dns_resolution_disabled: bool, | ||
| http_protocol: HttpProtocol, | ||
| #[cfg(unix)] | ||
| unix_socket_path: Option<PathBuf>, | ||
|
|
@@ -476,6 +477,18 @@ impl HttpsCapableConnectorBuilder { | |
| self | ||
| } | ||
|
|
||
| /// Disables DNS resolution for this connector. | ||
| /// | ||
| /// This is intended for clients that only use transports or destinations that do not require | ||
| /// 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5be14cda44. The documentation now states explicitly that DNS resolution is enabled by default. |
||
| pub fn without_dns_resolution(mut self) -> Self { | ||
| self.dns_resolution_disabled = true; | ||
| self | ||
| } | ||
|
|
||
| /// Sets a Unix domain socket path to route all connections through. | ||
| /// | ||
| /// When set, the connector will connect to this Unix socket instead of performing DNS resolution | ||
|
|
@@ -514,7 +527,13 @@ impl HttpsCapableConnectorBuilder { | |
| #[cfg(not(target_os = "linux"))] | ||
| let vsock_only = false; | ||
|
|
||
| let hickory_resolver = if vsock_only { | ||
| #[cfg(unix)] | ||
| let unix_socket_only = self.unix_socket_path.is_some(); | ||
| #[cfg(not(unix))] | ||
| let unix_socket_only = false; | ||
|
|
||
| let dns_resolution_disabled = self.dns_resolution_disabled || vsock_only || unix_socket_only; | ||
| let hickory_resolver = if dns_resolution_disabled { | ||
| HickoryResolver::noop() | ||
| } else { | ||
| build_dns_resolver(&self.error_telemetry)? | ||
|
|
@@ -611,7 +630,7 @@ pub(super) fn check_connection_state(captured_conn: CaptureConnection) { | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{configure_tls_alpn_for_http_protocol, HttpProtocol}; | ||
| use super::{configure_tls_alpn_for_http_protocol, HttpProtocol, HttpsCapableConnectorBuilder}; | ||
|
|
||
| fn empty_tls_config() -> rustls::ClientConfig { | ||
| rustls::ClientConfig::builder_with_provider(rustls::crypto::aws_lc_rs::default_provider().into()) | ||
|
|
@@ -635,6 +654,14 @@ mod tests { | |
| assert!(tls_config.alpn_protocols.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn connector_builds_without_dns_resolution() { | ||
| HttpsCapableConnectorBuilder::default() | ||
| .without_dns_resolution() | ||
| .build(empty_tls_config()) | ||
| .expect("connector should build with DNS resolution disabled"); | ||
| } | ||
|
|
||
| // vsock takes priority over unix when both are configured, matching Agent behavior. | ||
| // We verify by checking the error does not mention "unix" — if unix had priority it would | ||
| // fail with a socket-path error; vsock produces a connection or device error instead. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # Issue: ADP/Core Agent IPC must not require system DNS. | ||
| # | ||
| # The Core Agent IPC endpoint is a literal loopback address (`https://127.0.0.1:<cmd_port>`), | ||
| # so ADP should not need to load `/etc/resolv.conf` just to register with the Core Agent or | ||
| # receive its initial configuration. This test clears the resolver config at container startup to | ||
| # reproduce hosts where Hickory cannot load system DNS configuration. | ||
| # | ||
| # The test currently runs ADP in standalone mode until converged OTLP proxy endpoint ownership is | ||
| # configurable; see the standalone-mode env comment below. | ||
|
|
||
| type: integration | ||
| name: "adp-ipc-no-dns" | ||
| description: "Verifies ADP can start when /etc/resolv.conf has no nameservers" | ||
| timeout: 120s | ||
| runtimes: [linux] | ||
|
|
||
| env: | ||
| DD_API_KEY: "00000000000000000000000000000000" | ||
| DD_HOSTNAME: "integration-test-ipc-no-dns" | ||
| DD_DATA_PLANE_ENABLED: "true" | ||
| # Run standalone for now because converged mode streams the Core Agent OTLP | ||
| # receiver endpoints back to ADP. That prevents us from configuring ADP to own | ||
| # the normal OTLP listener ports while the Core Agent listens on dummy upstream | ||
| # ports for proxying. | ||
| DD_DATA_PLANE_STANDALONE_MODE: "true" | ||
|
aqian01 marked this conversation as resolved.
|
||
| # OTLP proxy mode keeps ADP running without creating the normal Datadog forwarder, | ||
| # so this test avoids normal intake DNS while /etc/resolv.conf is empty. | ||
| DD_DATA_PLANE_DOGSTATSD_ENABLED: "false" | ||
| DD_DATA_PLANE_OTLP_ENABLED: "true" | ||
| DD_DATA_PLANE_OTLP_PROXY_ENABLED: "true" | ||
|
|
||
| container: | ||
| files: | ||
| - "empty-resolv-conf.sh:/etc/cont-init.d/01-empty-resolv-conf.sh" | ||
| exposed_ports: | ||
| - "58125/udp" | ||
|
|
||
| procedure: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new case is not loadable by Panoramic: Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5be14cda44. The test now uses the required top-level |
||
| - 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 | ||
|
Comment on lines
+38
to
+51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5be14cda44. The test now uses |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| #!/bin/sh | ||
| set -eu | ||
|
|
||
| : > /etc/resolv.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in fe6fc2e1c7. The condition now checks
/usr/share/ca-certificatesand groups the update, install, and cleanup commands so they run only when the directory is absent.