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.

Sign up for GitHub

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

Jump to bottom

Move HTTP proxy CONNECT logic before ConnectionFactoryFilters #2697

Merged
merged 5 commits into from
Sep 29, 2023
Merged

Move HTTP proxy CONNECT logic before ConnectionFactoryFilters #2697

merged 5 commits into from
Sep 29, 2023

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Sep 19, 2023 •
edited
Loading

Motivation:

Currently, HTTP proxy CONNECT logic is prepended before user-defined ConnectionFactoryFilters. Because user filters can wrap FilterableStreamingHttpConnection:

  1. It makes it hard to upgrade the connection 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;
  • Rename ProxyConnectConnectionFactoryFilterTest to ProxyConnectLBHttpConnectionFactoryTest, adjust for testing processConnect method;

Result:

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

Behavior change:

  • Because user-defined ConnectionFactoryFilters can not intercept HTTP CONNECT requests anymore, they lose 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.

@idelpivnitskiy idelpivnitskiy self-assigned this Sep 19, 2023
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 19, 2023
@idelpivnitskiy
Add initializer API for HTTP proxy CONNECT request
b5dbbac 
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 `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 added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 19, 2023
@idelpivnitskiy
Add initializer API for HTTP proxy CONNECT request
1813f2c 
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 `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 added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 19, 2023
@idelpivnitskiy
Add initializer API for HTTP proxy CONNECT request
a50c57b 
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.
This was referenced Sep 19, 2023
Closed
Merged
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 19, 2023
@idelpivnitskiy
Add initializer API for HTTP proxy CONNECT request
4ee6048 
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
Move HTTP proxy CONNECT logic before ConnectionFactoryFilters
cab4d9b 
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.
@idelpivnitskiy
ProxyConnectConnectionFactoryFilterTest -> ProxyConnectLBHttpConnecti…
5496e18 
…onFactoryTest
@idelpivnitskiy
Remove unused ProxyConnectLBHttpConnectionFactoryTest.NettyHttpConnec…
b4314f3 
…tionContext
@idelpivnitskiy
comments
2d23fbf
@idelpivnitskiy
Cherry-pick tests for proxy-auth from apple#2698
93179ef
@idelpivnitskiy idelpivnitskiy merged commit 07a41d5 into apple:main Sep 29, 2023
15 checks passed
@idelpivnitskiy idelpivnitskiy deleted the move-proxy-connect branch September 29, 2023 17:53
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Nov 8, 2023
@idelpivnitskiy
Add initializer API for headers of HTTP proxy CONNECT request
46ae4e4 
Motivation:

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

Modifications:

- Introduce `ProxyConfig` and `ProxyConfigBuilder` in `http-api` that
can be used to provide additional options for proxy behavior;
- Add `SingleAddressHttpClientBuilder.proxyConfig(...)` overload that
takes `ProxyConfig`;
- Deprecate pre-existing `SingleAddressHttpClientBuilder.proxyAddress()`
method, recommend switching to new API;
- Enhance `HttpsProxyTest` to verify that new API can be used to send
`Proxy-Authorization` header;

Result:

Users have explicit API to alter HTTP `CONNECT` request headers, if
necessary.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Nov 8, 2023
Merged
idelpivnitskiy added a commit that referenced this pull request Nov 13, 2023
@idelpivnitskiy
Add initializer API for headers of HTTP proxy CONNECT request (#2744)
40e20f5 
Motivation:

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

Modifications:

- Introduce `ProxyConfig` and `ProxyConfigBuilder` in `http-api` that
can be used to provide additional options for proxy behavior;
- Add `SingleAddressHttpClientBuilder.proxyConfig(...)` overload that
takes `ProxyConfig`;
- Deprecate pre-existing `SingleAddressHttpClientBuilder.proxyAddress()`
method, recommend switching to new API;
- Enhance `HttpsProxyTest` to verify that new API can be used to send
`Proxy-Authorization` header;

Result:

Users have explicit API to alter HTTP `CONNECT` request headers, if
necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschl daschl daschl approved these changes

Assignees

@idelpivnitskiy idelpivnitskiy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@idelpivnitskiy @daschl