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

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

Respect TTL of a DNS record for proxy config #5960

Show file tree
Hide file tree
Changes from 7 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
32 changes: 30 additions & 2 deletions core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import java.net.InetSocketAddress;
import java.util.function.BiConsumer;

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

import com.linecorp.armeria.client.HttpChannelPool.PoolKey;
import com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException;
import com.linecorp.armeria.client.proxy.HAProxyConfig;
Expand Down Expand Up @@ -50,6 +53,7 @@

final class HttpClientDelegate implements HttpClient {

private static final Logger logger = LoggerFactory.getLogger(HttpClientDelegate.class);
private final HttpClientFactory factory;
private final AddressResolverGroup<InetSocketAddress> addressResolverGroup;

Expand Down Expand Up @@ -90,7 +94,7 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex
final SessionProtocol protocol = ctx.sessionProtocol();
final ProxyConfig proxyConfig;
try {
proxyConfig = getProxyConfig(protocol, endpoint);
proxyConfig = getProxyConfig(protocol, endpoint, ctx);
} catch (Throwable t) {
return earlyFailedResponse(t, ctx);
}
Expand Down Expand Up @@ -215,10 +219,34 @@ private void acquireConnectionAndExecute0(ClientRequestContext ctx, Endpoint end
}
}

