Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: move IpMappedAddresses to DnsResolver #3220

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Mar 9, 2025

Description

In order to facilitate QUIC address discovery inside of iroh, we needed a way to map resolved URLs to an IpMappedAddr. Previously, we did that by passing in an optional IpMappedAddresses struct into the iroh-net-report.

Now that we have an iroh DnsResolver, we can do the mapping inside of DnsResolver, when we know we are resolving a URL that needs mapping.

IpMappedAddr, IpMappedAddresses, etc, have been moved from iroh-net-report to iroh-relay where the DnsResolver is defined.

DnsResolver has an optional IpMappedAddresses field.

Now, when iroh-net-report resolves a relay-url that we know will be used in QAD, we call the new DnsResolver::lookup_relay_node_ipv4 and DnsResolver::lookup_relay_node_ipv6, and it will return the associated socket address for the IpMappedAddr.

Breaking Changes

  • iroh-net-report
    • changed
      • iroh-net-report::Client::new no longer contains the mapped_addrs parameter

Notes & open questions

Small PR, but questions around naming.

Should DnsResolver::lookup_relay_node_ipvX be more explicit? DnsResolver::lookup_relay_node_ipvX_mapped? Should it return a IpMappedAddr instead of a SocketAddr to be more explicit?

Not sure, from a user perspective, what the cleanest/least confusing API would be.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:

Copy link

github-actions bot commented Mar 9, 2025

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: dbb5515

@ramfox ramfox force-pushed the ramfox/dns-mapped-addrs branch from 564f1c9 to d0be662 Compare March 12, 2025 20:40
@ramfox ramfox changed the base branch from main to backon March 12, 2025 20:40
Copy link

github-actions bot commented Mar 12, 2025

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

Last updated: 2025-03-14T02:38:27Z

@matheus23
Copy link
Member

matheus23 commented Mar 13, 2025

I had a look at this initially, but it didn't really feel right for me to review this, as I mainly wonder why we'd be doing this? 😬

In my mind, the DnsResolver should eventually evolve into a trait, making it possible to use dns resolving in any way you want, e.g. always using DoH or DoT to hide DNS traffic.
And I see the IpMappedAddrs as an implementation detail of iroh, and it's not used in iroh-relay code at all, so I wouldn't really like to put it into the iroh-relay crate.

There are probably good reasons for doing this though! I just need to be made aware 😅

Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the relay URL is http://127.0.0.1:1234? I mean, when it directly contains an IP address? Could we have a test for that?

I can see the elegance of wrapping it into the resolver. But I agree that it feels a bit weird as well. Additionally the iroh-net-report crate didn't have to reach the 1.0 stability guarantees, while it living in iroh-relay changes that completely and now this IP mapped address stuff lives in a place where we'll need to give it API stability. Which I don't like.

It's also tricky because... maybe it'll go away with multipath. I think we might end up in a world where all the IP addresses that Quinn sees are directly the real IP addresses and only the relay paths would be mapped addresses. I don't yet know exactly how this will look, but something like that.

timeout: Duration,
delays_ms: &[u64],
port: u16,
) -> Result<SocketAddr> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mention these two points in the notes of the PR:

  • For name I think "relay" doesn't say anything. Maybe lookup_node_ipv4_and_map would be better.
  • Definitely return an IpMappedAddr instead of a SocketAddr.

@ramfox ramfox requested a review from Frando March 13, 2025 17:55
Base automatically changed from backon to main March 14, 2025 00:43
@ramfox ramfox force-pushed the ramfox/dns-mapped-addrs branch from d0be662 to 6ed071a Compare March 14, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants