Skip to content
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.

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

http: add StringMatcher in HeaderMatcher and deprecate the old fields (exact, prefix, etc.) #17119

Merged
merged 16 commits into from
Jul 15, 2021

Conversation

yangminzhu
Copy link
Contributor

@yangminzhu yangminzhu commented Jun 24, 2021

Signed-off-by: Yangmin Zhu [email protected]

Commit Message: Add StringMatcher in HeaderMatcher and deprecate the old fields (exact, prefix, etc.)
Additional Description:
Risk Level: Low
Testing: Unit Tests
Docs Changes:
Release Notes: Added notes for the new field string_match and the deprecation of the old fields (exact, prefix, etc.).
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17119 was opened by yangminzhu.

see: more, trace.

@yangminzhu
Copy link
Contributor Author

Could someone take a look at this? The envoy-presubmit failed seems not related to the PR, thanks.

Copy link
Member

@soulxu soulxu left a comment

Choose a reason for hiding this comment

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

@yangminzhu the envoy-presubmit doesn't seems your fault. :)

source/common/http/header_utility.cc Outdated Show resolved Hide resolved
test/common/http/header_utility_test.cc Outdated Show resolved Hide resolved
test/common/http/header_utility_test.cc Outdated Show resolved Hide resolved
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks, seems reasonable, just some minor comments.
/wait

Signed-off-by: Yangmin Zhu <[email protected]>
@yangminzhu
Copy link
Contributor Author

@htuch @markdroth updated to use StringMatcher now, PTAL, thanks.

@@ -62,8 +62,8 @@ message StringMatcher {
string contains = 7 [(validate.rules).string = {min_len: 1}];
}

// If true, indicates the exact/prefix/suffix matching should be case insensitive. This has no
// effect for the safe_regex match.
// If true, indicates the exact/prefix/suffix/contains matching should be case insensitive. This
Copy link
Contributor Author

@yangminzhu yangminzhu Jun 29, 2021

Choose a reason for hiding this comment

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

note: the containts field is also supported but just not documented in the API.

Signed-off-by: Yangmin Zhu <[email protected]>
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This approach looks great to me! I'll let Harvey do the API approval.

Signed-off-by: Yangmin Zhu <[email protected]>
Signed-off-by: Yangmin Zhu <[email protected]>
Signed-off-by: Yangmin Zhu <[email protected]>
@yangminzhu
Copy link
Contributor Author

@htuch would you mind to take a look at the PR again? I changed to use StringMatcher directly suggested by Mark and updated existing config and code to use it accordingly, thanks.

Signed-off-by: Yangmin Zhu <[email protected]>
@yangminzhu yangminzhu changed the title http: support ignore_case for exact/prefix/suffic/contain match in HeaderMatcher http: add StringMatcher in HeaderMatcher and deprecate the old fields (exact, prefix, etc.) Jul 1, 2021
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, just two small things.

test/tools/router_check/router.cc Outdated Show resolved Hide resolved
test/common/http/header_utility_test.cc Show resolved Hide resolved
Signed-off-by: Yangmin Zhu <[email protected]>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, just some small test questions.
/wait

@@ -201,7 +201,7 @@ name: test-header
EXPECT_EQ(HeaderUtility::HeaderMatchType::Present, header_data.header_match_type_);
}

TEST(HeaderDataConstructorTest, ExactMatchSpecifier) {
TEST(HeaderDataConstructorTest, DEPRECATED_FEATURE_TEST(ExactMatchSpecifier)) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm still vaguely curious why this was passing CI (with compile_time_options rejecting deprecated fields)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, by looking at the code, parseHeaderMatcherFromYaml() calls the TestUtility::loadFromYaml(yaml, header_matcher) which seems essentially called the MessageUtil::loadFromYaml() that does not check for the deprecated fields?

test/common/http/header_utility_test.cc Show resolved Hide resolved
];

// If specified, header match will be performed based on the string match of the header value.
type.matcher.v3.StringMatcher string_match = 13;
Copy link
Contributor

Choose a reason for hiding this comment

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

@snowp is copying the StringMatcher proto to the xds repo in cncf/xds#8. It might be a good idea to wait for that to land and then use the new type here, since that will save us the trouble of migrating this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the current StringMatcher is used everywhere in many places, is it better to wait and change all usages at once? It is already a large change for control plane to use the StringMatcher to replace the deprecated fields, just want to make it less complicated by not depending on the extra xds repo for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that @snowp was going to change the existing code that uses StringMatcher to be templated so that it would work with either copy of StringMatcher. Once that's done, I think it should be trivial to use the new type here.

I think this just comes down to a question of timing. Snow, how soon do you think you can land the StringMatcher change, including any necessary templatizing of code in Envoy?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm busy the next two days for company perf season, but I'll have more time to work on it starting Wednesday. Early review of #17096 could be helpful in speeding this up to make sure that there is agreement on the direction.

@yangminzhu
Copy link
Contributor Author

I'm not sure why the following tests only failed in CI but passed in my local run, @htuch @mattklein123 @qiwzhang do you might have any ideas about this? I checked the jwt filter code but has no idea how is it even affected by this PR. thanks.

[ RUN      ] Protocols/RemoteJwksIntegrationTest.WithGoodTokenAsyncFetchFast/IPv4_HttpDownstream_Http2Upstream
[2021-07-09 23:02:04.849][15090][critical][assert] [test/integration/http_integration.cc:470] assert failure: 0. Details: Timed out waiting for new connection.
[ RUN      ] Protocols/RemoteJwksIntegrationTest.WithGoodTokenAsyncFetchFast/IPv4_HttpDownstream_Http2Upstream
[       OK ] Protocols/RemoteJwksIntegrationTest.WithGoodTokenAsyncFetchFast/IPv4_HttpDownstream_Http2Upstream (68 ms)
[ RUN      ] Protocols/RemoteJwksIntegrationTest.WithGoodTokenAsyncFetchFast/IPv4_Http2Downstream_HttpUpstream
[2021-07-09 23:02:22.138][16531][critical][assert] [test/integration/http_integration.cc:470] assert failure: 0. Details: Timed out waiting for new connection.

@htuch
Copy link
Member

htuch commented Jul 12, 2021

@yangminzhu not sure, but with integration tests, machine load and timing can have an impact on flaky tests. Try the advice at https://github.com/envoyproxy/envoy/blob/main/test/integration/README.md#debugging-integration-tests.

@yangminzhu
Copy link
Contributor Author

@yangminzhu not sure, but with integration tests, machine load and timing can have an impact on flaky tests. Try the advice at https://github.com/envoyproxy/envoy/blob/main/test/integration/README.md#debugging-integration-tests.

Thanks for the suggestion. I reproduced the test flaky with --runs_per_test=1000 in a clean repo at main branch which makes me think this PR is unrelated to the test failure. I created #17303 for tracking the issue.

@mattklein123 @htuch Could you approve the PR if there are no other comments so that we can get this merged while @qiwzhang is working on the test flaky? thanks.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mattklein123 mattklein123 merged commit 68e38cb into envoyproxy:main Jul 15, 2021
@yangminzhu yangminzhu deleted the header-matcher branch July 15, 2021 23:14
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants