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

Tracking issue for DNS in std::net #27705

Closed
alexcrichton opened this Issue Aug 12, 2015 · 41 comments

Comments

Projects
None yet
@alexcrichton
Copy link
Member

alexcrichton commented Aug 12, 2015

This is a tracking issue for the lookup_addr and lookup_host unstable features in the standard library. These are currently the only exposed support for DNS as a public interface. Functions like TcpStream::connect already expose the ability to resolve names via passing a string, but this cannot be done programmatically currently.

There are a number of thorny issues to deal with here:

  • Is calling getaddrinfo and getnameinfo here appropriate?
  • What should the arguments be? Should this use a builder?
  • In the getaddrinfo case, should IP addresses or socket addresses be returned?
  • In the getnameinfo case, should IP addresses or socket addresses be taken?
  • What should the return values look like? Iterators? Structs exposing accessors? (like DirEntry)
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 7, 2015

I’d like to impl ToSocketAddrs for Url in rust-url. The URL parser already differentiates DNS names from IP addresses: https://url.spec.whatwg.org/#host-parsing. (And the algorithm is slightly different from Ipv{4,6}Addr::parse in some edge cases.) So I’d like some way to look up a DNS name. ToSocketAddrs for str does this, but it also attempts to parse an IP address, which is redundant.

I think a guiding principle in some other parts of std has been to (also) expose what the platform (~ OS + libc) provides without mandatory additional behavior on top, and stabilizing lookup_host fits in that principle. (It turns out that getaddrinfo in some libc’s also attempts to parse IP addresses, but that’s another story…)


I think the return value of lookup_host should be (an iterator of) IP addresses, not SocketAddr with an arbitrary port number. This could be a reason to bring back IpAddr #27801

@aturon aturon added the I-nominated label Dec 16, 2015

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 16, 2015

@SimonSapin proposes stabilization for this cycle.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Dec 17, 2015

The libs team discussed this during triage yesterday and the conclusion was to not move this into FCP just yet. I personally have a number of concerns about the API exposed here in that I think it's insufficient for exporting all the functionality we want and it's also insufficient for giving back all the information that we get.

I would be interested in seeing this developed externally on crates.io and then perhaps move it into the standard library. For now the libstd support won't be deprecated until a replacement externally exists, but I may work on such a replacement in the near future if no one else gets around to it!

@canndrew

This comment has been minimized.

Copy link
Contributor

canndrew commented Dec 23, 2015

+1 to everything @SimonSapin said. DNS is a mapping of names to IP addresses, not IP addresses+ports, so SocketAddr is the wrong type here. It's shame that IpAddr was removed for "not pulling it's weight" despite people speaking up and saying they were using it. How much can something like that weigh?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 11, 2016

Should lookup_addr have been un-deprecated along with IpAddr? (#31438)

@photino

This comment has been minimized.

Copy link

photino commented Mar 10, 2016

What is the decision on lookup_addr?

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 10, 2016

Whoops, missed @SimonSapin's comment. cc @rust-lang/libs -- the question is, given that we have stabilized IpAddr, what should happen to lookup_addr?

@aturon aturon added the I-nominated label Mar 10, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Mar 10, 2016

Given that we haven't stabilized lookup_host I don't think we should stabilize lookup_addr, it's basically the same set of problems.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 10, 2016

I’m suggesting to remove #[deprecated] but keep #[unstable] in order to signal "this API has issues or questions we need to resolve before it can be stabilized" rather than "this API is on its way to be removed."

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 10, 2016

Regardless of the state of IpAddr, I believe our feelings were that DNS lookup should leave the standard library for some external crate so the design can shake out.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 10, 2016

DNS lookup should leave the standard library

As we’ve discussed before, this can’t really happen since impl ToSocketAddrs for str and impl<'a> ToSocketAddrs for (&'a str, u16) rely on DNS lookup and are #[stable]. So removing lookup_host would only be removing one bit of API surface, the underlying infrastructure needs to stay. I find hypocritical to call this "removing DNS support".

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 10, 2016

Fine, "I believe our feelings where that lookup_host as a public API should leave the standard library for some external crate so that the design can shake out."

@troplin

This comment has been minimized.

Copy link
Contributor

troplin commented Mar 23, 2016

lookup_host is now undeprecated but lookup_addr was removed. Is that intentional? And if yes, why?

@keeperofdakeys

This comment has been minimized.

