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 all 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) {
Copy link
Copy Markdown
Contributor Author

@caschoener caschoener Aug 3, 2022

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
40 changes: 34 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,37 @@ 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());
Copy link
Copy Markdown
Contributor

@abeyad abeyad Aug 4, 2022

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?

Copy link
Copy Markdown
Contributor Author

@caschoener caschoener Aug 5, 2022

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.

Copy link
Copy Markdown
Contributor

@abeyad abeyad Aug 5, 2022

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(
const Http::TestRequestHeaderMapImpl& request_headers) {

Platform::RequestHeadersBuilder builder(
Platform::RequestMethod::GET,
std::string(default_request_headers_.Scheme()->value().getStringView()),
std::string(default_request_headers_.Host()->value().getStringView()),
std::string(default_request_headers_.Path()->value().getStringView()));
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 = std::string(header.key().getStringView());
if (request_headers.formatter().has_value()) {
const Envoy::Http::StatefulHeaderKeyFormatter& formatter =
request_headers.formatter().value();
key = formatter.format(key);
}
auto value = std::vector<std::string>();
value.push_back(std::string(header.value().getStringView()));
builder.set(key, value);
return Http::HeaderMap::Iterate::Continue;
});

return std::make_shared<Platform::RequestHeaders>(builder.build());
}

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
42 changes: 23 additions & 19 deletions test/common/integration/client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
#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/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 +34,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 +50,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 +78,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 +88,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 +113,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 +158,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 +195,18 @@ TEST_P(ClientIntegrationTest, CancelWithPartialStream) {

// Test header key case sensitivity.
TEST_P(ClientIntegrationTest, CaseSensitive) {
custom_headers_.emplace("FoO", "bar");
autonomous_upstream_ = false;
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 +215,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 +244,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 +265,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