-
Notifications
You must be signed in to change notification settings - Fork 38
fix(authbridge): Stream text/event-stream responses frame-by-frame #480
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,10 @@ | |
| package extproc | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "io" | ||
| "log/slog" | ||
| "net/http" | ||
| "strconv" | ||
|
|
@@ -20,6 +22,7 @@ import ( | |
| "google.golang.org/grpc/status" | ||
|
|
||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/auth" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/listener/internal/sseframe" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/session" | ||
| ) | ||
|
|
@@ -559,6 +562,16 @@ func (s *Server) handleResponseHeaders(ctx context.Context, headers *corev3.Head | |
| return rejectFromAction(action) | ||
| } | ||
|
|
||
| // Body-less response: deliver an empty last=true frame so | ||
| // StreamingResponder plugins can finalize (and emit no_response_body | ||
| // Skip rows for pairing). Mirrors the buffered-body path's single | ||
| // last=true dispatch. | ||
| if p.HasStreamingResponders() { | ||
| if frameAction := p.RunResponseFrame(ctx, pctx, nil, true); frameAction.Type == pipeline.Reject { | ||
| return rejectFromAction(frameAction) | ||
| } | ||
| } | ||
|
|
||
| // No body phase will run; record the response event here. A2A responses | ||
| // need the body to extract contextId, so the rekey path is body-only; | ||
| // skip it on this header-only path. | ||
|
|
@@ -596,6 +609,22 @@ func (s *Server) handleResponseBody(ctx context.Context, body []byte, pctx *pipe | |
| return rejectFromAction(action) | ||
| } | ||
|
|
||
| // Streaming-aware plugins use a single code path for both shapes | ||
| // (mirrors forwardproxy/reverseproxy). pipeline.RunResponse skips | ||
| // StreamingResponder plugins so they wouldn't get a response-phase | ||
| // dispatch otherwise; deliver the buffered body via RunResponseFrame | ||
| // so mcp/inference/a2a parsers populate their response state and | ||
| // the inbound A2A contextId rekey below sees pctx.Extensions.A2A | ||
| // fully populated. For text/event-stream bodies (Envoy already | ||
| // buffered them at this point), re-parse with sseframe so each | ||
| // event arrives as its own frame; otherwise dispatch the whole | ||
| // body as one last=true frame. | ||
| if p.HasStreamingResponders() { | ||
| if frameAction := dispatchBufferedFrames(ctx, p, pctx); frameAction.Type == pipeline.Reject { | ||
| return rejectFromAction(frameAction) | ||
| } | ||
| } | ||
|
|
||
| // The server's response may carry the server-assigned A2A contextId. If | ||
| // the request phase recorded events under DefaultSessionID (because the | ||
| // client had no contextId yet), migrate them to the real ID so subsequent | ||
|
|
@@ -849,3 +878,51 @@ func getHeader(headers *corev3.HeaderMap, key string) string { | |
| } | ||
| return "" | ||
| } | ||
|
|
||
| // dispatchBufferedFrames feeds the buffered response body to | ||
| // StreamingResponder plugins via RunResponseFrame, mirroring the | ||
| // proxy listeners' single-dispatch contract for buffered bodies. | ||
| // Envoy's ext_proc delivers response bodies pre-buffered (we requested | ||
| // ResponseBodyMode_BUFFERED via ModeOverride), so we get the whole | ||
| // body in one shot regardless of upstream framing. | ||
| // | ||
| // For application/json the entire body is one last=true frame, so | ||
| // non-streaming JSON-RPC responses look the same to plugins as on | ||
| // the proxy listeners. For text/event-stream we re-parse with | ||
| // sseframe so each event arrives as its own non-last frame followed | ||
| // by a final last=true — matches the per-message dispatch shape | ||
| // streaming-aware plugins expect. | ||
| func dispatchBufferedFrames(ctx context.Context, p *pipeline.Holder, pctx *pipeline.Context) pipeline.Action { | ||
| contentType := pctx.ResponseHeaders.Get("Content-Type") | ||
| if isEventStream(contentType) && len(pctx.ResponseBody) > 0 { | ||
| reader := sseframe.NewReader(bytes.NewReader(pctx.ResponseBody), maxBodySize) | ||
| for { | ||
| frame, err := reader.ReadFrame() | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| if err != nil { | ||
| slog.Warn("extproc: SSE re-parse error", "error", err) | ||
| break | ||
| } | ||
| if action := p.RunResponseFrame(ctx, pctx, frame, false); action.Type == pipeline.Reject { | ||
| return action | ||
| } | ||
| } | ||
| return p.RunResponseFrame(ctx, pctx, nil, true) | ||
| } | ||
| return p.RunResponseFrame(ctx, pctx, pctx.ResponseBody, true) | ||
| } | ||
|
|
||
| // isEventStream reports whether a Content-Type header value names the | ||
| // SSE media type. Tolerates parameters and ASCII case differences. | ||
| // Mirrors the helpers in forwardproxy/reverseproxy. | ||
| func isEventStream(contentType string) bool { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. |
||
| if contentType == "" { | ||
| return false | ||
| } | ||
| if idx := strings.IndexByte(contentType, ';'); idx >= 0 { | ||
| contentType = contentType[:idx] | ||
| } | ||
| return strings.EqualFold(strings.TrimSpace(contentType), "text/event-stream") | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking, scope note). Because envoy ext_proc delivers the response
ResponseBodyMode_BUFFERED, this re-parses an already-fully-buffered SSE body. So #477's actual streaming benefit — not holding a slow/large SSEtools/calluntil complete — applies to proxy-sidecar only; in envoy-sidecar mode the whole stream is still buffered (bounded bymaxBodySize= 1 MiB), so the original slow-tool/large-response risk persists there. This is a pre-existing extproc limitation and the right behavior for plugin dispatch — not something to fix in this PR — but worth a doc line or a tracked follow-up so operators know envoy mode isn't streamed end-to-end (it would need ext_proc STREAMED body mode).