Use on-demand cluster discovery in on-demand extension #20065

krnowak · 2022-02-21T16:34:12Z

Commit Message:
This adds an odcds_config field to the extension's config, and also allows the extension to be configured per-route. As it stands, it currently works only with routes using cluster-header config.

Additional Description:
Risk Level:
Medium, extending one extension in an opt-in way.

Testing:
Added unit tests and integration tests.

Docs Changes:
Not sure.

Release Notes:
Extended on-demand extension with on-demand cluster discovery functionality.

Platform Specific Features:
None.

[Optional Runtime guard:]
Fixes #2500
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

CC: @jamesmulcahy

Signed-off-by: Krzesimir Nowak <[email protected]>

repokitteh-read-only · 2022-02-21T16:34:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20065 was opened by krnowak.

see: more, trace.

Signed-off-by: Wanli Li <[email protected]>

adisuissa

Thanks for working on this, very exciting!
Took a first pass on the code, and left a few comments.

adisuissa · 2022-02-25T14:19:47Z

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto

+  // service. If not specified, the on-demand cluster discovery will
+  // be disabled. When it's specified, the filter will pause a request


Do you think that this field should be required?
IMO if a config update contains the OnDemand extension, then it is meant to be used (and enabled).
Unless if there are scenarios where a dummy placeholder is wanted.

I think that before the on-demand plugin was only talking to RDS if there wasn't any matching route. Not sure if we want to break this by requiring the odcds_config to be defined.

adisuissa · 2022-02-25T14:19:58Z

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto

+  // service. If not specified, the on-demand cluster discovery will
+  // be disabled. When it's specified, the filter will pause a request


Same as above

source/extensions/filters/http/on_demand/BUILD

adisuissa · 2022-02-25T14:34:31Z

source/extensions/filters/http/on_demand/on_demand_update.cc

