Typing #41
Typing #41
Conversation
src/filter/http_context.rs
Outdated
| } | ||
| Some(attribute_bytes) => match self.property_mapper.typed(path, attribute_bytes) { | ||
| Ok(tp) => tp, | ||
| Err(raw) => TypedProperty::string(raw), |
There was a problem hiding this comment.
So, if we fail to map a known property, we try to make it a String which in turn may fail, when there is an invalid byte sequence, which then becomes a Bytes. I added support for that in the as_literal tho unsure what one would do this that...
There was a problem hiding this comment.
Then maybe return Result<Option<TypedProperty>, Vec<u8>> instead of option and leave the decision to the caller?
In any case, what I would do is logging error saying.. "property A could not be parsed".
When the caller is the pattern expression, the error could be "overlooked" as "", while the descriptor builder can fallback to default field value.
There was a problem hiding this comment.
I prefer the current approach which clearly separate the two concerns...
Now I could not fold the error case in the TypedProperty::string function, but I am of the opinion that's the behavior we should aim for: unknown is tried as a String, if it isn't proper UTF-8, we fall back to string encoded bytes (see TypedProperty::as_string or ::as_literal for TypedProperty::Bytes)
| properties.insert( | ||
| "auth.metadata".to_string(), | ||
| Box::new(TypedProperty::complex_map), | ||
| ); | ||
| properties.insert( | ||
| "auth.authorization".to_string(), | ||
| Box::new(TypedProperty::complex_map), | ||
| ); | ||
| properties.insert( | ||
| "auth.response".to_string(), | ||
| Box::new(TypedProperty::complex_map), | ||
| ); | ||
| properties.insert( | ||
| "auth.callbacks".to_string(), | ||
| Box::new(TypedProperty::complex_map), | ||
| ); |
There was a problem hiding this comment.
auth.* are not properties that can be requested directly to the host through the ABI, right? For that reason they were unchecked for "RL" in RFC 0002 for now. Seeing them here makes me think we may want to progress to the point where these attributes are abstractions that translate to metadata.filter_metadata.envoy\.filters\.http\.ext_authz.* perhaps?
Do we need a special TypedProperty function to handle them maybe?
Another option would be the "translation" to happen in the policy controller, and therefore the wasm shim won't ever even need to know about them.
There was a problem hiding this comment.
This actually merely defines the typing... but you are right that I conflated the typing with the host. Filter::get_typed_property is actually what decides where the property is to be sourced.
Maybe I should treat all auth related ones differently... tho they'd still use some typed mapper function just as the envoy properties do... 🤔
| "request.raw_body".to_string(), | ||
| Box::new(TypedProperty::bytes), | ||
| ); | ||
| properties.insert("auth.identity".to_string(), Box::new(TypedProperty::bytes)); |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think that at the http_context.rs level:
- pattern expressions are evaluated with gateway specific implementation returning true or false. It would also require a gateway specific implementation for the parsing of the
valuefield (that could also defined as byte array to be whatever, ie. json int, string, array or object). - on descriptor building, data selectors call gateway specific implementation returning
Option<String>(as RLS only accepts strings). (Or maybeResult<Option<String>, <Vec<u8>>for dealing with parsing errors)
Needless to say the only gateway implementation for now is envoy.
Does it makes sense what I say ❓
| @@ -30,6 +30,8 @@ mod timestamp; | |||
| mod token_bucket; | |||
| mod value; | |||
|
|
|||
| pub mod properties; | |||
There was a problem hiding this comment.
the code from src/envoy has been copied&pasted from somewhere else. What if we "upgrade" this dependency?
| } | ||
| } | ||
|
|
||
| pub fn timestamp(bytes: Vec<u8>) -> Self { |
There was a problem hiding this comment.
I know this is WIP. I would expect this parsing to be according to ProtocolBuffers Timestamp struct. Right?
There was a problem hiding this comment.
Additionally, isn't this Envoy Specific implementation? Do we want to encapsulate envoy specific things?
| } | ||
| } | ||
|
|
||
| impl PartialEq<str> for TypedProperty { |
There was a problem hiding this comment.
why comparing as strings for all cases? Planning to strings types compare with strings, integer types compare with integers, and so on...?
src/filter/http_context.rs
Outdated
| } | ||
| Some(attribute_bytes) => match self.property_mapper.typed(path, attribute_bytes) { | ||
| Ok(tp) => tp, | ||
| Err(raw) => TypedProperty::string(raw), |
There was a problem hiding this comment.
Then maybe return Result<Option<TypedProperty>, Vec<u8>> instead of option and leave the decision to the caller?
In any case, what I would do is logging error saying.. "property A could not be parsed".
When the caller is the pattern expression, the error could be "overlooked" as "", while the descriptor builder can fallback to default field value.
| }, | ||
| let typed_property = match self.get_typed_property(&selector_item.selector) { | ||
| Some(typed_property) => typed_property, | ||
| None => return None, |
There was a problem hiding this comment.
where is &selector_item.default being used when the typed property is not found?
There was a problem hiding this comment.
Not there right now, as it itself introduces a rather non neglectable refactoring of this PR: tl;dr it needs type inference and coercing from the now not String value, but a Value literal.
|
Superseded by #60 |
Still work in progress, if only because of two things:
mainthis returns an empty string on missesas_literalsyntax will need to be supported on the limitador end too