feat: Add labels for upstream object #2279

imjoey · 2020-09-22T12:49:29Z

Signed-off-by: imjoey majunjiev@gmail.com

What this PR does / why we need it:

Hi fellows, we could use labels (as it in Kubernetes) to specify attributes that attached an object. This is a kind of extensibility mechanism for developers. Once I had a short discussion with @moonming at apache/apisix-dashboard#431 (comment) , so this PR is going to take upstream as the starting point and add a new labels property, via a rather simple implementation.

Looking forward to the feedback. Thanks.

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible?

labels can be used to specify additional attributes. Signed-off-by: imjoey <majunjiev@gmail.com>

membphis

need a test case, I think it should be invalid.

{
    ... ...
    lables: {
        key: {
             sub_key: ["a", "b", "c"]
        }
    }
}

apisix/schema_def.lua

membphis · 2020-09-23T02:41:27Z

apisix/schema_def.lua

@@ -332,6 +332,10 @@ local upstream_schema = {
            description = "enable websocket for request",
            type        = "boolean"
        },
+        labels = {
+            description = "key/value pairs to specify attributes",
+            type = "table"


I think it should be object here.

Need to limit the depth to 1

@membphis agreed, nice catch, thanks. I would make changes as the your suggestions and add one more failure test case after I find the lua way. 😄

@membphis PR is updated and labels are limited to string key/value pairs, so fix the depth problem. 😄

tokers · 2020-09-23T03:16:31Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

gxthrj · 2020-09-23T07:44:18Z

This is a very interesting topic.
I hope to add labels to all objects in apisix, so that some strong bindings through xxx_id can be changed to loose binding relationships through labels. this is very useful and better fit with etcd design.

imjoey · 2020-09-23T07:48:22Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

Hi @tokers , thanks for your feedback. I guess labels could be very useful for every single object in APISIX. While currently Node is not an independent object so we are not able to mange the labels attached. We may refer to the existing field named metadata in Node for your requirements.

gxthrj · 2020-09-23T07:52:58Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

Your idea is great,
But we also need to pay attention to the structure of APISIX,

The nodes under upstream is not an object yet;
Blue-green or canary releases are more tied to business, not suitable for writing in upstream objects, but more suitable for extension in plug-ins, But upstream currently does not provide any plug-in implementation;
if we add labels in nodes, I think it will be a big change.

imjoey · 2020-09-23T08:13:57Z

This is a very interesting topic.
I hope to add labels to all objects in apisix, so that some strong bindings through xxx_id can be changed to loose binding relationships through labels. this is very useful and better fit with etcd design.

@gxthrj yep, glad to hear your thoughts. Yesterday I was about to migrate the functionality of RouteGroup from MySQL to etcd. But it seemed still under discussions in community. So I deliberated on the whole design, I guess labels could make the relationships of objects in APISIX much more loosely coupled, and also make the extensibility more easily.

nic-chen · 2020-09-23T08:19:24Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

Your idea is great,
But we also need to pay attention to the structure of APISIX,

The nodes under upstream is not an object yet;

Blue-green or canary releases are more tied to business, not suitable for writing in upstream objects, but more suitable for extension in plug-ins, But upstream currently does not provide any plug-in implementation;
if we add labels in nodes, I think it will be a big change.

I think we don't need to separate nodes from upstream, just redefine the data structure of nodes.
Of course, there is also a lot of work in this way, but it is better to understand.

imjoey · 2020-09-23T08:22:34Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

Your idea is great,
But we also need to pay attention to the structure of APISIX,

The nodes under upstream is not an object yet;

Blue-green or canary releases are more tied to business, not suitable for writing in upstream objects, but more suitable for extension in plug-ins, But upstream currently does not provide any plug-in implementation;
if we add labels in nodes, I think it will be a big change.

I think we don't need to separate nodes from upstream, just redefine the data structure of nodes.
Of course, there is also a lot of work in this way, but it is better to understand.

@nic-chen Just FYI, we could make use of the existing metadata field in node, pls refer to https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L263 for details.

tokers · 2020-09-23T09:18:45Z

@imjoey One thing confuses me that the labels field is attached on the whole upstream object, It's closer to the labels on Kubernetes Service, not the Pod, and labels on Pod are more pragmatic, so what about attaching labels for each node in this upstream, with this support, we can group the nodes by labels, and in turn implementing some features like blue green release, canary release or other route rules.

Your idea is great,
But we also need to pay attention to the structure of APISIX,

The nodes under upstream is not an object yet;

Blue-green or canary releases are more tied to business, not suitable for writing in upstream objects, but more suitable for extension in plug-ins, But upstream currently does not provide any plug-in implementation;
if we add labels in nodes, I think it will be a big change.

I think whether the Node is embedded in Upstream or as a separate object doesn't matter, even in XDS protocol, the CDS does not necessarily depend on EDS.
What we can do is not implementing these route rules in upstream logic, instead, a traffic split plugin can be introduced to solve these problems, I will submit an issue this few days, with some details.

tokers · 2020-09-24T09:59:00Z

@imjoey @gxthrj @nic-chen @membphis @moonming I have submitted an issue about traffic split #2303.

Signed-off-by: imjoey <majunjiev@gmail.com>

imjoey · 2020-09-27T06:52:56Z

@membphis @nic-chen @gxthrj PR updated and any feedback would be much appreciated.

membphis · 2020-09-27T13:27:57Z

doc/zh-cn/architecture-design.md

@@ -273,6 +273,7 @@ APISIX 的 Upstream 除了基本的复杂均衡算法选择外，还支持对上
 |checks          |可选|配置健康检查的参数，详细可参考[health-check](../health-check.md)|
 |retries         |可选|使用底层的 Nginx 重试机制将请求传递给下一个上游，默认 APISIX 会启用重试机制，根据配置的后端节点个数设置重试次数，如果此参数显式被设置将会覆盖系统默认设置的重试次数。|
 |enable_websocket|可选| 是否启用 `websocket`（布尔值），默认不启用|
+|labels          |可选| 用于标识属性的键值对。 |


need more doc, we need an example about how to use this field.

@membphis thanks for reviewing. I will add more guidance docs about labels usage and best practice in APISIX after labels are added in all objects.

membphis

Some minor document issues need to be fixed, LGTM

membphis · 2020-09-28T03:20:40Z

@imjoey many thx, merged

Add labels for upstream object

508cd8c

labels can be used to specify additional attributes. Signed-off-by: imjoey <majunjiev@gmail.com>

imjoey force-pushed the feat/add_labels_for_upstream branch from c8550b7 to 508cd8c Compare September 22, 2020 12:50

membphis reviewed Sep 23, 2020

View reviewed changes

tokers mentioned this pull request Sep 24, 2020

Proposal: inptroduce traffic split plugin #2303

Closed

Limit the formats of value labels to string only

07ae2c7

Signed-off-by: imjoey <majunjiev@gmail.com>

imjoey force-pushed the feat/add_labels_for_upstream branch from 980e048 to 07ae2c7 Compare September 25, 2020 08:58

membphis reviewed Sep 27, 2020

View reviewed changes

membphis approved these changes Sep 27, 2020

View reviewed changes

moonming approved these changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into feat/add_labels_for_upstream

469ac17

nic-chen approved these changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into feat/add_labels_for_upstream

cbe5bd6

membphis merged commit ebe4f66 into apache:master Sep 28, 2020

moonming added this to the 2.0 milestone Sep 28, 2020

imjoey deleted the feat/add_labels_for_upstream branch September 28, 2020 05:54

imjoey mentioned this pull request Sep 29, 2020

feat: Add lables for route/consumer/service/ssl objects #2345

Merged

4 tasks

liuxiran mentioned this pull request Nov 26, 2020

feat: Route group feature apache/apisix-dashboard#861

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add labels for upstream object #2279

feat: Add labels for upstream object #2279

imjoey commented Sep 22, 2020

membphis left a comment

membphis Sep 23, 2020

membphis Sep 23, 2020

imjoey Sep 23, 2020 •

edited

Loading

imjoey Sep 25, 2020

tokers commented Sep 23, 2020

gxthrj commented Sep 23, 2020

imjoey commented Sep 23, 2020 •

edited

Loading

gxthrj commented Sep 23, 2020

imjoey commented Sep 23, 2020

nic-chen commented Sep 23, 2020

imjoey commented Sep 23, 2020

tokers commented Sep 23, 2020

tokers commented Sep 24, 2020

imjoey commented Sep 27, 2020

membphis Sep 27, 2020

imjoey Sep 28, 2020

membphis left a comment

membphis commented Sep 28, 2020

feat: Add labels for upstream object #2279

feat: Add labels for upstream object #2279

Conversation

imjoey commented Sep 22, 2020

What this PR does / why we need it:

Pre-submission checklist:

membphis left a comment

Choose a reason for hiding this comment

membphis Sep 23, 2020

Choose a reason for hiding this comment

membphis Sep 23, 2020

Choose a reason for hiding this comment

imjoey Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

imjoey Sep 25, 2020

Choose a reason for hiding this comment

tokers commented Sep 23, 2020

gxthrj commented Sep 23, 2020

imjoey commented Sep 23, 2020 • edited Loading

gxthrj commented Sep 23, 2020

imjoey commented Sep 23, 2020

nic-chen commented Sep 23, 2020

imjoey commented Sep 23, 2020

tokers commented Sep 23, 2020

tokers commented Sep 24, 2020

imjoey commented Sep 27, 2020

membphis Sep 27, 2020

Choose a reason for hiding this comment

imjoey Sep 28, 2020

Choose a reason for hiding this comment

membphis left a comment

Choose a reason for hiding this comment

membphis commented Sep 28, 2020

imjoey Sep 23, 2020 •

edited

Loading

imjoey commented Sep 23, 2020 •

edited

Loading