feat: support for Service Discovery Across Multiple Nacos Clusters #10950

ShenFeng312 · 2024-02-21T03:29:17Z

Description

Fixes #10799

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)

shreemaan-abhishek · 2024-02-23T03:38:33Z

please fix the linter

ShenFeng312 · 2024-02-23T06:51:04Z

@monkeyDluffy6017 hello~ These errors seem to be unrelated to this PR.

monkeyDluffy6017 · 2024-02-23T07:57:18Z

Could you add test cases to cover this?

ShenFeng312 · 2024-02-23T08:34:41Z

Could you add test cases to cover this?

I will add test cases next week

zll600 · 2024-02-27T15:07:36Z

@monkeyDluffy6017 hello~ These errors seem to be unrelated to this PR.

The ci problem has been fixed by #10959. Please merge latest master.

shreemaan-abhishek · 2024-04-18T13:58:16Z

CI is still failing

ShenFeng312 · 2024-04-22T01:21:22Z

@shreemaan-abhishek hello~ i dont know how to fix this error

shreemaan-abhishek · 2024-04-22T05:47:13Z

@ShenFeng312 try using --- response_body chomp

apisix/discovery/nacos/init.lua

shreemaan-abhishek · 2024-04-29T08:05:26Z

please pay attention here: #10950 (comment)

ShenFeng312 · 2024-04-29T10:00:09Z

please pay attention here: #10950 (comment)

fixed

shreemaan-abhishek · 2024-04-29T10:15:38Z

can you also mention that host is deprecated now (both in code and docs). Here is an example:

https://apisix.apache.org/docs/apisix/plugins/kafka-logger/#attributes

ShenFeng312 · 2024-04-30T02:44:26Z

can you also mention that host is deprecated now (both in code and docs). Here is an example:

https://apisix.apache.org/docs/apisix/plugins/kafka-logger/#attributes

Yes

shreemaan-abhishek · 2024-05-03T04:12:15Z

I think this line is enough: 6d2dcc2?diff=split&w=0#diff-8d872babc717e9d733641b56bfc530ef98751fbe4e68f08d79b2b83109c22fffR302

we can skip these:

And it think it's better if the comment says: "deprecated, use nacos.hosts instead"

apisix/discovery/nacos/init.lua

shreemaan-abhishek · 2024-05-03T05:07:51Z

apisix/discovery/nacos/init.lua

        local query_path = instance_list_path .. service_info.service_name
-                           .. token_param .. namespace_param .. group_name_param
-                           .. signature_param
+                .. token_param .. namespace_param .. group_name_param


there are many other whitespace changes like this, please revert them

shreemaan-abhishek · 2024-05-03T05:10:44Z

apisix/discovery/nacos/init.lua

@@ -405,12 +434,17 @@ function _M.init_worker()
        return
    end

-    default_weight = local_conf.discovery.nacos.weight
-    log.info('default_weight:', default_weight)
+    --default_weight = local_conf.discovery.nacos.weight


why change these?

Because default_weight in different nacos may different @shreemaan-abhishek

shreemaan-abhishek · 2024-05-03T05:10:53Z

apisix/discovery/nacos/init.lua

    local fetch_interval = local_conf.discovery.nacos.fetch_interval
    log.info('fetch_interval:', fetch_interval)
-    access_key = local_conf.discovery.nacos.access_key
-    secret_key = local_conf.discovery.nacos.secret_key
+    --access_key = local_conf.discovery.nacos.access_key


Because access_key and secret_key in different nacos may different @shreemaan-abhishek

shreemaan-abhishek · 2024-05-03T05:11:36Z

ci/pod/docker-compose.first.yml

@@ -109,6 +109,7 @@ services:
          - consul.cluster

  ## Nacos cluster
+


this change is not needed

+1 Please try to avoid unnecessary formatting changes, such as adding or reducing blank lines.

shreemaan-abhishek · 2024-05-03T05:14:11Z

docs/en/latest/discovery/nacos.md

-    host:
-      - "http://192.168.33.1:8848"
+    hosts:
+      - host:


this doesn't seem the right way. I think the following is better:

nacos: hosts: - xyz - abc

Do you mean to configure it like this?

hosts: name1: - host: - xxx name2: - host: - xxx

@shreemaan-abhishek we may need config different access_key in different nacos

Agree with @ShenFeng312, multiple nacos instances will be used in a multi-cluster environment (with multiple registries), not just for load balancing on a single endpoint.

Co-authored-by: Abhishek Choudhary <[email protected]>

ShenFeng312 · 2024-06-28T07:15:50Z

Hello, can I continue with this PR? Or do I need to create a new one? Or are we not planning to support this feature? @shreemaan-abhishek

bzp2010 · 2024-07-13T06:40:41Z

I think this really should go on, let me see. No new PR is needed and I am open to adding this feature, we can do that.

bzp2010 · 2024-07-13T06:50:29Z

apisix/discovery/nacos/init.lua

+    --default_weight = local_conf.discovery.nacos.weight
+    --log.info('default_weight:', default_weight)


Should it be deleted? Delete them if they are no longer used.