Copy link
Contributor

keeperofdakeys commented Mar 24, 2016

I wanted to use lookup_host in something I was writing, so I went to the effort of pulling out all the relevant code, and making it use the libc crate (so it works on stable) - https://crates.io/crates/dns-lookup. It requires a bit of work to improve it though (documentation, testing, refactoring). It's still using SocketAddrs, but since IpAddr has been un-deprecated, the first change should probably be using IpAddr instead.

@jfager

This comment has been minimized.

Copy link
Contributor

jfager commented Mar 25, 2016

lookup_addr died in #32112, would have been nice to ping the tracking issue. @sfackler's comments left me w/ the impression that there'd be an official-ish external crate stood up for this functionality, guess not?

@murarth

This comment has been minimized.

Copy link
Contributor

murarth commented Mar 25, 2016

I feel that the death of lookup_addr probably is justified. I seemed to be the only one who had use for it (I added it to std::net myself -- twice) and I've since realized that I need something more sophisticated and amenable to asynchronous requests.

As a result, I've created the resolve crate, which is implemented in pure Rust. It's not as complete as it could be -- asynchronous functionality is not supplied, but can be built on top. resolve is currently unstable-only, but will be available to stable users when 1.8.0 arrives.

@troplin

This comment has been minimized.

Copy link
Contributor

troplin commented Mar 25, 2016

I'm using it, so we're at least two. Well, at least I was, until it was removed.

Sure, lookup_host is more important that lookup_addr but I feel that if one is provided, the other should follow. They always appear in pairs, e.g. gethostbyname/gethostbyaddr or getaddrinfo/getnameinfo in C.

Also, when it comes to DNS, I really want to use the system/libc functions, not a pure Rust solution (because of caching, NSS, system settings ...).

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 28, 2016

There hasn't been a lot of clear communication from the libs team on these APIs; sorry about that. The TL;DR is that no one is comfortable stabilizing these APIs as-is, and we prefer when possible to prototype and gain experience with this kind of thing outside of std first.

Luckily, for networking in particular, we already have a crate for this exact purpose: net2. It's a kind of staging ground for networking API experiments that are ultimately on track for std. @alexcrichton and I propose moving the lookup_* methods there for the time being, and starting to work out answers to the questions posed at the top of this thread.

@troplin

This comment has been minimized.

Copy link
Contributor

troplin commented Mar 28, 2016

That sounds good to me. I'm already using net2 anyway.

@nodakai

This comment has been minimized.

Copy link
Contributor

nodakai commented Apr 3, 2016

Note that getaddrinfo(3) in general returns a list of concrete socket addresses

@troplin

This comment has been minimized.

Copy link
Contributor

troplin commented Apr 3, 2016

@nodakai I don't understand the emphasis on list. lookup_host already returns an iterator of IpAddrs, each of which can be either V4 or V6.

That said, getaddrinfo also looks up the "service name" (http, ftp, etc.) and returns the corresponding port. Is this what you meant, that the lookup should return a SocketAddr instead of IpAddr?

@keeperofdakeys

This comment has been minimized.

Copy link
Contributor

keeperofdakeys commented Apr 3, 2016

The getaddrinfo systemcall can also give information about other protocols, like unix sockets. It also incorporates a query interface for what kinds of sockets you'd like. I'm building a simple wrapper for getaddrinfo to play with in the dns-lookup repo I made, and I might submit it as a pr to net2 if it's useful.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Apr 29, 2016

The libs team discussed this at triage the other day, and the decision was to not move this issue to FCP yet but instead follow the strategy @aturon proposed earlier, which is to prototype these APIs in net2 first.

@kevinburke

This comment has been minimized.

Copy link

kevinburke commented Oct 25, 2016

Apologies if this is the wrong place... recently I tried to get github.com/kevinburke/dnstimeout up and running again and ran into

