feat: request buffer limit - #8893
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8893 +/- ##
==========================================
+ Coverage 75.65% 75.70% +0.04%
==========================================
Files 253 253
Lines 41879 41896 +17
==========================================
+ Hits 31684 31716 +32
+ Misses 8064 8052 -12
+ Partials 2131 2128 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
8d0f19b to
0ac8c0d
Compare
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
|
retest |
|
/retest |
…limit # Conflicts: # release-notes/current.yaml
|
/retest |
|
LGTM, thanks! |
…limit # Conflicts: # internal/gatewayapi/backendtrafficpolicy.go
|
/retest |
1 similar comment
|
/retest |
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:validation:Pattern="^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$" | ||
| // +optional | ||
| RequestBodyBufferLimit *resource.Quantity `json:"requestBodyBufferLimit,omitempty"` |
There was a problem hiding this comment.
not a must, but could this be part of RequestBuffer?
There was a problem hiding this comment.
They have different meanings:
- RequestBuffer: buffer and fully receive each request from a client before continuing to send the request upstream to the backends.
- RequestBodyBufferLimit: specifies the maximum size in bytes that Envoy may buffer for request bodies
I'm afraid it would cause confusion if we include both of them inside one RequestBuffer parent type.
There was a problem hiding this comment.
is there a use case where a user would want requestBuffer and not requestBodyBufferLimit ?
There was a problem hiding this comment.
Yes.
requestBuffer is self-sufficient. Envoy's buffer filter raises the stream's buffer limit to the configured limit on its own, so requestBuffer.limit: 10Mi works with default connection buffers and nothing else set.
Nesting it under requestBuffer implies it only applies when full buffering is on, which is wrong for the cases this field is meant to serve.
There was a problem hiding this comment.
Second thought - even though requestBuffer can work without requestBodyBufferLimit, it sliently raises requestBodyBufferLimit when requestBodyBufferLimit is less than requestBuffer.limit , so we may want:
- group them:
requestBuffer:
limit: 10Mi
mode: FullBuffer # default, today's behavior: insert the buffer filter
# LimitOnly: only set the route-level body buffer limit- or keep them separate and add a CEL rule forbidding both at once.
There was a problem hiding this comment.
+1 for 1 with alt options for naming :)
Signed-off-by: Huabing (Robin) Zhao <huabing@tetrate.io>
|
/retest |
6 participants
Implements: #8891
Release note: yes