thrift: adding xDS RDS support for thrift proxy.

[tkovacs-2]

Commit Message:
Adding xDS RDS support for thrift proxy.

Additional Description:

• The current RDS implementation is tied to the HTTP protocol. To make it reusable for other protocols
  base classes containing the generic part are extracted from the existing code.
• The location of the base classes are envoy/rds and source/common/rds and the Envoy::Rds namespace.
• Existing code is reworked to use the base classes and HTTP specific features added back by decorator pattern
  (StaticRouteConfigProviderImpl, RdsRouteConfigProviderImpl, RouteConfigUpdateReceiverImpl)
  and template method pattern (RdsRouteConfigSubscription).
• The RDS implementation for Thrift is simply the direct usage of base classes.
• The only thing which has to be implemented separately for every protocol is the
  RouteConfigProviderManagerImpl::createRdsRouteConfigProvider and
  RouteConfigProviderManagerImpl::createStaticRouteConfigProvider functions.
  To make it easier helper functions are added to Rds::RouteConfigProviderManagerImpl.
• New grpc/restapi endpoint is added for Thrift route discovery service.

Risk Level:
Medium

Testing:

• Unittest
• Manual test with static routing from bootstrap config, static routing and dynamic routing from GRPC based xDS server.

Docs Changes:
Comment added to the new API field.

Release Notes:

Platform Specific Features:

[repokitteh-read-only]

Hi @tkovacs-2, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17631 was opened by tkovacs-2.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17631 was opened by tkovacs-2.

see: more, trace.

[rgs1]

@tkovacs-2 this is great, thank you! Any reasons to avoid base classes in source/common/router/ ? My intuition is that we'd want to share, unless you think there'll be irreconcilable differences between HTTP and Thrift towards handling RDS.

cc: @htuch to get more guidance towards sharing common RDS code vs having Thrift and HTTP diverge.

[rgs1]

Generally LGTM, besides my previous comment about possibly adding base classes to share code with the existing router RDS implementation.

[rgs1]

nit: maybe add a mirroring policy here (#17544)

[tkovacs-2]

In these tests the exact content of the route configuration is not important.
If I understand right this mirroring is an attribute for the RouteAction. What is the benefit of add such here?

[htuch]

I'm not sure I fully understand what is proposed. How can Thrift make use of RDS, which is about HTTP routes?

We definitely should not fork the RDS code, there is a lot of subtlety and complexity in the warming, shared providers and lifecycle (for on-demand). I would recommend making the base code more generic if it doesn't add too much complexity to Envoy core (i.e. it's just about making some resource type opaque).

CC @envoyproxy/api-shepherds @adisuissa

[tkovacs-2]

@htuch @rgs1
Thank you for your comments.

If the static routing rules change in a listener a new listener is created and after a drain period the old one is deleted. This has two negative consequences:

• During the drain period the clients connected to old listener are using the outdated routing rules.
• When the drain period is over the connections will be closed by envoy which can cause lost messages.

With RDS the routing rules can be changed dynamically without creating new listener and the already established connections can use the new rules immediately, so it is a very good feature.

I know the major protocol in envoy is http, but RDS support in other protocols would could come handy sometimes.
For example in our case. Because the lack of it in thrift we had to add hacks on client side, like to poll envoy config dump to check if new rules have applied yet and do reconnect.

On api level the RDS is independent from the listener's protocol. I added the 'message Rds' to the thrift part just because I did not want to cross refer to envoy.extensions.filters.network.http_connection_manager.v3 for the same.
Also the RdsRouteConfigSubscription::onConfigUpdate gets generic Protobuf::Message-s.

Of course I did not want Thrift listener with HTTP routing config.
The problem with the classes in source/common/router/rds_impl.* is they directly refer to the envoy::config::route::v3::RouteConfiguration and Envoy::Router::Config which are HTTP routing config. So pointless to derive from them.
That is why I just copied them, deleted the unnecessary parts and change the HTTP route config class names to the Thrift counterpart. Even I don't like this solution that much.

What may be better is to turn to templates those classes what I added to the thrift with the protocol's routing config as template parameter and move back to source/common/routing.
These could be used as base class of current HTTP rds classes and with minimal new code possible to introduce RDS support into other protocols as well.

[rgs1]

I'm not sure I fully understand what is proposed. How can Thrift make use of RDS, which is about HTTP routes?

We definitely should not fork the RDS code, there is a lot of subtlety and complexity in the warming, shared providers and lifecycle (for on-demand). I would recommend making the base code more generic if it doesn't add too much complexity to Envoy core (i.e. it's just about making some resource type opaque).

