feat: add support for multiple nacos instances and replace events library with shdict #12263

Revolyssup · 2025-05-28T08:23:00Z

Description

Currently nacos discovery uses event library and during testing it was found out that when using lua-resty-events, sometimes not all workers get the events. And inconsistencies emerge. Moreover the idiomatic way to share data between workers is through a shared dict. Therefore this PR migrates nacos from older events mechanism to newer shared dict way. Now only priviliged agent will fetch data from nacos and write to shdict while all workers will read from it.
Currently nacos supports only a single nacos registry but users have use case of supporting multiple nacos registries. This PR adds support to specify discovery.nacos as an array of nacos instances each with unique ID.

NOTE: The older way of configuring nacos is still supported and older data will be valid. The older configuration now gets converted internally to new configuration before being passed onto the logic therefore making this change backwards compatible. Meaning users can use the single-instance configuration as well as multi instance configuration.

Fixes #11252

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

…rary with shdict

bzp2010

There are quite a few issues. I have already checked the hot paths in the code (excluding the utils section). You can take a look and refute the errors I have pointed out.

apisix/discovery/nacos/init.lua

bzp2010 · 2025-05-30T16:08:03Z

apisix/discovery/nacos/init.lua

+                                   ns_id,
+                                   group_name,
+                                   service_name)
+    local value = nacos_dict:get_stale(key)


Why is it necessary to retrieve potentially stale data using get_stale instead of get?

This is to retrieve the old data when nacos is down.

Note that the value of an expired key is not guaranteed to be available so one should never rely on the availability of expired items.

try to use get_stale to retrieve expired item is unstable way.

bzp2010 · 2025-05-30T16:08:06Z

apisix/discovery/nacos/init.lua

+        local waiting_time = 5
+        local step = 0.1
+        local logged = false
+        while not value and waiting_time > 0 do
+            if not logged then
+                logged = true
+            end

-    if body and 'table' == type(body) then
-        local err
-        body, err = core.json.encode(body)
-        if not body then
-            return nil, 'invalid body : ' .. err
+            ngx.sleep(step)
+            waiting_time = waiting_time - step
+            value = nacos_dict:get_stale(key)
        end


What does this code mean?

Currently, nodes have been reduced to a mechanism that only reads from shdict. Before the code I selected, you did not attempt to initiate a request, so nodes would not suddenly be filled.
I understand that filling shdict is done by a decoupled privileged process, but I don't understand the value of using a spin lock-like mechanism for waiting here.

In my opinion, it should fast fail, which would cause APISIX to return a 503 error and print the log.

No matter how I look at it, it's strange that the request was blocked for 5 seconds because the service discovery provider did not return nodes.

dict:get_stale is called before starting the wait. And if no value is found then it waits for "step" and tries again. It does that for 5 seconds. This waiting mechanism was already there before my PR. The only thing I have done here is replace get with get_stale for reasons mentioned in above comment.

It really depends on how you think the purpose of this PR is.

As I mentioned above, this logic is weird, so my suggestion is to remove it. Let this logic failfast instead of waiting for something.

If the upstream on a route is not loaded or does not exist at all, APISIX will obviously return 503 immediately.

You can request opinions from other reviewers here.

@nic-6443 What do you think?

Aggre to remove it , this is weird.

apisix/discovery/nacos/init.lua

bzp2010 · 2025-05-31T05:36:29Z

apisix/discovery/nacos/init.lua

