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 1 commit
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
58 changes: 58 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,63 @@ 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.

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Copy link
Copy Markdown
Contributor

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.

@alyssawilk alyssawilk Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

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) {
auto* static_resources = bootstrap.mutable_static_resources();
auto* cluster = static_resources->mutable_clusters(0);
auto* circuit_breakers = cluster->mutable_circuit_breakers();

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: there are quite a few uses of auto here where the type is not really obvious to me. I think the style guide recommends against auto in cases like this. (Down on line 475, too)

https://google.github.io/styleguide/cppguide.html#Type_deduction

@alyssawilk alyssawilk Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah I think envoy-internal style is to use auto pretty heavily especially for protos where the types are painfully long. I'm up for giving it a shot as beyond it being the style guide, I agree it can be confusing especially to newcomers

I've simply gotten in the habit of "grep for who has implements this function" =P

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.
auto encoder_decoder = codec_client_->startRequest(default_request_headers_);
request_encoder_ = &encoder_decoder.first;
auto response = std::move(encoder_decoder.second);

// make sure the headers are received.

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: make => Make

@alyssawilk alyssawilk Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

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.
auto 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)));

@RyanTheOptimist RyanTheOptimist Oct 25, 2021

Copy link
Copy Markdown
Contributor

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