reverseproxy: refactor HTTP transport layer

[mohammed90]

The earlier implementation smuggles the upstream dial info through context. It made it difficult to accommodate utilizing the standard env vars HTTP_PROXY, HTTPS_PROXY, and NO_PROXY for upstream connections. It also hurt connection caching. The earlier hack was to accommodate the case of upstreams being Unix sockets. The alternative approach is to register a custom protocol with a custom Transport, which is provided by the package github.com/peterbourgon/unixtransport, and let it manage the case of unix sockets. This PR fixes the mentioned pain points of the earlier implementation.

Fixes #5178

[mohammed90]

Oh, @mholt, this change loses the TCP connection timeouts 😕 I'm not sure how yet to retrofit that into the current change. Let me know if you have any ideas or to brainstorm something.

[WeidiDeng]

Why is connection pooling hurt from original implementation? Are there any reports?

[WeidiDeng]

@mohammed90 In slack you mentioned It doesn't work, does it mean adding proxy field won't work because of DialContext address smuggling? I'll do some local testing to find out.

[mohammed90]

@mohammed90 In slack you mentioned It doesn't work, does it mean adding proxy field won't work because of DialContext address smuggling? I'll do some local testing to find out.

The doesn't work refers to the earlier implementation, not this PR. As said, it's because of the request smuggling through context.Context. We used to dial upstream directly, ignoring any logic deferring to the environment HTTP proxy. You can refer to the PR #5359 for the earlier attempt where it didn't work. It works in this PR.

Why is connection pooling hurt from original implementation? Are there any reports?

After going through your reasoning in our Slack discussion, I realize I was mistaken 😅 Connection pooling wasn't hurt in the earlier implementation.

[mholt]

This looks great, actually -- thanks for the PR!

I'm curious how this works for people actually proxying to unix sockets; I have only done so here and there for testing purposes. Maybe we can get someone to try it in a real deployment? (Doesn't have to be a crazy busy/important one.)

Then once the linter is satisfied we can probably look at merging this in :)

[WeidiDeng]

This looks great, actually -- thanks for the PR!

I'm curious how this works for people actually proxying to unix sockets; I have only done so here and there for testing purposes. Maybe we can get someone to try it in a real deployment? (Doesn't have to be a crazy busy/important one.)

Then once the linter is satisfied we can probably look at merging this in :)

No, unix socket will bypass settings of http.transport because we already registered a new protocol (http+unix). It's up to unix transport to handle proxy (which is no-op).

[WeidiDeng]

I still think we should rewrite address only when network is unix, instead of importing a new dependency. This dependency encodes unix path as base64 under the hood.

[francislavoie]

I reworked the PR; mostly reverted what was there, and implemented the fix @WeidiDeng recommended in Slack.

Basically, we should only override the dial address from the context dialInfo if the context had unix as the network.

Integration tests pass, and I confirmed once again that the unix socket test fails if we don't override the dial address.

I also cleaned up whitespace in the tests, and I changed the listen address from :8080 to :18080 because I use :8080 locally for my dev environment 🙉 and I think 18080 should be less likely to conflict in general.

[mohammed90]

I still think we should rewrite address only when network is unix, instead of importing a new dependency. This dependency encodes unix path as base64 under the hood.

The reason it encodes the path in basae64 is because, more or less, files/directories names are not limited in any way on Linux ^{1 2}. Notice that it encodes it in the URL charset of base64 because URLs accept only a certain set of characters. So it encodes it in base64 to pass through the URL parsing of Go stdlib while retaining the socket path and request path, then decodes it before dialing.

Anyways, as long as we get it working, the approach is irrelevant to me. Thanks for the review and the rework!

Footnotes

1. https://dwheeler.com/essays/fixing-unix-linux-filenames.html ↩

2. https://lwn.net/Articles/71472/ ↩

[mholt]

This change LGTM.

Thank you everyone for collaborating on it!