Skip to content

matching: only provide string matcher in SinglePredicate#14084

Merged
snowp merged 1 commit intoenvoyproxy:masterfrom
snowp:string-matcher
Nov 19, 2020
Merged

matching: only provide string matcher in SinglePredicate#14084
snowp merged 1 commit intoenvoyproxy:masterfrom
snowp:string-matcher

Conversation

@snowp
Copy link
Copy Markdown
Contributor

@snowp snowp commented Nov 18, 2020

Signed-off-by: Snow Pettersen snowp@lyft.com

Per Slack convo this changes the SinglePredicate default matcher to only match against strings. This simplifies the handling of this default matcher as we can assume that all the inputs are strings and we assume no other types being encoded in the output string. This can be left to extensions that might want to interpret something like JSON values as structured strings.

Risk Level: Low, API not used
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Snow Pettersen <snowp@lyft.com>
@snowp
Copy link
Copy Markdown
Contributor Author

snowp commented Nov 18, 2020

https://envoyproxy.slack.com/archives/CEFDKQ3RQ/p1605272565120300 is the thread in question

fyi @markdroth and more generally @envoyproxy/api-shepherds

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #14084 was opened by snowp.

see: more, trace.

Copy link
Copy Markdown
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. I think we're going to have to include extensions from the get go to bring back functionality that exists in route matching in the range_match field.

@snowp
Copy link
Copy Markdown
Contributor Author

snowp commented Nov 19, 2020

@htuch Yep I agree. Somewhat begs the question of whether everything should be an extension and there be no standard matcher, but I think this should make the simpler matching cases easier to do for users so I think it's worth it.

@markdroth
Copy link
Copy Markdown
Contributor

I think it's fine to use extensions for things like the routing matching range_match field. Keep in mind that the way that's used in route matching, it's for HTTP headers, which means that that matcher is expecting the input to be a string anyway. It will need to internally convert the string to an integer before checking whether it's in the right range.

@snowp snowp merged commit 2f125dd into envoyproxy:master Nov 19, 2020
andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
…14084)

Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Qin Qin <qqin@google.com>
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.

3 participants