config: ADS implementation. #1658
Conversation
This patch introduces a GrpcMux implementation, loosely based on GrpcSubscriptionImpl, but generalized to encompass the case of both a single xDS API channel (e.g. EDS) and a multiplex channel (ADS).
|
@mattklein123 this is now ready for review. There are some additional unit/integration tests that I plan, as well as a full writeup on the ADS (and regular xDS) message sequencing in envoy-api, but this is basically it. |
| /** | ||
| * Constant Type URLs. | ||
| */ | ||
| class TypeUrlValues { |
There was a problem hiding this comment.
Should this go in the "well known" file? Don't care just a thought.
There was a problem hiding this comment.
I prefer to draw a line between the actual config and the delivery vehicle constants, so resources.h seems a better choice I think.
source/common/config/utility.cc
Outdated
| return MessageUtil::anyConvert<envoy::api::v2::ClusterLoadAssignment>(resource).cluster_name(); | ||
| } | ||
| ASSERT(false); | ||
| return ""; |
There was a problem hiding this comment.
should we throw exception here?
There was a problem hiding this comment.
Good idea, I think I originally didn't think this was in the scope of a try block, but it is.
|
@mattklein123 this is now ready for final review. |
|
@htuch looks like having some test issues. Ping me when passing and I can take a look. |
|
@mattklein123 should be fixed now. |
| stats_.update_attempt_.inc(); | ||
| } | ||
|
|
||
| const std::string versionInfo() const override { NOT_IMPLEMENTED; } | ||
| const std::string versionInfo() const override { return version_info_; } |
There was a problem hiding this comment.
As I understand it, the version here will be the last returned version from a discovery response, regardless of MUXd resource, right? I think that's fine but might be worth a comment.
There was a problem hiding this comment.
This will be for the specific type URL of the resource. There is one GrpcMuxSubscriptionImpl per type URL.
There was a problem hiding this comment.
Sorry where is that setup? (Just trying to understand)
There was a problem hiding this comment.
Ah I see: https://github.com/envoyproxy/envoy/pull/1658/files#diff-1adb93ed0d49e43e5ed44c13ff6ea6cbR78
OK got it. Makes sense. This is neat. It came together really well.
| envoy::api::v2::DiscoveryRequest request_; | ||
| SubscriptionStats stats_; | ||
| GrpcMuxImpl grpc_mux_; | ||
| GrpcMuxSubscriptionImpl<ResourceType> ads_subscription_; |
There was a problem hiding this comment.
I'm a little confused here. For a singleton gRPC subscription, how does this work? Does this somehow go to the null ADS?
There was a problem hiding this comment.
Or we are just using same code? If so rename grpc_subscription_?
There was a problem hiding this comment.
It uses its own instance of GrpcMuxImpl, with jut a single subscription on the channel. Will update variable name as suggested.
It turns out that removing Kotlin from Envoy Mobile is not really needed at this point. In reality, the Kotlin classes are a layer on top of the Java classes, and Cronvoy is a layer too: one level of indirection for the callbacks is enough. So instead Cronvoy uses the java classes directly. The main drawback is the CronvoyConfiguration - there is some duplication. Description: Have Cronvoy to only use the Envoy Mobile Java classes for its implementation Risk Level: None Testing: CI Docs Changes: N/A Release Notes: N/A Fixes #1658 Signed-off-by: Charles Le Borgne <[email protected]> Signed-off-by: JP Simard <[email protected]>
It turns out that removing Kotlin from Envoy Mobile is not really needed at this point. In reality, the Kotlin classes are a layer on top of the Java classes, and Cronvoy is a layer too: one level of indirection for the callbacks is enough. So instead Cronvoy uses the java classes directly. The main drawback is the CronvoyConfiguration - there is some duplication. Description: Have Cronvoy to only use the Envoy Mobile Java classes for its implementation Risk Level: None Testing: CI Docs Changes: N/A Release Notes: N/A Fixes #1658 Signed-off-by: Charles Le Borgne <[email protected]> Signed-off-by: JP Simard <[email protected]>
This patch introduces a GrpcMux implementation, loosely based on GrpcSubscriptionImpl, but
generalized to encompass the case of both a single xDS API channel (e.g. EDS) and a multiplex
channel (ADS).
Signed-off-by: Harvey Tuch [email protected]