feat: support multiple kubernets clusters discovery #7895

zhixiongdu027 · 2022-09-11T10:36:27Z

Description

Fixes # (issue)
#7569 -> -- Support mulitple kubernets cluster
#7199 , #7185 -> -- Remove possible extra spaces in client.token

Checklist

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

Todo

Doc ...

zhixiongdu027 · 2022-09-11T11:47:37Z

@spacewander @tokers @tzssangglass
Hello:

I think the code part is already ready for review,

The documentation part will be submitted later

Please help for review ,Many TKS.

spacewander · 2022-09-12T12:27:16Z

t/kubernetes/discovery/kubernetes2.t

+apisix:
+  node_listen: 1984
+  config_center: yaml
+  enable_admin: false


We don't need to disable admin when config_center is yaml. Many existing yaml relative tests have this configuration, but we can remove it from the new test.

I found an one-line script to remove them:
grep -lP 'config_center: yaml\n\s+enable_admin: false' t/*/*.t |xargs -r sed -i -r -z 's/\n\s+enable_admin: false\n/\n/g'

seems like remove "enable_admin: false" will cause all test case fail

spacewander · 2022-09-12T12:37:30Z

apisix/discovery/kubernetes/schema.lua

-                not_match = {
-                    type = "array",
-                    items = {
+                    service = {


We can save some repeated fields with something like service = xxx.service?

Maybe not a good idea , because service had default value in single mode but no default value in multiple mode .

spacewander · 2022-09-12T12:43:59Z

apisix/discovery/kubernetes/init.lua

@@ -29,10 +29,9 @@ local core = require("apisix.core")
 local util = require("apisix.cli.util")
 local local_conf = require("apisix.core.config_local").local_conf()
 local informer_factory = require("apisix.discovery.kubernetes.informer_factory")
+local setmetatable = setmetatable


Better to move the setmetatable together with pcall and others.

spacewander · 2022-09-12T12:49:06Z

apisix/discovery/kubernetes/init.lua

+    local pattern = "^(.*)/(.*/.*):(.*)$" -- id/namespace/name:port_name
+    local match = ngx.re.match(service_name, pattern, "jo")
+    if not match then
+        core.log.info("get unexpected upstream service_name:　", service_name)


We should use error level log?

spacewander · 2022-09-12T12:49:19Z

apisix/discovery/kubernetes/init.lua

+    local id = match[1]
+    local endpoint_dict = ctx[id]
+    if not endpoint_dict then
+        core.log.info("id not exis")


Suggested change

core.log.info("id not exis")

core.log.info("id not exist")

spacewander · 2022-09-12T13:01:35Z

apisix/discovery/kubernetes/init.lua

