Skip to content

Commit

Permalink
test: replace net2 dependency with socket2 (#635)
Browse files Browse the repository at this point in the history
The proxy's integration tests depend on the `net2` crate, which has been
deprecated and replaced by `socket2`. Since `net2` is no longer actively
maintained, `cargo audit` will warn us about it, so we should replace it
with `socket2`.

While I was making this change, I was curious why we were manually
constructing and binding these sockets at all, rather than just using
`tokio::net::TcpListener::bind`. After some archaeology, I determined
that this was added in linkerd/linkerd2#952, which added a test that
requires a delay between when a socket is _bound_ and when it starts
_listening_. `tokio::net::TcpListener::bind` (as well as the `std::net`
version) perform these operations together. Since this wasn't obvious
from the test code, I went ahead and moved the new `socket2` version of
this into a pair of functions, with comments explaining why we didn't
just use `tokio::net`.

Fixes linkerd/linkerd2#4891
  • Loading branch information
hawkw committed Aug 24, 2020
1 parent 487e85e commit fc7ae2d
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 23 deletions.
16 changes: 8 additions & 8 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,9 @@ checksum = "bc5729f27f159ddd61f4df6228e827e86643d4d3e7c32183cb30a1c08f604a14"

[[package]]
name = "libc"
version = "0.2.65"
version = "0.2.76"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1a31a0627fdf1f6a39ec0dd577e101440b7db22672c0901fe00a9a6fbb5c24e8"
checksum = "755456fae044e6fa1ebbbd1b3e902ae19e73097ed4ed87bb79934a867c007bc3"

[[package]]
name = "libmimalloc-sys"
Expand Down Expand Up @@ -775,7 +775,6 @@ dependencies = [
"linkerd2-metrics",
"linkerd2-opencensus",
"linkerd2-proxy-api",
"net2",
"quickcheck",
"regex 1.3.9",
"ring",
Expand Down Expand Up @@ -910,11 +909,11 @@ dependencies = [
"linkerd2-app-core",
"linkerd2-metrics",
"linkerd2-proxy-api",
"net2",
"quickcheck",
"regex 0.1.80",
"ring",
"rustls",
"socket2",
"tokio",
"tokio-rustls",
"tonic",
Expand Down Expand Up @@ -2138,9 +2137,9 @@ dependencies = [

[[package]]
name = "redox_syscall"
version = "0.1.37"
version = "0.1.57"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0d92eecebad22b767915e4d529f89f28ee96dbbf5a4810d2b844373f136417fd"
checksum = "41cc0f7e4d5d4544e8861606a285bb08d3e70712ccc7d2b84d7c0ccfaf4b05ce"

[[package]]
name = "regex"
Expand Down Expand Up @@ -2339,12 +2338,13 @@ checksum = "5c2fb2ec9bcd216a5b0d0ccf31ab17b5ed1d627960edff65bbe95d3ce221cefc"

[[package]]
name = "socket2"
version = "0.3.5"
version = "0.3.12"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ff606e0486e88f5fc6cfeb3966e434fb409abbc7a3ab495238f70a1ca97f789d"
checksum = "03088793f677dce356f3ccc2edb1b314ad191ab702a5de3faf49304f7e104918"
dependencies = [
"cfg-if",
"libc",
"redox_syscall",
"winapi 0.3.8",
]

Expand Down
1 change: 0 additions & 1 deletion linkerd/app/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ http = "0.2"
hyper = "0.13.7"
linkerd2-metrics = { path = "../metrics", features = ["test_util"] }
linkerd2-proxy-api = { git = "https://github.com/linkerd/linkerd2-proxy-api", tag = "v0.1.13", features = ["arbitrary"] }
net2 = "0.2"
quickcheck = { version = "0.9", default-features = false }
ring = "0.16"
rustls = "0.17"
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/integration/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ linkerd2-app-core = { path = "../core", features = ["mock-orig-dst"] }
linkerd2-metrics = { path = "../../metrics", features = ["test_util"] }
linkerd2-proxy-api = { git = "https://github.com/linkerd/linkerd2-proxy-api", tag = "v0.1.13", features = ["arbitrary"] }
regex = "0.1"
net2 = "0.2"
socket2 = "0.3.12"
quickcheck = { version = "0.9", default-features = false }
ring = "0.16"
rustls = "0.17"
Expand Down
13 changes: 6 additions & 7 deletions linkerd/app/integration/src/controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,17 +333,16 @@ where
B::Error: Into<Box<dyn std::error::Error + Send + Sync>>,
B::Data: Send + 'static,
{
let addr = SocketAddr::from(([127, 0, 0, 1], 0));
let listener = net2::TcpBuilder::new_v4().expect("Tcp::new_v4");
listener.bind(addr).expect("Tcp::bind");
let addr = listener.local_addr().expect("Tcp::local_addr");
let listener = listener.listen(1024).expect("listen");

let (listening_tx, listening_rx) = tokio::sync::oneshot::channel();
let (drain_signal, drain) = drain::channel();

// Bind an ephemeral port but do not start listening yet.
let (sock, addr) = crate::bind_ephemeral();

let task = tokio::spawn(
cancelable(drain.clone(), async move {
let mut listener = tokio::net::TcpListener::from_std(listener)?;
// Start listening on the socket.
let mut listener = crate::listen(sock);
let mut listening_tx = Some(listening_tx);

if let Some(delay) = delay {
Expand Down
33 changes: 33 additions & 0 deletions linkerd/app/integration/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub use http::{HeaderMap, Request, Response, StatusCode};
pub use http_body::Body as HttpBody;
pub use linkerd2_app as app;
pub use linkerd2_app_core::drain;
use socket2::Socket;
pub use std::collections::HashMap;
use std::fmt;
pub use std::future::Future;
Expand Down Expand Up @@ -286,3 +287,35 @@ pub async fn cancelable<E: Send + 'static>(
}
}
}

/// Binds a socket to an ephemeral port without starting listening on it.
///
/// Some tests require us to introduce a delay between binding the server
/// socket and listening on it. However, `tokio::net`'s
/// `TcpListener::bind` function both binds the socket _and_ starts
/// listening on it, so we can't just use that. Instead, we must manually
/// construct the socket using `socket2`'s lower-level interface to libc
/// socket APIs, and _then_ convert it into a Tokio `TcpListener`.
pub(crate) fn bind_ephemeral() -> (Socket, SocketAddr) {
use socket2::{Domain, Protocol, Type};

let addr = SocketAddr::from(([127, 0, 0, 1], 0));
let sock =
Socket::new(Domain::ipv4(), Type::stream(), Some(Protocol::tcp())).expect("Socket::new");
sock.bind(&addr.into()).expect("Socket::bind");
let addr = sock
.local_addr()
.expect("Socket::local_addr")
.as_std()
.expect("must be AF_INET");
(sock, addr)
}

/// Start listening on a socket previously bound using `socket2`, returning a
/// Tokio `TcpListener`.
pub(crate) fn listen(sock: Socket) -> TcpListener {
sock.listen(1024)
.expect("socket should be able to start listening");
TcpListener::from_std(sock.into_tcp_listener())
.expect("socket should be able to set nonblocking")
}
11 changes: 5 additions & 6 deletions linkerd/app/integration/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,8 @@ impl Server {
let srv_conn_count = Arc::clone(&conn_count);
let version = self.version;

let addr = SocketAddr::from(([127, 0, 0, 1], 0));
let listener = net2::TcpBuilder::new_v4().expect("Tcp::new_v4");
listener.bind(addr).expect("Tcp::bind");
let addr = listener.local_addr().expect("Tcp::local_addr");
// Bind an ephemeral port but do not start listening yet.
let (sock, addr) = crate::bind_ephemeral();

let (drain_signal, drain) = drain::channel();
let tls_config = self.tls.clone();
Expand All @@ -223,8 +221,9 @@ impl Server {
let _ = listening_tx.take().unwrap().send(());
delay.await;
}
let listener = listener.listen(1024).expect("Tcp::listen");
let mut listener = TcpListener::from_std(listener).expect("from_std");

// After the delay, start listening on the socket.
let mut listener = crate::listen(sock);

if let Some(listening_tx) = listening_tx {
let _ = listening_tx.send(());
Expand Down

0 comments on commit fc7ae2d

Please sign in to comment.