Skip to content

dfp: adding queueing test #18748

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.

Sign up for GitHub

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

Merged
alyssawilk merged 2 commits into from
Oct 25, 2021
Merged

dfp: adding queueing test #18748

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bbb877
dfp: adding queueing test
alyssawilk Oct 25, 2021
a495985
comments
alyssawilk Oct 25, 2021
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
62 changes: 62 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,5 +446,67 @@ TEST_P(ProxyFilterIntegrationTest, UseCacheFileAndTestHappyEyeballs) {
EXPECT_EQ(1, test_server_->counter("dns_cache.foo.host_added")->value());
}

TEST_P(ProxyFilterIntegrationTest, MultipleRequestsLowStreamLimit) {
setDownstreamProtocol(Http::CodecType::HTTP2);
setUpstreamProtocol(Http::CodecType::HTTP2);

// Ensure we only have one connection upstream, one request active at a time.
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Choose a reason for hiding this comment

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

Thank you for this comment. There is so much boilerplate required to get this config working that I would probably not have figured out the intent.

Copy link
Contributor Author

@alyssawilk alyssawilk Oct 25, 2021

Choose a reason for hiding this comment

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

yeah a lot of Envoy tests are hard to read especially with heavy mock use so I encourage overcommenting rather than under :-P

config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
envoy::config::bootstrap::v3::Bootstrap::StaticResources* static_resources =
bootstrap.mutable_static_resources();
envoy::config::cluster::v3::Cluster* cluster = static_resources->mutable_clusters(0);
envoy::config::cluster::v3::CircuitBreakers* circuit_breakers =
cluster->mutable_circuit_breakers();
circuit_breakers->add_thresholds()->mutable_max_connections()->set_value(1);
ConfigHelper::HttpProtocolOptions protocol_options;
protocol_options.mutable_explicit_http_config()
->mutable_http2_protocol_options()
->mutable_max_concurrent_streams()
->set_value(1);
ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
protocol_options);
});

// Start sending the request, but ensure no end stream will be sent, so the
// stream will stay in use.
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));

// Start sending the request, but ensure no end stream will be sent, so the
// stream will stay in use.
std::pair<Http::RequestEncoder&, IntegrationStreamDecoderPtr> encoder_decoder =
codec_client_->startRequest(default_request_headers_);
request_encoder_ = &encoder_decoder.first;
IntegrationStreamDecoderPtr response = std::move(encoder_decoder.second);

// Make sure the headers are received.
ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());

// Start another request.
IntegrationStreamDecoderPtr response2 =
codec_client_->makeHeaderOnlyRequest(default_request_headers_);
test_server_->waitForCounterEq("http.config_test.downstream_rq_total", 2);
// Make sure the stream is not received.
ASSERT_FALSE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_,
std::chrono::milliseconds(100)));
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Choose a reason for hiding this comment

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

This seems totally reasonable for an integration test, but it's mildly annoying that there is now way to "ensure something will never happen in the future". But I guess that's just not how the world works :)


// Finish the first stream.
codec_client_->sendData(*request_encoder_, 0, true);
upstream_request_->encodeHeaders(default_response_headers_, true);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().getStatusValue());

// This should allow the second stream to complete
ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
upstream_request_->encodeHeaders(default_response_headers_, true);
ASSERT_TRUE(response2->waitForEndStream());
EXPECT_TRUE(response2->complete());
EXPECT_EQ("200", response2->headers().getStatusValue());
}

} // namespace
} // namespace Envoy