Skip to content

Commit

Permalink
test(iroh-net): Removed unused stun_test_ip field from DerpNode (#1450)
Browse files Browse the repository at this point in the history
## Description

The DerpNode had a field to override the IP address of the STUN server
in testing scenarios.  However we do not use this, it is always the
same address as the derp server, just like in production.

Removing this removes some code complexity.

## Notes & open questions

I believe this was an unused left-over from the wireguard conversion.

## Change checklist

- [x] Self-review.
- [x] ~~Documentation updates if relevant.~~
- [x] Tests if relevant.
  • Loading branch information
flub authored Sep 4, 2023
1 parent bc26321 commit 4ef3611
Show file tree
Hide file tree
Showing 10 changed files with 17 additions and 49 deletions.
2 changes: 0 additions & 2 deletions iroh-net/src/defaults.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ pub fn default_na_derp_region() -> DerpRegion {
stun_port: DEFAULT_DERP_STUN_PORT,
ipv4: UseIpv4::Some(NA_DERP_IPV4),
ipv6: UseIpv6::TryDns,
stun_test_ip: None,
};
DerpRegion {
region_id: NA_REGION_ID,
Expand All @@ -60,7 +59,6 @@ pub fn default_eu_derp_region() -> DerpRegion {
stun_port: DEFAULT_DERP_STUN_PORT,
ipv4: UseIpv4::Some(EU_DERP_IPV4),
ipv6: UseIpv6::TryDns,
stun_test_ip: None,
};
DerpRegion {
region_id: EU_REGION_ID,
Expand Down
2 changes: 0 additions & 2 deletions iroh-net/src/derp/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ mod tests {
url: format!("http://localhost:{port}").parse().unwrap(),
stun_only: false,
stun_port: 0,
stun_test_ip: None,
ipv4: UseIpv4::Some(addr),
ipv6: UseIpv6::Disabled,
}
Expand Down Expand Up @@ -235,7 +234,6 @@ mod tests {
url: format!("https://localhost:{port}").parse().unwrap(),
stun_only: false,
stun_port: 0,
stun_test_ip: None,
ipv4: UseIpv4::Some(addr),
ipv6: UseIpv6::Disabled,
}
Expand Down
1 change: 0 additions & 1 deletion iroh-net/src/derp/http/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,6 @@ mod tests {
url: "https://bad.url".parse().unwrap(),
stun_only: false,
stun_port: 0,
stun_test_ip: None,
ipv4: UseIpv4::Some("35.175.99.112".parse().unwrap()),
ipv6: UseIpv6::Disabled,
}
Expand Down
18 changes: 10 additions & 8 deletions iroh-net/src/derp/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::{
collections::HashMap,
fmt,
net::{IpAddr, Ipv4Addr, Ipv6Addr},
net::{Ipv4Addr, Ipv6Addr},
sync::Arc,
};

Expand Down Expand Up @@ -92,7 +92,6 @@ impl DerpMap {
stun_port,
ipv4: derp_ipv4,
ipv6: derp_ipv6,
stun_test_ip: None,
}
.into()],
avoid: false,
Expand Down Expand Up @@ -190,17 +189,20 @@ pub struct DerpNode {
///
/// This name MUST be unique among all configured DERP servers.
pub name: String,
/// The numeric region ID
/// The numeric region ID.
pub region_id: u16,
/// The [`Url`] where this derp server can be dialed
/// The [`Url`] where this derp server can be dialed.
#[debug("{}", url)]
pub url: Url,
/// Whether this derp server should only be used for STUN requests
/// Whether this derp server should only be used for STUN requests.
///
/// This essentially allows you to use a normal STUN server as a DERP node, no DERP
/// functionality is used.
pub stun_only: bool,
/// The stun port of the derp server
/// The stun port of the derp server.
///
/// Setting this to `0` means the default STUN port is used.
pub stun_port: u16,
/// Optional stun-specific IP address
pub stun_test_ip: Option<IpAddr>,
/// Whether to dial this server on IPv4.
pub ipv4: UseIpv4,
/// Whether to dial this server on IPv6.
Expand Down
4 changes: 2 additions & 2 deletions iroh-net/src/magic_endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ pub async fn get_peer_id(connection: &quinn::Connection) -> Result<PublicKey> {
mod tests {
use tracing::{info, info_span, Instrument};

use crate::test_utils::run_derp_and_stun;
use crate::test_utils::run_derper;

use super::*;

Expand All @@ -491,7 +491,7 @@ mod tests {
#[tokio::test]
async fn magic_endpoint_connect_close() {
let _guard = iroh_test::logging::setup();
let (derp_map, region_id, _guard) = run_derp_and_stun([127, 0, 0, 1].into()).await.unwrap();
let (derp_map, region_id, _guard) = run_derper().await.unwrap();
let server_secret_key = SecretKey::generate();
let server_peer_id = server_secret_key.public();

Expand Down
18 changes: 3 additions & 15 deletions iroh-net/src/magicsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2541,7 +2541,7 @@ pub(crate) mod tests {
use tracing_subscriber::{prelude::*, EnvFilter};

use super::*;
use crate::{test_utils::run_derp_and_stun, tls, MagicEndpoint};
use crate::{test_utils::run_derper, tls, MagicEndpoint};

fn make_transmit(destination: SocketAddr) -> quinn_udp::Transmit {
quinn_udp::Transmit {
Expand Down Expand Up @@ -2661,10 +2661,6 @@ pub(crate) mod tests {
c.close().await.unwrap();
}

struct Devices {
stun_ip: IpAddr,
}

/// Magicsock plus wrappers for sending packets
#[derive(Clone)]
struct MagicStack {
Expand Down Expand Up @@ -2783,11 +2779,7 @@ pub(crate) mod tests {
async fn test_two_devices_roundtrip_quinn_magic() -> Result<()> {
setup_multithreaded_logging();

let devices = Devices {
stun_ip: "127.0.0.1".parse()?,
};

let (derp_map, region, _cleanup) = run_derp_and_stun(devices.stun_ip).await?;
let (derp_map, region, _cleanup) = run_derper().await?;

let m1 = MagicStack::new(derp_map.clone()).await?;
let m2 = MagicStack::new(derp_map.clone()).await?;
Expand Down Expand Up @@ -2954,12 +2946,8 @@ pub(crate) mod tests {
#[tokio::test(flavor = "multi_thread")]
async fn test_two_devices_setup_teardown() -> Result<()> {
setup_multithreaded_logging();
let devices = Devices {
stun_ip: "127.0.0.1".parse()?,
};

for _ in 0..10 {
let (derp_map, _, _cleanup) = run_derp_and_stun(devices.stun_ip).await?;
let (derp_map, _, _cleanup) = run_derper().await?;
println!("setting up magic stack");
let m1 = MagicStack::new(derp_map.clone()).await?;
let m2 = MagicStack::new(derp_map.clone()).await?;
Expand Down
1 change: 0 additions & 1 deletion iroh-net/src/netcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,6 @@ mod tests {
stun_port,
ipv4: UseIpv4::TryDns,
ipv6: UseIpv6::TryDns,
stun_test_ip: None,
})
.map(Arc::new)
.collect(),
Expand Down
10 changes: 0 additions & 10 deletions iroh-net/src/netcheck/reportgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -949,16 +949,6 @@ async fn get_derp_addr(n: &DerpNode, proto: ProbeProto) -> Result<SocketAddr> {
if port == 0 {
port = DEFAULT_DERP_STUN_PORT;
}
if let Some(ip) = n.stun_test_ip {
if proto == ProbeProto::StunIpv4 && ip.is_ipv6() {
bail!("STUN test IP set has mismatching protocol");
}
if proto == ProbeProto::StunIpv6 && ip.is_ipv4() {
bail!("STUN test IP set has mismatching protocol");
}
return Ok(SocketAddr::new(ip, port));
}

match proto {
ProbeProto::StunIpv4 => {
if let UseIpv4::Some(ip) = n.ipv4 {
Expand Down
1 change: 0 additions & 1 deletion iroh-net/src/stun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ pub mod test {
ipv6,
stun_port: port,
stun_only: true,
stun_test_ip: None,
};
DerpRegion {
region_id,
Expand Down
9 changes: 2 additions & 7 deletions iroh-net/src/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//! Internal utilities to support testing.
use std::net::IpAddr;

use anyhow::Result;
use tokio::sync::oneshot;
use tracing::{info_span, Instrument};
Expand All @@ -22,9 +20,7 @@ pub(crate) struct CleanupDropGuard(pub(crate) oneshot::Sender<()>);
/// is always `Some` as that is how the [`MagicEndpoint::connect`] API expects it.
///
/// [`MagicEndpoint::connect`]: crate::magic_endpoint::MagicEndpoint
pub(crate) async fn run_derp_and_stun(
stun_ip: IpAddr,
) -> Result<(DerpMap, Option<u16>, CleanupDropGuard)> {
pub(crate) async fn run_derper() -> Result<(DerpMap, Option<u16>, CleanupDropGuard)> {
// TODO: pass a mesh_key?

let server_key = SecretKey::generate();
Expand All @@ -38,7 +34,7 @@ pub(crate) async fn run_derp_and_stun(
let https_addr = server.addr();
println!("DERP listening on {:?}", https_addr);

let (stun_addr, _, stun_drop_guard) = crate::stun::test::serve(stun_ip).await?;
let (stun_addr, _, stun_drop_guard) = crate::stun::test::serve(server.addr().ip()).await?;
let region_id = 1;
let m = DerpMap::from_regions([DerpRegion {
region_id,
Expand All @@ -55,7 +51,6 @@ pub(crate) async fn run_derp_and_stun(
stun_port: stun_addr.port(),
ipv4: UseIpv4::Some("127.0.0.1".parse().unwrap()),
ipv6: UseIpv6::Disabled,
stun_test_ip: Some(stun_addr.ip()),
}
.into()],
avoid: false,
Expand Down

0 comments on commit 4ef3611

Please sign in to comment.