On demand loading of ScopedRouteConfiguration by chaoqin-li1123 · Pull Request #12640 · envoyproxy/envoy

chaoqin-li1123 · 2020-08-14T00:04:37Z

Commit Message:
Add a field to the current protobuf of ScopedRouteConfiguration to enable on demand scoped route table loading. The on demand scope route tables will be loaded lazily. The lazy loading feature of route table associated with scope is achieved by extending the current vhds on_demand filter to support lazy loading of RouteConfigurationscoped route discovery service.If a scoped route configuration is set to be loaded lazily, upon a http request using SRDS, when the corresponding route table of a scope is not found, post a callback to control plane, request the route table from the management server, after the route table has been initialized, continue the filter chain.
Additional Description: Design doc
Risk Level:
Testing: add unit tests and integration test to verifiy behavior changes
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue] on demand s/rds
[Optional Deprecated:]

Signed-off-by: chaoqinli <chaoqinli@google.com>

…y_rds

Signed-off-by: chaoqinli <chaoqinli@google.com>

…t_migration

Signed-off-by: chaoqinli <chaoqinli@google.com>

repokitteh-read-only · 2020-08-14T00:04:43Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12640 was opened by chaoqin-li1123.

see: more, trace.

Signed-off-by: chaoqinli <chaoqinli@google.com>

stevenzzzz · 2020-08-14T19:51:31Z

/cc stevenzzzz

…y_rds

Signed-off-by: chaoqinli <chaoqinli@google.com>

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

htuch · 2020-08-31T13:21:09Z

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

+data if RouteConfiguration is specified to be loaded on demand in the :ref:`Scoped RouteConfiguration <envoy_v3_api_msg_config.route.v3.ScopedRouteConfiguration>`. The
+contents of the http header is used to find the scope and create the on-demand request. 
+
+On-demand VHDS and on-demand S/RDS can not be used at the same time at this point.


Do you have a tracking issue yet for converging the two?

I guess not.

Recommend adding one.

htuch · 2020-08-31T13:23:43Z

include/envoy/router/scopes.h

+  bool operator==(const ScopeKey& other) const;
+
+private:
+  // Update the key's hash with the new fragment hash.


Arguably we should split this into interface/implementation, since there is a lot of low-level implementation logic here. We have precedence for doing it this way in some cases, e.g. header map, but strongly prefer to not put a ton of implementation logic (e.g. the hash function below) inside include/.

source/common/http/conn_manager_impl.cc

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

test/integration/scoped_rds_integration_test.cc

source/common/router/scoped_rds.cc

Signed-off-by: chaoqinli <chaoqinli@google.com>

…y_rds

stevenzzzz

almost there!

test/common/router/scoped_rds_test.cc

stevenzzzz · 2020-09-02T19:24:43Z

test/common/router/scoped_rds_test.cc

