feat: support reading configuration form xds(mvp) by tzssangglass · Pull Request #6614 · apache/apisix

tzssangglass · 2022-03-15T06:43:53Z

Description

Fixes # (issue)

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added in this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backwards compatible (If not, please discuss on the APISIX mailing list first)

membphis · 2022-03-15T06:57:23Z

+
+
+ffi.cdef[[
+extern void CreateMock(void* writeRoute);


change it to standard C style: create_mock

we should call a init() function here

init() is the name of a reserved function of go, with special meaning, I replaced it with initial()

Signed-off-by: tzssangglass <[email protected]>

spacewander · 2022-03-17T02:54:16Z

          - t/plugin
          - t/admin t/cli t/config-center-yaml t/control t/core t/debug t/discovery t/error_page t/misc
-          - t/node t/router t/script t/stream-node t/utils t/wasm
+          - t/node t/router t/script t/stream-node t/utils t/wasm t/amesh-library


Should be sorted in order

spacewander · 2022-03-17T02:56:17Z

    end
    sys_conf["wasm"] = yaml_conf.wasm

+    sys_conf["config_center"] = yaml_conf.apisix.config_center


Handled in

apisix/apisix/cli/ops.lua

Line 555 in 15517fe

for k,v in pairs(yaml_conf.apisix) do

spacewander · 2022-03-17T02:56:33Z

            properties = {
                config_center = {
-                    enum = {"etcd", "yaml"},
+                    enum = {"etcd", "yaml", "shdict"},


Suggested change

enum = {"etcd", "yaml", "shdict"},

enum = {"etcd", "yaml", "xds"},

would be better?

spacewander · 2022-03-17T02:58:05Z

+
+
+local function load_libamesh(lib_name)
+    ngx_timer_at(0, function(premature)


Why use the timer?

spacewander · 2022-03-17T02:59:30Z

+end
+
+
+function _M.new(key, opts)


We can use injection for test?

apisix/t/plugin/skywalking.t

Lines 32 to 50 in 15517fe

my $extra_init_by_lua = <<_EOC_;

-- reduce default report interval

local client = require("skywalking.client")

client.backendTimerDelay = 0.5

local sw_tracer = require("skywalking.tracer")

local inject = function(mod, name)

local old_f = mod[name]

mod[name] = function (...)

ngx.log(ngx.WARN, "skywalking run ", name)

return old_f(...)

end

end

inject(sw_tracer, "start")

inject(sw_tracer, "finish")

inject(sw_tracer, "prepareForReport")

_EOC_

tokers · 2022-03-17T09:21:23Z

          - t/plugin
          - t/admin t/cli t/config-center-yaml t/control t/core t/debug t/discovery t/error_page t/misc
-          - t/node t/router t/script t/stream-node t/utils t/wasm
+          - t/amesh-library t/node t/router t/script t/stream-node t/utils t/wasm


What is amesh?

fixed, use xds

tokers · 2022-03-17T09:22:02Z

          sudo dpkg -i tinygo_${TINYGO_VER}_amd64.deb
          cd t/wasm && find . -type f -name "*.go" | xargs -Ip tinygo build -o p.wasm -scheduler=none -target=wasi p

+      - name: Build Amesh library


tokers · 2022-03-17T09:23:06Z

+    local string_gmatch = string.gmatch
+    local string_match = string.match
+    local io_open = io.open
+    local io_close = io.close


Put them to the module level?

tokers · 2022-03-17T09:23:24Z

+    local tried_paths = new_tab(32, 0)
+    local i = 1
+
+    for k, _ in string_gmatch(cpath, "[^;]+") do


Why not use ngx.re.gmatch?

tokers · 2022-03-17T09:23:34Z

+    local i = 1
+
+    for k, _ in string_gmatch(cpath, "[^;]+") do
+        local fpath = string_match(k, "(.*/)")


What about ngx.re.match?

tokers · 2022-03-17T09:24:24Z

    require("apisix.balancer").init_worker()
    load_balancer = require("apisix.balancer")
+
+    if core.config == require("apisix.core.config_shdict") then


I think we may need to unify the term, the shdict is the container, the xds is the way, I think here config_xds will be better.

another option, we can use amesh

@membphis I don't think so, from the point of view of Apache APISIX, what is amesh? Why do we need to care about a commercial product?

yes, you are right. config_xds is fine

Signed-off-by: tzssangglass <[email protected]>

shuaijinchao · 2022-03-17T16:09:44Z

+
+
+function _M.init_worker()
+    local lib_name = "libxds.so"


Put it at the module level?

tao12345666333

In my opinion, I think all xds should be replaced with external or other names of similar meaning.

Although we are currently integrating Istio/xds, APISIX does not actually need to pay attention to whether the external Go application is docking with xDS or other protocols. If we name it external library, then users can use this mode to interface with any service, and even we can use it to watch etcd directly, it will be a very big change, and it will be more clear.

spacewander · 2022-03-18T01:57:08Z

IMHO, external is too general to know what it does. As etcd is an external service, what is the difference between etcd and external?
Moreover, we don't promise any API spec about the xds exchange format and we might optimize it for xds in the future. So I think using the name xds is specific enough for this feature.

spacewander · 2022-03-18T02:32:07Z

+__DATA__
+
+=== TEST 1: load Amesh library so successfully
+--- yaml_config


Let's set the yaml_config in the file level?

spacewander · 2022-03-18T02:35:50Z

    require("apisix.balancer").init_worker()
    load_balancer = require("apisix.balancer")
+
+    if core.config == require("apisix.core.config_xds") then


We can call it like:

apisix/apisix/init.lua

Line 83 in 9d450d7

if core.config.init then

tao12345666333 · 2022-03-18T04:50:59Z

So I think using the name xds is specific enough for this feature.

OK so we can keep this naming for now and let's move forward.

spacewander · 2022-03-20T12:45:58Z

    {% end %}

+    {% if config_center == "xds" then %}
+    lua_shared_dict router-config  10m;


Suggested change

lua_shared_dict router-config 10m;

lua_shared_dict xds-route-config 10m;

would that be better?

spacewander · 2022-03-20T12:46:42Z

+local new_tab           = base.new_tab
+local ffi               = require ("ffi")
+local C                 = ffi.C
+local router_config     = ngx.shared["router-config"]


It is not configuration for router, but for route.

spacewander · 2022-03-20T12:47:57Z

+
+    if not xdsagent then
+        tried_paths[#tried_paths + 1] = 'tried above paths but can not load ' .. lib_name
+        error("can not load Amesh library, tried paths: " ..


Suggested change

error("can not load Amesh library, tried paths: " ..

error("can not load xds library, tried paths: " ..

# Conflicts: # t/APISIX.pm

spacewander · 2022-03-21T06:05:53Z

+local new_tab           = base.new_tab
+local ffi               = require ("ffi")
+local C                 = ffi.C
+local router_config     = ngx.shared["xds-route-config"]


There are still using router in some places.

membphis

LGTM

# Conflicts: # t/APISIX.pm

leslie-tsang · 2022-03-22T03:36:22Z

+local new_tab           = base.new_tab
+local ffi               = require ("ffi")
+local C                 = ffi.C
+local route_config     = ngx.shared["xds-route-config"]


Suggested change

local route_config = ngx.shared["xds-route-config"]

local route_config = ngx.shared["xds-route-config"]

I want to change it in the next PR😅, otherwise it will dismiss the previous approval.

leslie-tsang · 2022-03-22T03:39:50Z

    lua_shared_dict ext-plugin 1m;
    lua_shared_dict kubernetes 1m;
    lua_shared_dict tars 1m;
+    lua_shared_dict xds-route-config 1m;


Should we sync the setting in apisix/cli/ngx_tpl.lua ?

Change 1m to 10m

It is enough in the test.

tzssangglass added 6 commits March 15, 2022 14:42

feat: support reading configuration form shdict(mvp)

079eefb

ci

808bd6e

fix code lint

5ffd2bf

fix code lint

815074d

fix CI

1ca0254

Merge branch 'master' into cgtf

dd1315e

membphis reviewed Mar 15, 2022

View reviewed changes

tzssangglass added 8 commits March 15, 2022 21:18

fix code review

6fa5a6b

replace amesh mock method

3123d81

revert amesh initial function

ede18da

fix test cases lib name

5b8726f

fix CI error

d4c8c19

fix CI error

ed3fa36

fix CI error

87e97c1

fix CI error

519a823

membphis reviewed Mar 16, 2022

View reviewed changes

Comment thread apisix/core/config_shdict.lua Outdated

Comment thread apisix/core/config_shdict.lua Outdated

tzssangglass added 3 commits March 17, 2022 00:10

revert test cases changed in traffic-split

3345a31

Signed-off-by: tzssangglass <[email protected]>

Merge branch 'master' of https://github.com/apache/apisix into cgtf

a80954b

fix code review

9a4cb1f

tzssangglass marked this pull request as ready for review March 17, 2022 02:27

tzssangglass requested review from spacewander, tao12345666333 and tokers March 17, 2022 02:28

spacewander reviewed Mar 17, 2022

View reviewed changes

tzssangglass added 2 commits March 17, 2022 14:25

fix code review

62190bc

revert config.yaml

82fbcdc

tokers reviewed Mar 17, 2022

View reviewed changes

tzssangglass added 3 commits March 17, 2022 17:33

resolve code review

b0fa534

Signed-off-by: tzssangglass <[email protected]>

rename

39cca64

Signed-off-by: tzssangglass <[email protected]>

rename

add82f7

Signed-off-by: tzssangglass <[email protected]>

shuaijinchao reviewed Mar 17, 2022

View reviewed changes

resolve code review

8a2a67d

tzssangglass dismissed membphis’s stale review via 8a2a67d March 17, 2022 16:27

tao12345666333 reviewed Mar 18, 2022

View reviewed changes

spacewander reviewed Mar 18, 2022

View reviewed changes

resolve code review

f1fee3b

tzssangglass added 2 commits March 18, 2022 15:51

fix CI error

a9c9ba0

rename test cases names

9b31b34

spacewander reviewed Mar 20, 2022

View reviewed changes

tzssangglass added 2 commits March 21, 2022 09:36

Merge branch 'master' into cgtf

0166d0a

# Conflicts: # t/APISIX.pm

resolve code review

513c2b3

spacewander reviewed Mar 21, 2022

View reviewed changes

membphis reviewed Mar 21, 2022

View reviewed changes

resolve code review

57c88d1

tzssangglass requested review from membphis, shuaijinchao, spacewander, tao12345666333 and tokers March 21, 2022 10:29

tzssangglass added 2 commits March 21, 2022 19:51

Merge branch 'master' into cgtf

41abb0a

Merge branch 'master' into cgtf

aee463f

# Conflicts: # t/APISIX.pm

spacewander approved these changes Mar 22, 2022

View reviewed changes

leslie-tsang reviewed Mar 22, 2022

View reviewed changes

tzssangglass requested a review from leslie-tsang March 22, 2022 07:18

leslie-tsang approved these changes Mar 22, 2022

View reviewed changes

spacewander merged commit d2e3380 into apache:master Mar 22, 2022

tzssangglass deleted the cgtf branch March 25, 2022 02:04

spacewander pushed a commit that referenced this pull request Jun 30, 2022

feat: support reading configuration form xds(mvp) (#6614)

555140a

	enum = {"etcd", "yaml", "shdict"},
	enum = {"etcd", "yaml", "xds"},



		local function load_libamesh(lib_name)
		ngx_timer_at(0, function(premature)

	my $extra_init_by_lua = <<_EOC_;
	-- reduce default report interval
	local client = require("skywalking.client")
	client.backendTimerDelay = 0.5

	local sw_tracer = require("skywalking.tracer")
	local inject = function(mod, name)
	local old_f = mod[name]
	mod[name] = function (...)
	ngx.log(ngx.WARN, "skywalking run ", name)
	return old_f(...)
	end
	end

	inject(sw_tracer, "start")
	inject(sw_tracer, "finish")
	inject(sw_tracer, "prepareForReport")
	_EOC_

	lua_shared_dict router-config 10m;
	lua_shared_dict xds-route-config 10m;

	error("can not load Amesh library, tried paths: " ..
	error("can not load xds library, tried paths: " ..

	local route_config = ngx.shared["xds-route-config"]
	local route_config = ngx.shared["xds-route-config"]

Uh oh!

Conversation

tzssangglass commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tao12345666333 left a comment

Choose a reason for hiding this comment

Uh oh!

spacewander commented Mar 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzssangglass commented Mar 15, 2022 •

edited

Loading

spacewander Mar 20, 2022 •

edited

Loading