From 54b1853e4024c175b1986609dbb0781ef829e244 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 10 Jan 2025 11:32:13 +0100 Subject: [PATCH 01/24] Fix ipv6 env var - not only incoming --- mirrord/config/src/feature/network.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mirrord/config/src/feature/network.rs b/mirrord/config/src/feature/network.rs index 1d86071e32e..976adf2b814 100644 --- a/mirrord/config/src/feature/network.rs +++ b/mirrord/config/src/feature/network.rs @@ -10,7 +10,7 @@ use crate::{ util::MirrordToggleableConfig, }; -const IPV6_ENV_VAR: &str = "MIRRORD_INCOMING_ENABLE_IPV6"; +const IPV6_ENV_VAR: &str = "MIRRORD_ENABLE_IPV6"; pub mod dns; pub mod filter; From 061c034ca0ebccb28ef0341401568d552c572ecb Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 10 Jan 2025 11:33:56 +0100 Subject: [PATCH 02/24] outgoing ipv6 E2E test --- tests/src/traffic.rs | 26 ++++++++++++++++++++++++-- tests/src/traffic/steal.rs | 2 +- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/src/traffic.rs b/tests/src/traffic.rs index abdaec6404c..d901be8aca2 100644 --- a/tests/src/traffic.rs +++ b/tests/src/traffic.rs @@ -16,8 +16,8 @@ mod traffic_tests { use tokio::{fs::File, io::AsyncWriteExt}; use crate::utils::{ - config_dir, hostname_service, kube_client, run_exec_with_target, service, - udp_logger_service, KubeService, CONTAINER_NAME, + config_dir, hostname_service, ipv6::ipv6_service, kube_client, run_exec_with_target, + service, udp_logger_service, KubeService, CONTAINER_NAME, }; #[cfg_attr(not(feature = "job"), ignore)] @@ -114,6 +114,28 @@ mod traffic_tests { assert!(res.success()); } + #[rstest] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + #[ignore] + pub async fn outgoing_traffic_single_request_ipv6_enabled(#[future] ipv6_service: KubeService) { + let service = ipv6_service.await; + let node_command = vec![ + "node", + "node-e2e/outgoing/test_outgoing_traffic_single_request_ipv6.mjs", + ]; + let mut process = run_exec_with_target( + node_command, + &service.pod_container_target(), + None, + None, + Some(vec![("MIRRORD_ENABLE_IPV6", "true")]), + ) + .await; + + let res = process.wait().await; + assert!(res.success()); + } + #[cfg_attr(not(feature = "job"), ignore)] #[rstest] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/tests/src/traffic/steal.rs b/tests/src/traffic/steal.rs index bf417813cf0..4b9fcbb2ddd 100644 --- a/tests/src/traffic/steal.rs +++ b/tests/src/traffic/steal.rs @@ -90,7 +90,7 @@ mod steal_tests { &service.pod_container_target(), Some(&service.namespace), Some(flags), - Some(vec![("MIRRORD_INCOMING_ENABLE_IPV6", "true")]), + Some(vec![("MIRRORD_ENABLE_IPV6", "true")]), ) .await; From 308b4e4bb82cb8bfd541cbe730793b86b11034d6 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 10 Jan 2025 11:46:35 +0100 Subject: [PATCH 03/24] getaddrinfo don't mix IPv4 and IPv6 --- mirrord/layer/src/socket/ops.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index efc0095c8a4..a040f47aeb5 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -209,7 +209,8 @@ fn is_ignored_tcp_port(addr: &SocketAddr, config: &IncomingConfig) -> bool { /// If the socket is not found in [`SOCKETS`], bypass. /// Otherwise, if it's not an ignored port, bind (possibly with a fallback to random port) and /// update socket state in [`SOCKETS`]. If it's an ignored port, remove the socket from [`SOCKETS`]. -#[mirrord_layer_macro::instrument(level = Level::TRACE, fields(pid = std::process::id()), ret, skip(raw_address))] +#[mirrord_layer_macro::instrument(level = Level::TRACE, fields(pid = std::process::id()), ret, skip(raw_address) +)] pub(super) fn bind( sockfd: c_int, raw_address: *const sockaddr, @@ -324,9 +325,9 @@ pub(super) fn bind( } }) } - .ok() - .and_then(|(_, address)| address.as_socket()) - .bypass(Bypass::AddressConversion)?; + .ok() + .and_then(|(_, address)| address.as_socket()) + .bypass(Bypass::AddressConversion)?; Arc::get_mut(&mut socket).unwrap().state = SocketState::Bound(Bound { requested_address, @@ -957,14 +958,15 @@ pub(super) fn getaddrinfo( // TODO(alex): Use more fields from `raw_hints` to respect the user's `getaddrinfo` call. let libc::addrinfo { + ai_family, ai_socktype, ai_protocol, .. } = raw_hints; // Some apps (gRPC on Python) use `::` to listen on all interfaces, and usually that just means - // resolve on unspecified. So we just return that in IpV4 because we don't support ipv6. - let resolved_addr = if node == "::" { + // resolve on unspecified. So we just return that in IPv4 if that's the used IP family. + let resolved_addr = if (ai_family == libc::AF_INET) && (node == "::") { // name is "" because that's what happens in real flow. vec![("".to_string(), IpAddr::V4(Ipv4Addr::UNSPECIFIED))] } else { From 3a9ac930365758dc14b2ba66d322136e1a36b350 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 10 Jan 2025 12:02:56 +0100 Subject: [PATCH 04/24] fix unused import warning --- mirrord/layer/src/file/ops.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs index bac20ad7cd9..fcc27507876 100644 --- a/mirrord/layer/src/file/ops.rs +++ b/mirrord/layer/src/file/ops.rs @@ -14,7 +14,9 @@ use mirrord_protocol::{ ResponseError, }; use rand::distributions::{Alphanumeric, DistString}; -use tracing::{error, trace, Level}; +#[cfg(debug_assertions)] +use tracing::Level; +use tracing::{error, trace}; use super::{hooks::FN_OPEN, open_dirs::OPEN_DIRS, *}; #[cfg(target_os = "linux")] From 77fb69d33ad5f9ee4c044c66f7636146fdb4e285 Mon Sep 17 00:00:00 2001 From: t4lz Date: Sat, 11 Jan 2025 09:56:50 +0100 Subject: [PATCH 05/24] request V2 --- mirrord/protocol/src/dns.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 855f52b8af5..6b4ced0f797 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -73,3 +73,15 @@ impl Deref for GetAddrInfoResponse { pub struct GetAddrInfoRequest { pub node: String, } + +/// Newer, advanced version of [`GetAddrInfoRequest`] +#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +pub struct GetAddrInfoRequestV2 { + pub node: String, + pub service: String, + // TODO: should I use c_int? + pub flags: i32, + pub family: i32, + pub socktype: i32, + pub protocol: i32, +} From 29c49e79c99c04343bf7d998c5c903efa0026eb1 Mon Sep 17 00:00:00 2001 From: t4lz Date: Mon, 13 Jan 2025 14:54:11 +0100 Subject: [PATCH 06/24] make layer generate V2 requests --- mirrord/intproxy/protocol/src/lib.rs | 8 ++--- mirrord/intproxy/src/background_tasks.rs | 2 +- mirrord/intproxy/src/lib.rs | 7 +++++ mirrord/intproxy/src/proxies/simple.rs | 37 ++++++++++++++++++++---- mirrord/layer/src/socket.rs | 14 ++++++++- mirrord/layer/src/socket/ops.rs | 36 +++++++++++++++++++---- mirrord/protocol/src/codec.rs | 3 +- mirrord/protocol/src/dns.rs | 16 ++++++++-- 8 files changed, 103 insertions(+), 20 deletions(-) diff --git a/mirrord/intproxy/protocol/src/lib.rs b/mirrord/intproxy/protocol/src/lib.rs index 69a551b50bc..e51fcdf773e 100644 --- a/mirrord/intproxy/protocol/src/lib.rs +++ b/mirrord/intproxy/protocol/src/lib.rs @@ -10,7 +10,7 @@ use std::{ use bincode::{Decode, Encode}; use mirrord_protocol::{ - dns::{GetAddrInfoRequest, GetAddrInfoResponse}, + dns::{GetAddrInfoRequestV2, GetAddrInfoResponse}, file::*, outgoing::SocketAddress, tcp::StealType, @@ -44,7 +44,7 @@ pub enum LayerToProxyMessage { /// A file operation request. File(FileRequest), /// A DNS request. - GetAddrInfo(GetAddrInfoRequest), + GetAddrInfo(GetAddrInfoRequestV2), /// A request to initiate a new outgoing connection. OutgoingConnect(OutgoingConnectRequest), /// Requests related to incoming connections. @@ -210,7 +210,7 @@ pub enum ProxyToLayerMessage { NewSession(LayerId), /// A response to layer's [`FileRequest`]. File(FileResponse), - /// A response to layer's [`GetAddrInfoRequest`]. + /// A response to layer's [`GetAddrInfoRequestV2`]. GetAddrInfo(GetAddrInfoResponse), /// A response to layer's [`OutgoingConnectRequest`]. OutgoingConnect(RemoteResult), @@ -428,7 +428,7 @@ impl_request!( ); impl_request!( - req = GetAddrInfoRequest, + req = GetAddrInfoRequestV2, res = GetAddrInfoResponse, req_path = LayerToProxyMessage::GetAddrInfo, res_path = ProxyToLayerMessage::GetAddrInfo, diff --git a/mirrord/intproxy/src/background_tasks.rs b/mirrord/intproxy/src/background_tasks.rs index 82e6865c67e..2c73f80bece 100644 --- a/mirrord/intproxy/src/background_tasks.rs +++ b/mirrord/intproxy/src/background_tasks.rs @@ -3,7 +3,7 @@ //! The proxy utilizes multiple background tasks to split the code into more self-contained parts. //! Structs in this module aim to ease managing their state. //! -//! Each background task implement the [`BackgroundTask`] trait, which specifies its properties and +//! Each background task implements the [`BackgroundTask`] trait, which specifies its properties and //! allows for managing groups of related tasks with one [`BackgroundTasks`] instance. use std::{collections::HashMap, fmt, future::Future, hash::Hash}; diff --git a/mirrord/intproxy/src/lib.rs b/mirrord/intproxy/src/lib.rs index 7dee93344bf..7b5944bc307 100644 --- a/mirrord/intproxy/src/lib.rs +++ b/mirrord/intproxy/src/lib.rs @@ -321,6 +321,13 @@ impl IntProxy { .send(FilesProxyMessage::ProtocolVersion(protocol_version.clone())) .await; + self.task_txs + .simple + .send(SimpleProxyMessage::ProtocolVersion( + protocol_version.clone(), + )) + .await; + self.task_txs .incoming .send(IncomingProxyMessage::AgentProtocolVersion(protocol_version)) diff --git a/mirrord/intproxy/src/proxies/simple.rs b/mirrord/intproxy/src/proxies/simple.rs index dae7881247e..760406d7c34 100644 --- a/mirrord/intproxy/src/proxies/simple.rs +++ b/mirrord/intproxy/src/proxies/simple.rs @@ -5,9 +5,10 @@ use std::collections::HashMap; use mirrord_intproxy_protocol::{LayerId, MessageId, ProxyToLayerMessage}; use mirrord_protocol::{ - dns::{GetAddrInfoRequest, GetAddrInfoResponse}, + dns::{GetAddrInfoRequestV2, GetAddrInfoResponse, ADDRINFO_V2_VERSION}, ClientMessage, DaemonMessage, GetEnvVarsRequest, RemoteResult, }; +use semver::Version; use thiserror::Error; use crate::{ @@ -20,10 +21,12 @@ use crate::{ #[derive(Debug)] pub enum SimpleProxyMessage { - AddrInfoReq(MessageId, LayerId, GetAddrInfoRequest), + AddrInfoReq(MessageId, LayerId, GetAddrInfoRequestV2), AddrInfoRes(GetAddrInfoResponse), GetEnvReq(MessageId, LayerId, GetEnvVarsRequest), GetEnvRes(RemoteResult>), + /// Protocol version was negotiated with the agent. + ProtocolVersion(Version), } #[derive(Error, Debug)] @@ -38,6 +41,23 @@ pub struct SimpleProxy { addr_info_reqs: RequestQueue, /// For [`GetEnvVarsRequest`]s. get_env_reqs: RequestQueue, + /// [`mirrord_protocol`] version negotiated with the agent. + /// Determines whether we can use `GetAddrInfoRequestV2`. + protocol_version: Option, +} + +impl SimpleProxy { + #[tracing::instrument(skip(self), level = tracing::Level::TRACE)] + fn protocol_version(&mut self, version: Version) { + self.protocol_version.replace(version); + } + + /// Returns whether [`mirrord_protocol`] version allows for a V2 addrinfo request. + fn addr_info_v2(&self) -> bool { + self.protocol_version + .as_ref() + .is_some_and(|version| ADDRINFO_V2_VERSION.matches(version)) + } } impl BackgroundTask for SimpleProxy { @@ -52,9 +72,15 @@ impl BackgroundTask for SimpleProxy { match msg { SimpleProxyMessage::AddrInfoReq(message_id, session_id, req) => { self.addr_info_reqs.push_back(message_id, session_id); - message_bus - .send(ClientMessage::GetAddrInfoRequest(req)) - .await; + if self.addr_info_v2() { + message_bus + .send(ClientMessage::GetAddrInfoRequestV2(req)) + .await; + } else { + message_bus + .send(ClientMessage::GetAddrInfoRequest(req.into())) + .await; + } } SimpleProxyMessage::AddrInfoRes(res) => { let (message_id, layer_id) = @@ -88,6 +114,7 @@ impl BackgroundTask for SimpleProxy { }) .await } + SimpleProxyMessage::ProtocolVersion(version) => self.protocol_version(version), } } diff --git a/mirrord/layer/src/socket.rs b/mirrord/layer/src/socket.rs index 79c197d408d..5c2d03f8ace 100644 --- a/mirrord/layer/src/socket.rs +++ b/mirrord/layer/src/socket.rs @@ -439,10 +439,22 @@ impl ProtocolAndAddressFilterExt for ProtocolAndAddressFilter { return Ok(false); } + let family = if address.is_ipv4() { + libc::AF_INET + } else { + libc::AF_INET6 + }; + + let addr_protocol = if matches!(protocol, NetProtocol::Stream) { + libc::SOCK_STREAM + } else { + libc::SOCK_DGRAM + }; + match &self.address { AddressFilter::Name(name, port) => { let resolved_ips = if crate::setup().remote_dns_enabled() && !force_local_dns { - match remote_getaddrinfo(name.to_string()) { + match remote_getaddrinfo(name.to_string(), *port, 0, family, 0, addr_protocol) { Ok(res) => res.into_iter().map(|(_, ip)| ip).collect(), Err(HookError::ResponseError(ResponseError::DnsLookup( DnsLookupError { diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index a040f47aeb5..fe505c4a6cf 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -22,7 +22,7 @@ use mirrord_intproxy_protocol::{ OutgoingConnectResponse, PortSubscribe, }; use mirrord_protocol::{ - dns::{GetAddrInfoRequest, LookupRecord}, + dns::{GetAddrInfoRequestV2, LookupRecord}, file::{OpenFileResponse, OpenOptionsInternal, ReadFileResponse}, }; use nix::sys::socket::{sockopt, SockaddrIn, SockaddrIn6, SockaddrLike, SockaddrStorage}; @@ -891,8 +891,23 @@ pub(super) fn dup(fd: c_int, dup_fd: i32) -> Result<(), /// /// This function updates the mapping in [`REMOTE_DNS_REVERSE_MAPPING`]. #[mirrord_layer_macro::instrument(level = Level::TRACE, ret, err)] -pub(super) fn remote_getaddrinfo(node: String) -> HookResult> { - let addr_info_list = common::make_proxy_request_with_response(GetAddrInfoRequest { node })?.0?; +pub(super) fn remote_getaddrinfo( + node: String, + service_port: u16, + flags: c_int, + family: c_int, + socktype: c_int, + protocol: c_int, +) -> HookResult> { + let addr_info_list = common::make_proxy_request_with_response(GetAddrInfoRequestV2 { + node, + service_port, + flags, + family, + socktype, + protocol, + })? + .0?; let mut remote_dns_reverse_mapping = REMOTE_DNS_REVERSE_MAPPING.lock()?; addr_info_list.iter().for_each(|lookup| { @@ -947,6 +962,8 @@ pub(super) fn getaddrinfo( Bypass::CStrConversion })? + // TODO: according to the man page, service could also be a service name, it doesn't have to + // be a port number. .and_then(|service| service.parse::().ok()) .unwrap_or(0); @@ -956,11 +973,11 @@ pub(super) fn getaddrinfo( .cloned() .unwrap_or_else(|| unsafe { mem::zeroed() }); - // TODO(alex): Use more fields from `raw_hints` to respect the user's `getaddrinfo` call. let libc::addrinfo { ai_family, ai_socktype, ai_protocol, + ai_flags, .. } = raw_hints; @@ -970,7 +987,14 @@ pub(super) fn getaddrinfo( // name is "" because that's what happens in real flow. vec![("".to_string(), IpAddr::V4(Ipv4Addr::UNSPECIFIED))] } else { - remote_getaddrinfo(node.clone())? + remote_getaddrinfo( + node.clone(), + service, + ai_flags, + ai_family, + ai_socktype, + ai_protocol, + )? }; let mut managed_addr_info = MANAGED_ADDRINFO.lock()?; @@ -1069,7 +1093,7 @@ pub(super) fn gethostbyname(raw_name: Option<&CStr>) -> Detour<*mut hostent> { crate::setup().dns_selector().check_query(&name, 0)?; - let hosts_and_ips = remote_getaddrinfo(name.clone())?; + let hosts_and_ips = remote_getaddrinfo(name.clone(), 0, 0, 0, 0, 0)?; // We could `unwrap` here, as this would have failed on the previous conversion. let host_name = CString::new(name)?; diff --git a/mirrord/protocol/src/codec.rs b/mirrord/protocol/src/codec.rs index 1b0975d350d..39961b3bce2 100644 --- a/mirrord/protocol/src/codec.rs +++ b/mirrord/protocol/src/codec.rs @@ -12,7 +12,7 @@ use mirrord_macros::protocol_break; use semver::VersionReq; use crate::{ - dns::{GetAddrInfoRequest, GetAddrInfoResponse}, + dns::{GetAddrInfoRequest, GetAddrInfoRequestV2, GetAddrInfoResponse}, file::*, outgoing::{ tcp::{DaemonTcpOutgoing, LayerTcpOutgoing}, @@ -117,6 +117,7 @@ pub enum ClientMessage { SwitchProtocolVersion(#[bincode(with_serde)] semver::Version), ReadyForLogs, Vpn(ClientVpn), + GetAddrInfoRequestV2(GetAddrInfoRequestV2), } /// Type alias for `Result`s that should be returned from mirrord-agent to mirrord-layer. diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 6b4ced0f797..87d4c10aaab 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -1,12 +1,17 @@ extern crate alloc; use core::ops::Deref; -use std::net::IpAddr; +use std::{net::IpAddr, sync::LazyLock}; use bincode::{Decode, Encode}; use hickory_resolver::{lookup_ip::LookupIp, proto::rr::resource::RecordParts}; +use semver::VersionReq; use crate::RemoteResult; +/// Minimal mirrord-protocol version that allows [`GetAddrInfoRequestV2`]. +pub static ADDRINFO_V2_VERSION: LazyLock = + LazyLock::new(|| ">=1.14.0".parse().expect("Bad Identifier")); + #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct LookupRecord { pub name: String, @@ -74,11 +79,18 @@ pub struct GetAddrInfoRequest { pub node: String, } +/// For when the new request is not supported, and we have to fall back to the old version. +impl From for GetAddrInfoRequest { + fn from(value: GetAddrInfoRequestV2) -> Self { + Self { node: value.node } + } +} + /// Newer, advanced version of [`GetAddrInfoRequest`] #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct GetAddrInfoRequestV2 { pub node: String, - pub service: String, + pub service_port: u16, // TODO: should I use c_int? pub flags: i32, pub family: i32, From 9dd606fb8e0738b37fee7b95bc114cf9c5e89a7c Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 15 Jan 2025 11:55:44 +0100 Subject: [PATCH 07/24] change agent to respect new dns request - not working? --- CONTRIBUTING.md | 6 ++++ mirrord/agent/src/dns.rs | 54 ++++++++++++++++++++++++++++----- mirrord/agent/src/entrypoint.rs | 14 +++++++-- mirrord/protocol/src/dns.rs | 13 ++++++++ 4 files changed, 76 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b99ddc4c65..497cb12e833 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -131,6 +131,12 @@ In order to test IPv6 on a local cluster on macOS, you can use Kind: 3. `kind create cluster --config kind-config.yaml` 4. When you run `kubectl get svc -o wide --all-namespaces` you should see IPv6 addresses. +In order to use an agent image from a local registry, you can load the image to kind's registry with: + +``` +kind load docker-image test:latest +``` + ### Cleanup diff --git a/mirrord/agent/src/dns.rs b/mirrord/agent/src/dns.rs index 0ad44c76934..a04c78b4380 100644 --- a/mirrord/agent/src/dns.rs +++ b/mirrord/agent/src/dns.rs @@ -3,7 +3,7 @@ use std::{future, path::PathBuf, time::Duration}; use futures::{stream::FuturesOrdered, StreamExt}; use hickory_resolver::{system_conf::parse_resolv_conf, Hosts, Resolver}; use mirrord_protocol::{ - dns::{DnsLookup, GetAddrInfoRequest, GetAddrInfoResponse}, + dns::{DnsLookup, GetAddrInfoRequest, GetAddrInfoRequestV2, GetAddrInfoResponse}, DnsLookupError, RemoteResult, ResolveErrorKindInternal, ResponseError, }; use tokio::{ @@ -21,9 +21,24 @@ use crate::{ watched_task::TaskStatus, }; +#[derive(Debug)] +pub(crate) enum ClientGetAddrInfoRequest { + Old(GetAddrInfoRequest), + V2(GetAddrInfoRequestV2), +} + +impl ClientGetAddrInfoRequest { + pub(crate) fn into_v2(self) -> GetAddrInfoRequestV2 { + match self { + ClientGetAddrInfoRequest::Old(old_req) => old_req.into(), + ClientGetAddrInfoRequest::V2(v2_req) => v2_req, + } + } +} + #[derive(Debug)] pub(crate) struct DnsCommand { - request: GetAddrInfoRequest, + request: ClientGetAddrInfoRequest, response_tx: oneshot::Sender>, } @@ -34,6 +49,7 @@ pub(crate) struct DnsWorker { request_rx: Receiver, attempts: usize, timeout: Duration, + support_ipv6: bool, } impl DnsWorker { @@ -45,7 +61,11 @@ impl DnsWorker { /// # Note /// /// `pid` is used to find the correct path of `etc` directory. - pub(crate) fn new(pid: Option, request_rx: Receiver) -> Self { + pub(crate) fn new( + pid: Option, + request_rx: Receiver, + support_ipv6: bool, + ) -> Self { let etc_path = pid .map(|pid| { PathBuf::from("/proc") @@ -66,6 +86,7 @@ impl DnsWorker { .ok() .and_then(|attempts| attempts.parse().ok()) .unwrap_or(1), + support_ipv6, } } @@ -79,9 +100,10 @@ impl DnsWorker { #[tracing::instrument(level = Level::TRACE, ret, err(level = Level::TRACE))] async fn do_lookup( etc_path: PathBuf, - host: String, + request: GetAddrInfoRequestV2, attempts: usize, timeout: Duration, + support_ipv6: bool, ) -> RemoteResult { // Prepares the `Resolver` after reading some `/etc` DNS files. // @@ -98,7 +120,15 @@ impl DnsWorker { hickory_resolver::config::ServerOrderingStrategy::UserProvidedOrder; options.timeout = timeout; options.attempts = attempts; - options.ip_strategy = hickory_resolver::config::LookupIpStrategy::Ipv4Only; + options.ip_strategy = if support_ipv6 { + match request.family { + libc::AF_INET => hickory_resolver::config::LookupIpStrategy::Ipv4Only, + libc::AF_INET6 => hickory_resolver::config::LookupIpStrategy::Ipv6Only, + _ => hickory_resolver::config::LookupIpStrategy::Ipv4AndIpv6, + } + } else { + hickory_resolver::config::LookupIpStrategy::Ipv4Only + }; let mut resolver = Resolver::tokio(config, options); @@ -111,7 +141,7 @@ impl DnsWorker { let lookup = resolver .inspect_err(|fail| tracing::error!(?fail, "Failed to build DNS resolver"))? - .lookup_ip(host) + .lookup_ip(request.node) .await .inspect(|lookup| tracing::trace!(?lookup, "Lookup finished"))? .into(); @@ -125,8 +155,16 @@ impl DnsWorker { let etc_path = self.etc_path.clone(); let timeout = self.timeout; let attempts = self.attempts; + let support_ipv6 = self.support_ipv6; let lookup_future = async move { - let result = Self::do_lookup(etc_path, message.request.node, attempts, timeout).await; + let result = Self::do_lookup( + etc_path, + message.request.into_v2(), + attempts, + timeout, + support_ipv6, + ) + .await; if let Err(result) = message.response_tx.send(result) { tracing::error!(?result, "Failed to send query response"); @@ -174,7 +212,7 @@ impl DnsApi { /// Results of scheduled requests are available via [`Self::recv`] (order is preserved). pub(crate) async fn make_request( &mut self, - request: GetAddrInfoRequest, + request: ClientGetAddrInfoRequest, ) -> Result<(), AgentError> { let (response_tx, response_rx) = oneshot::channel(); diff --git a/mirrord/agent/src/entrypoint.rs b/mirrord/agent/src/entrypoint.rs index fff9ee7192f..9e943114503 100644 --- a/mirrord/agent/src/entrypoint.rs +++ b/mirrord/agent/src/entrypoint.rs @@ -10,7 +10,7 @@ use std::{ }; use client_connection::AgentTlsConnector; -use dns::{DnsCommand, DnsWorker}; +use dns::{ClientGetAddrInfoRequest, DnsCommand, DnsWorker}; use futures::TryFutureExt; use mirrord_protocol::{ClientMessage, DaemonMessage, GetEnvVarsRequest, LogMessage}; use sniffer::tcp_capture::RawSocketTcpCapture; @@ -433,7 +433,14 @@ impl ClientConnectionHandler { .await? } ClientMessage::GetAddrInfoRequest(request) => { - self.dns_api.make_request(request).await?; + self.dns_api + .make_request(ClientGetAddrInfoRequest::Old(request)) + .await?; + } + ClientMessage::GetAddrInfoRequestV2(request) => { + self.dns_api + .make_request(ClientGetAddrInfoRequest::V2(request)) + .await?; } ClientMessage::Ping => self.respond(DaemonMessage::Pong).await?, ClientMessage::Tcp(message) => { @@ -613,7 +620,8 @@ async fn start_agent(args: Args) -> Result<()> { let cancellation_token = cancellation_token.clone(); let watched_task = WatchedTask::new( DnsWorker::TASK_NAME, - DnsWorker::new(state.container_pid(), dns_command_rx).run(cancellation_token), + DnsWorker::new(state.container_pid(), dns_command_rx, args.ipv6) + .run(cancellation_token), ); let status = watched_task.status(); let task = run_thread_in_namespace( diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 87d4c10aaab..969fa46020f 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -97,3 +97,16 @@ pub struct GetAddrInfoRequestV2 { pub socktype: i32, pub protocol: i32, } + +impl From for GetAddrInfoRequestV2 { + fn from(value: GetAddrInfoRequest) -> Self { + Self { + node: value.node, + service_port: 0, + flags: 0, + family: 0, + socktype: 0, + protocol: 0, + } + } +} From f1c37aab715141a83de6922b0c5226cd5b4134fc Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 15 Jan 2025 12:39:46 +0100 Subject: [PATCH 08/24] logs --- mirrord/agent/src/dns.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mirrord/agent/src/dns.rs b/mirrord/agent/src/dns.rs index a04c78b4380..cc021ec76d8 100644 --- a/mirrord/agent/src/dns.rs +++ b/mirrord/agent/src/dns.rs @@ -121,12 +121,14 @@ impl DnsWorker { options.timeout = timeout; options.attempts = attempts; options.ip_strategy = if support_ipv6 { + tracing::debug!("IPv6 support enabled. Respecting client IP family."); match request.family { libc::AF_INET => hickory_resolver::config::LookupIpStrategy::Ipv4Only, libc::AF_INET6 => hickory_resolver::config::LookupIpStrategy::Ipv6Only, _ => hickory_resolver::config::LookupIpStrategy::Ipv4AndIpv6, } } else { + tracing::debug!("IPv6 support disabled. Resolving IPv4 only."); hickory_resolver::config::LookupIpStrategy::Ipv4Only }; From c979a48eb9b8817ebfdf50b48fdf244d086af453 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 20:16:09 +0100 Subject: [PATCH 09/24] libc consts are different on mac and linux --- CONTRIBUTING.md | 5 +++++ mirrord/agent/src/dns.rs | 12 +++++------ mirrord/layer/src/socket/ops.rs | 12 ++++++++++- mirrord/protocol/src/dns.rs | 38 ++++++++++++++++++++++++++++----- tests/src/utils.rs | 8 ++++++- 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 497cb12e833..2971c74244d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -137,6 +137,11 @@ In order to use an agent image from a local registry, you can load the image to kind load docker-image test:latest ``` +In order to test on EKS, I used this blueprint: https://github.com/aws-ia/terraform-aws-eks-blueprints/tree/main/patterns/ipv6-eks-cluster + +After creating the cluster, I had to give myself permissions to the K8s objects, I did that via the AWS console (in the browser). +Feel free to add instructions on how to make that "manual" step unnecessary. + ### Cleanup diff --git a/mirrord/agent/src/dns.rs b/mirrord/agent/src/dns.rs index cc021ec76d8..45a8d2a59b0 100644 --- a/mirrord/agent/src/dns.rs +++ b/mirrord/agent/src/dns.rs @@ -116,23 +116,22 @@ impl DnsWorker { let hosts_conf = fs::read(hosts_path).await?; let (config, mut options) = parse_resolv_conf(resolv_conf)?; + tracing::debug!(?config, ?options, "parsed config options"); options.server_ordering_strategy = hickory_resolver::config::ServerOrderingStrategy::UserProvidedOrder; options.timeout = timeout; options.attempts = attempts; options.ip_strategy = if support_ipv6 { tracing::debug!("IPv6 support enabled. Respecting client IP family."); - match request.family { - libc::AF_INET => hickory_resolver::config::LookupIpStrategy::Ipv4Only, - libc::AF_INET6 => hickory_resolver::config::LookupIpStrategy::Ipv6Only, - _ => hickory_resolver::config::LookupIpStrategy::Ipv4AndIpv6, - } + request.family.into() } else { tracing::debug!("IPv6 support disabled. Resolving IPv4 only."); hickory_resolver::config::LookupIpStrategy::Ipv4Only }; + tracing::debug!(?config, ?options, "updated config options"); let mut resolver = Resolver::tokio(config, options); + tracing::debug!(?resolver, "tokio resolver"); let mut hosts = Hosts::default(); hosts.read_hosts_conf(hosts_conf.as_slice())?; @@ -145,7 +144,8 @@ impl DnsWorker { .inspect_err(|fail| tracing::error!(?fail, "Failed to build DNS resolver"))? .lookup_ip(request.node) .await - .inspect(|lookup| tracing::trace!(?lookup, "Lookup finished"))? + .inspect(|lookup| tracing::trace!(?lookup, "Lookup finished")) + .inspect_err(|e| tracing::trace!(%e, "lookup failed"))? .into(); Ok(lookup) diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index fe505c4a6cf..b93bd631401 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -22,7 +22,7 @@ use mirrord_intproxy_protocol::{ OutgoingConnectResponse, PortSubscribe, }; use mirrord_protocol::{ - dns::{GetAddrInfoRequestV2, LookupRecord}, + dns::{AddressFamily, GetAddrInfoRequestV2, LookupRecord, SockType}, file::{OpenFileResponse, OpenOptionsInternal, ReadFileResponse}, }; use nix::sys::socket::{sockopt, SockaddrIn, SockaddrIn6, SockaddrLike, SockaddrStorage}; @@ -899,6 +899,16 @@ pub(super) fn remote_getaddrinfo( socktype: c_int, protocol: c_int, ) -> HookResult> { + let family = match family { + libc::AF_INET => AddressFamily::Ipv4Only, + libc::AF_INET6 => AddressFamily::Ipv6Only, + _ => AddressFamily::Both, + }; + let socktype = match socktype { + libc::SOCK_STREAM => SockType::Stream, + libc::SOCK_DGRAM => SockType::Dgram, + _ => SockType::Any, + }; let addr_info_list = common::make_proxy_request_with_response(GetAddrInfoRequestV2 { node, service_port, diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 969fa46020f..789a6d61953 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -86,15 +86,43 @@ impl From for GetAddrInfoRequest { } } +#[derive(Encode, Decode, Debug, PartialEq, Eq, Copy, Clone)] +pub enum AddressFamily { + Ipv4Only, + Ipv6Only, + Both, +} + +impl From for hickory_resolver::config::LookupIpStrategy { + fn from(value: AddressFamily) -> Self { + match value { + AddressFamily::Ipv4Only => Self::Ipv4Only, + AddressFamily::Ipv6Only => Self::Ipv6Only, + AddressFamily::Both => Self::Ipv4AndIpv6, + } + } +} + +#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +pub enum SockType { + Stream, + Dgram, + Any, +} + /// Newer, advanced version of [`GetAddrInfoRequest`] #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct GetAddrInfoRequestV2 { pub node: String, + /// Currently not respected by the agent, there for future use. pub service_port: u16, - // TODO: should I use c_int? + pub family: AddressFamily, + pub socktype: SockType, + /// Including these fields so we can use them in the future without introducing a new request + /// type. But note that the constants are different on macOS and Linux so they should be + /// converted to the linux values first (on the client, because the agent does not know the + /// client is macOS). pub flags: i32, - pub family: i32, - pub socktype: i32, pub protocol: i32, } @@ -104,8 +132,8 @@ impl From for GetAddrInfoRequestV2 { node: value.node, service_port: 0, flags: 0, - family: 0, - socktype: 0, + family: AddressFamily::Ipv4Only, + socktype: SockType::Any, protocol: 0, } } diff --git a/tests/src/utils.rs b/tests/src/utils.rs index bf8e0d1a79b..744ff9c006a 100644 --- a/tests/src/utils.rs +++ b/tests/src/utils.rs @@ -633,11 +633,17 @@ pub async fn run_exec( .into_iter() .chain(process_cmd.into_iter()) .collect(); + let agent_image_env = "MIRRORD_AGENT_IMAGE"; + let agent_image_from_devs_env = std::env::var(agent_image_env); // used by the CI, to load the image locally: // docker build -t test . -f mirrord/agent/Dockerfile // minikube load image test:latest let mut base_env = HashMap::new(); - base_env.insert("MIRRORD_AGENT_IMAGE", "test"); + base_env.insert( + agent_image_env, + // Let devs running the test specify an agent image per env var. + agent_image_from_devs_env.as_deref().unwrap_or("test"), + ); base_env.insert("MIRRORD_CHECK_VERSION", "false"); base_env.insert("MIRRORD_AGENT_RUST_LOG", "warn,mirrord=debug"); base_env.insert("MIRRORD_AGENT_COMMUNICATION_TIMEOUT", "180"); From 3d74ede750e7c928e52a28ccfdfa00e6307f5519 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 21:36:35 +0100 Subject: [PATCH 10/24] test with kuberentes api --- tests/src/traffic.rs | 17 ++++++++++++++++- tests/src/utils.rs | 4 ++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/src/traffic.rs b/tests/src/traffic.rs index d901be8aca2..c511eb12e10 100644 --- a/tests/src/traffic.rs +++ b/tests/src/traffic.rs @@ -17,7 +17,7 @@ mod traffic_tests { use crate::utils::{ config_dir, hostname_service, ipv6::ipv6_service, kube_client, run_exec_with_target, - service, udp_logger_service, KubeService, CONTAINER_NAME, + service, udp_logger_service, Application, KubeService, CONTAINER_NAME, }; #[cfg_attr(not(feature = "job"), ignore)] @@ -136,6 +136,21 @@ mod traffic_tests { assert!(res.success()); } + #[rstest] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + #[timeout(Duration::from_secs(30))] + #[ignore] + pub async fn connect_to_kubernetes_api_service_over_ipv6() { + let app = Application::CurlToKubeApi; + let mut process = app + .run_targetless(None, None, Some(vec![("MIRRORD_ENABLE_IPV6", "true")])) + .await; + let res = process.wait().await; + assert!(res.success()); + let stdout = process.get_stdout().await; + assert!(stdout.contains(r#""apiVersion": "v1""#)) + } + #[cfg_attr(not(feature = "job"), ignore)] #[rstest] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/tests/src/utils.rs b/tests/src/utils.rs index 744ff9c006a..7fa4b7c68e7 100644 --- a/tests/src/utils.rs +++ b/tests/src/utils.rs @@ -99,6 +99,7 @@ pub enum Application { Go22HTTP, Go23HTTP, CurlToKubeApi, + CurlToKubeApiOverIpv6, PythonCloseSocket, PythonCloseSocketKeepConnection, RustWebsockets, @@ -477,6 +478,9 @@ impl Application { Application::CurlToKubeApi => { vec!["curl", "https://kubernetes/api", "--insecure"] } + Application::CurlToKubeApiOverIpv6 => { + vec!["curl", "-6", "https://kubernetes/api", "--insecure"] + } Application::RustWebsockets => vec!["../target/debug/rust-websockets"], Application::RustSqs => vec!["../target/debug/rust-sqs-printer"], } From 197f951546b1f55646926a3b5f56c75470741192 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 21:37:06 +0100 Subject: [PATCH 11/24] changelog --- changelog.d/2958.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/2958.added.md diff --git a/changelog.d/2958.added.md b/changelog.d/2958.added.md new file mode 100644 index 00000000000..af12472d466 --- /dev/null +++ b/changelog.d/2958.added.md @@ -0,0 +1 @@ +Support for in-cluster DNS resolution of IPv6 addresses. From 67932854c24e4c2f63a078dd20e018506a4d8370 Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 21:39:23 +0100 Subject: [PATCH 12/24] document tests in contributing guide --- CONTRIBUTING.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2971c74244d..0583782b4b1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -142,6 +142,10 @@ In order to test on EKS, I used this blueprint: https://github.com/aws-ia/terraf After creating the cluster, I had to give myself permissions to the K8s objects, I did that via the AWS console (in the browser). Feel free to add instructions on how to make that "manual" step unnecessary. +IPv6 tests (they currently don't run in the CI): +- steal_http_ipv6_traffic +- connect_to_kubernetes_api_service_over_ipv6 + ### Cleanup From f58f987c7ed48593ecc0164ac81f63eb5b786bee Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 21:40:32 +0100 Subject: [PATCH 13/24] fix docs --- mirrord/intproxy/src/proxies/simple.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mirrord/intproxy/src/proxies/simple.rs b/mirrord/intproxy/src/proxies/simple.rs index 760406d7c34..6a970e2146c 100644 --- a/mirrord/intproxy/src/proxies/simple.rs +++ b/mirrord/intproxy/src/proxies/simple.rs @@ -37,7 +37,7 @@ pub struct SimpleProxyError(#[from] UnexpectedAgentMessage); /// Run as a [`BackgroundTask`]. #[derive(Default)] pub struct SimpleProxy { - /// For [`GetAddrInfoRequest`]s. + /// For [`GetAddrInfoRequestV2`]s. addr_info_reqs: RequestQueue, /// For [`GetEnvVarsRequest`]s. get_env_reqs: RequestQueue, From 7025c1ed1587de96c0f240fb40faa4c6e455a44f Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 21:43:17 +0100 Subject: [PATCH 14/24] fix integration test --- mirrord/layer/tests/dns_resolve.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mirrord/layer/tests/dns_resolve.rs b/mirrord/layer/tests/dns_resolve.rs index 508f546ec03..9085b708d0a 100644 --- a/mirrord/layer/tests/dns_resolve.rs +++ b/mirrord/layer/tests/dns_resolve.rs @@ -8,7 +8,7 @@ use rstest::rstest; mod common; pub use common::*; use mirrord_protocol::{ - dns::{DnsLookup, GetAddrInfoRequest, GetAddrInfoResponse, LookupRecord}, + dns::{DnsLookup, GetAddrInfoRequestV2, GetAddrInfoResponse, LookupRecord}, ClientMessage, DaemonMessage, DnsLookupError, ResolveErrorKindInternal::NoRecordsFound, }; @@ -25,7 +25,7 @@ async fn test_dns_resolve( .await; let msg = intproxy.recv().await; - let ClientMessage::GetAddrInfoRequest(GetAddrInfoRequest { node }) = msg else { + let ClientMessage::GetAddrInfoRequestV2(GetAddrInfoRequestV2 { node, .. }) = msg else { panic!("Invalid message received from layer: {msg:?}"); }; @@ -39,7 +39,7 @@ async fn test_dns_resolve( .await; let msg = intproxy.recv().await; - let ClientMessage::GetAddrInfoRequest(GetAddrInfoRequest { node: _ }) = msg else { + let ClientMessage::GetAddrInfoRequestV2(GetAddrInfoRequestV2 { .. }) = msg else { panic!("Invalid message received from layer: {msg:?}"); }; From 7fd43115f08b8e5db917d16b26680680a8d8902e Mon Sep 17 00:00:00 2001 From: t4lz Date: Fri, 17 Jan 2025 22:18:03 +0100 Subject: [PATCH 15/24] fix another integration test --- mirrord/layer/tests/issue2055.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mirrord/layer/tests/issue2055.rs b/mirrord/layer/tests/issue2055.rs index c34d5e13f25..d24b71fadce 100644 --- a/mirrord/layer/tests/issue2055.rs +++ b/mirrord/layer/tests/issue2055.rs @@ -2,7 +2,7 @@ use std::{net::IpAddr, path::Path, time::Duration}; use mirrord_protocol::{ - dns::{DnsLookup, GetAddrInfoRequest, GetAddrInfoResponse, LookupRecord}, + dns::{DnsLookup, GetAddrInfoRequestV2, GetAddrInfoResponse, LookupRecord}, ClientMessage, DaemonMessage, DnsLookupError, ResolveErrorKindInternal::NoRecordsFound, ResponseError, @@ -23,10 +23,10 @@ async fn issue_2055(dylib_path: &Path) { .start_process_with_layer(dylib_path, vec![("MIRRORD_REMOTE_DNS", "true")], None) .await; - println!("Application started, waiting for `GetAddrInfoRequest`."); + println!("Application started, waiting for `GetAddrInfoRequestV2`."); let msg = intproxy.recv().await; - let ClientMessage::GetAddrInfoRequest(GetAddrInfoRequest { node }) = msg else { + let ClientMessage::GetAddrInfoRequestV2(GetAddrInfoRequestV2 { node, .. }) = msg else { panic!("Invalid message received from layer: {msg:?}"); }; @@ -40,7 +40,7 @@ async fn issue_2055(dylib_path: &Path) { .await; let msg = intproxy.recv().await; - let ClientMessage::GetAddrInfoRequest(GetAddrInfoRequest { node: _ }) = msg else { + let ClientMessage::GetAddrInfoRequestV2(GetAddrInfoRequestV2 { .. }) = msg else { panic!("Invalid message received from layer: {msg:?}"); }; From 81b05e653bcb1305b27fa0eaf218d1a27a89b772 Mon Sep 17 00:00:00 2001 From: t4lz Date: Mon, 20 Jan 2025 11:07:31 +0100 Subject: [PATCH 16/24] fix another integration test --- mirrord/layer/tests/issue2283.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mirrord/layer/tests/issue2283.rs b/mirrord/layer/tests/issue2283.rs index f3d7a2a9b2b..6987bbdffbf 100644 --- a/mirrord/layer/tests/issue2283.rs +++ b/mirrord/layer/tests/issue2283.rs @@ -3,7 +3,7 @@ use std::{assert_matches::assert_matches, net::SocketAddr, path::Path, time::Duration}; use mirrord_protocol::{ - dns::{DnsLookup, GetAddrInfoRequest, GetAddrInfoResponse, LookupRecord}, + dns::{DnsLookup, GetAddrInfoRequestV2, GetAddrInfoResponse, LookupRecord}, outgoing::{ tcp::{DaemonTcpOutgoing, LayerTcpOutgoing}, DaemonConnect, DaemonRead, LayerConnect, SocketAddress, @@ -48,7 +48,7 @@ async fn test_issue2283( } let message = intproxy.recv().await; - assert_matches!(message, ClientMessage::GetAddrInfoRequest(GetAddrInfoRequest { node }) if node == "test-server"); + assert_matches!(message, ClientMessage::GetAddrInfoRequestV2(GetAddrInfoRequestV2 { node, .. }) if node == "test-server"); let address = "1.2.3.4:80".parse::().unwrap(); From afa304a4b38ecaf63916e860b4534c7977334bc7 Mon Sep 17 00:00:00 2001 From: t4lz Date: Mon, 20 Jan 2025 11:24:09 +0100 Subject: [PATCH 17/24] change e2e test logs to debug for CI From 8381359d3c5fd8618fa2ae5bfbf5218134e9014e Mon Sep 17 00:00:00 2001 From: t4lz Date: Mon, 20 Jan 2025 11:48:09 +0100 Subject: [PATCH 18/24] fix? --- mirrord/layer/src/socket/ops.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index b93bd631401..a8a6e49e79a 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -977,7 +977,9 @@ pub(super) fn getaddrinfo( .and_then(|service| service.parse::().ok()) .unwrap_or(0); - crate::setup().dns_selector().check_query(&node, service)?; + let setup = crate::setup(); + setup.dns_selector().check_query(&node, service)?; + let ipv6_enabled = setup.layer_config().feature.network.ipv6; let raw_hints = raw_hints .cloned() @@ -992,8 +994,8 @@ pub(super) fn getaddrinfo( } = raw_hints; // Some apps (gRPC on Python) use `::` to listen on all interfaces, and usually that just means - // resolve on unspecified. So we just return that in IPv4 if that's the used IP family. - let resolved_addr = if (ai_family == libc::AF_INET) && (node == "::") { + // resolve on unspecified. So we just return that in IPv4, if IPv6 support is disabled. + let resolved_addr = if ipv6_enabled.not() && (node == "::") { // name is "" because that's what happens in real flow. vec![("".to_string(), IpAddr::V4(Ipv4Addr::UNSPECIFIED))] } else { From ff42f5ad3cbeed8bb36ff0d32a1c6eef63eb05e2 Mon Sep 17 00:00:00 2001 From: t4lz Date: Tue, 21 Jan 2025 17:44:45 +0100 Subject: [PATCH 19/24] Add other enum variants --- mirrord/agent/src/dns.rs | 12 ++++++++++- mirrord/protocol/src/dns.rs | 40 ++++++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/mirrord/agent/src/dns.rs b/mirrord/agent/src/dns.rs index 45a8d2a59b0..89b2eb66800 100644 --- a/mirrord/agent/src/dns.rs +++ b/mirrord/agent/src/dns.rs @@ -123,7 +123,17 @@ impl DnsWorker { options.attempts = attempts; options.ip_strategy = if support_ipv6 { tracing::debug!("IPv6 support enabled. Respecting client IP family."); - request.family.into() + request + .family + .try_into() + .inspect_err(|e| { + tracing::error!(%e, + "Unknown address family in addrinfo request. Using IPv4 and IPv6.") + }) + // If the agent gets some new, unknown variant of family address, it's the + // client's fault, so the agent queries both IPv4 and IPv6 and if that's not + // good enough for the client, the client can error out. + .unwrap_or(hickory_resolver::config::LookupIpStrategy::Ipv4AndIpv6) } else { tracing::debug!("IPv6 support disabled. Resolving IPv4 only."); hickory_resolver::config::LookupIpStrategy::Ipv4Only diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 789a6d61953..7b61a1d9662 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -86,28 +86,54 @@ impl From for GetAddrInfoRequest { } } -#[derive(Encode, Decode, Debug, PartialEq, Eq, Copy, Clone)] +#[derive( + serde::Serialize, serde::Deserialize, Encode, Decode, Debug, PartialEq, Eq, Copy, Clone, +)] pub enum AddressFamily { Ipv4Only, Ipv6Only, Both, + Any, + /// If we add a variant and a new client sends an old agent the new variant, the agent will see + /// this variant. + #[serde(other, skip_serializing)] + UnknownAddressFamilyFromNewerClient, +} + +#[derive(thiserror::Error, Debug)] +pub enum AddressFamilyError { + #[error( + "The agent received a GetAddrInfoRequestV2 with an address family that is not yet known \ + to this version of the agent." + )] + UnsupportedFamily, } -impl From for hickory_resolver::config::LookupIpStrategy { - fn from(value: AddressFamily) -> Self { +impl TryFrom for hickory_resolver::config::LookupIpStrategy { + type Error = AddressFamilyError; + + fn try_from(value: AddressFamily) -> Result { match value { - AddressFamily::Ipv4Only => Self::Ipv4Only, - AddressFamily::Ipv6Only => Self::Ipv6Only, - AddressFamily::Both => Self::Ipv4AndIpv6, + AddressFamily::Ipv4Only => Ok(Self::Ipv4Only), + AddressFamily::Ipv6Only => Ok(Self::Ipv6Only), + AddressFamily::Both => Ok(Self::Ipv4AndIpv6), + AddressFamily::Any => Ok(Self::Ipv4thenIpv6), + AddressFamily::UnknownAddressFamilyFromNewerClient => { + Err(AddressFamilyError::UnsupportedFamily) + } } } } -#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +#[derive(serde::Serialize, serde::Deserialize, Encode, Decode, Debug, PartialEq, Eq, Clone)] pub enum SockType { Stream, Dgram, Any, + /// If we add a variant and a new client sends an old agent the new variant, the agent will see + /// this variant. + #[serde(other, skip_serializing)] + UnknownSockTypeFromNewerClient, } /// Newer, advanced version of [`GetAddrInfoRequest`] From f54b608c2446efc1edfd2f35b4a83e6e8b3cbc7c Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 22 Jan 2025 10:27:55 +0100 Subject: [PATCH 20/24] CR: variant name Old->V1 --- mirrord/agent/src/dns.rs | 4 ++-- mirrord/agent/src/entrypoint.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mirrord/agent/src/dns.rs b/mirrord/agent/src/dns.rs index 89b2eb66800..3240856275a 100644 --- a/mirrord/agent/src/dns.rs +++ b/mirrord/agent/src/dns.rs @@ -23,14 +23,14 @@ use crate::{ #[derive(Debug)] pub(crate) enum ClientGetAddrInfoRequest { - Old(GetAddrInfoRequest), + V1(GetAddrInfoRequest), V2(GetAddrInfoRequestV2), } impl ClientGetAddrInfoRequest { pub(crate) fn into_v2(self) -> GetAddrInfoRequestV2 { match self { - ClientGetAddrInfoRequest::Old(old_req) => old_req.into(), + ClientGetAddrInfoRequest::V1(old_req) => old_req.into(), ClientGetAddrInfoRequest::V2(v2_req) => v2_req, } } diff --git a/mirrord/agent/src/entrypoint.rs b/mirrord/agent/src/entrypoint.rs index 9e943114503..407bf27c33f 100644 --- a/mirrord/agent/src/entrypoint.rs +++ b/mirrord/agent/src/entrypoint.rs @@ -434,7 +434,7 @@ impl ClientConnectionHandler { } ClientMessage::GetAddrInfoRequest(request) => { self.dns_api - .make_request(ClientGetAddrInfoRequest::Old(request)) + .make_request(ClientGetAddrInfoRequest::V1(request)) .await?; } ClientMessage::GetAddrInfoRequestV2(request) => { From b4c333f76a5bdfb2aaaefc607ec5dd9ea8269a20 Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 22 Jan 2025 10:31:38 +0100 Subject: [PATCH 21/24] CR: protocol_version->set_protocol_version --- mirrord/intproxy/src/proxies/simple.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mirrord/intproxy/src/proxies/simple.rs b/mirrord/intproxy/src/proxies/simple.rs index 6a970e2146c..1f6e8669cdc 100644 --- a/mirrord/intproxy/src/proxies/simple.rs +++ b/mirrord/intproxy/src/proxies/simple.rs @@ -48,7 +48,7 @@ pub struct SimpleProxy { impl SimpleProxy { #[tracing::instrument(skip(self), level = tracing::Level::TRACE)] - fn protocol_version(&mut self, version: Version) { + fn set_protocol_version(&mut self, version: Version) { self.protocol_version.replace(version); } @@ -114,7 +114,7 @@ impl BackgroundTask for SimpleProxy { }) .await } - SimpleProxyMessage::ProtocolVersion(version) => self.protocol_version(version), + SimpleProxyMessage::ProtocolVersion(version) => self.set_protocol_version(version), } } From 73b4738bb3a8e01b6205d3f43318c111662e9710 Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 22 Jan 2025 10:55:16 +0100 Subject: [PATCH 22/24] CR: version support warning --- mirrord/intproxy/src/proxies/simple.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/mirrord/intproxy/src/proxies/simple.rs b/mirrord/intproxy/src/proxies/simple.rs index 1f6e8669cdc..49e643b259a 100644 --- a/mirrord/intproxy/src/proxies/simple.rs +++ b/mirrord/intproxy/src/proxies/simple.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use mirrord_intproxy_protocol::{LayerId, MessageId, ProxyToLayerMessage}; use mirrord_protocol::{ - dns::{GetAddrInfoRequestV2, GetAddrInfoResponse, ADDRINFO_V2_VERSION}, + dns::{AddressFamily, GetAddrInfoRequestV2, GetAddrInfoResponse, ADDRINFO_V2_VERSION}, ClientMessage, DaemonMessage, GetEnvVarsRequest, RemoteResult, }; use semver::Version; @@ -77,6 +77,14 @@ impl BackgroundTask for SimpleProxy { .send(ClientMessage::GetAddrInfoRequestV2(req)) .await; } else { + if matches!(req.family, AddressFamily::Ipv6Only) { + tracing::warn!( + "The agent version you're using does not support DNS\ + queries for IPv6 addresses. This version will only fetch IPv4\ + address. Please update to a newer agent image for better IPv6\ + support." + ) + } message_bus .send(ClientMessage::GetAddrInfoRequest(req.into())) .await; From e7de8cd81f708db6066a960fb4e4330fe5799c51 Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 22 Jan 2025 10:55:39 +0100 Subject: [PATCH 23/24] bump version again as it was also bumped in main --- mirrord/protocol/Cargo.toml | 2 +- mirrord/protocol/src/dns.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mirrord/protocol/Cargo.toml b/mirrord/protocol/Cargo.toml index 276d5f164f4..26623eb3c08 100644 --- a/mirrord/protocol/Cargo.toml +++ b/mirrord/protocol/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mirrord-protocol" -version = "1.14.0" +version = "1.15.0" authors.workspace = true description.workspace = true documentation.workspace = true diff --git a/mirrord/protocol/src/dns.rs b/mirrord/protocol/src/dns.rs index 7b61a1d9662..5958376cd94 100644 --- a/mirrord/protocol/src/dns.rs +++ b/mirrord/protocol/src/dns.rs @@ -10,7 +10,7 @@ use crate::RemoteResult; /// Minimal mirrord-protocol version that allows [`GetAddrInfoRequestV2`]. pub static ADDRINFO_V2_VERSION: LazyLock = - LazyLock::new(|| ">=1.14.0".parse().expect("Bad Identifier")); + LazyLock::new(|| ">=1.15.0".parse().expect("Bad Identifier")); #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct LookupRecord { From ab916f1e1fbd3f979b0d731c1ae5ed7008a59af5 Mon Sep 17 00:00:00 2001 From: t4lz Date: Wed, 22 Jan 2025 11:43:31 +0100 Subject: [PATCH 24/24] lock --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index fd42ec7bb37..1269d9ad275 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4477,7 +4477,7 @@ dependencies = [ [[package]] name = "mirrord-protocol" -version = "1.14.0" +version = "1.15.0" dependencies = [ "actix-codec", "bincode",