[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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:
us...@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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:
us...@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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: us...@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2036: feature: support etcd v3, by mocking v2 API
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:
us...@infra.apache.org
