small refactor of pluggable bbr interfaces#2513
small refactor of pluggable bbr interfaces#2513k8s-ci-robot merged 14 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold I'm iterating on this a bit more to make it cleaner with the other parts of the code. |
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
| before := time.Now() | ||
| updatedHeaders, updatedBody, err = plugin.Execute(ctx, updatedHeaders, updatedBody) | ||
| metrics.RecordPluginProcessingLatency(executeExtensionPoint, plugin.TypedName().Type, plugin.TypedName().Name, time.Since(before)) | ||
| mutatedRequest, err = plugin.ProcessRequest(ctx, request) |
There was a problem hiding this comment.
when executing a chain, does the N+1 plugin see mutations done by Plugin N? Should it? If not, how do you "merge" mutations from different plugins?
There was a problem hiding this comment.
yes. plugin N+1 should see mutations of plugin N.
the order of execution is defined by whoever configures bbr. then plugins are running sequentially and may override the previous plugins.
latest update holds.
| for _, plugin := range s.responsePlugins { | ||
| log.FromContext(ctx).V(logutil.VERBOSE).Info("Executing response plugin", "plugin", plugin.TypedName()) | ||
| before := time.Now() | ||
| mutatedResponse, err = plugin.ProcessResponse(ctx, response) |
There was a problem hiding this comment.
Same question regarding processing of mutations in a chain.
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
f38b742 to
5a6e886
Compare
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
|
PR is ready for another review iteration. |
| // Payload processor can mutate the headers and/or the body of the message. | ||
| Execute(ctx context.Context, headers map[string]string, body map[string]any) (updatedHeaders map[string]string, updatedBody map[string]any, err error) | ||
| // ProcessRequest runs the RequestProcessor plugin. | ||
| // RequestProcessor can mutate the headers and/or the body of the request. |
There was a problem hiding this comment.
clarify what an error return means (abort chained processing, return error to Envoy to abort transaction, ignore this plugin, ...)
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
| func (r *InferenceMessage) RemoveHeader(key string) { | ||
| if _, ok := r.Headers[key]; ok { | ||
| delete(r.Headers, key) | ||
| delete(r.mutatedHeaders, key) // avoid sending set and remove for same key | ||
| r.removedHeaders.Insert(key) | ||
| } | ||
| } |
There was a problem hiding this comment.
@elevran eventually I've added also the removed headers cause that was a very simple change.
(less than 20 LoC).
Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
* small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> Signed-off-by: Abdallah Samara <abdallahsamabd@gmail.com>
* small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> Signed-off-by: Abdallah Samara <abdallahsamabd@gmail.com>
* small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
* small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
…pi-inference-extension#2513) * small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
…pi-inference-extension#2513) * small refactor of pluggable bbr interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor update Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * godoc Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * iterate on interfaces Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * more updates Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * code review Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * typo Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor change towards adding deleted headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * make linter happy Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * linter and tests Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * embed InferenceMessage in request/response Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * add support for removing headers Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> * minor tweak Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com> --------- Signed-off-by: Nir Rozenbaum <nrozenba@redhat.com>
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
small refactor of pluggable bbr plugins framework.
add request/response types and updated interfaces to make it possible for a single plugin to have both request/response hooks.
pay attention that pluggable bbr is still not wired.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: