feat: add nacos support #3820

benx203 · 2021-03-13T12:25:06Z

feat:add nacos support

add nacos support

spacewander

Please add test under t/discovery.

spacewander · 2021-03-13T13:59:43Z

And you need to add docs under https://github.com/apache/apisix/tree/master/docs/en/latest/discovery

spacewander · 2021-03-17T06:49:21Z

Please make sure it can pass the lint.

spacewander · 2021-03-18T07:06:12Z

docs/en/latest/discovery/nacos.md

+discovery:
+  nacos:
+    host:
+      - "http://192.168.33.1:8848"


Why don't mention the user / password feature?

Because Nacos not need login yet.

spacewander · 2021-03-18T07:13:49Z

apisix/discovery/nacos.lua

+        },
+        fetch_interval = {type = "integer", minimum = 1, default = 30},
+        prefix = {type = "string"},
+        weight = {type = "integer", minimum = 0},


Missing default for weight?
And the minimum weight should be 1.

OK,I copy from eureka.

spacewander · 2021-03-18T07:14:43Z

apisix/discovery/nacos.lua

+
+    local httpc = http.new()
+    local timeout = local_conf.discovery.nacos.timeout
+    local connect_timeout = timeout and timeout.connect or 2000


The default value will be injected via schema check. We don't need to assign it.

spacewander · 2021-03-18T07:18:51Z

apisix/discovery/nacos.lua

+    log.info("default_weight:", default_weight, ".")
+    local fetch_interval = local_conf.discovery.nacos.fetch_interval or 30
+    log.info("fetch_interval:", fetch_interval, ".")
+    service_list_path = local_conf.discovery.nacos.service_list_path or


Where should I configure service_list_path? There is neither schema nor doc for it.

I will fix it.

spacewander · 2021-03-18T07:18:59Z

apisix/discovery/nacos.lua

+    log.info("fetch_interval:", fetch_interval, ".")
+    service_list_path = local_conf.discovery.nacos.service_list_path or
+            'ns/service/list?pageNo=1&pageSize=20'
+    instance_list_path = local_conf.discovery.nacos.instance_list_path or


spacewander · 2021-03-18T07:19:30Z

apisix/discovery/nacos.lua

+    end
+
+    local up_apps = core.table.new(0, 0)
+    local data = get_url(base_uri,basic_auth,service_list_path)


Need space after the comma.
And please check the returned value of get_url.

spacewander · 2021-03-18T07:22:24Z

apisix/discovery/nacos.lua

+    log.info("nacos uri:", request_uri, ".")
+    local url = request_uri .. path
+    local headers = core.table.new(0, 5)
+    headers['Connection'] = 'Keep-Alive'


We don't need it for HTTP 1.1

spacewander · 2021-03-18T07:22:45Z

apisix/discovery/nacos.lua

+local function request(request_uri, basic_auth, method, path, query, body)
+    log.info("nacos uri:", request_uri, ".")
+    local url = request_uri .. path
+    local headers = core.table.new(0, 5)


Why choose 5?

spacewander · 2021-03-18T07:24:43Z

apisix/discovery/nacos.lua

