🔥 conditions et at, replacing with proper `cel::Predicate`s & `::Expression`s #396

alexsnaps · 2024-11-26T19:09:51Z

limitador/src/counter.rs

alexsnaps · 2024-11-27T20:14:07Z

limitador/src/limit/cel.rs

+}
+
+impl Context<'_> {
+    pub(crate) fn new(limit: &Limit, root: String, values: &HashMap<String, String>) -> Self {


So, this root relates to this point from #385

Should the descriptor(s) and their entries be first class citizens and have their own root binding?

Right now, limit binds the limit info (id & name), I was thinking that possibly we could have some root binding for the descriptor entries (e.g. descriptor), which... could then also be a List and hold multiple of them and all their entries: descriptor[0].foo == '1' && descriptor[1].bar != 'bad'... See this issue here, so it'd probably be a List<HashMap<String, String>> as well then...

Whatever the case, who would set that root namespace/binding to something? I'd expect it to be the server code... but that then creates further issues in the (de)serialization paths... Also, the logic of resolving the Limit.variables() would need to account for that (along side what happens in the wasm-shim's code to lazily resolve attributes from the host).

Also, doing so would tie releasing this with a release of the kuadrant-operator... right now, this isn't necessary.

So... thinking about this a little more, and given this comment, here's how I'm proposing to go about this:

Have a blanket From<&HashMap<String, String> for Context... which would not add the binding for limit.

Change Limit::applies & ::resolve_variables to take a &Context instead of creating it explicitly

~~Have the (Async)RateLimiter::counters_that_apply also take the &Context~~

~~Have the (Async)RateLimiter APIs expose the Context, instead of &HashMap<String, String>~~

Specialize the Context on resolving Predicates only, so the derived cel_interpreter::Context::Child contains the &limit data

Expose some API around Context to set the root(s)

Have the (Async)RateLimiter::counters_that_apply also take the &Context

Have the (Async)RateLimiter APIs expose the Context, instead of &HashMap<String, String>

Wire in the server code

That way, the server code can decide how it wires the RateLimitDescriptors and their .entries from envoy's protocol for external rate limiting.

Have the (Async)RateLimiter::counters_that_apply also take the &Context
Have the (Async)RateLimiter APIs expose the Context, instead of &HashMap<String, String>

duh! this obviously doesn't work 🤦 Or at least not while injecting the Limit this operates on.

alexsnaps · 2024-11-27T20:15:22Z

Pretty big PR, but I tried to keep a somewhat meaningful history and broke it down according to the #385
Thanks... and... sorry 🤦

alexsnaps · 2024-11-27T20:17:39Z

limitador/src/storage/distributed/mod.rs

