Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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 library/cc/bridge_utility.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "bridge_utility.h"

#include <iostream>
#include <sstream>

#include "library/common/data/utility.h"
Expand Down Expand Up @@ -38,7 +39,6 @@ RawHeaderMap envoyHeadersAsRawHeaderMap(envoy_headers raw_headers) {
for (auto i = 0; i < raw_headers.length; i++) {
auto key = Data::Utility::copyToString(raw_headers.entries[i].key);
auto value = Data::Utility::copyToString(raw_headers.entries[i].value);

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.

looks like changes to this file can be reverted?

if (!headers.contains(key)) {
headers.emplace(key, std::vector<std::string>());
}
Expand Down
9 changes: 4 additions & 5 deletions library/common/http/header_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ namespace Envoy {
namespace Http {
namespace Utility {

void toEnvoyHeaders(HeaderMap& transformed_headers, envoy_headers headers) {

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.

The edits to this file aren't strictly part of this diff. I just noticed that the toEnvoyHeaders function has better parameter names in its header_utility.h file, figured I'd fix the mismatch.

Envoy::Http::StatefulHeaderKeyFormatter& formatter = transformed_headers.formatter().value();
void toEnvoyHeaders(HeaderMap& envoy_result_headers, envoy_headers headers) {
Envoy::Http::StatefulHeaderKeyFormatter& formatter = envoy_result_headers.formatter().value();
for (envoy_map_size_t i = 0; i < headers.length; i++) {
std::string key = Data::Utility::copyToString(headers.entries[i].key);
// Make sure the formatter knows the original case.
formatter.processKey(key);
transformed_headers.addCopy(LowerCaseString(key),
Data::Utility::copyToString(headers.entries[i].value));
envoy_result_headers.addCopy(LowerCaseString(key),
Data::Utility::copyToString(headers.entries[i].value));
}
// The C envoy_headers struct can be released now because the headers have been copied.
release_envoy_headers(headers);
Expand Down Expand Up @@ -62,7 +62,6 @@ envoy_headers toBridgeHeaders(const HeaderMap& header_map, absl::string_view alp
}
envoy_data key = Data::Utility::copyToBridgeData(key_val);
envoy_data value = Data::Utility::copyToBridgeData(header.value().getStringView());

transformed_headers.entries[transformed_headers.length] = {key, value};
transformed_headers.length++;

Expand Down
24 changes: 20 additions & 4 deletions test/common/integration/base_client_integration_test.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "test/common/integration/base_client_integration_test.h"

#include "gtest/gtest.h"
#include "library/cc/bridge_utility.h"
#include "library/common/config/internal.h"
#include "library/common/http/header_utility.h"

namespace Envoy {
namespace {
Expand Down Expand Up @@ -90,16 +92,30 @@ void BaseClientIntegrationTest::initialize() {
stream_ = (*stream_prototype_).start(explicit_flow_control_);
std::string host(fake_upstreams_[0]->localAddress()->asStringView());
Platform::RequestHeadersBuilder builder(Platform::RequestMethod::GET, scheme_, host, "/");
for (auto& entry : custom_headers_) {
auto values = {entry.second};
builder.set(entry.first, values);
}
if (upstreamProtocol() == Http::CodecType::HTTP2) {
builder.addUpstreamHttpProtocol(Platform::UpstreamHttpProtocol::HTTP2);
}
default_request_headers_ = std::make_shared<Platform::RequestHeaders>(builder.build());

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.

I think we want to move default_request_headers_ to be a TestRequestHeaderMap impl, and shift all the tests over to using the new utility.

}

std::shared_ptr<Platform::RequestHeaders> BaseClientIntegrationTest::envoyToMobileHeaders(
Comment thread
alyssawilk marked this conversation as resolved.
const Http::TestRequestHeaderMapImpl& request_headers) {
std::string host(fake_upstreams_[0]->localAddress()->asStringView());

envoy_headers envoyHeaders = Http::Utility::toBridgeHeaders(request_headers);
Platform::RawHeaderMap rawHeaderMap = Platform::envoyHeadersAsRawHeaderMap(envoyHeaders);

Platform::RequestHeadersBuilder builder(Platform::RequestMethod::GET, scheme_, host, "/");

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.

I think we actually want to get the scheme and host and path from the request headers, which may mean setting the host and scheme in default_request_headers in the test set up (like they were before)
it's rare but a useful corner case to be able to test http schemed requests over a TLS conection

for (const auto& pair : rawHeaderMap) {

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.

can we do something like
header_map.iterate(
[&request_headers, builder](const HeaderEntry& header) -> HeaderMap::Iterate {
}
to get the key/value pairs from request_headers and stick them in builder without doing the above conversions?

builder.set(pair.first, pair.second);
}

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.

I'm not sure I follow L117-133. In L117, we iterate over request_headers, and add them to the RequestHeadersBuilder. In L131, we iterate over the rawHeaderMap and add them to the RequestHeadersBuilder. But the rawHeaderMap is built from envoy_headers which is built from request_headers, so aren't we adding the same headers to the RequestHeadersBuilder twice?

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.

Great catch yeah looks like I forgot to delete some code :O

if (upstreamProtocol() == Http::CodecType::HTTP2) {
builder.addUpstreamHttpProtocol(Platform::UpstreamHttpProtocol::HTTP2);
}
auto mobile_headers = std::make_shared<Platform::RequestHeaders>(builder.build());
return mobile_headers;

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: consider skipping the temporary variable and just return the make_shared

}

void BaseClientIntegrationTest::threadRoutine(absl::Notification& engine_running) {
setOnEngineRunning([&]() { engine_running.Notify(); });
engine_ = build();
Expand Down
3 changes: 2 additions & 1 deletion test/common/integration/base_client_integration_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,14 @@ class BaseClientIntegrationTest : public BaseIntegrationTest, public Platform::E
void threadRoutine(absl::Notification& engine_running);
// Must be called manually by subclasses in their TearDown();
void TearDown();
std::shared_ptr<Platform::RequestHeaders>

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.

comment?

envoyToMobileHeaders(const Http::TestRequestHeaderMapImpl& request_headers);

Event::ProvisionalDispatcherPtr dispatcher_ = std::make_unique<Event::ProvisionalDispatcher>();
envoy_http_callbacks bridge_callbacks_;
ConditionalInitializer terminal_callback_;
callbacks_called cc_{0, 0, 0, 0, 0, 0, 0, "", &terminal_callback_, {}};
std::shared_ptr<Platform::RequestHeaders> default_request_headers_;
absl::flat_hash_map<std::string, std::string> custom_headers_;
Event::DispatcherPtr full_dispatcher_;
Platform::StreamPrototypeSharedPtr stream_prototype_;
Platform::StreamSharedPtr stream_;
Expand Down
57 changes: 49 additions & 8 deletions test/common/integration/client_integration_test.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "source/extensions/http/header_formatters/preserve_case/config.h"
#include "source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h"

#include "test/common/http/common.h"
#include "test/common/integration/base_client_integration_test.h"
#include "test/integration/autonomous_upstream.h"

Expand Down Expand Up @@ -39,11 +40,16 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, ClientIntegrationTest,
TestUtility::ipTestParamsToString);

TEST_P(ClientIntegrationTest, Basic) {
Buffer::OwnedImpl request_data = Buffer::OwnedImpl("request body");
custom_headers_.emplace(AutonomousStream::EXPECT_REQUEST_SIZE_BYTES,
std::to_string(request_data.length()));
initialize();

Buffer::OwnedImpl request_data = Buffer::OwnedImpl("request body");
Http::TestRequestHeaderMapImpl custom_headers;
HttpTestUtility::addDefaultHeaders(custom_headers);

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.

I think we can use default_request_headers_ and make sure we addDefaultHeaders in initialize() to avoid having to create headers in each test case

custom_headers.addCopy(AutonomousStream::EXPECT_REQUEST_SIZE_BYTES,
std::to_string(request_data.length()));
std::shared_ptr<Platform::RequestHeaders> custom_request_headers =
envoyToMobileHeaders(custom_headers);

stream_prototype_->setOnData([this](envoy_data c_data, bool end_stream) {
if (end_stream) {
EXPECT_EQ(Data::Utility::copyToString(c_data), "");
Expand All @@ -54,7 +60,7 @@ TEST_P(ClientIntegrationTest, Basic) {
release_envoy_data(c_data);
});

stream_->sendHeaders(default_request_headers_, false);
stream_->sendHeaders(custom_request_headers, false);

envoy_data c_data = Data::Utility::toBridgeData(request_data);
stream_->sendData(c_data);
Expand Down Expand Up @@ -92,10 +98,15 @@ TEST_P(ClientIntegrationTest, BasicNon2xx) {
}

TEST_P(ClientIntegrationTest, BasicReset) {
custom_headers_.emplace(AutonomousStream::RESET_AFTER_REQUEST, "yes");
initialize();

stream_->sendHeaders(default_request_headers_, true);
Http::TestRequestHeaderMapImpl custom_headers;
HttpTestUtility::addDefaultHeaders(custom_headers);
custom_headers.addCopy(AutonomousStream::RESET_AFTER_REQUEST, "yes");
std::shared_ptr<Platform::RequestHeaders> custom_request_headers =
envoyToMobileHeaders(custom_headers);

stream_->sendHeaders(custom_request_headers, true);
terminal_callback_.waitReady();

ASSERT_EQ(cc_.on_error_calls, 1);
Expand Down Expand Up @@ -198,10 +209,39 @@ TEST_P(ClientIntegrationTest, CancelWithPartialStream) {

// Test header key case sensitivity.
TEST_P(ClientIntegrationTest, CaseSensitive) {
custom_headers_.emplace("FoO", "bar");
autonomous_upstream_ = false;
Envoy::Extensions::Http::HeaderFormatters::PreserveCase::
forceRegisterPreserveCaseFormatterFactoryConfig();
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
ConfigHelper::HttpProtocolOptions protocol_options;
auto typed_extension_config = protocol_options.mutable_explicit_http_config()
->mutable_http_protocol_options()
->mutable_header_key_format()
->mutable_stateful_formatter();
typed_extension_config->set_name("preserve_case");
typed_extension_config->mutable_typed_config()->set_type_url(
"type.googleapis.com/"
"envoy.extensions.http.header_formatters.preserve_case.v3.PreserveCaseFormatterConfig");
ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
protocol_options);
});

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.

why do we need this? Envoy defaults should include stateful formatter

initialize();

Http::TestRequestHeaderMapImpl custom_headers;
HttpTestUtility::addDefaultHeaders(custom_headers);

custom_headers.header_map_->setFormatter(
std::make_unique<
Extensions::Http::HeaderFormatters::PreserveCase::PreserveCaseHeaderFormatter>(
false, envoy::extensions::http::header_formatters::preserve_case::v3::
PreserveCaseFormatterConfig::DEFAULT));

custom_headers.addCopy("FoO", "bar");
custom_headers.header_map_->formatter().value().get().processKey("FoO");

std::shared_ptr<Platform::RequestHeaders> custom_request_headers =
envoyToMobileHeaders(custom_headers);

stream_prototype_->setOnHeaders(
[this](Platform::ResponseHeadersSharedPtr headers, bool, envoy_stream_intel) {
cc_.status = absl::StrCat(headers->httpStatus());
Expand All @@ -211,7 +251,7 @@ TEST_P(ClientIntegrationTest, CaseSensitive) {
return nullptr;
});

stream_->sendHeaders(default_request_headers_, true);
stream_->sendHeaders(custom_request_headers, true);

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand All @@ -220,6 +260,7 @@ TEST_P(ClientIntegrationTest, CaseSensitive) {
std::string upstream_request;
EXPECT_TRUE(upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("GET /"),
&upstream_request));

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: always good to revert non-necessary changes just to reduce churn :-)

EXPECT_TRUE(absl::StrContains(upstream_request, "FoO: bar")) << upstream_request;

// Send mixed case headers, and verify via setOnHeaders they are received correctly.
Expand Down