-
Notifications
You must be signed in to change notification settings - Fork 360
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.
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
feat(translator): http2 upstream settings #3682
Changes from 3 commits
3fd193d
03fc523
2eae402
44fe53f
c822391
dc1f2d2
3119ef0
c708977
3188106
1069fe3
23b20a0
5d0dfe0
b744109
d33b415
f8ea793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |
autoscalingv2 "k8s.io/api/autoscaling/v2" | ||
corev1 "k8s.io/api/core/v1" | ||
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
"k8s.io/apimachinery/pkg/api/resource" | ||
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
) | ||
|
||
|
@@ -478,3 +479,33 @@ type BackendRef struct { | |
// A CIDR can be an IPv4 address range such as "192.168.1.0/24" or an IPv6 address range such as "2001:0db8:11a3:09d7::/64". | ||
// +kubebuilder:validation:Pattern=`((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/([0-9]+))|((([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))\/([0-9]+))` | ||
type CIDR string | ||
|
||
// HTTP2Settings provides HTTP/2 configuration for listeners and backends. | ||
type HTTP2Settings struct { | ||
// InitialStreamWindowSize sets the initial window size for HTTP/2 streams. | ||
// If not set, the default value is 64 KiB(64*1024). | ||
// | ||
// +kubebuilder:validation:XValidation:rule="type(self) == string ? self.matches(r\"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\") : type(self) == int",message="initialStreamWindowSize must be of the format \"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\"" | ||
// +optional | ||
InitialStreamWindowSize *resource.Quantity `json:"initialStreamWindowSize,omitempty"` | ||
|
||
// InitialConnectionWindowSize sets the initial window size for HTTP/2 connections. | ||
// If not set, the default value is 1 MiB. | ||
// | ||
// +kubebuilder:validation:XValidation:rule="type(self) == string ? self.matches(r\"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\") : type(self) == int",message="initialConnectionWindowSize must be of the format \"^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$\"" | ||
// +optional | ||
InitialConnectionWindowSize *resource.Quantity `json:"initialConnectionWindowSize,omitempty"` | ||
|
||
// MaxConcurrentStreams sets the maximum number of concurrent streams allowed per connection. | ||
// If not set, the default value is 100. | ||
// +kubebuilder:validation:Minimum=1 | ||
// +kubebuilder:validation:Maximum=2147483647 | ||
// +optional | ||
MaxConcurrentStreams *uint32 `json:"maxConcurrentStreams,omitempty"` | ||
|
||
// ResetStreamOnError determines if Envoy will terminate the stream or the connection in the event of HTTP messaging error | ||
// It's recommended for L2 Envoy deployments to set this value to true. | ||
// https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/level_two | ||
// +optional | ||
ResetStreamOnError *bool `json:"resetStreamOnError,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, talking about connections instead of streams would be easier for users to understand and better comprehend the impact. The only downside is that this could be confusing to someone who reads the envoy docs. |
||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// Copyright Envoy Gateway Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// The full text of the Apache license is available in the LICENSE file at | ||
// the root of the repo. | ||
|
||
package gatewayapi | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
|
||
"k8s.io/utils/ptr" | ||
|
||
egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" | ||
"github.com/envoyproxy/gateway/internal/ir" | ||
) | ||
|
||
const ( | ||
MinHTTP2InitialStreamWindowSize = 65535 // https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/protocol.proto#envoy-v3-api-field-config-core-v3-http2protocoloptions-initial-stream-window-size | ||
MaxHTTP2InitialStreamWindowSize = 2147483647 // https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/protocol.proto#envoy-v3-api-field-config-core-v3-http2protocoloptions-initial-stream-window-size | ||
MinHTTP2InitialConnectionWindowSize = MinHTTP2InitialStreamWindowSize | ||
MaxHTTP2InitialConnectionWindowSize = MaxHTTP2InitialStreamWindowSize | ||
) | ||
|
||
func buildIRHTTP2Settings(http2Settings *egv1a1.HTTP2Settings) (*ir.HTTP2Settings, error) { | ||
var ( | ||
http2 = &ir.HTTP2Settings{} | ||
errs error | ||
) | ||
|
||
if http2Settings.InitialStreamWindowSize != nil { | ||
initialStreamWindowSize, ok := http2Settings.InitialStreamWindowSize.AsInt64() | ||
switch { | ||
case !ok: | ||
errs = errors.Join(errs, fmt.Errorf("invalid InitialStreamWindowSize value %s", http2Settings.InitialStreamWindowSize.String())) | ||
case initialStreamWindowSize < MinHTTP2InitialStreamWindowSize || initialStreamWindowSize > MaxHTTP2InitialStreamWindowSize: | ||
errs = errors.Join(errs, fmt.Errorf("InitialStreamWindowSize value %s is out of range, must be between %d and %d", | ||
http2Settings.InitialStreamWindowSize.String(), | ||
MinHTTP2InitialStreamWindowSize, | ||
MaxHTTP2InitialStreamWindowSize)) | ||
default: | ||
http2.InitialStreamWindowSize = ptr.To(uint32(initialStreamWindowSize)) | ||
} | ||
} | ||
|
||
if http2Settings.InitialConnectionWindowSize != nil { | ||
initialConnectionWindowSize, ok := http2Settings.InitialConnectionWindowSize.AsInt64() | ||
switch { | ||
case !ok: | ||
errs = errors.Join(errs, fmt.Errorf("invalid InitialConnectionWindowSize value %s", http2Settings.InitialConnectionWindowSize.String())) | ||
case initialConnectionWindowSize < MinHTTP2InitialConnectionWindowSize || initialConnectionWindowSize > MaxHTTP2InitialConnectionWindowSize: | ||
errs = errors.Join(errs, fmt.Errorf("InitialConnectionWindowSize value %s is out of range, must be between %d and %d", | ||
http2Settings.InitialConnectionWindowSize.String(), | ||
MinHTTP2InitialConnectionWindowSize, | ||
MaxHTTP2InitialConnectionWindowSize)) | ||
default: | ||
http2.InitialConnectionWindowSize = ptr.To(uint32(initialConnectionWindowSize)) | ||
} | ||
} | ||
|
||
http2.MaxConcurrentStreams = http2Settings.MaxConcurrentStreams | ||
|
||
http2.ResetStreamOnError = http2Settings.ResetStreamOnError | ||
|
||
return http2, errs | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the same default value for both the listeners and clusters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move some of the defaulting behavior to the API, but some will remain in the XDS translator (due to differences between clusters and listeners). So, I propose we keep it as-is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guydc Thanks for the explanation! I'm just curious because the recommendation is for listeners. They can be the same for Clusters if there's no unintentional side effects.