-function _M.dump_data()
-    return {config = local_conf.discovery.nacos, services = applications or {}}
+    return {
+        config = local_conf.discovery.nacos,


It seems that printing configurations are not secure.

If users are responsible for maintaining APISIX deployments, they can view these in the configuration files. The Control API itself should also be used by maintenance personnel, but we know that security controls on APIs are always difficult to configure correctly.

I don't think so there is sensitive data in this configuration.

This will contain access_key and secret_key.

It does seem that it should not be presented to the API in plaintext. You can't ensure that the API will only allow localhost calls for security consistent with local files.
The fact that this API (Control API) can be configured to allow access from any IP, and also does not (unsupported) turn on TLS, amounts to creating the possibility for a credential to be transmitted over the internet in plaintext.

Moreover, what is the point of passing this configuration through the API data, as an authorized system administrator who can access the configuration file at any time, or drive configuration changes through some CI/CD system. The significance of providing this convenience via the API seems trivial.

removed the configuration

bzp2010 · 2025-05-31T05:40:14Z

apisix/discovery/nacos/factory.lua

+        start = start,
+        stop = stop,
+        fetch_instances = fetch_instances,
+        fetch_full_registry = fetch_full_registry,


According to APISIX convention, you should use the meta table and _index. I don't remember us having such a precedent.

bzp2010 · 2025-05-31T05:48:16Z

apisix/discovery/nacos/factory.lua

+       ["username"] = username,
+       ["password"] = password,


Suggested change

["username"] = username,

["password"] = password,

username = username,

password = password,

ditto https://github.com/apache/apisix/pull/12263/files#diff-eb100eda46f74a8793829cab2eab9974ec0f98edc6f312a91e0a31c4bab12889R147-R150

apisix/discovery/nacos/factory.lua

bzp2010 · 2025-05-31T05:53:48Z

apisix/discovery/nacos/factory.lua

+        method = method,
+        headers = headers,
+        body = body,
+        ssl_verify = false,


It seems unsafe.

I think its okay to allow for non tls traffic for service discovery. Eureka service discovery code also has the same logic.

What I mean is that hardcoding to disable TLS certificate validation should not be done. If it needs to default to false, then it should be configurable; otherwise, it should be hard-coded to true.

Anyway, "no certificate validation at all times" isn't acceptable security-wise.

I think we should do the right thing instead of pleasing the lazy. Not to mention that we already have ca-certificates installed for container deployment and APISIX will use it by default.

okay I hardcoded it to true

apisix/discovery/nacos/factory.lua

…actor-nacos-2

apisix/discovery/nacos/factory.lua

nic-6443 · 2025-06-19T03:38:37Z

apisix/discovery/nacos/factory.lua

+        if not curr_services_in_use[config.id] then
+            curr_services_in_use[config.id] = {}
+        end
+        for name in pairs(service_names) do


why not assign service_names to curr_services_in_use[config.id] directly?

for name in pairs(curr_services_in_use[config.id]) do if not service_names[name] then nacos_dict:delete(name) end end curr_services_in_use[config.id] = service_names

nic-6443 · 2025-06-19T03:40:47Z

apisix/discovery/nacos/init.lua

+        -- maximum waiting time: 5 seconds
+        local waiting_time = 5
+        local step = 0.1
+        local logged = false
+        while not value and waiting_time > 0 do
+            if not logged then
+                logged = true
+            end

-    if body and 'table' == type(body) then
-        local err
-        body, err = core.json.encode(body)
-        if not body then
-            return nil, 'invalid body : ' .. err
+            ngx.sleep(step)
+            waiting_time = waiting_time - step
+            value = nacos_dict:get(key)


@Revolyssup why no any changes for this? #12263 (comment), we already discuss to remove it?

nic-6443 · 2025-06-19T03:48:11Z

apisix/discovery/nacos/init.lua

+-- Now we use control plane to list the services
+function _M.list_all_services()
+    return {}
+end


what's this ? it's unused function, please remove it.

feat: add support for multiple nacos instances and replace events lib…

e3dac56

…rary with shdict

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label May 28, 2025

Revolyssup marked this pull request as draft May 28, 2025 08:23

dosubot bot added the enhancement New feature or request label May 28, 2025

Revolyssup added 19 commits May 28, 2025 14:03

add shared dict

41a6a96

add timeout

53b82c3

add license

c722fb4

fix indent

6be3ccc

fix test

d15a621

docker compose revert

19bd92c

revert nacos3.t

ad5cde8

revert

a3a5c14

temp

33148d8

test

b93618f

fix

50f2547

reset waiting time

0d1066a

add logs

9542877

fix nacos2 assert

6b5c731

fix stream

450452c

fix lint

f77e00e

add dedup logic

bbfaf29

add log

e6d84b5

fix test2

91598fe

Revolyssup marked this pull request as ready for review May 28, 2025 19:28

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels May 28, 2025

Revolyssup added 4 commits May 29, 2025 01:15

fix lint

fab71bd

add multi cluster test

4ba8fc4

fix lint

612de20

fix nacos2

6ad86df

Revolyssup added 4 commits May 29, 2025 23:51

fix cli test

bb5d343

add sleep to allow workers to get ready

e8e09bc

fix indent

7daad27

fix indent

a4e225f

bzp2010 reviewed May 31, 2025

View reviewed changes

Revolyssup added 8 commits June 3, 2025 12:00

Merge branch 'master' of github.com:apache/apisix into revolyssup/ref…

add27eb

…actor-nacos-2

apply suggestions

fe05f02

init nacos shared dict in meta

1e9159a

fail fast

2d222ce

remove nacos_client upvalue

6608c7c

fix removal of wrong keys

6579ef0

add retry

9f15786

lint

def3458

nic-6443 reviewed Jun 5, 2025

View reviewed changes

apisix/discovery/nacos/factory.lua Outdated Show resolved Hide resolved

Revolyssup added 2 commits June 12, 2025 11:36

apply suggestion

a754888

fix lint

c1d89c9

Baoyuantop added this to ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop moved this to 👀 In review in ⚡️ Apache APISIX Roadmap Jun 13, 2025

Baoyuantop assigned Revolyssup Jun 13, 2025

Revolyssup added 2 commits June 13, 2025 12:40

applied suggestions

71598f4

fix lint

12a0847

Revolyssup requested review from bzp2010, nic-6443, membphis and moonming June 16, 2025 08:24

nic-6443 reviewed Jun 19, 2025

View reviewed changes

Revolyssup mentioned this pull request Jun 19, 2025

feat: replace events library with shdict #12353

Open

5 tasks

Revolyssup marked this pull request as draft June 22, 2025 07:52

feat: add support for multiple nacos instances and replace events library with shdict #12263

Are you sure you want to change the base?

feat: add support for multiple nacos instances and replace events library with shdict #12263

Uh oh!

Conversation

Revolyssup commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jun 12, 2025 • 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!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nic-6443 Jun 19, 2025 • 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!

Revolyssup commented May 28, 2025 •

edited

Loading

bzp2010 Jun 12, 2025 •

edited

Loading

nic-6443 Jun 19, 2025 •

edited

Loading