Add initializer API for headers of HTTP proxy CONNECT request #2744

idelpivnitskiy · 2023-11-08T00:08:56Z

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,
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.

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 · 2023-11-08T00:10:58Z

servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcProxyTunnelTest.java

@@ -89,7 +90,7 @@ class GrpcProxyTunnelTest {
                .listenAndAwait((Greeter.BlockingGreeterService) (ctx, request) ->
                        HelloReply.newBuilder().setMessage(GREETING_PREFIX + request.getName()).build());
        client = GrpcClients.forAddress(serverHostAndPort(serverContext))
-                .initializeHttp(httpBuilder -> httpBuilder.proxyAddress(proxyAddress)
+                .initializeHttp(httpBuilder -> httpBuilder.proxyConfig(ProxyConfig.of(proxyAddress))


I debated between ProxyConfig.forAddress(proxyAddress) and ProxyConfig.of(proxyAddress). Went with of only because wanted to be consistent with similar static method on other interfaces/classes, like HostAndPort, HttpRequestMethod, etc. If of doesn't read well for this use-case, happy to rename it by your recommendations.

idelpivnitskiy · 2023-11-08T00:17:32Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfig.java

+ *
+ * @param <A> the type of address
+ */
+public interface ProxyConfig<A> {


For others who review: this may look like an overkill to add a new "config" API while we need only one more config parameter, which could be a 2nd argument for original proxyAddress(...) method, like it was implemented in #2698. However, we discussed with @daschl and agreed that having a config object now (similar to RedirectConfig) will minimize API changes on SingleAddressHttpClientBuilder in the future, when we may need to add more options, like:

Timeout for CONNECT request, similar to what Netty does: https://github.com/netty/netty/blob/7e18e8344819428a5560064ac15a2f035b4728fd/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java#L57

SOCKS proxy config options

Note that SingleAddressHttpClientBuilder is used by users when they rely on HttpProviders. So, it's important to minimize API friction for them.

bryce-anderson

Other than the open question wrt equals and hashCode it looks good.

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfigBuilder.java

bryce-anderson · 2023-11-08T19:04:52Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfigBuilder.java

+        }
+
+        @Override
+        public boolean equals(final Object o) {


Because one of the fields is a Consumer, which is going to often be set by anonymous lambda, I think notions of equality and hash-code are going to be error prone. Does this class need to have such notions?

I think it's fine to keep best effort and we can rely on users to care about equality for consumer. If they don't care, having different lambdas will make configs different. If they care, they must provide the same instance of Consumer or override equals/hashCode for consumer.

If that's an invariant which is important for the semantics of the executed code, I think we should point this out in a javadoc somewhere.

It feels kind of obvious that if users pass 2 different lambdas, the resulting "pojo" will be different. Note that there is no use-case for equals/hashCode right now to really worry about it. This is a pkg-private class, which makes it harder to document impl details. Not sure if we need to put this info on interface bcz it's not required right now.

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectChannelSingle.java

…xyConfigBuilder.java Co-authored-by: Bryce Anderson <bryce_anderson@apple.com>

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfig.java

daschl · 2023-11-09T07:08:47Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfig.java

+     * always initialized using {@code HTTP/1.1 CONNECT} request. This {@link Consumer} can be used to set custom
+     * {@link HttpHeaders} for that request, like {@link HttpHeaderNames#PROXY_AUTHORIZATION proxy-authorization},
+     * tracing, or any other header.
+     *


If (are we?) doing any retries on the connect request, this comment I think should clarified if it is called N times or just once.

No, there are no retries specifically for a CONNECT request. In case of a general retry, it will go through process of opening a new connection and then it's not different from any other new connection event, regardless of if it was retried or not.

daschl · 2023-11-09T07:12:06Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfig.java

+     * @return a {@link ProxyConfig} for the specified {@code address}
+     * @see ProxyConfigBuilder
+     */
+    static <A> ProxyConfig<A> of(A address) {


"forAddress" I think reads easier (since you are creating a proxy config for an address, not of an address), but as you mentioned using of aligns it with HostAndPort. I'm ok either way and it's not a big deal.

Another option would be to not have this static method at all and let users go through the builder all the time?

I had it originally and it looked quite verbose

I will rename to forAddress to make it more readable when someone uses a static import.

daschl · 2023-11-09T07:15:28Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/ProxyConfigBuilder.java

+        }
+
+        @Override
+        public boolean equals(final Object o) {


If that's an invariant which is important for the semantics of the executed code, I think we should point this out in a javadoc somewhere.

daschl · 2023-11-09T07:18:51Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/SingleAddressHttpClientBuilder.java

-                + getClass().getName());
+    // FIXME: 0.43 - remove deprecated method
+    @Deprecated
+    @SuppressWarnings("DeprecatedIsStillUsed")


Should we keep this method for convenience? Related to my suggestions of the naming of the factory method: if we force them to go through the builder I would keep this convenience method, if we have the static factory I guess there is no harm in removing this since its going to be very short anyways.

Usually, users have more confusion when there are multiple overloads on the builder for the same functionality. Especially when they override each other. People don't really read javadoc and as a result, there is a high chance of confusing their understanding of runtime behavior when they use both of them:

builder.proxyAddress(address) .proxyConfig(config)

Another use-case is HttpProviders. It makes it harder to implement a provider that need to track 2 different methods for the same functionality. As a result, I proposed a static factory.

good point, makes sense. thanks!

…xyConfig.java Co-authored-by: Michael Nitschinger <michael@nitschinger.at>

idelpivnitskiy added 2 commits November 7, 2023 15:39

Consolidate ReadOnlyHttpClientConfig and ProxyConnectChannelSingle state

6e157a5

idelpivnitskiy requested review from daschl and mgodave November 8, 2023 00:08

idelpivnitskiy self-assigned this Nov 8, 2023

idelpivnitskiy commented Nov 8, 2023

View reviewed changes

idelpivnitskiy mentioned this pull request Nov 8, 2023

Add initializer API for HTTP proxy CONNECT request #2698

Closed

bryce-anderson approved these changes Nov 8, 2023

View reviewed changes

Update servicetalk-http-api/src/main/java/io/servicetalk/http/api/Pro…

9c5076d

…xyConfigBuilder.java Co-authored-by: Bryce Anderson <bryce_anderson@apple.com>

daschl reviewed Nov 9, 2023

View reviewed changes

idelpivnitskiy and others added 2 commits November 11, 2023 09:39

Update servicetalk-http-api/src/main/java/io/servicetalk/http/api/Pro…

a061338

…xyConfig.java Co-authored-by: Michael Nitschinger <michael@nitschinger.at>

ProxyConfig.of -> ProxyConfig.forAddress

c34a52e

idelpivnitskiy requested a review from daschl November 11, 2023 17:48

daschl approved these changes Nov 13, 2023

View reviewed changes

idelpivnitskiy merged commit 40e20f5 into apple:main Nov 13, 2023
15 checks passed

idelpivnitskiy deleted the ProxyConfig branch November 13, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initializer API for headers of HTTP proxy CONNECT request #2744

Add initializer API for headers of HTTP proxy CONNECT request #2744

idelpivnitskiy commented Nov 8, 2023

idelpivnitskiy Nov 8, 2023

idelpivnitskiy Nov 8, 2023

bryce-anderson left a comment

bryce-anderson Nov 8, 2023

idelpivnitskiy Nov 8, 2023 •

edited

Loading

daschl Nov 9, 2023

idelpivnitskiy Nov 11, 2023

daschl Nov 9, 2023

idelpivnitskiy Nov 11, 2023

daschl Nov 9, 2023

daschl Nov 9, 2023

idelpivnitskiy Nov 11, 2023

idelpivnitskiy Nov 11, 2023

daschl Nov 9, 2023

daschl Nov 9, 2023

idelpivnitskiy Nov 11, 2023

daschl Nov 13, 2023

Add initializer API for headers of HTTP proxy CONNECT request #2744

Add initializer API for headers of HTTP proxy CONNECT request #2744

Conversation

idelpivnitskiy commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Nov 8, 2023 •

edited

Loading