private ProxyConfig getProxyConfig(SessionProtocol protocol, Endpoint endpoint) {
private ProxyConfig getProxyConfig(SessionProtocol protocol, Endpoint endpoint, ClientRequestContext ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to recreate ProxyConfig with the new proxy address because the address is resolved:

if (proxyConfig.proxyAddress() != null) {
    final Future<InetSocketAddress> resolveFuture = addressResolverGroup.getResolver(
            ctx.eventLoop().withoutContext()).resolve(proxyConfig.proxyAddress());

    resolveFuture.addListener(future -> {
        if (future.isSuccess()) {
            final ProxyConfig newProxyConfig = proxyConfig.withNewProxyAddress(
                    (InetSocketAddress) future.get());
            ...
        }
    }
}

Because getting the ProxyConfig is now asynchronous, we have to change the signature of this method.
We have two options:

  • Returning CompletableFuture<ProxyConfig> instead of ProxyConfig 
    // in HttpClientDelegate.execute()
try {
    final CompletableFuture<ProxyConfig> future = getProxyConfig(protocol, endpoint, ctx);
    future.handle(....)
} catch (Throwable t) {
    return earlyFailedResponse(t, ctx);
}
  • Using a callback that is executed after resolution like we do for resolving address 
    private void resolveProxyConfig(..., 
    BiConsumer<@Nullable ProxyConig, @Nullable Throwable> onComplete) {
    final ProxyConfig proxyConfig = factory.proxyConfigSelector().select(protocol, endpoint);
      requireNonNull(proxyConfig, "proxyConfig");

      ...
}

Please let me know if you need further information. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@minwoox , Thanks for your comments 🙇‍♂️
I made a new commit with following second option!

Because ProxyConfig will be resolved in other thread, so the method maybeHAProxy() will get capturedProxyAddress instead of ServiceRequestContext.
Also, res instance will be returned right away even if future don't completed.
So I used the method earlyFailed(...) instead of earlyfailedResponse(...).

When you have time, Please take another look 🙇‍♂️

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");

// DirectProxyConfig does not have proxyAddress as its field.
if (proxyConfig.proxyAddress() != null) {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
final Future<InetSocketAddress> resolveFuture = addressResolverGroup.getResolver(
ctx.eventLoop().withoutContext()).resolve(proxyConfig.proxyAddress());

resolveFuture.addListener(future -> {
if (future.isSuccess()) {
final InetSocketAddress resolvedAddress = (InetSocketAddress) future.get();
if (resolvedAddress != null && !resolvedAddress.isUnresolved()) {
proxyConfig.refreshDns(resolvedAddress);
} else {
logger.warn("Resolved address is invalid or unresolved: {}. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand this case - are you concerned that the resolution succeeds but the InetAddress does not exist?

Would it make sense to just align error handling with dns resolution for the normal code path (not using proxy)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments!
After take a look RefreshingAddressResolver, RefreshingAddressResolver returns InetSocketAddress when succeed.
As you mentioned before, it is unnecessary codes.
Therefore, I delete this codes.

"Using previous address instead.", resolvedAddress);
}
} else {
final Throwable cause = future.cause();
logger.warn("Failed to refresh {}'s ip address. " +
"Use the previous inet address instead. reason: {}",
proxyConfig.proxyAddress(),
cause != null ? cause.getMessage() : "Unknown Error");
}
});
}

// special behavior for haproxy when sourceAddress is null
if (proxyConfig.proxyType() == ProxyType.HAPROXY &&
((HAProxyConfig) proxyConfig).sourceAddress() == null) {
Expand Down
10 changes: 9 additions & 1 deletion core/src/main/java/com/linecorp/armeria/client/proxy/ConnectProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.client.proxy;

import static java.util.Objects.requireNonNull;

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

Expand All @@ -29,7 +31,7 @@
*/
public final class ConnectProxyConfig extends ProxyConfig {

private final InetSocketAddress proxyAddress;
private InetSocketAddress proxyAddress;

@Nullable
private final String username;
Expand Down Expand Up @@ -90,6 +92,12 @@ public ProxyType proxyType() {
return ProxyType.CONNECT;
}

@Override
public void refreshDns(InetSocketAddress socketAddress) {
requireNonNull(socketAddress, "socketAddress");
this.proxyAddress = socketAddress;
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/proxy/DirectProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ public InetSocketAddress proxyAddress() {
return null;
}

@Override
public void refreshDns(InetSocketAddress socketAddress) {
// Do nothing.
}

@Override
public String toString() {
return "DirectProxyConfig{proxyType=DIRECT}";
Expand Down
9 changes: 8 additions & 1 deletion core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.linecorp.armeria.client.proxy;

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

import java.net.InetSocketAddress;
import java.util.Objects;
Expand All @@ -31,7 +32,7 @@
*/
public final class HAProxyConfig extends ProxyConfig {

private final InetSocketAddress proxyAddress;
private InetSocketAddress proxyAddress;

@Nullable
private final InetSocketAddress sourceAddress;
Expand Down Expand Up @@ -67,6 +68,12 @@ public InetSocketAddress sourceAddress() {
return sourceAddress;
}

@Override
public void refreshDns(InetSocketAddress socketAddress) {
requireNonNull(socketAddress, "socketAddress");
this.proxyAddress = socketAddress;
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/proxy/ProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,12 @@ public static ProxyConfig direct() {
*/
public abstract ProxyType proxyType();

/**
* Refreshes the proxyAddress which ProxyConfig has, to respect DNS TTL.
* @param socketAddress the inet socket address
*/
public abstract void refreshDns(InetSocketAddress socketAddress);

/**
* Returns the proxy address which is {@code null} only for {@link ProxyType#DIRECT}.
*/
Expand Down
10 changes: 9 additions & 1 deletion core/src/main/java/com/linecorp/armeria/client/proxy/Socks4ProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.client.proxy;

import static java.util.Objects.requireNonNull;

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

Expand All @@ -28,7 +30,7 @@
*/
public final class Socks4ProxyConfig extends ProxyConfig {

private final InetSocketAddress proxyAddress;
private InetSocketAddress proxyAddress;

@Nullable
private final String username;
Expand Down Expand Up @@ -56,6 +58,12 @@ public ProxyType proxyType() {
return ProxyType.SOCKS4;
}

@Override
public void refreshDns(InetSocketAddress socketAddress) {
requireNonNull(socketAddress, "socketAddress");
this.proxyAddress = socketAddress;
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
Expand Down
10 changes: 9 additions & 1 deletion core/src/main/java/com/linecorp/armeria/client/proxy/Socks5ProxyConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.client.proxy;

import static java.util.Objects.requireNonNull;

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

Expand All @@ -28,7 +30,7 @@
*/
public final class Socks5ProxyConfig extends ProxyConfig {

private final InetSocketAddress proxyAddress;
private InetSocketAddress proxyAddress;

@Nullable
private final String username;
Expand Down Expand Up @@ -69,6 +71,12 @@ public ProxyType proxyType() {
return ProxyType.SOCKS5;
}

@Override
public void refreshDns(InetSocketAddress socketAddress) {
requireNonNull(socketAddress, "socketAddress");
this.proxyAddress = socketAddress;
}

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