Added VHDS protobuf message and updated RouteConfig to include it. by dmitri-d · Pull Request #6418 · envoyproxy/envoy

dmitri-d · 2019-03-28T20:12:32Z

Signed-off-by: Dmitri Dolguikh ddolguik@redhat.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level: low
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

dmitri-d · 2019-03-28T20:14:25Z

@htuch: This is VHDS protocol related changes. I'm thinking that DeltaRequest/Response nonce use warrants a dedicated PR, when we get there?

dmitri-d · 2019-03-28T20:17:53Z

Hmm. I don't think CI likes "not-implemented:hide" here: https://github.com/envoyproxy/envoy/pull/6418/files#diff-1a4a1f5cf0eae786da45b64903e8dd21R66. Complains that import here: https://github.com/envoyproxy/envoy/pull/6418/files#diff-1a4a1f5cf0eae786da45b64903e8dd21R12 is not being used.

I suppose I can omit it?

dmitri-d · 2019-03-28T21:32:45Z

reverted [(gogoproto.nullable) = false] on RouteConfiguration.virtual_hosts, as vhds field is marked as #not-implemented-hide:

htuch

Great, this is nice self-contained PR.

/wait

api/envoy/api/v2/rds.proto

htuch · 2019-03-29T15:33:53Z

api/envoy/api/v2/rds.proto

Can you describe the intended final merge semantics?

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

htuch · 2019-03-29T15:34:53Z

api/envoy/api/v2/rds.proto

Only one protodoc-title per .proto :)

htuch · 2019-03-29T15:35:57Z

api/envoy/api/v2/rds.proto

Can you provide some discussion comparable to that for RouteDiscoveryService. In particular, should cover what the resource type is embedded in Any and how this related to RDS and expected use cases.

htuch · 2019-03-29T15:37:37Z

api/envoy/api/v2/rds.proto

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

brian-avery · 2019-03-29T17:47:36Z

api/XDS_PROTOCOL.md

Is it better to say

RDS updates related to the newly added listeners must arrive after CDS/EDS/LDS updates
VHDS updates (if any) related to the newly added Route Configurations must arrive after RDS updates

That sounds better I think.

brian-avery · 2019-03-29T17:50:10Z

api/envoy/api/v2/rds.proto

I think this should be nullable. If VHDS is enabled, it makes sense for virtual host updates to come over VHDS while route configs (minus virtual hosts) come across RDS.

Should drop the semicoolon after virtual_hosts =2

Agree, removed that.

api/envoy/api/v2/rds.proto

brian-avery · 2019-03-29T18:34:15Z

LGTM

htuch

Looks good, just a bunch of nits around formatting and a few comments requests, thanks.

/wait

htuch · 2019-04-02T04:29:31Z

api/XDS_PROTOCOL.md

Nit: prefer RouteConfigurations

htuch · 2019-04-02T04:30:40Z

api/envoy/api/v2/rds.proto

Nit: RouteConfiguration

htuch · 2019-04-02T04:31:50Z

api/envoy/api/v2/rds.proto

This should reference https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md#unsubscribing-from-resources

Added the reference

htuch · 2019-04-02T04:32:26Z

api/envoy/api/v2/rds.proto

Nit: the preferred RST formatting for literals such as header name is something like *host*

htuch · 2019-04-02T04:32:53Z

api/envoy/api/v2/rds.proto

*resource_names_subscribe* (or do a :ref: link to the field name).

htuch · 2019-04-02T04:33:31Z

api/envoy/api/v2/rds.proto

I think we need 1-2 sentences explaining what a VHDS is and why you need one :)

Added a short explanation

htuch · 2019-04-02T04:34:11Z

api/envoy/api/v2/rds.proto

Nit * syntax or :ref: for these field names.

htuch · 2019-04-02T04:35:00Z

api/envoy/api/v2/rds.proto

, with *vhds* derived configuration taking precedence

htuch · 2019-04-02T04:36:12Z

Also, needs DCO/format fixes.

dmitri-d · 2019-04-02T20:59:00Z

also restored [(gogoproto.nullable) = false] on RouteConfiguration.virtual_hosts: an empty array should be used in favour of a null field.

dmitri-d · 2019-04-02T21:11:33Z

fixed formatting/spelling and DCO errors.

dmitri-d · 2019-04-02T21:16:16Z

