Repository navigation
Conversation
Extract the protobuf contract, RFC 0009 update, and published docs from the HTTP session hooks change for API review. The implementation stays in the full change. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-5087.docs.buildwithfern.com/openshell |
BUFFERED becomes WHOLE_BODY and STREAM becomes DUPLEX, before the HTTP session hooks ship. WHOLE_BODY matches the v1 WHOLE_BODY_BYTES name, and DUPLEX leaves room for a later PER_CHUNK mode with one result per input chunk, like Envoy ext_proc STREAMED, without two modes both called streaming. Renames the proto enum values, HttpWholeBodyMode, HttpDuplexMode, HttpWholeBody, HttpWholeBodyResult, the whole_body and whole_body_result fields, and max_whole_body_bytes, along with the Rust names, the content-guard body_mode values, tests, and docs. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
The capability that selects the streaming HTTP RPCs is now openshell.supervisor-middleware.http-hooks. "Session" read as a cookie-style HTTP session and clashed with the WebSocket session RPC. Docs, comments, and errors call the new contract "HTTP hooks" and the deprecated contract "legacy HTTP hooks" instead of "v1". HttpHookVersion and ChainHttpHookVersion rename V1 to Legacy, with the audit label legacy_http_hook; the new contract's label is http_hook. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Finish the http-hooks rename so no HTTP hook name still says "session":
- Service HttpSessionMiddleware is now HttpHooks; Http{Request,Response}
Session{Event,Result,Preflight} messages are now Http*Hook*.
- http_session modules, the e2e test, and the mise task and CI suite are
now http_hooks / middleware-http-hooks.
- HttpHookVersion and ChainHttpHookVersion rename Session to Current;
HeaderAuthority renames Session{Request,Response} to Hook*.
- HttpRequestSession and HttpResponsePipelineSession are now
HttpRequestPipeline and HttpResponsePipeline, carried in a `pipeline`
field; MAX_CONCURRENT_HTTP_MIDDLEWARE_SESSIONS is now ..._MESSAGES.
Shared stream names (MiddlewareSessionEnd, session_capacity_exhausted)
and WebSocket session names are unchanged.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
"Messages" and "streams" read as gRPC messages and streams. The two
budgets are buffering slots (preflight and WHOLE_BODY) and streaming
slots (DUPLEX or deferred stages), as the docs already call them:
- MAX_CONCURRENT_HTTP_MIDDLEWARE_MESSAGES -> MAX_HTTP_HOOK_BUFFERING_SLOTS
- MAX_CONCURRENT_HTTP_MIDDLEWARE_STREAMS -> MAX_HTTP_HOOK_STREAMING_SLOTS
- http_{hooks,stream}_admission -> http_{buffering,streaming}_admission,
with matching reserve helpers and debug fields.
The streaming budget comment now says deferred stages also take a slot,
matching may_stream(). The docs limits row counts HTTP requests and
responses instead of messages.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
changed the title
Shorten the max_payload_bytes descriptions, drop the selector upgrade note and the regex overlap example, and split the RFC 0009 HTTP hooks update into a structured list. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…nfiguration Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…guration Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…ration Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Collaborator
Author
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note
Review-only draft. Do not merge. This PR contains only the API contract from #4359: the protobuf changes, the RFC 0009 update, and the published docs. The implementation, tests, and examples stay in #4359, and any changes agreed here land there.
Summary
Defines HTTP hooks: streaming HTTP request and response hooks for supervisor middleware that replace the v1 HTTP hooks. See #4359 for the motivation and the full change.
Why
The legacy hooks were built for small, buffered checks. The use cases we want next, such as signing a Git push or a large AWS request, need the whole body regardless of size and need to change the body and its headers together.
EvaluateHttpRequestis unary: the whole body in one message, at most 4 MiB, under one RPC deadline. A larger request fails withrequest_body_over_capacity. HTTP hooks can stream a body with no total size or time cap.Content-Lengthor a SigV4 payload hash. An HTTP hook stage can send header mutations after it has read the complete body.HttpResponsePreReturnis lockstep: each body unit gets exactly one result, and a service can't hold bytes for a later unit. In DUPLEX mode, input and output chunks are independent, so a service can redact across chunk boundaries or re-frame a stream.Why not extend the legacy hooks. A unary RPC can't become a stream without breaking every deployed service. Lockstep is a semantic guarantee, not a field, so relaxing it on the same RPC would change behavior that
buf breakingcan't detect. Older peers would also ignore new fields and call an extended service with legacy semantics. A separate service gated by a required capability makes older peers refuse it, and the legacy hooks keep working unchanged until they are removed.Tip
The walkthrough steps through the contract message by message, with a
WHOLE_BODYrequest, aDUPLEXresponse, and aDUPLEXrequest that writes headers after reading the body.What to review
proto/supervisor_middleware.proto: the newHttpHooksservice (EvaluateRequest,EvaluateResponse), the preflight, body, finish, and deny messages, theWHOLE_BODYandDUPLEXbody modes, and thedeprecatedmarkers on the v1 HTTP hooks.docs/extensibility/supervisor-middleware/operations.mdx: the user-facing contract, including lifecycle, body modes, limits, failure codes, and v1 migration.rfc/0009-supervisor-middleware/README.md: contract versioning through required capabilities, and removal of deprecated elements in a minor release.Services opt in by listing the
openshell.supervisor-middleware.http-hookscapability in their extensionrequired_capabilities. That capability constant is defined in Rust (openshell-core), so it does not appear in this diff.Related Issue
Part of #2431 and #3307.
Testing
mise run proto:lintpassesmise run docs:build:strictreports no errorsmise run pre-commit: expected to fail. Existing code still calls the v1 RPCs that this proto marksdeprecated, sorust:lintfails with-D warnings. feat(middleware): introduce improved HTTP request/response hooks for the middleware #4359 contains the code that handles this.Checklist