From e32217ea84af9d69817f835887949f005a9e1e2c Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 25 Aug 2023 08:19:37 +0100 Subject: [PATCH 01/11] HTTPRoute Timeouts * update HTTPRoute interfaces according to latest proposal in GEP-1742 * add Duration type as per GEP-2257 * include Request and BackendRequest in HTTPRouteTimeouts * add validation for HTTPRouteTimeouts * add unit tests in httproute_test.go * relates to #1997 --- apis/v1beta1/httproute_types.go | 47 ++++++++++++ apis/v1beta1/shared_types.go | 5 ++ apis/v1beta1/validation/httproute.go | 42 +++++++++++ apis/v1beta1/validation/httproute_test.go | 64 +++++++++++++++++ apis/v1beta1/zz_generated.deepcopy.go | 30 ++++++++ .../gateway.networking.k8s.io_httproutes.yaml | 72 +++++++++++++++++++ 6 files changed, 260 insertions(+) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index e8216ed33c..98a6a5a42a 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -263,6 +263,53 @@ type HTTPRouteRule struct { // +optional // +kubebuilder:validation:MaxItems=16 BackendRefs []HTTPBackendRef `json:"backendRefs,omitempty"` + + // Timeouts defines the timeouts that can be configured for an HTTP request. + // + // Support: Extended + // + // +optional + // + Timeouts *HTTPRouteTimeouts `json:"timeouts,omitempty"` +} + +// HTTPRouteTimeouts defines timeouts that can be configured for an HTTPRoute. +// Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration +// and MUST BE >= 1ms or 0 to disable (no timeout). +type HTTPRouteTimeouts struct { + // Request specifies the duration for processing an HTTP client request after which the + // gateway will time out if unable to send a response. + // + // For example, setting this field to the value `10s` in an HTTPRoute will cause + // a timeout if a client request is taking longer than 10 seconds to complete. + // + // This timeout is intended to cover as close to the whole request-response transaction + // as possible although an implementation MAY choose to start the timeout after the entire + // request stream has been received instead of immediately after the transaction is + // initiated by the client. + // + // When this field is unspecified, request timeout behavior is implementation-dependent. + // + // Support: Extended + // + // +optional + Request *Duration `json:"request,omitempty"` + + // BackendRequest specifies a timeout for an individual request from the gateway + // to a backend service. This covers the time from when the request first starts being + // sent from the gateway to when the full response has been received from the backend. + // + // An entire client HTTP transaction with a gateway, covered by the Request timeout, + // may result in more than one call from the gateway to the destination backend service, + // for example, if automatic retries are supported. + // + // Because the Request timeout encompasses the BackendRequest timeout, + // the value of BackendRequest defaults to and must be <= the value of Request timeout. + // + // Support: Extended + // + // +optional + BackendRequest *Duration `json:"backendRequest,omitempty"` } // PathMatchType specifies the semantics of how HTTP paths should be compared. diff --git a/apis/v1beta1/shared_types.go b/apis/v1beta1/shared_types.go index b18af1d0e0..401b90827a 100644 --- a/apis/v1beta1/shared_types.go +++ b/apis/v1beta1/shared_types.go @@ -610,6 +610,11 @@ type AddressType string // +k8s:deepcopy-gen=false type HeaderName string +// A duration is a ... +// +// +kubebuilder:validation:Pattern=`^([0-9]{1,5}(h|m|s|ms)){1,4}$` +type Duration string + const ( // A textual representation of a numeric IP address. IPv4 // addresses must be in dotted-decimal form. IPv6 addresses diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go index 8dbe4e8f3d..9f1ce3cc96 100644 --- a/apis/v1beta1/validation/httproute.go +++ b/apis/v1beta1/validation/httproute.go @@ -21,6 +21,7 @@ import ( "net/http" "regexp" "strings" + "time" "k8s.io/apimachinery/pkg/util/validation/field" @@ -41,6 +42,9 @@ var ( // All valid path characters per RFC-3986 validPathCharacters = "^(?:[A-Za-z0-9\\/\\-._~!$&'()*+,;=:@]|[%][0-9a-fA-F]{2})+$" + + // Minimum value of timeout fields + timeoutMinValue = time.Millisecond * 1 ) // ValidateHTTPRoute validates HTTPRoute according to the Gateway API specification. @@ -73,6 +77,10 @@ func ValidateHTTPRouteSpec(spec *gatewayv1b1.HTTPRouteSpec, path *field.Path) fi errs = append(errs, validateHTTPQueryParamMatches(m.QueryParams, matchPath.Child("queryParams"))...) } } + + if rule.Timeouts != nil { + errs = append(errs, validateHTTPRouteTimeouts(rule.Timeouts, path.Child("rules").Child("timeouts"))...) + } } errs = append(errs, validateHTTPRouteBackendServicePorts(spec.Rules, path.Child("rules"))...) errs = append(errs, ValidateParentRefs(spec.ParentRefs, path.Child("spec"))...) @@ -348,6 +356,40 @@ func validateHTTPHeaderModifier(filter gatewayv1b1.HTTPHeaderFilter, path *field return errs } +func validateHTTPRouteTimeouts(timeouts *gatewayv1b1.HTTPRouteTimeouts, path *field.Path) field.ErrorList { + var errs field.ErrorList + errorString := fmt.Sprintf("timeout value must be greater than or equal to %s", timeoutMinValue) + if timeouts.BackendRequest != nil { + backendTimeout, err := time.ParseDuration((string)(*timeouts.BackendRequest)) + if err != nil { + errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, err.Error())) + } + if backendTimeout < timeoutMinValue && backendTimeout != 0 { + errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, errorString)) + } + if timeouts.Request != nil { + timeout, err := time.ParseDuration((string)(*timeouts.Request)) + if err != nil { + errs = append(errs, field.Invalid(path.Child("request"), timeout, err.Error())) + } + if backendTimeout > timeout && timeout != 0 { + errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, "backendRequest timeout cannot be longer than request timeout")) + } + } + } + if timeouts.Request != nil { + timeout, err := time.ParseDuration((string)(*timeouts.Request)) + if err != nil { + errs = append(errs, field.Invalid(path.Child("request"), timeout, err.Error())) + } + if timeout < timeoutMinValue && timeout != 0 { + errs = append(errs, field.Invalid(path.Child("request"), timeout, errorString)) + } + } + + return errs +} + func hasExactlyOnePrefixMatch(matches []gatewayv1b1.HTTPRouteMatch) bool { if len(matches) != 1 || matches[0].Path == nil { return false diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index c157dc3704..4ec0083fa6 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -1176,3 +1176,67 @@ func TestValidateRequestRedirectFiltersWithNoBackendRef(t *testing.T) { }) } } + +func toDuration(durationString string) *gatewayv1b1.Duration { + return (*gatewayv1b1.Duration)(&durationString) +} + +func TestValidateHTTPTimeouts(t *testing.T) { + tests := []struct { + name string + rules []gatewayv1b1.HTTPRouteRule + errCount int + }{ + { + name: "valid httpRoute Rules timeouts", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1ms"), + }, + }, + }, + }, { + name: "valid httpRoute Rules timeout set to 0 (disabled)", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0ms"), + }, + }, + }, + }, { + name: "invalid httpRoute Rules timeout", + errCount: 1, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("500us"), + }, + }, + }, + }, { + name: "invalid httpRoute Rules backendRequest timeout cannot be longer than request timeout", + errCount: 1, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1ms"), + BackendRequest: toDuration("2ms"), + }, + }, + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + route := gatewayv1b1.HTTPRoute{Spec: gatewayv1b1.HTTPRouteSpec{Rules: tc.rules}} + errs := ValidateHTTPRoute(&route) + if len(errs) != tc.errCount { + t.Errorf("got %d errors, want %d errors: %s", len(errs), tc.errCount, errs) + } + }) + } +} diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index d1d46269d2..b4c5028366 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -815,6 +815,11 @@ func (in *HTTPRouteRule) DeepCopyInto(out *HTTPRouteRule) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Timeouts != nil { + in, out := &in.Timeouts, &out.Timeouts + *out = new(HTTPRouteTimeouts) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HTTPRouteRule. @@ -871,6 +876,31 @@ func (in *HTTPRouteStatus) DeepCopy() *HTTPRouteStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *HTTPRouteTimeouts) DeepCopyInto(out *HTTPRouteTimeouts) { + *out = *in + if in.Request != nil { + in, out := &in.Request, &out.Request + *out = new(Duration) + **out = **in + } + if in.BackendRequest != nil { + in, out := &in.BackendRequest, &out.BackendRequest + *out = new(Duration) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HTTPRouteTimeouts. +func (in *HTTPRouteTimeouts) DeepCopy() *HTTPRouteTimeouts { + if in == nil { + return nil + } + out := new(HTTPRouteTimeouts) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HTTPURLRewriteFilter) DeepCopyInto(out *HTTPURLRewriteFilter) { *out = *in diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index 49f324c1f6..f7a3c0290a 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2113,6 +2113,42 @@ spec: type: object maxItems: 8 type: array + timeouts: + description: "Timeouts defines the timeouts that can be configured + for an HTTP request. \n Support: Extended \n " + properties: + backendRequest: + description: "BackendRequest specifies a timeout for an + individual request from the gateway to a backend service. + This covers the time from when the request first starts + being sent from the gateway to when the full response + has been received from the backend. \n An entire client + HTTP transaction with a gateway, covered by the Request + timeout, may result in more than one call from the gateway + to the destination backend service, for example, if automatic + retries are supported. \n Because the Request timeout + encompasses the BackendRequest timeout, the value of BackendRequest + defaults to and must be <= the value of Request timeout. + \n Support: Extended" + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + request: + description: "Request specifies the duration for processing + an HTTP client request after which the gateway will time + out if unable to send a response. \n For example, setting + this field to the value `10s` in an HTTPRoute will cause + a timeout if a client request is taking longer than 10 + seconds to complete. \n This timeout is intended to cover + as close to the whole request-response transaction as + possible although an implementation MAY choose to start + the timeout after the entire request stream has been received + instead of immediately after the transaction is initiated + by the client. \n When this field is unspecified, request + timeout behavior is implementation-dependent. \n Support: + Extended" + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + type: object type: object x-kubernetes-validations: - message: RequestRedirect filter must not be used together with @@ -4512,6 +4548,42 @@ spec: type: object maxItems: 8 type: array + timeouts: + description: "Timeouts defines the timeouts that can be configured + for an HTTP request. \n Support: Extended \n " + properties: + backendRequest: + description: "BackendRequest specifies a timeout for an + individual request from the gateway to a backend service. + This covers the time from when the request first starts + being sent from the gateway to when the full response + has been received from the backend. \n An entire client + HTTP transaction with a gateway, covered by the Request + timeout, may result in more than one call from the gateway + to the destination backend service, for example, if automatic + retries are supported. \n Because the Request timeout + encompasses the BackendRequest timeout, the value of BackendRequest + defaults to and must be <= the value of Request timeout. + \n Support: Extended" + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + request: + description: "Request specifies the duration for processing + an HTTP client request after which the gateway will time + out if unable to send a response. \n For example, setting + this field to the value `10s` in an HTTPRoute will cause + a timeout if a client request is taking longer than 10 + seconds to complete. \n This timeout is intended to cover + as close to the whole request-response transaction as + possible although an implementation MAY choose to start + the timeout after the entire request stream has been received + instead of immediately after the transaction is initiated + by the client. \n When this field is unspecified, request + timeout behavior is implementation-dependent. \n Support: + Extended" + pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ + type: string + type: object type: object x-kubernetes-validations: - message: RequestRedirect filter must not be used together with From 6281ef19931f78d873d920088ba4abb36055f346 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 1 Sep 2023 15:06:00 +0100 Subject: [PATCH 02/11] httproute-timeout: update * update comments based on GEP-1742 * add clone to Duration type in v1alpha2 * fix validation test according to specs "0s" * update CRD for httproutes --- apis/v1alpha2/shared_types.go | 6 ++ apis/v1beta1/httproute_types.go | 17 +++-- apis/v1beta1/shared_types.go | 3 +- apis/v1beta1/validation/httproute_test.go | 2 +- .../gateway.networking.k8s.io_httproutes.yaml | 74 +++++++++++++------ 5 files changed, 70 insertions(+), 32 deletions(-) diff --git a/apis/v1alpha2/shared_types.go b/apis/v1alpha2/shared_types.go index bba8e95160..7286f4d216 100644 --- a/apis/v1alpha2/shared_types.go +++ b/apis/v1alpha2/shared_types.go @@ -340,6 +340,12 @@ type AnnotationValue = v1beta1.AnnotationValue // +kubebuilder:validation:Pattern=`^Hostname|IPAddress|NamedAddress|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$` type AddressType = v1beta1.AddressType +// Duration is a string value representing a duration in time. The format is as specified +// in GEP-2257, a strict subset of the syntax parsed by Golang time.ParseDuration. +// +// +kubebuilder:validation:Pattern=`^([0-9]{1,5}(h|m|s|ms)){1,4}$` +type Duration v1beta1.Duration + const ( // A textual representation of a numeric IP address. IPv4 // addresses must be in dotted-decimal form. IPv6 addresses diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index 98a6a5a42a..cb7c22de06 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -274,14 +274,19 @@ type HTTPRouteRule struct { } // HTTPRouteTimeouts defines timeouts that can be configured for an HTTPRoute. -// Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration -// and MUST BE >= 1ms or 0 to disable (no timeout). +// Timeout values are formatted like 1h/1m/1s/1ms, as specified in GEP-2257, +// and MUST BE >= 1ms or "0s" to disable (no timeout). +// +// +kubebuilder:validation:XValidation:message="request timeout value must be greater than or equal to 1ms",rule="!(has(self.request) && duration(self.request) != duration('0s') && duration(self.request) < duration('1ms'))" +// +kubebuilder:validation:XValidation:message="backendRequest timeout value must be greater than or equal to 1ms",rule="!(has(self.backendRequest) && duration(self.backendRequest) != duration('0s') && duration(self.backendRequest) < duration('1ms'))" +// +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { // Request specifies the duration for processing an HTTP client request after which the // gateway will time out if unable to send a response. // - // For example, setting this field to the value `10s` in an HTTPRoute will cause - // a timeout if a client request is taking longer than 10 seconds to complete. + // For example, setting the `rules.timeouts.request` field to the value `10s` in an + // `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds + // to complete. // // This timeout is intended to cover as close to the whole request-response transaction // as possible although an implementation MAY choose to start the timeout after the entire @@ -303,8 +308,8 @@ type HTTPRouteTimeouts struct { // may result in more than one call from the gateway to the destination backend service, // for example, if automatic retries are supported. // - // Because the Request timeout encompasses the BackendRequest timeout, - // the value of BackendRequest defaults to and must be <= the value of Request timeout. + // Because the Request timeout encompasses the BackendRequest timeout, the value of + // BackendRequest must be <= the value of Request timeout. // // Support: Extended // diff --git a/apis/v1beta1/shared_types.go b/apis/v1beta1/shared_types.go index 401b90827a..b879d3a1fd 100644 --- a/apis/v1beta1/shared_types.go +++ b/apis/v1beta1/shared_types.go @@ -610,7 +610,8 @@ type AddressType string // +k8s:deepcopy-gen=false type HeaderName string -// A duration is a ... +// Duration is a string value representing a duration in time. The format is as specified +// in GEP-2257, a strict subset of the syntax parsed by Golang time.ParseDuration. // // +kubebuilder:validation:Pattern=`^([0-9]{1,5}(h|m|s|ms)){1,4}$` type Duration string diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index 4ec0083fa6..97d07475db 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -1203,7 +1203,7 @@ func TestValidateHTTPTimeouts(t *testing.T) { rules: []gatewayv1b1.HTTPRouteRule{ { Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("0ms"), + Request: toDuration("0s"), }, }, }, diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index f7a3c0290a..39b85d1908 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2128,27 +2128,40 @@ spec: to the destination backend service, for example, if automatic retries are supported. \n Because the Request timeout encompasses the BackendRequest timeout, the value of BackendRequest - defaults to and must be <= the value of Request timeout. - \n Support: Extended" + must be <= the value of Request timeout. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string request: description: "Request specifies the duration for processing an HTTP client request after which the gateway will time out if unable to send a response. \n For example, setting - this field to the value `10s` in an HTTPRoute will cause - a timeout if a client request is taking longer than 10 - seconds to complete. \n This timeout is intended to cover - as close to the whole request-response transaction as - possible although an implementation MAY choose to start - the timeout after the entire request stream has been received - instead of immediately after the transaction is initiated - by the client. \n When this field is unspecified, request - timeout behavior is implementation-dependent. \n Support: - Extended" + the `rules.timeouts.request` field to the value `10s` + in an `HTTPRoute` will cause a timeout if a client request + is taking longer than 10 seconds to complete. \n This + timeout is intended to cover as close to the whole request-response + transaction as possible although an implementation MAY + choose to start the timeout after the entire request stream + has been received instead of immediately after the transaction + is initiated by the client. \n When this field is unspecified, + request timeout behavior is implementation-dependent. + \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string type: object + x-kubernetes-validations: + - message: request timeout value must be greater than or equal + to 1ms + rule: '!(has(self.request) && duration(self.request) != duration(''0s'') + && duration(self.request) < duration(''1ms''))' + - message: backendRequest timeout value must be greater than + or equal to 1ms + rule: '!(has(self.backendRequest) && duration(self.backendRequest) + != duration(''0s'') && duration(self.backendRequest) < duration(''1ms''))' + - message: backendRequest timeout cannot be longer than request + timeout + rule: '!(has(self.request) && has(self.backendRequest) && + duration(self.request) != duration(''0s'') && duration(self.backendRequest) + > duration(self.request))' type: object x-kubernetes-validations: - message: RequestRedirect filter must not be used together with @@ -4563,27 +4576,40 @@ spec: to the destination backend service, for example, if automatic retries are supported. \n Because the Request timeout encompasses the BackendRequest timeout, the value of BackendRequest - defaults to and must be <= the value of Request timeout. - \n Support: Extended" + must be <= the value of Request timeout. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string request: description: "Request specifies the duration for processing an HTTP client request after which the gateway will time out if unable to send a response. \n For example, setting - this field to the value `10s` in an HTTPRoute will cause - a timeout if a client request is taking longer than 10 - seconds to complete. \n This timeout is intended to cover - as close to the whole request-response transaction as - possible although an implementation MAY choose to start - the timeout after the entire request stream has been received - instead of immediately after the transaction is initiated - by the client. \n When this field is unspecified, request - timeout behavior is implementation-dependent. \n Support: - Extended" + the `rules.timeouts.request` field to the value `10s` + in an `HTTPRoute` will cause a timeout if a client request + is taking longer than 10 seconds to complete. \n This + timeout is intended to cover as close to the whole request-response + transaction as possible although an implementation MAY + choose to start the timeout after the entire request stream + has been received instead of immediately after the transaction + is initiated by the client. \n When this field is unspecified, + request timeout behavior is implementation-dependent. + \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string type: object + x-kubernetes-validations: + - message: request timeout value must be greater than or equal + to 1ms + rule: '!(has(self.request) && duration(self.request) != duration(''0s'') + && duration(self.request) < duration(''1ms''))' + - message: backendRequest timeout value must be greater than + or equal to 1ms + rule: '!(has(self.backendRequest) && duration(self.backendRequest) + != duration(''0s'') && duration(self.backendRequest) < duration(''1ms''))' + - message: backendRequest timeout cannot be longer than request + timeout + rule: '!(has(self.request) && has(self.backendRequest) && + duration(self.request) != duration(''0s'') && duration(self.backendRequest) + > duration(self.request))' type: object x-kubernetes-validations: - message: RequestRedirect filter must not be used together with From d8f2b2bd1792c458eeb93fffee6c08f0392c05f6 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 1 Sep 2023 15:32:19 +0100 Subject: [PATCH 03/11] httproute-timeout: address comments * remove no-longer needed validation based on GEP-2257 * fix type alias in v1alpha2 for Duration type --- apis/v1alpha2/shared_types.go | 2 +- apis/v1beta1/httproute_types.go | 2 -- .../gateway.networking.k8s.io_httproutes.yaml | 16 ---------------- 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/apis/v1alpha2/shared_types.go b/apis/v1alpha2/shared_types.go index 7286f4d216..73927ed0e2 100644 --- a/apis/v1alpha2/shared_types.go +++ b/apis/v1alpha2/shared_types.go @@ -344,7 +344,7 @@ type AddressType = v1beta1.AddressType // in GEP-2257, a strict subset of the syntax parsed by Golang time.ParseDuration. // // +kubebuilder:validation:Pattern=`^([0-9]{1,5}(h|m|s|ms)){1,4}$` -type Duration v1beta1.Duration +type Duration = v1beta1.Duration const ( // A textual representation of a numeric IP address. IPv4 diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index cb7c22de06..9f335422b2 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -277,8 +277,6 @@ type HTTPRouteRule struct { // Timeout values are formatted like 1h/1m/1s/1ms, as specified in GEP-2257, // and MUST BE >= 1ms or "0s" to disable (no timeout). // -// +kubebuilder:validation:XValidation:message="request timeout value must be greater than or equal to 1ms",rule="!(has(self.request) && duration(self.request) != duration('0s') && duration(self.request) < duration('1ms'))" -// +kubebuilder:validation:XValidation:message="backendRequest timeout value must be greater than or equal to 1ms",rule="!(has(self.backendRequest) && duration(self.backendRequest) != duration('0s') && duration(self.backendRequest) < duration('1ms'))" // +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { // Request specifies the duration for processing an HTTP client request after which the diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index 39b85d1908..3f145e419c 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2149,14 +2149,6 @@ spec: type: string type: object x-kubernetes-validations: - - message: request timeout value must be greater than or equal - to 1ms - rule: '!(has(self.request) && duration(self.request) != duration(''0s'') - && duration(self.request) < duration(''1ms''))' - - message: backendRequest timeout value must be greater than - or equal to 1ms - rule: '!(has(self.backendRequest) && duration(self.backendRequest) - != duration(''0s'') && duration(self.backendRequest) < duration(''1ms''))' - message: backendRequest timeout cannot be longer than request timeout rule: '!(has(self.request) && has(self.backendRequest) && @@ -4597,14 +4589,6 @@ spec: type: string type: object x-kubernetes-validations: - - message: request timeout value must be greater than or equal - to 1ms - rule: '!(has(self.request) && duration(self.request) != duration(''0s'') - && duration(self.request) < duration(''1ms''))' - - message: backendRequest timeout value must be greater than - or equal to 1ms - rule: '!(has(self.backendRequest) && duration(self.backendRequest) - != duration(''0s'') && duration(self.backendRequest) < duration(''1ms''))' - message: backendRequest timeout cannot be longer than request timeout rule: '!(has(self.request) && has(self.backendRequest) && From e56b07bba434dcad3253d6ff03a07083f1aa198d Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Thu, 7 Sep 2023 21:26:02 +0100 Subject: [PATCH 04/11] httproute-timeout * remove kubebuilder validation from v1alpha2 shared_types * fix comments as per discussion * add to unit tests * update httproute.go (remove old code) * update crd --- apis/v1alpha2/shared_types.go | 2 - apis/v1beta1/httproute_types.go | 10 +-- apis/v1beta1/validation/httproute.go | 28 +----- apis/v1beta1/validation/httproute_test.go | 29 +++++- .../gateway.networking.k8s.io_httproutes.yaml | 90 ++++++++++--------- 5 files changed, 79 insertions(+), 80 deletions(-) diff --git a/apis/v1alpha2/shared_types.go b/apis/v1alpha2/shared_types.go index 73927ed0e2..788fef9a06 100644 --- a/apis/v1alpha2/shared_types.go +++ b/apis/v1alpha2/shared_types.go @@ -342,8 +342,6 @@ type AddressType = v1beta1.AddressType // Duration is a string value representing a duration in time. The format is as specified // in GEP-2257, a strict subset of the syntax parsed by Golang time.ParseDuration. -// -// +kubebuilder:validation:Pattern=`^([0-9]{1,5}(h|m|s|ms)){1,4}$` type Duration = v1beta1.Duration const ( diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index 9f335422b2..4a1e269ef7 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -279,8 +279,8 @@ type HTTPRouteRule struct { // // +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { - // Request specifies the duration for processing an HTTP client request after which the - // gateway will time out if unable to send a response. + // Request specifies the maximum duration for a Gateway to respond to a HTTP request. If the Gateway + // has not been able to respond before this deadline is met, the Gateway MUST return a timeout error. // // For example, setting the `rules.timeouts.request` field to the value `10s` in an // `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds @@ -291,7 +291,7 @@ type HTTPRouteTimeouts struct { // request stream has been received instead of immediately after the transaction is // initiated by the client. // - // When this field is unspecified, request timeout behavior is implementation-dependent. + // When this field is unspecified, request timeout behavior is implementation-specific. // // Support: Extended // @@ -299,11 +299,11 @@ type HTTPRouteTimeouts struct { Request *Duration `json:"request,omitempty"` // BackendRequest specifies a timeout for an individual request from the gateway - // to a backend service. This covers the time from when the request first starts being + // to a backend. This covers the time from when the request first starts being // sent from the gateway to when the full response has been received from the backend. // // An entire client HTTP transaction with a gateway, covered by the Request timeout, - // may result in more than one call from the gateway to the destination backend service, + // may result in more than one call from the gateway to the destination backend, // for example, if automatic retries are supported. // // Because the Request timeout encompasses the BackendRequest timeout, the value of diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go index 9f1ce3cc96..76f76400a0 100644 --- a/apis/v1beta1/validation/httproute.go +++ b/apis/v1beta1/validation/httproute.go @@ -42,9 +42,6 @@ var ( // All valid path characters per RFC-3986 validPathCharacters = "^(?:[A-Za-z0-9\\/\\-._~!$&'()*+,;=:@]|[%][0-9a-fA-F]{2})+$" - - // Minimum value of timeout fields - timeoutMinValue = time.Millisecond * 1 ) // ValidateHTTPRoute validates HTTPRoute according to the Gateway API specification. @@ -358,34 +355,15 @@ func validateHTTPHeaderModifier(filter gatewayv1b1.HTTPHeaderFilter, path *field func validateHTTPRouteTimeouts(timeouts *gatewayv1b1.HTTPRouteTimeouts, path *field.Path) field.ErrorList { var errs field.ErrorList - errorString := fmt.Sprintf("timeout value must be greater than or equal to %s", timeoutMinValue) if timeouts.BackendRequest != nil { - backendTimeout, err := time.ParseDuration((string)(*timeouts.BackendRequest)) - if err != nil { - errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, err.Error())) - } - if backendTimeout < timeoutMinValue && backendTimeout != 0 { - errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, errorString)) - } + backendTimeout, _ := time.ParseDuration((string)(*timeouts.BackendRequest)) if timeouts.Request != nil { - timeout, err := time.ParseDuration((string)(*timeouts.Request)) - if err != nil { - errs = append(errs, field.Invalid(path.Child("request"), timeout, err.Error())) - } - if backendTimeout > timeout && timeout != 0 { + timeout, _ := time.ParseDuration((string)(*timeouts.Request)) + if backendTimeout > timeout { errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, "backendRequest timeout cannot be longer than request timeout")) } } } - if timeouts.Request != nil { - timeout, err := time.ParseDuration((string)(*timeouts.Request)) - if err != nil { - errs = append(errs, field.Invalid(path.Child("request"), timeout, err.Error())) - } - if timeout < timeoutMinValue && timeout != 0 { - errs = append(errs, field.Invalid(path.Child("request"), timeout, errorString)) - } - } return errs } diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index 97d07475db..b78dedb5ce 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -1198,7 +1198,7 @@ func TestValidateHTTPTimeouts(t *testing.T) { }, }, }, { - name: "valid httpRoute Rules timeout set to 0 (disabled)", + name: "valid httpRoute Rules timeout set to 0s (disabled)", errCount: 0, rules: []gatewayv1b1.HTTPRouteRule{ { @@ -1208,12 +1208,33 @@ func TestValidateHTTPTimeouts(t *testing.T) { }, }, }, { - name: "invalid httpRoute Rules timeout", - errCount: 1, + name: "valid httpRoute Rules timeout set to 0ms (disabled)", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0ms"), + }, + }, + }, + }, {}, { + name: "valid httpRoute Rules timeout set to 0h (disabled)", + errCount: 0, rules: []gatewayv1b1.HTTPRouteRule{ { Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("500us"), + Request: toDuration("0h"), + }, + }, + }, + }, {}, {}, { + name: "valid httpRoute Rules timeout and backendRequest have the same value", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1ms"), + BackendRequest: toDuration("1ms"), }, }, }, diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index 3f145e419c..9e5961ffb0 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2119,31 +2119,32 @@ spec: properties: backendRequest: description: "BackendRequest specifies a timeout for an - individual request from the gateway to a backend service. - This covers the time from when the request first starts - being sent from the gateway to when the full response - has been received from the backend. \n An entire client - HTTP transaction with a gateway, covered by the Request - timeout, may result in more than one call from the gateway - to the destination backend service, for example, if automatic - retries are supported. \n Because the Request timeout - encompasses the BackendRequest timeout, the value of BackendRequest - must be <= the value of Request timeout. \n Support: Extended" + individual request from the gateway to a backend. This + covers the time from when the request first starts being + sent from the gateway to when the full response has been + received from the backend. \n An entire client HTTP transaction + with a gateway, covered by the Request timeout, may result + in more than one call from the gateway to the destination + backend, for example, if automatic retries are supported. + \n Because the Request timeout encompasses the BackendRequest + timeout, the value of BackendRequest must be <= the value + of Request timeout. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string request: - description: "Request specifies the duration for processing - an HTTP client request after which the gateway will time - out if unable to send a response. \n For example, setting - the `rules.timeouts.request` field to the value `10s` - in an `HTTPRoute` will cause a timeout if a client request - is taking longer than 10 seconds to complete. \n This - timeout is intended to cover as close to the whole request-response - transaction as possible although an implementation MAY - choose to start the timeout after the entire request stream - has been received instead of immediately after the transaction - is initiated by the client. \n When this field is unspecified, - request timeout behavior is implementation-dependent. + description: "Request specifies the maximum duration for + a Gateway to respond to a HTTP request. If the Gateway + has not been able to respond before this deadline is met, + the Gateway MUST return a timeout error. \n For example, + setting the `rules.timeouts.request` field to the value + `10s` in an `HTTPRoute` will cause a timeout if a client + request is taking longer than 10 seconds to complete. + \n This timeout is intended to cover as close to the whole + request-response transaction as possible although an implementation + MAY choose to start the timeout after the entire request + stream has been received instead of immediately after + the transaction is initiated by the client. \n When this + field is unspecified, request timeout behavior is implementation-specific. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string @@ -4559,31 +4560,32 @@ spec: properties: backendRequest: description: "BackendRequest specifies a timeout for an - individual request from the gateway to a backend service. - This covers the time from when the request first starts - being sent from the gateway to when the full response - has been received from the backend. \n An entire client - HTTP transaction with a gateway, covered by the Request - timeout, may result in more than one call from the gateway - to the destination backend service, for example, if automatic - retries are supported. \n Because the Request timeout - encompasses the BackendRequest timeout, the value of BackendRequest - must be <= the value of Request timeout. \n Support: Extended" + individual request from the gateway to a backend. This + covers the time from when the request first starts being + sent from the gateway to when the full response has been + received from the backend. \n An entire client HTTP transaction + with a gateway, covered by the Request timeout, may result + in more than one call from the gateway to the destination + backend, for example, if automatic retries are supported. + \n Because the Request timeout encompasses the BackendRequest + timeout, the value of BackendRequest must be <= the value + of Request timeout. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string request: - description: "Request specifies the duration for processing - an HTTP client request after which the gateway will time - out if unable to send a response. \n For example, setting - the `rules.timeouts.request` field to the value `10s` - in an `HTTPRoute` will cause a timeout if a client request - is taking longer than 10 seconds to complete. \n This - timeout is intended to cover as close to the whole request-response - transaction as possible although an implementation MAY - choose to start the timeout after the entire request stream - has been received instead of immediately after the transaction - is initiated by the client. \n When this field is unspecified, - request timeout behavior is implementation-dependent. + description: "Request specifies the maximum duration for + a Gateway to respond to a HTTP request. If the Gateway + has not been able to respond before this deadline is met, + the Gateway MUST return a timeout error. \n For example, + setting the `rules.timeouts.request` field to the value + `10s` in an `HTTPRoute` will cause a timeout if a client + request is taking longer than 10 seconds to complete. + \n This timeout is intended to cover as close to the whole + request-response transaction as possible although an implementation + MAY choose to start the timeout after the entire request + stream has been received instead of immediately after + the transaction is initiated by the client. \n When this + field is unspecified, request timeout behavior is implementation-specific. \n Support: Extended" pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$ type: string From e148233a0a32ebdd63d5d8c52468f0910ef345a2 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Thu, 7 Sep 2023 22:11:05 +0100 Subject: [PATCH 05/11] add CEL tests for HTTPRouteTimeouts --- pkg/test/cel/httproute_test.go | 91 ++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/pkg/test/cel/httproute_test.go b/pkg/test/cel/httproute_test.go index f80e004c1e..91c79f2e8d 100644 --- a/pkg/test/cel/httproute_test.go +++ b/pkg/test/cel/httproute_test.go @@ -1391,3 +1391,94 @@ func validateHTTPRoute(t *testing.T, route *gatewayv1b1.HTTPRoute, wantErrors [] t.Errorf("Unexpected response while creating HTTPRoute %q; got err=\n%v\n;missing strings within error=%q", fmt.Sprintf("%v/%v", route.Namespace, route.Name), err, missingErrorStrings) } } + +func toDuration(durationString string) *gatewayv1b1.Duration { + return (*gatewayv1b1.Duration)(&durationString) +} + +func TestHTTPRouteTimeouts(t *testing.T) { + tests := []struct { + name string + wantErrors []string + rules []gatewayv1b1.HTTPRouteRule + }{ + { + name: "invalid timeout unit us is not supported", + wantErrors: []string{"Invalid value: \"100us\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("100us"), + }, + }, + }, + }, + { + name: "invalid timeout unit ns is not supported", + wantErrors: []string{"Invalid value: \"500ns\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("500ns"), + }, + }, + }, + }, + { + name: "valid timeout request and backendRequest", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("4s"), + BackendRequest: toDuration("2s"), + }, + }, + }, + }, + { + name: "valid timeout request", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0s"), + }, + }, + }, + }, + { + name: "invalid timeout request day unit not supported", + wantErrors: []string{"Invalid value: \"1d\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1d"), + }, + }, + }, + }, + { + name: "invalid timeout request decimal not supported ", + wantErrors: []string{"Invalid value: \"0.5s\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0.5s"), + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + route := &gatewayv1b1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("foo-%v", time.Now().UnixNano()), + Namespace: metav1.NamespaceDefault, + }, + Spec: gatewayv1b1.HTTPRouteSpec{Rules: tc.rules}, + } + validateHTTPRoute(t, route, tc.wantErrors) + }) + } +} From a9bd3a912f04a152b38034cdb56cd31b1dc41f51 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Thu, 7 Sep 2023 22:26:39 +0100 Subject: [PATCH 06/11] add CEL test for valide infinite request --- pkg/test/cel/httproute_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/test/cel/httproute_test.go b/pkg/test/cel/httproute_test.go index 91c79f2e8d..19ba2b78ee 100644 --- a/pkg/test/cel/httproute_test.go +++ b/pkg/test/cel/httproute_test.go @@ -1467,6 +1467,17 @@ func TestHTTPRouteTimeouts(t *testing.T) { }, }, }, + { + name: "valid timeout request infinite greater than backend request 1ms", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0s"), + BackendRequest: toDuration("1ms"), + }, + }, + }, + }, } for _, tc := range tests { From 482ce1559fb95af38b191cef5eb30f9bfd638e54 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 8 Sep 2023 14:59:30 +0100 Subject: [PATCH 07/11] httproute-timeout: address comments * add to cel tests * add same to webhook tests * update comment on HTTPRouteTimeouts type --- apis/v1beta1/httproute_types.go | 4 +-- apis/v1beta1/validation/httproute_test.go | 35 +++++++++++++++++++++- pkg/test/cel/httproute_test.go | 36 ++++++++++++++++++++++- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index 4a1e269ef7..b997ad9aea 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -274,8 +274,8 @@ type HTTPRouteRule struct { } // HTTPRouteTimeouts defines timeouts that can be configured for an HTTPRoute. -// Timeout values are formatted like 1h/1m/1s/1ms, as specified in GEP-2257, -// and MUST BE >= 1ms or "0s" to disable (no timeout). +// Timeout values are represented with Gateway API Duration formatting. +// Specifying a zero value such as "0s" is interpreted as no timeout. // // +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index b78dedb5ce..26653b961f 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -1227,7 +1227,7 @@ func TestValidateHTTPTimeouts(t *testing.T) { }, }, }, - }, {}, {}, { + }, { name: "valid httpRoute Rules timeout and backendRequest have the same value", errCount: 0, rules: []gatewayv1b1.HTTPRouteRule{ @@ -1249,6 +1249,39 @@ func TestValidateHTTPTimeouts(t *testing.T) { }, }, }, + }, { + name: "valid httpRoute Rules request timeout 1s and backendRequest timeout 200ms", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1s"), + BackendRequest: toDuration("200ms"), + }, + }, + }, + }, { + name: "valid httpRoute Rules request timeout 10s and backendRequest timeout 10s", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("10s"), + BackendRequest: toDuration("10s"), + }, + }, + }, + }, { + name: "invalid httpRoute Rules backendRequest timeout cannot be greater than request timeout", + errCount: 1, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("200ms"), + BackendRequest: toDuration("1s"), + }, + }, + }, }, } for _, tc := range tests { diff --git a/pkg/test/cel/httproute_test.go b/pkg/test/cel/httproute_test.go index 19ba2b78ee..5dd4682a39 100644 --- a/pkg/test/cel/httproute_test.go +++ b/pkg/test/cel/httproute_test.go @@ -1468,7 +1468,7 @@ func TestHTTPRouteTimeouts(t *testing.T) { }, }, { - name: "valid timeout request infinite greater than backend request 1ms", + name: "valid timeout request infinite greater than backendRequest 1ms", rules: []gatewayv1b1.HTTPRouteRule{ { Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ @@ -1478,6 +1478,40 @@ func TestHTTPRouteTimeouts(t *testing.T) { }, }, }, + { + name: "valid timeout request 1s greater than backendRequest 200ms", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1s"), + BackendRequest: toDuration("200ms"), + }, + }, + }, + }, + { + name: "valid timeout request 10s equal backendRequest 10s", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("10s"), + BackendRequest: toDuration("10s"), + }, + }, + }, + }, + { + name: "invalid timeout request 200ms less than backendRequest 1s", + wantErrors: []string{"Invalid value: \"object\": backendRequest timeout cannot be longer than request timeout"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("200ms"), + BackendRequest: toDuration("1s"), + }, + }, + }, + }, } for _, tc := range tests { From df718175e7eca41ee34401ba7eeeb044358043a5 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 8 Sep 2023 15:19:48 +0100 Subject: [PATCH 08/11] shorten comment lines and minor grammar fix --- apis/v1beta1/httproute_types.go | 5 +++-- .../experimental/gateway.networking.k8s.io_httproutes.yaml | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index b997ad9aea..d89b66ef2d 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -279,8 +279,9 @@ type HTTPRouteRule struct { // // +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { - // Request specifies the maximum duration for a Gateway to respond to a HTTP request. If the Gateway - // has not been able to respond before this deadline is met, the Gateway MUST return a timeout error. + // Request specifies the maximum duration for a Gateway to respond to an HTTP request. + // If the Gateway has not been able to respond before this deadline is met, the Gateway + // MUST return a timeout error. // // For example, setting the `rules.timeouts.request` field to the value `10s` in an // `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index 9e5961ffb0..9f6c6f813d 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2133,7 +2133,7 @@ spec: type: string request: description: "Request specifies the maximum duration for - a Gateway to respond to a HTTP request. If the Gateway + a Gateway to respond to an HTTP request. If the Gateway has not been able to respond before this deadline is met, the Gateway MUST return a timeout error. \n For example, setting the `rules.timeouts.request` field to the value @@ -4574,7 +4574,7 @@ spec: type: string request: description: "Request specifies the maximum duration for - a Gateway to respond to a HTTP request. If the Gateway + a Gateway to respond to an HTTP request. If the Gateway has not been able to respond before this deadline is met, the Gateway MUST return a timeout error. \n For example, setting the `rules.timeouts.request` field to the value From d8d74726cc2f2a1c2e1e853865745ce3caee3af4 Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 8 Sep 2023 16:26:32 +0100 Subject: [PATCH 09/11] fix webhook validation and add test --- apis/v1beta1/validation/httproute.go | 2 +- apis/v1beta1/validation/httproute_test.go | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go index 76f76400a0..a1c96b8722 100644 --- a/apis/v1beta1/validation/httproute.go +++ b/apis/v1beta1/validation/httproute.go @@ -359,7 +359,7 @@ func validateHTTPRouteTimeouts(timeouts *gatewayv1b1.HTTPRouteTimeouts, path *fi backendTimeout, _ := time.ParseDuration((string)(*timeouts.BackendRequest)) if timeouts.Request != nil { timeout, _ := time.ParseDuration((string)(*timeouts.Request)) - if backendTimeout > timeout { + if backendTimeout > timeout && timeout != 0 { errs = append(errs, field.Invalid(path.Child("backendRequest"), backendTimeout, "backendRequest timeout cannot be longer than request timeout")) } } diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index 26653b961f..ca29f0b067 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -1282,6 +1282,17 @@ func TestValidateHTTPTimeouts(t *testing.T) { }, }, }, + }, { + name: "valid httpRoute Rules request 0s (infinite) and backendRequest 100ms", + errCount: 0, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0s"), + BackendRequest: toDuration("100ms"), + }, + }, + }, }, } for _, tc := range tests { From e40ed42b80c4c98c3d5dd160a88a359d4bd4203d Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 8 Sep 2023 16:35:33 +0100 Subject: [PATCH 10/11] fix capitalisation when referring to gateway instance --- apis/v1beta1/httproute_types.go | 4 ++-- .../gateway.networking.k8s.io_httproutes.yaml | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index d89b66ef2d..fb6809ddb3 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -279,8 +279,8 @@ type HTTPRouteRule struct { // // +kubebuilder:validation:XValidation:message="backendRequest timeout cannot be longer than request timeout",rule="!(has(self.request) && has(self.backendRequest) && duration(self.request) != duration('0s') && duration(self.backendRequest) > duration(self.request))" type HTTPRouteTimeouts struct { - // Request specifies the maximum duration for a Gateway to respond to an HTTP request. - // If the Gateway has not been able to respond before this deadline is met, the Gateway + // Request specifies the maximum duration for a gateway to respond to an HTTP request. + // If the gateway has not been able to respond before this deadline is met, the gateway // MUST return a timeout error. // // For example, setting the `rules.timeouts.request` field to the value `10s` in an diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index 9f6c6f813d..020cdc1ac3 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -2133,9 +2133,9 @@ spec: type: string request: description: "Request specifies the maximum duration for - a Gateway to respond to an HTTP request. If the Gateway + a gateway to respond to an HTTP request. If the gateway has not been able to respond before this deadline is met, - the Gateway MUST return a timeout error. \n For example, + the gateway MUST return a timeout error. \n For example, setting the `rules.timeouts.request` field to the value `10s` in an `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds to complete. @@ -4574,9 +4574,9 @@ spec: type: string request: description: "Request specifies the maximum duration for - a Gateway to respond to an HTTP request. If the Gateway + a gateway to respond to an HTTP request. If the gateway has not been able to respond before this deadline is met, - the Gateway MUST return a timeout error. \n For example, + the gateway MUST return a timeout error. \n For example, setting the `rules.timeouts.request` field to the value `10s` in an `HTTPRoute` will cause a timeout if a client request is taking longer than 10 seconds to complete. From acadd11d95e1f4209ac1bd628dd8d36e9a02e32c Mon Sep 17 00:00:00 2001 From: Simone-Rodigari Date: Fri, 8 Sep 2023 20:12:14 +0100 Subject: [PATCH 11/11] move CEL tests to httproute_experimental_test.go --- pkg/test/cel/httproute_experimental_test.go | 136 ++++++++++++++++++++ pkg/test/cel/httproute_test.go | 136 -------------------- 2 files changed, 136 insertions(+), 136 deletions(-) diff --git a/pkg/test/cel/httproute_experimental_test.go b/pkg/test/cel/httproute_experimental_test.go index d77ea3638a..7cbb064c4a 100644 --- a/pkg/test/cel/httproute_experimental_test.go +++ b/pkg/test/cel/httproute_experimental_test.go @@ -233,3 +233,139 @@ func TestHTTPRouteParentRefExperimental(t *testing.T) { }) } } + +func toDuration(durationString string) *gatewayv1b1.Duration { + return (*gatewayv1b1.Duration)(&durationString) +} + +func TestHTTPRouteTimeouts(t *testing.T) { + tests := []struct { + name string + wantErrors []string + rules []gatewayv1b1.HTTPRouteRule + }{ + { + name: "invalid timeout unit us is not supported", + wantErrors: []string{"Invalid value: \"100us\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("100us"), + }, + }, + }, + }, + { + name: "invalid timeout unit ns is not supported", + wantErrors: []string{"Invalid value: \"500ns\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("500ns"), + }, + }, + }, + }, + { + name: "valid timeout request and backendRequest", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("4s"), + BackendRequest: toDuration("2s"), + }, + }, + }, + }, + { + name: "valid timeout request", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0s"), + }, + }, + }, + }, + { + name: "invalid timeout request day unit not supported", + wantErrors: []string{"Invalid value: \"1d\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1d"), + }, + }, + }, + }, + { + name: "invalid timeout request decimal not supported ", + wantErrors: []string{"Invalid value: \"0.5s\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0.5s"), + }, + }, + }, + }, + { + name: "valid timeout request infinite greater than backendRequest 1ms", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("0s"), + BackendRequest: toDuration("1ms"), + }, + }, + }, + }, + { + name: "valid timeout request 1s greater than backendRequest 200ms", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("1s"), + BackendRequest: toDuration("200ms"), + }, + }, + }, + }, + { + name: "valid timeout request 10s equal backendRequest 10s", + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("10s"), + BackendRequest: toDuration("10s"), + }, + }, + }, + }, + { + name: "invalid timeout request 200ms less than backendRequest 1s", + wantErrors: []string{"Invalid value: \"object\": backendRequest timeout cannot be longer than request timeout"}, + rules: []gatewayv1b1.HTTPRouteRule{ + { + Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ + Request: toDuration("200ms"), + BackendRequest: toDuration("1s"), + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + route := &gatewayv1b1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("foo-%v", time.Now().UnixNano()), + Namespace: metav1.NamespaceDefault, + }, + Spec: gatewayv1b1.HTTPRouteSpec{Rules: tc.rules}, + } + validateHTTPRoute(t, route, tc.wantErrors) + }) + } +} diff --git a/pkg/test/cel/httproute_test.go b/pkg/test/cel/httproute_test.go index 5dd4682a39..f80e004c1e 100644 --- a/pkg/test/cel/httproute_test.go +++ b/pkg/test/cel/httproute_test.go @@ -1391,139 +1391,3 @@ func validateHTTPRoute(t *testing.T, route *gatewayv1b1.HTTPRoute, wantErrors [] t.Errorf("Unexpected response while creating HTTPRoute %q; got err=\n%v\n;missing strings within error=%q", fmt.Sprintf("%v/%v", route.Namespace, route.Name), err, missingErrorStrings) } } - -func toDuration(durationString string) *gatewayv1b1.Duration { - return (*gatewayv1b1.Duration)(&durationString) -} - -func TestHTTPRouteTimeouts(t *testing.T) { - tests := []struct { - name string - wantErrors []string - rules []gatewayv1b1.HTTPRouteRule - }{ - { - name: "invalid timeout unit us is not supported", - wantErrors: []string{"Invalid value: \"100us\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("100us"), - }, - }, - }, - }, - { - name: "invalid timeout unit ns is not supported", - wantErrors: []string{"Invalid value: \"500ns\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("500ns"), - }, - }, - }, - }, - { - name: "valid timeout request and backendRequest", - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("4s"), - BackendRequest: toDuration("2s"), - }, - }, - }, - }, - { - name: "valid timeout request", - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("0s"), - }, - }, - }, - }, - { - name: "invalid timeout request day unit not supported", - wantErrors: []string{"Invalid value: \"1d\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("1d"), - }, - }, - }, - }, - { - name: "invalid timeout request decimal not supported ", - wantErrors: []string{"Invalid value: \"0.5s\": spec.rules[0].timeouts.request in body should match '^([0-9]{1,5}(h|m|s|ms)){1,4}$'"}, - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("0.5s"), - }, - }, - }, - }, - { - name: "valid timeout request infinite greater than backendRequest 1ms", - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("0s"), - BackendRequest: toDuration("1ms"), - }, - }, - }, - }, - { - name: "valid timeout request 1s greater than backendRequest 200ms", - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("1s"), - BackendRequest: toDuration("200ms"), - }, - }, - }, - }, - { - name: "valid timeout request 10s equal backendRequest 10s", - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("10s"), - BackendRequest: toDuration("10s"), - }, - }, - }, - }, - { - name: "invalid timeout request 200ms less than backendRequest 1s", - wantErrors: []string{"Invalid value: \"object\": backendRequest timeout cannot be longer than request timeout"}, - rules: []gatewayv1b1.HTTPRouteRule{ - { - Timeouts: &gatewayv1b1.HTTPRouteTimeouts{ - Request: toDuration("200ms"), - BackendRequest: toDuration("1s"), - }, - }, - }, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - route := &gatewayv1b1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("foo-%v", time.Now().UnixNano()), - Namespace: metav1.NamespaceDefault, - }, - Spec: gatewayv1b1.HTTPRouteSpec{Rules: tc.rules}, - } - validateHTTPRoute(t, route, tc.wantErrors) - }) - } -}