Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a test to verify vhds updates when client closes connection #11341

Merged
merged 7 commits into from
Jun 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() {
Http::RequestHeaderMapImpl host_header;
host_header.setHost(VhdsSubscription::aliasToDomainName(it->alias_));
const bool host_exists = config->virtualHostExists(host_header);
auto current_cb = it->cb_;
std::weak_ptr<Http::RouteConfigUpdatedCallback> current_cb(it->cb_);
it->thread_local_dispatcher_.post([current_cb, host_exists] {
if (auto cb = current_cb.lock()) {
(*cb)(host_exists);
Expand Down
5 changes: 5 additions & 0 deletions source/extensions/filters/http/on_demand/on_demand_update.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCal
callbacks_ = &callbacks;
}

// A weak_ptr copy of the route_config_updated_callback_ is kept by RdsRouteConfigProviderImpl
// in config_update_callbacks_. By resetting the pointer in onDestroy() callback we ensure
// that this filter/filter-chain will not be resumed if the corresponding has been closed
void OnDemandRouteUpdate::onDestroy() { route_config_updated_callback_.reset(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a test for this!

So you're resetting the callback, but what tells the caller of the callback to not call it?
you've created this lambda capturing this of the filter, and if the filter is destroyed, and the resolution completes, won't the callback still be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback pointer is eventually stored in a weak_ptr, which is verified before the invocation: https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L288. A longer description of what's going on in relation to the callback is here: #9784 (comment).


// This is the callback which is called when an update requested in requestRouteConfigUpdate()
// has been propagated to workers, at which point the request processing is restarted from the
// beginning.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter {

void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override;

void onDestroy() override {}
void onDestroy() override;

private:
Http::StreamDecoderFilterCallbacks* callbacks_{};
Expand Down
35 changes: 34 additions & 1 deletion test/integration/vhds_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -653,5 +653,38 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateFailToResolveOneAliasOutOfSeveral)
cleanupUpstreamAndDownstream();
}

// Verify that an vhds update succeeds even when the client closes its connection
TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateHttpConnectionCloses) {
// RDS exchange with a non-empty virtual_hosts field
useRdsWithVhosts();

testRouterHeaderOnlyRequestAndResponse(nullptr, 1);
cleanupUpstreamAndDownstream();
EXPECT_TRUE(codec_client_->waitForDisconnect());

// Attempt to make a request to an unknown host
codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
{":path", "/"},
{":scheme", "http"},
{":authority", "vhost_1"},
{"x-lyft-user-id", "123"}};
auto encoder_decoder = codec_client_->startRequest(request_headers);
Http::RequestEncoder& encoder = encoder_decoder.first;
IntegrationStreamDecoderPtr response = std::move(encoder_decoder.second);
EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost,
{vhdsRequestResourceName("vhost_1")}, {}, vhds_stream_));

envoy::api::v2::DeltaDiscoveryResponse vhds_update =
createDeltaDiscoveryResponseWithResourceNameUsedAsAlias();
vhds_stream_->sendGrpcMessage(vhds_update);

codec_client_->sendReset(encoder);
response->waitForReset();
EXPECT_TRUE(codec_client_->connected());

cleanupUpstreamAndDownstream();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's wait for the client_codec_->waitForEndStream() and assert the connection is connected, to make sure we're covering the case we're trying to cover (stream is reset,connection is not)

}

} // namespace
} // namespace Envoy
} // namespace Envoy