route: custom cluster specifier plugin support by wbpcode · Pull Request #20151 · envoyproxy/envoy

wbpcode · 2022-03-01T11:50:53Z

Signed-off-by: wbpcode wbphub@live.com

Commit Message: route: custom cluster provider support
Additional Description:

Now, the cluster name, cluster header and weighted clusters are supported to get a target cluster of the route entry.

And this PR try to create extensible cluster providers for the route entry to support more flexible approaches to get the target cluster.

Ref #20060 for more info.

Risk Level: Low.
Testing: Wait.
Docs Changes: N/A.
Release Notes: Wait.
Platform Specific Features: N/A.

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only · 2022-03-01T11:51:00Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20151 was opened by wbpcode.

see: more, trace.

wbpcode · 2022-03-01T11:52:03Z

First commit is pushed for quick API and design review.

wbpcode · 2022-03-01T11:52:38Z

cc @tyxia cc @alyssawilk

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-03-04T03:48:30Z

@lizan Friendly ping.

Hi, could you give a quick API review for this PR when you have free time, then we can push this work to the next step?

RyanTheOptimist · 2022-03-10T00:32:29Z

/wait

lizan · 2022-03-11T08:54:26Z

/lgtm api

markdroth · 2022-03-14T15:11:27Z

+  // A list of cluster providers and their configurations which may be used by a
+  // :ref:`cluster provider name <envoy_v3_api_field_config.route.v3.RouteAction.cluster_provider_name>`
+  // within the route. All *name* fields in this list must be unique.
+  repeated core.v3.TypedExtensionConfig cluster_providers = 12;


This change is not okay. This will break a field that is actually being used by gRPC. Just because a field is not used in Envoy does not mean it's okay to change it.

Please revert these API changes.

@markdroth So you implemented a hiden API of Envoy in the gRPC? While I think we can revert these API changes this time around, I don't think this is the right pattern. cc @envoyproxy/api-shepherds

I think you may be misunderstanding the context of the xDS API. While this API did start as part of Envoy, these days it's actually moving toward being a general-purpose data plane API, where Envoy is only one of many clients. In fact, gRPC has xDS client implementations in 4 different languages. We do sometimes add new fields to the API that gRPC implements support for before Envoy does; when this happens, the field may still be tagged with a "not-implemented-hide" tag for the benefit of Envoy's auto-generated documentation, but that doesn't mean that it's not used by other clients and is okay to break.

That having been said, it looks to me like the API versioning policy does incorrectly say that the policy does not apply to fields tagged with "not-implemented-hide". I think that's out of date and needs to be changed, and I'll put together a PR to fix that.

In any case, speaking as one of the API shepherds, it is definitely not okay to break these fields, since we do absolutely have clients of this API that are using them. Please revert these changes.

API versioning policy being updated in #20347.

Get it. May we need some new tags for fields that envoy not implemented but other proxies used.

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-03-20T12:18:17Z

/waiting

Waiting #20301 merged to avoid unnecessary conflict.

markdroth

I'd like input from the other @envoyproxy/api-shepherds on this.

markdroth · 2022-03-21T15:12:35Z

    string cluster_specifier_plugin = 37;
+
+    // Custom cluster provider configuration to use to determine the cluster for requests on this route.
+    core.v3.TypedExtensionConfig cluster_provider = 39


The way this is structured, it looks like you're adding a new extension point instead of implementing the one that already exists in the API. I don't think we want two different ways of doing the same thing.

If we do wind up wanting a way to define the plugin on a per-route basis, then I think it should just be another way of instantiating a ClusterSpecifierPlugin. We should not define a separate extension type called ClusterProvider that does basically the same thing. In other words, it could be something like this:

ClusterSpecifierPlugin cluster_specifier_plugin_instance = 39;

You are right. And I am waiting #20301 to be merged, then I will update this pr. Because maybe I need to move Cluster SpecifierPlugin to route_component.proto from route.proto.

Yeah, I agree with using the original type for clarity in this case.

…er_provider

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-03-29T16:59:15Z

Sorry, I didn't follow that explanation. Let me try asking a more specific question: If we allow the plugins to be defined at the VirtualHost level so that they work with VHDS, what will not work?

In fact, RouteConfiguration's plugins are already worked (Although in the vhds case there are some issues, it always worked). But it's not friendly for the current users of cluster_header/weighted_clusters to migrate their configs. And it's not friendly for some control planes who have special abstraction for Route.

wbpcode · 2022-03-29T17:14:16Z

To be clear, I'm really not comfortable with defining the plugin in the individual route. I think that will badly confuse the semantics of the ClusterSpecifierPlugin, which are intended to allow the plugin instance to be stateful and to have that same state apply across multiple routes that are using the same plugin. I think that in order to consider doing that, we need some justification as to why it's worth introducing that problem, and at the moment, it's not clear to me that we have such a justification.

IMO, it will not break any semantics. The shared plugin has the shared states. And the route level plugin is shared by only one single route. It is just a special use case of shared plugin.

And this should be a common design in the envoy API. A global shared config and a route level specified config. You can find some similar designs in the lua, rabc, etc.

wbpcode · 2022-03-29T17:27:21Z

The API I expected only has two targets:

extendable.
easy to replace current cluster_header/weighted_clusters.

It's hard to convince users to migrate their current route-level configs to RouteConfiguration and ref them in the Route. But it would be possible to convince users to move their route-level configs to a route-level Any wrapper.

So that we can finally smoothly deprecate the cluster_header/weighted_clusters.

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-04-21T13:28:24Z

Sorry for the late update. I opened too much threads at same time. 😞

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2022-04-22T12:24:33Z

/retest

repokitteh-read-only · 2022-04-22T12:24:37Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20151 (comment) was created by @wbpcode.