bzp2010 · 2024-07-13T06:51:47Z

apisix/discovery/nacos/init.lua

+    --access_key = local_conf.discovery.nacos.access_key
+    --secret_key = local_conf.discovery.nacos.secret_key


Delete them if they are no longer used too.

bzp2010 · 2024-07-13T06:58:46Z

apisix/discovery/nacos/init.lua

+    fetch_from_naocs(local_conf.discovery.nacos)
+    local others_nacos = local_conf.discovery.nacos.hosts
+    if others_nacos and #others_nacos > 0 then
+        for _, nacos in ipairs(others_nacos) do
+            fetch_from_naocs(nacos)
+        end
    end


The old mode will be deprecated later right?
Should we treat them as mutually exclusive now (i.e. disable single registry mode when multiple registries mode are enabled), I think it's clearer for the user rather than mixing them up and suddenly not working anymore when we remove it.

bzp2010 · 2024-07-13T07:04:14Z

apisix/discovery/nacos/init.lua

+        local signature_param = get_signed_param(service_info.group_name,
+                service_info.service_name, nacos)


Suggested change

local signature_param = get_signed_param(service_info.group_name,

service_info.service_name, nacos)

local signature_param = get_signed_param(service_info.group_name,

service_info.service_name, nacos)

bzp2010 · 2024-07-13T07:12:26Z

apisix/discovery/nacos/init.lua

+        local nacos_name_from_args = (up.discovery_args and up.discovery_args.name)
+             or default_nacos_name
+        local nacos_name = nacos.name or default_nacos_name
+        if nacos_name ~= nacos_name_form_args then


Suggested change

if nacos_name ~= nacos_name_form_args then

if nacos_name ~= nacos_name_from_args then

A typo here.

bzp2010 · 2024-07-13T07:17:22Z

apisix/discovery/nacos/init.lua

-    if not applications or not applications[namespace_id]
-        or not applications[namespace_id][group_name]
+    if not applications or not applications[nacos_name]
+        or not  applications[nacos_name][namespace_id]


Suggested change

or not applications[nacos_name][namespace_id]

or not applications[nacos_name][namespace_id]

bzp2010 · 2024-07-13T07:32:03Z

apisix/discovery/nacos/schema.lua

@@ -54,6 +54,49 @@ return {
        },
        access_key = {type = 'string', default = ''},
        secret_key = {type = 'string', default = ''},
