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

net/http: ProxyFromEnvironment should return nil for ip.IsUnspecified #24737

Closed
glasser opened this issue Apr 6, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@glasser
Copy link
Contributor

commented Apr 6, 2018

What version of Go are you using (go version)?

go1.10.1

Does this issue reproduce with the latest release?

Yes, from code inspection the issue exists on master.

What operating system and processor architecture are you using (go env)?

darwin amd64 (shouldn't be specific)

What did you do?

  • Set a broken HTTP_PROXY environment variable
  • Run a web server listening on the unspecified interface (note that I specify tcp4 but this happens with tcp4 or tcp6)
  • Attempt to use the http client to talk to the web server
  • Get an error because it tried to use $HTTP_PROXY

https://play.golang.org/p/zVHDyxwZREM

What did you expect to see?

I expected to see the same result as if there was no $HTTP_PROXY, because $HTTP_PROXY is ignored for loopback IPs:

2009/11/10 23:00:00 0.0.0.0:2
2009/11/10 23:00:00 200 OK

What did you see instead?

2009/11/10 23:00:00 0.0.0.0:2
2009/11/10 23:00:00 Get http://0.0.0.0:2/bar: proxyconnect tcp: dial tcp: Protocol not available

Discussion

This is either a bug report for http.ProxyFromEnvironment, or a request for a new helper function in net/http. I am happy to submit a CL for either one, but I'm not sure which is right.

It seems to me that what I'm trying to do is a pretty common operation, especially for testing and local development: listen on the unspecified port/interface, get the listen address, and connect to it over HTTP. This seems to mostly work, but it doesn't work if you have a corporate proxy set up with $HTTP_PROXY.

So either I have a bug report: http.ProxyFromEnvironment should return nil for IPs that are "unspecified" just like it does for IPs that are "loopback"

Or I have a feature request: There should be an easy way to map from the Addr you get from net.Listener.Addr() to a fully-functional HTTP URL (or host/port pair suitable for use in an HTTP URL). Specifically this method would change the unspecified IPv4 address to the loopback IPv4 address, and the unspecified IPv6 address to the loopback IPv6 address. Certainly I can write this code manually but it seems like something that others using net.Listen will run into.

My suspicion is that I'm looking for the feature request here, because even though URLs like http://[::]:4000/ and http://0.0.0.0:4000/ work with Go's net/http client (in the absence of proxy env vars), they might not work as well with other ecosystems' implementations, so being able to generate a more broadly usable URL might be a good idea. On the other hand, this might just be a fool's errand; for example, I've learned that CircleCI's default environment returns :: from net.Listen but doesn't support connecting to ::1 (connecting to :: does work, modulo the proxy issue).

But I think the bug may still be a bug, since it's unclear to me when you would ever want to send the unspecified IP to a corporate HTTP proxy.

@mvdan

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

/cc @rogpeppe

glasser added a commit to apollographql/apollo-engine-js that referenced this issue Apr 6, 2018

glasser added a commit to apollographql/apollo-engine-js that referenced this issue Apr 6, 2018

glasser added a commit to apollographql/apollo-engine-js that referenced this issue Apr 6, 2018

Don't generate URLs with unspecified IPs
Sort of a workaround for golang/go#24737

But also makes us actually match our docs and comment, which claim we do this
already.

Users will still trigger golang/go#24737 if they pass `innerHost: '::'`.

glasser added a commit to apollographql/apollo-engine-js that referenced this issue Apr 6, 2018

Don't generate URLs with unspecified IPs
Sort of a workaround for golang/go#24737

But also makes us actually match our docs and comment, which claim we do this
already.

Users will still trigger golang/go#24737 if they pass `innerHost: '::'`.

glasser added a commit to apollographql/apollo-engine-js that referenced this issue Apr 6, 2018

Don't generate URLs with unspecified IPs (#170)
Sort of a workaround for golang/go#24737

But also makes us actually match our docs and comment, which claim we do this
already.

Users will still trigger golang/go#24737 if they pass `innerHost: '::'`.
@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Hello @glasser, thank you for filing this issue.

For posterity, the bug report has an inadvertently misleading result, because of running from the playground as that doesn't represent actual behavior so far, that is

2009/11/10 23:00:00 Get http://0.0.0.0:2/bar: proxyconnect tcp: dial tcp: Protocol not available

is expected on the playground because the network resolver for NaCl(on which the playground runs) always returns an error. Sorry if am preaching to the choir but this code will always return the same error

package main

import (
	"fmt"
	"net/http"
)

func main() {
	_, err := http.Get("https://golang.org/")
	fmt.Println(err)
}

Playground

screen shot 2018-04-08 at 10 19 46 pm

Other machines

$ go run tk.go 
<nil>

Please update that in your report lest folks might go down a different rabbit hole.

And then onto the bug request, I still don't yet understand what the problem is and perhaps the connection between (net.IP).IsUnspecified is there, but perhaps could you help me articulate the exact problem and how we get there?

/cc @bradfitz @tombergan @rs who might understand the problem ASAP but also to put the bug on their radar.

Thank you.

@glasser

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

My point is that it doesn't make sense to invoke the proxy code at all when talking to the unspecified IP address, in the same way that http.Transport avoids invoking the proxy code when talking to a loopback IP address or the domain name "localhost". (After all, it doesn't seem likely that the result of asking a corporate HTTP proxy to open a connection to 0.0.0.0 will be particularly useful, just like you wouldn't ask it to connect to 127.0.0.1.)

So the playground demonstration is in fact apt, in that the error is not the same as the error you showed: mine contains the word "proxyconnect" and yours doesn't. But yeah, it's even more clear outside of the playground.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

I believe that it's an error to try to use the address returned from net.Listener.Addr as a Dial address when the address is all zeros. It happens to work under Linux, but it does not necessarily work under other platforms. For that reason, I'm not sure it's a good idea to treat this as a special case in the proxy code. A better solution, I believe, is either to explicitly listen on localhost or, if that's not reasonable because you need to listen on all available network interfaces, avoid using the Listener.Addr when the dialer is on the same host.

@glasser

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

Fair enough. I guess

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
is a good model for what I should do to listen in a test that needs to work in IPv4 or IPv6 environments.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@glasser, why don't we close this bug then and you can open a new bug about why that's as tedious as it is? We have a dozen some copies of that newLocalListener sprinkled about.

@glasser

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

Sure thing, #24778.

@golang golang locked and limited conversation to collaborators Apr 9, 2019

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