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

Remove inconsistent address resolution #114

Merged
merged 3 commits into from
May 19, 2023

Conversation

PetrichorIT
Copy link
Contributor

There is an inconsistency in address resolution / parsing.
The trait ToIpAddr interprets strings that can be parsed as an IpAddr as
domain names instead of parsing the address. ToSocketAddr however will
parse the Ip part of the address if possible. To remove this inconsistency
ToIpAddr will now always interpret strings that can be parse to a IpAddr as IpAddr.

Example

Without this change:

use turmoil::{*, net::*};

fn main() -> Result {
    let mut sim = Builder::new().build();
    sim.client("192.168.42.42", async move {
        let addr = UdpSocket::bind("0.0.0.0:0").await.unwrap().local_addr().unwrap();
        // addr will be 192.168.0.1:<port>, interpreting the clients name as a domain addr
        Ok(())
    });
    sim.client("192.168.57.57".parse::<IpAddr>().unwrap(), async move {
        let addr = UdpSocket::bind("0.0.0.0:0").await.unwrap().local_addr().unwrap();
        // addr will be 192.168.57.57:<port>, respecting the provided parameter
        Ok(())
    });
   
   // The problem is even more pronounced with lookups
   sim.client("x", async move {
        // lookups will use the ip->ip dns entry
        assert_eq!(lookup("192.168.42.42"), Ipv4Addr::new(192,168,0,1));
       
       // ToSocketAddr however not use dns but rather parse the raw ip part
       // Thus this will panic, since the link `x->192.168.42.42`does not exits
       let stream = TcpStream::connect("192.168.42.42:80").await?;
       Ok(()) 
   });

  sim.run()
}

On a site note, ToIpAddr was not implemented for Ipv4Addr and Ipv6Addr which
was problematic since APIs with impl ToIpAddr could not use .into() due to missing
type inference. Same goes for ToSocketAddr with SocketAddrV4 and SocketAddrV6 respectivly
(Tokio's ToSocketAddr does is implemented for these types)

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

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

Thanks for making the Addrs more complete!

src/dns.rs Outdated Show resolved Hide resolved
src/dns.rs Outdated Show resolved Hide resolved
@mcches mcches merged commit 1971d60 into tokio-rs:main May 19, 2023
3 checks passed
@PetrichorIT PetrichorIT deleted the addr-resolution-consistency branch July 27, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants