xdsmatcher: add evaluation framework for xDS matching API#511
xdsmatcher: add evaluation framework for xDS matching API#511snowp merged 5 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
@alecholmez wanna give this one a look? LMK how I can help moving this forward |
|
Maybe @howardjohn if you're interested in this bit as well, I'd appreciate the eyes :) |
Yup looking this over, sorry got a bit wrapped up |
alecholmez
left a comment
alecholmez
left a comment
There was a problem hiding this comment.
I think overall this looks nice. The API is pretty friendly so long as you understand the original matching API. Overall nothing major from my end. Maybe just some more comments here and there and beefing up the docs for godoc but other than that it looks good!
| // report this back up. | ||
| if result.NeedMoreData { | ||
| m.logger.log("matcher tree requires more data") | ||
| return result, nil |
There was a problem hiding this comment.
If this is a failure does it warrant an error returned?
There was a problem hiding this comment.
as i understand it a result with "needs more data" is valid, might just need a small comment adjustment
There was a problem hiding this comment.
Yeah this is not an error, it's the complex data type understanding that this data may change in the future, so it's unable to conclusively say that the data doesn't match.
An example here is how this is used for matching HTTP streams in Envoy: if you run into a matcher that tries to match against trailers before trailers are seen, we respond "NeedMoreData" so that the caller knows to call this again when trailers are made available.
| } | ||
|
|
||
| // Internal logger that helps debugging by providing indention during matching to provide some visual guidance to the current nesting level. | ||
| // TODO(snowp): Make this optional, not ideal if this ends up being used outside of testing scenarios. |
There was a problem hiding this comment.
Should we pull this out before it gets merged? Or at least make sure its optional?
| @@ -0,0 +1,33 @@ | |||
| package types | |||
There was a problem hiding this comment.
Maybe some comments in this file above the important types/interfaces might be useful? Like the Matcher interface for example. Just so it shows up nicely in godoc for people consuming the framework
| return fooInput{}, nil | ||
| } | ||
|
|
||
| // Implement types.DataInput for the input type. |
There was a problem hiding this comment.
Just so I understand correctly, is the InputType the registered type URL in the init() function? I'd assume so since we are attempting a cast below that never fails
There was a problem hiding this comment.
To make things super generic, the entire tree acts against a specific InputType (simple case here is just a string, but can be a complex data structure). Once the tree is built, you pass a value of type InputType to it, which is then passed to all DataInputs (which tell us how to extract a specific value out of the complex data type).
So the InputType is never registered, but you must be defining DataInputs that all support the specific type that you provide to the matcher on evaluation time
| r, _ := m.Match(data) | ||
| if r.MatchResult != nil && r.MatchResult.Action != nil { | ||
| // resulting action | ||
| action, ok := r.MatchResult.Action.(action) |
There was a problem hiding this comment.
I saw that Actions are interfaces, what kind of things could we do on this resulting variable? Or would that be for the consumer of this API to decide?
There was a problem hiding this comment.
Yeah they are opaque objects that are defined using extension points in the config. You'd expect to check the type of these by casting to the type you might care about, then you can do whatever you want using the resulting casted type (which may have more interesting fields/functions defined)
|
Updated, ptal @alecholmez |
|
ping @alecholmez |
3 participants
No description provided.