Skip to content

test: fix headerMapEqualIgnoreOrder utility missing a check #19158

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
yanavlasov merged 7 commits into from
Dec 2, 2021
Merged

test: fix headerMapEqualIgnoreOrder utility missing a check #19158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f2cd071
Added missing check outside lambda
tangsaidi Dec 1, 2021
d650d78
style fix
tangsaidi Dec 1, 2021
6b091fe
Added missing headers in http cache impl test
tangsaidi Dec 1, 2021
b50a75d
Trigger build
tangsaidi Dec 1, 2021
5b21f36
fixed error message
tangsaidi Dec 1, 2021
0efe7b6
fixed ext_proc
tangsaidi Dec 1, 2021
69f7266
moved x-forwarded-proto from ignored set to expected
tangsaidi Dec 1, 2021
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
35 changes: 27 additions & 8 deletions test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,17 +343,25 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersAndMetadata) {
const std::string time_value_1 = formatter_.fromTime(time_source_.systemTime());
Http::TestResponseHeaderMapImpl response_headers{{"date", time_value_1},
{"cache-control", "public,max-age=3600"}};
Http::TestResponseHeaderMapImpl response_headers_with_age(response_headers);
response_headers_with_age.setReferenceKey(Http::LowerCaseString("age"), "0");

insert(request_path_1, response_headers, "body");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers));
EXPECT_TRUE(
expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_with_age));

// Update the date field in the headers
time_source_.advanceTimeWait(Seconds(3601));
const SystemTime time_2 = time_source_.systemTime();
const std::string time_value_2 = formatter_.fromTime(time_2);
response_headers = Http::TestResponseHeaderMapImpl{{"date", time_value_2},
{"cache-control", "public,max-age=3600"}};
updateHeaders(request_path_1, response_headers, {time_2});
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers));
Http::TestResponseHeaderMapImpl response_headers_2 = Http::TestResponseHeaderMapImpl{
{"date", time_value_2}, {"cache-control", "public,max-age=3600"}};
Http::TestResponseHeaderMapImpl response_headers_with_age_2(response_headers_2);
response_headers_with_age_2.setReferenceKey(Http::LowerCaseString("age"), "0");

updateHeaders(request_path_1, response_headers_2, {time_2});
EXPECT_TRUE(
expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_with_age_2));
}

TEST_F(SimpleHttpCacheTest, UpdateHeadersForMissingKey) {
Expand All @@ -373,6 +381,8 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersDisabledForVaryHeaders) {
{"accept", "image/*"},
{"vary", "accept"}};
insert(request_path_1, response_headers_1, "body");
// An age header is inserted by `makeLookUpResult`
response_headers_1.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_1));

// Update the date field in the headers
Expand All @@ -384,7 +394,8 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersDisabledForVaryHeaders) {
{"accept", "image/*"},
{"vary", "accept"}};
updateHeaders(request_path_1, response_headers_2, {time_2});

response_headers_1.setReferenceKey(Http::LowerCaseString("age"), "3600");
// the age is still 0 because an entry is considered fresh after validation
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_1));
}

Expand All @@ -394,6 +405,8 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersSkipEtagHeader) {
Http::TestResponseHeaderMapImpl response_headers_1{
{"date", time_value_1}, {"cache-control", "public,max-age=3600"}, {"etag", "0000-0000"}};
insert(request_path_1, response_headers_1, "body");
// An age header is inserted by `makeLookUpResult`
response_headers_1.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_1));

// Update the date field in the headers
Expand All @@ -407,7 +420,7 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersSkipEtagHeader) {
{"date", time_value_2}, {"cache-control", "public,max-age=3600"}, {"etag", "0000-0000"}};

updateHeaders(request_path_1, response_headers_2, {time_2});

response_headers_3.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_3));
}

Expand All @@ -425,6 +438,9 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersSkipSpecificHeaders) {
{"etag", "1111-1111"},
{"link", "<https://example.com>; rel=\"preconnect\""}};
insert(request_path_1, origin_response_headers, "body");

// An age header is inserted by `makeLookUpResult`
origin_response_headers.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(
expectLookupSuccessWithHeaders(lookup(request_path_1).get(), origin_response_headers));
time_source_.advanceTimeWait(Seconds(100));
Expand Down Expand Up @@ -454,7 +470,6 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersSkipSpecificHeaders) {
{"link", "<https://changed.com>; rel=\"preconnect\""}};

updateHeaders(request_path_1, incoming_response_headers, {time_2});

