membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487773165
##########
File path: apisix/core/config_etcd.lua
##########
@@ -92,17 +105,29 @@ local function waitdir(etcd_cli, key, modified_index,
timeout)
return nil, nil, "not inited"
end
- local res, err = etcd_cli:waitdir(key, modified_index, timeout)
+ local opts = {}
+ opts.start_revision = modified_index
+ opts.timeout = timeout
+ local res_fun, fun_err = etcd_cli:watchdir(key, opts)
+ if not res_fun then
+ return nil, fun_err
+ end
+
+ -- try twice to skip create info
Review comment:
need more comments, it makes me confused
##########
File path: README.md
##########
@@ -166,10 +166,9 @@ There are several ways to install the Apache Release
version of APISIX:
apisix start
```
-**Note**: Apache APISIX does not yet support the v3 protocol of etcd, so you
need to enable v2 protocol when starting etcd.
-We are doing support for etcd v3 protocol.
+**Note**: Apache APISIX would not support the v2 protocol of etcd anymore
since APISIX v2.0, so you need to enable v3 protocol when starting etcd, if
etcd version is below v3.4.
Review comment:
we still not release `v2.0` now. We need to add a comment here
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +49,144 @@ end
_M.new = new
+local function kvs_to_node(kvs)
+ local node = {}
+ node.key = kvs.key
+ node.value = kvs.value
+ node.createdIndex = tonumber(kvs.create_revision)
Review comment:
is it possible the `kvs.create_revision` is non-numeric value?
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +49,144 @@ end
_M.new = new
+local function kvs_to_node(kvs)
+ local node = {}
+ node.key = kvs.key
+ node.value = kvs.value
+ node.createdIndex = tonumber(kvs.create_revision)
+ node.modifiedIndex = tonumber(kvs.mod_revision)
+ return node
+end
+
+local function kvs_to_nodes(res, start_index)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=start_index, #res.body.kvs do
+ if start_index == 1 then
+ res.body.node.nodes[i] = kvs_to_node(res.body.kvs[i])
+ else
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ end
+ end
+ return res
+end
+
+
+local function not_found(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.get_format(res, realkey)
+ if res.body.error == "etcdserver: user name is empty" then
+ return nil, "insufficient credentials code: 401"
+ end
+
+ res.headers["X-Etcd-Index"] = res.body.header.revision
+
+ if not res.body.kvs then
+ return not_found(res)
+ end
+ res.body.action = "get"
+
+ res.body.node = kvs_to_node(res.body.kvs[1])
+ -- kvs.value = nil, so key is root
+ if type(res.body.kvs[1].value) == "userdata" or not res.body.kvs[1].value
then
+ -- remove last "/" when necesary
+ if string.sub(res.body.node.key, -1, -1) == "/" then
+ res.body.node.key = string.sub(res.body.node.key, 1,
#res.body.node.key-1)
+ end
+ res = kvs_to_nodes(res, 2)
+ -- key not match, so realkey is root
+ elseif res.body.kvs[1].key ~= realkey then
+ res.body.node.key = realkey
+ res = kvs_to_nodes(res, 1)
+ -- first is root (in v2, root not contains value), others are nodes
+ elseif #res.body.kvs > 1 then
+ res = kvs_to_nodes(res, 2)
+ end
+
+ res.body.kvs = nil
+ return res, nil
Review comment:
`, nil` useless, we can remove it
##########
File path: t/core/etcd.t
##########
@@ -0,0 +1,335 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+log_level("info");
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: delete test data if exists
+--- config
+ location /delete {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /delete
+--- no_error_log
+[error]
+--- ignore_response
+
+
+
+=== TEST 2: (add + update + delete) *2 (same uri)
+--- config
+ location /add {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /update {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 2
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /delete {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- pipelined_requests eval
+["GET /add", "GET /hello", "GET /update", "GET /hello", "GET /delete", "GET
/hello",
+"GET /add", "GET /hello", "GET /update", "GET /hello", "GET /delete", "GET
/hello"]
+--- more_headers
+Host: foo.com
+--- error_code eval
+[201, 200, 200, 200, 200, 404, 201, 200, 200, 200, 200, 404]
+--- response_body eval
+["passed\n", "hello world\n", "passed\n", "hello world\n", "passed\n",
"{\"error_msg\":\"failed to match any routes\"}\n",
+"passed\n", "hello world\n", "passed\n", "hello world\n", "passed\n",
"{\"error_msg\":\"failed to match any routes\"}\n"]
+--- no_error_log
+[error]
+--- timeout: 5
+
+
+
+=== TEST 3: add + update + delete + add + update + delete (different uris)
+--- config
+ location /add {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /update {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 2
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/status"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /delete {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /add2 {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello_"
+ }]],
+ nil
+ )
+ ngx.sleep(1)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /update2 {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 2
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello1"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /delete2 {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- pipelined_requests eval
+["GET /add", "GET /hello", "GET /update", "GET /hello", "GET /status", "GET
/delete", "GET /status",
Review comment:
ditto
##########
File path: t/core/etcd-auth-fail.t
##########
@@ -18,24 +18,33 @@ BEGIN {
$ENV{"ETCD_ENABLE_AUTH"} = "false"
}
-use t::APISIX 'no_plan';
+use t::APISIX;
repeat_each(1);
no_long_string();
no_root_location();
log_level("info");
-# Authentication is enabled at etcd and credentials are set
-system('etcdctl --endpoints="http://127.0.0.1:2379"; -u root:5tHkHhYkjr6cQY
user add root:5tHkHhYkjr6cQY');
-system('etcdctl --endpoints="http://127.0.0.1:2379"; -u root:5tHkHhYkjr6cQY
auth enable');
-system('etcdctl --endpoints="http://127.0.0.1:2379"; -u root:5tHkHhYkjr6cQY
role revoke --path "/*" -rw guest');
+my $etcd_version = `etcdctl version`;
+if ($etcd_version =~ /etcdctl version: 3.2/) {
+ plan(skip_all => "skip for etcd version v3.2");
Review comment:
we only support etcd `3.4` in this PR.
so I think we can remove this line.
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +49,144 @@ end
_M.new = new
+local function kvs_to_node(kvs)
+ local node = {}
+ node.key = kvs.key
+ node.value = kvs.value
+ node.createdIndex = tonumber(kvs.create_revision)
+ node.modifiedIndex = tonumber(kvs.mod_revision)
+ return node
+end
+
+local function kvs_to_nodes(res, start_index)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=start_index, #res.body.kvs do
+ if start_index == 1 then
+ res.body.node.nodes[i] = kvs_to_node(res.body.kvs[i])
+ else
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
Review comment:
please confirm this logic, it seems wrong.
if the number of `res.body.kvs` is `2`, all of the data may be set to
`res.body.node.nodes[1]`.
##########
File path: t/core/etcd.t
##########
@@ -0,0 +1,335 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+log_level("info");
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: delete test data if exists
+--- config
+ location /delete {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /delete
+--- no_error_log
+[error]
+--- ignore_response
+
+
+
+=== TEST 2: (add + update + delete) *2 (same uri)
+--- config
+ location /add {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /update {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 2
+ },
+ "type": "roundrobin"
+ },
+ "host": "foo.com",
+ "uri": "/hello"
+ }]],
+ nil
+ )
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+ location /delete {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- pipelined_requests eval
+["GET /add", "GET /hello", "GET /update", "GET /hello", "GET /delete", "GET
/hello",
Review comment:
Your single test case is very complicated. Can it be split into multiple
simple test cases?
it is hard to review and update it later.
##########
File path: bin/apisix
##########
@@ -870,35 +870,21 @@ local function init_etcd(show_output)
local host_count = #(yaml_conf.etcd.host)
- -- check whether the user has enabled etcd v2 protocol
- for index, host in ipairs(yaml_conf.etcd.host) do
- uri = host .. "/v2/keys"
- local cmd = "curl -i -m ".. timeout * 2 .. " -o /dev/null -s -w
%{http_code} " .. uri
- local res = excute_cmd(cmd)
- if res == "404" then
- io.stderr:write(string.format("failed: please make sure that you
have enabled the v2 protocol of etcd on %s.\n", host))
- return
- end
- end
-
local etcd_ok = false
for index, host in ipairs(yaml_conf.etcd.host) do
local is_success = true
- uri = host .. "/v2/keys" .. (etcd_conf.prefix or "")
for _, dir_name in ipairs({"/routes", "/upstreams", "/services",
Review comment:
> we can remove those `for` code in etcd v3 now, it is useless
ignore what I have said, we should keep those codes. Without these base
paths, etcd will fail to subscribe.
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +49,144 @@ end
_M.new = new
+local function kvs_to_node(kvs)
+ local node = {}
+ node.key = kvs.key
+ node.value = kvs.value
+ node.createdIndex = tonumber(kvs.create_revision)
+ node.modifiedIndex = tonumber(kvs.mod_revision)
+ return node
+end
+
+local function kvs_to_nodes(res, start_index)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=start_index, #res.body.kvs do
+ if start_index == 1 then
+ res.body.node.nodes[i] = kvs_to_node(res.body.kvs[i])
+ else
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ end
+ end
+ return res
+end
+
+
+local function not_found(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.get_format(res, realkey)
+ if res.body.error == "etcdserver: user name is empty" then
+ return nil, "insufficient credentials code: 401"
+ end
+
+ res.headers["X-Etcd-Index"] = res.body.header.revision
+
+ if not res.body.kvs then
+ return not_found(res)
+ end
+ res.body.action = "get"
+
+ res.body.node = kvs_to_node(res.body.kvs[1])
+ -- kvs.value = nil, so key is root
+ if type(res.body.kvs[1].value) == "userdata" or not res.body.kvs[1].value
then
+ -- remove last "/" when necesary
+ if string.sub(res.body.node.key, -1, -1) == "/" then
+ res.body.node.key = string.sub(res.body.node.key, 1,
#res.body.node.key-1)
+ end
+ res = kvs_to_nodes(res, 2)
+ -- key not match, so realkey is root
+ elseif res.body.kvs[1].key ~= realkey then
+ res.body.node.key = realkey
+ res = kvs_to_nodes(res, 1)
+ -- first is root (in v2, root not contains value), others are nodes
+ elseif #res.body.kvs > 1 then
+ res = kvs_to_nodes(res, 2)
+ end
+
+ res.body.kvs = nil
+ return res, nil
+end
+
+
+function _M.watch_format(v3res)
Review comment:
I think `yes`.
##########
File path: bin/apisix
##########
@@ -873,35 +873,26 @@ local function init_etcd(show_output)
local host_count = #(yaml_conf.etcd.host)
- -- check whether the user has enabled etcd v2 protocol
- for index, host in ipairs(yaml_conf.etcd.host) do
- uri = host .. "/v2/keys"
- local cmd = "curl -i -m ".. timeout * 2 .. " -o /dev/null -s -w
%{http_code} " .. uri
- local res = excute_cmd(cmd)
- if res == "404" then
- io.stderr:write(string.format("failed: please make sure that you
have enabled the v2 protocol of etcd on %s.\n", host))
- return
- end
- end
-
local etcd_ok = false
for index, host in ipairs(yaml_conf.etcd.host) do
local is_success = true
- uri = host .. "/v2/keys" .. (etcd_conf.prefix or "")
for _, dir_name in ipairs({"/routes", "/upstreams", "/services",
"/plugins", "/consumers", "/node_status",
"/ssl", "/global_rules", "/stream_routes",
"/proto"}) do
- local cmd = "curl " .. uri .. dir_name
- .. "?prev_exist=false -X PUT -d dir=true "
- .. "--connect-timeout " .. timeout
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+
+ local base64_encode = require("base64").encode
Review comment:
we can not use `ngx.encode_base64` in this file, it is Standard Lua land.
the current way is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