Yeah, that's what I was referring to -- reusing as much as possible as long as it doesn't become too complex.

[rgs1]

If the static routing rules change in a listener a new listener is created and after a drain period the old one is deleted. This has two negative consequences:

• During the drain period the clients connected to old listener are using the outdated routing rules.
• When the drain period is over the connections will be closed by envoy which can cause lost messages.

With RDS the routing rules can be changed dynamically without creating new listener and the already established connections can use the new rules immediately, so it is a very good feature.

I know the major protocol in envoy is http, but RDS support in other protocols would could come handy sometimes.
For example in our case. Because the lack of it in thrift we had to add hacks on client side, like to poll envoy config dump to check if new rules have applied yet and do reconnect.

I agree with all of this, this is a great addition.

On api level the RDS is independent from the listener's protocol. I added the 'message Rds' to the thrift part just because I did not want to cross refer to envoy.extensions.filters.network.http_connection_manager.v3 for the same.

Makes sense.

Also the RdsRouteConfigSubscription::onConfigUpdate gets generic Protobuf::Message-s.

Of course I did not want Thrift listener with HTTP routing config.
The problem with the classes in source/common/router/rds_impl.* is they directly refer to the envoy::config::route::v3::RouteConfiguration and Envoy::Router::Config which are HTTP routing config. So pointless to derive from them.
That is why I just copied them, deleted the unnecessary parts and change the HTTP route config class names to the Thrift counterpart. Even I don't like this solution that much.

Yeah that was my concern.

What may be better is to turn to templates those classes what I added to the thrift with the protocol's routing config as template parameter and move back to source/common/routing.
These could be used as base class of current HTTP rds classes and with minimal new code possible to introduce RDS support into other protocols as well.

That would be great, shall we go down that path?

Thanks!

[htuch]

So, there is probably two things here to consider API and config plane wise:

1. I agree that we probably want the moral equivalent of route providers and RDS for non-HTTP routing. We should be able to reuse almost all of what was created for RDS by making it generic. I would be fairly strongly opposed to copy+paste forking it.
2. We shouldn't use the RDS gRPC endpoint for the same purpose, since it's actually delivering a different xDS resource type. I would suggest when it comes to gRPC endpoint (if not using ADS), that we have a distinct endpoint.

[tkovacs-2]

So, there is probably two things here to consider API and config plane wise:

1. I agree that we probably want the moral equivalent of route providers and RDS for non-HTTP routing. We should be able to reuse almost all of what was created for RDS by making it generic. I would be fairly strongly opposed to copy+paste forking it.

Understood.

2. We shouldn't use the RDS gRPC endpoint for the same purpose, since it's actually delivering a different xDS resource type. I would suggest when it comes to gRPC endpoint (if not using ADS), that we have a distinct endpoint.

In case of RDS without ADS to introduce new endpoints maybe not necessary. Please see the change in the commit 3235184. We can just map the new type url-s back to envoy.config.route.v3.RouteConfiguration and we can reuse the RouteDiscoveryService endpoint. The DiscoveryRequest/DiscoveryResponse doesn't depend on the protocol also it is backward compatible from xds server point of view.

Alternatively we can say the non-http RDS can be used just with ADS.

[tkovacs-2]

That would be great, shall we go down that path?

Thanks!

Sure.

[markdroth]

I agree with @htuch that it's not obvious to me that we want to use exactly the same resource type for HTTP RouteConfiguration and Thrift RouteConfiguration. The resource aliasing thing seems a little ugly. And even if the two resource types happen to be the same right now, who's to say that they won't diverge in the future?

Also, in terms of the API representation, you may want to use the new matching API, which @snowp is working on integrating into RDS in #17633.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[repokitteh-read-only]

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

🐱

Caused by: a #17631 (comment) was created by @tkovacs-2.

see: more, trace.

[htuch]

@tkovacs-2 thanks. I think the next step is as follows:

• @adisuissa will provide a detailed review pass as first-pass reviewer.
• I think we need to break the PR up into probably two PRs. The first should be the RDS and route config provider refactor (i.e. with no Thrift code whatsoever) and the second will be the incremental Thrift usage of RDS. We can refer back to this PR during the review to show how they integrate.
• It's still a little unclear to me how this sits with the efforts in api: MetaProtocol Proxy #18823. I imagine they must have to solve this same problem somehow, @wbpcode @zhaohuabing can you take a look at this PR and comment on how much similarity there is? Have you built an efficient shared route config provider as done in this PR?

[zhaohuabing]

