Skip to content

test: use TestRequestHeaderMapImpl for building custom headers #2455

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 11 commits into from
Aug 8, 2022
Merged

test: use TestRequestHeaderMapImpl for building custom headers #2455

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0f08b73
uno
caschoener Aug 3, 2022
bbb24fd
remove debug
caschoener Aug 3, 2022
d7f31d3
add indirection to make tests pass
caschoener Aug 3, 2022
3ac0389
format again
caschoener Aug 3, 2022
fe86f65
pass by reference
caschoener Aug 3, 2022
defc626
Change default_headers to impl
caschoener Aug 4, 2022
a398b4e
conversion function from scratch
caschoener Aug 4, 2022
fb2276d
cleanup
caschoener Aug 4, 2022
9e72e3c
cleanup 2
caschoener Aug 4, 2022
464f0e6
Address comments
caschoener Aug 4, 2022
2703f3a
ali comments
caschoener Aug 5, 2022
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
8 changes: 4 additions & 4 deletions library/common/http/header_utility.cc
View file
Open in desktop
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) {

@caschoener caschoener Aug 3, 2022

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
47 changes: 41 additions & 6 deletions test/common/integration/base_client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
#include "test/common/integration/base_client_integration_test.h"

#include <string>

#include "test/common/http/common.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 @@ -89,15 +95,44 @@ 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);
}
HttpTestUtility::addDefaultHeaders(default_request_headers_);
default_request_headers_.setHost(fake_upstreams_[0]->localAddress()->asStringView());

@abeyad abeyad Aug 4, 2022

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 is setHost on default_request_headers_ called after adding the default request headers?

@caschoener caschoener Aug 5, 2022

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.

addDefaultHeaders is really just an initialization function, which sets host to "/" I believe. And then we overwrite the values afterwards.

@abeyad abeyad Aug 5, 2022

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.

Got it, thanks for clarifying. The addDefaultHeaders name was a bit confusing, wonder if something like setDefaultHeaderValues would make more sense, but in any case, that's not something for this PR.

}

std::shared_ptr<Platform::RequestHeaders> BaseClientIntegrationTest::envoyToMobileHeaders(
Comment thread
alyssawilk marked this conversation as resolved.
const Http::TestRequestHeaderMapImpl& request_headers) {

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

Platform::RequestHeadersBuilder builder(
Platform::RequestMethod::GET,
std::string(default_request_headers_.Scheme()->value().getStringView()),
std::string(default_request_headers_.Host()->value().getStringView()), "/");

@alyssawilk alyssawilk Aug 4, 2022

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.

let's get path from the headers as well?

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

request_headers.iterate(
[&request_headers, &builder](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
std::string key_val = std::string(header.key().getStringView());
if (request_headers.formatter().has_value()) {
const Envoy::Http::StatefulHeaderKeyFormatter& formatter =
request_headers.formatter().value();
key_val = formatter.format(key_val);
}
auto key = std::string(key_val);

@alyssawilk alyssawilk Aug 4, 2022

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.

isn't key_val already a string? Why do we need the extra one?

auto value = std::vector<std::string>();
value.push_back(std::string(header.value().getStringView()));
builder.set(key, value);
return Http::HeaderMap::Iterate::Continue;
});

for (const auto& pair : rawHeaderMap) {
builder.set(pair.first, pair.second);
}

@abeyad abeyad Aug 4, 2022

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?

@caschoener caschoener Aug 5, 2022

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

auto mobile_headers = std::make_shared<Platform::RequestHeaders>(builder.build());
return mobile_headers;

@alyssawilk alyssawilk Aug 4, 2022

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) {
Expand Down
8 changes: 4 additions & 4 deletions test/common/integration/base_client_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,19 @@ class BaseClientIntegrationTest : public BaseIntegrationTest, public Platform::E
void threadRoutine(absl::Notification& engine_running);
// Must be called manually by subclasses in their TearDown();
void TearDown();

// Converts TestRequestHeaderMapImpl to Envoy::Platform::RequestHeadersSharedPtr
Envoy::Platform::RequestHeadersSharedPtr
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_;
Http::TestRequestHeaderMapImpl default_request_headers_;
Event::DispatcherPtr full_dispatcher_;
Platform::StreamPrototypeSharedPtr stream_prototype_;
Platform::StreamSharedPtr stream_;
Platform::EngineSharedPtr engine_;
Thread::ThreadPtr envoy_thread_;
std::string scheme_ = "http";
bool explicit_flow_control_ = false;
bool expect_dns_ = true;
bool override_builder_config_ = false;
Expand Down
56 changes: 38 additions & 18 deletions test/common/integration/client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
#include "test/common/integration/base_client_integration_test.h"
#include "test/integration/autonomous_upstream.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "library/common/data/utility.h"
#include "library/common/engine.h"
#include "library/common/http/header_utility.h"
#include "library/common/types/c_types.h"

using testing::ReturnRef;
Expand Down Expand Up @@ -39,11 +35,12 @@ 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");
default_request_headers_.addCopy(AutonomousStream::EXPECT_REQUEST_SIZE_BYTES,
std::to_string(request_data.length()));

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 +51,7 @@ TEST_P(ClientIntegrationTest, Basic) {
release_envoy_data(c_data);
});

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