see: more, trace.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

markdroth · 2022-04-22T14:41:38Z

-    // :ref:`envoy_v3_api_field_config.route.v3.RouteConfiguration.cluster_specifier_plugins`
-    // in the
-    // :ref:`envoy_v3_api_field_config.core.v3.TypedExtensionConfig.name` field.
+    // Name of the cluster provider to use to determine the cluster for requests on this route.


This text still needs to be restored to the original wording. The original wording called this a "cluster specifier plugin", which is the correct term; the modified text says "cluster provider", which is not the term we're using here.

Get it. Thanks for calling it out.

To my surprise, I thought I had done all the name substitutions. I will check it again.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2022-04-24T02:18:25Z

Now, these should be no any 'cluster provider'. Could you take a look again? 😄 cc @markdroth

wbpcode · 2022-04-24T02:18:56Z

And it ready for an code review also. Could you take a look when you have free time? cc @mattklein123

markdroth · 2022-04-25T15:31:16Z

/lgtm api

mattklein123

Thanks LGTM with small comments. It would be nice to add a small integration test that adds a test plugin to set a route? Thank you!

/wait

wbpcode · 2022-04-26T02:15:09Z

It would be nice to add a small integration test that adds a test plugin to set a route? Thank you!

Get it.

wbpcode · 2022-04-26T02:15:14Z

/wait

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2022-04-26T14:07:07Z

/retest

repokitteh-read-only · 2022-04-26T14:07:11Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20151 (comment) was created by @wbpcode.

see: more, trace.

mattklein123

Thanks!

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

initial commit for quick review

535df6f

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only Bot added the api label Mar 1, 2022

repokitteh-read-only Bot assigned lizan Mar 1, 2022

fix docs

c59bab7

Signed-off-by: wbpcode <wbphub@live.com>

lizan reviewed Mar 9, 2022

View reviewed changes

Comment thread api/envoy/config/route/v3/route_components.proto Outdated

repokitteh-read-only Bot added the waiting label Mar 10, 2022

repokitteh-read-only Bot removed the api label Mar 11, 2022

markdroth suggested changes Mar 14, 2022

View reviewed changes

revert exist plugin API

c7d3a0e

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only Bot added api and removed waiting labels Mar 20, 2022

fix format

a356512

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode added the waiting label Mar 21, 2022

markdroth reviewed Mar 21, 2022

View reviewed changes

wbpcode added 2 commits March 28, 2022 11:24

Merge branch 'main' of https://github.com/envoyproxy/envoy into clust…

bdcd49d

…er_provider

new api and implementation

f6fbce8

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only Bot removed the waiting label Mar 29, 2022

fix docs

626b2c8

Signed-off-by: wbpcode <wbphub@live.com>

markdroth reviewed Mar 29, 2022

View reviewed changes

Comment thread api/envoy/config/route/v3/route.proto

Comment thread api/envoy/config/route/v3/route_components.proto

Comment thread api/envoy/config/route/v3/route_components.proto

Comment thread api/envoy/config/route/v3/route_components.proto Outdated

fix docs

03762d4

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode added 2 commits April 21, 2022 16:34

mock

3d5700f

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

add more test

47ed903

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode added 4 commits April 22, 2022 12:27

minor api

453948c

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

fix interface comments

35ce2b7

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

support optional

59f5feb

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

rename cluster provider to cluster specifier plugin

1edf1c0

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

markdroth reviewed Apr 22, 2022

View reviewed changes

wbpcode added 2 commits April 23, 2022 10:39

fix some name

cbcdbbe

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

add optional test

53306eb

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode changed the title ~~route: custom cluster plugin support~~ route: custom cluster specifier plugin support Apr 23, 2022

repokitteh-read-only Bot removed the api label Apr 25, 2022

mattklein123 requested changes Apr 25, 2022

View reviewed changes

Comment thread envoy/router/cluster_specifier_plugin.h Outdated

Comment thread source/common/router/config_impl.cc Outdated

repokitteh-read-only Bot added the waiting label Apr 25, 2022

wbpcode added 2 commits April 26, 2022 04:01

address review comments

8a030fb

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

add integration test

8e356bd

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only Bot removed the waiting label Apr 26, 2022

minor update

27ad1e6

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

mattklein123 approved these changes Apr 26, 2022

View reviewed changes

mattklein123 merged commit 07e414c into envoyproxy:main Apr 26, 2022

ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022

route: custom cluster specifier plugin support (envoyproxy#20151)

2a992e2

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

Conversation

wbpcode commented Mar 1, 2022

Uh oh!

repokitteh-read-only Bot commented Mar 1, 2022

Uh oh!

wbpcode commented Mar 1, 2022

Uh oh!

wbpcode commented Mar 1, 2022

Uh oh!

wbpcode commented Mar 4, 2022

Uh oh!

Uh oh!

RyanTheOptimist commented Mar 10, 2022

Uh oh!

lizan commented Mar 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode commented Mar 20, 2022

Uh oh!

markdroth left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

wbpcode commented Apr 21, 2022

Uh oh!

wbpcode commented Apr 22, 2022

Uh oh!

repokitteh-read-only Bot commented Apr 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdroth commented Apr 25, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Apr 26, 2022

Uh oh!

wbpcode commented Apr 26, 2022

Uh oh!

wbpcode commented Apr 26, 2022

wbpcode commented Mar 29, 2022 •

edited

Loading

wbpcode commented Mar 29, 2022 •

edited

Loading

wbpcode commented Mar 29, 2022 •

edited

Loading

wbpcode commented Apr 24, 2022 •

edited

Loading

wbpcode commented Apr 24, 2022 •

edited

Loading