Fixup QCMP ping (#1095)

Author: Jake-Shadle

.ci/xdp/veth-integ-test.sh

@@ -27,21 +27,30 @@ ip link add veth-cs type veth peer name veth-proxy
 ip link set veth-cs netns cs
 ip link set veth-proxy netns proxy
 
+PROXY_IP="10.0.0.2"
+OUTSIDE_IP="10.0.0.1"
+
 echo "::notice file=$source,line=$LINENO::Adding IPs"
-ip -n cs addr add 10.0.0.1/24 dev veth-cs
-ip -n proxy addr add 10.0.0.2/24 dev veth-proxy
+ip -n cs addr add $OUTSIDE_IP/24 dev veth-cs
+ip -n proxy addr add $PROXY_IP/24 dev veth-proxy
 
 echo "::notice file=$source,line=$LINENO::Creating network namespaces"
 ip -n cs link set veth-cs up
 ip -n proxy link set veth-proxy up
 
+# XDP on a veth has a bit of an annoying requirement in newer kernel versions,
+# both sides need to have an XDP program attached for traffic to appear on the
+# one we actually want to test
+ROOT=$(git rev-parse --show-toplevel)
+echo "Adding dummy program"
+ip -n cs link set veth-cs xdpgeneric obj "$ROOT/crates/xdp/bin/dummy.bin" sec xdp
+
 ip netns exec cs fortio udp-echo&
-ip netns exec proxy ./target/debug/quilkin proxy --to 10.0.0.1:8078 --publish.udp.xdp&
+ip netns exec proxy ./target/debug/quilkin --service.udp --service.qcmp proxy --to $OUTSIDE_IP:8078 --service.udp.xdp --service.udp.xdp.network-interface veth-proxy&
 
 echo "::notice file=$source,line=$LINENO::Launching client"
-ip netns exec cs fortio load -n 10 udp://10.0.0.2:7777 2> ./target/logs.txt
+ip netns exec cs fortio load -n 10 udp://$PROXY_IP:7777 2> ./target/logs.txt
 logs=$(cat ./target/logs.txt)
-echo "$logs"
 
 regex="Total Bytes sent: ([0-9]+), received: ([0-9]+)"
 
@@ -52,6 +61,10 @@ if [[ $logs =~ $regex ]]; then
   # even consistently get that on my local machine so I doubt CI will fair better
   if [[ $recv -ne "0" ]]; then
     echo "::notice file=$source,line=$LINENO::Successfully sent ${send}B and received ${recv}B"
+
+    # Now test QCMP pings which was also enabled in the proxy
+    ip netns exec cs ./target/debug/quilkin qcmp ping $PROXY_IP:7600
+
     exit 0
   fi
 

crates/ebpf/Cargo.toml

@@ -6,6 +6,7 @@ edition = "2021"
 
 [dependencies]
 aya-ebpf = "0.1.1"
+#aya-log-ebpf = "0.1"
 network-types = "0.0.7"
 
 [[bin]]

crates/ebpf/build.sh

@@ -9,7 +9,9 @@ ROOT=$(git rev-parse --show-toplevel)
 EBPF_ROOT="$ROOT/crates/ebpf"
 
 cargo +nightly build -Z build-std=core --release --target bpfel-unknown-none --manifest-path "$EBPF_ROOT/Cargo.toml"
+clang -target bpf -Wall -O2 -g -c "$EBPF_ROOT/src/dummy.c" -o "$EBPF_ROOT/target/bpfel-unknown-none/release/dummy"
 
 if [[ $1 == '--update' ]]; then
     cp "$EBPF_ROOT/target/bpfel-unknown-none/release/packet-router" "$ROOT/crates/xdp/bin/packet-router.bin"
+    cp "$EBPF_ROOT/target/bpfel-unknown-none/release/dummy" "$ROOT/crates/xdp/bin/dummy.bin"
 fi

crates/ebpf/src/dummy.c

@@ -0,0 +1,8 @@
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp")
+int socket_router(struct xdp_md *ctx)
+{
+    return XDP_PASS;
+}

crates/xdp/bin/dummy.bin

@@ -187,9 +187,10 @@ impl EbpfProgram {
         nic: NicIndex,
         flags: aya::programs::XdpFlags,
     ) -> Result<aya::programs::xdp::XdpLinkId, aya::programs::ProgramError> {
-        // Would be good to enable this if we do end up adding log messages to
-        // the eBPF program
         if let Err(_error) = aya_log::EbpfLogger::init(&mut self.bpf) {
+            // Would be good to enable this if we do end up adding log messages to
+            // the eBPF program, right now we don't so this will error as the ring
+            // buffer used to transfer log messages is not created if there are none
             //tracing::warn!(%error, "failed to initialize eBPF logging");
         }
 

crates/xdp/src/lib.rs

@@ -269,7 +269,6 @@ impl Cli {
         let locality = self.locality.locality();
         self.providers
             .spawn_providers(&config, ready.clone(), locality.clone());
-        self.service.spawn_services(&config, &shutdown_rx)?;
 
         match self.command {
             Some(Commands::Agent(agent)) => {
@@ -307,7 +306,10 @@ impl Cli {
                 relay.run(locality, config, old_ready, shutdown_rx).await
             }
             Some(_) => unreachable!(),
-            None => shutdown_rx.changed().await.map_err(From::from),
+            None => {
+                self.service.spawn_services(&config, &shutdown_rx)?;
+                shutdown_rx.changed().await.map_err(From::from)
+            }
         }
     }
 

src/cli.rs

@@ -22,7 +22,7 @@ use crate::filters::FilterFactory;
 
 use crate::signal::ShutdownRx;
 
-pub use crate::components::proxy::Ready;
+pub use crate::{cli::service::XdpOptions, components::proxy::Ready};
 
 define_port!(7777);
 
@@ -65,55 +65,6 @@ pub struct Proxy {
     pub xdp_opts: XdpOptions,
 }
 
-/// XDP (eXpress Data Path) options
-#[derive(clap::Args, Clone, Debug)]
-pub struct XdpOptions {
-    /// The name of the network interface to bind the XDP socket(s) to.
-    ///
-    /// If not specified quilkin will attempt to determine the most appropriate
-    /// network interface to use. Quilkin will exit with an error if the network
-    /// interface does not exist, or a suitable default cannot be determined.
-    #[clap(long = "publish.udp.xdp.network-interface")]
-    pub network_interface: Option<String>,
-    /// Forces the use of XDP.
-    ///
-    /// If XDP is not available on the chosen NIC, Quilkin exits with an error.
-    /// If false, io-uring will be used as the fallback implementation.
-    #[clap(long = "publish.udp.xdp")]
-    pub force_xdp: bool,
-    /// Forces the use of [`XDP_ZEROCOPY`](https://www.kernel.org/doc/html/latest/networking/af_xdp.html#xdp-copy-and-xdp-zerocopy-bind-flags)
-    ///
-    /// If zero copy is not available on the chosen NIC, Quilkin exits with an error
-    #[clap(long = "publish.udp.xdp.zerocopy")]
-    pub force_zerocopy: bool,
-    /// Forces the use of [TX checksum offload](https://docs.kernel.org/6.8/networking/xsk-tx-metadata.html)
-    ///
-    /// TX checksum offload is an optional feature allowing the data portion of
-    /// a packet to have its internet checksum calculation offloaded to the NIC,
-    /// as otherwise this is done in software
-    #[clap(long = "publish.udp.xdp.tco")]
-    pub force_tx_checksum_offload: bool,
-    /// The maximum amount of memory mapped for packet buffers, in bytes
-    ///
-    /// If not specified, this defaults to 4MiB (2k allocated packets of 2k each at a time)
-    /// per NIC queue, ie 128MiB on a 32 queue NIC
-    #[clap(long = "publish.udp.xdp.memory-limit")]
-    pub maximum_memory: Option<u64>,
-}
-
-#[allow(clippy::derivable_impls)]
-impl Default for XdpOptions {
-    fn default() -> Self {
-        Self {
-            network_interface: None,
-            force_xdp: false,
-            force_zerocopy: false,
-            force_tx_checksum_offload: false,
-            maximum_memory: None,
-        }
-    }
-}
-
 impl Default for Proxy {
     fn default() -> Self {
         Self {

src/cli/proxy.rs

@@ -102,7 +102,9 @@ impl Ping {
                     "final results"
                 );
             }
-            None => tracing::error!("no successful results"),
+            None => {
+                eyre::bail!("no successful results");
+            }
         }
 
         Ok(())

src/cli/qcmp.rs

@@ -196,6 +196,11 @@ impl Service {
         self
     }
 
+    pub fn xdp(mut self, xdp_opts: XdpOptions) -> Self {
+        self.xdp = xdp_opts;
+        self
+    }
+
     /// Sets the xDS service port.
     pub fn any_service_enabled(&self) -> bool {
         self.udp_enabled
@@ -367,7 +372,7 @@ impl Service {
                         return Err(err);
                     }
 
-                    tracing::debug!(
+                    tracing::warn!(
                         ?err,
                         "failed to spawn XDP I/O loop, falling back to io-uring"
                     );
@@ -439,15 +444,18 @@ impl Service {
             eyre::bail!("XDP currently disabled by default");
         }
 
-        tracing::info!(port=%self.mds_port, "setting up xdp module");
+        let udp_port = if self.udp_enabled { self.udp_port } else { 0 };
+        let qcmp_port = if self.qcmp_enabled { self.qcmp_port } else { 0 };
+
+        tracing::info!(udp_port, qcmp_port, "setting up xdp module");
         let workers = xdp::setup_xdp_io(xdp::XdpConfig {
             nic: self
                 .xdp
                 .network_interface
                 .as_deref()
                 .map_or(xdp::NicConfig::Default, xdp::NicConfig::Name),
-            external_port: if self.udp_enabled { self.udp_port } else { 0 },
-            qcmp_port: if self.qcmp_enabled { self.qcmp_port } else { 0 },
+            external_port: udp_port,
+            qcmp_port,
             maximum_packet_memory: self.xdp.maximum_memory,
             require_zero_copy: self.xdp.force_zerocopy,
             require_tx_checksum: self.xdp.force_tx_checksum_offload,