Respect TTL of a DNS record for proxy config by chickenchickenlove · Pull Request #5960 · line/armeria

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: `0fc757c`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/uom2qia3cgcv6
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/kzndhig7ph2ec
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/o5im5raqittwy
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/uuqe5dz5fwe6c
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/gluk4hsxbq4rq
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/xc5lxkawosyhm
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/tri6pyoefdkjs

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

    }

-    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 🙇

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

+                        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


+    @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

+     * 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. 🙇‍♂️

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

+        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!

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

    }

-    @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!

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

-    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

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


Have been reverted!

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

+            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!

minwoox

Sorry about the late review.
It looks great now. 👍

…ate.java Co-authored-by: minux <[email protected]>

…nfig.java Co-authored-by: minux <[email protected]>

minwoox · 2025-02-11T06:59:58Z

@jrhee17 Please, review this PR when you have time. 😉

jrhee17 · 2025-02-12T05:12:43Z

+                                                    DecodedHttpResponse response) {
+        final UnprocessedRequestException cause = UnprocessedRequestException.of(t);
+        ctx.cancel(cause);
+        response.close(cause);


nit; ctx.cancel calls response.close since the cancellation task is updated. The current implementation is fine though.

minwoox · 2025-02-21T05:17:28Z

Great job, @chickenchickenlove! 👏 👏 👏

chickenchickenlove · 2025-02-24T08:31:39Z

Thanks a lot for reviewing my PR. 🙇‍♂️

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

Closed

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 jrhee17 and minwoox November 4, 2024 15:35

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

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

chickenchickenlove commented Nov 21, 2024

View reviewed changes

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

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

minwoox approved these changes Nov 28, 2024

View reviewed changes

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

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

chickenchickenlove and others added 3 commits November 28, 2024 17:06

Update core/src/main/java/com/linecorp/armeria/client/HttpClientDeleg…

d1c9596

…ate.java Co-authored-by: minux <[email protected]>

Update core/src/main/java/com/linecorp/armeria/client/proxy/HAProxyCo…

d5c8326

…nfig.java Co-authored-by: minux <[email protected]>

Remove unused import

1240a24

github-actions Bot added the Stale label Dec 31, 2024

github-actions Bot removed the Stale label Feb 12, 2025

Merge branch 'main' into 241027-proxy-config

0fc757c

jrhee17 approved these changes Feb 12, 2025

View reviewed changes

minwoox merged commit fd540d4 into line:main Feb 21, 2025

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

Conversation

chickenchickenlove commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

github-actions Bot commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 0fc757c)

Uh oh!

minwoox commented Oct 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chickenchickenlove Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chickenchickenlove commented Nov 12, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chickenchickenlove commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minwoox commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chickenchickenlove commented Oct 27, 2024 •

edited

Loading

github-actions Bot commented Oct 27, 2024 •

edited

Loading

🔍 Build Scan® (commit: `0fc757c`)

chickenchickenlove Nov 4, 2024 •

edited

Loading

chickenchickenlove commented Nov 21, 2024 •

edited

Loading