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.

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): implement connection limit #2952

Merged
merged 21 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions api/v1alpha1/connection_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,19 @@ import gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"

// Connection allows users to configure connection-level settings
type Connection struct {
Copy link
Member

Choose a reason for hiding this comment

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

will this be a little odd?

connection:
  connectionLimit:

Copy link
Contributor

Choose a reason for hiding this comment

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

its to tackle the issue outlined in #2805

// Limit defines limits related to connections
// ConnectionLimit defines limits related to connections
//
// +optional
Limit *ConnectionLimit `json:"limit,omitempty"`
ConnectionLimit *ConnectionLimit `json:"connectionLimit,omitempty"`
}

type ConnectionLimit struct {
// Value of the maximum concurrent connections limit.
// When the limit is reached, incoming connections will be closed after the CloseDelay duration.
// Default: unlimited.
//
// +optional
// +kubebuilder:validation:Minimum=0
Value *int64 `json:"value,omitempty"`
Value int64 `json:"value,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a default envoy uses here ?
there maybe a valid use case here where the user wants to use the default here (not set it) and set CloseDelay instead (where the default isn't good enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No default is mentioned in the docs. It looks like the filter constructor requires a value to be provided: https://github.com/envoyproxy/envoy/blob/e4bd0e6f70749a50dd122d5d7006e8913bb9e84c/source/extensions/filters/network/connection_limit/connection_limit.cc#L17C24-L17C53 by the filter config. So, I think that it's legitimate to require a value if the user decides to opt-in for this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think EG should set a default here ?

Copy link
Member

@zhaohuabing zhaohuabing Mar 20, 2024

Choose a reason for hiding this comment

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

I'd vote to leave it to users to decide what this value should be.

It might be difficult to choose a default value for max connections because it depends on things we don't know before deploying EG in production: the scale of clients, the spec of the machine the EG is running on, etc.

Copy link
Contributor Author

@guydc guydc Mar 20, 2024

Choose a reason for hiding this comment

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

+1 to huabing's position. When we support instance (overload manager) and/or listener connection limits with defaults (e.g. 50k), we can validate that this limit is not higher than those limits, which will create an effective "range" for users to choose from.


// CloseDelay defines the delay to use before closing connections that are rejected
// once the limit value is reached.
Expand Down
9 changes: 2 additions & 7 deletions api/v1alpha1/zz_generated.deepcopy.go

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
Expand Up @@ -94,8 +94,8 @@ spec:
connection:
description: Connection includes client connection settings.
properties:
limit:
description: Limit defines limits related to connections
connectionLimit:
description: ConnectionLimit defines limits related to connections
properties:
closeDelay:
description: 'CloseDelay defines the delay to use before closing
Expand Down
34 changes: 34 additions & 0 deletions internal/gatewayapi/clienttrafficpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ func (t *Translator) translateClientTrafficPolicyForListener(policy *egv1a1.Clie
// Translate TCPKeepalive
translateListenerTCPKeepalive(policy.Spec.TCPKeepalive, httpIR)

// Translate Connection
if err := translateListenerConnection(policy.Spec.Connection, httpIR); err != nil {
return err
}

// Translate Proxy Protocol
translateListenerProxyProtocol(policy.Spec.EnableProxyProtocol, httpIR)

Expand Down Expand Up @@ -644,3 +649,32 @@ func (t *Translator) translateListenerTLSParameters(policy *egv1a1.ClientTraffic

return nil
}

func translateListenerConnection(connection *egv1a1.Connection, httpIR *ir.HTTPListener) error {
// Return early if not set
if connection == nil {
return nil
}

irConnection := &ir.Connection{}

if connection.ConnectionLimit != nil {
irConnectionLimit := &ir.ConnectionLimit{}

irConnectionLimit.Value = ptr.To(uint64(connection.ConnectionLimit.Value))

if connection.ConnectionLimit.CloseDelay != nil {
d, err := time.ParseDuration(string(*connection.ConnectionLimit.CloseDelay))
if err != nil {
return fmt.Errorf("invalid CloseDelay value %s", *connection.ConnectionLimit.CloseDelay)
}
irConnectionLimit.CloseDelay = ptr.To(metav1.Duration{Duration: d})
}

irConnection.Limit = irConnectionLimit
}

httpIR.Connection = irConnection

return nil
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
clientTrafficPolicies:
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
namespace: envoy-gateway
name: target-gateway-1
spec:
tcpKeepalive: {}
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
namespace: envoy-gateway
name: target-gateway-1-section-http-1
spec:
connection:
connectionLimit:
value: 3
closeDelay: 10mib
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
sectionName: http-1
namespace: envoy-gateway
gateways:
- apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
namespace: envoy-gateway
name: gateway-1
spec:
gatewayClassName: envoy-gateway-class
listeners:
- name: http-1
protocol: HTTP
port: 80
allowedRoutes:
namespaces:
from: Same
- name: http-2
protocol: HTTP
port: 8080
allowedRoutes:
namespaces:
from: Same
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
clientTrafficPolicies:
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
creationTimestamp: null
name: target-gateway-1-section-http-1
namespace: envoy-gateway
spec:
connection:
connectionLimit:
closeDelay: 10mib
value: 3
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
sectionName: http-1
status:
ancestors:
- ancestorRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
sectionName: http-1
conditions:
- lastTransitionTime: null
message: Invalid CloseDelay value 10mib
reason: Invalid
status: "False"
type: Accepted
controllerName: gateway.envoyproxy.io/gatewayclass-controller
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
creationTimestamp: null
name: target-gateway-1
namespace: envoy-gateway
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
tcpKeepalive: {}
status:
ancestors:
- ancestorRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
conditions:
- lastTransitionTime: null
message: There are existing ClientTrafficPolicies that are overriding these
sections [http-1]
reason: Overridden
status: "True"
type: Overridden
- lastTransitionTime: null
message: Policy has been accepted.
reason: Accepted
status: "True"
type: Accepted
controllerName: gateway.envoyproxy.io/gatewayclass-controller
gateways:
- apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
creationTimestamp: null
name: gateway-1
namespace: envoy-gateway
spec:
gatewayClassName: envoy-gateway-class
listeners:
- allowedRoutes:
namespaces:
from: Same
name: http-1
port: 80
protocol: HTTP
- allowedRoutes:
namespaces:
from: Same
name: http-2
port: 8080
protocol: HTTP
status:
listeners:
- attachedRoutes: 0
conditions:
- lastTransitionTime: null
message: Sending translated listener configuration to the data plane
reason: Programmed
status: "True"
type: Programmed
- lastTransitionTime: null
message: Listener has been successfully translated
reason: Accepted
status: "True"
type: Accepted
- lastTransitionTime: null
message: Listener references have been resolved
reason: ResolvedRefs
status: "True"
type: ResolvedRefs
name: http-1
supportedKinds:
- group: gateway.networking.k8s.io
kind: HTTPRoute
- group: gateway.networking.k8s.io
kind: GRPCRoute
- attachedRoutes: 0
conditions:
- lastTransitionTime: null
message: Sending translated listener configuration to the data plane
reason: Programmed
status: "True"
type: Programmed
- lastTransitionTime: null
message: Listener has been successfully translated
reason: Accepted
status: "True"
type: Accepted
- lastTransitionTime: null
message: Listener references have been resolved
reason: ResolvedRefs
status: "True"
type: ResolvedRefs
name: http-2
supportedKinds:
- group: gateway.networking.k8s.io
kind: HTTPRoute
- group: gateway.networking.k8s.io
kind: GRPCRoute
infraIR:
envoy-gateway/gateway-1:
proxy:
listeners:
- address: null
name: envoy-gateway/gateway-1/http-1
ports:
- containerPort: 10080
name: http-1
protocol: HTTP
servicePort: 80
- address: null
name: envoy-gateway/gateway-1/http-2
ports:
- containerPort: 8080
name: http-2
protocol: HTTP
servicePort: 8080
metadata:
labels:
gateway.envoyproxy.io/owning-gateway-name: gateway-1
gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway
name: envoy-gateway/gateway-1
xdsIR:
envoy-gateway/gateway-1:
accessLog:
text:
- path: /dev/stdout
http:
- address: 0.0.0.0
hostnames:
- '*'
isHTTP2: false
name: envoy-gateway/gateway-1/http-1
path:
escapedSlashesAction: UnescapeAndRedirect
mergeSlashes: true
port: 10080
- address: 0.0.0.0
hostnames:
- '*'
isHTTP2: false
name: envoy-gateway/gateway-1/http-2
path:
escapedSlashesAction: UnescapeAndRedirect
mergeSlashes: true
port: 8080
tcpKeepalive: {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
clientTrafficPolicies:
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
namespace: envoy-gateway
name: target-gateway-1
spec:
connection: {}
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
namespace: envoy-gateway
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
namespace: envoy-gateway
name: target-gateway-1-section-http-1
spec:
connection:
connectionLimit:
value: 3
closeDelay: 10s
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
sectionName: http-1
namespace: envoy-gateway
gateways:
- apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
namespace: envoy-gateway
name: gateway-1
spec:
gatewayClassName: envoy-gateway-class
listeners:
- name: http-1
protocol: HTTP
port: 80
allowedRoutes:
namespaces:
from: Same
- name: http-2
protocol: HTTP
port: 8080
allowedRoutes:
namespaces:
from: Same
Loading
Loading