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 timeout in ClientTrafficPolicy #2667

Merged
merged 10 commits into from
Feb 22, 2024
33 changes: 33 additions & 0 deletions internal/gatewayapi/clienttrafficpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@
// Translate Path Settings
translatePathSettings(policy.Spec.Path, httpIR)

// Translate Client Timeout Settings
if err := translateClientTimeout(policy.Spec.Timeout, httpIR); err != nil {
return err
}

Check warning on line 330 in internal/gatewayapi/clienttrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clienttrafficpolicy.go#L329-L330

Added lines #L329 - L330 were not covered by tests

// Translate HTTP1 Settings
if err := translateHTTP1Settings(policy.Spec.HTTP1, httpIR); err != nil {
return err
Expand Down Expand Up @@ -395,6 +400,34 @@
}
}

func translateClientTimeout(clientTimeout *egv1a1.ClientTimeout, httpIR *ir.HTTPListener) error {
if clientTimeout == nil {
return nil
}

if clientTimeout.HTTP != nil {
if clientTimeout.HTTP.RequestReceivedTimeout != nil {
d, err := time.ParseDuration(string(*clientTimeout.HTTP.RequestReceivedTimeout))
if err != nil {
return err
}

Check warning on line 413 in internal/gatewayapi/clienttrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clienttrafficpolicy.go#L412-L413

Added lines #L412 - L413 were not covered by tests

if httpIR.Timeout == nil {
Copy link
Contributor

@liorokman liorokman Feb 21, 2024

Choose a reason for hiding this comment

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

Small nit, but maybe this is slightly more readable as a switch statement:

    switch {
        case httpIR.Timeout == nil:
                      httpIR.Timeout = &ir.ClientTimeout{}
                      fallthrough
         case httpIR.Timeout.HTTP == nil:
                      httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}                      
    }

    httpIR.Timeout.HTTP.RequestReceivedTimeout = &metav1.Duration{
                                 Duration: d,
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Lior
I think that this suggestion offers a cleaner structure, but is it a convention for implementing switch cases? I'm not sure that all readers are familiar with the fallthrough keyword.

Copy link
Contributor

@liorokman liorokman Feb 21, 2024

Choose a reason for hiding this comment

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

The fallthrough keyword is part of the language - readers should be familiar with the language.

But this can also be written without it:

    switch {
        case httpIR.Timeout == nil:
                      httpIR.Timeout = &ir.ClientTimeout{
                              HTTP: &ir.HTTPClientTimeout{},
                      }
         case httpIR.Timeout.HTTP == nil:
                      httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}
    }

    httpIR.Timeout.HTTP.RequestReceivedTimeout = &metav1.Duration{
                                 Duration: d,
    }

httpIR.Timeout = &ir.ClientTimeout{}
}
if httpIR.Timeout.HTTP == nil {
httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}
}

httpIR.Timeout.HTTP.RequestReceivedTimeout = &metav1.Duration{
Duration: d,
}
}
}

return nil
}

func translateListenerProxyProtocol(enableProxyProtocol *bool, httpIR *ir.HTTPListener) {
// Return early if not set
if enableProxyProtocol == nil {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
clientTrafficPolicies:

Check failure on line 1 in internal/gatewayapi/testdata/clienttrafficpolicy-timeout.in.yaml

View workflow job for this annotation

GitHub Actions / lint

1:23 [new-lines] wrong new line character: expected \n

Check failure on line 1 in internal/gatewayapi/testdata/clienttrafficpolicy-timeout.in.yaml

View workflow job for this annotation

GitHub Actions / lint

1:23 [new-lines] wrong new line character: expected \n
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
namespace: envoy-gateway
name: target-gateway
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway
namespace: envoy-gateway
sectionName: http-1
timeout:
http:
requestReceivedTimeout: "5s"
gateways:
- apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
namespace: envoy-gateway
name: gateway
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
145 changes: 145 additions & 0 deletions internal/gatewayapi/testdata/clienttrafficpolicy-timeout.out.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
clientTrafficPolicies:

Check failure on line 1 in internal/gatewayapi/testdata/clienttrafficpolicy-timeout.out.yaml

View workflow job for this annotation

GitHub Actions / lint

1:23 [new-lines] wrong new line character: expected \n

Check failure on line 1 in internal/gatewayapi/testdata/clienttrafficpolicy-timeout.out.yaml

View workflow job for this annotation

GitHub Actions / lint

1:23 [new-lines] wrong new line character: expected \n
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
creationTimestamp: null
name: target-gateway
namespace: envoy-gateway
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway
namespace: envoy-gateway
sectionName: http-1
timeout:
http:
requestReceivedTimeout: "5s"
status:
conditions:
- lastTransitionTime: null
message: ClientTrafficPolicy has been accepted.
reason: Accepted
status: "True"
type: Accepted
gateways:
- apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
creationTimestamp: null
name: gateway
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:
proxy:
listeners:
- address: null
name: envoy-gateway/gateway/http-1
ports:
- containerPort: 10080
name: http-1
protocol: HTTP
servicePort: 80
- address: null
name: envoy-gateway/gateway/http-2
ports:
- containerPort: 8080
name: http-2
protocol: HTTP
servicePort: 8080
metadata:
labels:
gateway.envoyproxy.io/owning-gateway-name: gateway
gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway
name: envoy-gateway/gateway
xdsIR:
envoy-gateway/gateway:
accessLog:
text:
- path: /dev/stdout
http:
- address: 0.0.0.0
hostnames:
- '*'
isHTTP2: false
name: envoy-gateway/gateway/http-1
path:
mergeSlashes: true
escapedSlashesAction: UnescapeAndRedirect
port: 10080
timeout:
http:
RequestReceivedTimeout: "5s"
- address: 0.0.0.0
hostnames:
- '*'
isHTTP2: false
name: envoy-gateway/gateway/http-2
path:
mergeSlashes: true
escapedSlashesAction: UnescapeAndRedirect
port: 8080

24 changes: 20 additions & 4 deletions internal/ir/xds.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@

import (
"cmp"
"errors"

Check failure on line 10 in internal/ir/xds.go

View workflow job for this annotation

GitHub Actions / lint

File is not `goimports`-ed with -local github.com/envoyproxy/gateway/ (goimports)
"net/http"
"net/netip"
"reflect"

"golang.org/x/exp/slices"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/validation"
"net/http"
"net/netip"
"reflect"
"sigs.k8s.io/yaml"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
Expand Down Expand Up @@ -225,6 +224,8 @@
// HTTP1 provides HTTP/1 configuration on the listener
// +optional
HTTP1 *HTTP1Settings `json:"http1,omitempty" yaml:"http1,omitempty"`
// ClientTimeout sets the timeout configuration for downstream connections
Timeout *ClientTimeout `json:"timeout,omitempty" yaml:"clientTimeout,omitempty"`
}

// Validate the fields within the HTTPListener structure
Expand Down Expand Up @@ -389,6 +390,21 @@
EnableEnvoyHeaders bool `json:"enableEnvoyHeaders,omitempty" yaml:"enableEnvoyHeaders,omitempty"`
}

// ClientTimeout sets the timeout configuration for downstream connections
// +k8s:deepcopy-gen=true
type ClientTimeout struct {
// Timeout settings for HTTP.
HTTP *HTTPClientTimeout `json:"http,omitempty" yaml:"http,omitempty"`
}

// HTTPClientTimeout set the configuration for client HTTP.
// +k8s:deepcopy-gen=true
type HTTPClientTimeout struct {
// The duration envoy waits for the complete request reception. This timer starts upon request
// initiation and stops when either the last byte of the request is sent upstream or when the response begins.
RequestReceivedTimeout *metav1.Duration `json:"requestReceivedTimeout,omitempty" yaml:"requestReceivedTimeout,omitempty"`
}

// HTTPRoute holds the route information associated with the HTTP Route
// +k8s:deepcopy-gen=true
type HTTPRoute struct {
Expand Down
45 changes: 45 additions & 0 deletions internal/ir/zz_generated.deepcopy.go

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

5 changes: 5 additions & 0 deletions internal/xds/translator/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
package translator

import (
"errors"

Check failure on line 9 in internal/xds/translator/listener.go

View workflow job for this annotation

GitHub Actions / lint

File is not `goimports`-ed with -local github.com/envoyproxy/gateway/ (goimports)
"google.golang.org/protobuf/types/known/durationpb"

xdscore "github.com/cncf/xds/go/xds/core/v3"
matcher "github.com/cncf/xds/go/xds/type/matcher/v3"
Expand Down Expand Up @@ -230,6 +231,10 @@
Tracing: hcmTracing,
}

if irListener.Timeout != nil && irListener.Timeout.HTTP != nil && irListener.Timeout.HTTP.RequestReceivedTimeout != nil {
mgr.RequestTimeout = durationpb.New(irListener.Timeout.HTTP.RequestReceivedTimeout.Duration)
}

// Add the proxy protocol filter if needed
patchProxyProtocolFilter(xdsListener, irListener)

Expand Down
21 changes: 21 additions & 0 deletions internal/xds/translator/testdata/in/xds-ir/client-timeout.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
http:

Check failure on line 1 in internal/xds/translator/testdata/in/xds-ir/client-timeout.yaml

View workflow job for this annotation

GitHub Actions / lint

1:6 [new-lines] wrong new line character: expected \n

Check failure on line 1 in internal/xds/translator/testdata/in/xds-ir/client-timeout.yaml

View workflow job for this annotation

GitHub Actions / lint

1:6 [new-lines] wrong new line character: expected \n
- name: "first-listener"
address: "0.0.0.0"
port: 10080
hostnames:
- "*"
path:
mergeSlashes: true
escapedSlashesAction: UnescapeAndRedirect
routes:
- name: "first-route"
hostname: "*"
destination:
name: "first-route-dest"
settings:
- endpoints:

Check failure on line 16 in internal/xds/translator/testdata/in/xds-ir/client-timeout.yaml

View workflow job for this annotation

GitHub Actions / lint

16:11 [indentation] wrong indentation: expected 12 but found 10
- host: "1.2.3.4"

Check failure on line 17 in internal/xds/translator/testdata/in/xds-ir/client-timeout.yaml

View workflow job for this annotation

GitHub Actions / lint

17:13 [indentation] wrong indentation: expected 14 but found 12
port: 50000
timeout:
http:
RequestReceivedTimeout: "5s"
Loading
Loading