Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: support buffer limit in clientTrafficPolicy #2805

Merged
merged 28 commits into from
Mar 29, 2024
Merged

api: support buffer limit in clientTrafficPolicy #2805

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
cafad23
api: support buffer limit in clientTrafficPolicy
yaelSchechter Mar 6, 2024
b1deee3
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 7, 2024
26d992d
run make commands
yaelSchechter Mar 7, 2024
4f6c2da
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 7, 2024
b6b1763
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 10, 2024
e3e46c8
resolve conflicts
yaelSchechter Mar 18, 2024
874d24f
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 19, 2024
dd6b526
run gen-check
yaelSchechter Mar 19, 2024
f303cc0
Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…
yaelSchechter Mar 19, 2024
8339fa6
Merge branch 'main' of https://github.com/yaelSchechter/envoy-gateway…
yaelSchechter Mar 20, 2024
847ee5d
fix cr comment
yaelSchechter Mar 21, 2024
21a8c9c
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 21, 2024
b86fd68
add default
yaelSchechter Mar 21, 2024
f9755cb
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 21, 2024
83e8cbf
resolve conflicts
yaelSchechter Mar 26, 2024
6c90143
resolve conflicts
yaelSchechter Mar 26, 2024
1b679b0
resolve conflicts
yaelSchechter Mar 26, 2024
99ba4d4
add examples
yaelSchechter Mar 26, 2024
ebb0a4e
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 26, 2024
9112b6a
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 26, 2024
93ae109
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 27, 2024
249fac4
use uint32 for buffer limit
yaelSchechter Mar 27, 2024
59ce260
Merge branch 'main' of https://github.com/yaelSchechter/envoy-gateway…
yaelSchechter Mar 27, 2024
ea9696c
Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…
yaelSchechter Mar 27, 2024
1eaf88f
use quantity and add a schema validation
yaelSchechter Mar 28, 2024
abf1f36
Merge branch 'main' into buffer-limit-api
yaelSchechter Mar 28, 2024
3706559
add a type check
yaelSchechter Mar 28, 2024
1326cb4
Merge branch 'buffer-limit-api' of https://github.com/yaelSchechter/e…
yaelSchechter Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion api/v1alpha1/connection_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,23 @@

package v1alpha1

import gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
import (
"k8s.io/apimachinery/pkg/api/resource"
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
)

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lior's suggestion solved the problem 👍

// Default: 32768 bytes.
//
// +optional
BufferLimit *resource.Quantity `json:"bufferLimit,omitempty"`
zirain marked this conversation as resolved.
Show resolved Hide resolved
}

type ConnectionLimit struct {
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions charts/gateway-helm/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,15 @@ spec:
connection:
description: Connection includes client connection settings.
properties:
bufferLimit:
anyOf:
- type: integer
- type: string
description: 'BufferLimit provides configuration for the maximum
buffer size for each incoming connection. For example, 128Mi,
500m, 2G, etc. Default: 32768 bytes.'
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
connectionLimit:
description: ConnectionLimit defines limits related to connections
properties:
Expand Down
1 change: 1 addition & 0 deletions site/content/en/latest/api/extension_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ _Appears in:_
| Field | Type | Required | Description |
| --- | --- | --- | --- |
| `connectionLimit` | _[ConnectionLimit](#connectionlimit)_ | false | ConnectionLimit defines limits related to connections |
| `bufferLimit` | _[Quantity](#quantity)_ | false | BufferLimit provides configuration for the maximum buffer size for each incoming connection. For example, 128Mi, 500m, 2G, etc. Default: 32768 bytes. |


#### ConnectionLimit
Expand Down
Loading