Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor entire filter flow #161

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Refactor entire filter flow #161

wants to merge 21 commits into from

Conversation

adam-cattermole
Copy link
Member

@adam-cattermole adam-cattermole commented Jan 16, 2025

Changes

In this PR I've refactored the existing approach which had become somewhat unwieldy. I have tried to clean up the commits here, but it's worth noting this was an iterative process, and so some of the changes in the initial commits are changed/reverted in subsequent ones.

The main principle is as follows - I have aimed to reduce the leaking of the wasm/envoy hostcalls across every layer and instead delegate this to the filter. The filter has knowledge of the different 'phases' (on_http_request.. on_grpc_call_response..), and simply just performs Operations based on it's current state and what it is being asked to do. The configuration itself remains unchanged, and the concerns of building and consuming of the specialised message types is delegated to the owner (auth_action/rl_action). The filter has no knowledge of the individual actions but instead uses the RuntimeActionSet to initiate the flow, and from then on the Operations to determine what to do next.

Each of the operations tell the filter it must do something (that the layers below should not and do not know about):

  • SendGrpcRequest: This informs the filter it needs to make a GRPC call and contains all of the the information it requires to make the request - the filter does not need to know which action/service this relates to
  • AwaitGrpcResponse: This tells the filter that there is nothing left to do within the current method and it must pause to await a GRPC response
  • AddHeaders: This tells the filter it must add headers to the response in the required phase
  • Die: This informs the filter that it is finished due to an error and must send a response before it finishes - this may not be an "error" due to a failure, but is also used for the case where we must return an error code to the caller (i.e. 401 Forbidden, 429 Too Many Requests..)
  • Done: This informs the filter that it has successfully finished operating and the request may be resumed

The specialised actions currently handle both the building of messages to the transfer type, and conversion to the message they expect. I have also made some decisions on how these actions return information to the layers above, based on the different operations the filter may have to perform.

Outstanding

There are several outstanding things to address (in follow up work):

  • Currently some parts of the logic are based around whether there is a message to send or not, this could be reworked to determine whether we have anything to do, for example our action just requires us to add headers and not send a message
  • Whilst the hostcalls have been removed in lots of places throughout the code, there are still uses through the set/get_property calls, this would ideally be replaced with some kind of resolver layer with memoization
  • To keep an abstraction between the KuadrantFilter, Operations, and RuntimeActions there was a need to retain the index of the current active RuntimeAction within some Operations (to progress the flow). I would like to investigate eliminating this, potentially by building the chain of Operations up front (where each contains self + next). Another alternative might be implementing the iterator trait so we can just call .next() to go through each

alexsnaps and others added 4 commits January 16, 2025 13:28
Co-authored-by: Adam Cattermole <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Adam Cattermole <[email protected]>
Signed-off-by: Adam Cattermole <[email protected]>
@adam-cattermole adam-cattermole added the enhancement New feature or request label Jan 16, 2025
@adam-cattermole adam-cattermole self-assigned this Jan 16, 2025
Signed-off-by: Adam Cattermole <[email protected]>
Signed-off-by: Adam Cattermole <[email protected]>
Digest generates a Vec of operations we iterate over

Signed-off-by: Adam Cattermole <[email protected]>
Signed-off-by: Adam Cattermole <[email protected]>
Signed-off-by: Adam Cattermole <[email protected]>
@adam-cattermole adam-cattermole marked this pull request as ready for review January 17, 2025 13:53
@adam-cattermole adam-cattermole requested a review from a team as a code owner January 17, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

2 participants