Operation dispatcher FSM #77

didierofrivia · 2024-08-27T14:40:48Z

Part of the work needed for #58

This PR introduces the OperationDispatcher FSM, that it's responsible of not only keeping track of the current state of the operations, but also triggering the actual actions defined by configuration.
It also includes a refactor that was needed in order to wire the Filter, Services, ServiceHandlers and Operations together.

The implementation:

OperationDispatcher:
- its responsible of building the operations and exposing the next function in charge of triggering the current
  Operation procedure
- The next fn will return the current Operation when called.
Operation:
- Exposes its State, Result and Extension
- will trigger the request for the Auth/RateLimit GRPC service
- It will also track and mutate the State (Pending -> Waiting -> Done)
- Responsible of providing the hostcalls fns

Regarding the refactoring of the other pieces:

A new type GrpcMessage was created, implementing the trait Message in order to abstract its logic
GrpcService now keeps a Rc of Extension
GrpcServiceHandler.send method takes as args get_map_values_bytes and grpc_call following DI pattern in order to unit test the operations.
Configuration Actions have been added.

Notes

The wiring only replicates the previous behaviour, still needs to implement the actual flow with multiple actions
The Rules from the PluginConfiguration follows still the old state, duplicating DataType that resides within the Actions
Missing tests for the service module
FOR THE REVIEWERs OF THIS PR: Don't go per commit, just go for the Files Changed unless you want to go through a convoluted collection of changes.

alexsnaps

Left some notes, but I think this is good starting point... Let's iterate!

src/service.rs

src/operation_dispatcher.rs

alexsnaps · 2024-09-05T15:37:35Z

src/service.rs

+    {
+        #[allow(non_upper_case_globals)]
+        static instance: ::protobuf::rt::LazyV2<GrpcMessage> = ::protobuf::rt::LazyV2::INIT;
+        instance.get(|| GrpcMessage::RateLimit(RateLimitRequest::new()))


The requirement for this confuses me... couldn't this "just" be a factory function and be passed around as a fn pointer instead? Not a big deal tho

This one was to please the compiler, got the implementation from the message lib xD

alexsnaps · 2024-09-05T15:41:29Z

src/operation_dispatcher.rs

+        self.operations.borrow().first().unwrap().get_result()
+    }
+
+    pub fn next(&self) -> Option<Operation> {


I think this signature will need to be somewhat rethought... Probably something along the lines of a

.start() that starts a "sequence", initial state (that would only be started in on_request_headers phase)

.next(...) with some params (gRPC response's token_id & response_size?) to go from one operation to the next (that would only ever be progressed in on_grpc_response phase, Pausewith the request)

() i.e. "void"/unit, i.e. nothing left to do, representing the terminated state (transitioned to in the on_grpc_response phase, possibly leaving more work for some other phase to handle, Continue with the request)

Yup, after discussing about how would it be consumed on_grpc_response and the requirements to digest the actual response needing the response_size makes sense. Will shoot a following PR addressing this. Thanks!

Signed-off-by: dd di cesare <didi@posteo.net>

* Could get confusing with proxy_wasm `Actions` * Also with plugin configuration `Action` Signed-off-by: dd di cesare <didi@posteo.net>

Signed-off-by: dd di cesare <didi@posteo.net>

* GrpcMessage type created Signed-off-by: dd di cesare <didi@posteo.net>

Signed-off-by: dd di cesare <didi@posteo.net>

* Easier to test, mocking fn * Assigned fn on creation, default hostcall and mock on tests Signed-off-by: dd di cesare <didi@posteo.net>

* Bonus: Addressed review regarding testing and Fn types Signed-off-by: dd di cesare <didi@posteo.net>

eguzki · 2024-09-06T17:09:39Z

I want to understand and follow. Shouldn't the distpatcher be called on the on_grpc_call_response method of the Context trait?

didierofrivia · 2024-09-09T08:06:06Z

@eguzki you're correct, it should be call on on_grpc_call_response too, this PR is implementing the FSM but not changing the current behaviour. Next PR will address that since the API needs to change a bit, around this #77 (comment)

eguzki · 2024-09-09T16:32:18Z

ok, let's go then. Forget about the clippy thing for now.

eguzki

let's iterate! 🔨

didierofrivia force-pushed the action_dispatcher branch 2 times, most recently from d1d196b to a66a992 Compare August 29, 2024 13:46

didierofrivia changed the base branch from main to external-auth August 29, 2024 13:46

didierofrivia force-pushed the action_dispatcher branch 10 times, most recently from ca3cdff to 876b38f Compare September 4, 2024 13:14

didierofrivia force-pushed the external-auth branch from ed8eec1 to 6c9343d Compare September 4, 2024 15:16

didierofrivia force-pushed the action_dispatcher branch from 90d45ee to b513b69 Compare September 4, 2024 15:16

didierofrivia changed the title ~~[wip, feat] Action dispatcher state machine~~ Action dispatcher FSM Sep 4, 2024

didierofrivia self-assigned this Sep 4, 2024

didierofrivia force-pushed the action_dispatcher branch 2 times, most recently from 88c2caf to 2b288a1 Compare September 4, 2024 18:17

didierofrivia changed the title ~~Action dispatcher FSM~~ Operation dispatcher FSM Sep 4, 2024

didierofrivia marked this pull request as ready for review September 4, 2024 18:35

didierofrivia requested review from adam-cattermole, eguzki and alexsnaps September 4, 2024 18:35

didierofrivia added the enhancement New feature or request label Sep 4, 2024

alexsnaps approved these changes Sep 5, 2024

View reviewed changes

didierofrivia added 4 commits September 6, 2024 18:29

[feat] Action dispatcher state machine, naive impl

cfc7932

Signed-off-by: dd di cesare <didi@posteo.net>

[feat] A simplistic approach, agnostic to extension type

1f7b8b3

Signed-off-by: dd di cesare <didi@posteo.net>

[tmp] Allowing dead code

4ce4148

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Changing name to Operation instead of Action

698384d

* Could get confusing with proxy_wasm `Actions` * Also with plugin configuration `Action` Signed-off-by: dd di cesare <didi@posteo.net>

didierofrivia added 12 commits September 6, 2024 18:29

[refactor] Configuration, adding Actions

94bf5d1

Signed-off-by: dd di cesare <didi@posteo.net>

[wip, refactor] GrpcServiceHandler builds message

63a211a

* GrpcMessage type created Signed-off-by: dd di cesare <didi@posteo.net>

[clean] Removing obsolete code

172bdbd

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] OperationDispatcher triggering procedures

eb8b924

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Wiring up altogether

6dc8aee

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Implementing own Message for GrpcMessage

fb9ff45

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Inlucing Extension within Service and Operation as Rc

64510b0

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] OperationDispatcher.next() returns Option<Operation>

