feat(net-report)!: add net_report::Options to specify which probes you want to run

[ramfox]

Description

Breaking change that allows you to specify which probe protocols you would like to run.

Really written so it is easier to test QUIC address discovery on iroh doctor, but this also allows us to test specific protocols in isolation.

Breaking Changes

• iroh-net-report
  • added
    • net_report::Client::get_report_with_options
  • changed
    • net_report::Client::get_report_channel now takes an opts: net_report::Options
    • net_report::Client will no longer bind UdpSockets when one is not provided for both STUN over IPv4 or STUN over IPv6.

Notes & open questions

I'm open to adjustments on the Options. The whole "we build a UDP socket for you if one isn't provided" bit means that I couldn't rely on, for example, stun_sock_v4 == None to know whether the user wanted to run a stun ipv4 probe, so I needed the protocols: BTreeSet<ProbeProtocol> field. I think this is a decent compromise between annoying and useful, but would love feedback.

Edit:
Adjusted this so that the Options DO correlate to the protocols we want to run. This means that we will no longer bind a UdpSocket for you if one is not provided.

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[github-actions]

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3032/docs/iroh/

Last updated: 2024-12-14T07:13:50Z

[github-actions]

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: ab367d9

[dignifiedquire]

The whole "we build a UDP socket for you if one isn't provided"

I wonder if we should change this

[flub]

little point in another variable is there?

[flub]

I'm not entirely sure why this needs changing. The old version uses let start = *max_stun_delay.get_or_insert_with(|| plan.max_delay()). I would expect plan.max_delay() to return 0 if there were no high-prio probes. So then start should also be 0?

Admittedly I'm just reading code and didn't try this out.

[ramfox]

okay, I definitely changed this in the most confusing way, so refactored and added comments.

Basically, if there are no high priority probes, then we don't need start to also add the DEFAULT_INITIAL_RETRANSMIT, but if it DOES than we want to add that buffer.

Hopefully this is clearer. The issue was that if you don't have any high prio probes in the list of probes you are attempting, then all the probes added in this section started with a 100ms delay.

[flub]

The whole "we build a UDP socket for you if one isn't provided"

I wonder if we should change this

Yes, I agree this is worth trying out. I forgot what the reason for this is, but I think it might have just been for ease of testing, which you're changing here anyway by being able to not have stun probes when testing. And in the MagicSock we'll never need this functionality.

[divagant-martian]

I tried to create an alternative that would look like:
Options::empty().run_stun(probe_ipv4: bool, probe_ipv6: socket: None) and so on for each "type" of protocol to keep the functionality and make it consistent.

But I wonder if we should then just remove it completely. Creating a socket is not that big of a hassle to justify essentially equivalent options to behave so differently/inconsistently among them, and make it consistent across protocols is also (comparatively) a lot more work.

[ramfox]

Okay here is what it looks like with removing the UdpSocket and adjusting the Options accordingly.