-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
connect: regression testing bugfix #34112
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,15 +120,17 @@ name: envoy.clusters.dynamic_forward_proxy | |
} | ||
} | ||
|
||
void stripConnectUpgradeAndRespond() { | ||
void stripConnectUpgradeAndRespond(bool include_content_length = false) { | ||
// Strip the CONNECT upgrade. | ||
std::string prefix_data; | ||
const std::string hostname(default_request_headers_.getHostValue()); | ||
ASSERT_TRUE(fake_upstream_connection_->waitForInexactRawData("\r\n\r\n", &prefix_data)); | ||
EXPECT_EQ(absl::StrCat("CONNECT ", hostname, ":443 HTTP/1.1\r\n\r\n"), prefix_data); | ||
|
||
absl::string_view content_length = include_content_length ? "Content-Length: 0\r\n" : ""; | ||
// Ship the CONNECT response. | ||
fake_upstream_connection_->writeRawData("HTTP/1.1 200 OK\r\n\r\n"); | ||
fake_upstream_connection_->writeRawData( | ||
absl::StrCat("HTTP/1.1 200 OK\r\n", content_length, "\r\n")); | ||
} | ||
bool use_alpn_ = false; | ||
bool try_http3_ = false; | ||
|
@@ -450,7 +452,8 @@ TEST_P(Http11ConnectHttpIntegrationTest, DfpNoProxyAddress) { | |
EXPECT_THAT(waitForAccessLog(access_log_name_), testing::HasSubstr("dns_resolution_failure")); | ||
} | ||
|
||
TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddress) { | ||
// Regression tests https://github.com/envoyproxy/envoy/issues/34096 | ||
TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddressAndContentLength) { | ||
Comment on lines
+455
to
+456
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the test without content length also still exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't get a response on this comment. Seems like both paths should be getting tested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah sorry we do that by default - all th eother tests test this with stripConnectUpgradeAndRespond() |
||
addDfpConfig(); | ||
|
||
initialize(); | ||
|
@@ -470,7 +473,7 @@ TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddress) { | |
// The request should be sent to fake upstream 1, as DNS lookup is bypassed due to the | ||
// connect-proxy header. | ||
ASSERT_TRUE(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); | ||
stripConnectUpgradeAndRespond(); | ||
stripConnectUpgradeAndRespond(true); | ||
|
||
ASSERT_TRUE(fake_upstream_connection_->readDisable(false)); | ||
ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put this operation into the
status200()
function rather than precalculating and storing? (If there is a reason, a comment explaining that reason would be good.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I see the reason in #34096 now, so explanatory comment is the request!)