@htuch We have built an unified RDS solution in Aeraki https://github.com/aeraki-framework/meta-protocol-proxy, proven working with both Thrift and Dubbo. The MetaProtocol PR is intended to push what we have done in Aeraki to the Envoy, including unified routing (RDS), tracing, logging, etc., so it can be used as the chassis for Dubbo, Thrift, and other request/response, stateless style protocols users would like to support in Envoy with their own codecs.

[tkovacs-2]

Hi @zhaohuabing,
The RDS in Aeraki is a copy-paste of the current RDS implementation in envoy. First I did the same for Thrift RDS in this PR. But it was not accepted by reviewers.
Now the major part of the PR is to make the RDS implementation reusable for other protocols than HTTP. So I suppose this will be useful for you as well to avoid copy-paste.
The Thrift RDS has become a minor part.

One things are not clear for me in the metaprotocol:
What is your plan with current thrift, dubbo, etc. implementations in envoy?
If your aim is to simplify the filter implementations then maybe better approach is to extract the common parts of the current implementations to reusable classes without introducing new api.
Now it seems to me you are building up a parallel universe with the metaprotocol.

[zhaohuabing]

Hi @zhaohuabing, The RDS in Aeraki is a copy-paste of the current RDS implementation in the envoy. First I did the same for Thrift RDS in this PR. But it was not accepted by reviewers. Now the major part of the PR is to make the RDS implementation reusable for other protocols than HTTP. So I suppose this will be useful for you as well to avoid copy-paste. The Thrift RDS has become a minor part.

The RDS(we call it MetaRDS) in Aeraki is a unified solution for all the applications built on top of the MetaProtocol Proxy, including Thrift, Dubbo, and other proprietary protocols. We didn't include HTTP/gRPC routing in the MetaRDS because HTTP Routing is already handled very well by the current RDS and HCM, and it's not in the design scope of the MetaProtcol proxy.
The current MetaRDS code in Aeraki resues the HTTP RDS endpoint for quick prototype and verification. We could improve that in the final implementation.

One things are not clear for me in the metaprotocol: What is your plan with current thrift, Dubbo, etc. implementations in envoy? If your aim is to simplify the filter implementations then maybe better approach is to extract the common parts of the current implementations to reusable classes. Now it seems to me you are building up a parallel universe with the metaprotocol.

The goal of the MetaProtocol Proxy is to build a unified solution for all request/response, stateless protocols, avoiding duplication and allowing users to create protocol filters more easily. For example, if you want to create a X protocol filter, instead of writing the whole TCP filter, now you only need to implement the codec Interface of MetaProtocol Proxy. More about the goal and design can be found here: https://docs.google.com/document/d/1y_ALyjGp_H1_TRrrRxZxnqENISqeZI6eLn71FAFsZPk/edit#heading=h.ic0mhtrvlmy9

[tkovacs-2]

@htuch
Please see the PR #18846 for the RDS refactor part.

[tkovacs-2]

@zhaohuabing
Yes, I understand the document, but the answer for my question is still not clear.
I suppose you want to merge the thrift (and dubbo) codec together with the metaprotocol filter, at least as example.
What will happen with the current native thrift (and dubbo) filter?
To drop them is a little drastic because the new api is different, also it should provide at least the same features and performance as the native. To keep them is also strange because then these protocols will be duplicated.
The aim was to simplify the filter implementations. So I would not introduce the metaprotocol on api level as a new filter but as a kind of framework for filter implementation. Also turn the existing native filter's code to use the metaprotocol framework with their current api.
To make a new filter api is not a big thing, so I don't feel the unified api as big gain, but rather limit on long term when the needs of different protocols start to diverge.
But this is just my personal opinion.

[zhaohuabing]

@zhaohuabing
Yes, I understand the document, but the answer for my question is still not clear.
I suppose you want to merge the thrift (and dubbo) codec together with the metaprotocol filter, at least as example.
What will happen with the current native thrift (and dubbo) filter?
To drop them is a little drastic because the new api is different, also it should provide at least the same features and performance as the native. To keep them is also strange because then these protocols will be duplicated.

The framework code will be pushed to the MetaProtocol proxy filter. Although Dubbo and Thrift have been proven working on top of the Meta Protocol proxy, however, we need community consense if we'd like to port dubbo and thrift to metaprotocol. The core maintainers and owners of Dubbo and Thrift should agree on that before the shift. I personally think it's not the key issue here, Thrift filter could exist as it is now if the community think it should be, and the MetaProtocol proxy can be used to facilitate protocols that are not currently supported in the Envoy.

