Improve http filter port UX (#2486)

* aux-funcs

* Don't allow same ports in filtered and unfiltered ports

* display ports on start when filter set

* warning when binding port not in filter ports

* medschool and schema

* CR: get instead of indexing

* Clippy: .first() instead of .get(0)

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`.

changelog.d/+show-filtered-ports.added.md

@@ -0,0 +1 @@
+Display filtered and unfiltered stolen ports when filter is set.

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.

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"

mirrord/cli/src/main.rs

@@ -202,6 +202,58 @@ fn print_config<P>(
         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::<Vec<String>>()
+                            .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,

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.
 

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())
+    }
+}
+
 /// <!--${internal}-->
 /// Helper struct for setting up ports configuration (part of the HTTP traffic stealer feature).
 ///

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

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)
 }