+    if local_conf.discovery.nacos.prefix then
+        url = url .. local_conf.discovery.nacos.prefix
+    end
+    if string_sub(url, #url) ~= "/" then


Better to use str_byte(uri, #uri) == str_byte("/")

apisix/discovery/nacos.lua

spacewander · 2021-03-23T10:11:34Z

docs/en/latest/discovery/nacos.md

+    fetch_interval: 30    # default 30 sec
+    weight: 100           # default 100
+    timeout:
+      connect: 2000       # default 2000


Better to mention the unit is ms.

spacewander · 2021-03-23T10:14:45Z

t/discovery/nacos.t

+--- pipelined_requests eval
+[
+    "POST /nacos/v1/ns/instance?port=18001&healthy=true&ip=127.0.0.1&weight=1.0&serviceName=APISIX-NACOS&encoding=GBK&enabled=true",
+    "DELETE /nacos/v1/ns/service?serviceName=APISIX-NACOS",


Why POST and then DELETE?

If has no data,delete return code is 400,if has data,delete return code is 200.

But it is not the feature of APISIX. We don't need to test nacos itself.

spacewander · 2021-03-23T10:15:20Z

t/discovery/nacos.t

+#END
+--- request
+GET /hello
+--- error_code: 200


We need to show if server 1 or server 2 is hit.
And we need a test to show the default weight

spacewander · 2021-03-23T10:16:23Z

docs/en/latest/discovery/nacos.md

+```shell
+$ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -i -d '
+{
+    "uri": "/nacos/**",


Use /nacos/* is enough?

spacewander · 2021-03-23T10:17:16Z

.travis/linux_tengine_runner.sh

@@ -40,6 +40,9 @@ before_install() {
    # start consul servers
    docker run --rm --name consul_1 -d -p 8500:8500 consul:1.7 consul agent -server -bootstrap-expect=1 -client 0.0.0.0 -log-level info -data-dir=/consul/data
    docker run --rm --name consul_2 -d -p 8600:8500 consul:1.7 consul agent -server -bootstrap-expect=1 -client 0.0.0.0 -log-level info -data-dir=/consul/data
+
+    # start nacos server
+    docker run --rm --name nacos_1 -d --env PREFER_HOST_MODE=hostname --env MODE=standalone --env JVM_XMS=512m --env JVM_XMX=512m --env JVM_XMN=256m -p8848:8848 nacos/nacos-server:latest


We need test the user/password feature.

If add NACOS_AUTH_ENABLE=true, file nacos.t need login too.
Could you give me a t file login demo?

You can use different conf in different tests, one for auth success and another for auth fail.

How to check nacos in discovery plugin whether authed?

If the auth enable nacos can be used, the auth is successful.

But if nacos start with auth mode,in nacos.t add services to nacos need send auth token.
How can i save auth token in nacos.t?

You can export the auth function: https://github.com/apache/apisix/pull/3820/files#diff-46fd008790f9ea29612a92ca5c94212ae43c81ab6b902ef3014d3d6b4bdfc5b3R202
and call it in the test file.

spacewander · 2021-03-23T10:19:55Z

apisix/discovery/nacos.lua

+}
+
+
+local function str_split (inputstr, sep)


Should use existent API:

apisix/apisix/plugins/proxy-cache.lua

Line 194 in e9a59ac

local levels = ngx_re.split(cache_levels, ":")

spacewander · 2021-03-23T10:22:08Z

apisix/discovery/nacos.lua

+        prefix = {type = "string", default = "/nacos/v1/"},
+        auth_path = {type = "string", default = "auth/login"},
+        service_list_path = {type = "string", default = "ns/service/list?pageNo=1&pageSize=20"},
+        instance_list_path = {type = "string", default = "ns/instance/list?serviceName="},


Need a test to check the user-defined paths.
BTW, in what situation people need to redefine the paths?

When Nacos api change.

@benx203
I think we can remove it now and add it if needed.
We need to add a doc and test for this feature if you insist to keep it.
(I don't recommend to do that as this PR is big enough)

apisix/discovery/nacos.lua

spacewander · 2021-03-23T11:23:57Z

apisix/discovery/nacos.lua

+        fetch_interval = {type = "integer", minimum = 1, default = 30},
+        prefix = {type = "string", default = "/nacos/v1/"},
+        auth_path = {type = "string", default = "auth/login"},
+        service_list_path = {type = "string", default = "ns/service/list?pageNo=1&pageSize=20"},


Only return the first 20 services is not a good idea.
We need to iterate the upstreams and build a service list from those that have nacos discovery_type.
Like this:

apisix/apisix/control/v1.lua

Lines 144 to 149 in e9a59ac

values = get_upstreams()

else

return 400, {error_msg = str_format("invalid src type %s", src_type)}

end

local info, err = iter_and_find_healthcheck_info(values, src_type, src_id)

You might need some lazy load trick to break the dependent cycle.

spacewander · 2021-04-08T07:47:19Z

apisix/discovery/nacos.lua

+        end
+
+        for _, host in ipairs(data.hosts) do
+            if tostring(host.valid) == 'true' and


I set up the environment and debug it myself.
Here is the host: {"enabled":true,"instanceHeartBeatTimeOut":15000,"ipDeleteTimeout":30000,"instanceHeartBeatInterval":5000,"metadata":{"preserved.register.source":"SPRING_CLOUD"},"port":18001,"serviceName":"DEFAULT_GROUP@@APISIX-NACOS","instanceId":"192.168.65.3#18001#DEFAULT#DEFAULT_GROUP@@APISIX-NACOS","healthy":true,"ip":"192.168.65.3","ephemeral":true,"clusterName":"DEFAULT","weight":1}

There is not valid field. The response doesn't match their API doc: https://nacos.io/en-us/docs/open-api.html

I am not sure if it is because the latest Nacos has changed the API. Maybe we need to use the tagged nacos image 2.0.0 instead of the latest one?

spacewander · 2021-04-08T07:48:38Z

.github/workflows/centos7-ci.yml

+        nohup docker network rm nacos_net > /dev/null 2>&1 &
+        nohup docker network create nacos_net > /dev/null 2>&1 &
+        # nacos no auth server - for test no auth
+        docker run --rm -d --name nacos_no_auth --network nacos_net --hostname nacos2 --env NACOS_SERVERS="nacos1:8848 nacos2:8848" --env PREFER_HOST_MODE=hostname --env MODE=cluster --env EMBEDDED_STORAGE=embedded  --env JVM_XMS=512m --env JVM_XMX=512m --env JVM_XMN=256m -p8858:8848 nacos/nacos-server:latest


Would it be better to use nacos/nacos-server:2.0.0?

The nacos version should be 1.4.1,2.0.0 just released few days ago.

@benx203
Why can't we support 2.0.0 directly? Is 2.0.0 incompatible with 1.4.1?

I think better way is support both 2.0.0 and 1.4.1.
Maybe support 2.0.0 later?

spacewander · 2021-04-08T07:49:22Z

.github/workflows/centos7-ci.yml

+        rm -rf tmp
+        mkdir tmp
+        cd tmp
+        wget https://raw.githubusercontent.com/benx203/nacos-test-service/main/spring-nacos-1.0-SNAPSHOT.jar


@benx203
Would you also submit the Java code to https://github.com/benx203/nacos-test-service? Thanks!

spacewander · 2021-04-08T11:30:06Z

apisix/discovery/nacos.lua

+        for _, host in ipairs(data.hosts) do
+            if tostring(host.valid) == 'true' and
+                    tostring(host.healthy) == 'true' and
+                    tostring(host.enabled) == 'true' then


One more question: why we need to filter the result according to those fields?

If query all records,need to filter.
But now should not need any more.

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander · 2021-04-09T08:09:57Z

@benx203
I will work on this PR and fulfill my promise in a few days. Please be patient.

benx203 · 2021-04-09T09:32:59Z

@spacewander
OK.Take your time.

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

membphis · 2021-04-14T14:01:52Z

apisix/discovery/nacos.lua

+            type = "array",
+            minItems = 1,
+            items = {
+                type = "string",


add a pattern to limit the string to the host format

membphis · 2021-04-14T14:02:12Z

apisix/discovery/nacos.lua

+            },
+        },
+        fetch_interval = {type = "integer", minimum = 1, default = 30},
+        prefix = {type = "string", default = "/nacos/v1/"},


membphis · 2021-04-14T14:03:03Z

apisix/discovery/nacos.lua

+
+
+local _M = {
+    version = 0.1,


this version is useless, we can drop it

membphis · 2021-04-14T14:04:31Z

apisix/discovery/nacos.lua

+    local url = request_uri .. path
+    log.info("request url:", url)
+    local headers = core.table.new(0, 0)
+    headers['Accept'] = 'application/json'


for the string object, I think we use ", it is easier for reading.

headers["Accept"] = "application/json"

and please fix the similar points

membphis · 2021-04-14T14:07:22Z

apisix/discovery/nacos.lua

+
+
+local function get_token_param(base_uri, username, password)
+    if username and password then


better style:

if not username or not password then return "" end ...

membphis · 2021-04-14T14:09:50Z

apisix/discovery/nacos.lua

+
+local function get_token_param(base_uri, username, password)
+    if username and password then
+        local data, err = post_url(base_uri, auth_path .. "?username=" .. username


I think we should call ngx.encode_args for username and password

membphis · 2021-04-14T14:11:42Z

apisix/discovery/nacos.lua

+
+
+local function iter_and_add_service(services, values)
+	if not values then


bad indentation

membphis · 2021-04-14T14:14:22Z

apisix/discovery/nacos.lua

+    local host = local_conf.discovery.nacos.host
+    -- TODO Add health check to get healthy nodes.
+    local url = host[math_random(#host)]
+    local auth_idx = str_find(url, "@")


we'd better use the plain text model, better performance.

https://stackoverflow.com/questions/18973019/is-there-a-lua-string-find-without-pattern

membphis · 2021-04-14T14:17:35Z

apisix/discovery/nacos.lua

+
+function _M.nodes(service_name)
+    local logged = false
+    while not applications do


I think we need to set a maximum waiting time , eg: 5 seconds

moonming · 2021-04-14T15:41:27Z

@benx203 thanks for your contribution.
You can create a new PR to fix the code style issue.
And it will be better if you can add support Nacos in the README.md to let more people know about this feature.

benx203 · 2021-04-15T11:31:33Z

@benx203 thanks for your contribution.
You can create a new PR to fix the code style issue.
And it will be better if you can add support Nacos in the README.md to let more people know about this feature.

OK,i will try,thank you all.

spacewander · 2021-04-19T08:17:25Z

apisix/discovery/nacos.lua

@@ -275,41 +273,33 @@ end


 function _M.nodes(service_name)
-    local logged = false


The original code is good. We just need to set a 5s limitation.

Just keep simple.

The lazy init is important as the sync is slow.

OK,i rollback the code.

spacewander · 2021-04-19T08:21:25Z

apisix/discovery/nacos.lua

+    local host = local_conf.discovery.nacos.host
+    -- TODO Add health check to get healthy nodes.
+    local url = host[math_random(#host)]
+    local auth_idx = str_find(url, '@', 1, true)


The core.string.find is already the plain string find... 😅
Not all the suggestion from the reviewer is correct.

benx203 · 2021-04-19T08:36:41Z

apisix/discovery/nacos.lua

+    local values = get_upstreams()
+    iter_and_add_service(services, values)
+    values = get_routes()
+    iter_and_add_service(services, values)


By the way,Are there 'discovery_type' config in service and route?
@spacewander

Those can have embedded upstream field.

I see,thanks.

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

add nacos support

baab627

add nacos support

spacewander requested changes Mar 13, 2021

View reviewed changes

add docs

e053355

benx203 changed the title ~~add nacos support~~ feat:add nacos support Mar 15, 2021

spacewander changed the title ~~feat:add nacos support~~ feat: add nacos support Mar 15, 2021

benx203 added 2 commits March 18, 2021 11:28

fix lint warnings

5b14149

fix test

613c221

spacewander reviewed Mar 18, 2021

View reviewed changes

benx203 added 18 commits March 18, 2021 18:05

fix nacos

f0b0942

fix nacos test script

221ee3a

fix

8e3e6a5

fix nacos - support auth

69055a0

fix

e412a70

fix

9b48317

fix

d8f819b

fix

46fe9fd

reindex

9e51570

fix env script

4a0dac6

fix env script

bbc3f1f

fix env script

b153240

fix test script

962be01

fix test script

26de944

fix

c1e6f00

fix

95ab302

fix

63eba52

fix

df1d54f

spacewander reviewed Mar 23, 2021

View reviewed changes

fix

3ea6a66

debug test

6bbcaf1

spacewander reviewed Apr 8, 2021

View reviewed changes

benx203 added 2 commits April 8, 2021 16:18

debug test

f150a72

debug test

9ac15df

spacewander reviewed Apr 8, 2021

View reviewed changes

spacewander added 2 commits April 9, 2021 14:45

fork the repo to prevent the original repo deleted by accident

c97c0a2

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

tweak

4936b19

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander added 3 commits April 14, 2021 11:20

WIP

fb69097

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

Merge branch 'master' into benx203_master

6171c2e

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

fix lint

83d7500

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander approved these changes Apr 14, 2021

View reviewed changes

membphis reviewed Apr 14, 2021

View reviewed changes

moonming approved these changes Apr 14, 2021

View reviewed changes

fix

5af1d9e

spacewander reviewed Apr 19, 2021

View reviewed changes

fix

69408be

benx203 commented Apr 19, 2021

View reviewed changes

benx203 and others added 5 commits April 19, 2021 16:48

fix

dfd6921

fix

8b196f3

fix

09767bd

trigger test

660d2b6

style

407fb42

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

spacewander merged commit 8757080 into apache:master Apr 21, 2021

lijing-21 mentioned this pull request Jan 25, 2022

request help: Cooperation invitation from Apache APISIX community alibaba/nacos#7705

Closed

	values = get_upstreams()
	else
	return 400, {error_msg = str_format("invalid src type %s", src_type)}
	end

	local info, err = iter_and_find_healthcheck_info(values, src_type, src_id)



		local function get_token_param(base_uri, username, password)
		if username and password then



		local function iter_and_add_service(services, values)
		if not values then

		@@ -275,41 +273,33 @@ end


		function _M.nodes(service_name)
		local logged = false

feat: add nacos support #3820

feat: add nacos support #3820

Conversation

benx203 commented Mar 13, 2021 • edited Loading

spacewander left a comment

Choose a reason for hiding this comment

spacewander commented Mar 13, 2021

spacewander commented Mar 17, 2021

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

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

benx203 Mar 23, 2021 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander Apr 8, 2021 • 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

spacewander commented Apr 9, 2021

benx203 commented Apr 9, 2021 • 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

Choose a reason for hiding this comment

moonming commented Apr 14, 2021

benx203 commented Apr 15, 2021

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

benx203 commented Mar 13, 2021 •

edited

Loading

benx203 Mar 23, 2021 •

edited

Loading

spacewander Apr 8, 2021 •

edited

Loading

benx203 commented Apr 9, 2021 •

edited

Loading