Add initializer API for headers of HTTP proxy CONNECT request #2744
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| /* | ||
| * Copyright © 2023 Apple Inc. and the ServiceTalk project authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.servicetalk.http.api; | ||
|
|
||
| import io.servicetalk.transport.api.ClientSslConfig; | ||
|
|
||
| import java.util.function.Consumer; | ||
|
|
||
| /** | ||
| * Configuration for a proxy. | ||
| * | ||
| * @param <A> the type of address | ||
| */ | ||
| public interface ProxyConfig<A> { | ||
|
There was a problem hiding this comment. 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
Note that |
||
|
|
||
| /** | ||
| * Address of the proxy. | ||
| * <p> | ||
| * Usually, this is an unresolved proxy address and its type must match the type of address before resolution used | ||
| * by {@link SingleAddressHttpClientBuilder}. However, if the client builder was created for a resolved address, | ||
| * this address must also be already resolved. Otherwise, a runtime exceptions will occur. | ||
idelpivnitskiy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * @return address of the proxy | ||
| */ | ||
| A address(); | ||
|
|
||
| /** | ||
| * An initializer for {@link HttpHeaders} related to | ||
| * <a href="https://datatracker.ietf.org/doc/html/rfc9110#section-9.3.6">HTTP/1.1 CONNECT</a> request. | ||
| * <p> | ||
| * When this {@link ProxyConfig} is used for secure proxy tunnels (when | ||
| * {@link SingleAddressHttpClientBuilder#sslConfig(ClientSslConfig) ClientSslConfig} is configured) the tunnel is | ||
| * 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. | ||
| * | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. No, there are no retries specifically for a |
||
| * @return An initializer for {@link HttpHeaders} related to | ||
| * <a href="https://datatracker.ietf.org/doc/html/rfc9110#section-9.3.6">HTTP/1.1 CONNECT</a> request | ||
| */ | ||
| Consumer<HttpHeaders> connectRequestHeadersInitializer(); | ||
|
|
||
| /** | ||
| * Returns a {@link ProxyConfig} for the specified {@code address}. | ||
| * <p> | ||
| * All other {@link ProxyConfig} options will use their default values applied by {@link ProxyConfigBuilder}. | ||
| * | ||
| * @param address Address of the proxy | ||
| * @param <A> the type of address | ||
| * @return a {@link ProxyConfig} for the specified {@code address} | ||
| * @see ProxyConfigBuilder | ||
| */ | ||
| static <A> ProxyConfig<A> of(A address) { | ||
|
There was a problem hiding this comment. "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. There was a problem hiding this comment. Another option would be to not have this static method at all and let users go through the builder all the time? There was a problem hiding this comment. I had it originally and it looked quite verbose There was a problem hiding this comment. I will rename to |
||
| return new ProxyConfigBuilder<>(address).build(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /* | ||
| * Copyright © 2023 Apple Inc. and the ServiceTalk project authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.servicetalk.http.api; | ||
|
|
||
| import java.util.function.Consumer; | ||
|
|
||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * Builder for {@link ProxyConfig}. | ||
| * | ||
| * @param <A> the type of address | ||
| */ | ||
| public final class ProxyConfigBuilder<A> { | ||
|
|
||
| private static final Consumer<HttpHeaders> NOOP_HEADERS_CONSUMER = new Consumer<HttpHeaders>() { | ||
| @Override | ||
| public void accept(final HttpHeaders headers) { | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "NOOP"; | ||
idelpivnitskiy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }; | ||
|
|
||
| private final A address; | ||
| private Consumer<HttpHeaders> connectRequestHeadersInitializer = NOOP_HEADERS_CONSUMER; | ||
|
|
||
| /** | ||
| * Creates a new instance. | ||
| * | ||
| * @param address Proxy address | ||
| * @see ProxyConfig#address() | ||
| */ | ||
| public ProxyConfigBuilder(final A address) { | ||
| this.address = requireNonNull(address); | ||
| } | ||
|
|
||
| /** | ||
| * Sets an initializer for {@link HttpHeaders} related to | ||
| * <a href="https://datatracker.ietf.org/doc/html/rfc9110#section-9.3.6">HTTP/1.1 CONNECT</a> request. | ||
| * | ||
| * @param connectRequestHeadersInitializer {@link Consumer} that can be used to set custom {@link HttpHeaders} for | ||
| * {@code HTTP/1.1 CONNECT} request (auth, tracing, etc.) | ||
| * @return {@code this} | ||
| * @see ProxyConfig#connectRequestHeadersInitializer() | ||
| */ | ||
| public ProxyConfigBuilder<A> connectRequestHeadersInitializer( | ||
| final Consumer<HttpHeaders> connectRequestHeadersInitializer) { | ||
| this.connectRequestHeadersInitializer = requireNonNull(connectRequestHeadersInitializer); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Builds a new {@link ProxyConfig}. | ||
| * | ||
| * @return a new {@link ProxyConfig}. | ||
| */ | ||
| public ProxyConfig<A> build() { | ||
| return new DefaultProxyConfig<>(address, connectRequestHeadersInitializer); | ||
| } | ||
|
|
||
| private static final class DefaultProxyConfig<A> implements ProxyConfig<A> { | ||
|
|
||
| private final A address; | ||
| private final Consumer<HttpHeaders> connectRequestHeadersInitializer; | ||
|
|
||
| private DefaultProxyConfig(final A address, final Consumer<HttpHeaders> connectRequestHeadersInitializer) { | ||
| this.address = address; | ||
| this.connectRequestHeadersInitializer = connectRequestHeadersInitializer; | ||
| } | ||
|
|
||
| @Override | ||
| public A address() { | ||
| return address; | ||
| } | ||
|
|
||
| @Override | ||
| public Consumer<HttpHeaders> connectRequestHeadersInitializer() { | ||
| return connectRequestHeadersInitializer; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { | ||
|
There was a problem hiding this comment. Because one of the fields is a There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof DefaultProxyConfig)) { | ||
| return false; | ||
| } | ||
|
|
||
| final DefaultProxyConfig<?> that = (DefaultProxyConfig<?>) o; | ||
| if (!address.equals(that.address)) { | ||
| return false; | ||
| } | ||
| return connectRequestHeadersInitializer.equals(that.connectRequestHeadersInitializer); | ||
bryce-anderson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| int result = address.hashCode(); | ||
| result = 31 * result + connectRequestHeadersInitializer.hashCode(); | ||
| return result; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return getClass().getSimpleName() + | ||
| "{address=" + address + | ||
| ", connectRequestHeadersInitializer=" + connectRequestHeadersInitializer + | ||
| '}'; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated between
ProxyConfig.forAddress(proxyAddress)andProxyConfig.of(proxyAddress). Went withofonly because wanted to be consistent with similar static method on other interfaces/classes, likeHostAndPort,HttpRequestMethod, etc. Ifofdoesn't read well for this use-case, happy to rename it by your recommendations.