Add publisher delete and queryable reply messages to ACL #1259
Conversation
|
Requesting reviews from @Mallets @OlivierHecart @fuzzypixelz @gabrik |
fuzzypixelz
requested review from
OlivierHecart,
fuzzypixelz,
gabrik and
Mallets
and removed request for
OlivierHecart
oteffahi
changed the title
| @@ -718,7 +718,7 @@ pub(crate) fn route_send_response( | |||
| ext_tstamp, | |||
| ext_respid, | |||
| }, | |||
| "".to_string(), // @TODO provide the proper key expression of the response for interceptors | |||
| key_expr.to_string(), | |||
There was a problem hiding this comment.
This allocates everytime in the critical path. Why it should return a String and not a keyexpr?
There was a problem hiding this comment.
It's because RoutingContext<Msg> expects a String. In other instances where this is called no additional allocation is made, but here it is the case...
For more detail see here: https://github.com/eclipse-zenoh/zenoh/blob/main/zenoh/src/net/routing/mod.rs
There was a problem hiding this comment.
Let's then return empty String for the time being until a proper solution is found.
Does returning an empty String impact the behaviour?
There was a problem hiding this comment.
Returning an empty string breaks the keyexpr matching (empty string does not match with any key expression, even **).
Also, rules are made on key expressions, so having an empty string on all replies would impact the behavior of ACL
There was a problem hiding this comment.
Ok, understood. Can you then verify the impact on performance?
There was a problem hiding this comment.
I'll look into it 👍
There was a problem hiding this comment.
Due to high impact on performance, this change will be reverted. We can use Response{ wire_expr, ... } in ACL code to only get the keyexpr when needed.
… filtering of reply messages This reverts commit 3a78d5d.
Ingress reply messages are not affected by the unimplemented key_expr in routing/dispatcher/queries.rs
| // @TODO: Remove wire_expr usage when issue #1255 is implemented | ||
| if self.action(AclMessage::Reply, "Reply (egress)", wire_expr.as_str()) | ||
| == Permission::Deny | ||
| { |
There was a problem hiding this comment.
This will have to be adressed once #1255 is implemented.
Adds filtering of publisher delete and queryable reply messages to ACL. Follow-up to PR #1200
Example of ACL rules with delete and reply messages:
{ "access_control": { "enabled": true, "default_permission": "deny", "rules": [ { "id": "allow queryables", "permission": "allow", "flows": ["ingress", "egress"], "messages": [ "query", "reply", "declare_queryable" ], "key_exprs": [ "**" ] }, { "id": "allow delete", "permission": "allow", "flows": ["ingress", "egress"], "messages": [ "delete", ], "key_exprs": [ "**" ] }, ], // rest of ACL config } }