envoy_data c_data = Data::Utility::toBridgeData(request_data);
stream_->sendData(c_data);
Expand Down Expand Up @@ -82,7 +79,7 @@ TEST_P(ClientIntegrationTest, BasicNon2xx) {
->setResponseHeaders(std::make_unique<Http::TestResponseHeaderMapImpl>(
Http::TestResponseHeaderMapImpl({{":status", "503"}, {"content-length", "0"}})));

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

ASSERT_EQ(cc_.on_error_calls, 0);
Expand All @@ -92,10 +89,11 @@ TEST_P(ClientIntegrationTest, BasicNon2xx) {
}

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

stream_->sendHeaders(default_request_headers_, true);
default_request_headers_.addCopy(AutonomousStream::RESET_AFTER_REQUEST, "yes");

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

ASSERT_EQ(cc_.on_error_calls, 1);
Expand All @@ -116,7 +114,7 @@ TEST_P(ClientIntegrationTest, BasicCancel) {
return nullptr;
});

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

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand Down Expand Up @@ -161,7 +159,7 @@ TEST_P(ClientIntegrationTest, CancelWithPartialStream) {
return nullptr;
});

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

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand Down Expand Up @@ -198,10 +196,33 @@ 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);
});

@alyssawilk alyssawilk Aug 4, 2022

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();

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

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

stream_prototype_->setOnHeaders(
[this](Platform::ResponseHeadersSharedPtr headers, bool, envoy_stream_intel) {
cc_.status = absl::StrCat(headers->httpStatus());
Expand All @@ -210,8 +231,7 @@ TEST_P(ClientIntegrationTest, CaseSensitive) {
EXPECT_TRUE((*headers)["My-ResponsE-Header"][0] == "foo");
return nullptr;
});

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

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand Down Expand Up @@ -240,7 +260,7 @@ TEST_P(ClientIntegrationTest, TimeoutOnRequestPath) {
autonomous_upstream_ = false;
initialize();

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

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand All @@ -261,7 +281,7 @@ TEST_P(ClientIntegrationTest, TimeoutOnResponsePath) {
autonomous_upstream_ = false;
initialize();

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

Envoy::FakeRawConnectionPtr upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
Expand Down
4 changes: 2 additions & 2 deletions test/common/integration/rtds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class RtdsIntegrationTest : public XdsIntegrationTest {
}

void initialize() override {
BaseClientIntegrationTest::initialize();
XdsIntegrationTest::initialize();
initializeXdsStream();
}
};
Expand All @@ -64,7 +64,7 @@ TEST_P(RtdsIntegrationTest, RtdsReload) {
initialize();

// Send a request on the data plane.
stream_->sendHeaders(default_request_headers_, true);
stream_->sendHeaders(envoyToMobileHeaders(default_request_headers_), true);
terminal_callback_.waitReady();

EXPECT_EQ(cc_.on_headers_calls, 1);
Expand Down
2 changes: 1 addition & 1 deletion test/common/integration/sds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ TEST_P(SdsIntegrationTest, SdsForUpstreamCluster) {
ASSERT_TRUE(
waitForCounterGe("cluster.base_h2.client_ssl_socket_factory.ssl_context_update_by_sds", 1));

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

EXPECT_EQ(cc_.on_headers_calls, 1);
Expand Down
6 changes: 5 additions & 1 deletion test/common/integration/xds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ XdsIntegrationTest::XdsIntegrationTest() : BaseClientIntegrationTest(ipVersion()
expect_dns_ = false; // TODO(alyssawilk) debug.
create_xds_upstream_ = true;
sotw_or_delta_ = sotwOrDelta();
scheme_ = "https";

if (sotw_or_delta_ == Grpc::SotwOrDelta::UnifiedSotw ||
sotw_or_delta_ == Grpc::SotwOrDelta::UnifiedDelta) {
Expand Down Expand Up @@ -55,6 +54,11 @@ XdsIntegrationTest::XdsIntegrationTest() : BaseClientIntegrationTest(ipVersion()
setAdminAddressPathForTests(admin_filename_);
}

void XdsIntegrationTest::initialize() {
BaseClientIntegrationTest::initialize();
default_request_headers_.setScheme("https");
}

Network::Address::IpVersion XdsIntegrationTest::ipVersion() const {
return std::get<0>(GetParam());
}
Expand Down
1 change: 1 addition & 0 deletions test/common/integration/xds_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class XdsIntegrationTest : public BaseClientIntegrationTest,
public:
XdsIntegrationTest();
virtual ~XdsIntegrationTest() = default;
void initialize() override;

protected:
void SetUp() override;
Expand Down