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

Respect TTL of a DNS record for proxy config #5960

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
417bf3b
Client respects DNS TTL.
chickenchickenlove Oct 27, 2024
3681f0f
Merge branch 'main' into 241027-proxy-config
chickenchickenlove Oct 27, 2024
f57259c
Revert "Client respects DNS TTL."
chickenchickenlove Oct 30, 2024
f40cbe4
Use RefreshingAddressResolver to refresh proxy address.
chickenchickenlove Oct 30, 2024
a26e3f5
Merge branch '241027-proxy-config' of github.com-ojt90902:chickenchic…
chickenchickenlove Oct 30, 2024
649469e
Handle null of proxyconfig.
chickenchickenlove Oct 30, 2024
ab43277
Add a copmment.
chickenchickenlove Oct 30, 2024
a6e725c
Add withNewProxyAddress() to create after refresh DNS.
chickenchickenlove Nov 2, 2024
e0819c4
Resolve Proxy DNS to respect DNS TTL asynchronously.
chickenchickenlove Nov 3, 2024
0b3cc2b
Remove useless method from ProxyConfig.
chickenchickenlove Nov 3, 2024
54ec6f3
Fixes lint error.
chickenchickenlove Nov 3, 2024
a67ee3f
Revert previous commit.
chickenchickenlove Nov 3, 2024
241b4ff
Remove useless log codes.
chickenchickenlove Nov 4, 2024
fce4fd2
Remove unused import.
chickenchickenlove Nov 4, 2024
7d85a69
Fixes lint error.
chickenchickenlove Nov 4, 2024
67f4997
Add integration test for DNS refreshing.
chickenchickenlove Nov 4, 2024
e775351
Remove deprecated test case.
chickenchickenlove Nov 4, 2024
18b24ea
Apply reviews.
chickenchickenlove Nov 11, 2024
979e6fd
Add a private method to refresh proxy DNS.
chickenchickenlove Nov 17, 2024
3dbc847
Skip refreshing ProxyConfig DNS when it is configured with a direct IP.
chickenchickenlove Nov 20, 2024
fcfed45
clean up
ikhoon Nov 21, 2024
09efa86
add more tests
ikhoon Nov 21, 2024
25ff1af
Fixes broken test codes and add new test cases.
chickenchickenlove Nov 21, 2024
43212b6
Remove useless logging.
chickenchickenlove Nov 22, 2024
195033f
Add null condition.
chickenchickenlove Nov 22, 2024
d1c9596
Update core/src/main/java/com/linecorp/armeria/client/HttpClientDeleg…
chickenchickenlove Nov 28, 2024
d5c8326
Update core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyCo…
chickenchickenlove Nov 28, 2024
1240a24
Remove unused import
chickenchickenlove Nov 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,35 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex
}

final SessionProtocol protocol = ctx.sessionProtocol();
final ProxyConfig proxyConfig;

final Endpoint endpointWithPort = endpoint.withDefaultPort(ctx.sessionProtocol());
final EventLoop eventLoop = ctx.eventLoop().withoutContext();
// TODO(ikhoon) Use ctx.exchangeType() to create an optimized HttpResponse for non-streaming response.
final DecodedHttpResponse res = new DecodedHttpResponse(eventLoop);
updateCancellationTask(ctx, req, res);

try {
proxyConfig = getProxyConfig(protocol, endpoint);
resolveProxyConfig(protocol, endpoint, ctx, (proxyConfig, thrown) -> {
if (thrown != null) {
earlyFailedResponse(thrown, ctx, res);
} else {
execute0(ctx, endpointWithPort, req, res, proxyConfig);
chickenchickenlove marked this conversation as resolved.
Show resolved Hide resolved
}
});
} catch (Throwable t) {
return earlyFailedResponse(t, ctx);
}
return res;
}