EXPECT_TRUE(
expectLookupSuccessWithHeaders(lookup(request_path_1).get(), expected_response_headers));
}
Expand All @@ -471,6 +486,9 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersWithMultivalue) {
{"link", "<https://www.example.com>; rel=\"preconnect\""},
{"link", "<https://example.com>; rel=\"preconnect\""}};
insert(request_path_1, response_headers_1, "body");

// An age header is inserted by `makeLookUpResult`
response_headers_1.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_1));

Http::TestResponseHeaderMapImpl response_headers_2{
Expand All @@ -481,6 +499,7 @@ TEST_F(SimpleHttpCacheTest, UpdateHeadersWithMultivalue) {

updateHeaders(request_path_1, response_headers_2, {time_1});

response_headers_2.setReferenceKey(Http::LowerCaseString("age"), "0");
EXPECT_TRUE(expectLookupSuccessWithHeaders(lookup(request_path_1).get(), response_headers_2));
}

Expand Down
8 changes: 3 additions & 5 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,11 +447,9 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeaders) {
[](Http::HeaderMap& headers) { headers.addCopy(LowerCaseString("x-remove-this"), "yes"); });

processRequestHeadersMessage(true, [](const HttpHeaders& headers, HeadersResponse& headers_resp) {
Http::TestRequestHeaderMapImpl expected_request_headers{{":scheme", "http"},
{":method", "GET"},
{"host", "host"},
{":path", "/"},
{"x-remove-this", "yes"}};
Http::TestRequestHeaderMapImpl expected_request_headers{
{":scheme", "http"}, {":method", "GET"}, {"host", "host"},
{":path", "/"}, {"x-remove-this", "yes"}, {"x-forwarded-proto", "http"}};
EXPECT_THAT(headers.headers(), HeaderProtosEqual(expected_request_headers));

auto response_header_mutation = headers_resp.mutable_response()->mutable_header_mutation();
Expand Down
9 changes: 8 additions & 1 deletion test/extensions/filters/http/ext_proc/utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,20 @@ namespace Extensions {
namespace HttpFilters {
namespace ExternalProcessing {

const absl::flat_hash_set<std::string> ExtProcTestUtility::ignoredHeaders() {
CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "x-request-id",
"x-envoy-upstream-service-time");
}

bool ExtProcTestUtility::headerProtosEqualIgnoreOrder(
const Http::HeaderMap& expected, const envoy::config::core::v3::HeaderMap& actual) {
// Comparing header maps is hard because they have duplicates in them.
// So we're going to turn them into a HeaderMap and let Envoy do the work.
Http::TestRequestHeaderMapImpl actual_headers;
for (const auto& header : actual.headers()) {
actual_headers.addCopy(header.key(), header.value());
if (!ignoredHeaders().contains(header.key())) {
actual_headers.addCopy(header.key(), header.value());
}
}
return TestUtility::headerMapEqualIgnoreOrder(expected, actual_headers);
}
Expand Down
5 changes: 5 additions & 0 deletions test/extensions/filters/http/ext_proc/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ class ExtProcTestUtility {
// Compare a reference header map to a proto
static bool headerProtosEqualIgnoreOrder(const Http::HeaderMap& expected,
const envoy::config::core::v3::HeaderMap& actual);

private:
// These headers are present in the actual, but cannot be specified in the expected
// ignoredHeaders should not be used for equal comparison
static const absl::flat_hash_set<std::string> ignoredHeaders();
};

MATCHER_P(HeaderProtosEqual, expected, "HTTP header protos match") {
Expand Down
7 changes: 5 additions & 2 deletions test/test_common/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ bool TestUtility::headerMapEqualIgnoreOrder(const Http::HeaderMap& lhs,
lhs_keys.insert(key);
return Http::HeaderMap::Iterate::Continue;
});
rhs.iterate([&lhs, &rhs, &rhs_keys](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
bool values_match = true;
rhs.iterate([&values_match, &lhs, &rhs,
&rhs_keys](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
const std::string key{header.key().getStringView()};
// Compare with canonicalized multi-value headers. This ensures we respect order within
// a header.
Expand All @@ -84,12 +86,13 @@ bool TestUtility::headerMapEqualIgnoreOrder(const Http::HeaderMap& lhs,
Http::HeaderUtility::getAllOfHeaderAsString(rhs, Http::LowerCaseString(key));
ASSERT(rhs_entry.result());
if (lhs_entry.result() != rhs_entry.result()) {
values_match = false;
return Http::HeaderMap::Iterate::Break;
}
rhs_keys.insert(key);
return Http::HeaderMap::Iterate::Continue;
});
return lhs_keys.size() == rhs_keys.size();
return values_match && lhs_keys.size() == rhs_keys.size();
}

bool TestUtility::buffersEqual(const Buffer::Instance& lhs, const Buffer::Instance& rhs) {
Expand Down