api: support buffer limit in clientTrafficPolicy #2805

yaelSchechter · 2024-03-06T15:51:49Z

What this PR does / why we need it:

This PR introduces configurability for the per_connection_buffer_limit_bytes parameter. It uses the struct proposed in this PR: #2709, pending its merge.

Which issue(s) this PR fixes:

Fixes #2600

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

codecov · 2024-03-07T06:26:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.50%. Comparing base (3d51933) to head (1326cb4).

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.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

…nvoy-gateway into buffer-limit-api

api/v1alpha1/connection_types.go

…into buffer-limit-api

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

api/v1alpha1/connection_types.go

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

arkodg · 2024-03-26T14:18:49Z

api/v1alpha1/connection_types.go


 // Connection allows users to configure connection-level settings
 type Connection struct {
 	// ConnectionLimit defines limits related to connections
 	//
 	// +optional
 	ConnectionLimit *ConnectionLimit `json:"connectionLimit,omitempty"`
+	// BufferLimit provides configuration for the maximum buffer size for each incoming connection.
+	// For example, 128Mi, 500m, 2G, etc.


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

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.

Maybe *uint32 is a better option after all, validating the *resource.Quantity might be a bit complex (unnecessarily) :)
Changed to *uint32.

fwiw I did like the fact that the abstracted type was handling known quantities very well

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

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

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?

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 ?

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>

liorokman · 2024-03-28T12:46:54Z

api/v1alpha1/connection_types.go

+	// 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])?$\""


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.

CEL playground

Thanks, that did the trick

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

…nvoy-gateway into buffer-limit-api

guydc

LGTM

* api: support buffer limit in clientTrafficPolicy Signed-off-by: Yael Shechter <yael.shechter@sap.com> * run make commands Signed-off-by: Yael Shechter <yael.shechter@sap.com> * run gen-check Signed-off-by: Yael Shechter <yael.shechter@sap.com> * fix cr comment Signed-off-by: Yael Shechter <yael.shechter@sap.com> * add default Signed-off-by: Yael Shechter <yael.shechter@sap.com> * add examples Signed-off-by: Yael Shechter <yael.shechter@sap.com> * use uint32 for buffer limit Signed-off-by: Yael Shechter <yael.shechter@sap.com> * use quantity and add a schema validation Signed-off-by: Yael Shechter <yael.shechter@sap.com> * add a type check Signed-off-by: Yael Shechter <yael.shechter@sap.com> --------- Signed-off-by: Yael Shechter <yael.shechter@sap.com>

api: support buffer limit in clientTrafficPolicy

cafad23

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

yaelSchechter requested a review from a team as a code owner March 6, 2024 15:51

yaelSchechter and others added 2 commits March 7, 2024 08:21

Merge branch 'main' into buffer-limit-api

b1deee3

run make commands

26d992d

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

4f6c2da

zirain added this to the Backlog milestone Mar 7, 2024

yaelSchechter and others added 5 commits March 10, 2024 10:18

Merge branch 'main' into buffer-limit-api

b6b1763

resolve conflicts

e3e46c8

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

874d24f

run gen-check

dd6b526

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…

f303cc0

…nvoy-gateway into buffer-limit-api

zhaohuabing reviewed Mar 20, 2024

View reviewed changes

api/v1alpha1/connection_types.go Outdated Show resolved Hide resolved

yaelSchechter and others added 5 commits March 20, 2024 18:30

Merge branch 'main' of https://github.com/yaelSchechter/envoy-gateway …

8339fa6

…into buffer-limit-api

fix cr comment

847ee5d

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

21a8c9c

add default

b86fd68

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

f9755cb

zhaohuabing reviewed Mar 21, 2024

View reviewed changes

api/v1alpha1/connection_types.go Outdated Show resolved Hide resolved

guydc mentioned this pull request Mar 25, 2024

feat(translator): implement connection limit #2952

Merged

arkodg reviewed Mar 26, 2024

View reviewed changes

api/v1alpha1/connection_types.go Show resolved Hide resolved

yaelSchechter and others added 5 commits March 26, 2024 13:45

resolve conflicts

83e8cbf

resolve conflicts

6c90143

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

resolve conflicts

1b679b0

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

add examples

99ba4d4

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

ebb0a4e

arkodg reviewed Mar 26, 2024

View reviewed changes

yaelSchechter and others added 3 commits March 26, 2024 17:56

Merge branch 'main' into buffer-limit-api

9112b6a

Merge branch 'main' into buffer-limit-api

93ae109

use uint32 for buffer limit

249fac4

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

yaelSchechter and others added 4 commits March 27, 2024 18:12

Merge branch 'main' of https://github.com/yaelSchechter/envoy-gateway …

59ce260

…into buffer-limit-api

Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…

ea9696c

…nvoy-gateway into buffer-limit-api

use quantity and add a schema validation

1eaf88f

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into buffer-limit-api

abf1f36

liorokman reviewed Mar 28, 2024

View reviewed changes

yaelSchechter added 2 commits March 28, 2024 15:35

add a type check

3706559

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…

1326cb4

…nvoy-gateway into buffer-limit-api

yaelSchechter requested review from liorokman and arkodg March 28, 2024 14:22

guydc approved these changes Mar 28, 2024

View reviewed changes

zirain approved these changes Mar 29, 2024

View reviewed changes

arkodg approved these changes Mar 29, 2024

View reviewed changes

arkodg merged commit deea895 into envoyproxy:main Mar 29, 2024
22 checks passed

yaelSchechter mentioned this pull request Mar 29, 2024

feat(translator): implement buffer limit support in CTP #3058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: support buffer limit in clientTrafficPolicy #2805

api: support buffer limit in clientTrafficPolicy #2805

yaelSchechter commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

arkodg Mar 26, 2024

yaelSchechter Mar 26, 2024

yaelSchechter Mar 27, 2024

arkodg Mar 27, 2024

yaelSchechter Mar 28, 2024

arkodg Mar 28, 2024

yaelSchechter Mar 28, 2024

arkodg Mar 28, 2024

yaelSchechter Mar 28, 2024

liorokman Mar 28, 2024 •

edited

Loading

yaelSchechter Mar 28, 2024

guydc left a comment

api: support buffer limit in clientTrafficPolicy #2805

api: support buffer limit in clientTrafficPolicy #2805

Conversation

yaelSchechter commented Mar 6, 2024 • edited Loading

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorokman Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guydc left a comment

Choose a reason for hiding this comment

yaelSchechter commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

liorokman Mar 28, 2024 •

edited

Loading