diff --git a/changelog.d/+forbid-ambiguous-filter-ports.changed.md b/changelog.d/+forbid-ambiguous-filter-ports.changed.md new file mode 100644 index 00000000000..6b15b438f10 --- /dev/null +++ b/changelog.d/+forbid-ambiguous-filter-ports.changed.md @@ -0,0 +1 @@ +Now not accepting configs with the same port in `feature.network.incoming.ports` and in `feature.network.incoming.http_filter.ports`. diff --git a/changelog.d/+show-filtered-ports.added.md b/changelog.d/+show-filtered-ports.added.md new file mode 100644 index 00000000000..6a9cd3d510e --- /dev/null +++ b/changelog.d/+show-filtered-ports.added.md @@ -0,0 +1 @@ +Display filtered and unfiltered stolen ports when filter is set. diff --git a/changelog.d/+warn-excluded-filter-port.added.md b/changelog.d/+warn-excluded-filter-port.added.md new file mode 100644 index 00000000000..b0dcaa18d7c --- /dev/null +++ b/changelog.d/+warn-excluded-filter-port.added.md @@ -0,0 +1 @@ +When an http filter is set and a port is bound that is not included in the filtered ports, and there are no unfiltered ports specified, emit a warning. diff --git a/mirrord-schema.json b/mirrord-schema.json index 12d5780b97e..5e089010ac7 100644 --- a/mirrord-schema.json +++ b/mirrord-schema.json @@ -784,7 +784,7 @@ }, "ports": { "title": "feature.network.incoming.http_filter.ports {#feature-network-incoming-http_filter-ports}", - "description": "Activate the HTTP traffic filter only for these ports.\n\nOther ports will *not* be stolen, unless listed in [`feature.network.incoming.ports`](#feature-network-incoming-ports).", + "description": "Activate the HTTP traffic filter only for these ports.\n\nOther ports will *not* be stolen, unless listed in [`feature.network.incoming.ports`](#feature-network-incoming-ports).\n\nSet to [80, 8080] by default.", "anyOf": [ { "$ref": "#/definitions/PortList" diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index d5a87a1e0a9..87544d70267 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -202,6 +202,58 @@ fn print_config

( incoming_info )); + // When the http filter is set, the rules of what ports get stolen are different, so make it + // clear to users in that case which ports are stolen. + if config.feature.network.incoming.is_steal() + && config.feature.network.incoming.http_filter.is_filter_set() + { + let filtered_ports_str = config + .feature + .network + .incoming + .http_filter + .get_filtered_ports() + .and_then(|filtered_ports| match filtered_ports.len() { + 0 => None, + 1 => Some(format!( + "port {} (filtered)", + filtered_ports.first().unwrap() + )), + _ => Some(format!("ports {filtered_ports:?} (filtered)")), + }); + let unfiltered_ports_str = + config + .feature + .network + .incoming + .ports + .as_ref() + .and_then(|ports| match ports.len() { + 0 => None, + 1 => Some(format!( + "port {} (unfiltered)", + ports.iter().next().unwrap() + )), + _ => Some(format!( + "ports [{}] (unfiltered)", + ports + .iter() + .copied() + .map(|n| n.to_string()) + .collect::>() + .join(", ") + )), + }); + let and = if filtered_ports_str.is_some() && unfiltered_ports_str.is_some() { + " and " + } else { + "" + }; + let filtered_port_str = filtered_ports_str.unwrap_or_default(); + let unfiltered_ports_str = unfiltered_ports_str.unwrap_or_default(); + messages.push(format!("incoming: traffic will only be stolen from {filtered_port_str}{and}{unfiltered_ports_str}")); + } + let outgoing_info = match ( config.feature.network.outgoing.tcp, config.feature.network.outgoing.udp, diff --git a/mirrord/config/configuration.md b/mirrord/config/configuration.md index eb3d8f101a3..3f21d634353 100644 --- a/mirrord/config/configuration.md +++ b/mirrord/config/configuration.md @@ -990,6 +990,8 @@ Activate the HTTP traffic filter only for these ports. Other ports will *not* be stolen, unless listed in [`feature.network.incoming.ports`](#feature-network-incoming-ports). +Set to [80, 8080] by default. + ### feature.network.outgoing {#feature-network-outgoing} Tunnel outgoing network operations through mirrord. diff --git a/mirrord/config/src/feature/network/incoming/http_filter.rs b/mirrord/config/src/feature/network/incoming/http_filter.rs index 6ff1326b8c3..a57ff653b33 100644 --- a/mirrord/config/src/feature/network/incoming/http_filter.rs +++ b/mirrord/config/src/feature/network/incoming/http_filter.rs @@ -86,10 +86,22 @@ pub struct HttpFilterConfig { /// /// Other ports will *not* be stolen, unless listed in /// [`feature.network.incoming.ports`](#feature-network-incoming-ports). + /// + /// Set to [80, 8080] by default. #[config(env = "MIRRORD_HTTP_FILTER_PORTS", default)] pub ports: PortList, } +impl HttpFilterConfig { + pub fn is_filter_set(&self) -> bool { + self.header_filter.is_some() || self.path_filter.is_some() + } + + pub fn get_filtered_ports(&self) -> Option<&[u16]> { + self.is_filter_set().then(|| self.ports.as_slice()) + } +} + /// /// Helper struct for setting up ports configuration (part of the HTTP traffic stealer feature). /// diff --git a/mirrord/config/src/lib.rs b/mirrord/config/src/lib.rs index 8b9e81128c6..aa8b16f816b 100644 --- a/mirrord/config/src/lib.rs +++ b/mirrord/config/src/lib.rs @@ -15,7 +15,7 @@ pub mod internal_proxy; pub mod target; pub mod util; -use std::path::Path; +use std::{collections::HashSet, ops::Not, path::Path}; use config::{ConfigContext, ConfigError, MirrordConfig}; use experimental::ExperimentalConfig; @@ -362,6 +362,28 @@ impl LayerConfig { ))? } + if let (Some(unfiltered_ports), Some(filtered_ports)) = ( + self.feature.network.incoming.ports.as_ref(), + self.feature + .network + .incoming + .http_filter + .get_filtered_ports(), + ) { + if unfiltered_ports + .is_disjoint(&HashSet::from_iter(filtered_ports.iter().copied())) + .not() + { + Err(ConfigError::Conflict(format!( + "`feature.network.incoming.ports` (set to {unfiltered_ports:?}) and \ + `feature.network.incoming.http_filter.ports` (set to {filtered_ports:?}) must \ + be disjoint. If you want traffic to a port ot be filtered, include it only in \ + the filter port. To steal all the traffic from that port without filtering, \ + include it only in `feature.network.incoming.ports`." + )))? + } + } + if !self.feature.copy_target.enabled && self .target diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index 20d26b99b1f..bcb62fb98b5 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -180,26 +180,51 @@ fn is_ignored_tcp_port(addr: &SocketAddr, config: &IncomingConfig) -> bool { .get_by_left(&addr.port()) .copied() .unwrap_or_else(|| addr.port()); - let http_filter_used = config.mode == IncomingMode::Steal - && (config.http_filter.header_filter.is_some() || config.http_filter.path_filter.is_some()); + let http_filter_used = config.mode == IncomingMode::Steal && config.http_filter.is_filter_set(); // this is a bit weird but it makes more sense configured ports are the remote port // and not the local, so the check is done on the mapped port // see https://github.com/metalbear-co/mirrord/issues/2397 - let not_stolen_with_filter = !http_filter_used - || config - .http_filter - .ports - .as_slice() - .iter() - .all(|port| *port != mapped_port); + let not_a_filtered_port = config + .http_filter + .ports + .as_slice() + .iter() + .all(|port| *port != mapped_port); + + let not_stolen_with_filter = !http_filter_used || not_a_filtered_port; + // Unfiltered ports were specified and the requested port is not one of them, or an HTTP filter + // is set and no unfiltered ports were specified. let not_whitelisted = config .ports .as_ref() .map(|ports| !ports.contains(&mapped_port)) .unwrap_or(http_filter_used); + if http_filter_used && not_a_filtered_port && config.ports.is_none() { + // User specified a filter that does not include this port, and did not specify any + // unfiltered ports. + // It's plausible that the user did not know the port has to be in either port list to be + // stolen when an HTTP filter is set, so show a warning. + let port_text = if mapped_port != addr.port() { + format!( + "Remote port {mapped_port} (mapped from local port {})", + addr.port() + ) + } else { + format!("Port {mapped_port}") + }; + warn!( + "{port_text} was not included in the filtered ports, and also not in the non-filtered \ + ports, and will therefore be bound locally. If this is intentional, ignore this \ + warning. If you want the http filter to apply to this port, add it to \ + `feature.network.incoming.http_filter.ports`. \ + If you want to steal all the traffic of this port, add it to \ + `feature.network.incoming.ports`." + ); + } + is_ignored_port(addr) || (not_stolen_with_filter && not_whitelisted) }