Fix HTTP client proxy support #742
Conversation
Since the introduction of HTTP/2 support in the client, proxy handlers (requested via proxy-options) would be installed only after the HTTP version was negotiated. In case of a HTTPS connection, this would be attempted via ALPN which means the TLS handshake would have already taken place with the actual destination host, bypassing the configured proxy. This resulted in an error (see #731). With this patch, the proxy handlers are installed earlier again so that a configured proxy is also used during the TLS handshake phase.
Turns out this worked perfectly fine already.
|
FYI: There's a huge diff there due to wrapping + reindent, so I recommend to hide whitespace when looking at the diff once more! |
| @@ -396,7 +396,7 @@ | |||
|
|
|||
| :else | |||
| cause)] | |||
| (s/put! response-stream response) | |||
| (d/error! early-response-d response) | |||
There was a problem hiding this comment.
Oh yeah, so far, I was unable to even reach this code path and have it result in an error response anyway. It would probably be a good idea to come up with a bunch of test cases for the proxy functionality (currently there are none).
There was a problem hiding this comment.
Given how untested proxy code is, I'd strongly suggest writing tests before approving.
Maybe whoever needed the proxies could lend a hand?
There was a problem hiding this comment.
I was able to come up with a test that fails as expected without the patch - sanity check welcome 🙏
Using the test as a basis, I should be able to see how we can reach the error case referenced above. Stay tuned.
Fixes #731