Skip to content

Added VHDS protobuf message and updated RouteConfig to include it. #6418

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.

Sign up for GitHub

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

Merged
htuch merged 1 commit into from
Apr 3, 2019
Merged

Added VHDS protobuf message and updated RouteConfig to include it. #6418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/XDS_PROTOCOL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ correspondence between an xDS API and a resource type. That is:

* [LDS: `envoy.api.v2.Listener`](envoy/api/v2/lds.proto)
* [RDS: `envoy.api.v2.RouteConfiguration`](envoy/api/v2/rds.proto)
* [VHDS: `envoy.api.v2.Vhds`](envoy/api/v2/rds.proto)
* [CDS: `envoy.api.v2.Cluster`](envoy/api/v2/cds.proto)
* [EDS: `envoy.api.v2.ClusterLoadAssignment`](envoy/api/v2/eds.proto)
* [SDS: `envoy.api.v2.Auth.Secret`](envoy/api/v2/auth/cert.proto)
Expand Down Expand Up @@ -245,7 +246,8 @@ In general, to avoid traffic drop, sequencing of updates should follow a
* CDS updates (if any) must always be pushed first.
* EDS updates (if any) must arrive after CDS updates for the respective clusters.
* LDS updates must arrive after corresponding CDS/EDS updates.
* RDS updates related to the newly added listeners must arrive in the end.
* RDS updates related to the newly added listeners must arrive after CDS/EDS/LDS updates.
* VHDS updates (if any) related to the newly added RouteConfigurations must arrive after RDS updates.
* Stale CDS clusters and related EDS endpoints (ones no longer being
referenced) can then be removed.

Expand Down
2 changes: 2 additions & 0 deletions api/envoy/api/v2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ api_proto_library_internal(
deps = [
":discovery",
"//envoy/api/v2/core:base",
"//envoy/api/v2/core:config_source",
"//envoy/api/v2/route",
],
)
Expand All @@ -139,6 +140,7 @@ api_go_grpc_library(
deps = [
":discovery_go_proto",
"//envoy/api/v2/core:base_go_proto",
"//envoy/api/v2/core:config_source_go_proto",
"//envoy/api/v2/route:route_go_proto",
],
)
35 changes: 34 additions & 1 deletion api/envoy/api/v2/rds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ option java_package = "io.envoyproxy.envoy.api.v2";
option java_generic_services = true;

import "envoy/api/v2/core/base.proto";
import "envoy/api/v2/core/config_source.proto";
Copy link
Member

@htuch htuch Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix the Bazel target deps to include config_source to fix the build.

Copy link
Contributor Author

@dmitri-d dmitri-d Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

import "envoy/api/v2/discovery.proto";
import "envoy/api/v2/route/route.proto";

Expand Down Expand Up @@ -44,7 +45,23 @@ service RouteDiscoveryService {
}
}

// [#comment:next free field: 9]
// Virtual Host Discovery Service (VHDS) is used to dynamically update the list of virtual hosts for
// a given RouteConfiguration. If VHDS is configured a virtual host list update will be triggerred
// during the processing of an HTTP request if a route for the request cannot be resolved. The
Copy link
Member

@mattklein123 mattklein123 Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually true? Do we plan on pausing request processing and making a VHDS request? IMO this should probably be done as a separate filter if we want behavior. cc @htuch @dmitri-d

Copy link
Member

@htuch htuch Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 how would this work as an HTTP filter? Don't we need an initial route/cluster pick before we start down the HTTP filter chain? I think we were thinking of augmenting HCM to handle this.

Copy link
Member

@mattklein123 mattklein123 Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we still iterate all filters even if there is no route, so we can have an optional filter which pauses and fetches a new VHDS entry if users want that functionality vs. just standard eventually consistent VHDS. I would argue this is a cleaner design.

Copy link
Member

@htuch htuch Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 yeah, it is arguably much cleaner. What to do about things that are done by HCM in decodeHeaders before filter execution though, e.g.

envoy/source/common/http/conn_manager_impl.cc

Line 701 in 1cf59d4

