api: support buffer limit in clientTrafficPolicy#2805
api: support buffer limit in clientTrafficPolicy#2805arkodg merged 28 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2805 +/- ##
=======================================
Coverage 64.50% 64.50%
=======================================
Files 121 121
Lines 21397 21397
=======================================
Hits 13803 13803
Misses 6723 6723
Partials 871 871 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
…nvoy-gateway into buffer-limit-api
…into buffer-limit-api
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
api/v1alpha1/connection_types.go
Outdated
| // +optional | ||
| ConnectionLimit *ConnectionLimit `json:"connectionLimit,omitempty"` | ||
| // BufferLimit provides configuration for the maximum buffer size for each incoming connection. | ||
| // For example, 128Mi, 500m, 2G, etc. |
There was a problem hiding this comment.
looks like the type accepts a lot more such as 500m is not relevant for this field.
can we limit it to specific types relevant for memory
There was a problem hiding this comment.
Do you mean, limit the use for only binary suffixes (Ki | Mi | Gi | Ti | Pi | Ei) and not decimal suffixed? (m | "" | k | M | G | T | P | E)
If so, I can change the example and limit it using the kubebuilder schema validation.
There was a problem hiding this comment.
Maybe *uint32 is a better option after all, validating the *resource.Quantity might be a bit complex (unnecessarily) :)
Changed to *uint32.
There was a problem hiding this comment.
fwiw I did like the fact that the abstracted type was handling known quantities very well
There was a problem hiding this comment.
what do you think about runtime validation instead? users will get an error on the CTP but it won't prevent them from applying a resource with cpu specific suffixes
There was a problem hiding this comment.
sure, so the outlining the goal
- as a user, i'd like to specify mem in terms on absolute value (bytes), or known quantities (K, M, G, Ki, Mi, Gi)
Options
- CEL validation for resource.Quanitity
- Runtime Validation of resource.Quanitity
- Creating our own type similar to what upstream did for Duration
https://github.com/kubernetes-sigs/gateway-api/blob/9183329ef9cff3f9222f001d2e112f325108c761/apis/v1/shared_types.go#L702
There was a problem hiding this comment.
Changed back to Quantity and added a cel validation. It will check that the suffix, if present, is compatible with the suffixes for memory: units of memory in k8s.
One more thing, values without suffix, like 1500 (bytes) must be specified with "" (otherwise there is an error). Is it something known to users as k8s behavior, or should I document it as well?
There was a problem hiding this comment.
can you elaborate on "" , that feels like something that might annoy the user :)
thoughts on the string approach with kubebuilder validations, and then typecasting into resource.Quantity so the user doesnt hit the gotcha ?
There was a problem hiding this comment.
Lior's suggestion solved the problem 👍
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
…into buffer-limit-api
…nvoy-gateway into buffer-limit-api
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
api/v1alpha1/connection_types.go
Outdated
| // Note that when the suffix is not provided, the value is interpreted as bytes. | ||
| // Default: 32768 bytes. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(r\"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\")",message="bufferLimit must be of the format \"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\"" |
There was a problem hiding this comment.
I suggest changing the CEL rule to:
type(self) == string ? self.matches(r"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$") : type(self) == int
This way it works for raw numbers as well as for string values.
There was a problem hiding this comment.
Thanks, that did the trick
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
…nvoy-gateway into buffer-limit-api
What this PR does / why we need it:
This PR introduces configurability for the
per_connection_buffer_limit_bytesparameter. It uses the struct proposed in this PR: #2709, pending its merge.Which issue(s) this PR fixes:
Fixes #2600