+Upstream::OdCdsApiHandlePtr createOdCdsApi(const ProtoConfig& proto_config,
+                                           Upstream::ClusterManager& cm,
+                                           ProtobufMessage::ValidationVisitor& validation_visitor) {
+  if (!proto_config.has_odcds_config() && proto_config.resources_locator().empty()) {


IIRC the config-source is required no matter if resources-locator is set or not.
@htuch to correct me if I'm wrong.

Sorry, my confusion here. I was reading this as resource-name instead of locator.

source/extensions/filters/http/on_demand/on_demand_update.cc

adisuissa · 2022-02-25T14:45:00Z

source/extensions/filters/http/on_demand/on_demand_update.cc

+  filter_iteration_state_ = Http::FilterHeadersStatus::StopIteration;
+  auto callback = std::make_unique<Upstream::ClusterDiscoveryCallback>(
+      [this](Upstream::ClusterDiscoveryStatus cluster_status) {
+        onClusterDiscoveryCompletion(cluster_status);


Could there be a case where the filter is destroyed before the callback is invoked?
What I'm not certain about is whether we'll get an invocation of onClusterDiscoveryCompletion on a destroyed filter.

If the filter is destroyed, then cluster_discovery_handle_ is going to be destroyed too, which will unregister and destroy the callback.

source/extensions/filters/http/on_demand/on_demand_update.h

rojkov · 2022-03-01T07:48:01Z

/wait

Signed-off-by: Krzesimir Nowak <[email protected]>

rojkov · 2022-03-04T08:39:17Z

/wait
on coverage

Signed-off-by: Krzesimir Nowak <[email protected]>

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto

jamesmulcahy · 2022-03-10T22:39:51Z

@adisuissa could you please take a look when you get a moment? Thanks!

adisuissa

Overall LGTM.
/assign @htuch

test/integration/odcds_integration_test.cc

htuch

LGTM modulo remaining comments.
/wait

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto

source/extensions/filters/http/on_demand/on_demand_update.cc

test/integration/odcds_integration_test.cc

Signed-off-by: Krzesimir Nowak <[email protected]>

krnowak · 2022-04-29T07:20:57Z

/retest

repokitteh-read-only · 2022-04-29T07:21:01Z

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

🐱

Caused by: a #20065 (comment) was created by @krnowak.

see: more, trace.

krnowak · 2022-04-29T14:53:04Z

CI is green, so it's ready for review.

jamesmulcahy · 2022-04-29T15:13:01Z

Thanks again, @krnowak -- fantastic work, I really appreciate it.

Signed-off-by: Krzesimir Nowak <[email protected]>

krnowak · 2022-04-29T18:06:34Z

@jamesmulcahy: Thank you for the kind words. :)

I updated the PR again, this time no code changes (besides another merge from the main branch), but the CI needs to run again. I found out that there are some extra docs for on_demand extension, so I tried my hand at updating them a bit. I'd be grateful for a review of the wording. Thanks!

htuch

LGTM with two nits (look forward to getting this mergred)

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto

docs/root/configuration/http/http_filters/on_demand_updates_filter.rst

Signed-off-by: Krzesimir Nowak <[email protected]>

htuch

LGTM, thanks! Huge win for xDS APIs, thank you so much @krnowak for your contributions here and patience through the review process.

) This adds an odcds_config field to the extension's config, and also allows the extension to be configured per-route. As it stands, it currently works only with routes using cluster-header config. Risk Level: Medium, extending one extension in an opt-in way. Testing: Added unit tests and integration tests. Fixes envoyproxy#2500 Signed-off-by: Krzesimir Nowak <[email protected]>

krnowak and others added 4 commits February 21, 2022 17:24

on_demand: Plug the ODCDS functionality to the extension

f20b284

Signed-off-by: Krzesimir Nowak <[email protected]>

test: unit tests for on_demand extension

9132529

Signed-off-by: Krzesimir Nowak <[email protected]>

test: integration tests for ODCDS + on_demand

2a38576

Signed-off-by: Krzesimir Nowak <[email protected]>

test: Add integration test for ODCDS + on_demand + ADS

2375a85

Signed-off-by: Krzesimir Nowak <[email protected]>

krnowak requested a review from htuch as a code owner February 21, 2022 16:34

repokitteh-read-only bot added the api label Feb 21, 2022

repokitteh-read-only bot assigned lizan Feb 21, 2022

wanlill mentioned this pull request Feb 23, 2022

fix odcds test kinvolk/envoy#86

Closed

lambdai mentioned this pull request Feb 23, 2022

on demand cluster api for tcp_proxy filter #20091

Closed

adisuissa self-requested a review February 23, 2022 22:08

wanlill and others added 2 commits February 25, 2022 08:05

fix test

0da5138

Signed-off-by: Wanli Li <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

98dfd08

adisuissa reviewed Feb 25, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 1, 2022

krnowak added 2 commits March 1, 2022 15:28

make on-demand extension visible for tests

e919752

Signed-off-by: Krzesimir Nowak <[email protected]>

document the on-demand filter methods

136cdf4

Signed-off-by: Krzesimir Nowak <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 1, 2022

repokitteh-read-only bot added the waiting label Mar 4, 2022

adisuissa self-assigned this Mar 5, 2022

krnowak added 2 commits March 9, 2022 18:23

improve coverage

11c5bfe

Signed-off-by: Krzesimir Nowak <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

80bb090

repokitteh-read-only bot removed the waiting label Mar 9, 2022

lambdai reviewed Mar 9, 2022

View reviewed changes

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto Show resolved Hide resolved

adisuissa reviewed Mar 11, 2022

View reviewed changes

test/integration/odcds_integration_test.cc Outdated Show resolved Hide resolved

htuch suggested changes Mar 11, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 11, 2022

krnowak added 8 commits April 28, 2022 16:34

Add integration tests that uses VHDS and ODCDS at the same time

f72df76

Signed-off-by: Krzesimir Nowak <[email protected]>

Do not hardcode fake upstream indices

b83710a

Signed-off-by: Krzesimir Nowak <[email protected]>

Add failure cases

7db53eb

Signed-off-by: Krzesimir Nowak <[email protected]>

Refactor the tests further

d241316

Signed-off-by: Krzesimir Nowak <[email protected]>

Further refactor

c77fe43

Signed-off-by: Krzesimir Nowak <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

61075db

Fix issues raised by clang tidy

82d387e

Signed-off-by: Krzesimir Nowak <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

adc6358

krnowak added 2 commits April 29, 2022 19:59

Add some more documentation about on demand CDS

7fbc0a8

Signed-off-by: Krzesimir Nowak <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

9bf3b74

htuch reviewed May 2, 2022

View reviewed changes

api/envoy/extensions/filters/http/on_demand/v3/on_demand.proto Outdated Show resolved Hide resolved

docs/root/configuration/http/http_filters/on_demand_updates_filter.rst Outdated Show resolved Hide resolved

krnowak added 5 commits May 2, 2022 16:09

Factor out the on-demand CDS stuff into a separate message

d9bcb35

Signed-off-by: Krzesimir Nowak <[email protected]>

Update docs

71c88a2

Signed-off-by: Krzesimir Nowak <[email protected]>

Formatting

22cf688

Signed-off-by: Krzesimir Nowak <[email protected]>

Build fixes

81f1832

Signed-off-by: Krzesimir Nowak <[email protected]>

Merge remote-tracking branch 'origin/main' into krnowak/odcds

e382581

repokitteh-read-only bot added the api label May 2, 2022

krnowak added 3 commits May 2, 2022 18:49

Fix typos

745fea0

Signed-off-by: Krzesimir Nowak <[email protected]>

Drop the not-implemented-hide annotations

0c6a12d

Signed-off-by: Krzesimir Nowak <[email protected]>

Make source mandatory if OnDemandCds is used

0676eaf

Signed-off-by: Krzesimir Nowak <[email protected]>

htuch approved these changes May 3, 2022

View reviewed changes

repokitteh-read-only bot removed the api label May 3, 2022

htuch merged commit a41b254 into envoyproxy:main May 3, 2022

krnowak deleted the krnowak/odcds branch May 3, 2022 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use on-demand cluster discovery in on-demand extension #20065

Use on-demand cluster discovery in on-demand extension #20065

krnowak commented Feb 21, 2022 •

edited

Loading

repokitteh-read-only bot commented Feb 21, 2022

adisuissa left a comment

adisuissa Feb 25, 2022

krnowak Feb 25, 2022

adisuissa Feb 25, 2022

adisuissa Feb 25, 2022

adisuissa Feb 25, 2022

adisuissa Feb 25, 2022

krnowak Feb 25, 2022

rojkov commented Mar 1, 2022

rojkov commented Mar 4, 2022

jamesmulcahy commented Mar 10, 2022

adisuissa left a comment

htuch left a comment

krnowak commented Apr 29, 2022

repokitteh-read-only bot commented Apr 29, 2022

krnowak commented Apr 29, 2022

jamesmulcahy commented Apr 29, 2022

krnowak commented Apr 29, 2022

htuch left a comment

htuch left a comment

		// service. If not specified, the on-demand cluster discovery will
		// be disabled. When it's specified, the filter will pause a request

Use on-demand cluster discovery in on-demand extension #20065

Use on-demand cluster discovery in on-demand extension #20065

Conversation

krnowak commented Feb 21, 2022 • edited Loading

repokitteh-read-only bot commented Feb 21, 2022

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa Feb 25, 2022

Choose a reason for hiding this comment

krnowak Feb 25, 2022

Choose a reason for hiding this comment

adisuissa Feb 25, 2022

Choose a reason for hiding this comment

adisuissa Feb 25, 2022

Choose a reason for hiding this comment

adisuissa Feb 25, 2022

Choose a reason for hiding this comment

adisuissa Feb 25, 2022

Choose a reason for hiding this comment

krnowak Feb 25, 2022

Choose a reason for hiding this comment

rojkov commented Mar 1, 2022

rojkov commented Mar 4, 2022

jamesmulcahy commented Mar 10, 2022

adisuissa left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

krnowak commented Apr 29, 2022

repokitteh-read-only bot commented Apr 29, 2022

krnowak commented Apr 29, 2022

jamesmulcahy commented Apr 29, 2022

krnowak commented Apr 29, 2022

htuch left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

krnowak commented Feb 21, 2022 •

edited

Loading