-  std::function<void(bool)> route_config_updated_cb = [](bool) {};
+  // Scope no longer exists after srds update.
+  std::function<void(bool)> route_config_updated_cb = [](bool scope_exist) {
+    EXPECT_FALSE(scope_exist);


ditto here.

docs/root/intro/arch_overview/http/http_routing.rst

source/common/http/conn_manager_impl.cc

test/integration/scoped_rds_integration_test.cc

source/common/router/scoped_rds.cc

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch

Agree with @stevenzzzz, this looks great and close to ready to ship.
/wait

htuch · 2020-09-03T19:41:41Z

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

+data if RouteConfiguration is specified to be loaded on demand in the :ref:`Scoped RouteConfiguration <envoy_v3_api_msg_config.route.v3.ScopedRouteConfiguration>`. The
+contents of the http header is used to find the scope and create the on-demand request. 
+
+On-demand VHDS and on-demand S/RDS can not be used at the same time at this point.


Recommend adding one.

htuch · 2020-09-03T19:48:58Z

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

@@ -1,15 +1,21 @@
 .. _config_http_filters_on_demand:

-On-demand VHDS Updates


We also need an entry in https://github.com/envoyproxy/envoy/blob/master/docs/root/version_history/current.rst

source/common/http/conn_manager_impl.cc

test/common/router/scoped_rds_test.cc

htuch · 2020-09-03T20:07:04Z

test/integration/scoped_rds_integration_test.cc

+  test_server_->waitForCounterGe("http.config_test.rds.foo_route1.update_attempt", 1);
+  // Close the connection and destroy the active stream.
+  cleanupUpstreamAndDownstream();
+  // Push rds update, on demand updated callback is post to worker thread.


Here's another situation I'm curious about. If I'm an untrusted client and I keep making requests that trigger on-demand behavior, then immediately shutdown the stream, and maybe the upstream is slow to respond, do we keep accumulate on-demand update callbacks on the main thread indefinitely?

The callbacks will be cleared after the RouteConfiguration is fetched. I guess that wouldn't cause a problem if the management server is alive?

True, I wonder if there is a possible combined attack where you take out the availability of the management server, and can then OOM the Envoy by unbounded growth of these per-stream resources that hang around when the stream is gone. Maybe a bit far fetched, it probably takes millions of requests to OOM given how tiny the closure is.

Signed-off-by: chaoqinli <chaoqinli@google.com>

…y_rds

Signed-off-by: chaoqinli <chaoqinli@google.com>

source/common/http/conn_manager_impl.cc

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch · 2020-09-08T18:30:10Z

LGTM modulo coverage validation. @stevenzzzz any additional comments?

stevenzzzz · 2020-09-08T19:35:00Z

LGTM modulo coverage validation. @stevenzzzz any additional comments?

I have been talking to Chaoqin offline regularly, LGTM +1 modulo unresolved replies around clearer comments.

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch

LGTM, thanks!

chaoqinli added 16 commits August 7, 2020 15:17

merge

ef08270

Signed-off-by: chaoqinli <chaoqinli@google.com>

add test and test migration to v3

c3d39db

Signed-off-by: chaoqinli <chaoqinli@google.com>

add test and test migration to v3

7f93c95

Signed-off-by: chaoqinli <chaoqinli@google.com>

add unit test

88704aa

Signed-off-by: chaoqinli <chaoqinli@google.com>

add unit test for dangling reference

71d9526

Signed-off-by: chaoqinli <chaoqinli@google.com>

add unit test for dangling reference

1fb167f

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

3cb0a1c

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into laz…

399ad9b

…y_rds

clean up

2e5eccc

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

477dc6b

Signed-off-by: chaoqinli <chaoqinli@google.com>

test migration

abd354b

Signed-off-by: chaoqinli <chaoqinli@google.com>

...

25f487d

Signed-off-by: chaoqinli <chaoqinli@google.com>

finish

ab60437

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into tes…

ae188bf

…t_migration

clean up

7353474

Signed-off-by: chaoqinli <chaoqinli@google.com>

make control flag bool

3ce3677

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 requested review from htuch and lambdai as code owners August 14, 2020 00:04

repokitteh-read-only bot added the api label Aug 14, 2020

chaoqin-li1123 changed the title ~~Lazy rds~~ on demand loading of scoped rds Aug 14, 2020

chaoqin-li1123 changed the title ~~on demand loading of scoped rds~~ On demand loading of ScopedRouteConfiguration Aug 14, 2020

chaoqinli added 4 commits August 14, 2020 02:15

fix spelling

d54fc36

Signed-off-by: chaoqinli <chaoqinli@google.com>

fix spelling

6588bf2

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

6cca147

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

f05c622

Signed-off-by: chaoqinli <chaoqinli@google.com>

jmarantz assigned htuch Aug 14, 2020

clean up

9626719

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into laz…

3f23c10

…y_rds

clean up

d9affce

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch reviewed Aug 31, 2020

View reviewed changes

chaoqinli added 3 commits September 1, 2020 23:49

add test

896f52a

Signed-off-by: chaoqinli <chaoqinli@google.com>

test

826ef9d

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into laz…

e1e1e74

…y_rds

stevenzzzz reviewed Sep 2, 2020

View reviewed changes

chaoqinli added 2 commits September 2, 2020 20:32

clean up

44cb868

Signed-off-by: chaoqinli <chaoqinli@google.com>

resolve conflict

d8c3552

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch suggested changes Sep 3, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 3, 2020

less verbose

62cb93f

Signed-off-by: chaoqinli <chaoqinli@google.com>

repokitteh-read-only bot removed the waiting label Sep 7, 2020

chaoqinli added 7 commits September 7, 2020 04:52

less verbose

6d3f7f0

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

d26f173

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

6a46ebb

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into laz…

d78d733

…y_rds

clean up

70ec602

Signed-off-by: chaoqinli <chaoqinli@google.com>

clean up

d60da44

Signed-off-by: chaoqinli <chaoqinli@google.com>

fix missing test coverage

ae7f157

Signed-off-by: chaoqinli <chaoqinli@google.com>

stevenzzzz reviewed Sep 8, 2020

View reviewed changes

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved

add comment

1da611e

Signed-off-by: chaoqinli <chaoqinli@google.com>

add comment

f867e53

Signed-off-by: chaoqinli <chaoqinli@google.com>

htuch approved these changes Sep 8, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 8, 2020

htuch merged commit 709d1c3 into envoyproxy:master Sep 9, 2020

chaoqin-li1123 deleted the lazy_rds branch February 23, 2021 21:18

adisuissa mentioned this pull request Aug 17, 2023

Questions about xDS API flow #29043

Closed

		@@ -1,15 +1,21 @@
		.. _config_http_filters_on_demand:

		On-demand VHDS Updates

Conversation

chaoqin-li1123 commented Aug 14, 2020

Uh oh!

repokitteh-read-only bot commented Aug 14, 2020

Uh oh!

stevenzzzz commented Aug 14, 2020

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

htuch commented Sep 8, 2020

Uh oh!

stevenzzzz commented Sep 8, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants