Predicate on our "well-known attributes" #113

alexsnaps · 2024-10-19T12:46:46Z

fix to #112

alexsnaps · 2024-10-19T12:47:18Z

This may need to supersede #110 ... stil wip tho

alexsnaps · 2024-10-21T23:04:36Z

tl;dr this replaces PatternExpression with CEL Predicates, while keeping the former as a working fallback...

Of interest if probably this change as well, where the whole fn needs to be replace as if cfg!(test) within the method would work, but won't make the linker happy on anything but wasm...

eguzki · 2024-10-22T15:41:00Z

doc missing. At the very least on README.md.

Doc about routeRuleConditions.predicates list being any or allOf

eguzki · 2024-10-22T15:41:37Z

I would change the PR tittle. That goes on the auto-generated release notes.

eguzki

review still WIP

src/data/attribute.rs

src/configuration.rs

src/configuration/action.rs

alexsnaps · 2024-10-22T17:14:10Z

Just to be clear about the state of this... now that #110 was merged, while broken, let's not merge this quite yet.
I took @didierofrivia work over in supporting "not so well known attribute", i.e. the data coming from authorino. I'll do that in 2 passes, scalar values (e.g. auth.identity.anonymous being resolved as being of type bool and then supporting any expression coming from authorino, expressed as JSON, in there too: List & Object)
I should get the former done by EOD EST, possibly the latter too...

alexsnaps · 2024-10-22T20:30:10Z

this change has the first step, but I think the latter step is not required because of how the struct is decomposed in process_metadata; that being said reading this on the Authorino side, I'm unsure treating all values as String would actually always work. I'd need to delegate to @didierofrivia or possibly @adam-cattermole that did that integration

alexsnaps · 2024-10-23T09:32:37Z

src/data/property.rs

        ["source", "remote_address"] => remote_address(),
+        // todo, unsure whether to drop the "auth" part here...


If auth isn't indeed part of it, which I think it may well not be, then I think it should be added when we store_metadata... rather than shoving what comes back from Authorino in the metadata struct straight into the KUADRANT_NAMESPACE...

eguzki

nice peace of work!

Some minor comments dropped.

Doc is required

src/data/cel.rs

eguzki · 2024-10-23T13:42:27Z

src/data/cel.rs

+    }
+
+    impl AttributeMap {
+        pub fn new(attributes: Vec<Attribute>) -> Self {


Consider adding some comment to explain this high level.

Some example can help like

A map from properties [["auth", "identity", "user"], ["auth", "identity", "group"]] would be { "auth": Node( { "identity": Node( { "user": Value(["auth", "identity", "c"]), "group": Value(["auth", "identity", "group"]) } ) } ) }

src/data/cel.rs

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps · 2024-10-23T19:30:30Z

Alright, it should "all" work. i.e. now we store JSON representation of the auth data coming back from authorino, so to not lose the type information... We even get integers! I also added support for getting to headers...

didierofrivia · 2024-10-23T19:33:34Z

src/data/property.rs

+    }
+}
+
+#[cfg(not(test))]


I like this way of defining the fn when it's meant to be mocked for testing and not. Might be worthy to follow the same pattern for the operation dispatcher and grpc message req/res //TODO:(didierofrivia)

didierofrivia

LGTM. 💪🏼 🥇 🙏🏼

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps · 2024-10-24T10:59:26Z

src/configuration/action.rs

