-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Changes from 10 commits
2d1ed7f
e9ba434
548d08c
ed274b0
affb6b3
91baeaa
2eb8966
1a1de1e
b2bcee5
36f2acc
7c05be9
b8fd421
9841e14
92284de
ee35d9d
5a92d83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: the |
||
// has no effect for the safe_regex match. | ||
// For example, the matcher *data* will match both input string *Data* and *data* if set to true. | ||
bool ignore_case = 6; | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ofStringMatcher
. 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?There was a problem hiding this comment.
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.