Fixes #7617 - fixes handling creation of VHDS subscription after Init… #8650

dmitri-d · 2019-10-17T19:29:10Z

…Manager has completed initialization

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: Fixes #7617
Risk Level:
Testing: added an integration test
Docs Changes:
Release Notes:
[Optional Fixes #Issue] #7617
[Optional Deprecated:]

lambdai

Thank you for the fix!
Could you fix below and explain the cleanup block? Thanks!

lambdai · 2019-10-17T20:48:05Z

source/common/router/rds_impl.cc

+// Initialize a no-op InitManager in case the one in the factory_context has completed
+// initialization. This can happen if an RDS config update for an already established RDS
+// subscription contains VHDS configuration.
+void RdsRouteConfigSubscription::maybeCreateInitManager(


style: return unique_ptrInit::Manager. Maybe optionalInit::ManagerImpl

I'm populating both init_manager and init_vhds method parameters, I think returning just one of them as the result of the method call would be confusing...

source/common/router/rds_impl.cc

lambdai

Fat finger before. Update my opinion. Sorry!

zuercher · 2019-10-21T16:12:13Z

/wait

mattklein123

Requesting changes on this one also just to make sure that we have documentation before landing any more VHDS changes. Thank you for making Envoy's documentation first class! :)

dmitri-d · 2019-10-21T22:43:54Z

@mattklein123: This is just a bugfix, with no changes to existing VHDS functionality, not sure what docs you wanted to go with this PR.

mattklein123 · 2019-10-21T22:46:57Z

@mattklein123: This is just a bugfix, with no changes to existing VHDS functionality, not sure what docs you wanted to go with this PR.

There are no docs for VHDS currently. I'm using these PRs as a forcing function to get docs into the code base. We should have never gotten this far without docs.

dmitri-d · 2019-10-21T23:34:29Z

rebased, responded to feedback

dmitri-d · 2019-10-22T22:30:53Z

added a test that verifies creation of a noop init manager

dmitri-d · 2019-10-24T20:18:07Z

ping @lambdai

lambdai

Thank you! The new integration test case is awesome!

lambdai · 2019-10-31T15:30:27Z

Could you merge master to see if test passes?

dmitri-d · 2019-11-01T19:18:34Z

rebased and squashed the commits

zuercher · 2019-11-01T20:45:22Z

You have a use-after-free error because the std::unique_ptr<Cleanup> you construct has a reference to the std::unique_ptr<Init::ManagerImpl>. The init manager is being destroyed first in this test and when the cleanup runs it references freed memory.

Rather than just fixing the ordering in the test, I think it's probably better to figure out a way to remove or make explicit the destruction order dependency.

dmitri-d · 2019-11-04T21:32:39Z

@zuercher: the issue was that the route_config_provider created by route_config_provider_manager_->createRdsRouteConfigProvider call was going out of scope and taking the subscription with it.

dmitri-d · 2019-11-04T21:32:52Z

ping @lambdai

dmitri-d · 2019-11-07T19:28:23Z

rebased

ping @lambdai: I think this is ready to be merged now.

…Manager has completed initialization Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2019-11-12T19:55:22Z

rebased

ping @lambdai

lambdai

LGTM in terms of the core fix: The lifetime of CleanUp and Init::Manager is correct.

I don't understand the fuzz failure.

lambdai · 2019-11-14T17:53:56Z

CC @zuercher for more comments

zuercher

Couple relatively minor things.

@mattklein123 were you documentation concerns addressed?

zuercher · 2019-11-14T22:18:23Z

source/common/router/rds_impl.h

@@ -111,6 +115,9 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
    return route_config_providers_;
  }
  RouteConfigUpdatePtr& routeConfigUpdate() { return config_update_info_; }
+  void maybeCreateInitManager(const std::string& version_info,


Let's not leak this method into the public API of this class. Is it not possible to test its behavior in the context of onConfigUpdate?

Let's not leak this method into the public API of this class.

This is a concrete implementation class, not a public interface. I very strongly prefer testability over encapsulation in such a case.

Is it not possible to test its behavior in the context of onConfigUpdate?

I'm trying to test this in isolation; the setup is already difficult (and fragile) enough without dragging in another method.

Perhaps make it private and make the test class a friend so that it can access it?

That would result in a dependency to test code in production code, would it not?

I think the friend class statement constitutes a forward reference and needn't ever be declared, but I could be wrong.

zuercher · 2019-11-14T22:25:29Z

test/integration/vhds_integration_test.cc

@@ -72,6 +72,16 @@ const char Config[] = R"EOF(
                    cluster_name: xds_cluster
 )EOF";

+const char RdsWithoutVhdsConfig[] = R"EOF(


We prefer these configs to are created using the protobuf objects directly rather than parsing configs.

This is the first time I hear about this preference. This test suit been around for a while now, with this exact setup (and it's not the only test suite to use yaml for setup).

FWIW, configurations used in these tests have enough nested fields/protobufs that building them programmatically will be laborious and harder to read.

The ConfigHelper object underlying the integration test should be helpful here. I'm ok if you leave this as-is for now with a todo to circle back and refactor this test to use ConfigHelper. I think it's a matter of picking one of the default configs from test/utility/config.cc, adding your RDS/VHDS clusters with a ConfigModifier, and adding virtual hosts either with addVirtualHost or createVirtualHost.

I disagree re: adding vhds to one of default configs (shared test configuration couples independent tests and can make them brittle), but I see value in adding helper methods to generate VHDS config. Will do, but in a separate PR if that's ok.

zuercher · 2019-11-14T22:25:40Z

test/integration/vhds_integration_test.cc

@@ -100,6 +110,113 @@ name: my_route
          cluster_name: xds_cluster
 )EOF";

+const char VhostTemplate[] = R"EOF(


Same as above.

mattklein123 · 2019-11-14T23:54:31Z

@mattklein123 were you documentation concerns addressed?

There is still no documentation merged for VHDS. Since this is a bug fix I won't hold this PR hostage, but nothing else will be merged until there is docs.

dmitri-d · 2019-11-20T17:39:09Z

ping @zuercher, @lambdai

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

dmitri-d · 2019-11-20T17:41:05Z

updated with the latest changes from master

lambdai · 2019-11-22T22:32:44Z

@dmitri-d My understanding is that it's only a TODO away from @zuercher 's approval on this PR

lambdai · 2019-11-22T22:33:02Z

The rest LGTM

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

dmitri-d · 2019-11-23T03:16:38Z

added a todo re: test configuration
merged latest changes from upstream master

dmitri-d · 2019-11-25T18:17:27Z

ping @lambdai, @zuercher

Per comment that we'll allow this fix, dimissing Matt's change request.

lambdai approved these changes Oct 17, 2019

View reviewed changes

lambdai suggested changes Oct 17, 2019

View reviewed changes

jmarantz assigned zuercher Oct 18, 2019

repokitteh-read-only bot added the waiting label Oct 21, 2019

mattklein123 previously requested changes Oct 21, 2019

View reviewed changes

repokitteh-read-only bot removed the waiting label Oct 21, 2019

lambdai previously approved these changes Oct 31, 2019

View reviewed changes

dmitri-d dismissed lambdai’s stale review via 6816819 November 1, 2019 19:18

Fixes #7617 - fixes handling creation of VHDS subscription after Init…

c2f8a6f

…Manager has completed initialization Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

lambdai previously approved these changes Nov 14, 2019

View reviewed changes

zuercher reviewed Nov 14, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into vhds-init-manager

eb5a1dd

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

Dmitri Dolguikh added 2 commits November 22, 2019 18:21

Added todo

94abf00

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

Merge remote-tracking branch 'upstream' into vhds-init-manager

e7b6615

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

dmitri-d dismissed lambdai’s stale review via e7b6615 November 23, 2019 03:15

zuercher approved these changes Nov 26, 2019

View reviewed changes

zuercher merged commit fb628ce into envoyproxy:master Nov 27, 2019

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

Fixes #7617 - fixes handling creation of VHDS subscription after Init… #8650

Fixes #7617 - fixes handling creation of VHDS subscription after Init… #8650

Conversation

dmitri-d commented Oct 17, 2019

lambdai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdai left a comment

Choose a reason for hiding this comment

zuercher commented Oct 21, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

dmitri-d commented Oct 21, 2019

mattklein123 commented Oct 21, 2019

dmitri-d commented Oct 21, 2019

dmitri-d commented Oct 22, 2019

dmitri-d commented Oct 24, 2019

lambdai left a comment

Choose a reason for hiding this comment

lambdai commented Oct 31, 2019

dmitri-d commented Nov 1, 2019

zuercher commented Nov 1, 2019 • edited Loading

dmitri-d commented Nov 4, 2019

dmitri-d commented Nov 4, 2019

dmitri-d commented Nov 7, 2019

dmitri-d commented Nov 12, 2019

lambdai left a comment

Choose a reason for hiding this comment

lambdai commented Nov 14, 2019

zuercher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Nov 14, 2019

dmitri-d commented Nov 20, 2019

dmitri-d commented Nov 20, 2019

lambdai commented Nov 22, 2019

lambdai commented Nov 22, 2019

dmitri-d commented Nov 23, 2019

dmitri-d commented Nov 25, 2019

zuercher commented Nov 1, 2019 •

edited

Loading

dmitri-d Nov 20, 2019 •

edited

Loading

dmitri-d Nov 22, 2019 •

edited

Loading