+                        .get()
+                        .expect("Expression must be compiled by now")
+                        .eval()
+                    {


This is how we've send data to limitador so far, losing the type information... just a string: true, 42, bob looks "good" today

alexsnaps · 2024-10-24T11:00:09Z

src/configuration/action.rs

+                        // todo this probably should be a proper string literal!
+                        Value::String(s) => (*s).clone(),
+                        Value::Bool(b) => format!("{b}"),
+                        Value::Null => "null".to_owned(),


Wondering if we should skip that descriptor entry... this could happen if a value is explicitly set to null in auth ...

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps · 2024-10-24T13:05:51Z

Status update: this 🔥 the need for anything but CEL. While the previous ways are still there, they should NOT be used anymore and comes with bunch of caveat... I'll let the code speak for itself as how one can shoot themselves in the foot by going to raw properties/attribute values.

eguzki

Left some comments and food for thoughts.

This is not about requesting changes.

From my side, this is ready to be merged

eguzki · 2024-10-25T07:33:01Z

src/data/attribute.rs

@@ -128,7 +129,7 @@ pub fn set_attribute(attr: &str, value: &[u8]) {
 pub fn store_metadata(metastruct: &Struct) {
    let metadata = process_metadata(metastruct, String::new());
    for (key, value) in metadata {
-        let attr = format!("{KUADRANT_NAMESPACE}\\.{key}");
+        let attr = format!("{KUADRANT_NAMESPACE}\\.auth\\.{key}");


I was thinking that the auth prefix should come from pub fn process_auth_grpc_response method.

Then, pub fn store_metadata(metastruct: &Struct) could be pub fn store_metadata(prefix: &str, metastruct: &Struct)

My diff

diff --git a/src/data/attribute.rs b/src/data/attribute.rs index 9e65caa..7888462 100644 --- a/src/data/attribute.rs +++ b/src/data/attribute.rs @@ -126,10 +126,10 @@ pub fn set_attribute(attr: &str, value: &[u8]) { }; } -pub fn store_metadata(metastruct: &Struct) { +pub fn store_metadata(prefix: &str, metastruct: &Struct) { let metadata = process_metadata(metastruct, String::new()); for (key, value) in metadata { - let attr = format!("{KUADRANT_NAMESPACE}\\.auth\\.{key}"); + let attr = format!("{KUADRANT_NAMESPACE}\\.{prefix}\\.{key}"); debug!("set_attribute: {attr} = {value}"); set_attribute(attr.as_str(), value.into_bytes().as_slice()); } diff --git a/src/service/auth.rs b/src/service/auth.rs index c8e4a4d..8364a8b 100644 --- a/src/service/auth.rs +++ b/src/service/auth.rs @@ -128,7 +128,7 @@ impl AuthService { ) -> Result<(), StatusCode> { if let GrpcMessageResponse::Auth(check_response) = auth_resp { // store dynamic metadata in filter state - store_metadata(check_response.get_dynamic_metadata()); + store_metadata("auth", check_response.get_dynamic_metadata()); match check_response.http_response { Some(CheckResponse_oneof_http_response::ok_response(ok_response)) => {

eguzki · 2024-10-25T07:33:43Z

src/data/attribute.rs

@@ -128,7 +129,7 @@ pub fn set_attribute(attr: &str, value: &[u8]) {
 pub fn store_metadata(metastruct: &Struct) {
    let metadata = process_metadata(metastruct, String::new());
    for (key, value) in metadata {
-        let attr = format!("{KUADRANT_NAMESPACE}\\.{key}");
+        let attr = format!("{KUADRANT_NAMESPACE}\\.auth\\.{key}");
        debug!("set_attribute: {attr} = {value}");


What about this debug! macro in pub fn set_attribute(attr: &str, value: &[u8]) ?

eguzki · 2024-10-25T07:36:23Z

src/data/attribute.rs

@@ -128,7 +129,7 @@ pub fn set_attribute(attr: &str, value: &[u8]) {
 pub fn store_metadata(metastruct: &Struct) {
    let metadata = process_metadata(metastruct, String::new());
    for (key, value) in metadata {
-        let attr = format!("{KUADRANT_NAMESPACE}\\.{key}");
+        let attr = format!("{KUADRANT_NAMESPACE}\\.auth\\.{key}");


Can you please add a comment saying that attributes set here are accessible with wasm.* prefix?

eguzki · 2024-10-25T07:51:50Z

src/configuration.rs

@@ -22,6 +23,23 @@ pub mod action;
 pub mod action_set;
 mod action_set_index;

+#[derive(Deserialize, Debug, Clone)]
+pub struct ExpressionItem {
+    pub key: String,


can we make key optional? it would default to the expression value (not the referenced value, though).

I think it makes for a very horrible default, but... 🤷

alexsnaps · 2024-10-25T09:07:55Z

@eguzki This is my gift to you... you all of course :)
Do whatever you want with the PR! You have my blessing. I need to move on here...

alexsnaps force-pushed the known_attr branch from 5771740 to 204f190 Compare October 19, 2024 12:52

alexsnaps linked an issue Oct 21, 2024 that may be closed by this pull request

CEL expressions #112

Closed

alexsnaps marked this pull request as ready for review October 21, 2024 23:04

alexsnaps mentioned this pull request Aug 9, 2024

CEL Kuadrant/kuadrant-operator#800

Open

9 tasks

alexsnaps force-pushed the known_attr branch from 661dc86 to ff92dc9 Compare October 22, 2024 14:53

eguzki added the enhancement New feature or request label Oct 22, 2024

eguzki reviewed Oct 22, 2024

View reviewed changes

src/data/attribute.rs Outdated Show resolved Hide resolved

src/data/attribute.rs Outdated Show resolved Hide resolved

src/data/attribute.rs Outdated Show resolved Hide resolved

src/configuration.rs Show resolved Hide resolved

src/configuration/action.rs Show resolved Hide resolved

didierofrivia mentioned this pull request Oct 23, 2024

Auth json scalar values #115

Closed

alexsnaps commented Oct 23, 2024

View reviewed changes

alexsnaps changed the title ~~Known attr~~ Predicate on our "well-known attributes" Oct 23, 2024

eguzki requested changes Oct 23, 2024

View reviewed changes

alexsnaps added 13 commits October 23, 2024 11:01

Refactored Attribute to data::AttributeValue

271b464

Signed-off-by: Alex Snaps <[email protected]>

Refactored property stuff to data::property

762091d

Signed-off-by: Alex Snaps <[email protected]>

Refactored using PropertyPath everywhere

78d5eb0

Signed-off-by: Alex Snaps <[email protected]>

Refactored using AttributeValue everywhere

7624900

Signed-off-by: Alex Snaps <[email protected]>

ports are u64, not strings... this err'ed into Default

54ad632

Signed-off-by: Alex Snaps <[email protected]>

Slowly getting there

eede19a

Signed-off-by: Alex Snaps <[email protected]>

Wired the base

222a7cf

Signed-off-by: Alex Snaps <[email protected]>

Clippy fixes

f4fd18f

Signed-off-by: Alex Snaps <[email protected]>

Notes for auth/json integration

34c7050

Signed-off-by: Alex Snaps <[email protected]>

Typos and test infra for hostcalls::get_property

df48e11

Signed-off-by: Alex Snaps <[email protected]>

Predicate test

8c001f3

Signed-off-by: Alex Snaps <[email protected]>

Transparently replace PatternExpression with CEL Predicate

ce49eb2

Signed-off-by: Alex Snaps <[email protected]>

Moar clippy

60820e0

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps added 2 commits October 23, 2024 11:01

Deal with unknown attributes as JSON literals

067aaf2

Signed-off-by: Alex Snaps <[email protected]>

Arguable: but lets fall back to string on bad json

06000e0

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the known_attr branch from 3776b5f to b4d0b8b Compare October 23, 2024 15:02

wasm props are in the filter_state

7dc7ac9

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the known_attr branch from ed2b256 to ae2a298 Compare October 23, 2024 15:52

Deal with more types & json encode values

611a900

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the known_attr branch from ae2a298 to 611a900 Compare October 23, 2024 16:05

alexsnaps added 5 commits October 23, 2024 14:09

Have auth in wasm prop and log on unexpected type

f911798

Signed-off-by: Alex Snaps <[email protected]>

Deal with previously resolved values on path digging into these

dbdf7e1

Signed-off-by: Alex Snaps <[email protected]>

Don't warn on structs when storing values as json

5deae3b

Signed-off-by: Alex Snaps <[email protected]>

Support headers in CEL

a786e94

Signed-off-by: Alex Snaps <[email protected]>

Remove trailing comment

00d4863

Signed-off-by: Alex Snaps <[email protected]>

didierofrivia reviewed Oct 23, 2024

View reviewed changes

didierofrivia approved these changes Oct 23, 2024

View reviewed changes

No more dead code

b63057f

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps commented Oct 24, 2024

View reviewed changes

alexsnaps added 2 commits October 24, 2024 07:41

Support for CEL expressions in data exports

b9af3dc

Signed-off-by: Alex Snaps <[email protected]>

request.time is a Timestamp

7528942

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the known_attr branch from e7c662a to 7528942 Compare October 24, 2024 11:41

README reflecting new config

06cd03d

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps requested review from didierofrivia and eguzki October 24, 2024 12:33

eguzki reviewed Oct 25, 2024

View reviewed changes

eguzki approved these changes Oct 25, 2024

View reviewed changes

eguzki merged commit fcff575 into main Oct 25, 2024
9 checks passed

eguzki deleted the known_attr branch October 25, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Predicate on our "well-known attributes" #113

Predicate on our "well-known attributes" #113

alexsnaps commented Oct 19, 2024 •

edited

Loading

alexsnaps commented Oct 19, 2024

alexsnaps commented Oct 21, 2024

eguzki commented Oct 22, 2024

eguzki commented Oct 22, 2024

eguzki left a comment

alexsnaps commented Oct 22, 2024

alexsnaps commented Oct 22, 2024 •

edited

Loading

alexsnaps Oct 23, 2024

eguzki left a comment

eguzki Oct 23, 2024

alexsnaps commented Oct 23, 2024

didierofrivia Oct 23, 2024

didierofrivia left a comment

alexsnaps Oct 24, 2024 •

edited

Loading

alexsnaps Oct 24, 2024

alexsnaps commented Oct 24, 2024

eguzki left a comment

eguzki Oct 25, 2024

eguzki Oct 25, 2024

eguzki Oct 25, 2024

eguzki Oct 25, 2024

alexsnaps Oct 25, 2024

alexsnaps commented Oct 25, 2024

		["source", "remote_address"] => remote_address(),
		// todo, unsure whether to drop the "auth" part here...

Predicate on our "well-known attributes" #113

Predicate on our "well-known attributes" #113

Conversation

alexsnaps commented Oct 19, 2024 • edited Loading

alexsnaps commented Oct 19, 2024

alexsnaps commented Oct 21, 2024

eguzki commented Oct 22, 2024

eguzki commented Oct 22, 2024

eguzki left a comment

Choose a reason for hiding this comment

alexsnaps commented Oct 22, 2024

alexsnaps commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

eguzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsnaps commented Oct 23, 2024

Choose a reason for hiding this comment

didierofrivia left a comment

Choose a reason for hiding this comment

alexsnaps Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsnaps commented Oct 24, 2024

eguzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsnaps commented Oct 25, 2024

alexsnaps commented Oct 19, 2024 •

edited

Loading

alexsnaps commented Oct 22, 2024 •

edited

Loading

alexsnaps Oct 24, 2024 •

edited

Loading