feat(bootstrap): allow max_receive_message_length override on xDS gRPC service#9485
feat(bootstrap): allow max_receive_message_length override on xDS gRPC service#9485linmosko wants to merge 3 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 054d2a12e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1 @@ | |||
| Added support for updating `max_receive_message_length` on the xDS gRPC service via bootstrap override, allowing operators to tune the gRPC receive buffer when the xDS snapshot grows large at scale. | |||
There was a problem hiding this comment.
When make lint runs in CI, its lint.release-notes-filenames target invokes tools/hack/check-release-notes-filenames.sh, which requires current fragments to match ^[0-9]+-[a-z0-9-]+\.md$; this XXXXX-... placeholder filename fails that check and will block the lint job until it is renamed with a numeric PR/issue prefix.
Useful? React with 👍 / 👎.
1637ce8 to
beefcc9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9485 +/- ##
==========================================
- Coverage 75.43% 75.41% -0.02%
==========================================
Files 252 252
Lines 41641 41642 +1
==========================================
- Hits 31411 31405 -6
- Misses 8098 8105 +7
Partials 2132 2132 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
| grpcServices: | ||
| - envoyGrpc: | ||
| clusterName: xds_cluster | ||
| maxReceiveMessageLength: 8388608 |
There was a problem hiding this comment.
Is there an equivalent parameter on the Envoy Gateway side that also needs tuning to allow sending large messages? I think grpc doesn't limit sent message sizes by default, but EG/go-control-plane may.
There was a problem hiding this comment.
internal/xds/runner/runner.go:212 uses grpc.NewServer() with no send size limit - Go gRPC default is unlimited for sends.
| @@ -14,6 +14,7 @@ dynamicResources: | |||
| grpcServices: | |||
| - envoyGrpc: | |||
There was a problem hiding this comment.
can you also add this to one of the in/out merge tests? Demonstrate that the merge is not block and it works as expected, patching the relevant fields.
|
/retest |
…C service Signed-off-by: Lin Moskovitch <lin.moskovitch@sap.com>
…C service Signed-off-by: Lin Moskovitch <lin.moskovitch@sap.com>
Signed-off-by: Lin Moskovitch <lin.moskovitch@sap.com>
11c1acf to
44b9350
Compare
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Pull request overview
This PR extends Envoy Gateway’s bootstrap override validation so operators can set max_receive_message_length for the xDS gRPC service without tripping the “dynamic_resources cannot be modified” guard, enabling larger xDS snapshots to be delivered at scale.
Changes:
- Allow
dynamic_resourcesequality validation to ignoremax_receive_message_lengthfor the xDS gRPC Envoy gRPC service. - Add merge/JSONPatch test data and unit test coverage for applying this bootstrap override.
- Add a release-note fragment documenting the new capability.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| release-notes/current/new_features/9485-xds-grpc-max-receive-message-length.md | Documents the new bootstrap override capability for xDS gRPC receive message sizing. |
| internal/xds/bootstrap/validate.go | Updates bootstrap validation to permit overriding max_receive_message_length for the xDS gRPC service. |
| internal/xds/bootstrap/util_test.go | Adds a unit test case for patching max_receive_message_length into the rendered bootstrap. |
| internal/xds/bootstrap/testdata/validate/valid-user-bootstrap.yaml | Updates “valid” user bootstrap sample to include the new field. |
| internal/xds/bootstrap/testdata/merge/merge-max-receive-message-length.{in,out}.yaml | Adds JSONPatch input and expected bootstrap output for the new override behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
should we just default to unlimited |
|
also https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/grpc_service.proto says it defaults to 0 ? so is this PR more about increase limits or reducing them ? |
@linmosko Could you explain the use case here? You shouldn't need to tune the buffer, since the envoy default is |
|
I'm also not a sure if this parameter actually controls the envoy max receive message size. AI suggests that specifically for the xds stream, a custom grpc client is used, where this parameter is actually used to tune the decoder's HTTP2 maxFrameLength, with an unlimited default value. I think that after some additional investigation, we assumed that an EG-side 4MB grpc message size limit was causing envoy ACKs/subscriptions to be rejected, when these envoy message had a very long list of resource names. @linmosko - maybe we should change this PR to focus on exposing EG's XDS server message size settings? |
What this PR does / why we need it:
Extends bootstrap validation to allow users to override
max_receive_message_lengthon the xDS gRPC service.Useful at scale when the xDS snapshot grows large - operators can tune the gRPC receive buffer via bootstrap override without triggering the dynamic_resources cannot be modified validation error.
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.