Add pattern to string.proto #40
Conversation
xds/type/matcher/v3/string.proto
Outdated
| // | ||
| // Examples: | ||
| // | ||
| // abc/{lang}/{county} matches the value abc/us/eng |
There was a problem hiding this comment.
what are the semantics of a "pattern"?
There was a problem hiding this comment.
+1.
Can you clarify where lang/county categories and their possible values are defined?
There was a problem hiding this comment.
I've added clarify in comments/code above
|
cc @adisuissa |
Signed-off-by: silverstar195 <seanmaloney@google.com>
Signed-off-by: silverstar195 <seanmaloney@google.com>
Signed-off-by: silverstar195 <seanmaloney@google.com>
Signed-off-by: silverstar195 <seanmaloney@google.com>
| // abc/{lang}/{county} matches the value abc/eng/us | ||
| // Note format of: abs/<user-defined-var>/<user-defined-var> | ||
| // {lang} == "eng" and {country} == "us" | ||
| string pattern = 8 [(validate.rules).string = {min_len: 1}]; |
There was a problem hiding this comment.
Sorry I think I'm missing something.
Is the idea to match a string given some pattern, and assign the values to their variables (e.g., lang will be "eng"), or are there known categories and known possible values for these categories, and a match will occur if the used values are in the categories.
There was a problem hiding this comment.
The will be matched and assigned to variables. No know categories.
There was a problem hiding this comment.
How would the string "abcdef" match against pattern="abc{first}{second}", as the possible assignments are possible:
first="de", second="f"
first="d", second="ef"
?
There was a problem hiding this comment.
This would not be a valid config. Only items between "/"s are used as vars.
There was a problem hiding this comment.
This is an interesting use-case, and I'm not sure if it should be specified as part of the StringMatcher (the StringMatcher just matches according to some pattern, and doesn't assign values to external variables).
@snowp was this kind of use-case ever considered?
There was a problem hiding this comment.
This type of matching seems very specific to path matching, but StringMatcher can be used for things other than paths, so I'm not sure this is really the right place for this. I think it would make more sense to create a new matcher for this purpose.
There was a problem hiding this comment.
It can be used in any context where simplified wildcard matching based on a delimiter is needed.
There was a problem hiding this comment.
But the delimiter is fixed to slash, and only wildcards or strings w/o wildcards are allowed in between.
There was a problem hiding this comment.
I am ok with expanding the / to be generalized. There is a lot of value here by providing simplified regex style matches.
There was a problem hiding this comment.
I think that even with making this more generalized, it still seems to be a fairly special case. I think it makes more sense for this to be a separate matcher, just like RegexMatcher is.
Signed-off-by: Sean Maloney <seanmaloney@google.com>
xds/type/matcher/v3/string.proto
Outdated
| // | ||
| // Examples: | ||
| // | ||
| // abc/*/* matches the value abc/eng/us |
There was a problem hiding this comment.
Each example should be in bullet (line prefix with *). Also each of the actual pattern/value should be italic (reference). See rendered output of previous docs: https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/matcher/v3/string.proto
Unfortunately I don't think the cncf repo runs the docs rendering script in a PR. @phlax is this correct? How do we make sure these docs are rendered well?
There was a problem hiding this comment.
envoy pulls in the xds protos as a dep so when the dep is updated the newer protos are rendered
if you need to test a PR ahead of this you can create a PR in envoy and point the dep at the PR (commit hash for PR mostly will work as release version in bazel/repository_locations.bzl)
xds/type/matcher/v3/string.proto
Outdated
| // Note format of: abs/<wildcard-1>/<wildcard-2> | ||
| // {wildcard-1} == "eng" and {wildcard-2} == "us" |
There was a problem hiding this comment.
I think that wildcard-1/2 makes this more confusing, as the matcher only matches the entries.
xds/type/matcher/v3/string.proto
Outdated
| // Note format of: abs/<wildcard-1>/<wildcard-2> | ||
| // {wildcard-1} == "eng" and {wildcard-2} == "us" | ||
| // | ||
| // abc/foo matches the value abc/foo |
Signed-off-by: Sean Maloney <seanmaloney@google.com>
No description provided.