feat: QUIC Address discovery extension #2043

divagant-martian · 2024-11-14T16:13:32Z

Implements the Quic Address discovery extension IETF draft

Transport parameters are implemented with tests and 0rtt verification as spec'd
The spec declares two frames, one for informing of IpV4 addresses and one for informing IpV6 frames. This is implemented as a single structure with tests.
Transmission is done after the initial path has been established, when path validation is required, and sent with path responses. The last part is not required but provides a small reconfirmation of the observed external address to the peer, and it's allowed by the spec being observed address frames defined as probing frames. This is implemented with tests.
Retransmission is done by sending a fresh frame if it's identified as lost. This ensures that the path over which the frame is sent is always the correct one, i.e. That the reported observed address is always sent using the path for which the address was observed. This is implemented with tests.
Surfacing is done from the protocol implementation via Connection events.
Configuration is done with two methods to independently turn on or off the receiving and sending of reports.

Ralith

Thanks for working on this!

Ralith · 2024-11-22T20:23:29Z

quinn/src/connection.rs

@@ -974,6 +981,8 @@ pub(crate) struct State {
    send_buffer: Vec<u8>,
    /// We buffer a transmit when the underlying I/O would block
    buffered_transmit: Option<proto::Transmit>,
+    /// Our last external address reported by the peer.
+    pub(crate) observed_external_addr: watch::Sender<Option<SocketAddr>>,


Why is pub(crate) necessary here?

Ralith · 2024-11-22T20:26:06Z

quinn/src/connection.rs

@@ -636,6 +636,12 @@ impl Connection {
        // May need to send MAX_STREAMS to make progress
        conn.wake();
    }
+
+    /// Track changed on our external address as reported by the peer.
+    pub fn observed_external_addr(&self) -> watch::Receiver<Option<SocketAddr>> {


Explicitly document when this is None, whether it might contain spurious updates, and the influence of configuration and peer support.

Not sure about exposing the watch::Receiver as part of the public API, that seems like it's leaking a bunch of implementation details?

Ralith · 2024-11-22T20:28:49Z

quinn/Cargo.toml

@@ -68,6 +68,7 @@ tokio = { workspace = true, features = ["rt", "rt-multi-thread", "time", "macros
 tracing-subscriber = { workspace = true }
 tracing-futures = { workspace = true }
 url = { workspace = true }
+tokio-stream = "0.1.15"


Where is this used?

Ralith · 2024-11-22T20:33:52Z

quinn-proto/src/tests/mod.rs

+    assert_matches!(conn.poll(), Some(Event::HandshakeDataReady));
+    assert_matches!(
+        conn.poll(),
+        Some(Event::ConnectionLost { reason }) if matches!(reason, ConnectionError::TransportError(_) )


style nit: Why not Some(Event::ConnectionLost { reason: ConnectionError::TransportError(_) })?

Ralith · 2024-11-22T20:34:44Z

quinn-proto/src/tests/mod.rs

+/// NOTE: this test is the same as zero_rtt_happypath, changing client transport parameters on
+/// resumption.
+#[test]
+fn address_discovery_zero_rtt_accepted() {


What additional coverage does this case supply? I don't think changing client transport parameters are ever a concern for 0-RTT.

Ralith · 2024-11-22T21:28:23Z

quinn-proto/src/frame.rs

+
+/// Conjuction of the information contained in the address discovery frames
+/// ([`FrameType::OBSERVED_IPV4_ADDR`], [`FrameType::OBSERVED_IPV6_ADDR`]).
+#[derive(Debug, PartialEq, Eq, Clone)]


Suggested change

#[derive(Debug, PartialEq, Eq, Clone)]

#[derive(Debug, PartialEq, Eq, Copy, Clone)]

Ralith · 2024-11-22T21:29:15Z

quinn-proto/src/connection/mod.rs

@@ -3760,6 +3874,8 @@ pub enum Event {
    DatagramReceived,
    /// One or more application datagrams have been sent after blocking
    DatagramsUnblocked,
+    /// Received an observation of our external address from the peer.


Explicitly document whether this is necessarily different from the previous one, and the required configuration and peer support.

Ralith · 2024-11-22T21:34:53Z

quinn-proto/src/connection/mod.rs

+                        // include in migration
+                        migration_observed_addr = Some(observed)


Why is this a special case?

Ralith · 2024-11-22T21:37:44Z

quinn-proto/src/frame.rs

+}
+
+impl ObservedAddr {
+    pub(crate) fn new<N: Into<VarInt>>(remote: std::net::SocketAddr, seq_no: N) -> Self {


Do we ever want to pass something that isn't already a VarInt in here? If not, skip the generic for simplicity.

Also, import SocketAddr instead of qualifying it.

Ralith · 2024-11-22T21:39:37Z

quinn-proto/src/address_discovery.rs

+///
+/// When enabled, this is reported as a transport parameter.
+#[derive(PartialEq, Eq, Clone, Copy, Debug, Default)]
+pub(crate) enum Role {


It seems like we go to a lot of trouble to convert this to and from a pair of bools. Would it it be simpler to represent it that way in the first place?

I also don't love the Role name for something arguably pretty niche, should we make the type name AddressDiscoveryRole instead? Or maybe with the bool fields it's no longer really a Role, either?

djc · 2024-11-25T09:38:51Z

quinn-proto/src/address_discovery.rs

+
+use crate::VarInt;
+
+pub(crate) const TRANSPORT_PARAMETER_CODE: u64 = 0x9f81a176;


Nit: constants last in the file, please.

djc · 2024-11-25T09:55:40Z

quinn-proto/src/address_discovery.rs

+///
+/// When enabled, this is reported as a transport parameter.
+#[derive(PartialEq, Eq, Clone, Copy, Debug, Default)]
+pub(crate) enum Role {


I also don't love the Role name for something arguably pretty niche, should we make the type name AddressDiscoveryRole instead? Or maybe with the bool fields it's no longer really a Role, either?

djc · 2024-11-25T09:57:44Z

quinn-proto/src/connection/mod.rs

+                    {
+                        return Err(TransportError::PROTOCOL_VIOLATION(
+                            "received OBSERVED_ADDRESS frame when not negotiated",
+                        ));


Could we yield this error from within should_report() instead? (Maybe also pass in packet.header.space()?)

djc · 2024-11-25T10:01:11Z

quinn-proto/src/connection/mod.rs

+        new_path.last_observed_addr_report = self.path.last_observed_addr_report.clone();
+        if let Some(report) = observed_addr {
+            if let Some(updated) = new_path.update_observed_addr_report(report) {
+                self.events.push_back(Event::ObservedAddr(updated));
+            }
+        }


Can we push all/most of this logic into a method on PathData?

djc · 2024-11-25T10:05:13Z

quinn-proto/src/frame.rs

+}
+
+impl ObservedAddr {
+    pub(crate) fn new<N: Into<VarInt>>(remote: std::net::SocketAddr, seq_no: N) -> Self {


Also, import SocketAddr instead of qualifying it.

djc · 2024-11-25T10:06:52Z

quinn-proto/src/frame.rs

+        let seq_no = bytes.get()?;
+        let ip = if is_ipv6 {
+            IpAddr::V6(bytes.get()?)
+        } else {
+            IpAddr::V4(bytes.get()?)
+        };
+        let port = bytes.get()?;
+        Ok(Self { seq_no, ip, port })


Ok(Self { seq_no: bytes.get()?, ip: match is_ipv6 { true => IpAddr::V6(bytes.get()?), false => IpAddr::V4(bytes.get()?), }, port: bytes.get()?, })

djc · 2024-11-25T10:08:09Z

quinn-proto/src/transport_parameters.rs

+                    if !params.address_discovery_role.is_disabled() {
+                        // duplicate parameter
+                        return Err(Error::Malformed);
+                    }
+                    let value: VarInt = r.get()?;
+                    if len != value.size() {
+                        return Err(Error::Malformed);
+                    }
+                    params.address_discovery_role = value.try_into()?;
+                    tracing::debug!(
+                        role = ?params.address_discovery_role,
+                        "address discovery enabled for peer"
+                    );


Suggest rolling this up into a Role::from_parameter() (name TBD) constructor.

djc · 2024-11-25T10:08:51Z

quinn/examples/client.rs

+    let mut external_addresses = conn.observed_external_addr();
+    tokio::spawn(async move {
+        loop {
+            if let Some(new_addr) = *external_addresses.borrow_and_update() {
+                info!(%new_addr, "new external address report");
+            }
+            if external_addresses.changed().await.is_err() {
+                break;
+            }
+        }
+    });


I don't think this is widely used enough that it merits real estate in these examples.

djc · 2024-11-25T10:10:25Z

quinn/src/connection.rs

@@ -636,6 +636,12 @@ impl Connection {
        // May need to send MAX_STREAMS to make progress
        conn.wake();
    }
+
+    /// Track changed on our external address as reported by the peer.
+    pub fn observed_external_addr(&self) -> watch::Receiver<Option<SocketAddr>> {


Not sure about exposing the watch::Receiver as part of the public API, that seems like it's leaking a bunch of implementation details?

divagant-martian force-pushed the quic-address-discovery branch from 276c164 to b0f0a30 Compare November 14, 2024 16:24

divagant-martian force-pushed the quic-address-discovery branch from eaa83bc to 5c0e633 Compare November 21, 2024 17:26

divagant-martian marked this pull request as ready for review November 21, 2024 17:26

divagant-martian added 7 commits November 22, 2024 10:31

Define address discovery roles present in the extension negotiation

d5de5c2

add ObserevedAdd frame and frame types

b3de82c

add transport parameter for address discovery

a87c61c

Add saturating add for Varint

112f8ee

Add transmission and retransmission logic

05b3f6a

add tests

2a4891b

use in example

60b2e57

divagant-martian force-pushed the quic-address-discovery branch from 5c0e633 to 60b2e57 Compare November 22, 2024 15:33

Ralith requested changes Nov 22, 2024

View reviewed changes

djc reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: QUIC Address discovery extension #2043

feat: QUIC Address discovery extension #2043

divagant-martian commented Nov 14, 2024 •

edited

Loading

Ralith left a comment

Ralith Nov 22, 2024

Ralith Nov 22, 2024

djc Nov 25, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

Ralith Nov 22, 2024

djc Nov 25, 2024

Ralith Nov 22, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

djc Nov 25, 2024

	#[derive(Debug, PartialEq, Eq, Clone)]
	#[derive(Debug, PartialEq, Eq, Copy, Clone)]

		// include in migration
		migration_observed_addr = Some(observed)


		use crate::VarInt;

		pub(crate) const TRANSPORT_PARAMETER_CODE: u64 = 0x9f81a176;

feat: QUIC Address discovery extension #2043

Are you sure you want to change the base?

feat: QUIC Address discovery extension #2043

Conversation

divagant-martian commented Nov 14, 2024 • edited Loading

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divagant-martian commented Nov 14, 2024 •

edited

Loading