You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/08/10 14:29:17 UTC
[GitHub] [apisix] Yiyiyimu opened a new pull request #2036: feature: support etcd v3, by mocking v2 API
Yiyiyimu opened a new pull request #2036:
URL: https://github.com/apache/apisix/pull/2036
### What this PR does / why we need it:
A simplified way to support etcd v3, comparing to #1943, by mocking both requests and responses of v2 API.
#1767
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488305087
##########
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:
got it. thx
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487852649
##########
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:
@membphis You can get the prototype of ETCD response here:
* https://github.com/etcd-io/etcd/blob/master/etcdserver/etcdserverpb/rpc.proto#L502
* https://github.com/etcd-io/etcd/blob/master/mvcc/mvccpb/kv.proto#L12
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488484438
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +48,134 @@ 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)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=2, #res.body.kvs do
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ 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
Review comment:
does etcd has error code with msg?
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +48,134 @@ 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)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=2, #res.body.kvs do
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ 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"
Review comment:
why not return `res.body.error`?
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +48,134 @@ 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)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=2, #res.body.kvs do
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ 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"
Review comment:
and do we need to deal with other error msg in `res.body.error`?
##########
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:
Need a more meaningful function name
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488331969
##########
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:
I checked the source code of `base64`: https://github.com/iskolbin/lbase64/blob/master/base64.lua
It is pure Lua source code, confirm it works fine with `Lua 5.1+` and `LuaJIT`, and it under MIT license.
We can use this library.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471383503
##########
File path: .github/workflows/build.yml
##########
@@ -12,7 +12,7 @@ jobs:
fail-fast: false
matrix:
platform: [ubuntu-18.04]
- os_name: [linux_openresty, linux_tengine, linux_apisix_master_luarocks, linux_apisix_current_luarocks, linux_openresty_mtls]
+ os_name: [linux_openresty, linux_tengine, linux_apisix_master_luarocks, linux_apisix_current_luarocks, linux_openresty_mtls, linux_openresty_v2]
Review comment:
My fault I'll change it to linux_openresty_etcd_v3
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-693536211
TODO: we need to add doc for etcd migration
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671440970
It seems travis ci [passed in my personal repo](https://travis-ci.com/github/Yiyiyimu/incubator-apisix/builds/179139937), but github CI failed quite early. I'm a bit confused here.
---
UPDATE:
it seems travis ci is using etcd v3.4, but github actions is using etcd v3.2. working on multi-version support
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-678647597
I think we can merge this PR at next week.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475215480
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
https://github.com/apache/apisix/blob/bcba1b81dbce62b330873048d4086bd6b91e40b0/bin/apisix#L882
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680750236
I need more time to review this PR, please take it open one more day.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483745591
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Two CI test: [mtls](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755716#step:6:57) and [current_luarocks(cli_test)](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755675#step:6:279) seems to take `init_etcd` as the premise. Since when no `/admin` got inited, it would return `key not found` error. Do you have any suggestions for this place?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477316456
##########
File path: .travis/linux_apisix_current_luarocks_runner.sh
##########
@@ -50,7 +50,8 @@ script() {
sudo service etcd stop
mkdir -p ~/etcd-data
/usr/bin/etcd --listen-client-urls 'http://0.0.0.0:2379' --advertise-client-urls='http://0.0.0.0:2379' --data-dir ~/etcd-data > /dev/null 2>&1 &
- etcd --version
+ etcdctl --version
+ export ETCDCTL_API=3
Review comment:
Before etcd v3.4, etcd would use v2 API as default, thus we need to set it to v3
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-678748139
> I think we should create a new branch that using etcd v2 before the merge,and maintain it for a while,for the users who can’t migrate to etcd v
I have created a new branch named `etcd-v2` right now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488328057
##########
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:
got it, keep the current way ^_^
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488739432
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +48,134 @@ 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)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=2, #res.body.kvs do
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ 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"
Review comment:
1. The error resulted from `get` when etcd auth failed in v3 is different from v2, so I tried to keep it the same
2. That's the only difference I know between v2 and v3. We could return other error directly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-692411184
> one more thing, we need to check the `etcd` version in `bin/apisix`, confirm the `etcd` version `>= 3.4` .
we can fix this in a new PR, here is the related issue: https://github.com/apache/apisix/issues/2227
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671401256
I make a default benchmark test on my local PC, it seems that deploying etcd v3 could improve performance quite a lot
| | v3 | v2 |
| ---------------------------------- | ------------------------------------------------------------ | ------------------------------------------------------------ |
| 1 worker + 1 upstream + no plugin | + sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.17ms 153.33us 3.44ms 77.84%<br/> Req/Sec 6.83k 271.91 7.53k 72.55%<br/> 69316 requests in 5.10s, 275.85MB read<br/>Requests/sec: 13591.19<br/>Transfer/sec: 54.09MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.18ms 176.65us 5.14ms 83.33%<br/> Req/Sec 6.80k 268.62 7.48k 73.53%<br/> 68972 requests in 5.10s, 274.49MB read<br/>Requests/sec: 13524.55<br/>Transfer/sec: 53.82MB | + sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads
and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.33ms 211.08us 5.95ms 82.60%<br/> Req/Sec 6.05k 289.03 6.63k 71.57%<br/> 61423 requests in 5.10s, 245.32MB read<br/>Requests/sec: 12043.51<br/>Transfer/sec: 48.10MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.31ms 195.78us 3.63ms 81.55%<br/> Req/Sec 6.11k 600.94 11.41k 97.03%<br/> 61396 requests in 5.10s, 245.22MB read<br/>Requests/sec: 12039.57<br/>Transfer/sec: 48.09MB |
| 1 worker + 1 upstream + 2 plugins | + sleep 3<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 558.87us 1.24ms 35.81ms 96.89%<br/> Req/Sec 19.64k 1.91k 28.10k 76.24%<br/> 197213 requests in 5.10s, 801.24MB read<br/>Requests/sec: 38669.03<br/>Transfer/sec: 157.11MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 521.08us 1.05ms 25.48ms 97.08%<br/> Req/Sec 19.72k 1.84k 23.12k 73.00%<br/> 196309 requests in 5.00s, 797.58MB read<br/>Requests/sec: 39251.72<br/>Transfer/sec: 159.47MB | + sleep 3<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threa
ds and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.47ms 252.12us 9.54ms 84.44%<br/> Req/Sec 5.48k 588.37 10.98k 97.03%<br/> 55055 requests in 5.10s, 223.67MB read<br/>Requests/sec: 10796.49<br/>Transfer/sec: 43.86MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.51ms 292.56us 9.64ms 88.99%<br/> Req/Sec 5.34k 540.96 10.24k 97.03%<br/> 53658 requests in 5.10s, 217.99MB read<br/>Requests/sec: 10521.64<br/>Transfer/sec: 42.75MB |
| fake empty apisix server: 1 worker | + sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.20ms 249.70us 8.56ms 90.65%<br/> Req/Sec 6.71k 299.13 7.87k 75.49%<br/> 68112 requests in 5.10s, 271.06MB read<br/>Requests/sec: 13356.74<br/>Transfer/sec: 53.15MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.27ms 1.00ms 18.01ms 98.38%<br/> Req/Sec 6.74k 786.85 13.37k 96.04%<br/> 67753 requests in 5.10s, 269.64MB read<br/>Requests/sec: 13287.07<br/>Transfer/sec: 52.88MB | + sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads
and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.17ms 153.33us 3.44ms 77.84%<br/> Req/Sec 6.83k 271.91 7.53k 72.55%<br/> 69316 requests in 5.10s, 275.85MB read<br/>Requests/sec: 13591.19<br/>Transfer/sec: 54.09MB<br/>+ sleep 1<br/>+ wrk -d 5 -c 16 http://127.0.0.1:9080/hello<br/>Running 5s test @ http://127.0.0.1:9080/hello<br/> 2 threads and 16 connections<br/> Thread Stats Avg Stdev Max +/- Stdev<br/> Latency 1.18ms 176.65us 5.14ms 83.33%<br/> Req/Sec 6.80k 268.62 7.48k 73.53%<br/> 68972 requests in 5.10s, 274.49MB read<br/>Requests/sec: 13524.55<br/>Transfer/sec: 53.82MB |
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680966931
@membphis it seems I could not directly reply in your comment on "etcd v2 version".
The original implementation should directly use the version detection in lua-resty-etcd, but it is somehow not working, as I stated here: https://github.com/apache/apisix/pull/2036#discussion_r475052806
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-678712944
> I think we can merge this PR at next week.
I think we should create a new branch that using etcd v2 before the merge,and maintain it for a while,for the users who can’t migrate to etcd v
3 very soon.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-681198221
> No, apisix and etcd will not in the same machine. Shuyang Wu <no...@github.com> 于 2020年8月26日周三 下午9:51写道：
> […](#)
> *@Yiyiyimu* commented on this pull request. ------------------------------ In apisix/core/etcd.lua <[#2036 (comment)](https://github.com/apache/apisix/pull/2036#discussion_r477316914)>: > local _M = {version = 0.1} +local prefix_v3 = { + ["3.5"] = "/v3", + ["3.4"] = "/v3", + ["3.3"] = "/v3beta", + ["3.2"] = "/v3alpha", +} + + +-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd +-- we don't need to call this so many times, need to save it in some place +local function etcd_version_from_cmd() + local cmd = "export ETCDCTL_API=3 && etcdctl version" etcdctl would be installed alongside with etcd — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <[#2036 (comment)](https://github.com/apache/apisix/pull/2036#discussion_r477316914)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJZBKZIYJTOHVGJMEI3IGDSCUHPJANCNFSM4P2AS3KQ> .
I created a new issue for this: https://github.com/apache/apisix/issues/2125, CI should be adjusted to find such problems
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r489141555
##########
File path: bin/apisix
##########
@@ -879,35 +887,35 @@ local function init_etcd(show_output)
local host_count = #(yaml_conf.etcd.host)
- -- check whether the user has enabled etcd v2 protocol
+ local etcd_ok = false
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
+ -- check if etcd version above 3.4
+ cmd = "curl " .. host .. "/version 2>&1"
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))
+ local op_ver = str_split(res, "\"")[4]
+ local need_ver = "3.4.0"
+ if not check_version(op_ver, need_ver) then
+ io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
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
+ local uri = host .. "/v3/kv/put"
+ local post_json = '{"value":"' .. base64_encode("init_dir") .. '", "key":"' .. base64_encode(key) .. '"}'
+ cmd = "curl " .. uri .. " -X POST -d '" .. post_json
+ .. "' --connect-timeout " .. timeout
.. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
local res = excute_cmd(cmd)
- if not res:find("index", 1, true)
- and not res:find("createdIndex", 1, true) then
+ if (etcd_version == "v3" and not res:find("OK", 1, true)) then
Review comment:
you can create a new issue about this
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-672564838
Fixed by iterating through watch response.
---
~~DEBUG HELP NEEDED!!~~
Currently there is only one error in test file, which is the last test of t/plugin/key-auth.t. Normally in etcd v2, it would add 20 consumers and find the 13th. But in current implementation of etcd v3, the test would add 20 consumers but only get first three of them, so it could not get the 13th. However, if I rerun the test, the test would get all 20 consumers and passed.
I think it might related to the implementation of waitdir between two versions, but I could not find the way to debug.
I print `self.values_hash` at the start of each `sync_data`, and the log is [here](https://gist.github.com/Yiyiyimu/f98647847ef49440454c9fff8a4f7295), maybe it could be of help.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671945683
Currently implemented multi-version support (auto change etcd prefix) in t/APISIX.pm, worked for most tests. But for test files like t/admin/stream-routes-disable.t, which change the content of config.yaml before test would not get the prefix change in APISIX.pm.
So right now mannually set etcd prefix to "/v3alpha" to try to pass the github ci
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475196578
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
Review comment:
fixed
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
local res = excute_cmd(cmd)
- if not res:find("index", 1, true)
- and not res:find("createdIndex", 1, true) then
+ if (etcd_version == "v3" and not res:find("OK", 1, true)) then
+ -- (etcd_version == "v3" and not res:find("revision", 1, true)) then
Review comment:
fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-674666089
> Is there any reason that could cause the difference? Or the default benchmark test is not suitable for this change
I think you need to check the error log first for confirming some detail. If you need more help, you need to provide your benchmark step one by one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488042113
##########
File path: .travis/linux_tengine_runner.sh
##########
@@ -259,6 +259,12 @@ do_install() {
sudo luarocks install luacheck > build.log 2>&1 || (cat build.log && exit 1)
+ wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
+ tar xf etcd-v3.4.0-linux-amd64.tar.gz
+ sudo cp etcd-v3.4.0-linux-amd64/etcd /usr/local/bin/
+ sudo cp etcd-v3.4.0-linux-amd64/etcdctl /usr/local/bin/
+ rm -rf etcd-v3.4.0-linux-amd64
Review comment:
fixed by @nic-chen thx
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477402880
##########
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
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
Review comment:
here is an example code: https://github.com/api7/lua-resty-etcd/blob/master/lib/resty/etcd.lua#L17
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
Review comment:
If we only fetch the etcd version, we can use `etcd` v2 library, it should work fine.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475095648
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,36 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
Review comment:
style: two blank lines
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
Review comment:
we can remove the old comments?
##########
File path: apisix/core/etcd.lua
##########
@@ -43,25 +70,140 @@ local function new()
end
_M.new = new
+local function kvs2node(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
+
Review comment:
two blank lines between different functions.
and please fix some other similar points
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
local res = excute_cmd(cmd)
- if not res:find("index", 1, true)
- and not res:find("createdIndex", 1, true) then
+ if (etcd_version == "v3" and not res:find("OK", 1, true)) then
+ -- (etcd_version == "v3" and not res:find("revision", 1, true)) then
Review comment:
if it is useless, we can remove it
##########
File path: doc/install-dependencies.md
##########
@@ -28,12 +28,12 @@
Note
====
-- Apache APISIX currently only supports the v2 protocol storage to etcd, but the latest version of etcd (starting with 3.4) has turned off the v2 protocol by default.
+- Since v2.0, Apache APISIX would not support the v2 protocol storage to etcd anymore. If etcd version is below 3.4, the default protocol is still v2 and you need to turn on v3 protocol mannually.
Review comment:
we still not release `v2.0` now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477318270
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +73,143 @@ end
_M.new = new
+local function kvs2node(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 notfound(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.postget(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
+
Review comment:
It is to transfer the etcd v3 output structure to the same as the original v2, to keep all the interfaces the same.
Maybe they need to have a better function name
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475192884
##########
File path: apisix/core/config_etcd.lua
##########
@@ -268,86 +289,90 @@ local function sync_data(self)
return false, err
end
- local key = short_key(self, res.key)
- if res.value and type(res.value) ~= "table" then
- self:upgrade_version(res.modifiedIndex)
- return false, "invalid item data of [" .. self.key .. "/" .. key
- .. "], val: " .. tostring(res.value)
- .. ", it shoud be a object"
- end
-
- if res.value and self.item_schema then
- local ok, err = check_schema(self.item_schema, res.value)
- if not ok then
+ local res_copy = res
+ for _, res in ipairs(res_copy) do
+ local key = short_key(self, res.key)
+ if res.value and type(res.value) ~= "table" then
self:upgrade_version(res.modifiedIndex)
-
- return false, "failed to check item data of ["
- .. self.key .. "] err:" .. err
+ return false, "invalid item data of [" .. self.key .. "/" .. key
+ .. "], val: " .. tostring(res.value)
Review comment:
bad indentation
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
why we need this Lua module?
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
Review comment:
only one way is easier for user. `etcdctl` is enough.
##########
File path: apisix/core/config_etcd.lua
##########
@@ -268,86 +289,90 @@ local function sync_data(self)
return false, err
end
- local key = short_key(self, res.key)
- if res.value and type(res.value) ~= "table" then
- self:upgrade_version(res.modifiedIndex)
- return false, "invalid item data of [" .. self.key .. "/" .. key
- .. "], val: " .. tostring(res.value)
- .. ", it shoud be a object"
- end
-
- if res.value and self.item_schema then
- local ok, err = check_schema(self.item_schema, res.value)
- if not ok then
+ local res_copy = res
+ for _, res in ipairs(res_copy) do
+ local key = short_key(self, res.key)
+ if res.value and type(res.value) ~= "table" then
self:upgrade_version(res.modifiedIndex)
-
- return false, "failed to check item data of ["
- .. self.key .. "] err:" .. err
+ return false, "invalid item data of [" .. self.key .. "/" .. key
+ .. "], val: " .. tostring(res.value)
Review comment:
if the `res.value` is a talbe, we need to encode it with `json`
if the `res.value` is non-table, we do not need to call `tostring`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488751665
##########
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:
The change [I made last time](https://github.com/apache/apisix/pull/2036#discussion_r477341702)
Maybe I need some suggestions on how to name these two functions 🤣
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680972604
> @membphis it seems I could not directly reply in your comment on "etcd v2 version".
> The original implementation should directly use the version detection in lua-resty-etcd, but it is somehow not working, as I stated here: [#2036 (comment)](https://github.com/apache/apisix/pull/2036#discussion_r475052806)
here is another way, you can make a try:
```
$ curl http://127.0.0.1:2379/version
{"etcdserver":"3.2.26","etcdcluster":"3.2.0"}%
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-672564838
Fixed by iterating through watch response.
---
**DEBUG HELP NEEDED!!**
Currently there is only one error in test file, which is the last test of t/plugin/key-auth.t. Normally in etcd v2, it would add 20 consumers and find the 13th. But in current implementation of etcd v3, the test would add 20 consumers but only get first three of them, so it could not get the 13th. However, if I rerun the test, the test would get all 20 consumers and passed.
I think it might related to the implementation of waitdir between two versions, but I could not find the way to debug.
I print `self.values_hash` at the start of each `sync_data`, and the log is [here](https://gist.github.com/Yiyiyimu/f98647847ef49440454c9fff8a4f7295), maybe it could be of help.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487989714
##########
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:
got it. many thx
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475051880
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
Review comment:
etcd v3 is not suggested to use `curl` to call etcd API due to we need to transmit base64 encoded key and value, and base64 library is somehow broken here.
Thus I turned to use suggested `etcdctl` to do the same thing, but in this case, we could not set `connect-timeout` and `max-time` anymore, does that matter?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488672188
##########
File path: utils/install-etcd.sh
##########
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+#
+# 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.
+#
+
+wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
Review comment:
sure.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-672564838
**DEBUG HELP NEEDED!!**
Currently there is only one error in test file, which is the last test of t/plugin/key-auth.t. Normally in etcd v2, it would add 20 consumers and find the 13th. But in current implementation of etcd v3, the test would add 20 consumers but only get first three of them, so it could not get the 13th. However, if I rerun the test, the test would get all 20 consumers and passed.
I think it might related to the implementation of waitdir between two versions, but I could not find the way to debug.
I print `self.values_hash` at the start of each `sync_data`, and the log is [here](https://gist.github.com/Yiyiyimu/f98647847ef49440454c9fff8a4f7295), maybe it could be of help.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r489460767
##########
File path: bin/apisix
##########
@@ -879,35 +887,35 @@ local function init_etcd(show_output)
local host_count = #(yaml_conf.etcd.host)
- -- check whether the user has enabled etcd v2 protocol
+ local etcd_ok = false
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
+ -- check if etcd version above 3.4
+ cmd = "curl " .. host .. "/version 2>&1"
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))
+ local op_ver = str_split(res, "\"")[4]
+ local need_ver = "3.4.0"
+ if not check_version(op_ver, need_ver) then
+ io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
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
+ local uri = host .. "/v3/kv/put"
+ local post_json = '{"value":"' .. base64_encode("init_dir") .. '", "key":"' .. base64_encode(key) .. '"}'
+ cmd = "curl " .. uri .. " -X POST -d '" .. post_json
+ .. "' --connect-timeout " .. timeout
.. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
local res = excute_cmd(cmd)
- if not res:find("index", 1, true)
- and not res:find("createdIndex", 1, true) then
+ if (etcd_version == "v3" and not res:find("OK", 1, true)) then
Review comment:
fix by #2233
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488485180
##########
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
+ local res, err = res_fun()
+ if not res or not res.result or not res.result.events then
+ res, err = res_fun()
+ end
+
Review comment:
in etcd v3, the 1st response of watch is the success of watch creation, useless to us
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471395590
##########
File path: conf/config.yaml
##########
@@ -129,6 +129,8 @@ etcd:
- "http://127.0.0.1:2379" # multiple etcd address
prefix: "/apisix" # apisix configurations prefix
timeout: 30 # 30 seconds
+ version: "v3" # etcd version: v2/v3
+ api_prefix: "/v3alpha" # 3.2 -> "/v3alpha", 3.3 -> "/v3beta", 3.4+ -> "/v3"
Review comment:
api_prefix has implemented to be detectd and set by lua code, as changed in [t/APISIX.pm](https://github.com/apache/apisix/blob/97809d8fabf29e78d5470835f8f53888bcb2f23e/t/APISIX.pm#L83).
I could remove it and move those lua code to etcd_v3.lua if needed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-692788881
Currently the log produced by lua-resty-etcd would show message without base64 decode. For example
```
2020/09/15 23:18:52 [info] 1964#1964: *2 [lua] v3.lua:284: set(): v3 set body: {"prev_kv":{"value":"InRlc3RfdmFsdWUi","create_revision":"149","mod_revision":"150","key":"L2FwaXNpeC90ZXN0X2tleQ==","version":"2"},"header":{"raft_term":"2","cluster_id":"8925027824743593106","member_id":"13803658152347727308","revision":"151"}}, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
2020/09/15 23:18:52 [info] 1964#1964: *15 [lua] v3.lua:500: request_chunk(): http request method: POST path: /v3/watch body: {"create_request":{"range_end":"L2FwaXNpeC91cHN0cmVhbXQ=","start_revision":151,"key":"L2FwaXNpeC91cHN0cmVhbXM="}} query: nil, context: ngx.timer
```
Shall we do the decode on etcd side, or just remove these logs like v2 did?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477341702
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +73,143 @@ end
_M.new = new
+local function kvs2node(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 notfound(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.postget(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
+
Review comment:
I changed:
`postget` -> `get_res_format`
`postwatch` -> `watch_res_format`
Maybe that would be more readable
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477143439
##########
File path: .travis/linux_openresty_mtls_runner.sh
##########
@@ -94,7 +94,8 @@ script() {
sudo service etcd stop
mkdir -p ~/etcd-data
/usr/bin/etcd --listen-client-urls 'http://0.0.0.0:2379' --advertise-client-urls='http://0.0.0.0:2379' --data-dir ~/etcd-data > /dev/null 2>&1 &
- etcd --version
+ etcdctl --version
+ export ETCDCTL_API=3
Review comment:
ditto
##########
File path: .travis/linux_apisix_current_luarocks_runner.sh
##########
@@ -50,7 +50,8 @@ script() {
sudo service etcd stop
mkdir -p ~/etcd-data
/usr/bin/etcd --listen-client-urls 'http://0.0.0.0:2379' --advertise-client-urls='http://0.0.0.0:2379' --data-dir ~/etcd-data > /dev/null 2>&1 &
- etcd --version
+ etcdctl --version
+ export ETCDCTL_API=3
Review comment:
why need `export ETCDCTL_API=3`? Is etcd support v3 by default in CI?
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
+ local t, err = io.popen(cmd)
+ if not t then
+ return nil, "failed to execute command: " .. cmd .. ", error info:" .. err
+ end
+ local data = t:read("*all")
+ t:close()
+ return prefix_v3[data:sub(-4,-2)]
+end
+_M.etcd_version_from_cmd = etcd_version_from_cmd
Review comment:
if `etcd_version_from_cmd` failed, `_M.etcd_version_from_cmd` will be `nil`, is test case cover this?
##########
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",
"/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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ local cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
Review comment:
`etcdctl` maybe not install in apisix node
##########
File path: t/core/etcd-auth.t
##########
@@ -18,23 +18,28 @@ BEGIN {
$ENV{"ETCD_ENABLE_AUTH"} = "true"
}
-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");
+} else {
Review comment:
why etcd 3.2 is special？
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
Review comment:
maybe we can only support 3.4+?
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
Review comment:
Is user must install `etcdctl` in the APISIX node?
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +73,143 @@ end
_M.new = new
+local function kvs2node(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 notfound(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.postget(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
+
Review comment:
To be honest, I don’t understand these codes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475100649
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,36 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
Review comment:
Thank you!
fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475196805
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
To set environment variable ETCDCTL_API=3. Is there better way to implement this?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475100953
##########
File path: doc/install-dependencies.md
##########
@@ -28,12 +28,12 @@
Note
====
-- Apache APISIX currently only supports the v2 protocol storage to etcd, but the latest version of etcd (starting with 3.4) has turned off the v2 protocol by default.
+- Since v2.0, Apache APISIX would not support the v2 protocol storage to etcd anymore. If etcd version is below 3.4, the default protocol is still v2 and you need to turn on v3 protocol mannually.
Review comment:
Sure I'll fix it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu removed a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu removed a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-686846176
Hi @membphis I noticed there are still errrors in your latest commit like https://github.com/apache/apisix/runs/1046997001#step:6:423, but since there is no `no_error_log` add to the unit test, it's still be regarded as success in CI. Is there any reason that we do not add `no_error_log` to these or we do need to do so.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-675165303
you can create a discuss thread in mailinglist for this issue, which is a
big change.
Shuyang Wu <no...@github.com> 于 2020年8月17日周一 下午6:38写道：
> *@Yiyiyimu* commented on this pull request.
> ------------------------------
>
> In apisix/core.lua
> <https://github.com/apache/apisix/pull/2036#discussion_r471392464>:
>
> > @@ -19,7 +19,9 @@ local local_conf = require("apisix.core.config_local").local_conf()
>
> local config_center = local_conf.apisix and local_conf.apisix.config_center
> or "etcd"
> +local etcd_version = local_conf.etcd.version == "v3" and "_v3" or ""
>
> Currently, the implementation is to use v2 as the default, and user would
> change to v3 if needed. Of course we can use v3 as default.
>
> ping @nic-chen <https://github.com/nic-chen> @membphis
> <https://github.com/membphis> maybe we need to discuss about it
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2036#discussion_r471392464>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK3MOR6XOWTTQEQIPL3SBECDZANCNFSM4P2AS3KQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-688567402
I added this PR to 2.0 milestone, which due by September 18, 2020
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477341702
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +73,143 @@ end
_M.new = new
+local function kvs2node(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 notfound(res)
+ res.body.message = "Key not found"
+ res.reason = "Not found"
+ res.status = 404
+ return res
+end
+
+
+function _M.postget(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
+
Review comment:
I changed:
`postget` -> `get_format`
`postwatch` -> `watch_format`
Maybe that would be more readable
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488306915
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "base64 = 1.5-2"
Review comment:
same as https://github.com/apache/apisix/pull/2036/files#r487795007
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-691920387
No, we need to change 4 files if we upgrade etcd version.
Thanks,
Ming Wen
Twitter: _WenMing
YuanSheng Wang <no...@github.com> 于2020年9月14日周一 下午4:55写道：
> *@membphis* commented on this pull request.
> ------------------------------
>
> In .travis/linux_tengine_runner.sh
> <https://github.com/apache/apisix/pull/2036#discussion_r487755488>:
>
> > + wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
> + tar xf etcd-v3.4.0-linux-amd64.tar.gz
> + sudo cp etcd-v3.4.0-linux-amd64/etcd /usr/local/bin/
> + sudo cp etcd-v3.4.0-linux-amd64/etcdctl /usr/local/bin/
> + rm -rf etcd-v3.4.0-linux-amd64
>
> Duplication is acceptable here so that each use case script is independent.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2036#discussion_r487755488>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBKYY2SBLAY2LUTGYNW3SFXLAZANCNFSM4P2AS3KQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r479714677
##########
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:
I noticed users could still set up multiple addresses for the etcd cluster, so maybe we keep the loop?
https://github.com/apache/apisix/blob/c54aec8f6cec2bd31c7c8717bf5220327584be03/conf/config-default.yaml#L133-L135
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475100877
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
Review comment:
> etcd v3 is not suggested to use `curl` to call etcd API due to we need to transmit base64 encoded key and value, and base64 library is somehow broken here.
> Thus I turned to use suggested `etcdctl` to do the same thing, **but in this case, we could not set `connect-timeout` and `max-time` anymore, does that matter**?
Would this matter?
I left the comment here to remind me ask this question... I'll remove it if it does not matter.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-686846176
Hi @membphis I noticed there are still errrors in your latest commit like https://github.com/apache/apisix/runs/1046997001#step:6:423, but since there is no `no_error_log` add to the unit test, it's still be regarded as success in CI. Is there any reason that we do not add `no_error_log` to these or we do need to do so.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488745722
##########
File path: apisix/core/etcd.lua
##########
@@ -44,24 +48,134 @@ 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)
+ res.body.node.dir = true
+ res.body.node.nodes = {}
+ for i=2, #res.body.kvs do
+ res.body.node.nodes[i-1] = kvs_to_node(res.body.kvs[i])
+ 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
Review comment:
Some does and some don't. For example in this case, ngx.status is set to 400.
```
{
body = {
code = 3,
error = "etcdserver: user name is empty",
message = "etcdserver: user name is empty"
},
body_reader = <function 1>,
has_body = true,
headers = {...},
read_body = <function 4>,
read_trailers = <function 5>,
reason = "Bad Request",
status = 400,
trailer_reader = <function 6>
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-691957197
one more thing, we need to check the `etcd` version in `bin/apisix`, confirm the `etcd` version `>= 3.4` .
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-674549680
> Use v3 or v2 protocol, their test results should be the same. Because `etcd` is incrementally notified, most requests are in the `wait` status.
Is there any reason that could cause the difference? Or the default benchmark test is not suitable for this change
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671412054
TODO:
- [x] currenly only test v3, **test for both v2 and v3**
- [x] currently it only support etcd v2 and v3.3+, **do we need to support other etcd version between these two**?
- [ ] documentation of migrating data from v2 to v3, to avoid data loss
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671412054
TODO:
- [ ] currenly only test v3, **test for both v2 and v3**
- [ ] currently it only support etcd v2 and v3.3+, **do we need to support other etcd version between these two**?
- [ ] documentation of migrating data from v2 to v3, to avoid data loss
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488305164
##########
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:
got it, thx
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487762501
##########
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
+ local res, err = res_fun()
+ if not res or not res.result or not res.result.events then
+ res, err = res_fun()
+ end
+
Review comment:
why call `res_fun` twice?
##########
File path: utils/install-etcd.sh
##########
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+#
+# 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.
+#
+
+wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
Review comment:
Just a suggestion, you can refer to the way of docker: https://github.com/apache/apisix/pull/2225/files#diff-65e6a3c4290328a0a57797b4cf3de4d2R39.
we can not modify it in this PR
##########
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)
Review comment:
`fun` -> `func`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475199087
##########
File path: apisix/core/config_etcd.lua
##########
@@ -268,86 +289,90 @@ local function sync_data(self)
return false, err
end
- local key = short_key(self, res.key)
- if res.value and type(res.value) ~= "table" then
- self:upgrade_version(res.modifiedIndex)
- return false, "invalid item data of [" .. self.key .. "/" .. key
- .. "], val: " .. tostring(res.value)
- .. ", it shoud be a object"
- end
-
- if res.value and self.item_schema then
- local ok, err = check_schema(self.item_schema, res.value)
- if not ok then
+ local res_copy = res
+ for _, res in ipairs(res_copy) do
+ local key = short_key(self, res.key)
+ if res.value and type(res.value) ~= "table" then
self:upgrade_version(res.modifiedIndex)
-
- return false, "failed to check item data of ["
- .. self.key .. "] err:" .. err
+ return false, "invalid item data of [" .. self.key .. "/" .. key
+ .. "], val: " .. tostring(res.value)
Review comment:
Thank you! Both fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488305432
##########
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:
many thx for your explain
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471383287
##########
File path: .travis/linux_openresty_v2_runner.sh
##########
@@ -0,0 +1,198 @@
+#!/usr/bin/env bash
Review comment:
the other one test etcd v3, and this test for v2. Thus this file change etcd protocol in config.yaml to `v2` and didn't set ETCDCTL_API to 3
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r479714677
##########
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:
I noticed users could still set up multiple addresses for the etcd cluster, so maybe we need to keep the loop?
https://github.com/apache/apisix/blob/c54aec8f6cec2bd31c7c8717bf5220327584be03/conf/config-default.yaml#L133-L135
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471392464
##########
File path: apisix/core.lua
##########
@@ -19,7 +19,9 @@ local local_conf = require("apisix.core.config_local").local_conf()
local config_center = local_conf.apisix and local_conf.apisix.config_center
or "etcd"
+local etcd_version = local_conf.etcd.version == "v3" and "_v3" or ""
Review comment:
Currently, the implementation is to use v2 as the default, and user would change to v3 if needed. Of course we can use v3 as default.
ping @nic-chen @membphis maybe we need to discuss about it
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-692553222
> > one more thing, we need to check the `etcd` version in `bin/apisix`, confirm the `etcd` version `>= 3.4` .
>
> we can fix this in a new PR, here is the related issue: #2227
fix #2227 in new commit
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475196637
##########
File path: bin/apisix
##########
@@ -870,35 +870,30 @@ 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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local res = require("posix.stdlib").setenv("ETCDCTL_API", 3)
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ -- use suggested v3 ctl `etcdctl` and avoid base64 conversion
+ --local base64 = require("base64")
+ --uri = host .. "/v3/kv/put"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ local cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
Review comment:
fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477322262
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
Review comment:
The official apt source - which is used right now for apisix - currently only supports v3.2.x.
We could support v3.4 by using etcd github source if needed, but there are chances that the etcd deployed in users' environment is below v3.4
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483745591
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Two CI test: [mtls](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755716#step:6:57) and [current_luarocks(cli_test)](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755675#step:6:279) seems to take `init_etcd` as the premise. Since no /admin would be init, it would return `key not found` error. Do you have any suggestions for this place?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r468490250
##########
File path: bin/apisix
##########
@@ -811,35 +811,52 @@ 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
+ -- check whether the user has enabled etcd v2 protocol for v2 API
+ local etcd_version = etcd_conf.version or "v2"
+ if etcd_version == "v2" then
+ 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
end
+ --local base64 = require("base64")
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 "")
+ local cmd
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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ if etcd_version == "v3" then
+ --uri = host .. "/v3/kv/put"
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
Review comment:
maybe we need to specify etcd version here ?
ETCDCTL_API=3
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477316914
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
Review comment:
etcdctl would be installed alongside with etcd
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471343434
##########
File path: .travis/linux_openresty_v2_runner.sh
##########
@@ -0,0 +1,198 @@
+#!/usr/bin/env bash
Review comment:
What is the difference between this file and `linux_openresty_runner.sh`?
##########
File path: conf/config.yaml
##########
@@ -129,6 +129,8 @@ etcd:
- "http://127.0.0.1:2379" # multiple etcd address
prefix: "/apisix" # apisix configurations prefix
timeout: 30 # 30 seconds
+ version: "v3" # etcd version: v2/v3
+ api_prefix: "/v3alpha" # 3.2 -> "/v3alpha", 3.3 -> "/v3beta", 3.4+ -> "/v3"
Review comment:
So we don't need to add configures for that.
##########
File path: .github/workflows/build.yml
##########
@@ -12,7 +12,7 @@ jobs:
fail-fast: false
matrix:
platform: [ubuntu-18.04]
- os_name: [linux_openresty, linux_tengine, linux_apisix_master_luarocks, linux_apisix_current_luarocks, linux_openresty_mtls]
+ os_name: [linux_openresty, linux_tengine, linux_apisix_master_luarocks, linux_apisix_current_luarocks, linux_openresty_mtls, linux_openresty_v2]
Review comment:
what is `linux_openresty_v2`? I think we need a meaningful name
##########
File path: conf/config.yaml
##########
@@ -129,6 +129,8 @@ etcd:
- "http://127.0.0.1:2379" # multiple etcd address
prefix: "/apisix" # apisix configurations prefix
timeout: 30 # 30 seconds
+ version: "v3" # etcd version: v2/v3
+ api_prefix: "/v3alpha" # 3.2 -> "/v3alpha", 3.3 -> "/v3beta", 3.4+ -> "/v3"
Review comment:
we should only support `v3` and `/v3`
##########
File path: apisix/core.lua
##########
@@ -19,7 +19,9 @@ local local_conf = require("apisix.core.config_local").local_conf()
local config_center = local_conf.apisix and local_conf.apisix.config_center
or "etcd"
+local etcd_version = local_conf.etcd.version == "v3" and "_v3" or ""
Review comment:
IMO, we should ONLY supports etcd v3, not etcd v2.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r479726294
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
as I said, we do not need `init_etcd` in v3 protocol.
I think we can remove the function `init_etcd` totally.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487867038
##########
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:
This is used to test `watch`, if split it, I am afraid that the test will not be effective.
Or could you please give a better suggestion ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477333950
##########
File path: t/core/etcd-auth.t
##########
@@ -18,23 +18,28 @@ BEGIN {
$ENV{"ETCD_ENABLE_AUTH"} = "true"
}
-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");
+} else {
Review comment:
I'm not so sure about this. I couldn't find relevant changes in etcd v3.3 CHANGELOG. I did this because 1. the lua-resty-etcd side auth test also skips v3.2 and 2. there are errors using the current test in v3.2.
I'm in a hurry these days and I'll fix it later. I think it could be fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-693429050
@Yiyiyimu you can create a new Github issue if you find some other things.
this PR has been merged, we should use the new issue to resolve the problem.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487992864
##########
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:
Actually we are iterating through `i`, not `start_index`. Assume `start_index = 1`, nodes[1] would be kvs[1], and nodes[2] = kvs[2].
BTW, `start_index` is to choose whether the first node of kvs should be included in nodes, `1` means include and `2` means not.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-692553222
> > one more thing, we need to check the `etcd` version in `bin/apisix`, confirm the `etcd` version `>= 3.4` .
>
> we can fix this in a new PR, here is the related issue: #2227
fix #2227
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r469744994
##########
File path: apisix/core/config_etcd.lua
##########
@@ -48,14 +49,15 @@ local mt = {
return " etcd key: " .. self.key
end
}
+local etcd_version = _M.local_conf().etcd.version or "v2"
Review comment:
Create a new `config_etcd_v3.lua` for v3, instead of mixing the logic of v2 and v3, it may be more clear.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-671440970
It seems travis ci [passed in my personal repo](https://travis-ci.com/github/Yiyiyimu/incubator-apisix/builds/179139937), but github CI failed quite early. I'm a bit confused here.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-673298480
> Currently there is only one error in test file, which is the last test of t/plugin/key-auth.t. Normally in etcd v2, it would add 20 consumers and find the 13th. But in current implementation of etcd v3, the test would add 20 consumers but only get first three of them, so it could not get the 13th. However, if I rerun the test, the test would get all 20 consumers and passed.
I think your guess is correct, it should be a `waitdir` problem, you can debug to see why the etcd data is not synchronized in time
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475051124
##########
File path: apisix/core/config_etcd.lua
##########
@@ -48,14 +49,15 @@ local mt = {
return " etcd key: " .. self.key
end
}
+local etcd_version = _M.local_conf().etcd.version or "v2"
Review comment:
Support etcd v3 only so we don't need to worry about that
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483745591
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Two CI test: [mtls](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755716#step:6:57) and [current_luarocks(cli_test)](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755675#step:6:279) seems to take `init_etcd` as the premise, which return 404 due to `key not found` error. Do you have any suggestions for this place?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483479509
##########
File path: t/core/etcd-auth.t
##########
@@ -18,23 +18,28 @@ BEGIN {
$ENV{"ETCD_ENABLE_AUTH"} = "true"
}
-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");
+} else {
Review comment:
auth in etcd grpc-gateway is supported since v3.3, as discussed in [this issue](https://github.com/etcd-io/etcd/issues/6643).
_Originally posted by @Yiyiyimu in https://github.com/api7/lua-resty-etcd/pull/41#issuecomment-686303994_
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483375985
##########
File path: apisix/core/config_etcd.lua
##########
@@ -453,7 +476,7 @@ function _M.new(key, opts)
values = nil,
need_reload = true,
Review comment:
The problem is not here. Closed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483745591
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Two CI test: [mtls](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755716#step:6:57) and [current_luarocks(cli_test)](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1072755675#step:6:279) seems to take `init_etcd` as the premise. Actually it seems should not relate to `init_etcd`... Do you have any suggestions for this place?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680909763
No, apisix and etcd will not in the same machine.
Shuyang Wu <no...@github.com> 于 2020年8月26日周三 下午9:51写道：
> *@Yiyiyimu* commented on this pull request.
> ------------------------------
>
> In apisix/core/etcd.lua
> <https://github.com/apache/apisix/pull/2036#discussion_r477316914>:
>
> >
> local _M = {version = 0.1}
>
> +local prefix_v3 = {
> + ["3.5"] = "/v3",
> + ["3.4"] = "/v3",
> + ["3.3"] = "/v3beta",
> + ["3.2"] = "/v3alpha",
> +}
> +
> +
> +-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
> +-- we don't need to call this so many times, need to save it in some place
> +local function etcd_version_from_cmd()
> + local cmd = "export ETCDCTL_API=3 && etcdctl version"
>
> etcdctl would be installed alongside with etcd
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2036#discussion_r477316914>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBKZIYJTOHVGJMEI3IGDSCUHPJANCNFSM4P2AS3KQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483479109
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
Review comment:
support v3.4 as default
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483345966
##########
File path: apisix/core/config_etcd.lua
##########
@@ -453,7 +476,7 @@ function _M.new(key, opts)
values = nil,
need_reload = true,
Review comment:
One option is to set `need_reload` to false when init `/consumer`, but I'm not sure changing it would still stay the same with real situation,
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-689952623
The newly imported lib [base64](https://github.com/iskolbin/lbase64) holds MIT License and only 200 lines. So I think we could directly use it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-692401902
Note:
I found that run `etcd` v3 on docker for mac cause high CPU usage, like:
https://github.com/etcd-io/etcd/issues/11460
Running on Linux is ok.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483344517
##########
File path: apisix/core/config_etcd.lua
##########
@@ -453,7 +476,7 @@ function _M.new(key, opts)
values = nil,
need_reload = true,
Review comment:
Hi @nic-chen , currently after removing init_etcd, test would fail in etcd_sync.t that it could not auto update prev_index as shown [here](https://github.com/apache/apisix/pull/2036/checks?check_run_id=1066582120#step:6:107). Since removing init_etcd would not set need_reload to false so it could not get to watch anymore.
Do you have any suggestions on how to modify etcd_sync test file
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r486040363
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
After discussing with @nic-chen, we think we should keep `init_etcd` for now. I imported another tiny base64 encode lib for the this step.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680923213
> No, apisix and etcd will not in the same machine.
If they are not on the same machine, could etcdctl installed on apisix's machine connect to the other machine's etcd?
If not and the only way to connect the other machine's etcd is through gRPC gateway, I think I'll some more time on this feature.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487997808
##########
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:
I think supporting etcd v3 has been added as [one of the milestones](https://github.com/apache/apisix/pull/2036#issuecomment-688567402) of 2.0. Or I could remove phrase `since APISIX v2.0`.
I'm not sure what kind of comment we need to add.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-693018270
> Currently the log produced by lua-resty-etcd would show message without base64 decode. For example
>
> ```
>
> 2020/09/15 23:18:52 [info] 1964#1964: *2 [lua] v3.lua:284: set(): v3 set body: {"prev_kv":{"value":"InRlc3RfdmFsdWUi","create_revision":"149","mod_revision":"150","key":"L2FwaXNpeC90ZXN0X2tleQ==","version":"2"},"header":{"raft_term":"2","cluster_id":"8925027824743593106","member_id":"13803658152347727308","revision":"151"}}, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
>
> 2020/09/15 23:18:52 [info] 1964#1964: *15 [lua] v3.lua:500: request_chunk(): http request method: POST path: /v3/watch body: {"create_request":{"range_end":"L2FwaXNpeC91cHN0cmVhbXQ=","start_revision":151,"key":"L2FwaXNpeC91cHN0cmVhbXM="}} query: nil, context: ngx.timer
>
> ```
>
> Shall we do the decode on etcd side, or just remove these logs like v2 did?
we could optimize it in another pr
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488008439
##########
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:
I rewrite `=== TEST 2: (add + update + delete) *2 (same uri)` right now, please confirm if it is acceptable.
@Yiyiyimu @nic-chen
```
=== TEST 2: create route(id: 1)
--- config
location /t {
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)
}
}
--- request
GET /t
--- error_code: 201
--- no_error_log
[error]
=== TEST 3: missing route(no host)
--- request
GET /hello
--- error_code: 404
--- no_error_log
[error]
=== TEST 4: hit routes
--- request
GET /hello
--- more_headers
Host: foo.com
--- response_body
hello world
--- no_error_log
[error]
=== TEST 5: update route(host: foo2.com)
--- config
location /t {
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": "foo2.com",
"uri": "/hello"
}]],
nil
)
ngx.status = code
ngx.say(body)
}
}
--- request
GET /t
--- error_code: 200
--- no_error_log
[error]
=== TEST 6: missing route(no host)
--- request
GET /hello
--- more_headers
Host: foo.com
--- error_code: 404
--- no_error_log
[error]
=== TEST 7: hit routes
--- request
GET /hello
--- more_headers
Host: foo2.com
--- response_body
hello world
--- no_error_log
[error]
=== TEST 8: delete route
--- config
location /t {
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 /t
--- error_code: 200
--- no_error_log
[error]
=== TEST 9: hit nothing
--- request
GET /hello
--- more_headers
Host: foo2.com
--- error_code: 404
--- no_error_log
[error]
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488019814
##########
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:
thx! fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-672938803
> I make a default benchmark test on my local PC, it seems that deploying etcd v3 could improve performance quite a lot
Use v3 or v2 protocol, their test results should be the same. Because `etcd` is incrementally notified, most requests are in the `wait` status.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487990382
##########
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:
let me make a try ^_^
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475100669
##########
File path: apisix/core/etcd.lua
##########
@@ -43,25 +70,140 @@ local function new()
end
_M.new = new
+local function kvs2node(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
+
Review comment:
Thank you!
fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475052806
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,36 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
+ local t, err = io.popen(cmd)
+ if not t then
+ return nil, "failed to execute command: " .. cmd .. ", error info:" .. err
+ end
+ local data = t:read("*all")
+ t:close()
+ return prefix_v3[data:sub(-4,-2)]
+end
+_M.etcd_version_from_cmd = etcd_version_from_cmd
Review comment:
> we should only support `v3` and `/v3`, and we can detect and set by Lua code instead of configure.
> _Originally posted by @moonming in https://github.com/apache/apisix/pull/2036_
I turned to do the job by call `etcdctl version` in command line in Lua code, since the original lua-resty-etcd version auto-detection seems not work and return error
```
.../deps/share/lua/5.1/resty/http.lua:133: API disabled in the context of init_worker_by_lua*
# stack traceback:
# [C]: in function 'ngx_socket_tcp'
# .../incubator-apisix/deps/share/lua/5.1/resty/http.lua:133: in function 'new'
# .../incubator-apisix/deps/share/lua/5.1/resty/etcd/v2.lua:157: in function 'version'
# .../incubator-apisix/deps/share/lua/5.1/resty/etcd.lua:22: in function 'etcd_version'
# .../incubator-apisix/deps/share/lua/5.1/resty/etcd.lua:46: in function 'new'
# ./apisix/core/config_etcd.lua:459: in function 'new'
```
But calling command line this much time might be too time-consuming. I still think it might be a good choice to save it in the configuration or maybe someplace else.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r489129014
##########
File path: bin/apisix
##########
@@ -879,35 +887,35 @@ local function init_etcd(show_output)
local host_count = #(yaml_conf.etcd.host)
- -- check whether the user has enabled etcd v2 protocol
+ local etcd_ok = false
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
+ -- check if etcd version above 3.4
+ cmd = "curl " .. host .. "/version 2>&1"
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))
+ local op_ver = str_split(res, "\"")[4]
+ local need_ver = "3.4.0"
+ if not check_version(op_ver, need_ver) then
+ io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
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
+ local uri = host .. "/v3/kv/put"
+ local post_json = '{"value":"' .. base64_encode("init_dir") .. '", "key":"' .. base64_encode(key) .. '"}'
+ cmd = "curl " .. uri .. " -X POST -d '" .. post_json
+ .. "' --connect-timeout " .. timeout
.. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
local res = excute_cmd(cmd)
- if not res:find("index", 1, true)
- and not res:find("createdIndex", 1, true) then
+ if (etcd_version == "v3" and not res:find("OK", 1, true)) then
Review comment:
Where is the definition of `etcd_version`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-678654583
> I think we can merge this PR at next week.
That would be great!
One more question, which place in doc do you suggest to put the etcd migration link? Like we could put it in the main README.md temporarily to remind users and maybe we need someplace else permanently.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-689952623
The newly imported lib [base64](https://github.com/iskolbin/lbase64) is used in `init_etcd` step to encode k/v send to etcd. Lib `base64` holds MIT License and only 200 lines. So I think we could directly use it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r469755712
##########
File path: apisix/core/config_etcd.lua
##########
@@ -48,14 +49,15 @@ local mt = {
return " etcd key: " .. self.key
end
}
+local etcd_version = _M.local_conf().etcd.version or "v2"
Review comment:
I thought config_etcd_v3.lua would reuse quite a lot code of config_etcd.lua, it might result in difficulty in maintenance.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2036:
URL: https://github.com/apache/apisix/pull/2036
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475051310
##########
File path: bin/apisix
##########
@@ -811,35 +811,52 @@ 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
+ -- check whether the user has enabled etcd v2 protocol for v2 API
+ local etcd_version = etcd_conf.version or "v2"
+ if etcd_version == "v2" then
+ 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
end
+ --local base64 = require("base64")
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 "")
+ local cmd
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
- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ if etcd_version == "v3" then
+ --uri = host .. "/v3/kv/put"
+ local key = (etcd_conf.prefix or "") .. dir_name .. "/"
+ --local post_json = '{"value":"' .. base64.encode("null") .. '", "key":"' .. base64.encode(key) .. '"}'
+ --cmd = "curl " .. uri
+ -- .. " -X POST -d '" .. post_json
+ -- .. "' --connect-timeout " .. timeout
+ -- .. " --max-time " .. timeout * 2 .. " --retry 1 2>&1"
+ cmd = "etcdctl --endpoints=" .. host .. " put " .. key .. " init_dir"
Review comment:
Thank you for your suggestions! I changed to use `luaposix` to set environmental variable here
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680966931
@membphis it seems I could not reply to your comment on "etcd v2 version".
The original implementation should directly use the version detection in lua-resty-etcd, but it is somehow not working, as I stated here: https://github.com/apache/apisix/pull/2036#discussion_r475052806
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488014261
##########
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:
In etcd v3, the first response of watch would be the success of watch creation, which is useless for us.
Will add this line to comment.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483905871
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Hi @membphis After removing `init_etcd`, users could not [this method](https://github.com/apache/apisix/blob/master/doc/getting-started.md#step-1-install-apisix):
`curl "http://127.0.0.1:9080/apisix/admin/services/" -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1'`
to check if they successfully installed APISIX. Do you have any suggestions on this
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r475212996
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
where we use this module `luaposix`? please send the code link which used this library.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r477334858
##########
File path: apisix/core/etcd.lua
##########
@@ -15,11 +15,38 @@
-- limitations under the License.
--
local fetch_local_conf = require("apisix.core.config_local").local_conf
-local etcd = require("resty.etcd")
-local clone_tab = require("table.clone")
+local etcd = require("resty.etcd")
+local clone_tab = require("table.clone")
+local io = io
+local type = type
+local ipairs = ipairs
+local string = string
+local tonumber = tonumber
local _M = {version = 0.1}
+local prefix_v3 = {
+ ["3.5"] = "/v3",
+ ["3.4"] = "/v3",
+ ["3.3"] = "/v3beta",
+ ["3.2"] = "/v3alpha",
+}
+
+
+-- TODO: Default lua-resty-etcd version auto-detection is broken, so directly get version from cmd
+-- we don't need to call this so many times, need to save it in some place
+local function etcd_version_from_cmd()
+ local cmd = "export ETCDCTL_API=3 && etcdctl version"
+ local t, err = io.popen(cmd)
+ if not t then
+ return nil, "failed to execute command: " .. cmd .. ", error info:" .. err
+ end
+ local data = t:read("*all")
+ t:close()
+ return prefix_v3[data:sub(-4,-2)]
+end
+_M.etcd_version_from_cmd = etcd_version_from_cmd
Review comment:
Thank you!
fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487706803
##########
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:
Is this the format conversion function between etcd v2 and v3?
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "base64 = 1.5-2"
Review comment:
we don't need this dependency, you can use ngx.encode_base64 and ngx.decode_base64
##########
File path: .travis/linux_tengine_runner.sh
##########
@@ -259,6 +259,12 @@ do_install() {
sudo luarocks install luacheck > build.log 2>&1 || (cat build.log && exit 1)
+ wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
+ tar xf etcd-v3.4.0-linux-amd64.tar.gz
+ sudo cp etcd-v3.4.0-linux-amd64/etcd /usr/local/bin/
+ sudo cp etcd-v3.4.0-linux-amd64/etcdctl /usr/local/bin/
+ rm -rf etcd-v3.4.0-linux-amd64
Review comment:
repeat many times, can we move them to one script?
##########
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:
please use ngx.encode_base64
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488320970
##########
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:
I think a `TEST` may run a total new APISIX instance, it will use a `readdir` but not `watchdir`. is that right ? @membphis
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487837001
##########
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:
no, it's int64 in ETCD.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r471378194
##########
File path: conf/config.yaml
##########
@@ -129,6 +129,8 @@ etcd:
- "http://127.0.0.1:2379" # multiple etcd address
prefix: "/apisix" # apisix configurations prefix
timeout: 30 # 30 seconds
+ version: "v3" # etcd version: v2/v3
+ api_prefix: "/v3alpha" # 3.2 -> "/v3alpha", 3.3 -> "/v3beta", 3.4+ -> "/v3"
Review comment:
we should only support `v3` and `/v3`, and we can detect and set by Lua code instead of configure.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r483905871
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "luaposix = 35.0-1",
Review comment:
Hi @membphis After removing `init_etcd`, users could not [this method](curl http://127.0.0.1:9080/apisix/admin/routes?api_key=edd1c9f034335f136f87ad84b625c8f1-i) to check if they successfully installed APISIX. Do you have any suggestions on this
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-680969971
@Yiyiyimu that is another problem. you can take a look at the doc of lua-nginx-module: https://github.com/openresty/lua-nginx-module#cosockets-not-available-everywhere
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r488484262
##########
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)
Review comment:
fix. thx!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#discussion_r487755488
##########
File path: .travis/linux_tengine_runner.sh
##########
@@ -259,6 +259,12 @@ do_install() {
sudo luarocks install luacheck > build.log 2>&1 || (cat build.log && exit 1)
+ wget https://github.com/etcd-io/etcd/releases/download/v3.4.0/etcd-v3.4.0-linux-amd64.tar.gz
+ tar xf etcd-v3.4.0-linux-amd64.tar.gz
+ sudo cp etcd-v3.4.0-linux-amd64/etcd /usr/local/bin/
+ sudo cp etcd-v3.4.0-linux-amd64/etcdctl /usr/local/bin/
+ rm -rf etcd-v3.4.0-linux-amd64
Review comment:
Duplication is acceptable here so that each use case script is independent.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-688697918
> I added this PR to 2.0 milestone, which due by September 18, 2020
Sure.
It just I need some help in the current problem, which discussed in https://github.com/apache/apisix/pull/2036#discussion_r483905871
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2036: feature: support etcd v3, by mocking v2 API
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2036:
URL: https://github.com/apache/apisix/pull/2036#issuecomment-693426643
> we could optimize it in another pr
@nic-chen I think we need to release a newer version of lua-resty-etcd, since the current version would output TONS of logs.
----------------------------------------------------------------
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:
users@infra.apache.org