Hub: Support for no_proxy environment variable

[woneill]

It looks like https://github.com/github/hub/blob/master/github/http.go#L171-L193 is reimplementing the proxy logic from net/http/transport.go but it's missing the sections that support setting no_proxy.

[mislav]

Good catch! Wanna take a stab at implementing it?

[woneill]

Sure! I'll give it a shot since it's been causing a ton of pain for me having to mix proxy settings between git and hub. I'm a newbie at go so might take me a few weeks.

[mislav]

@woneill Just out of curiousty: how are you using http_proxy and no_proxy in your dev environment and at which point does it break in hub?

[woneill]

We have a site-wide proxy for all internet access but internal websites can/should be accessed directly. And we have github enterprise setup internally. So when I have HTTP_PROXY=http://proxy and NO_PROXY=.mydomain.com set, 'stock' git access works but anything handled by 'hub' (usually pull request generation) fails until I undefine HTTP_PROXY.

[woneill]

BTW, I have the no_proxy code from golang ported over, just trying to figure out a good way to write a cucumber test around it.

[jmaxxz]

:+1: on this issue, can not use hub with my local github enterprise without this feature.

[thale013]

+1 facing the same issue - no_proxy and NO_PROXY env vars are being ignored

[h0nIg]

+1

it seems a custom implementation is getting used and the full go class will support no_proxy env variables as well: https://golang.org/src/net/http/transport.go

[elovelan]

Here's the commit explaining why this was re-implemented. This is still a concern, so it may help to update the comment so it's a bit more clear why this was done.

[elovelan]

@woneill, I'm thinking I might pick this up. Do you have some existing code I can start from?

[elovelan]

@mislav, given the support for github.localhost introduced in 67ba00ae6653ef37a78f240b158418e727750f2d, is the separate proxy implementation introduced in 9a7d6fadf80893967e01a1c4525a2e3db1e1b95b for proxying to localhost or a loopback IP support still relevant?

[mislav]

@elovelan The support for github.localhost was added mostly for Hubbers like me and is not very useful for anyone who doesn't work at GitHub. It has nothing to do with proxy implementation.

If the current support for proxying and related environment variables is lacking, PRs welcome! Thank you

[elovelan]

@mislav, in your commit where you implemented this custom proxy logic, you said the following which seems to reference the exact use case for github.localhost that you mentioned above:

Go otherwise respects HTTP_PROXY environment variables by default, but special-cases requests made to "localhost" or 127.0.0.1 by disabling proxy for them, which makes me a sad panda since I want to debug the requests hub makes to our local test server.

I'm happy to open a PR with a clarifying code comment (in addition to adding the no_proxy logic) if there are other use cases you're thinking of for avoiding the proxy logic from Go's stdlib.

[mislav]

you said the following which seems to reference the exact use case for github.localhost that you mentioned above

Ah yes, thanks for digging that up. I do a lot of local testing/hacking, some using localhost and some using github.localhost (they are treated differently on my machine, though).

I haven't touched the proxy-handling code in a while, and I'm unsure if there are any blind spots that we still have (lacking support for no_proxy is one), but as I said, if you find anything that's broken/unclear, I'd appreciate PRs. Bonus points if we can remove my proxy hacks in favor of Go stdlib (note that hub still keeps compatibility with Go 1.8)

[mislav]

Actually, I'd prefer to _keep_ the implementation of reading proxy-related environment variables in hub just so that we can ensure that hub's proxy handling stays identical across Go versions 1.8–1.11 (and future ones). It would be great if no_proxy was respected!

[spackard]

+1 on this. hub should respect no_proxy.

[L4R5]

+1