Skip to content

api: adjust the behavior of header's present_match #16627

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
mattklein123 merged 4 commits into from
May 27, 2021
Merged

api: adjust the behavior of header's present_match #16627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4fc7986
api: adjust the behavior when present match's value is false
soulxu May 22, 2021
3ee1b03
add release note
soulxu May 22, 2021
361e895
address comments
soulxu May 24, 2021
99a69d2
address comment
soulxu May 25, 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
4 changes: 2 additions & 2 deletions api/envoy/config/route/v3/route_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1885,8 +1885,8 @@ message HeaderMatcher {
// "-1somestring"
type.v3.Int64Range range_match = 6;

// If specified, header match will be performed based on whether the header is in the
// request.
// If specified as true, header match will be performed based on whether the header is in the
// request. If specified as false, header match will be performed based on whether the header is absent.
Copy link
Copy Markdown
Member

@htuch htuch May 23, 2021

Choose a reason for hiding this comment

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

I think this is technically a data plane behavioral change. That said, I don't see any sensible way that this field could have been configured with false and a meaningful match expected previously, so I don't think we need runtime protection.

Copy link
Copy Markdown

@hanjm hanjm Jul 7, 2022

Choose a reason for hiding this comment

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

the doc is not correct, config present_match: false will not effected, shoud use

present_match: true
invert_match:true

Copy link
Copy Markdown
Member Author

@soulxu soulxu Jul 7, 2022

Choose a reason for hiding this comment

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

@hanjm I tested it on the main branch, it works as expected. I pasted the detail at #17345 (comment)

Copy link
Copy Markdown

@hanjm hanjm Jul 7, 2022

Choose a reason for hiding this comment

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

sorry, my version(1.17) is too low. Thanks you reply.

bool present_match = 7;

// If specified, header match will be performed based on the prefix of the header value.
Expand Down
4 changes: 2 additions & 2 deletions api/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ Minor Behavior Changes
``envoy.reloadable_features.send_strict_1xx_and_204_response_headers``
(do not send 1xx or 204 responses with these headers). Both are true by default.
* http: serve HEAD requests from cache.
* http: the behavior of the *present_match* in route header matcher changed. The value of *present_match* is ignored in the past. The new behavior is *present_match* performed when value is true. absent match performed when the value is false. Please reference :ref:`present_match
<envoy_v3_api_field_config.route.v3.HeaderMatcher.present_match>`.
* listener: respect the :ref:`connection balance config <envoy_v3_api_field_config.listener.v3.Listener.connection_balance_config>`
defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior.
* tcp: switched to the new connection pool by default. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.new_tcp_connection_pool`` to false.


Bug Fixes
---------
*Changes expected to improve the state of the world and are unlikely to have negative effects*
Expand Down
4 changes: 2 additions & 2 deletions generated_api_shadow/envoy/config/route/v3/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions generated_api_shadow/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions source/common/http/header_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ HeaderUtility::HeaderData::HeaderData(const envoy::config::route::v3::HeaderMatc
break;
case envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase::kPresentMatch:
header_match_type_ = HeaderMatchType::Present;
present_ = config.present_match();
break;
case envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase::kPrefixMatch:
header_match_type_ = HeaderMatchType::Prefix;
Expand All @@ -76,6 +77,7 @@ HeaderUtility::HeaderData::HeaderData(const envoy::config::route::v3::HeaderMatc
FALLTHRU;
default:
header_match_type_ = HeaderMatchType::Present;
present_ = true;
break;
}
}
Expand Down Expand Up @@ -135,7 +137,11 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD
const auto header_value = getAllOfHeaderAsString(request_headers, header_data.name_);

if (!header_value.result().has_value()) {
return header_data.invert_match_ && header_data.header_match_type_ == HeaderMatchType::Present;
if (header_data.invert_match_) {
return header_data.header_match_type_ == HeaderMatchType::Present && header_data.present_;
} else {
return header_data.header_match_type_ == HeaderMatchType::Present && !header_data.present_;
}
}

bool match;
Expand All @@ -154,7 +160,7 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD
break;
}
case HeaderMatchType::Present:
match = true;
match = header_data.present_;
break;
case HeaderMatchType::Prefix:
match = absl::StartsWith(header_value.result().value(), header_data.value_);
Expand Down
1 change: 1 addition & 0 deletions source/common/http/header_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class HeaderUtility {
Regex::CompiledMatcherPtr regex_;
envoy::type::v3::Int64Range range_;
const bool invert_match_;
bool present_;

// HeaderMatcher
bool matchesHeaders(const HeaderMap& headers) const override {
Expand Down
47 changes: 45 additions & 2 deletions test/common/http/header_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,9 @@ invert_match: true
EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data));
}