if (cached_route_.value()) {
? Ideally, VHDS should be as expressive and capable as regular RouteConfiguration.

Copy link
Member

@mattklein123 mattklein123 Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have all the answers right now, but I think this code should be moved into the route refresh code anyway, since arguably the timeout should be changed if the route changes. This would then "just work" on the on-demand VHDS filter. I'm pretty sure we can make this work cleanly as a filter.

Copy link
Member

@htuch htuch Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. So, there will need to be some HCM changes, but we'll push most of the logic into a filter. @dmitri-d WDYT?

Copy link
Contributor Author

@dmitri-d dmitri-d Apr 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty close to the WIP implementation (see https://github.com/envoyproxy/envoy/pull/6406/files#diff-be24bc0fe90bba5c4e871fb56928e085R12). An (the main?) issue with moving code into refreshCachedRoute is probably going to be here:

envoy/source/common/http/conn_manager_impl.cc

Line 686 in 1cf59d4

const bool upgrade_rejected = createFilterChain() == false;
. It relies on a routing table being present to build a filter chain. I need to think what can be done about that.

Copy link
Member

@mattklein123 mattklein123 Apr 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @alyssawilk for comment. I think we might be able to handle this via an internal redirect from the filter if we can't figure out a more elegant solution during route cache reload.

// :ref:`resource_names_subscribe <envoy_api_msg_DeltaDiscoveryRequest.resource_names_subscribe>`
// field contains a list of virtual host names or aliases to track. The contents of an alias would
// be the contents of a *host* or *authority* header used to make an http request. An xDS server
// will match an alias to a virtual host based on the content of :ref:`domains'
// <envoy_api_msg_route.VirtualHost.domains>` field. The *resource_names_unsubscribe* field contains
// a list of virtual host names that have been `unsubscribed
// <https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md#unsubscribing-from-resources>`_
// from the routing table associated with the RouteConfiguration.
service VirtualHostDiscoveryService {
rpc DeltaVirtualHosts(stream DeltaDiscoveryRequest) returns (stream DeltaDiscoveryResponse) {
}
}

// [#comment:next free field: 10]
message RouteConfiguration {
// The name of the route configuration. For example, it might match
// :ref:`route_config_name
Expand All @@ -55,6 +72,15 @@ message RouteConfiguration {
// An array of virtual hosts that make up the route table.
repeated route.VirtualHost virtual_hosts = 2 [(gogoproto.nullable) = false];

// An array of virtual hosts will be dynamically loaded via the VHDS API.
// Both *virtual_hosts* and *vhds* fields will be used when present. *virtual_hosts* can be used
// for a base routing table or for infrequently changing virtual hosts. *vhds* is used for
// on-demand discovery of virtual hosts. The contents of these two fields will be merged to
// generate a routing table for a given RouteConfiguration, with *vhds* derived configuration
// taking precedence.
// [#not-implemented-hide:]
Copy link
Member

@htuch htuch Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe the intended final merge semantics?

Copy link
Contributor Author

@dmitri-d dmitri-d Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Member

@htuch htuch Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smallest nit in the world: the last sentence is missing a period at the end.

Vhds vhds = 9;

// Optionally specifies a list of HTTP headers that the connection manager
// will consider to be internal only. If they are found on external requests they will be cleaned
// prior to filter invocation. See :ref:`config_http_conn_man_headers_x-envoy-internal` for more
Expand Down Expand Up @@ -102,3 +128,10 @@ message RouteConfiguration {
// using CDS with a static route table).
google.protobuf.BoolValue validate_clusters = 7;
}

// [#not-implemented-hide:]
message Vhds {
Copy link
Member

@htuch htuch Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message should be // [#not-implemented-hide:]; it's visible at the CI generated docs https://192807-65214191-gh.circle-artifacts.com/0/source/generated/docs/api-v2/api/v2/rds.proto.html

Copy link
Contributor Author

@dmitri-d dmitri-d Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// Configuration source specifier for VHDS.
envoy.api.v2.core.ConfigSource config_source = 1
[(validate.rules).message.required = true, (gogoproto.nullable) = false];
}
1 change: 1 addition & 0 deletions tools/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ UTF
UUID
UUIDs
VH
VHDS
VLOG
WKT
WRR
Expand Down