@@ -333,6 +334,15 @@ fn encode_counter_to_key(counter: &Counter) -> Vec<u8> {
 }

 fn encode_limit_to_key(limit: &Limit) -> Vec<u8> {
-    let counter = Counter::new(limit.clone(), HashMap::default());
+    // fixme this is broken!


This has always been broken... and will need revisiting! But we can't just fake a the variables values like this (and they were simply left empty before, but this won't work now, as they'd fail resolution. I could "just" use the new Counter::resolved_vars, but that would just hide the problem a little more again.

alexsnaps · 2024-11-27T22:30:49Z

limitador/src/counter.rs

+        let var = "timestamp(ts).getHours()";
+        let limit = Limit::new(
+            "",
+            10,
+            60,
+            Vec::default(),
+            [var.try_into().expect("failed parsing!")],
+        );
+        let counter = Counter::new(
+            limit,
+            HashMap::from([("ts".to_string(), "2019-10-12T13:20:50.52Z".to_string())]),
+        )
+        .expect("failed creating counter");
+        assert_eq!(
+            counter.set_variables.get(var),
+            Some("13".to_string()).as_ref()
+        );


So I guess this is now new...

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

limitador-server/examples/envoy.yaml

alexsnaps · 2024-12-03T00:22:39Z

As per this, starting to address these issues

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

alexsnaps · 2024-12-03T12:02:36Z

This now fixes #62 as well

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

alexsnaps · 2024-12-03T12:39:39Z

limitador-server/sandbox/limits.yaml

+    - "descriptors[0]['req.method'] == 'GET'"
+    - "descriptors[0]['req.path'] == '/json'"


This is now a breaking change obviously...
We could tell the server to not do that tho. But this is the only way I can think of to support multiple RateLimitDescriptor coming in from envoy, as well as the fact that each of their entries are String, String... i.e. any valid utf-8 string, which for the key has to be exposed as a Map<String, _> somehow.

For Kuadrant tho, we can now use the limit.id == '3f5e80af-1056-4349-bb84-c8010f34ecbb' if we'd wanted (and wire the id field of the Limit itself in the config)... eventually this semantic could then be optimized for

adam-cattermole

After our discussion the other day and time to go through it, the changes look sound to me, so I'm happy to approve to unblock

eguzki · 2024-12-09T10:40:55Z

Can we tell that this is a breaking change? Just to confirm the breaking-change I just added.

eguzki · 2024-12-09T10:49:13Z

Is there any plan to have some documentation here or somewhere else? So, whenever I want to write some limitador configuration, what context do I have available in CEL expressions? How do I make reference to descriptor entries to write predicates?

eguzki · 2024-12-09T10:50:17Z

After merging this, is it correct to say that Kuadrant's RatelimitPolicies are broken on HEAD of main ?

alexsnaps force-pushed the cel_predicates branch 5 times, most recently from 787bf96 to 9edeb4e Compare November 27, 2024 18:32

alexsnaps commented Nov 27, 2024

View reviewed changes

limitador/src/counter.rs Outdated Show resolved Hide resolved

alexsnaps commented Nov 27, 2024

View reviewed changes

alexsnaps marked this pull request as ready for review November 27, 2024 20:14

alexsnaps requested a review from a team November 27, 2024 20:14

alexsnaps commented Nov 27, 2024

View reviewed changes

alexsnaps mentioned this pull request Nov 27, 2024

CEL Interperter for Conditions #354

Closed

alexsnaps commented Nov 27, 2024

View reviewed changes

alexsnaps force-pushed the cel_predicates branch from 4e2db75 to b318247 Compare November 27, 2024 22:33

alexsnaps added 13 commits November 29, 2024 08:42

Basic cel::Expression & ::Predicate impl

d4296ec

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

Impl Error::source for EvaluationError

83c5f8a

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

Serialization and other traits for CEL types

2a53ce8

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

Replaced Conditions with cel::Predicates

d895098

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

req. is not valid and there is no magic

8488bae

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

🔥 feature lenient_conditions

16ad79e

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

🔥 module crate::limit::conditions et al

e212b0e

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

Inject Limit id & name into Context

d3911b9

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

Slightly rearranged/cleaned deps

a1e9304

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

Parse Limit's Predicates on creation

d60e5f7

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

Parse Limit's variables as exp and resolve

1652ba8

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

key variable with full expression

9fe7d80

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

More time needed sometimes

7c33b77

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

alexsnaps force-pushed the cel_predicates branch from b318247 to 7c33b77 Compare November 29, 2024 13:44

alexsnaps requested review from eguzki and didierofrivia November 29, 2024 13:45

alexsnaps requested a review from adam-cattermole November 29, 2024 13:45

eguzki reviewed Dec 2, 2024

View reviewed changes

limitador-server/examples/envoy.yaml Outdated Show resolved Hide resolved

alexsnaps marked this pull request as draft December 3, 2024 00:22

Opening seam for injectable root

b816ec3

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

alexsnaps force-pushed the cel_predicates branch from 4716e30 to b816ec3 Compare December 3, 2024 00:23

alexsnaps added 5 commits December 2, 2024 20:29

Properly check for variables when they are cel

db9b97f

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

Treat CEL NoSuchKey as ignored false in pred, and miss in keys

ef45382

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

Support for list values as binding to cel::Context

345be88

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

Expose Context all the way up

e0ae4f6

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

Fixed test and take ownership of vars in ctx

7cc4ff4

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

alexsnaps marked this pull request as ready for review December 3, 2024 12:02

alexsnaps requested a review from eguzki December 3, 2024 12:02

alexsnaps force-pushed the cel_predicates branch 2 times, most recently from 103eddf to 2dde346 Compare December 3, 2024 12:18

Wired proper descriptor wiring into context

8941898

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

alexsnaps force-pushed the cel_predicates branch from 2dde346 to 8941898 Compare December 3, 2024 12:19

alexsnaps linked an issue Dec 3, 2024 that may be closed by this pull request

all the descriptors are consolidated in a single descriptor #62

Closed

alexsnaps commented Dec 3, 2024

View reviewed changes

adam-cattermole approved these changes Dec 6, 2024

View reviewed changes

alexsnaps merged commit bf6a26f into main Dec 6, 2024
10 checks passed

alexsnaps deleted the cel_predicates branch December 6, 2024 11:24

eguzki added breaking-change enhancement labels Dec 9, 2024

alexsnaps mentioned this pull request Dec 12, 2024

Limitador v2: Translate RLP's predicates & counter variable to new data "path" Kuadrant/kuadrant-operator#1079

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔥 conditions et at, replacing with proper `cel::Predicate`s & `::Expression`s #396

🔥 conditions et at, replacing with proper `cel::Predicate`s & `::Expression`s #396

alexsnaps commented Nov 26, 2024

alexsnaps Nov 27, 2024

alexsnaps Nov 27, 2024

alexsnaps Dec 2, 2024 •

edited

Loading

alexsnaps Dec 2, 2024

alexsnaps commented Nov 27, 2024

alexsnaps Nov 27, 2024

alexsnaps Nov 27, 2024

alexsnaps commented Dec 3, 2024

alexsnaps commented Dec 3, 2024

alexsnaps Dec 3, 2024

adam-cattermole left a comment

eguzki commented Dec 9, 2024

eguzki commented Dec 9, 2024

eguzki commented Dec 9, 2024

		- "descriptors[0]['req.method'] == 'GET'"
		- "descriptors[0]['req.path'] == '/json'"

🔥 conditions et at, replacing with proper cel::Predicates & ::Expressions #396

🔥 conditions et at, replacing with proper cel::Predicates & ::Expressions #396

Conversation

alexsnaps commented Nov 26, 2024

alexsnaps Nov 27, 2024

Choose a reason for hiding this comment

alexsnaps Nov 27, 2024

Choose a reason for hiding this comment

alexsnaps Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

alexsnaps Dec 2, 2024

Choose a reason for hiding this comment

alexsnaps commented Nov 27, 2024

alexsnaps Nov 27, 2024

Choose a reason for hiding this comment

alexsnaps Nov 27, 2024

Choose a reason for hiding this comment

alexsnaps commented Dec 3, 2024

alexsnaps commented Dec 3, 2024

alexsnaps Dec 3, 2024

Choose a reason for hiding this comment

adam-cattermole left a comment

Choose a reason for hiding this comment

eguzki commented Dec 9, 2024

eguzki commented Dec 9, 2024

eguzki commented Dec 9, 2024

🔥 conditions et at, replacing with proper `cel::Predicate`s & `::Expression`s #396

🔥 conditions et at, replacing with proper `cel::Predicate`s & `::Expression`s #396

alexsnaps Dec 2, 2024 •

edited

Loading