TEST(MatchHeadersTest, HeaderPresentMatch) {
// Test the case present_match is true. Expected true when
// header matched, expected false when no header matched.
TEST(MatchHeadersTest, HeaderPresentMatchWithTrueValue) {
TestRequestHeaderMapImpl matching_headers{{"match-header", "123"}};
TestRequestHeaderMapImpl unmatching_headers{{"nonmatch-header", "1234"},
{"other-nonmatch-header", "123.456"}};
Expand All @@ -512,7 +514,28 @@ present_match: true
EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data));
}

TEST(MatchHeadersTest, HeaderPresentInverseMatch) {
// Test the case present_match is false. Expected false when
// header matched, expected true when no header matched.
TEST(MatchHeadersTest, HeaderPresentMatchWithFalseValue) {
TestRequestHeaderMapImpl matching_headers{{"match-header", "123"}};
TestRequestHeaderMapImpl unmatching_headers{{"nonmatch-header", "1234"},
{"other-nonmatch-header", "123.456"}};

const std::string yaml = R"EOF(
name: match-header
present_match: false
)EOF";

std::vector<HeaderUtility::HeaderDataPtr> header_data;
header_data.push_back(
std::make_unique<HeaderUtility::HeaderData>(parseHeaderMatcherFromYaml(yaml)));
EXPECT_FALSE(HeaderUtility::matchHeaders(matching_headers, header_data));
EXPECT_TRUE(HeaderUtility::matchHeaders(unmatching_headers, header_data));
}

// Test the case present_match is true and invert_match is true. Expected true when
// no header matched, expected false when header matched.
TEST(MatchHeadersTest, HeaderPresentInverseMatchWithTrueValue) {
TestRequestHeaderMapImpl unmatching_headers{{"match-header", "123"}};
TestRequestHeaderMapImpl matching_headers{{"nonmatch-header", "1234"},
{"other-nonmatch-header", "123.456"}};
Expand All @@ -530,6 +553,26 @@ invert_match: true
EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data));
}

// Test the case present_match is true and invert_match is true. Expected false when
// no header matched, expected true when header matched.
TEST(MatchHeadersTest, HeaderPresentInverseMatchWithFalseValue) {
TestRequestHeaderMapImpl unmatching_headers{{"match-header", "123"}};
TestRequestHeaderMapImpl matching_headers{{"nonmatch-header", "1234"},
{"other-nonmatch-header", "123.456"}};

const std::string yaml = R"EOF(
name: match-header
present_match: false
invert_match: true
)EOF";

std::vector<HeaderUtility::HeaderDataPtr> header_data;
header_data.push_back(
std::make_unique<HeaderUtility::HeaderData>(parseHeaderMatcherFromYaml(yaml)));
EXPECT_FALSE(HeaderUtility::matchHeaders(matching_headers, header_data));
EXPECT_TRUE(HeaderUtility::matchHeaders(unmatching_headers, header_data));
}

TEST(MatchHeadersTest, HeaderPrefixMatch) {
TestRequestHeaderMapImpl matching_headers{{"match-header", "value123"}};
TestRequestHeaderMapImpl unmatching_headers{{"match-header", "123value"}};
Expand Down