combine proxymode = shared and inpod_mode = true (istio#1031)

* Combine `inpod_mode = true` and `proxy_mode = shared` Signed-off-by: Benjamin Leggett <[email protected]> * Fixup Signed-off-by: Benjamin Leggett <[email protected]> * Fixup Signed-off-by: Benjamin Leggett <[email protected]> * Bring back some tests as dedicated Signed-off-by: Benjamin Leggett <[email protected]> * comment Signed-off-by: Benjamin Leggett <[email protected]> * Fix bench Signed-off-by: Benjamin Leggett <[email protected]> * Fixup/resync Signed-off-by: Benjamin Leggett <[email protected]> --------- Signed-off-by: Benjamin Leggett <[email protected]>
howardjohn · Jul 8, 2024 · 1963cd7 · 1963cd7
1 parent c68a919
commit 1963cd7
Show file tree

Hide file tree

Showing 11 changed files with 68 additions and 79 deletions.
diff --git a/Development.md b/Development.md
@@ -54,7 +54,7 @@ INPOD_UDS=/tmp/ztunnel cargo run --example inpodserver -- pod1
 run ztunnel (as root) with:
 
 ```shell
-RUST_LOG=debug INPOD_ENABLED=true INPOD_UDS=/tmp/ztunnel FAKE_CA="true" XDS_ADDRESS="" LOCAL_XDS_PATH=./examples/localhost.yaml cargo run --features testing
+RUST_LOG=debug PROXY_MODE=shared INPOD_UDS=/tmp/ztunnel FAKE_CA="true" XDS_ADDRESS="" LOCAL_XDS_PATH=./examples/localhost.yaml cargo run --features testing
 ```
 
 (note: to run ztunnel as root, consider using `export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="sudo -E"` so cargo `sudo` the binary)
@@ -162,7 +162,7 @@ xargs env <<EOF
 INPOD_UDS=/tmp/worker1-ztunnel/ztunnel.sock
 CLUSTER_ID=Kubernetes
 RUST_LOG=debug
-INPOD_ENABLED="true"
+PROXY_MODE="shared"
 ISTIO_META_DNS_CAPTURE="true"
 ISTIO_META_DNS_PROXY_ADDR="127.0.0.1:15053"
 SERVICE_ACCOUNT=ztunnel

diff --git a/benches/throughput.rs b/benches/throughput.rs
@@ -136,7 +136,7 @@ fn initialize_environment(
     SyncSender<usize>,
     Receiver<Result<(), io::Error>>,
 )> {
-    let mut manager = setup_netns_test!(TestMode::InPod);
+    let mut manager = setup_netns_test!(TestMode::Shared);
     let (server, mut manager) = run_async_blocking(async move {
         if ztunnel_mode != WorkloadMode::Direct {
             // we need a client ztunnel

diff --git a/src/app.rs b/src/app.rs
@@ -134,8 +134,8 @@ pub async fn build_with_cert(
     )
     .map_err(|e| anyhow::anyhow!("failed to start proxy factory {:?}", e))?;
 
-    if config.inpod_enabled {
-        tracing::info!("in-pod mode enabled");
+    if config.proxy_mode == config::ProxyMode::Shared {
+        tracing::info!("shared proxy mode - in-pod mode enabled");
         let run_future = init_inpod_proxy_mgr(
             &mut registry,
             &mut admin_server,

diff --git a/src/config.rs b/src/config.rs
@@ -37,7 +37,6 @@ const KUBERNETES_SERVICE_HOST: &str = "KUBERNETES_SERVICE_HOST";
 const NETWORK: &str = "NETWORK";
 const NODE_NAME: &str = "NODE_NAME";
 const PROXY_MODE: &str = "PROXY_MODE";
-const INPOD_ENABLED: &str = "INPOD_ENABLED";
 const INPOD_MARK: &str = "INPOD_MARK";
 const INPOD_UDS: &str = "INPOD_UDS";
 const INPOD_PORT_REUSE: &str = "INPOD_PORT_REUSE";
@@ -224,7 +223,6 @@ pub struct Config {
     // System dns resolver opts used for on-demand ztunnel dns resolution
     pub dns_resolver_opts: ResolverOpts,
 
-    pub inpod_enabled: bool,
     pub inpod_uds: PathBuf,
     pub inpod_port_reuse: bool,
     pub inpod_mark: u32,
@@ -490,7 +488,6 @@ pub fn construct_config(pc: ProxyConfig) -> Result<Config, Error> {
         proxy_args: parse_args(),
         dns_resolver_cfg,
         dns_resolver_opts,
-        inpod_enabled: parse_default(INPOD_ENABLED, false)?,
         inpod_uds: parse_default(INPOD_UDS, PathBuf::from("/var/run/ztunnel/ztunnel.sock"))?,
         inpod_port_reuse: parse_default(INPOD_PORT_REUSE, true)?,
         inpod_mark: parse_default(INPOD_MARK, DEFAULT_INPOD_MARK)?,

diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs
@@ -40,6 +40,7 @@ use crate::state::workload::address::Address;
 use crate::state::workload::application_tunnel::Protocol as AppProtocol;
 use crate::{assertions, copy, proxy, socket, strng, tls};
 
+use crate::config::ProxyMode;
 use crate::drain::run_with_drain;
 use crate::proxy::h2;
 use crate::state::workload::{self, NetworkAddress, Workload};
@@ -187,7 +188,7 @@ impl Inbound {
                     return req.send_error(build_response(StatusCode::BAD_REQUEST));
                 }
             };
-        let illegal_call = if pi.cfg.inpod_enabled {
+        let illegal_call = if pi.cfg.proxy_mode == ProxyMode::Shared {
             // User sent a request to pod:15006. This would forward to pod:15006 infinitely
             // Use hbone_addr instead of upstream_addr to allow for sandwich mode, which intentionally
             // sends to 15008.

diff --git a/src/proxy/inbound_passthrough.rs b/src/proxy/inbound_passthrough.rs
@@ -127,12 +127,14 @@ impl InboundPassthrough {
         let dest_addr = socket::orig_dst_addr_or_default(&inbound_stream);
         // Check if it is an illegal call to ourself, which could trampoline to illegal addresses or
         // lead to infinite loops
-        let illegal_call = if pi.cfg.inpod_enabled {
+        let illegal_call = if pi.cfg.proxy_mode == ProxyMode::Shared {
             // User sent a request to pod:15006. This would forward to pod:15006 infinitely
+            // OR
+            // User sent a request to the ztunnel directly. This isn't allowed
             pi.cfg.illegal_ports.contains(&dest_addr.port())
+                || Some(dest_addr.ip()) == pi.cfg.local_ip
         } else {
-            // User sent a request to the ztunnel directly. This isn't allowed
-            pi.cfg.proxy_mode == ProxyMode::Shared && Some(dest_addr.ip()) == pi.cfg.local_ip
+            false
         };
         if illegal_call {
             metrics::log_early_deny(

diff --git a/src/proxy/outbound.rs b/src/proxy/outbound.rs
@@ -60,13 +60,13 @@ impl Outbound {
             transparent,
             "listener established",
         );
-        let inpod = pi.cfg.inpod_enabled;
+        let mode = pi.cfg.proxy_mode;
         Ok(Outbound {
             pi,
             listener,
             drain,
             // Do not need to spoof with inpod mode for outbound
-            enable_orig_src: transparent && !inpod,
+            enable_orig_src: transparent && mode != ProxyMode::Shared,
         })
     }
 
@@ -165,9 +165,9 @@ impl OutboundConnection {
 
         // Block calls to ztunnel directly, unless we are in "in-pod".
         // For in-pod, this isn't an issue and is useful: this allows things like prometheus scraping ztunnel.
-        if self.pi.cfg.proxy_mode == ProxyMode::Shared
+        // TODO is this actually wrong for ProxyMode::Dedicated? Seems like a valid case
+        if self.pi.cfg.proxy_mode != ProxyMode::Shared
             && Some(dest_addr.ip()) == self.pi.cfg.local_ip
-            && !self.pi.cfg.inpod_enabled
         {
             metrics::log_early_deny(source_addr, dest_addr, Reporter::source, Error::SelfCall);
             return;
@@ -268,7 +268,7 @@ impl OutboundConnection {
     ) -> Result<(), Error> {
         // Create a TCP connection to upstream
         // We do not need spoofing for inbound
-        let local = if self.enable_orig_src && !self.pi.cfg.inpod_enabled {
+        let local = if self.enable_orig_src && self.pi.cfg.proxy_mode != ProxyMode::Shared {
             super::get_original_src_from_stream(&stream)
         } else {
             None

diff --git a/src/proxy/socks5.rs b/src/proxy/socks5.rs
@@ -31,6 +31,7 @@ use tokio::net::TcpStream;
 use tokio::sync::watch;
 use tracing::{debug, error, info, info_span, Instrument};
 
+use crate::config;
 use crate::drain::run_with_drain;
 use crate::drain::DrainWatcher;
 use crate::proxy::outbound::OutboundConnection;
@@ -60,13 +61,13 @@ impl Socks5 {
             "listener established",
         );
 
-        let inpod = pi.cfg.inpod_enabled;
+        let mode = pi.cfg.proxy_mode;
         Ok(Socks5 {
             pi,
             listener,
             drain,
             // Do not need to spoof with inpod mode for outbound
-            enable_orig_src: transparent && !inpod,
+            enable_orig_src: transparent && mode != config::ProxyMode::Shared,
         })
     }
 

diff --git a/src/test_helpers.rs b/src/test_helpers.rs
@@ -122,6 +122,7 @@ pub fn test_config_with_port_xds_addr_and_root_cert(
         outbound_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0),
         inbound_plaintext_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0),
         dns_proxy_addr: config::Address::Localhost(false, 0),
+        proxy_mode: config::ProxyMode::Dedicated,
         illegal_ports: HashSet::new(), // for "direct" tests, since the ports are latebound, we can't test illegal ports
         fake_self_inbound: true, // for "direct" tests, since the ports are latebound, we have to do this. Yes, this is test concerns leaking into prod code
         ..config::parse_config().unwrap()

diff --git a/src/test_helpers/linux.rs b/src/test_helpers/linux.rs
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::config::ConfigSource;
+use crate::config::{ConfigSource, ProxyMode};
 use crate::rbac::Authorization;
 use crate::state::service::{endpoint_uid, Endpoint, Service};
 use crate::state::workload::{gatewayaddress, Workload};
@@ -24,7 +24,7 @@ use crate::{config, identity, proxy, strng};
 
 use crate::signal::ShutdownTrigger;
 use crate::test_helpers::inpod::start_ztunnel_server;
-use crate::test_helpers::linux::TestMode::InPod;
+use crate::test_helpers::linux::TestMode::Shared;
 use itertools::Itertools;
 use nix::unistd::mkdtemp;
 use std::net::IpAddr;
@@ -79,8 +79,8 @@ impl Drop for WorkloadManager {
 
 #[derive(Clone, Copy, Ord, PartialOrd, PartialEq, Eq)]
 pub enum TestMode {
-    InPod,
-    SharedNode,
+    Shared,
+    Dedicated,
 }
 
 impl WorkloadManager {
@@ -107,7 +107,7 @@ impl WorkloadManager {
     /// deploy_ztunnel is called. As such, you must ensure this is called after all other workloads are created.
     pub async fn deploy_ztunnel(&mut self, node: &str) -> anyhow::Result<TestApp> {
         let mut inpod_uds: PathBuf = "/dev/null".into();
-        let ztunnel_server = if self.mode == InPod {
+        let ztunnel_server = if self.mode == Shared {
             inpod_uds = self.tmp_dir.join(node);
             Some(start_ztunnel_server(inpod_uds.clone()))
         } else {
@@ -125,7 +125,11 @@ impl WorkloadManager {
             policies: self.policies.clone(),
             services: self.services.values().cloned().collect_vec(),
         };
-        let inpod_enabled = ztunnel_server.is_some();
+        let proxy_mode = if ztunnel_server.is_some() {
+            ProxyMode::Shared
+        } else {
+            ProxyMode::Dedicated
+        };
         let (mut tx_cfg, rx_cfg) = mpsc_ack(1);
         tx_cfg.send(initial_config).await?;
         let local_xds_config = Some(ConfigSource::Dynamic(Arc::new(Mutex::new(rx_cfg))));
@@ -137,15 +141,15 @@ impl WorkloadManager {
             local_node: Some(node.to_string()),
             local_ip: Some(ns.ip()),
             inpod_uds,
-            inpod_enabled,
+            proxy_mode,
             ..config::parse_config().unwrap()
         };
         let (tx, rx) = std::sync::mpsc::sync_channel(0);
         // Setup the ztunnel...
         let cloned_ns = ns.clone();
         ns.run_ready(move |ready| async move {
-            if !inpod_enabled {
-                // not needed in inpod mode. In in pod mode we run `ztunnel-redirect-inpod.sh`
+            if proxy_mode != ProxyMode::Shared {
+                // not needed in "inpod" (shared proxy) mode. In shared mode we run `ztunnel-redirect-inpod.sh`
                 // inside the pod's netns
                 helpers::run_command(&format!("scripts/ztunnel-redirect.sh {ip}"))?;
             }
@@ -473,7 +477,7 @@ impl<'a> TestWorkloadBuilder<'a> {
         if self.captured {
             // Setup redirection
             let zt_info = self.manager.ztunnels.get_mut(node.as_str()).unwrap();
-            if self.manager.mode == InPod {
+            if self.manager.mode == Shared {
                 // In the new pod network
                 network_namespace
                     .netns()