f0b0648

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Wiring up with the new API

a2c69f1

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] grpc_call function delegated to the caller

159d247

Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Operation responsible of providing hostcalls fns

41b920d

* Easier to test, mocking fn * Assigned fn on creation, default hostcall and mock on tests Signed-off-by: dd di cesare <didi@posteo.net>

[refactor] Fix OperationDispatcher.next() behaviour

c221036

* Bonus: Addressed review regarding testing and Fn types Signed-off-by: dd di cesare <didi@posteo.net>

didierofrivia force-pushed the action_dispatcher branch from de5060e to c221036 Compare September 6, 2024 16:29

eguzki approved these changes Sep 9, 2024

View reviewed changes

didierofrivia merged commit 17cc2f0 into external-auth Sep 10, 2024
7 of 8 checks passed

didierofrivia deleted the action_dispatcher branch September 10, 2024 09:02

didierofrivia mentioned this pull request Oct 1, 2024

[DO NOT MERGE YET] External Auth ~ish #92

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operation dispatcher FSM #77

Operation dispatcher FSM #77

didierofrivia commented Aug 27, 2024 •

edited

Loading

alexsnaps left a comment

alexsnaps Sep 5, 2024

didierofrivia Sep 6, 2024

alexsnaps Sep 5, 2024 •

edited

Loading

didierofrivia Sep 6, 2024

eguzki commented Sep 6, 2024

didierofrivia commented Sep 9, 2024

eguzki commented Sep 9, 2024

eguzki left a comment

Operation dispatcher FSM #77

Operation dispatcher FSM #77

Conversation

didierofrivia commented Aug 27, 2024 • edited Loading

The implementation:

Regarding the refactoring of the other pieces:

Notes

alexsnaps left a comment

Choose a reason for hiding this comment

alexsnaps Sep 5, 2024

Choose a reason for hiding this comment

didierofrivia Sep 6, 2024

Choose a reason for hiding this comment

alexsnaps Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

didierofrivia Sep 6, 2024

Choose a reason for hiding this comment

eguzki commented Sep 6, 2024

didierofrivia commented Sep 9, 2024

eguzki commented Sep 9, 2024

eguzki left a comment

Choose a reason for hiding this comment

didierofrivia commented Aug 27, 2024 •

edited

Loading

alexsnaps Sep 5, 2024 •

edited

Loading