+        hosts = {


Maybe like https://github.com/apache/apisix/blob/master/apisix/discovery/kubernetes/schema.lua#L114-L216 would be a better way? Users have the option to continue using the single registry mode, or switch to the multi-registry mode altogether.

If we change the configuration file to the above format, the redundant nesting called hosts is unnecessary, i.e. the configuration should be:

discovery: nacos: host: xxx

Or,

discovery: nacos: - id: default host: xxx - id: test host: xxx

bzp2010 · 2024-07-13T07:41:28Z

apisix/discovery/nacos/init.lua

+        local nacos_name_from_args = (up.discovery_args and up.discovery_args.name)
+             or default_nacos_name


I remember other service discovery implementations using different rules on service name to handle multiple registries instead of using discovery_args, please follow that similar.

https://apisix.apache.org/docs/apisix/discovery/kubernetes/#multi-cluster-mode-query-interface

bzp2010 · 2024-07-13T07:43:12Z

conf/config-default.yaml

-#    host:                        # Nacos address(es)
+
+#  nacos:
+#    name: "default"       # Deprecated,see nacos.hosts.name


The single-registry mode that is now supported does not require a name, and I think your defaults in the code can handle that scenario.
So this is not required and should be removed.

bzp2010 · 2024-07-13T07:46:17Z

conf/config-default.yaml

+#    access_key: ""        # Deprecated see nacos.hosts.access_key
+#    secret_key: ""        # Deprecated see nacos.hosts.secret_key
+#    hosts:
+#      - name: "your_nacos_cluster_name"  #your nacos cluster name


The field used to identify the registry should be id not name. please follow the existing rules unless you are the first to do so.

apisix/conf/config-default.yaml

Line 398 in 0cad329

# - id: release # a custom name refer to the cluster, pattern ^[a-z0-9]{1,8}

bzp2010 · 2024-07-13T07:51:02Z

t/discovery/nacos.t

+    host:
+      - "http://127.0.0.1:8858"
+    prefix: "/nacos/v1/"
+    fetch_interval: 1
+    weight: 1
+    timeout:
+      connect: 2000
+      send: 2000
+      read: 5000
+    hosts:
+      - name: nacos3
+        host:
+          - "http://127.0.0.1:8868"
+        prefix: "/nacos/v1/"
+        fetch_interval: 1
+        weight: 1
+        timeout:
+          connect: 2000
+          send: 2000
+          read: 5000


I don't think a mixed mode of single registry and multiple registries should exist, and the user should explicitly choose one or the other. This will also simplify our testing.

do you propose removing host and introduce hosts as it's replacement? It would be a breaking change.

@shreemaan-abhishek I am proposing: anyOf host or hosts, and prohibits the mixing of the two modes, so the following configuration is acceptable.

discovery: nacos: host: xxx prefix: xxx

Or,

discovery: nacos: - id: default host: xxx prefix: xxx - id: backup host: xxx prefix: xxx

Yes this is much better. cc: @ShenFeng312

bzp2010 · 2024-07-13T08:07:02Z

@ShenFeng312 Except for a few minor issues that could be modified, this is an excellent job.

ShenFeng312 added 3 commits February 21, 2024 11:25

Support for Service Discovery Across Multiple Nacos Clusters

8c630ea

fix test

a375384

fix test

de48972

fix codestyle

b67062c

monkeyDluffy6017 changed the title ~~feat: Support for Service Discovery Across Multiple Nacos Clusters~~ feat: support for Service Discovery Across Multiple Nacos Clusters Feb 23, 2024

monkeyDluffy6017 added the need test cases label Feb 26, 2024

ShenFeng312 and others added 11 commits April 16, 2024 11:26

add test

b54d165

Merge branch 'apache:master' into master

1e9ce85

fix test

69e5752

Merge branch 'master' of github.com:ShenFeng312/apisix

b8062ea

fix test

ff5f2c5

add polish test

f824ed1

polish test

9ca5361

fix test

f07e9c4

fix test

5acd79a

fix test

3a1ebd1

Merge branch 'apache:master' into master

a05ff3b

ShenFeng312 added 4 commits April 19, 2024 15:48

fix test

ca1e89a

Merge branch 'master' of github.com:ShenFeng312/apisix

ee0b383

fix test

d7e4991

fix test

a1f5807

github-actions bot added the user responded label Apr 22, 2024

shreemaan-abhishek reviewed Apr 24, 2024

View reviewed changes

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

fix code style

b317869

rename others to hosts

accfd55

update docs and add deprecated warn log

6d2dcc2

shreemaan-abhishek reviewed May 3, 2024

View reviewed changes

Update apisix/discovery/nacos/init.lua

ea3b35c

Co-authored-by: Abhishek Choudhary <[email protected]>

ShenFeng312 requested a review from shreemaan-abhishek June 28, 2024 07:22

bzp2010 self-requested a review July 13, 2024 06:38

bzp2010 reviewed Jul 13, 2024

View reviewed changes

bzp2010 requested review from juzhiyuan, nic-6443 and SkyeYoung July 13, 2024 07:59

		@@ -109,6 +109,7 @@ services:
		- consul.cluster

		## Nacos cluster

		--default_weight = local_conf.discovery.nacos.weight
		--log.info('default_weight:', default_weight)

		--access_key = local_conf.discovery.nacos.access_key
		--secret_key = local_conf.discovery.nacos.secret_key

		local signature_param = get_signed_param(service_info.group_name,
		service_info.service_name, nacos)

	if nacos_name ~= nacos_name_form_args then
	if nacos_name ~= nacos_name_from_args then

	or not applications[nacos_name][namespace_id]
	or not applications[nacos_name][namespace_id]

		local nacos_name_from_args = (up.discovery_args and up.discovery_args.name)
		or default_nacos_name

feat: support for Service Discovery Across Multiple Nacos Clusters #10950

Are you sure you want to change the base?

feat: support for Service Discovery Across Multiple Nacos Clusters #10950

Conversation

ShenFeng312 commented Feb 21, 2024 • edited Loading

Description

Checklist

shreemaan-abhishek commented Feb 23, 2024

ShenFeng312 commented Feb 23, 2024

monkeyDluffy6017 commented Feb 23, 2024

ShenFeng312 commented Feb 23, 2024

zll600 commented Feb 27, 2024

shreemaan-abhishek commented Apr 18, 2024

ShenFeng312 commented Apr 22, 2024

shreemaan-abhishek commented Apr 22, 2024

shreemaan-abhishek commented Apr 29, 2024

ShenFeng312 commented Apr 29, 2024

shreemaan-abhishek commented Apr 29, 2024 • edited Loading

ShenFeng312 commented Apr 30, 2024

shreemaan-abhishek commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShenFeng312 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShenFeng312 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzp2010 Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShenFeng312 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShenFeng312 commented Jun 28, 2024

bzp2010 commented Jul 13, 2024 • edited Loading

bzp2010 Jul 13, 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

bzp2010 Jul 13, 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

bzp2010 Jul 13, 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

bzp2010 commented Jul 13, 2024

ShenFeng312 commented Feb 21, 2024 •

edited

Loading

shreemaan-abhishek commented Apr 29, 2024 •

edited

Loading

ShenFeng312 May 10, 2024 •

edited

Loading

ShenFeng312 May 10, 2024 •

edited

Loading

bzp2010 Jul 13, 2024 •

edited

Loading

ShenFeng312 May 10, 2024 •

edited

Loading

bzp2010 commented Jul 13, 2024 •

edited

Loading

bzp2010 Jul 13, 2024 •

edited

Loading

bzp2010 Jul 13, 2024 •

edited

Loading

bzp2010 Jul 13, 2024 •

edited

Loading