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

Add initializer API for HTTP proxy CONNECT request #2698

Closed

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

After #2697 moved HTTP proxy CONNECT logic before user-defined
ConnectionFactoryFilters, users lost the ability to intercept CONNECT
requests for the purpose of adding custom headers, like auth.

Modifications:

  • Add SingleAddressHttpClientBuilder.proxyAddress(...) overload that
    takes Consumer<StreamingHttpRequest> as a 2nd argument;
  • Recompute HttpExecutionStrategy after CONNECT request initializer
    is invoked in ProxyConnectLBHttpConnectionFactory;
  • Enhance ProxyConnectLBHttpConnectionFactoryTest to verify that the
    initializer is invoked and users can alter the execution strategy;
  • Enhance ProxyTunnel and HttpsProxyTest to verify that new API
    can be used to send Proxy-Authorization header;

Result:

Users have explicit API to alter HTTP CONNECT requests if necessary.


Depends on #2697, review only the last commit.

Motivation:

Currently, HTTP proxy `CONNECT` logic is prepended before user-defined
`ConnectionFactoryFilter`s. Because user filters can wrap
`FilterableStreamingHttpConnection`:
1. It makes it hard to upgrade connection protocol if ALPN agrees on a
different protocol.
2. `ProxyConnectConnectionFactoryFilter` can not access Netty's
`Channel` to trigger TLS handshake.

Modifications:
- Create `ProxyConnectLBHttpConnectionFactory` as an alternative to
`PipelinedLBHttpConnectionFactory` that makes HTTP proxy `CONNECT`
request after `PipelinedStreamingHttpConnection` is created;
- Keep `ProxyConnectConnectionFactoryFilter` only for the purpose of
propagating `HTTP_TARGET_ADDRESS_BEHIND_PROXY` key upstream;
- Add internal `NettyFilterableStreamingHttpConnection` to get access to
Netty `Channel` without type cast;
- Enhance `StacklessClosedChannelException` to carry a message;

Result:

1. No need to worry that HTTP CONNECT won't work if users wrapped
`FilterableStreamingHttpConnection` in their `ConnectionFactoryFilter`s.
2. Makes it possible to change protocol based on ALPN (will be done in
a follow-up PRs).

Behavior change:
- Because user-defined `ConnectionFactoryFilter`s can not intercept
HTTP `CONNECT` request anymore, they loose ability to alter the request
(for example, to set auth or debug headers). Follow-up PRs will add new
API to let users intercept such requests.
* @return {@code this}.
*/
default SingleAddressHttpClientBuilder<U, R> proxyAddress(U proxyAddress, // FIXME: 0.43 - remove default impl
Consumer<StreamingHttpRequest> requestInitializer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I know it's in the javadoc, but consider renaming it to "connectRequestInitializer" to make it clear that it is really only for the initial connect and not any subsequent http requests going through the proxy.

Motivation:

After apple#2697 moved HTTP proxy `CONNECT` logic before user-defined
`ConnectionFactoryFilter`s, users lost ability to intercept `CONNECT`
requests for the purpose of adding custom headers, like auth.

Modifications:

- Add `SingleAddressHttpClientBuilder.proxyAddress(...)` overload that
takes `Consumer<StreamingHttpRequest>` as a 2nd argument;
- Recompute `HttpExecutionStrategy` after `CONNECT` request initializer
is invoked in `ProxyConnectLBHttpConnectionFactory`;
- Enhance `ProxyConnectLBHttpConnectionFactoryTest` to verify that the
initializer is invoked and users can alter execution strategy;
- Enhance `ProxyTunnel` and `HttpsProxyTest` to verify that new API
can be used to send `Proxy-Authorization` header;

Result:

Users have explicit API to alter HTTP `CONNECT` request if necessary.
@idelpivnitskiy idelpivnitskiy force-pushed the connect-requestInitializer branch from a50c57b to 3f2be34 Compare September 19, 2023 17:11
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 25, 2023
@idelpivnitskiy
Copy link
Member Author

We made a decision to defer adding this API for now. Taking into account that while before #2697 it was possible to add proxy-authorization header via ConnectionFactoryFilter, in practice that approach was too complicated for users to figure it out without our help and we are not aware of anyone requiring this feature for now.
If the use-case appears, we will provide the required API. At this point, we would like to hear real user request before we prematurely add new API. This will help us to implement better API design and answer questions like:

  1. Should we let them initialize the whole StreamingHttpRequest or limit it to only HttpRequestMetaData or HttpHeaders?
  2. Do they need a function/consumer or a static way to provide auth info?
  3. Do we need a more complex configuration API for proxies or not, like a way to let users configure a timeout for CONNECT request?

I cherry-picked enhancements for tests from this PR to #2697.

@idelpivnitskiy
Copy link
Member Author

Reworked in #2744

@idelpivnitskiy idelpivnitskiy deleted the connect-requestInitializer branch November 8, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants