From 7c003ce3581e0c01c8615decf1559b016a21f58a Mon Sep 17 00:00:00 2001 From: meowjesty <43983236+meowjesty@users.noreply.github.com> Date: Tue, 5 Mar 2024 11:59:51 -0300 Subject: [PATCH] Remove resolve_dns special handling from agent. (#2290) * Remove resolve_dns special handling from agent. * changelog * Docs for dns resolution * update schema * enable ignored test * remove notice from ex-ignored test --- changelog.d/2289.fixed.md | 1 + mirrord-schema.json | 2 +- mirrord/agent/src/outgoing/udp.rs | 56 +++++++-------------------- mirrord/config/src/feature/network.rs | 6 +++ mirrord/layer/tests/outgoing.rs | 4 -- 5 files changed, 22 insertions(+), 47 deletions(-) create mode 100644 changelog.d/2289.fixed.md diff --git a/changelog.d/2289.fixed.md b/changelog.d/2289.fixed.md new file mode 100644 index 00000000000..2870d3b8ec2 --- /dev/null +++ b/changelog.d/2289.fixed.md @@ -0,0 +1 @@ +Remove special handling for DNS when dealing with UDP outgoing sockets (manual UDP resolving). \ No newline at end of file diff --git a/mirrord-schema.json b/mirrord-schema.json index e19e42ef796..4d8f7c3fdf2 100644 --- a/mirrord-schema.json +++ b/mirrord-schema.json @@ -857,7 +857,7 @@ "properties": { "dns": { "title": "feature.network.dns {#feature-network-dns}", - "description": "Resolve DNS via the remote pod.\n\nDefaults to `true`.", + "description": "Resolve DNS via the remote pod.\n\nDefaults to `true`.\n\n- Caveats: DNS resolving can be done in multiple ways, some frameworks will use `getaddrinfo`, while others will create a connection on port `53` and perform a sort of manual resolution. Just enabling the `dns` feature in mirrord might not be enough. If you see an address resolution error, try enabling the [`fs`](#feature-fs) feature, and setting `read_only: [\"/etc/resolv.conf\"]`.", "type": [ "boolean", "null" diff --git a/mirrord/agent/src/outgoing/udp.rs b/mirrord/agent/src/outgoing/udp.rs index 02b54cda327..c6d560fecaa 100644 --- a/mirrord/agent/src/outgoing/udp.rs +++ b/mirrord/agent/src/outgoing/udp.rs @@ -1,7 +1,6 @@ use std::{ collections::HashMap, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, - sync::LazyLock, thread, }; @@ -12,13 +11,10 @@ use futures::{ }; use mirrord_protocol::{ outgoing::{udp::*, *}, - ConnectionId, RemoteError, ResponseError, + ConnectionId, ResponseError, }; -use regex::Regex; use streammap_ext::StreamMap; use tokio::{ - fs::File, - io::AsyncReadExt, net::UdpSocket, select, sync::mpsc::{self, Receiver, Sender}, @@ -35,9 +31,6 @@ use crate::{ type Layer = LayerUdpOutgoing; type Daemon = DaemonUdpOutgoing; -static NAMESERVER: LazyLock = LazyLock::new(|| Regex::new(r"^nameserver.*").unwrap()); -const DNS_PORT: u16 = 53; - /// Handles (briefly) the `UdpOutgoingRequest` and `UdpOutgoingResponse` messages, mostly the /// passing of these messages to the `interceptor_task` thread. pub(crate) struct UdpOutgoingApi { @@ -54,40 +47,16 @@ pub(crate) struct UdpOutgoingApi { daemon_rx: Receiver, } -async fn resolve_dns() -> Result { - trace!("resolve_dns -> "); - - let mut resolv_conf_contents = String::with_capacity(1024); - let mut resolv_conf = File::open("/etc/resolv.conf").await?; - - resolv_conf - .read_to_string(&mut resolv_conf_contents) - .await?; - - let nameserver = resolv_conf_contents - .lines() - .find(|line| NAMESERVER.is_match(line)) - .ok_or(RemoteError::NameserverNotFound)? - .split_whitespace() - .last() - .ok_or(RemoteError::NameserverNotFound)?; - - let dns_address: SocketAddr = format!("{nameserver}:{DNS_PORT}") - .parse() - .map_err(RemoteError::from)?; - - Ok(dns_address) -} - +/// Performs an [`UdpSocket::connect`] that handles 3 situations: +/// +/// 1. Normal `connect` called on an udp socket by the user, through the [`LayerConnect`] message; +/// 2. DNS special-case connection that comes on port `53`, where we have a hack that fakes a +/// connected udp socket. This case in particular requires that the user enable file ops with +/// read access to `/etc/resolv.conf`, otherwise they'll be getting a mismatched connection; +/// 3. User is trying to use `sendto` and `recvfrom`, we use the same hack as in DNS to fake a +/// connection. +#[tracing::instrument(level = "trace", ret)] async fn connect(remote_address: SocketAddr) -> Result { - trace!("connect -> remote_address {:#?}", remote_address); - - let remote_address = if remote_address.port() == DNS_PORT { - resolve_dns().await? - } else { - remote_address - }; - let mirror_address = match remote_address { std::net::SocketAddr::V4(_) => SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 0), std::net::SocketAddr::V6(_) => SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 0), @@ -108,6 +77,7 @@ impl UdpOutgoingApi { let watched_task = WatchedTask::new(Self::TASK_NAME, Self::interceptor_task(layer_rx, daemon_tx)); + let task_status = watched_task.status(); let task = run_thread_in_namespace( watched_task.start(), @@ -124,7 +94,9 @@ impl UdpOutgoingApi { } } - /// Does the actual work for `Request`s and prepares the `Responses: + /// The [`UdpOutgoingApi`] task. + /// + /// Receives [`LayerUdpOutgoing`] messages and replies with [`DaemonUdpOutgoing`]. #[allow(clippy::type_complexity)] async fn interceptor_task( mut layer_rx: Receiver, diff --git a/mirrord/config/src/feature/network.rs b/mirrord/config/src/feature/network.rs index acead9ecc82..b4b424b941b 100644 --- a/mirrord/config/src/feature/network.rs +++ b/mirrord/config/src/feature/network.rs @@ -60,6 +60,12 @@ pub struct NetworkConfig { /// Resolve DNS via the remote pod. /// /// Defaults to `true`. + /// + /// - Caveats: DNS resolving can be done in multiple ways, some frameworks will use + /// `getaddrinfo`, while others will create a connection on port `53` and perform a sort + /// of manual resolution. Just enabling the `dns` feature in mirrord might not be enough. + /// If you see an address resolution error, try enabling the [`fs`](#feature-fs) feature, + /// and setting `read_only: ["/etc/resolv.conf"]`. #[config(env = "MIRRORD_REMOTE_DNS", default = true)] pub dns: bool, } diff --git a/mirrord/layer/tests/outgoing.rs b/mirrord/layer/tests/outgoing.rs index e453f945f4f..1af552dcbd7 100644 --- a/mirrord/layer/tests/outgoing.rs +++ b/mirrord/layer/tests/outgoing.rs @@ -26,10 +26,6 @@ pub use common::*; /// 2. Connects to the remote peer /// 3. Sends some data /// 4. Expects the peer to send the same data back -/// -/// # Ignored -/// This test is ignored due to a bug - `recv_from` call returns an invalid remote peer address. -#[ignore] #[rstest] #[tokio::test] #[timeout(Duration::from_secs(10))]