Respect TTL of a DNS record for proxy config #5960

chickenchickenlove · 2024-10-27T07:07:27Z

Motivation:

From now on, armeria client ignores DNS TTL.
Thus the client will keep sending the request to the old proxy.

Modifications:

Make InetSocketAddress mutable to support a feature which update DNS.
Add fields to xxxProxyConfig (lastUpdatedTime, Schedulers)
Add a method for DNS update scheduling.
Add create method which supports refreshInterval

JVM doesn't consider DNS TTL as default.
Therefore, client has a intent to DNS update,
IMHO, Client should configure JVM options.

# java.security
networkaddress.cache.ttl=...
networkaddress.cache.negative.ttl=...

# Command line
-Dsun.net.inetaddr.ttl=...

Result:

Fixes Respect TTL of a DNS record for proxy config. #5843

github-actions · 2024-10-27T07:40:07Z

🔍 Build Scan® (commit: `195033f`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/t2hjiavw4g6yy
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/wb43g5xo3uc42
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/jhmvhtgau3kzg
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/fzkxuly754tee
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/4nuidfxhtlk7w
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/6ixi4q6f2qxtk
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/zzsuvllohwoy4

minwoox · 2024-10-30T02:00:30Z

Hi, @chickenchickenlove! Thanks for submitting this PR.

There is a limitation in the current patch:

It still does not respect TTL.
Users would need to carefully consider and set TTL values appropriate to each use case.

To address this, we need to ensure TTL is respected and refresh the address once TTL expires.
Luckily, we already have the implementation for this: the RefreshingAddressResolver. The resolver is created internally in a ClientFactory so we all have to do is make use of it. We can integrate it here as follows:

final ProxyConfig proxyConfig = factory.proxyConfigSelector().select(protocol, endpoint);
final Future<InetSocketAddress> resolveFuture = addressResolverGroup.getResolver(
        ctx.eventLoop().withoutContext()).resolve(proxyConfig.proxyAddress());
if (resolveFuture.isSuccess()) {
    ...
} else {
    resolveFuture.addListener(...);
}

Because we resolve the address internally, we can now accept unresolved addresses as well:

armeria/core/src/main/java/com/linecorp/armeria/client/proxy/ProxyConfig.java

Line 53 in 417bf3b

checkArgument(!proxyAddress.isUnresolved(), "proxyAddress must be resolved");

Please, let me know if you need any further information. 😉

This reverts commit 417bf3b.

…kenlove/armeria into 241027-proxy-config

minwoox · 2024-10-31T09:35:26Z

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

@@ -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) {


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

@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 🙇‍♂️

jrhee17

Looks good overall, left some minor comments 🙇

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

jrhee17 · 2024-11-04T02:31:54Z

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

+                        final ProxyConfig newProxyConfig = proxyConfig.withNewProxyAddress(resolvedAddress);
+                        onComplete.accept(maybeHAProxy(newProxyConfig, capturedProxiedAddresses), null);
+                    } else {
+                        logger.warn("Resolved address is invalid or unresolved: {}. " +


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)?

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.

jrhee17 · 2024-11-04T02:32:32Z

core/src/main/java/com/linecorp/armeria/client/proxy/DirectProxyConfig.java

@@ -40,6 +40,11 @@ public InetSocketAddress proxyAddress() {
        return null;
    }

+    @Override
+    public ProxyConfig withNewProxyAddress(InetSocketAddress newProxyAddress) {
+        return new DirectProxyConfig();


nit; it may make more sense to throw an exception instead

jrhee17 · 2024-11-04T02:33:30Z

core/src/main/java/com/linecorp/armeria/client/proxy/ProxyConfig.java

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


nit; I believe that currently only resolved InetSocketAddress can be used to create a ProxyConfig.
Can you add some integration tests to confirm the current code works correctly?

@jrhee17 Thanks for your comments. 🙇‍♂️
I added two integration test codes for this feature.
Due to restrictions on mocking, narrow Access specifier, I couldn't use a spy instance to validate it or inject DNSResolver as well.
That's why I made a new class called DNSResolverFacadeUtils class 😅 .

As @minwoox mentioned, we don't need to check if InetSocketAddress is resolved, since it will be resolved when HttpClientDelegate calls execute(). After removing checkArgument(...), I was finally able to write some integration test cases... 😅

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

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

core/src/main/java/com/linecorp/armeria/client/proxy/DirectProxyConfig.java

ikhoon · 2024-11-11T09:33:49Z

core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyConfig.java

+        return this.sourceAddress == null ? new HAProxyConfig(proxyAddress)
+                                          : new HAProxyConfig(proxyAddress, this.sourceAddress);


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

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

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

core/src/main/java/com/linecorp/armeria/client/proxy/ProxyConfig.java

ikhoon · 2024-11-11T09:41:06Z

core/src/test/java/com/linecorp/armeria/client/proxy/ProxyConfigTest.java

@@ -44,19 +44,6 @@ void testEqualityAndHashCode(ProxyConfig config) {
        assertThat(equalProxyConfigs.get(0).hashCode()).isEqualTo(config.hashCode());
    }

-    @Test
-    void testUnresolvedProxyAddress() {


Should we revive this test and fix it to check if ProxyConfig is successfully created with unresolved addresses?

Thanks for your comments 🙇‍♂️
Good point, I didn't expect it needed to be kept at all. 😓
I revived this test and fixed it!

chickenchickenlove · 2024-11-12T00:40:48Z

@ikhoon nim, thanks for your comments! 🙇‍♂️
I made a new commit to apply your comments.
When you have time, please take another look!

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

ikhoon · 2024-11-19T15:51:16Z

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

@@ -215,24 +224,48 @@ 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);


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

ikhoon · 2024-11-21T09:09:15Z

core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyConfig.java

-        return Objects.hash(proxyAddress, sourceAddress);
+        return hash(proxyAddress, sourceAddress);


Have been reverted!

core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

chickenchickenlove · 2024-11-21T13:06:13Z

core/src/main/java/com/linecorp/armeria/internal/common/util/IpAddrUtil.java

+
+        final InetAddress inetAddress = socketAddress.getAddress();
+        // If hostname and host address are the same, it was created with an IP address
+        return socketAddress.getHostString().equals(inetAddress.getHostAddress());


@ikhoon nim
I removed this code because of test fail.

final InetAddress inetAddress = socketAddress.getAddress(); if (inetAddress.isAnyLocalAddress()) { return false; // Wildcard address }

If IP is wildcard address, addressResolver start to resolve wildcard address like 0.0.0.0, ::.
In that case, The addressResolver try to resolve 0.0.0.0 and throws UnknownHostException.
The error message is Caused by: java.net.UnknownHostException: Failed to resolve '0.0.0.0.akadns.net.' [A(1)] after 2 queries.

Please refer to HAProxyClientIntegrationTest#testConnectionFailure(). It's failed test reason described above.

I think that we don't need to resolve either 0.0.0.0 or ::.
Also, I think 0.0.0.0 and :: are fixed IP address.

What do you think?

I agreed. Thanks for fixing that.

core/src/test/java/com/linecorp/armeria/internal/common/util/IpAddrUtilTest.java

chickenchickenlove · 2024-11-21T13:14:06Z

@ikhoon nim, thanks for your clean up commit.
It's more readable 👍 👍 👍 .
By the way, I found unexpected failed two test cases.

The one is fixed.
The other one is fixed as well. However, to fix it, I make a change IpAddrUtil.isCreatedWithIpAddressOnly().
Because the AddressResolver attempts to resolve considering wildcard domains and even akamai CDN addresses.
I have updated the commit with my thoughts on this matter, so I would appreciate your feedback.

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

ikhoon

Thanks, @chickenchickenlove!

ikhoon · 2024-11-21T15:32:45Z

core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyConfig.java

+            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. " +


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

I agree. I removed it!

Client respects DNS TTL.

417bf3b

chickenchickenlove requested review from ikhoon, jrhee17, minwoox and trustin as code owners October 27, 2024 07:07

chickenchickenlove mentioned this pull request Oct 27, 2024

Respect TTL of a DNS record for proxy config. #5843

Open

chickenchickenlove closed this Oct 27, 2024

chickenchickenlove reopened this Oct 27, 2024

Merge branch 'main' into 241027-proxy-config

3681f0f

minwoox added the defect label Oct 30, 2024

chickenchickenlove added 5 commits October 30, 2024 22:08

Revert "Client respects DNS TTL."

f57259c

This reverts commit 417bf3b.

Use RefreshingAddressResolver to refresh proxy address.

f40cbe4

Merge branch '241027-proxy-config' of github.com-ojt90902:chickenchic…

a26e3f5

…kenlove/armeria into 241027-proxy-config

Handle null of proxyconfig.

649469e

Add a copmment.

ab43277

minwoox reviewed Oct 31, 2024

View reviewed changes

chickenchickenlove added 5 commits November 2, 2024 23:04

Add withNewProxyAddress() to create after refresh DNS.

a6e725c

Resolve Proxy DNS to respect DNS TTL asynchronously.

e0819c4

Remove useless method from ProxyConfig.

0b3cc2b

Fixes lint error.

54ec6f3

Revert previous commit.

a67ee3f

jrhee17 reviewed Nov 4, 2024

View reviewed changes

chickenchickenlove added 4 commits November 4, 2024 21:32

Remove useless log codes.

241b4ff

Remove unused import.

fce4fd2

Fixes lint error.

7d85a69

Add integration test for DNS refreshing.

67f4997

chickenchickenlove requested review from minwoox and jrhee17 November 4, 2024 15:35

Remove deprecated test case.

e775351

jrhee17 added this to the 1.32.0 milestone Nov 6, 2024

ikhoon reviewed Nov 11, 2024

View reviewed changes

Apply reviews.

18b24ea

Add a private method to refresh proxy DNS.

979e6fd

chickenchickenlove requested a review from ikhoon November 17, 2024 08:58

ikhoon reviewed Nov 19, 2024

View reviewed changes

Skip refreshing ProxyConfig DNS when it is configured with a direct IP.

3dbc847

chickenchickenlove requested a review from ikhoon November 21, 2024 00:30

ikhoon reviewed Nov 21, 2024

View reviewed changes

ikhoon and others added 3 commits November 21, 2024 18:59

clean up

fcfed45

add more tests

09efa86

Fixes broken test codes and add new test cases.

25ff1af

chickenchickenlove commented Nov 21, 2024

View reviewed changes

core/src/test/java/com/linecorp/armeria/internal/common/util/IpAddrUtilTest.java Show resolved Hide resolved

ikhoon approved these changes Nov 21, 2024

View reviewed changes

chickenchickenlove added 2 commits November 22, 2024 10:02

Remove useless logging.

43212b6

Add null condition.

195033f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect TTL of a DNS record for proxy config #5960

Respect TTL of a DNS record for proxy config #5960

chickenchickenlove commented Oct 27, 2024 •

edited

Loading

github-actions bot commented Oct 27, 2024 •

edited

Loading

minwoox commented Oct 30, 2024

minwoox Oct 31, 2024

chickenchickenlove Nov 3, 2024

jrhee17 left a comment

jrhee17 Nov 4, 2024

chickenchickenlove Nov 4, 2024

jrhee17 Nov 4, 2024

jrhee17 Nov 4, 2024

chickenchickenlove Nov 4, 2024 •

edited

Loading

ikhoon Nov 11, 2024

ikhoon Nov 21, 2024

chickenchickenlove Nov 21, 2024

ikhoon Nov 11, 2024

chickenchickenlove Nov 11, 2024

chickenchickenlove commented Nov 12, 2024

ikhoon Nov 19, 2024

ikhoon Nov 21, 2024

chickenchickenlove Nov 21, 2024

chickenchickenlove Nov 21, 2024 •

edited

Loading

ikhoon Nov 21, 2024

chickenchickenlove commented Nov 21, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Nov 21, 2024

chickenchickenlove Nov 22, 2024

		return this.sourceAddress == null ? new HAProxyConfig(proxyAddress)
		: new HAProxyConfig(proxyAddress, this.sourceAddress);

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

		return Objects.hash(proxyAddress, sourceAddress);
		return hash(proxyAddress, sourceAddress);

Respect TTL of a DNS record for proxy config #5960

Are you sure you want to change the base?

Respect TTL of a DNS record for proxy config #5960

Conversation

chickenchickenlove commented Oct 27, 2024 • edited Loading

Motivation:

Modifications:

Result:

github-actions bot commented Oct 27, 2024 • edited Loading

🔍 Build Scan® (commit: 195033f)

minwoox commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a 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

chickenchickenlove Nov 4, 2024 • 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

chickenchickenlove commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenchickenlove Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenchickenlove commented Nov 21, 2024 • edited Loading

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenchickenlove commented Oct 27, 2024 •

edited

Loading

github-actions bot commented Oct 27, 2024 •

edited

Loading

🔍 Build Scan® (commit: `195033f`)

chickenchickenlove Nov 4, 2024 •

edited

Loading

chickenchickenlove Nov 21, 2024 •

edited

Loading

chickenchickenlove commented Nov 21, 2024 •

edited

Loading