-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
router: scoped RDS (2/4): support scoped routing configuration #5839
Changes from 8 commits
cdae885
db48450
756afcd
8e624ea
98149e5
76b7196
f924eb7
51d8dff
ccca775
e5d87db
9c3d45b
afcc2a5
c774f00
8cc1b13
71f2a74
5aed783
15b1c7c
b216034
c8f531b
39309ac
8fe33ad
8c38526
dd2261c
a7a621b
090bbd8
8b8a5e7
43c95e8
6adaa22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
syntax = "proto3"; | ||
|
||
package envoy.api.v2; | ||
|
||
option java_outer_classname = "SrdsProto"; | ||
option java_package = "io.envoyproxy.envoy.api.v2"; | ||
option java_multiple_files = true; | ||
option java_generic_services = true; | ||
|
||
import "envoy/api/v2/discovery.proto"; | ||
|
||
import "google/api/annotations.proto"; | ||
|
||
import "validate/validate.proto"; | ||
import "gogoproto/gogo.proto"; | ||
|
||
option (gogoproto.equal_all) = true; | ||
|
||
// [#protodoc-title: HTTP scoped routing configuration] | ||
// * Routing :ref:`architecture overview <arch_overview_http_routing>` | ||
// | ||
// .. attention:: | ||
// | ||
// The Scoped RDS API is not yet fully implemented and *should not* be enabled in | ||
// :ref:`envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager`. | ||
|
||
// The resource_names field in DiscoveryRequest specifies a set of route configuration scopes. Each | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add ref link to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// scope points to a route_configuration_name which will be used to request a | ||
// :ref:`RouteConfiguration<envoy_api_msg_Route.RouteConfiguration>` via the RDS API. | ||
service ScopedRoutesDiscoveryService { | ||
rpc StreamScopedRoutes(stream DiscoveryRequest) returns (stream DiscoveryResponse) { | ||
} | ||
|
||
rpc IncrementalScopedRoutes(stream IncrementalDiscoveryRequest) | ||
returns (stream IncrementalDiscoveryResponse) { | ||
} | ||
|
||
rpc FetchScopedRoutes(DiscoveryRequest) returns (DiscoveryResponse) { | ||
option (google.api.http) = { | ||
post: "/v2/discovery:scoped-routes" | ||
body: "*" | ||
}; | ||
} | ||
} | ||
|
||
// This configuration represents a set of "scopes", each containing independent routing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to be honest in saying that it's very hard for me to follow the messages/documentation in this file. I read the spec a while ago but I've forgotten it. Would it be possible to add some high level documentation here that walks the end user through some examples of how the API is setup, the fragment builder works, etc. If you plan on doing this in arch overview docs that's fine too, whatever is easiest. The reason I ask for this now is I think it would make it a lot easier to review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial plan was to add in-depth documentation including examples to the arch overview docs (which I wasn't planning to do until the feature is fully implemented in PR 3), but I agree better documentation in the proto definition would be helpful for this review. I am working on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a ton. I think even just some YAML examples here in the proto docs would be super helpful. You could move them to the arch overview later or just link to them here, so hopefully zero wasted effort. |
||
// configuration (see :ref:`routing architecture overview <arch_overview_http_routing>`). A scope is | ||
// assigned to each request based on request attributes, such as the value of a header designated | ||
// via this configuration. | ||
// [#comment:next free field: 4] | ||
message ScopedRouteConfigurationsSet { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Upon revisiting the SRDS proto in the context of Incremental xDS, I have decided to make a few changes after discussing with @htuch. To easily enable incremental updates to the routing scopes (represented by the This change creates much better alignment with the API/protocol already defined for incremental updates, and also simplifies the SRDS API. I am actively working on this now and will introduce the changes discussed as part of the next few commits to this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AndresGuedez cool I will defer looking at the API again until this is done. Thank you for iterating on this. /wait |
||
// The name of the set of route configuration scopes. This will be the | ||
// :ref:`scoped_routes_config_set_name | ||
// <envoy_api_field_config.filter.network.http_connection_manager.v2.ScopedRds.scoped_routes_config_set_name>` | ||
// set in :ref:`envoy_api_msg_config.filter.network.http_connection_manager.v2.ScopedRds`. | ||
string name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// Specifies the mechanism for constructing keys based on request attributes to match | ||
// :ref:`scopes <envoy_api_field_ScopedRouteConfigurationsSet.scopes>` against. | ||
// | ||
// Upon receiving a request's headers, the Router will build a key using the algorithm specified | ||
// by this message. This key will be used to look up a corresponding | ||
// :ref:`Scope <envoy_api_msg_ScopedRouteConfigurationsSet.Scope>` in a table containing the set | ||
// of :ref:`scopes <envoy_api_field_ScopedRouteConfigurationsSet.scopes>`. | ||
message ScopeKeyBuilder { | ||
// Specifies the mechanism for constructing fragments which are composed into keys. | ||
message FragmentBuilder { | ||
// Specifies how the value of a header should be extracted. | ||
// The following example maps the structure of a header to the fields in this message. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe include an example of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an example in the ScopedRouteConfigurationsSet documentation. |
||
// | ||
// .. code:: | ||
// | ||
// X-Header: a,b;c,d | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// | || | | ||
// | || \\----> <element_separator> | ||
// | || | ||
// | |\\----> <element.separator> | ||
// | | | ||
// | \\----> <element.key> | ||
// | | ||
// \\----> <name> | ||
message HeaderValueExtractor { | ||
// The name of the header to extract the value from. | ||
string name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// The element separator (e.g., ';' separates 'a;b;c;d'). | ||
string element_separator = 2; | ||
|
||
// Specifies a key value pair to match on. | ||
message KvElement { | ||
// The separator between key and value (e.g., '=' separates 'k=v;...'). | ||
string separator = 1; | ||
|
||
// The key to match on. | ||
string key = 2; | ||
} | ||
|
||
oneof extract_type { | ||
// Specifies the index of the element to extract. | ||
int32 index = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unsigned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't find mention of signed vs unsigned in Envoy's style guide so I defaulted to Google's style of using signed integers and relying on assertions to check val >= 0. |
||
|
||
// Specifies the key value pair to extract the value from. | ||
KvElement element = 4; | ||
} | ||
} | ||
|
||
oneof type { | ||
option (validate.required) = true; | ||
|
||
// Specifies how a header field's value should be extracted. | ||
HeaderValueExtractor header_value_extractor = 1; | ||
} | ||
} | ||
|
||
// The constructed key consists of the union of these fragments. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does order matter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, updated comment. |
||
repeated FragmentBuilder fragments = 1 [(validate.rules).repeated .min_items = 1]; | ||
} | ||
|
||
// The key construction mechanism. | ||
ScopeKeyBuilder scope_key_builder = 2 [(validate.rules).message.required = true]; | ||
|
||
// Specifies a routing scope, which associates a :ref:`envoy_api_msg_RouteConfiguration` to a | ||
// :ref:`Key <envoy_api_msg_ScopedRouteConfigurationsSet.Scope.Key>` which is matched against | ||
// each HTTP request. | ||
message Scope { | ||
// Specifies a key which is matched against by the output of a :ref:`ScopeKeyBuilder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "matched against the output" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// <envoy_api_msg_ScopedRouteConfigurationsSet.ScopeKeyBuilder>`. | ||
message Key { | ||
message Fragment { | ||
oneof type { | ||
option (validate.required) = true; | ||
|
||
// A string to match against. | ||
string string_key = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this match on the key from the associated FragmentBuilder? Or on the extracted value? Maybe add some details to the comments (either here or on the top-level Key) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment a few lines above pertaining to the 'Key' message as a whole attempts to clarify this. Let me know if it is sufficient. |
||
} | ||
} | ||
|
||
// The ordered set of fragments to match against. | ||
repeated Fragment fragments = 1 [(validate.rules).repeated .min_items = 1]; | ||
} | ||
|
||
// The resource name to use for a :ref:`envoy_api_msg_DiscoveryRequest` to an RDS server to | ||
// fetch the :ref:`envoy_api_msg_RouteConfiguration` associated with this scope. | ||
string route_configuration_name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// The key to match against. | ||
Key key = 2 [(validate.rules).message.required = true]; | ||
} | ||
|
||
// The set of scopes containing :ref:`Key <envoy_api_msg_ScopedRouteConfigurationsSet.Scope.Key>` | ||
// to :ref:`envoy_api_msg_RouteConfiguration` mappings. | ||
repeated Scope scopes = 3 [(validate.rules).repeated .min_items = 1]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ option go_package = "v2"; | |
import "envoy/api/v2/core/config_source.proto"; | ||
import "envoy/api/v2/core/protocol.proto"; | ||
import "envoy/api/v2/rds.proto"; | ||
import "envoy/api/v2/srds.proto"; | ||
import "envoy/config/filter/accesslog/v2/accesslog.proto"; | ||
import "envoy/type/percent.proto"; | ||
|
||
|
@@ -24,7 +25,7 @@ import "gogoproto/gogo.proto"; | |
// [#protodoc-title: HTTP connection manager] | ||
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`. | ||
|
||
// [#comment:next free field: 30] | ||
// [#comment:next free field: 32] | ||
message HttpConnectionManager { | ||
enum CodecType { | ||
option (gogoproto.goproto_enum_prefix) = false; | ||
|
@@ -63,6 +64,14 @@ message HttpConnectionManager { | |
envoy.api.v2.RouteConfiguration route_config = 4; | ||
} | ||
|
||
oneof scoped_routes_specifier { | ||
// Configuration for the Scoped RDS API which dynamically loads routing configuration scopes. | ||
ScopedRds scoped_rds = 30; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be simpler if these options were just in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial intention keeping them separate was to avoid confusion that SRDS and RDS are tightly coupled and require the same config source, etc. However, based on the feedback in this review and reassessing this from the user's standpoint, I agree this is simpler if |
||
|
||
// A static set of routing scopes. | ||
envoy.api.v2.ScopedRouteConfigurationsSet scoped_routes_config = 31; | ||
} | ||
|
||
// A list of individual HTTP filters that make up the filter chain for | ||
// requests made to the connection manager. Order matters as the filters are | ||
// processed sequentially as request events happen. | ||
|
@@ -389,11 +398,31 @@ message Rds { | |
envoy.api.v2.core.ConfigSource config_source = 1 | ||
[(validate.rules).message.required = true, (gogoproto.nullable) = false]; | ||
|
||
// The name of the route configuration. This name will be passed to the RDS | ||
// API. This allows an Envoy configuration with multiple HTTP listeners (and | ||
// associated HTTP connection manager filters) to use different route | ||
// configurations. | ||
string route_config_name = 2 [(validate.rules).string.min_bytes = 1]; | ||
oneof subscription_specifier { | ||
option (validate.required) = true; | ||
|
||
// The name of the route configuration. This name will be passed to the RDS | ||
// API. This allows an Envoy configuration with multiple HTTP listeners (and | ||
// associated HTTP connection manager filters) to use different route | ||
// configurations. | ||
string route_config_name = 2 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// Must be set to true when scoped RDS is enabled. | ||
// If this is enabled, RDS subscriptions will be triggered by scoped RDS config updates using | ||
// this configuration as a template. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What part of this configuration is being used as a template? Unclear to me what that means exactly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per #5839 (comment), I am changing this and will be getting rid of the scoped_rds_template. Instead, the ScopedRds message in HttpConnectionManager will include the ConfigSource required for the RDS server along with the ConfigSource for the SRDS server. |
||
bool scoped_rds_template = 3; | ||
} | ||
} | ||
|
||
message ScopedRds { | ||
// Configuration source specifier for scoped RDS. | ||
envoy.api.v2.core.ConfigSource config_source = 1 | ||
[(validate.rules).message.required = true, (gogoproto.nullable) = false]; | ||
|
||
// The name of the set of routing configuration scopes. This name will be passed to the scoped RDS | ||
// API. | ||
// This allows Envoy to segment routing configuration based on a configurable request attribute. | ||
string scoped_routes_config_set_name = 2 [(validate.rules).string.min_bytes = 1]; | ||
} | ||
|
||
message HttpFilter { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,4 +6,5 @@ HTTP route management | |
:maxdepth: 2 | ||
|
||
../api/v2/rds.proto | ||
../api/v2/srds.proto | ||
../api/v2/route/route.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AndresGuedez for this, my main thought looking at this is that the PR is too big to reasonably review and without diluting review bandwidth. Can we split the PR up further for review?
I think we should have this master PR kept open to show how things tie together, but then separate PRs for:
I think we can get better review velocity if we do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Working on splitting this up.