server: add server owned http route manager by junr03 · Pull Request #1345 · envoyproxy/envoy

junr03 · 2017-07-27T22:52:32Z

This PR adds a server owned HttpRouteManager, which observes the server's RDSRouteConfigProviders. This allows for deduplication of RDSRouteConfigProviders of the same name via shared ownership, and allows the server to iterate over all RDSRouteConfigProviders.

junr03 · 2017-07-27T22:56:07Z

source/server/http_route_manager_impl.cc

+  std::vector<Router::RouteConfigProviderSharedPtr> ret;
+  ret.reserve(route_config_providers_.size());
+  for (const auto& element : route_config_providers_) {
+    ret.push_back(element.second.lock());


@mattklein123 is it possible here that between the time that the last shared_ptr for the provider gets reset, and the destructor for the provider is called (thus cleaning the entry in the manager), this iteration happens?

In other words, should I lock, check for nullptr, and then push into the vector only is the shared_ptr is not null?

The whole implementation will need to change a bit so let's touch on this later.

mattklein123

nice start, but I would suggest some fundamental changes before working on tests.

mattklein123 · 2017-07-28T17:42:00Z

source/server/http_route_manager_impl.cc

+  std::vector<Router::RouteConfigProviderSharedPtr> ret;
+  ret.reserve(route_config_providers_.size());
+  for (const auto& element : route_config_providers_) {
+    ret.push_back(element.second.lock());


The whole implementation will need to change a bit so let's touch on this later.

mattklein123 · 2017-07-28T17:43:47Z

include/envoy/server/filter_config.h


+  /**
+   * @return HttpRouteManager& singleton for use by the entire server.
+   */


At the factory context level, we should not expose the entire manager. Only the ability to get a route provider by name. See below for interface changes.

there is no reason to expose list() at the context level. Just have two interfaces, with one deriving from the other, and expose the first from the context.

makes sense

updated. I am not thrilled by the naming here. I'll think a bit more.

mattklein123 · 2017-07-28T17:47:47Z

include/envoy/server/http_route_manager.h

+   * @param routeConfigProvider is a weak_ptr to the RouteConfigProvider to add.
+   */
+  virtual void
+  addRouteConfigProvider(const std::string& name,


add/get/remove should be merged into a single API so that RAII can be used when the returned provider is destroyed. Basically, I would recommend doing the following:

Return a shared_ptr to a route provider (or interface handle to the route provider).

Internally inside the manager, store both a weak_ptr and a raw pointer. The raw pointer can be used to lookup the manager entry to remove when the last shared_ptr reference goes away. The weak_ptr can be used to create a new shared_ptr to return if needed (AFAIK there is no way to get the raw pointer out of a weak_ptr making this necessary).

This will allow you to have a single get() method exposed.

mattklein123 · 2017-08-01T23:25:12Z

source/common/router/rds_impl.h

@junr03 to make reviewing this easier, can you please split out the file move into a separate PR with no code changes. Then do the PR with the changes? It's really hard to tell what changed. Or just don't move things in this PR and clean it up in another PR. Thanks.

junr03 · 2017-08-02T08:17:40Z

@mattklein123 updated with the manager in the router namespace. Right now the manager is in the rds_impl file because I was getting tangled in the imports and forward declarations necessary to make the manager reside in a http_route_manager_impl file.

mattklein123 · 2017-08-02T14:31:41Z

include/envoy/router/http_route_manager.h

+   * @param init_manager supplies the init manager.
+   */
+  virtual RouteConfigProviderSharedPtr
+  getRouteConfigProvider(const Json::Object& config, Runtime::Loader& runtime,


Quick comment to work on (sorry really busy this week so I'm giving you drive by reviews :( ): From an interface perspective, I think almost all of these things don't need to be passed. They can be known internally to the route manager. I wouldn't pass any json either, and just unpack into exactly what you need which I think is just route table name and cluster name. (This will likely get converted to v2 but the conversion will be easy cc @htuch ).

So I think at quick glance you need to pass cluster name, route table name, init manager, scope, and maybe stat_prefix.

No worries, I should have seen that myself. After I update I will ping @htuch to see if he can give a review to load balance you a bit.

This is close enough. Please just add tests and get the PR ready to ship with full coverage and then we can do a full review.

junr03 · 2017-08-03T17:49:51Z

@mattklein123 @htuch tests are in. This is ready for review

htuch

Nice, this makes sense.

htuch · 2017-08-03T22:37:09Z

include/envoy/router/http_route_manager.h

+
+/**
+ * The HttpRouteManager exposes the ability to get a RouteConfigProvider. This interface is exposed
+ * to the Server's FactoryContext in order to allow HttpConnMans to get RouteConfigProviders.


Nit: HttpConnectionManagers, prefer not to abbreviate type names.

htuch · 2017-08-03T22:42:51Z

include/envoy/router/http_route_manager.h

+namespace Router {
+
+/**
+ * The HttpRouteManager exposes the ability to get a RouteConfigProvider. This interface is exposed


Nit: Prefer the more verbose RouteConfigProviderManager.

htuch · 2017-08-03T22:43:11Z

include/envoy/router/http_route_manager.h

+  virtual ~HttpRouteManager() {}
+
+  /**
+   * get a Router::RouteConfigProviderSharedPtr. Ownership of the RouteConfigProvider is shared by


Nit: s/get/Get.

htuch · 2017-08-03T22:43:56Z

include/envoy/router/http_route_manager.h

+   * get a Router::RouteConfigProviderSharedPtr. Ownership of the RouteConfigProvider is shared by
+   * all the ConnectionManagers who own a Router::RouteConfigProviderSharedPtr. The HttpRouteManager
+   * holds weak_ptrs to the RouteConfigProviders. Clean up of the weak ptrs happen from the
+   * destructor of the RouteConfigProvider. This function creates a RouteConfigProvider is there


Nit: s/is/if

htuch · 2017-08-03T22:45:43Z

include/envoy/router/http_route_manager.h

+   * @return std::vector<Router::RouteConfigProviderSharedPtr> a list of all the
+   * RouteConfigProviders currently loaded.
+   */
+  virtual std::vector<RouteConfigProviderSharedPtr> routeConfigProviders() PURE;


Is this only for test? Maybe add a ForTest suffix. Do we need to return a proper shared ptr here, or is a reference good enough?

this function will be used in a subsequent PR by a new admin handler. The handler will iterate
over all the configure RouteConfigProviders, and pretty print all the configured routes.

htuch · 2017-08-03T22:50:12Z

source/common/router/rds_impl.cc

+
+  // RdsRouteConfigProviders are unique based on their <route_config_name>_<cluster>.
+  std::string map_identifier = config.getString("route_config_name") + "_";
+  map_identifier += config.getString("cluster");


There's some duplicated logic in creating the map key with the erase in the destructor, probably best to refactor.

Is it possible that as a result of _ appearing in either cluster or route config name, we could get unintended clashes here? e.g. cluster "A_B" and route config name "C" would hit the same slot as cluster "A" and route config name "B_C".

Nit: prefer creating map_identifier in one expression and making it `const.

Good point. I guess that the correct way to do this is to have the map key be the tuple of the route_config_name and cluster. And it solves 1, and 3 as well.

Sure, or some other distinguished identifier (e.g. : can't appear in the cluster name.

@htuch concatenated with :. If we go this route then the route_config_name field of the RDS config also cannot contain :. I have documented that. But if I understand our breaking change policy I cannot enforce that until the 1.4.0 release. That means that when we release 1.4.0 I would make a PR enforcing the rule?

htuch · 2017-08-03T22:51:09Z

source/common/router/rds_impl.cc

+  std::vector<Router::RouteConfigProviderSharedPtr> ret;
+  ret.reserve(route_config_providers_.size());
+  for (const auto& element : route_config_providers_) {
+    ret.push_back(element.second.lock());


Do you want to include the failed locks here?

That is what I was discussing with @mattklein123 in my initial comment above. I wanted to understand if the threading model allowed for failed locks or not. I understand that the way to protect here is to lock, check the returned ptr is nullptr, and then push only if it isn't, just wanted to make sure it was necessary.

threading has nothing to do with it here. All of this is single threaded. I haven't looked at the code yet but assuming it's correct lock() should never return nullptr.

Right, that's what I thought.

Well, yeah, threading isn't relevant, lock is a misnomer. It's a way to turn weak into a real shared pointer. Assuming this is weak for a reason, it seems it should be possible for the lock to fail... or did I miss something here?

I think I wasn't clear with my comment. I understand what lock does, and that it has nothing to do with threading. I was originally concerned that in the envoy threading model it would be possible to get to a state where the RouteConfigObject were destroyed but the weak ptr was still present in the Manager. I now understand that is not possible.

@htuch The pointer is weak because we want don't want the manager to own the RouteConfigProvider, we only want it to be able to dole out share ptrs to the HttpConnectionManagers that create RouteConfigProviders.

htuch

LGTM modulo comments.

htuch · 2017-08-07T01:10:23Z

source/common/router/rds_impl.cc

+
+  // RdsRouteConfigProviders are unique based on their <route_config_name>_<cluster>.
+  std::string map_identifier = config.getString("route_config_name") + "_";
+  map_identifier += config.getString("cluster");


Sure, or some other distinguished identifier (e.g. : can't appear in the cluster name.

htuch · 2017-08-07T01:54:19Z

source/common/router/rds_impl.cc

+    return std::move(new_provider);
+  }
+
+  return it->second.lock();


Can this lock() fail? Based on other comment threads and the comment above about only cleaning up in RouteConfigProvider destructors, I'm thinking no. If that's the case, please add an ASSERT/comment to document.

If it can fail here, shouldn't we have similar handling to the "not found" case above and create a new provider?

htuch · 2017-08-07T01:57:43Z

source/common/router/rds_impl.cc

+  std::vector<Router::RouteConfigProviderSharedPtr> ret;
+  ret.reserve(route_config_providers_.size());
+  for (const auto& element : route_config_providers_) {
+    ret.push_back(element.second.lock());


OK, I think I get what's going on now. We're using a weak pointer simply as a means to allow handing out shared pointers later when we lock, otherwise we could be using plain * pointers. Seems legit. As below, I'd add an ASSERT here and comment to explain why this can never fail.

htuch · 2017-08-07T02:01:37Z

test/common/router/rds_impl_test.cc

+  RouteConfigProviderSharedPtr provider2 = route_config_provider_manager_.getRouteConfigProvider(
+      *config, cm_, store_, "foo_prefix", init_manager_);
+  // So this means that both shared_ptrs should be the same.
+  EXPECT_EQ(provider, provider2);


Have you considered using the shared_ptr use_count() method to validate the behaviors around reference counting?

mattklein123 · 2017-08-07T04:45:44Z

Please also merge master which will fix ASAN issue. I will take a look after you have addressed @htuch comments. Thanks.

junr03 · 2017-08-08T14:35:56Z

@mattklein123 updated from comments. Merged master.

htuch · 2017-08-08T14:42:13Z

docs/configuration/http_conn_man/rds.rst

  :ref:`RDS HTTP API <config_http_conn_man_rds_api>`. This allows an Envoy configuration with
  multiple HTTP listeners (and associated HTTP connection manager filters) to use different route
-  configurations.
+  configurations. The route_config_name must **not** contain ``:``.


Yeah, on second thoughts, maybe best to avoid baking in these dependencies. I know that we've been discussing trying to relax the : constraint on clusters at some point (but not now), so if there is a cleaner way, e.g. using tuples and hashing on that, then maybe do that.

Agreed. Don't think there is any explicit reason to block ':' ?

Thinking about this some more, I was being near sighted, and thought that it was possible to fall into the same collision situation that I was trying to escape. But that is not the case if one of the strings has the blocked character.

@htuch how serious are we thinking about relaxing the : constraint? I could leave a TODO to move the to pair, or struct key when that happens, but for now I'll leave it this way to avoid having to deal with hashing the key.

I don't really understand how ':' matters here at all. The key is internal. If you just add config name + cluster, what difference does it make what characters it contains?

Because if for example someone specifies route_config_name foo and cluster bar and someone else does route_config_name foob and cluster ar the concatenation would make the two providers collide when it shouldn't.

You probably see that. So I am guessing I am the one that doesn't see why this wouldn't matter? I guess it is pretty contrived that such a collision would happen, but I think protecting against it is better?

Then just do something like "hash_separator_you_need_to_be_really_insane_to_use_this" or whatever. (I haven't thought through it enough but seems like something like this should work fine.

The other option is to just do a custom hasher/comparer which operates on a struct or tuple of config/cluster.

mattklein123 · 2017-08-08T16:46:56Z

Meta comment. I think this should probably use the singleton manager that I am building in #1410. This should merge today. This will avoid a lot of boilerplate addition for something that not everyone needs (for people that don't use HTTP). Even for admin endpoint, the singleton can register a handler into admin. Not a huge deal if we are eager for this to merge, but I might at least put in a TODO to this effect. I will review the rest of this change as is later today.

mattklein123

look good other than some small nits and fixing test compile errors. Up to you if you want to do the singleton thing here or not.

mattklein123 · 2017-08-08T17:42:11Z

docs/configuration/http_conn_man/rds.rst

  :ref:`RDS HTTP API <config_http_conn_man_rds_api>`. This allows an Envoy configuration with
  multiple HTTP listeners (and associated HTTP connection manager filters) to use different route
-  configurations.
+  configurations. The route_config_name must **not** contain ``:``.


Agreed. Don't think there is any explicit reason to block ':' ?

mattklein123 · 2017-08-08T17:45:34Z

source/common/router/rds_impl.cc

+    // of this code, locking the weak_ptr will not fail.
+    Router::RouteConfigProviderSharedPtr provider = element.second.lock();
+    ASSERT(provider)
+    ret.push_back(std::move(provider));


del std::move here as this is a shared_ptr

mattklein123 · 2017-08-08T17:47:08Z

source/common/router/rds_impl.cc

+
+  // RdsRouteConfigProviders are unique based on their <route_config_name>_<cluster>.
+  const std::string manager_identifier =
+      config.getString("route_config_name") + ":" + config.getString("cluster");


The key is opaque. If you just remove ':' and then you can allow ':' in the name (you probably could allow ':' in the name anyway and it would work).

mattklein123 · 2017-08-08T17:48:10Z

source/common/router/rds_impl.cc

+    new_provider->registerInitTarget(init_manager);
+
+    route_config_providers_.insert(
+        {manager_identifier, std::weak_ptr<RdsRouteConfigProviderImpl>(new_provider)});


nit: I don't thinkstd::weak_ptr<RdsRouteConfigProviderImpl> is needed here as it should auto convert.

mattklein123 · 2017-08-08T17:48:32Z

source/common/router/rds_impl.cc

+  // of this code, locking the weak_ptr will not fail.
+  Router::RouteConfigProviderSharedPtr new_provider = it->second.lock();
+  ASSERT(new_provider);
+  return std::move(new_provider);


del std::move

@mattklein123 for my information. Here and above, doesn't moving the pointer save the atomic operations of copying the pointer on return (here) and insertion into the vector?

The compiler will optimize usually. It's not worth playing games like this IMO with shared pointers.

ok, got it.

junr03 · 2017-08-09T00:18:27Z

@mattklein123 for sake of finishing this PR, I am leaning into merging in the current state and doing a subsequent PR to use #1410.

htuch · 2017-08-09T14:05:13Z

source/common/router/rds_impl.cc

+  auto it = route_config_providers_.find(manager_identifier);
+  if (it == route_config_providers_.end()) {
+    std::shared_ptr<RdsRouteConfigProviderImpl> new_provider{
+        new RdsRouteConfigProviderImpl(config, manager_identifier, runtime_, cm, dispatcher_,


Nit: make_shared

mattklein123

Looks great. Few small nits remaining.

mattklein123 · 2017-08-09T15:17:28Z

test/common/router/rds_impl_test.cc


+class RouteConfigProviderManagerImplTest : public testing::Test {
+public:
+  RouteConfigProviderManagerImplTest() {}


nit: del empty constructor/destructor

mattklein123 · 2017-08-09T15:18:34Z

test/common/router/rds_impl_test.cc

+  configured_providers.clear();
+
+  // All shared_ptrs to the provider pointed at by provider1, and provider2 have been deleted, so
+  // now we should only have the provider porinted at by provider3.


typo "porinted"

junr03 · 2017-08-09T18:30:03Z

@mattklein123 updated.

mattklein123 · 2017-08-09T18:43:10Z

@junr03 still need to fix @htuch nit.

Description: Splits iOS CI workflows into test and build, and updates build job to use cached library artifact when run on the same SHA. Risk Level: Low Testing: CI Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

**Description** This fixes the broken table in #1257. Signed-off-by: Takeshi Yoneda <[email protected]>

junr03 commented Jul 27, 2017

View reviewed changes

mattklein123 reviewed Jul 28, 2017

View reviewed changes

mattklein123 reviewed Aug 1, 2017

View reviewed changes

junr03 force-pushed the http-route-manager branch from 7643549 to d53f2bb Compare August 2, 2017 00:54

junr03 changed the base branch from master to pre-http-route-manager August 2, 2017 00:55

junr03 force-pushed the http-route-manager branch from d714609 to 946964d Compare August 2, 2017 08:13

junr03 changed the base branch from pre-http-route-manager to master August 2, 2017 08:14

junr03 mentioned this pull request Aug 2, 2017

rds: moving RdsRouteConfigProvider to Server namespace #1370

Closed

mattklein123 reviewed Aug 2, 2017

View reviewed changes

Jose Nino added 4 commits August 3, 2017 13:09

rds: add server owned http route manager

402b35d

http route manager now owns some things

950a974

fix

f54d454

fix

2c925f2

junr03 force-pushed the http-route-manager branch from fb846b6 to 2c925f2 Compare August 3, 2017 20:19

htuch reviewed Aug 3, 2017

View reviewed changes

comments

cdbc006

htuch reviewed Aug 7, 2017

View reviewed changes

Jose Nino added 3 commits August 8, 2017 08:06

identifier

1a1581f

Merge branch 'master' into http-route-manager

4d15e12

comments

431a18e

htuch reviewed Aug 8, 2017

View reviewed changes

mattklein123 reviewed Aug 8, 2017

View reviewed changes

Jose Nino added 3 commits August 8, 2017 18:14

comments

3ca9675

comments

66d2d2c

todo

cc4d801

htuch previously approved these changes Aug 9, 2017

View reviewed changes

mattklein123 reviewed Aug 9, 2017

View reviewed changes

Jose Nino added 2 commits August 9, 2017 11:19

Merge branch 'master' into http-route-manager

735f5fe

nits

380c0c2

junr03 dismissed htuch’s stale review via 380c0c2 August 9, 2017 18:29

Jose Nino added 2 commits August 9, 2017 14:52

comment

2500d93

comment

5f60b01

mattklein123 approved these changes Aug 9, 2017

View reviewed changes

mattklein123 merged commit ef8f0e0 into master Aug 9, 2017

mattklein123 deleted the http-route-manager branch August 9, 2017 21:40

mathetake added a commit that referenced this pull request Mar 3, 2026

docs: fixes broken supported-endpoints table (#1345)

bdd9785

**Description** This fixes the broken table in #1257. Signed-off-by: Takeshi Yoneda <[email protected]>

Conversation

junr03 commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

junr03 commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

junr03 commented Aug 3, 2017

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!

junr03 Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

junr03 commented Jul 27, 2017 •

edited

Loading

junr03 commented Aug 2, 2017 •

edited

Loading

junr03 Aug 3, 2017 •

edited

Loading