Add Nacos Support #5409

KonaEspresso94 · 2024-01-25T16:19:04Z

Motivation:

Nacos is a service discovery application extensively used. By providing native support for Nacos, it will be beneficial to Armeria users.

#5360 #5365

Modification:

Add a nacos module.
Add NacosClient.
Its internal implementation includes LoginClient for obtaining an accessToken through basic Nacos authentication, QueryInstancesClient for querying service instances, and RegisterInstanceClient for handling the registration and deregistration of instances.
Implement NacosEndpointGroup and NacosUpdatingListener.
Configure unit tests using Testcontainers and the Nacos image to facilitate actual call testing.

Result:

Closes #5365

Nacos discovery and instance registration functionalities are now integrated into Armeria.

In writing this code, I was strongly influenced by the implementation on the consul and eureka modules.
I appreciate your review and feedback. Thank you.

CLAassistant · 2024-01-25T16:19:10Z

All committers have signed the CLA.

github-actions · 2024-01-26T06:38:13Z

🔍 Build Scan® (commit: `4e29132`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/qrgmhr3lgu7pa
build-self-hosted-unsafe-jdk-8	❌ (failure)	https://ge.armeria.dev/s/adns6mhwc26zu
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/m4ivgcmixj4e4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/3hd3u2wtz65l4
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/q4y3o6n42ef6m
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/tvq3duxbeidp2
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/weddgqcinnm52
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/cr7ldzhadihgs

…stead of QueryParams, making it easier to append additional parameters (e.g., accessToken).

ikhoon

The overall direction looks great.

ikhoon · 2024-02-07T07:54:41Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/LoginClient.java

+        String accessToken;
+
+        Integer tokenTtl;
+
+        @Nullable
+        Boolean globalAdmin;


Let's make them immutable.

Suggested change

String accessToken;

Integer tokenTtl;

@Nullable

Boolean globalAdmin;

private final String accessToken;

private final Integer tokenTtl;

@Nullable

private final Boolean globalAdmin;

ikhoon · 2024-02-07T08:41:10Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/LoginClient.java

+ * A Nacos client that is responsible for
+ * <a href="https://nacos.io/en-us/docs/auth.html">Nacos Authentication</a>.
+ */
+final class LoginClient {


The decorator pattern may hide the details of authentication.
How about making LoginClient as a decorator to decorate WebClient?

ikhoon · 2024-02-07T08:47:13Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/LoginClient.java

+
+    private static final String NACOS_ACCESS_TOKEN_CACHE_KEY = "NACOS_ACCESS_TOKEN_CACHE_KEY";
+    private final AsyncLoadingCache<String, LoginResult> tokenCache = Caffeine.newBuilder()
+            .maximumSize(1)


Optional) It seems overkill to use Caffeine only for 1 element.
What do you think of creating a function that triggers loginInternal() if access tokens are expired while invoking login()?

If you find it more difficult than Caffeine approach, I'm okay to keep this as is.

ikhoon · 2024-02-07T08:50:03Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/QueryInstancesClient.java

+        final StringBuilder path = new StringBuilder("/").append(nacosApiVersion).append("/ns/instance/list?");
+        final QueryParamsBuilder paramsBuilder = NacosClientUtil
+                .queryParamsBuilder(namespaceId, groupName, serviceName, clusterName, healthyOnly, app, null,
+                                    null, null);


It would be inefficient to create QueryParams for the same parameters for all requests.
How about creating and caching the QueryParams as a member field when QueryInstancesClient is created?

ikhoon · 2024-02-07T08:51:04Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/QueryInstancesClient.java

+                    .as(HttpEntity::content)
+                    .execute();
+        } else {
+            return loginClient.login().thenCompose(accessToken ->


As I mentioned above, would you make loginClient as a decorator?

ikhoon · 2024-02-07T08:51:34Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/QueryInstancesClient.java

+    @JsonIgnoreProperties(ignoreUnknown = true)
+    private static final class Host {
+        @Nullable
+        String instanceId;


Ditto. Let's make them immutable.

… immutable.

Hide login-related context from other clients as much as possible.

…peatedly produced.

…sarily repeatedly produced.

…ll immutable parameters to properties.

…ilder instead"

…ntation.

KonaEspresso94 · 2024-02-11T11:13:03Z

@ikhoon

Thank you for your review comments. I've applied your suggestions into the code.

LoginClient now implements SimpleDecoratingHttpClient. Contexts related to 'nacos basic authentication' have been hidden as much as possible from other clients.
Caching access token by own implementation instead of Caffeine cache.
Changed the query string that was being created each time in LoginClient and QueryInstanceClient to be reused efficiently.
NacosUpdatingListener and RegisterInstanceClient are not guaranteed to always have the same endpoint used as parameter. Eventually, we will need to create a StringBuilder or QueryParamsBuilder. However, I have minimized the parameter of the API by turning immutable parameters into properties, such as in the above two clients.
Fixed some missing final.

The implementation to replace the Caffein, I made it as simple as possible. So now the login api can be called concurrently. Considering the usability of NacosUpdatingListener and NacosEndpointGroup, I think it's unlikely that concurrent calls will actually happen, and if they do, it won't be an issue. But I'm not sure. I'd be interested in your thoughts.

Thank you!

ikhoon · 2024-03-15T02:14:52Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/LoginClient.java

+                return UnmodifiableFuture.completedFuture(cacheEntry.loginResult.accessToken);
+            }
+
+            return login().thenApply(loginResult -> {


This may result in sending multiple login requests that are not what we really want.
If login() is being performed, other requests can piggyback on the returned future.

By the way, I think an AsyncLoader loading a value when it is expired or periodically would be useful in many places.
Created #5506

Maybe we shouldn't let @KonaEspresso94 wait for too long for the PR to be merged but just use Caffeine internally and later replace the Caffeine usage with AsyncLoader?

I didn't know it would take this long. 😅 Let's leave that part as is. I will continue to review this PR tomorrow.

Thank you for your consideration. Following your comments, I have reverted the commit to use Caffeine instead of my implementation.

trustin · 2024-05-08T05:40:24Z

@KonaEspresso94 Gentle ping - looking forward to having this feature merged 🙇

… implementation." This reverts commit 0120fd7. excluding linting style.

…ry 'this.')

jrhee17

Direction looks good to me 👍 Left some minor comments

jrhee17 · 2024-06-17T02:49:19Z

nacos/src/main/java/com/linecorp/armeria/client/nacos/NacosEndpointGroup.java

+ * using <a href="https://nacos.io/en-us/docs/v2/guide/user/open-api.html">Nacos's HTTP Open API</a>
+ * and updates the {@link Endpoint}s periodically.
+ */
+public class NacosEndpointGroup extends DynamicEndpointGroup {


Question) Can NacosEndpointGroup be package-private?

final class NacosEndpointGroup { ... public static EndpointGroup of(URI }

jrhee17 · 2024-06-17T03:11:51Z

nacos/src/main/java/com/linecorp/armeria/client/nacos/NacosEndpointGroup.java

+    private final long registryFetchIntervalMillis;
+
+    @Nullable
+    private volatile ScheduledFuture<?> scheduledFuture;


nit; I don't think we need to worry about race conditions when closing the EndpointGroup if we pre-choose an EventExecutor

private final EventExecutor eventLoop; ... NacosEndpointGroup(...) { eventLoop = CommonPools.workerGroup().next(); } ... protected void doCloseAsync(CompletableFuture<?> future) { if (!eventExecutor.inEventLoop()) { eventExecutor.execute(() -> doCloseAsync(future)); } ...

jrhee17 · 2024-06-17T03:15:16Z

nacos/src/main/java/com/linecorp/armeria/client/nacos/NacosEndpointGroupBuilder.java

+ * sb.serverListener(listener);
+ * }</pre>
+ */
+public class NacosEndpointGroupBuilder extends AbstractDynamicEndpointGroupBuilder {


Suggested change

public class NacosEndpointGroupBuilder extends AbstractDynamicEndpointGroupBuilder {

public final class NacosEndpointGroupBuilder extends AbstractDynamicEndpointGroupBuilder<NacosEndpointGroupBuilder> {

jrhee17 · 2024-06-17T07:05:19Z

nacos/src/main/java/com/linecorp/armeria/common/nacos/NacosConfigSetters.java

+ * Sets properties for building a Nacos client.
+ */
+@UnstableApi
+public interface NacosConfigSetters {


optional; What do you think of generalizing this interface and adding the related methods?

e.g.

public interface NacosConfigSetters<SELF extends NacosConfigSetters<SELF>> { SELF nacosApiVersion(...) }

jrhee17 · 2024-06-17T07:14:14Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/QueryInstancesClient.java

+                        .map(QueryInstancesClient::toEndpoint)
+                        .filter(Objects::nonNull)
+                        .collect(toImmutableList()));


Do you mind running an auto-indent for files in this PR in general one more time?

jrhee17 · 2024-06-17T07:22:12Z

nacos/src/main/java/com/linecorp/armeria/internal/nacos/QueryInstancesClient.java

+
+    CompletableFuture<List<Endpoint>> endpoints() {
+        return queryInstances()
+                .thenApply(response -> response.data.hosts.stream()


Question) Just to make sure, even if code != 0 (which I assume means successful) will data and hosts always exist?
I'm asking because there doesn't seem to defence against null check

jrhee17 · 2024-06-17T07:39:05Z

nacos/src/test/java/com/linecorp/armeria/internal/nacos/NacosTestBase.java

+        return nacosUri;
+    }
+
+    protected static int[] unusedPorts(int numPorts) {


To other reviewers) I guess the ideal approach (binding to port 0) can be done for all similar tests at once. I think this code is fine if the CI passes reliably at least for now.

jrhee17 · 2024-06-17T07:42:57Z

nacos/src/main/java/com/linecorp/armeria/client/nacos/NacosEndpointGroupBuilder.java

+    @Override
+    public NacosEndpointGroupBuilder allowEmptyEndpoints(boolean allowEmptyEndpoints) {
+        return (NacosEndpointGroupBuilder) super.allowEmptyEndpoints(allowEmptyEndpoints);
+    }
+
+    /**
+     * Sets the timeout to wait until a successful {@link Endpoint} selection.
+     * {@link Duration#ZERO} disables the timeout.
+     * If unspecified, {@link Flags#defaultResponseTimeoutMillis()} is used by default.
+     */
+    @Override
+    public NacosEndpointGroupBuilder selectionTimeout(Duration selectionTimeout) {
+        return (NacosEndpointGroupBuilder) super.selectionTimeout(selectionTimeout);
+    }
+
+    /**
+     * Sets the timeout to wait until a successful {@link Endpoint} selection.
+     * {@code 0} disables the timeout.
+     * If unspecified, {@link Flags#defaultResponseTimeoutMillis()} is used by default.
+     */
+    @Override
+    public NacosEndpointGroupBuilder selectionTimeoutMillis(long selectionTimeoutMillis) {
+        return (NacosEndpointGroupBuilder) super.selectionTimeoutMillis(selectionTimeoutMillis);
+    }


This is unrelated to your PR, but we can probably remove these lines now

…a single eventLoop, instead of using volatile.

KonaEspresso94 added 3 commits January 26, 2024 00:43

Add NacosClient and NacosEndpointGroup

785b353

Add NacosUpdatingListener

a76d4fc

Test codes

4caabaf

KonaEspresso94 requested review from ikhoon, jrhee17, minwoox and trustin as code owners January 25, 2024 16:19

ikhoon added the new feature label Jan 26, 2024

KonaEspresso94 and others added 2 commits January 27, 2024 16:59

Modified NacosClientUtil::queryParams to return QueryParamsBuilder in…

3af6f6c

…stead of QueryParams, making it easier to append additional parameters (e.g., accessToken).

Merge branch 'main' into nacos-support

d4332aa

ikhoon reviewed Feb 7, 2024

View reviewed changes

KonaEspresso94 marked this pull request as draft February 9, 2024 13:58

KonaEspresso94 added 7 commits February 10, 2024 20:17

Make properties of the classes that define data rendering private and…

9530372

… immutable.

Make LoginClient an implementation of SimpleDecoratingHttpClient.

ffc64d7

Hide login-related context from other clients as much as possible.

LoginClient : Cache and reuse QueryStrings that were unnecessarily re…

68e1137

…peatedly produced.

QueryInstancesClient : Cache and reuse request path that were unneces…

0f71869

…sarily repeatedly produced.

Reduce the parameters of the RegisterInstanceClient APIS, by moving a…

46abd6d

…ll immutable parameters to properties.

Revert "Modified NacosClientUtil::queryParams to return QueryParamsBu…

adb40fc

…ilder instead"

To reuse access-token, instead of the Caffeine cache, use own impleme…

0120fd7

…ntation.

KonaEspresso94 force-pushed the nacos-support branch from a5e4a8c to 0120fd7 Compare February 11, 2024 06:52

KonaEspresso94 requested a review from ikhoon February 11, 2024 11:13

KonaEspresso94 marked this pull request as ready for review February 11, 2024 11:13

Merge branch 'main' into nacos-support

12b2214

ikhoon reviewed Mar 15, 2024

View reviewed changes

github-actions bot added the Stale label Apr 15, 2024

Merge branch 'main' into nacos-support

23d8913

github-actions bot removed the Stale label May 9, 2024

KonaEspresso94 and others added 5 commits June 16, 2024 13:39

Merge branch 'main' into nacos-support

0d0082d

Revert "To reuse access-token, instead of the Caffeine cache, use own…

a0d7895

… implementation." This reverts commit 0120fd7. excluding linting style.

Lint code. (omitted 'final', meaningless lambda expression, unnecessa…

a427074

…ry 'this.')

Lint code. (Remove meaningless property and use parameter directly)

625535a

Lint code. (Optimize imports, according to :nacos:checkStyleMain)

ed1d5ba

KonaEspresso94 requested a review from ikhoon June 16, 2024 06:15

KonaEspresso94 added 2 commits June 16, 2024 15:25

Copyright header: Line Corporation -> LY Corporation

f94cf66

TODO comment about AsyncLoadingCache

c10ebc8

jrhee17 added this to the 1.30.0 milestone Jun 17, 2024

jrhee17 reviewed Jun 17, 2024

View reviewed changes

KonaEspresso94 added 6 commits June 19, 2024 13:59

Hide NacosEndpointGroup as package private.

5fe5ba2

Apply AbstractDynamicEndpointGroupBuilder's changes(SELF type parameter)

03150f2

Use SELF type parameter for NacosConfigSetters.

88f6235

Lint code: Run auto-indent.

0a715cd

NacosEndpointGroup : Batch the tasks related to scheduledFuture into …

7476ede

…a single eventLoop, instead of using volatile.

lint code.

4e29132

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Nacos Support #5409

Add Nacos Support #5409

KonaEspresso94 commented Jan 25, 2024 •

edited

Loading

CLAassistant commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Feb 7, 2024

ikhoon Feb 7, 2024

ikhoon Feb 7, 2024

ikhoon Feb 7, 2024

ikhoon Feb 7, 2024

ikhoon Feb 7, 2024

KonaEspresso94 commented Feb 11, 2024

ikhoon Mar 15, 2024

trustin Jun 6, 2024 •

edited

Loading

ikhoon Jun 10, 2024

KonaEspresso94 Jun 16, 2024 •

edited

Loading

trustin commented May 8, 2024

jrhee17 left a comment •

edited

Loading

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

jrhee17 Jun 17, 2024

	public class NacosEndpointGroupBuilder extends AbstractDynamicEndpointGroupBuilder {
	public final class NacosEndpointGroupBuilder extends AbstractDynamicEndpointGroupBuilder<NacosEndpointGroupBuilder> {

Add Nacos Support #5409

Are you sure you want to change the base?

Add Nacos Support #5409

Conversation

KonaEspresso94 commented Jan 25, 2024 • edited Loading

CLAassistant commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 26, 2024 • edited Loading

🔍 Build Scan® (commit: 4e29132)

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

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

KonaEspresso94 commented Feb 11, 2024

Choose a reason for hiding this comment

trustin Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonaEspresso94 Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

trustin commented May 8, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonaEspresso94 commented Jan 25, 2024 •

edited

Loading

CLAassistant commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024 •

edited

Loading

🔍 Build Scan® (commit: `4e29132`)

trustin Jun 6, 2024 •

edited

Loading

KonaEspresso94 Jun 16, 2024 •

edited

Loading

jrhee17 left a comment •

edited

Loading