@htuch: I'll squash the commits once you are happy with the PR, I thought keeping them as is might help during the review.

htuch

I think last few comments and we're good to go, thanks.
/wait

htuch · 2019-04-02T22:39:23Z

api/envoy/api/v2/rds.proto

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

htuch · 2019-04-02T22:41:35Z

api/envoy/api/v2/rds.proto

You can drop this line entirely actually, the entry above is actually file-wide.

htuch · 2019-04-02T22:44:13Z

api/envoy/api/v2/rds.proto

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

htuch · 2019-04-02T22:45:11Z

api/envoy/api/v2/rds.proto

"during the processing of an HTTP request if..."

htuch · 2019-04-02T22:46:27Z

api/envoy/api/v2/rds.proto

Nit: you can actually link directly to fields, grep for envoy_api_field_ to see how.

dmitri-d · 2019-04-02T23:59:04Z

fixes based on the feedback

htuch · 2019-04-03T02:39:42Z

@dmitri-d LGTM, can you fix format and I can merge? DCO seems to be green. Thanks.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2019-04-03T05:08:42Z

fixed formatting
squashed commits

htuch

Thanks!

dmitri-d · 2019-04-03T16:03:52Z

Thanks for the reviews, @htuch!

mattklein123 · 2019-04-04T03:30:48Z

api/envoy/api/v2/rds.proto

-// [#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


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

@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.

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.

@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.

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.

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?

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.

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.

* master: (137 commits) test: router upstream log to v2 config stubs (envoyproxy#6499) remove idle timeout validation (envoyproxy#6500) build: Change namespace of chromium_url. (envoyproxy#6506) coverage: exclude chromium_url (envoyproxy#6498) fix(tracing): allow 256 chars in path tag (envoyproxy#6492) Common: Introduce StopAllIteration filter status for decoding and encoding filters (envoyproxy#5954) build: update PGV url (envoyproxy#6495) subset lb: avoid partitioning host lists on worker threads (envoyproxy#6302) ci: Make envoy_select_quiche no-op. (envoyproxy#6393) watcher: notify when watched files are modified (envoyproxy#6215) stat: Add counterFromStatName(), gaugeFromStatName(), and histogramFromStatName() (envoyproxy#6475) bump to 1.11.0-dev (envoyproxy#6490) release: bump to 1.10.0 (envoyproxy#6489) hcm: path normalization. (#1) build: import manually minified Chrome URL lib. (envoyproxy#3) codec: reject embedded NUL in headers. (envoyproxy#2) Added veryfication if path contains query params and add them to path header (envoyproxy#6466) redis: basic integration test for redis_proxy (envoyproxy#6450) stats: report sample count as an integer to prevent loss of precision (envoyproxy#6274) Added VHDS protobuf message and updated RouteConfig to include it. (envoyproxy#6418) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

mattklein123 assigned htuch Mar 28, 2019

htuch mentioned this pull request Mar 29, 2019

VHDS: implementation of on-demand RDS updates #6406

Closed

htuch suggested changes Mar 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 29, 2019

brian-avery reviewed Mar 29, 2019

View reviewed changes

repokitteh-read-only bot removed the waiting label Mar 29, 2019

brian-avery reviewed Mar 29, 2019

View reviewed changes

api/envoy/api/v2/rds.proto Outdated Show resolved Hide resolved

htuch suggested changes Apr 2, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 2, 2019

repokitteh-read-only bot removed the waiting label Apr 2, 2019

htuch suggested changes Apr 2, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Apr 2, 2019

htuch added the waiting label Apr 3, 2019

Added VHDS protobuf message and updated RouteConfig to include it.

d79f551

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only bot removed the waiting label Apr 3, 2019

htuch approved these changes Apr 3, 2019

View reviewed changes

htuch merged commit a781218 into envoyproxy:master Apr 3, 2019

mattklein123 reviewed Apr 4, 2019

View reviewed changes

dmitri-d deleted the vhds-proto branch May 20, 2020 19:45

Conversation

dmitri-d commented Mar 28, 2019

Uh oh!

dmitri-d commented Mar 28, 2019

Uh oh!

dmitri-d commented Mar 28, 2019

Uh oh!

dmitri-d commented Mar 28, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

brian-avery commented Mar 29, 2019

Uh oh!

htuch 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!

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!

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!

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