The aim was to simplify the filter implementations. So I would not introduce the metaprotocol on api level as a new filter but as a kind of framework for filter implementation. Also turn the existing native filter's code to use the metaprotocol framework with their current api.
To make a new filter api is not a big thing, so I don't feel the unified api as big gain, but rather limit on long term when the needs of different protocols start to diverge.
But this is just my personal opinion.

It's not just APIs. The implementation will be pushed later after the community agrees on the design and finalizes the API. The application protocols built on top of the MetaProtocol proxy only needs to implement the codec interface. Sorry for causing the confusion.

[wbpcode]

@tkovacs-2
We hope that the meta protocol proxy will make it easier to implement new protocols in Envoy, including various private protocols. So we will provide a full set of proto APIs and a new implementation.
Replacing the dubbo proxy and thrift proxy is not our goal. Unless the meta protocol proxy is proven to work very well in Envoy in the future, and the community agrees, we may replace their implementations while keeping the existing dubbo/thrift APIs the same.
Since there would be a lot of work for this big feature. So we split the PR into multiple, proto APIs, core frameworks, features, etc.

[htuch]

I think there are a few things going on here that would be good to clarify:

1. @zhaohuabing @wbpcode the point that @tkovacs-2 makes around the MetaRDS implementation has me a little concerned. Can you point me to the MetaRDS implementation? We've been working with @tkovacs-2 so that we don't end up doing a copy+paste of the common route config provider code from the core in this PR, and I expect we might do the same for MetaRDS. If so, by first landing rds: extracting base classes as reusable generic routing DS implementation. #18846, we can maybe be in a better position to integrate MetaRDS without as much copy+paste from how RDS works today (if that is how it is done). Do you think rds: extracting base classes as reusable generic routing DS implementation. #18846 would help with MetaRDS?
2. @zuercher @rgs1 can you weight in on the positioning of the existing Thrift filter with the MetaProtocol Thrift? I agree it's confusing to have two independent implementations and we'll want some clarity on which is the production ready solution.

[wbpcode]

Hi, @htuch We will rewrite the whole meta protocol proxy, so it doesn't matter what the old implementation is before. After merging the core framework of new meta protocol proxy, we will have another special PR and design to do this work. I think at that time, #18846 will be very helpful.

And, replacing the dubbo proxy and thrift proxy is not our goal. Although we have implemented thrift codec/dubbo codec in the old meta protocol proxy, but it would not be the part of new meta protocol proxy. So there is no need to worry about the co-existence of thrift proxy and meta protocol thrift.

[tkovacs-2]

@wbpcode
Ok, clear.

@htuch

1. #18846 would help with MetaRDS?

Yes, the current MetaRDS implementation I think is here (rds.h rds_impl.* route_config_*)
Once the #18846 is merged, basically it can be the same like the Thrift RDS part in this PR.

[htuch]

OK, thanks for the clarity; it makes sense to hold back on replacing extant filters until we have stability here and there is enough commonality that we can ahead with the refactoring PR. I wonder if we want both TRDS and MetaRDS though; could these be the same t hing once we merge #18846?

[tkovacs-2]

I think they can't be exactly the same, since the type of route config (ConfigImpl and the related proto) is different for them.
And somebody need to create these objects of the proper types. That means filter specific implementations of RouteConfigProviderManagerImpl and ConfigTraitsImp will always be needed.
However in case of TRDS, MetaRDS and other simple cases (when the ConfigImpl depends only on the proto and no other ticking like in http RDS) the difference is only the types names and the code is just copy-paste.

I have planned to introduce a template default implementation for these under Envoy::Rds.
In this way the code what is currently in thrift_proxy/router/rds.h, rds_impl.h, rds_impl.cc and rds_impl_test.cc just need once. Otherwise same would be needed for every filter.

[zuercher]

Thanks! I think this looks good generally, but I was only able to give it cursory look today. I'm also out tomorrow, so I can't get back to this until Friday.

[tkovacs-2]

@zuercher
Thanks for your comments. For easier review meanwhile I split the generic part into a separate PR #18846. I will add the fixes for it there.

@htuch Do you have some suggestion how to handle thrift part?
I mean that depends on the generic part. If I merge the branch of #18846 into the branch of this PR, can github handle that?
Or just collect the comments here and fix here after #18846 is merged?

[htuch]

@tkovacs-2 "Or just collect the comments here and fix here after #18846 is merged?" <-- yep, GitHub doesn't have very good support for a PR pipeline that I know of.

[rojkov]

Wait for #18846 to be merged.
/wait

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!