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.

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

Jump to bottom

connect: regression testing bugfix #34112

Merged
merged 4 commits into from
May 16, 2024
Merged

connect: regression testing bugfix #34112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
983d075
connect: working around balsa bug
alyssawilk May 13, 2024
65c5c49
comments
alyssawilk May 13, 2024
5a8e4f1
relying on bnc fix
alyssawilk May 14, 2024
e76725a
Merge branch 'main' into connect
alyssawilk May 15, 2024
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
3 changes: 1 addition & 2 deletions source/extensions/transport_sockets/http_11_proxy/connect.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ bool UpstreamHttp11ConnectSocket::isValidConnectResponse(absl::string_view respo
bytes_processed = parser.parser().execute(response_payload.data(), response_payload.length());
headers_complete = parser.headersComplete();

return parser.parser().getStatus() != Http::Http1::ParserStatus::Error &&
parser.headersComplete() && parser.parser().statusCode() == Http::Code::OK;
return parser.headersComplete() && parser.status200();
}

UpstreamHttp11ConnectSocket::UpstreamHttp11ConnectSocket(
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/transport_sockets/http_11_proxy/connect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ class SelfContainedParser : public Http::Http1::ParserCallbacks {
}
Http::Http1::CallbackResult onHeadersComplete() override {
headers_complete_ = true;
status_200_ = parser().getStatus() != Http::Http1::ParserStatus::Error &&
parser().statusCode() == Http::Code::OK;
Copy link
Contributor

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

Copy link
Contributor

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!)

parser_.pause();
return Http::Http1::CallbackResult::Success;
}
Expand All @@ -91,10 +93,12 @@ class SelfContainedParser : public Http::Http1::ParserCallbacks {
void onChunkHeader(bool) override {}

bool headersComplete() const { return headers_complete_; }
bool status200() const { return status_200_; }
Http::Http1::BalsaParser& parser() { return parser_; }

private:
bool headers_complete_ = false;
bool status_200_ = false;
Http::Http1::BalsaParser parser_;
};

Expand Down
11 changes: 7 additions & 4 deletions test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

std::string content_length = include_content_length ? "Content-Length: 0\r\n" : "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Super-nitty since it's just a test, but absl::string_view is good for this.

// 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;
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the test without content length also still exist?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Expand All @@ -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_));
Expand Down
Loading