Repository navigation
feat(agent-gateway): configure bounded remote MCP payload budgets - #8411
hanzhe-one wants to merge 2 commits into
Conversation
Add trusted per-Server remoteRequestBytes/remoteResponseBytes with 8 KiB defaults and hard maxima, validated by the shared Admin/gateway parser and captured in the immutable remote binding. Reject declared or streamed bodies at receipt, refuse non-identity encodings and keep the entry budget independent. Fixes apache#8391
wy471x
left a comment
wy471x
left a comment
There was a problem hiding this comment.
The validation plumbing (shared parser, default 8 KiB, hard maxima, request bytes counted before any side effect, header-first body rejection) looks solid. A few points that should be resolved before merge:
-
Budgets in
RemoteServerBinding.Configequality break third-party credential resolvers.RemoteServerBinding.resolverequiresconfig.equals(credential.target). Only the built-inFileRemoteServiceCredentialResolverwas updated to copytarget.requestBytes()/responseBytes()into theConfigit rebuilds.RemoteServiceCredentialResolveris a public extension point (documented in the README), and any resolver written against the previous pattern — buildingnew Config(name, endpoint, credentialRef, credentialVersion)from its own file, exactly what the file resolver used to do — now silently gets the 8192 defaults and fails the binding check withSecurityException("Missing or mismatched service credential binding")for every non-default budget. The symptom is a dropped data-sync update with no hint about the real cause. Consider excluding the budgets from the credential-binding comparison (compare name/endpoint/ref/version explicitly) instead of requiring every resolver to mirror them. -
README vs. behaviour. The new section says the per-Server remote limit "also applies after entry transformation when the final rendered response is checked". It does not: the final rendered-response check uses the rule handle's
maxResponseBytes(AgentHttpToolExecutor), notremoteResponseBytes. RaisingremoteResponseBytesabove the handle limit therefore still rejects, contrary to the documentation. -
Range top is unusable and unbounded.
MAX_REMOTE_RESPONSE_BYTESequals the default entrymaxResponseBytes(1 MiB), so a response at the documented maximum cannot pass the entry check once the JSON-RPC envelope is counted; the top of the advertised range only works if the rule handle is also raised, and nothing validates that pairing. At the same time, 1 MiB ×MAX_IN_FLIGHT(64) × up to 8 Servers is ~512 MiB per node with no aggregate cap. Please document the relationship to the entry budget and state the memory bound (or add one). -
Minor:
RemoteServerBinding.Configuses inline FQNs forAgentGatewayAggregationConfigwhileManagedRemoteMcpCatalogimports the same class — an import would match the surrounding style; andbudget()/validateRemotePayloadBudget()return two different messages for the same range rule.
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8411 adds trusted, bounded per-Server remote MCP payload budgets (remoteRequestBytes/remoteResponseBytes) to AgentGatewayAggregationConfig and the agent-gateway transport. Full diff was read; code, tests, fixtures and docs are consistent. CI at the reviewed head did not pass (one build check failed), which prevents a PASS conclusion.
Evidence
- Full VERSIONED_DIFF read (has_more=false, total 51085 chars); all 9 changed files inspected, including head-version AgentGatewayAggregationConfig bounds (MIN_REMOTE_BYTES=1024, MAX_REMOTE_REQUEST_BYTES=65536, MAX_REMOTE_RESPONSE_BYTES=1048576).
- CI at head sha 99892a8: complete=true but passed=false, with failed check
build (shenyu-integrated-test-k8s-ingress-grpc)(run_id 42931286618). - Budgets enforced via validateRemotePayloadBudget and never read from rule/agent/metadata; legacy 8 KiB default preserved; covered by AgentGatewayAggregationConfigTest (defaults, range, invalid, unknown fields) and agentGateway-plugin-v1.tsv fixtures (above-max/below-min/non-integer).
Recommended next steps
- Resolve or confirm the unrelated
build (shenyu-integrated-test-k8s-ingress-grpc)failure at head sha 99892a8; re-run CI to obtain a green build before merge.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 8172dd34-54b3-4c59-a472-f0cc6ae847df · Commit: 99892a88ed106d85ea853f54ce797607b3cbdd9b
Review verdict: INCONCLUSIVE
Full diff coverage was achieved and the change is internally coherent: the trusted-config path is isolated (budgets parsed/validated by the shared Admin+gateway parser and captured in the immutable RemoteServerBinding.Config snapshot, never from a rule handle, Agent request, _meta or result metadata), the legacy 8 KiB default is preserved via the 4-arg constructor, bounds are enforced in validateRemotePayloadBudget, and tests/fixtures/README all agree on the documented limits. However, PASS requires passing CI, and the CI run at the reviewed head did not pass: one build check (build (shenyu-integrated-test-k8s-ingress-grpc)) is failing. The available evidence does not establish whether that failure is caused by this PR (the changed modules are shenyu-common and shenyu-plugin-agent-gateway), so a PASS conclusion cannot be reached and the verdict is INCONCLUSIVE.
Publication: COMMENT (INCONCLUSIVE_REVIEW)
Review details
The feature change is coherent and well-tested, but the required CI run at head sha 99892a8 is not green (build (shenyu-integrated-test-k8s-ingress-grpc) failed). Please resolve or confirm that failure is unrelated and re-run CI before merging; then this can move to PASS.
…dgets Address the review of apache#8411. The remote binding compared the whole Config record, which now contains the request/response budgets, against the config returned by the credential resolver. Any resolver written before this feature rebuilds that target from the four reference fields and therefore carries the 8192 defaults, so every non-default budget failed with "Missing or mismatched service credential binding" and the data-sync update was silently dropped. Binding identity is name/endpoint/credentialRef/credentialVersion only (Config#bindsTo); budgets stay trusted transport configuration. The file resolver no longer mirrors budgets into its comparison, and the Config javadoc now states that record equals/hashCode still cover the complete snapshot, budgets included. Also make the documented limit relationship accurate: remoteResponseBytes bounds upstream JSON-RPC wire bytes, while the response the rule renders is bounded by the rule handle's maxResponseBytes in AgentHttpToolExecutor. The two are independent and both must pass, so a 1 MiB remote budget is only usable when the entry budget is raised as well. Document the resulting memory bound (8 Servers x 64 in-flight x remoteResponseBytes, ~512 MiB only at the hard maximum, 4 MiB at the defaults) and state explicitly that this change adds no aggregate admission limit, which stays with apache#8095. Finally use the imported AgentGatewayAggregationConfig instead of inline FQNs and give the budget range rule a single error message in budget() and validateRemotePayloadBudget().
|
Thanks for the review — all four points are addressed in 1. Credential binding no longer depends on the payload budgets (code)
2. The entry limit is the rule handle's, not the remote budget (docs)The README claimed the per-Server limit also applies "after entry transformation when the final 3. Why the 1 MiB response ceiling stays, and what it does and does not bound (docs)Two facts had to be stated, not argued away:
I kept 1048576 rather than lowering it because the value is an opt-in ceiling, not a default: the 4. Consistency cleanup (code)
Verification
|
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8411 adds configurable, trusted per-server remote MCP request/response byte budgets. The frozen evidence shows CI still queued/pending, and repeated re-reads of the core config/client diff returned archived empty observations, so I cannot issue PASS; verdict is INCONCLUSIVE with no confirmed blocking defect.
Evidence
- Frozen attachment: ci.complete=true but passed=false, legacy_state=pending, all 11 checks queued with null conclusion and failed_checks=[] — CI is not passing.
- Engine ledger records the full pinned diff as delivered (61,271 chars, has_more=false), but path-scoped re-reads of AgentGatewayAggregationConfig.java and RequestScopedMcpClient.java returned archived/empty observations, limiting exact content verification.
- Verified snippets: ManagedRemoteMcpCatalog threads remoteRequestBytes/remoteResponseBytes into RemoteServerBinding.Config; FileRemoteServiceCredentialResolver now uses target.bindsTo(bound) so credential binding ignores budgets. No concrete HIGH/CRITICAL defect was confirmed.
Recommended next steps
- Re-evaluate once the 11 queued CI checks report conclusions; PASS requires passing CI.
- Re-read the archived core diff pages (AgentGatewayAggregationConfig.java, RequestScopedMcpClient.java, tests, README, TSV fixture) to verify literal enforcement and validation.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 35e92a17-1f0b-4bee-b3ee-d745c87a0107 · Commit: 371b9edfeaea8d2e9e8e3a3573ab8d7844e0587f
Review verdict: INCONCLUSIVE
Diff pages were delivered for head 371b9ed, but the frozen CI evidence shows every check queued/pending (passed=false), so the passing-CI requirement for PASS is unmet. Targeted re-reads of the core config and client diffs repeatedly returned archived observations with empty pages, so exact enforcement/validation content could not be re-verified in this pass. No concrete blocking defect was established, so NEEDS_CHANGES is unsupported; the only defensible explicit verdict is INCONCLUSIVE.
Publication: COMMENT (INCONCLUSIVE_REVIEW)
Fixes #8391
Problem
The managed remote MCP adapter fixed both the outbound request limit and the
response body limit at 8192 bytes (
RequestScopedMcpClient). A gateway rulecould not raise that transport bound, even when its own entry budget allowed a
larger result, so a valid 20 KiB tool result or resource read was rejected by the
remote adapter. There was no trusted configuration path and no explicit policy for
declared lengths, compressed bodies or early cancellation.
Change
Adds two optional, trusted per-Server budgets to the shared Agent Gateway
aggregation configuration:
remoteRequestBytes8192102465536remoteResponseBytes819210241048576AgentGatewayAggregationConfigparses and validates the fields once, so Adminpersistence/import and the gateway data-sync path reject the same invalid
values (non-integer, unknown field, below minimum, above hard maximum). The
existing 8 KiB behavior remains the default for every configuration that omits
the fields, including the four-argument
Server/RemoteServerBinding.Configconstructors used by existing code and tests.
RemoteServerBinding.Configcarries the validated budgets into the immutablebinding snapshot captured when a directory generation is built. Rule handles,
Agent requests, request
_metaand result metadata cannot change them; anold/new configuration swap cannot mix snapshots because each in-flight client
keeps its own captured
Config.RequestScopedMcpClientuses the captured budgets for every remote message:initialize/initialized, tools and resources pagination,
tools/callandresources/read. Requests are counted as UTF-8 bytes of the serializedJSON-RPC body before any upstream side effect.
Content-Lengthabove the budget or a body crossing the budget while streaming cancels the
upstream body immediately, so chunked and SSE responses stop mid-stream and
nothing beyond the bound is buffered, decoded or serialized. The owned request
future, active counter and in-flight registration are released on cancel; the
configured deadline and cancellation semantics are unchanged.
Content-Encodingother than absent/identityis refused from the headersonly; compressed bodies are neither inflated nor retried. There is no fallback
to a larger budget or to a different encoding.
that remote wire bytes (including JSON/SSE framing) are counted, that entry
rule budgets stay independent and are still enforced after transformation, and
which layer rejects an oversized payload.
No new admission controller: aggregate concurrency/queue admission remains
#7951 / #8095.
Tests
shenyu-common:AgentGatewayAggregationConfigTest— 46 tests (defaults,explicit bounds, minimum/maximum rejection, non-integer and unknown fields,
shared validator), plus new valid/invalid rows in the
agentGateway-plugin-v1configuration-contract fixture used by the Admin/gatewaycontract registry.
shenyu-plugin-agent-gateway: newRemotePayloadBudgetTest(11 tests, realloopback HTTP fixture): legacy 8 KiB rejection, 20 KiB tool result and 20 KiB
resource result with an explicit budget, responses at/below/above the bound,
requests exactly at the bound and one byte above, oversized chunked SSE
cancelled before the whole body, compressed response refused without inflating,
declared length above the budget refused before the body, a legacy 8 KiB client
and a larger client running concurrently with isolated outcomes, and budget
range rejection in the binding snapshot.
./mvnw -pl shenyu-common,shenyu-plugin/shenyu-plugin-agent-gateway -am install -DskipTestspasses (checkstyle, RAT, compilation).Verified locally with Java 17; no Docker-based integrated/e2e suite was run.