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 <[email protected]>

* Could get confusing with proxy_wasm `Actions` * Also with plugin configuration `Action` Signed-off-by: dd di cesare <[email protected]>

Signed-off-by: dd di cesare <[email protected]>

* GrpcMessage type created Signed-off-by: dd di cesare <[email protected]>

Signed-off-by: dd di cesare <[email protected]>

* Easier to test, mocking fn * Assigned fn on creation, default hostcall and mock on tests Signed-off-by: dd di cesare <[email protected]>

* Bonus: Addressed review regarding testing and Fn types Signed-off-by: dd di cesare <[email protected]>

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 <[email protected]>

[feat] A simplistic approach, agnostic to extension type

1f7b8b3

Signed-off-by: dd di cesare <[email protected]>

[tmp] Allowing dead code

4ce4148

Signed-off-by: dd di cesare <[email protected]>

[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 <[email protected]>

didierofrivia added 12 commits September 6, 2024 18:29

[refactor] Configuration, adding Actions

94bf5d1

Signed-off-by: dd di cesare <[email protected]>

[wip, refactor] GrpcServiceHandler builds message

63a211a

* GrpcMessage type created Signed-off-by: dd di cesare <[email protected]>

[clean] Removing obsolete code

172bdbd

Signed-off-by: dd di cesare <[email protected]>

[refactor] OperationDispatcher triggering procedures

eb8b924

Signed-off-by: dd di cesare <[email protected]>

[refactor] Wiring up altogether

6dc8aee

Signed-off-by: dd di cesare <[email protected]>

[refactor] Implementing own Message for GrpcMessage

fb9ff45

Signed-off-by: dd di cesare <[email protected]>

[refactor] Inlucing Extension within Service and Operation as Rc

64510b0

Signed-off-by: dd di cesare <[email protected]>

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

f0b0648

Signed-off-by: dd di cesare <[email protected]>

[refactor] Wiring up with the new API

a2c69f1

Signed-off-by: dd di cesare <[email protected]>

[refactor] grpc_call function delegated to the caller

159d247

Signed-off-by: dd di cesare <[email protected]>

[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 <[email protected]>

[refactor] Fix OperationDispatcher.next() behaviour

c221036

* Bonus: Addressed review regarding testing and Fn types Signed-off-by: dd di cesare <[email protected]>

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

External Auth ~ish #92

Merged

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