private void execute0(ClientRequestContext ctx, Endpoint endpointWithPort, HttpRequest req,
DecodedHttpResponse res, ProxyConfig proxyConfig) {
final Throwable cancellationCause = ctx.cancellationCause();
if (cancellationCause != null) {
return earlyFailedResponse(cancellationCause, ctx);
earlyFailedResponse(cancellationCause, ctx, res);
return;
}

final Endpoint endpointWithPort = endpoint.withDefaultPort(ctx.sessionProtocol());
final EventLoop eventLoop = ctx.eventLoop().withoutContext();
// TODO(ikhoon) Use ctx.exchangeType() to create an optimized HttpResponse for non-streaming response.
final DecodedHttpResponse res = new DecodedHttpResponse(eventLoop);
updateCancellationTask(ctx, req, res);

final ClientConnectionTimingsBuilder timingsBuilder = ClientConnectionTimings.builder();

if (endpointWithPort.hasIpAddr() ||
Expand All @@ -125,8 +136,6 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex
}
});
}

return res;
}

private static void updateCancellationTask(ClientRequestContext ctx, HttpRequest req,
Expand Down Expand Up @@ -215,24 +224,53 @@ private void acquireConnectionAndExecute0(ClientRequestContext ctx, Endpoint end
}
}

private ProxyConfig getProxyConfig(SessionProtocol protocol, Endpoint endpoint) {
private void resolveProxyConfig(SessionProtocol protocol, Endpoint endpoint, ClientRequestContext ctx,
BiConsumer<@Nullable ProxyConfig, @Nullable Throwable> onComplete) {
final ProxyConfig proxyConfig = factory.proxyConfigSelector().select(protocol, endpoint);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be efficient if we could skip the resolution process if proxyConfig.proxyAddress() was created with IP addresses.

requireNonNull(proxyConfig, "proxyConfig");

final ServiceRequestContext serviceCtx = ServiceRequestContext.currentOrNull();
final ProxiedAddresses capturedProxiedAddresses = serviceCtx == null ? null
: serviceCtx.proxiedAddresses();
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
// DirectProxyConfig does not have proxyAddress as its field.
if (proxyConfig.proxyAddress() != null) {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
final InetSocketAddress currentAddr = proxyConfig.proxyAddress();
if (!currentAddr.isUnresolved() && (currentAddr.getHostName() == currentAddr.getHostString())) {
// If InetSocket is created from IP Address, we don't need to refresh DNS.
onComplete.accept(maybeHAProxy(proxyConfig, capturedProxiedAddresses), null);
} else {
final Future<InetSocketAddress> resolveFuture = addressResolverGroup
.getResolver(ctx.eventLoop().withoutContext())
.resolve(createUnresolvedAddressForRefreshing(proxyConfig.proxyAddress()));

resolveFuture.addListener(future -> {
if (future.isSuccess()) {
final InetSocketAddress resolvedAddress = (InetSocketAddress) future.getNow();
final ProxyConfig newProxyConfig = proxyConfig.withProxyAddress(resolvedAddress);
onComplete.accept(maybeHAProxy(newProxyConfig, capturedProxiedAddresses), null);
} else {
final Throwable cause = future.cause();
onComplete.accept(null, cause);
}
});
}
} else {
onComplete.accept(maybeHAProxy(proxyConfig, capturedProxiedAddresses), null);
}
}

private ProxyConfig maybeHAProxy(ProxyConfig proxyConfig, @Nullable ProxiedAddresses proxiedAddresses) {
// special behavior for haproxy when sourceAddress is null
if (proxyConfig.proxyType() == ProxyType.HAPROXY &&
((HAProxyConfig) proxyConfig).sourceAddress() == null) {
final InetSocketAddress proxyAddress = proxyConfig.proxyAddress();
assert proxyAddress != null;

// use proxy information in context if available
final ServiceRequestContext serviceCtx = ServiceRequestContext.currentOrNull();
if (serviceCtx != null) {
final ProxiedAddresses proxiedAddresses = serviceCtx.proxiedAddresses();
if (proxiedAddresses != null) {
return ProxyConfig.haproxy(proxyAddress, proxiedAddresses.sourceAddress());
}
}

return proxyConfig;
}

Expand All @@ -253,11 +291,24 @@ private static HttpResponse earlyFailedResponse(Throwable t, ClientRequestContex
return HttpResponse.ofFailure(cause);
}

private static HttpResponse earlyFailedResponse(Throwable t,
ClientRequestContext ctx,
DecodedHttpResponse response) {
final UnprocessedRequestException cause = UnprocessedRequestException.of(t);
ctx.cancel(cause);
response.close(cause);
return response;
}

private static void doExecute(PooledChannel pooledChannel, ClientRequestContext ctx,
HttpRequest req, DecodedHttpResponse res) {
final Channel channel = pooledChannel.get();
final HttpSession session = HttpSession.get(channel);
res.init(session.inboundTrafficController());
session.invoke(pooledChannel, ctx, req, res);
}

private static InetSocketAddress createUnresolvedAddressForRefreshing(InetSocketAddress previousAddress) {
return InetSocketAddress.createUnresolved(previousAddress.getHostString(), previousAddress.getPort());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ public ProxyType proxyType() {
return ProxyType.CONNECT;
}

@Override
public ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress) {
return new ConnectProxyConfig(newProxyAddress, this.username,
this.password, this.headers, this.useTls);
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ public InetSocketAddress proxyAddress() {
return null;
}

@Override
public ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress) {
throw new UnsupportedOperationException(
"A proxy address can't be set to DirectProxyConfig.");
}

@Override
public String toString() {
return "DirectProxyConfig{proxyType=DIRECT}";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@
package com.linecorp.armeria.client.proxy;

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.hash;
import static java.util.Objects.requireNonNull;

import java.net.InetSocketAddress;
import java.util.Objects;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.MoreObjects;

import com.linecorp.armeria.common.annotation.Nullable;
Expand All @@ -31,6 +36,8 @@
*/
public final class HAProxyConfig extends ProxyConfig {

private static final Logger logger = LoggerFactory.getLogger(HAProxyConfig.class);

chickenchickenlove marked this conversation as resolved.
Show resolved Hide resolved
private final InetSocketAddress proxyAddress;

@Nullable
Expand All @@ -42,8 +49,18 @@ public final class HAProxyConfig extends ProxyConfig {
}

HAProxyConfig(InetSocketAddress proxyAddress, InetSocketAddress sourceAddress) {
checkArgument(sourceAddress.getAddress().getClass() == proxyAddress.getAddress().getClass(),
"sourceAddress and proxyAddress should be the same type");
if (sourceAddress.getAddress() != null &&
proxyAddress.getAddress() != null) {
checkArgument(sourceAddress.getAddress().getClass() == proxyAddress.getAddress().getClass(),
"sourceAddress and proxyAddress should be the same type");
} else {
logger.warn("Either the source or proxy address could not be resolved. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of removing this warning? This message doesn't seem useful, and I'm unsure if warn is a proper level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I removed it!

"The proxy address may be resolved later. " +
"proxyAddress: {}, sourceAddress: {}",
proxyAddress.getAddress(),
sourceAddress.getAddress());
}

this.proxyAddress = proxyAddress;
this.sourceAddress = sourceAddress;
}
Expand All @@ -67,6 +84,13 @@ public InetSocketAddress sourceAddress() {
return sourceAddress;
}

@Override
public ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress) {
requireNonNull(newProxyAddress, "newProxyAddress");
return this.sourceAddress == null ? new HAProxyConfig(proxyAddress)
: new HAProxyConfig(proxyAddress, this.sourceAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

The input is ignored.

Suggested change
return this.sourceAddress == null ? new HAProxyConfig(proxyAddress)
: new HAProxyConfig(proxyAddress, this.sourceAddress);
requireNonNull(newProxyAddress, "newProxyAddress");
return this.sourceAddress == null ? new HAProxyConfig(newProxyAddress)
: new HAProxyConfig(newProxyAddress, this.sourceAddress);

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment wasn't addressed. Was it intended? Let me know if I'm missing something or misunderstood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really sorry about this. 🙇‍♂️
I missed it.
Now, I fixed it!

}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand All @@ -82,7 +106,7 @@ public boolean equals(@Nullable Object o) {

@Override
public int hashCode() {
return Objects.hash(proxyAddress, sourceAddress);
return hash(proxyAddress, sourceAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have been reverted!

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ public abstract class ProxyConfig {
*/
public static Socks4ProxyConfig socks4(InetSocketAddress proxyAddress) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new Socks4ProxyConfig(proxyAddress, null);
}

Expand All @@ -52,7 +51,6 @@ public static Socks4ProxyConfig socks4(InetSocketAddress proxyAddress) {
*/
public static Socks4ProxyConfig socks4(InetSocketAddress proxyAddress, String username) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new Socks4ProxyConfig(proxyAddress, requireNonNull(username, "username"));
}

Expand All @@ -63,7 +61,6 @@ public static Socks4ProxyConfig socks4(InetSocketAddress proxyAddress, String us
*/
public static Socks5ProxyConfig socks5(InetSocketAddress proxyAddress) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new Socks5ProxyConfig(proxyAddress, null, null);
}

Expand All @@ -77,7 +74,6 @@ public static Socks5ProxyConfig socks5(InetSocketAddress proxyAddress) {
public static Socks5ProxyConfig socks5(
InetSocketAddress proxyAddress, String username, String password) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new Socks5ProxyConfig(proxyAddress, requireNonNull(username, "username"),
requireNonNull(password, "password"));
}
Expand All @@ -89,7 +85,6 @@ public static Socks5ProxyConfig socks5(
*/
public static ConnectProxyConfig connect(InetSocketAddress proxyAddress) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new ConnectProxyConfig(proxyAddress, null, null, HttpHeaders.of(), false);
}

Expand All @@ -101,7 +96,6 @@ public static ConnectProxyConfig connect(InetSocketAddress proxyAddress) {
*/
public static ConnectProxyConfig connect(InetSocketAddress proxyAddress, boolean useTls) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new ConnectProxyConfig(proxyAddress, null, null, HttpHeaders.of(), useTls);
}

Expand Down Expand Up @@ -129,7 +123,6 @@ public static ConnectProxyConfig connect(
public static ConnectProxyConfig connect(
InetSocketAddress proxyAddress, HttpHeaders headers, boolean useTls) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new ConnectProxyConfig(proxyAddress, null, null, headers, useTls);
}

Expand All @@ -146,7 +139,6 @@ public static ConnectProxyConfig connect(
public static ConnectProxyConfig connect(InetSocketAddress proxyAddress, String username, String password,
HttpHeaders headers, boolean useTls) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
requireNonNull(username, "username");
requireNonNull(password, "password");
requireNonNull(headers, "headers");
Expand All @@ -162,7 +154,6 @@ public static ConnectProxyConfig connect(InetSocketAddress proxyAddress, String
public static HAProxyConfig haproxy(
InetSocketAddress proxyAddress, InetSocketAddress sourceAddress) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
requireNonNull(sourceAddress, "sourceAddress");
checkArgument(!sourceAddress.isUnresolved(), "sourceAddress must be resolved");
return new HAProxyConfig(proxyAddress, sourceAddress);
Expand All @@ -176,7 +167,6 @@ public static HAProxyConfig haproxy(
*/
public static ProxyConfig haproxy(InetSocketAddress proxyAddress) {
requireNonNull(proxyAddress, "proxyAddress");
checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");
return new HAProxyConfig(proxyAddress);
}

Expand All @@ -201,6 +191,12 @@ public static ProxyConfig direct() {
@Nullable
public abstract InetSocketAddress proxyAddress();

/**
* Returns a new proxy address instance that respects DNS TTL.
* @param newProxyAddress the inet socket address
*/
public abstract ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress);

@Nullable
static String maskPassword(@Nullable String username, @Nullable String password) {
return username != null ? "****" : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public ProxyType proxyType() {
return ProxyType.SOCKS4;
}

@Override
public ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress) {
return new Socks4ProxyConfig(newProxyAddress, this.username);
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public ProxyType proxyType() {
return ProxyType.SOCKS5;
}

@Override
public ProxyConfig withProxyAddress(InetSocketAddress newProxyAddress) {
return new Socks5ProxyConfig(newProxyAddress, this.username, this.password);
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
Loading
Loading