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

proposal: net/http/httptest: listen on IPv6/IPv4-agnostic loopback address #56998

Open
aktau opened this issue Nov 30, 2022 · 9 comments
Open
Labels
Milestone

Comments

@aktau
Copy link
Contributor

aktau commented Nov 30, 2022

Related:

Currently (1.20-dev), net/http/httptest listens preferentially on 127:0:0:1:0, and falls back to [::1]:0 [1]. At work, our (modified) Linux kernel/system is planned to start behaving like this:

  1. net.Listen("tcp", "127.0.0.1:0") succeeds (imagine it picked port 9999).
  2. net.Dial("tcp", "localhost:9999") resolves to [::1]:9999.

This causes tests to fail, as httptest.NewServer only registers on IPv4 in this case. Given that httptest is much more limited in scope than the net.Listener (#9334), it may be feasible to change httptest behaviour to listen on :0 so that both IPv4 and IPv6 are listened on.

The current behaviour in package net/http/httptest stems from f88abda (2011), it was copied as-is from the erstwhile fs_test aa9c213. I couldn't really find a reasoning for why we explicitly listen on 127.0.0.1, so I thought I'd propose this and see what I missed.

[1]:

func newLocalListener() net.Listener {
if serveFlag != "" {
l, err := net.Listen("tcp", serveFlag)
if err != nil {
panic(fmt.Sprintf("httptest: failed to listen on %v: %v", serveFlag, err))
}
return l
}
l, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
if l, err = net.Listen("tcp6", "[::1]:0"); err != nil {
panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err))
}
}
return l
}

@aktau aktau added the Proposal label Nov 30, 2022
@gopherbot gopherbot added this to the Proposal milestone Nov 30, 2022
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@seankhliao
Copy link
Member

See also #31054 (comment)
which states that you should only be using the URL returned by the server.

@aktau
Copy link
Contributor Author

aktau commented Nov 30, 2022

See also #31054 (comment) which states that you should only be using the URL returned by the server.

That's a good point. I've inquired about this internally and it appears there are a number of tests which don't do this. Now I need to figure out whether there are situations where not using URL is legitimate or at least hard to avoid.

@neild
Copy link
Contributor

neild commented Nov 30, 2022

How are these tests finding an address to connect to? Can you give an example of how this would be used?

I don't think listening on :0 by default works; you don't want your httptest server to be exposed to the external network.

@aktau
Copy link
Contributor Author

aktau commented Dec 2, 2022

I've pointed the engineer handling this problem to this issue to engage.

@xuanya0
Copy link

xuanya0 commented Dec 11, 2022

@neild responsible engineer here, I see a very common pattern as follows:

client retrieves port and then try to connect “localhost:retrieved_port". this is rather common when server & client are separate executables and the client would simple have an arg —port= and assume/default hostname to be “localhost”

@neild
Copy link
Contributor

neild commented Dec 11, 2022

It's more work to just get a port out of httptest.Server than it is to get a base URL (ts.URL) or an addrport (ts.Listener.Addr()), either of which will work properly. Extracting a port and assuming the server is listening on loopback:$PORT seems like more effort to get a worse result.

That said, I don't see anything fundamentally wrong with httptest.Server listening on multiple loopback interfaces when available. It absolutely must not listen on non-loopback interfaces, however, so listening on :0 isn't an option, and the alternatives seem like a lot of complexity to support a rare use case. (If #9334 were resolved, making it easier to listen on multiple interfaces at the same time, the change would be simple, of course.)

@xuanya0
Copy link

xuanya0 commented Dec 16, 2022

just a second thought, is it possible to add 2 new funcs to this library, namely, NewServerV6() & NewTLSServerV6() that explicitly try [::1] and just panic if failed?

@perj
Copy link

perj commented Mar 20, 2023

It seems very strange to me to not have net.Dial("tcp", "localhost:9999") not try both IPv6 and IPv4. It's explicitly using tcp, which means either tcp4 or tcp6 and it's explicitly using localhost which resolves to both ::1 and 127.0.0.1. It seems to me like the fix should be on the client side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

7 participants