+
+        local endpoint_dict = ngx.shared["kubernetes-" .. id]
+        if not endpoint_dict then
+            error(core.table.concat {


I would recommend using string.format which is more common

See apache#7895 (comment) Signed-off-by: spacewander <[email protected]>

spacewander · 2022-09-13T06:17:38Z

apisix/discovery/kubernetes/schema.lua

-            properties = {
-                token = {
-                    type = "string",
+                namespace_selector = {


What about reusing the namespace_selector schema?

ok, I'll try to optimize the schema fully

tzssangglass · 2022-09-13T08:47:00Z

apisix/discovery/kubernetes/init.lua

+
+
+function _M.init_worker()
+    local discovery_conf = local_conf.discovery.kubernetes


we should update config-default.yaml, added id attribute

sorry i didn't understand what you mean @tzssangglass

here:

apisix/conf/config-default.yaml

Lines 307 to 336 in 2a44ba6

# kubernetes:

# service:

# schema: https #apiserver schema, options [http, https], default https

# host: ${KUBERNETES_SERVICE_HOST} #apiserver host, options [ipv4, ipv6, domain, environment variable], default ${KUBERNETES_SERVICE_HOST}

# port: ${KUBERNETES_SERVICE_PORT} #apiserver port, options [port number, environment variable], default ${KUBERNETES_SERVICE_PORT}

# client:

# # serviceaccount token or path of serviceaccount token_file

# token_file: ${KUBERNETES_CLIENT_TOKEN_FILE}

# # token: |-

# # eyJhbGciOiJSUzI1NiIsImtpZCI6Ikx5ME1DNWdnbmhQNkZCNlZYMXBsT3pYU3BBS2swYzBPSkN3ZnBESGpkUEEif

# # 6Ikx5ME1DNWdnbmhQNkZCNlZYMXBsT3pYU3BBS2swYzBPSkN3ZnBESGpkUEEifeyJhbGciOiJSUzI1NiIsImtpZCI

# # kubernetes discovery plugin support use namespace_selector

# # you can use one of [equal, not_equal, match, not_match] filter namespace

# namespace_selector:

# # only save endpoints with namespace equal default

# equal: default

# # only save endpoints with namespace not equal default

# #not_equal: default

# # only save endpoints with namespace match one of [default, ^my-[a-z]+$]

# #match:

# #- default

# #- ^my-[a-z]+$

# # only save endpoints with namespace not match one of [default, ^my-[a-z]+$ ]

# #not_match:

# #- default

# #- ^my-[a-z]+$

# # kubernetes discovery plugin support use label_selector

# # for the expression of label_selector, please refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/labels

# label_selector: |-

# first="a",second="b"

only single_mode_nodes configuration here

Ok ,get your point .
This part will be submitted together with the documentation

tzssangglass · 2022-09-13T09:17:25Z

A discussion:

In the current implementation, single cluster and multiple clusters are implemented separately in the code.

Can we support only multiple clusters, the original single cluster just has a cluster of multiple clusters.

zhixiongdu027 · 2022-09-13T09:36:57Z

"Can we support only multiple clusters"

Makes me think it has two meanings.

1: Abandon compatibility with the current single cluster configuration, users need to manually modify the previous configuration after the upgrade, otherwise it cannot be used

2: Compatible with the current single cluster configuration, but modify the code to treat the single cluster as a multi-cluster configuration with only one array element

I think you may mean 2,

However, there is no full benefit, but it brings performance loss.
Because the performance when dealing with multi-cluster configurations is lower than that of single-cluster configurations

tzssangglass · 2022-09-13T09:58:23Z

apisix/discovery/kubernetes/init.lua

+            core.log.error("list_watch failed, kind: ", handle.kind,
+                    ", reason: ", "RuntimeException", ", message : ", status)


"RuntimeException" does not look like APISIX style logs

But i think is kubernetes style

People view APISIX logs from here, and we don't use their style of logs for every new ecology we dock.

I think it's best practice to keep things as they are

In the List-Watch process, there are many error messages that may appear in many Kubernetes responses, all of them are in this format (Reason and Message)

It's not necessary to change the raw error message content for "apisix style"

tokers · 2022-09-13T10:04:31Z

apisix/cli/ops.lua

    if enabled_discoveries["kubernetes"] then

-        local kubernetes_conf = yaml_conf.discovery["kubernetes"]
+        if not sys_conf["discovery_shared_dict"] then


Use plural: discovery_shared_dicts.

tzssangglass · 2022-09-13T10:26:48Z

"Can we support only multiple clusters"

Makes me think it has two meanings.

1: Abandon compatibility with the current single cluster configuration, users need to manually modify the previous configuration after the upgrade, otherwise it cannot be used

2: Compatible with the current single cluster configuration, but modify the code to treat the single cluster as a multi-cluster configuration with only one array element

I think you may mean 2,

However, there is no full benefit, but it brings performance loss. Because the performance when dealing with multi-cluster configurations is lower than that of single-cluster configurations

I mean 1.

but it brings performance loss.
Because the performance when dealing with multi-cluster configurations is lower than that of single-cluster configurations

It's persuasive. Can you talk more about performance loss.

zhixiongdu027 · 2022-09-14T00:47:14Z

I mean 1.

Maybe this needs more people to join the discussion

It's persuasive. Can you talk more about performance loss.

Additional id detection and table field access are required in the multiple_mode_nodes function, obviously there is a certain performance loss

spacewander · 2022-09-16T06:57:15Z

@zhixiongdu027
We are going to release a new version next week. Would you like to add a doc before that?

zhixiongdu027 · 2022-09-16T07:20:33Z

I will asap, it may take three or four days

wolgod · 2022-09-21T02:49:37Z

i see lua_shared_dict kubernetes default is 1m , If there are many services in multiple cluster, it may not be enough, but it cannot be set very large. How to evaluate the memory usage of a service?

zhixiongdu027 · 2022-09-21T03:42:19Z

i see lua_shared_dict kubernetes default is 1m , If there are many services in multiple cluster, it may not be enough, but it cannot be set very large. How to evaluate the memory usage of a service?

@wolgod

Please don't worry.
Usually, 1m of memory can store about 1000 endpoints information

And in multi-cluster mode, each cluster uses its own ngx.shared.DICT.
For example, if you define a release cluster and a debug cluster,
Then there will be two ngx.shared.DICT:
[ kubernetes-release , kubernetes-debug ] ,
and you can specify the memory size separately

In addition, you can use namespace_selector and label_selector to filter endpoint information to reduce memory

wolgod · 2022-09-21T08:56:13Z

thanks, I wonder if the community will support apisix-seed service discovery for kubernetes discovery in the future?

zhixiongdu027 · 2022-09-21T09:31:50Z

@wolgod
This is not a place for discussion, you should create a ISSUE

wolgod · 2022-09-23T12:07:55Z

thanks, Will there be a release for 2.15.x support multiple kubernets clusters discovery ？ because apisix 2.x can't switch to version 3 seamlessly

tzssangglass · 2022-09-23T15:28:06Z

thanks, Will there be a release for 2.15.x support multiple kubernets clusters discovery ？ because apisix 2.x can't switch to version 3 seamlessly

No, functional PRs are not back-ported.

zhixiongdu027 added 7 commits September 11, 2022 17:20

feat: support multi kubernetes cluster

2bf4a7a

feat: support multi kubernetes cluster

46f79f4

fix lint

6a716b0

fix lint

d342bb8

fix lint

3b0e8b1

fix cli test

59479bc

fix test case

bca7855

zhixiongdu027 marked this pull request as draft September 11, 2022 11:44

zhixiongdu027 marked this pull request as ready for review September 11, 2022 11:44

spacewander reviewed Sep 12, 2022

View reviewed changes

spacewander added a commit to spacewander/incubator-apisix that referenced this pull request Sep 12, 2022

test: remove unnecessary enable_admin: false

8a3c4c8

See apache#7895 (comment) Signed-off-by: spacewander <[email protected]>

spacewander mentioned this pull request Sep 12, 2022

test: remove unnecessary enable_admin: false #7896

Merged

5 tasks

spacewander added a commit to spacewander/incubator-apisix that referenced this pull request Sep 12, 2022

test: remove unnecessary enable_admin: false

f531268

See apache#7895 (comment) Signed-off-by: spacewander <[email protected]>

zhixiongdu027 added 3 commits September 13, 2022 09:38

accept the review suggestion

2471ff9

add "enable_admin: false" in t file

8934131

fix lint

eabca1c

zhixiongdu027 requested a review from spacewander September 13, 2022 02:10

spacewander reviewed Sep 13, 2022

View reviewed changes

optimize scheme

a2578f9

zhixiongdu027 requested a review from spacewander September 13, 2022 07:34

optimize scheme

ae4075d

tzssangglass reviewed Sep 13, 2022

View reviewed changes

tokers reviewed Sep 13, 2022

View reviewed changes

accept review suggestion

de0f87a

zhixiongdu027 requested review from tzssangglass, tokers and spacewander and removed request for spacewander, tzssangglass and tokers September 14, 2022 01:01

spacewander approved these changes Sep 14, 2022

View reviewed changes

tzssangglass approved these changes Sep 14, 2022

View reviewed changes

spacewander merged commit 5a630b8 into apache:master Sep 15, 2022

zhixiongdu027 mentioned this pull request Sep 19, 2022

docs: refactor kubernetes discovery #7944

Merged

5 tasks

tzssangglass mentioned this pull request Sep 20, 2022

help request: if i wan't support multi cluster of k8s'discovery,what can i do? #7951

Closed

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022

feat: support multiple kubernets clusters discovery (apache#7895)

3ba234d

moonming mentioned this pull request Jul 26, 2023

Apisix with multiple K8s clusters as a source for service discovery #7569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support multiple kubernets clusters discovery #7895

feat: support multiple kubernets clusters discovery #7895

zhixiongdu027 commented Sep 11, 2022 •

edited

Loading

zhixiongdu027 commented Sep 11, 2022

spacewander Sep 12, 2022

spacewander Sep 12, 2022

zhixiongdu027 Sep 13, 2022

spacewander Sep 12, 2022

zhixiongdu027 Sep 13, 2022

spacewander Sep 12, 2022

spacewander Sep 12, 2022

spacewander Sep 12, 2022

spacewander Sep 12, 2022

spacewander Sep 13, 2022

zhixiongdu027 Sep 13, 2022

tzssangglass Sep 13, 2022

zhixiongdu027 Sep 13, 2022 •

edited

Loading

tzssangglass Sep 13, 2022

zhixiongdu027 Sep 13, 2022 •

edited

Loading

tzssangglass commented Sep 13, 2022

zhixiongdu027 commented Sep 13, 2022

tzssangglass Sep 13, 2022

zhixiongdu027 Sep 13, 2022

tzssangglass Sep 13, 2022

zhixiongdu027 Sep 14, 2022

tokers Sep 13, 2022

tzssangglass commented Sep 13, 2022

zhixiongdu027 commented Sep 14, 2022

spacewander commented Sep 16, 2022

zhixiongdu027 commented Sep 16, 2022 •

edited

Loading

wolgod commented Sep 21, 2022

zhixiongdu027 commented Sep 21, 2022

wolgod commented Sep 21, 2022

zhixiongdu027 commented Sep 21, 2022

wolgod commented Sep 23, 2022

tzssangglass commented Sep 23, 2022



		function _M.init_worker()
		local discovery_conf = local_conf.discovery.kubernetes

	# kubernetes:
	# service:
	# schema: https #apiserver schema, options [http, https], default https
	# host: ${KUBERNETES_SERVICE_HOST} #apiserver host, options [ipv4, ipv6, domain, environment variable], default ${KUBERNETES_SERVICE_HOST}
	# port: ${KUBERNETES_SERVICE_PORT} #apiserver port, options [port number, environment variable], default ${KUBERNETES_SERVICE_PORT}
	# client:
	# # serviceaccount token or path of serviceaccount token_file
	# token_file: ${KUBERNETES_CLIENT_TOKEN_FILE}
	# # token: \|-
	# # eyJhbGciOiJSUzI1NiIsImtpZCI6Ikx5ME1DNWdnbmhQNkZCNlZYMXBsT3pYU3BBS2swYzBPSkN3ZnBESGpkUEEif
	# # 6Ikx5ME1DNWdnbmhQNkZCNlZYMXBsT3pYU3BBS2swYzBPSkN3ZnBESGpkUEEifeyJhbGciOiJSUzI1NiIsImtpZCI
	# # kubernetes discovery plugin support use namespace_selector
	# # you can use one of [equal, not_equal, match, not_match] filter namespace
	# namespace_selector:
	# # only save endpoints with namespace equal default
	# equal: default
	# # only save endpoints with namespace not equal default
	# #not_equal: default
	# # only save endpoints with namespace match one of [default, ^my-[a-z]+$]
	# #match:
	# #- default
	# #- ^my-[a-z]+$
	# # only save endpoints with namespace not match one of [default, ^my-[a-z]+$ ]
	# #not_match:
	# #- default
	# #- ^my-[a-z]+$
	# # kubernetes discovery plugin support use label_selector
	# # for the expression of label_selector, please refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/labels
	# label_selector: \|-
	# first="a",second="b"

		core.log.error("list_watch failed, kind: ", handle.kind,
		", reason: ", "RuntimeException", ", message : ", status)

feat: support multiple kubernets clusters discovery #7895

feat: support multiple kubernets clusters discovery #7895

Conversation

zhixiongdu027 commented Sep 11, 2022 • edited Loading

Description

Checklist

Todo

zhixiongdu027 commented Sep 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

tzssangglass commented Sep 13, 2022

zhixiongdu027 commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzssangglass commented Sep 13, 2022

zhixiongdu027 commented Sep 14, 2022

spacewander commented Sep 16, 2022

zhixiongdu027 commented Sep 16, 2022 • edited Loading

wolgod commented Sep 21, 2022

zhixiongdu027 commented Sep 21, 2022

wolgod commented Sep 21, 2022

zhixiongdu027 commented Sep 21, 2022

wolgod commented Sep 23, 2022

tzssangglass commented Sep 23, 2022

zhixiongdu027 commented Sep 11, 2022 •

edited

Loading

zhixiongdu027 Sep 13, 2022 •

edited

Loading

zhixiongdu027 Sep 13, 2022 •

edited

Loading

zhixiongdu027 commented Sep 16, 2022 •

edited

Loading