error: use of unstable library feature 'lookup_host': unsure about the returned iterator and returning socket addresses (see issue #27705)
  --> src/lib.rs:10:77
   |
10 | pub fn lookup(host: String, timeout_duration: time::Duration) -> io::Result<net::LookupHost> {

If I am reading correctly, there is no current way to fix this issue (do DNS lookups) using Rust stable? Without using glibc or friends..

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 25, 2016

@kevinburke You can use the ToSocketAddrs trait with a dummy port number like this:

use std::net::ToSocketAddrs;
use std::io;

fn resolve(host: &str) -> io::Result<Vec<IpAddr>> {
    (s, 0).to_socket_addrs().map(|iter| iter.map(|socket_address| socket_address.ip()).collect())
}

… but this API does not support timeouts.

@fweimer

This comment has been minimized.

Copy link

fweimer commented Jan 7, 2017

getaddrinfo, due to its use of socket addresses, also provides scope IDs for IPv6 addresses. These are required for using IPv6 link-local addresses. This bug describes some of the consequences getting this wrong (from a getaddrinfo implementation perspective):

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 29, 2017

The story here is pretty clearly settled: you can use the ToSocketAddr trait to convert from a string to get basic DNS. Full-blown DNS should live outside of std.

@rfcbot fcp close

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 29, 2017

Team member @aturon has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@dimbleby

This comment has been minimized.

Copy link

dimbleby commented Aug 29, 2017

The story here is pretty clearly settled: you can use the ToSocketAddr trait to convert from a string to get basic DNS. Full-blown DNS should live outside of std.

I see two possible justifications for settling for this: one disappointing-but-reasonable, one surely wrong. I'd like to check which (if either) we have in mind when closing this.

The 'disappointing-but-reasonable' position is: well, that's where we are. It's true that the ToSocketAddr backdoor is unsatisfactory but we can't remove it because back-compatibility, so let's at least not make things worse.

The 'surely wrong' position is: this is actually where we'd want to be, and all is well with the world. The current ToSocketAddrs trait is a good API for DNS resolution and totally deserves its place as the unique way to do DNS resolution in std.

I suspect that the first position is what is being proposed, in which case fair enough. But if it's the second, please leave a note so that I can come back and say why I think that's surely wrong and should be revisited!

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Aug 29, 2017

Neither of those options are the justification:

The ToSocketAddrs implementation is nice for convenient use of sockets. It does what you'd expect most of the time.

There is a place for a more robust and featureful DNS API in the standard library, but no one has cared about it enough to actually do any of that design or implementation work.

@dimbleby

This comment has been minimized.

Copy link

dimbleby commented Aug 29, 2017

There is a place for a more robust and featureful DNS API in the standard library, but no one has cared about it enough to actually do any of that design or implementation work.

Isn't that saying exactly that the story is not yet settled? I mean that's also a reasonable position, but it seems to argue for keeping this issue open rather than for closing it.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Aug 30, 2017

The story of the specific functions for which this is a tracking issue is proposed to be settled. Feature requests tend to live on the rfcs repo.

@fweimer

This comment has been minimized.

Copy link

fweimer commented Aug 30, 2017

I find the terminology here confusing. A lot of programmers need host name lookup, and they need to resolve host names according to the system configuration, which usually includes more than just DNS, and can end up performing exotics such as LDAP or NIS lookups. These kinds of lookups are limited in the types of data they can process (basically, just names, addresses, and, to some degree, host aliases).

DNS, in contrast, offers a much wider range of record types, and a DNS API is required to get anything out of DNS which does not involve address information. But some names users expect to resolve on a system will not be resolvable through DNS, so a DNS API is not a replacement for something that ends up calling getaddrinfo and related functions.

@dimbleby

This comment has been minimized.

Copy link

dimbleby commented Aug 30, 2017

So, if I may so so, the communication has not been entirely clear. But I think that the reasoning for closing this one is some combination of the 'disappointing-but-reasonable' position - essentially, that we are where we are and can't do much about that - and that: as a matter of policy, proposals to make things better no longer belong in this repository but instead should these days be tracked by RFCs. Right?

Seems OK to me.

(Though it's a bit annoying: I'm already watching this issue, and instead I am now expected to watch future and as yet non-existent proposals somewhere else...)

@fweimer I can't tell whether you're in favour of closing this issue, or doing something else, or what. Care to expand?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Sep 12, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Sep 17, 2017

Alright now that we've signed off, I'm going to close as per the review above.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Sep 17, 2017

Er actually, forgot that we still need to mark the apis deprecated..

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 16, 2018

lookup_addr is gone, but std::net::lookup_host is still there and not deprecated. Should this be re-opened?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 16, 2018

The intention was to get rid of all of it, so I think we'd just deprecate that as well.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 16, 2018

That’s fine, it just looks like one function slipped through the cracks. I meant re-open so we don’t forget it.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 17, 2018

Fixed in #47510.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.