diff --git a/internal/annotation/annotations.go b/internal/annotation/annotations.go index d1b1464e4a5..6aafe60ffc7 100644 --- a/internal/annotation/annotations.go +++ b/internal/annotation/annotations.go @@ -17,9 +17,9 @@ import ( "fmt" "strconv" "strings" - "time" envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth" + "github.com/projectcontour/contour/internal/timeout" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -182,8 +182,8 @@ func NumRetries(i *v1beta1.Ingress) uint32 { } // PerTryTimeout returns the duration envoy will wait per retry cycle. -func PerTryTimeout(i *v1beta1.Ingress) time.Duration { - return ParseTimeout(CompatAnnotation(i, "per-try-timeout")) +func PerTryTimeout(i *v1beta1.Ingress) timeout.Setting { + return timeout.Parse(CompatAnnotation(i, "per-try-timeout")) } // IngressClass returns the first matching ingress class for the following diff --git a/internal/annotation/timeout.go b/internal/annotation/timeout.go deleted file mode 100644 index 6ac475256f2..00000000000 --- a/internal/annotation/timeout.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright Project Contour Authors -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package annotation - -import "time" - -// TODO(youngnick): This needs to move to another package, but we need to be careful -// about the import graph, this must stay a leaf node. - -// ParseTimeout parses timeouts we pass in various places in a standard way. -func ParseTimeout(timeout string) time.Duration { - if timeout == "" { - // Blank is interpreted as no timeout specified, use envoy defaults - // By default envoy applies a 15 second timeout to all backend requests. - // The explicit value 0 turns off the timeout, implying "never time out" - // https://www.envoyproxy.io/docs/envoy/v1.5.0/api-v2/rds.proto#routeaction - return 0 - } - - // Interpret "infinity" explicitly as an infinite timeout, which envoy config - // expects as a timeout of 0. This could be specified with the duration string - // "0s" but want to give an explicit out for operators. - if timeout == "infinity" { - return -1 - } - - d, err := time.ParseDuration(timeout) - if err != nil { - // TODO(cmalonty) plumb a logger in here so we can log this error. - // Assuming infinite duration is going to surprise people less for - // a not-parseable duration than a implicit 15 second one. - return -1 - } - return d -} diff --git a/internal/assert/assert.go b/internal/assert/assert.go index 271e948d4e8..51c0036968a 100644 --- a/internal/assert/assert.go +++ b/internal/assert/assert.go @@ -32,22 +32,22 @@ type Assert struct { // Equal will test that want == got, and call t.Fatal if it does not. // Notably, for errors, they are equal if they are both nil, or are both non-nil. // No value information is checked for errors. -func Equal(t *testing.T, want, got interface{}) { +func Equal(t *testing.T, want, got interface{}, opts ...cmp.Option) { t.Helper() - Assert{t}.Equal(want, got) + Assert{t}.Equal(want, got, opts...) } // Equal will call t.Fatal if want and got are not equal. -func (a Assert) Equal(want, got interface{}) { +func (a Assert) Equal(want, got interface{}, opts ...cmp.Option) { a.t.Helper() - opts := []cmp.Option{ + opts = append(opts, cmpopts.IgnoreFields(v2.DiscoveryResponse{}, "VersionInfo", "Nonce"), cmpopts.AcyclicTransformer("UnmarshalAny", unmarshalAny), // errors to be equal only if both are nil or both are non-nil. cmp.Comparer(func(x, y error) bool { return (x == nil) == (y == nil) }), - } + ) diff := cmp.Diff(want, got, opts...) if diff != "" { a.t.Fatal(diff) diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go index 5d7840c63aa..e01d7b5fe5d 100644 --- a/internal/dag/builder_test.go +++ b/internal/dag/builder_test.go @@ -21,6 +21,7 @@ import ( envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth" "github.com/google/go-cmp/cmp" projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" + "github.com/projectcontour/contour/internal/timeout" v1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -3687,8 +3688,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, // invalid timeout equals infinity ¯\_(ツ)_/¯. + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), // invalid timeout equals infinity ¯\_(ツ)_/¯. }, }), ), @@ -3707,8 +3708,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, // invalid timeout equals infinity ¯\_(ツ)_/¯. + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), // invalid timeout equals infinity ¯\_(ツ)_/¯. }, }), ), @@ -3728,8 +3729,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("bar.com", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, // invalid timeout equals the default, 90s. + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), // invalid timeout equals infinity ¯\_(ツ)_/¯. }, }), ), @@ -3748,8 +3749,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: 90 * time.Second, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(90 * time.Second), }, }), ), @@ -3768,8 +3769,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: 90 * time.Second, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(90 * time.Second), }, }), ), @@ -3789,8 +3790,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("bar.com", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: 90 * time.Second, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(90 * time.Second), }, }), ), @@ -3809,8 +3810,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), }, }), ), @@ -3829,8 +3830,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("*", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), }, }), ), @@ -3850,8 +3851,8 @@ func TestDAGInsert(t *testing.T) { virtualhost("bar.com", &Route{ PathMatchCondition: prefix("/"), Clusters: clustermap(s1), - TimeoutPolicy: &TimeoutPolicy{ - ResponseTimeout: -1, + TimeoutPolicy: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), }, }), ), @@ -3879,7 +3880,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "5xx", NumRetries: 6, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }), ), @@ -3901,7 +3902,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "5xx", NumRetries: 6, - PerTryTimeout: 0, + PerTryTimeout: timeout.DefaultSetting(), }, }), ), @@ -3923,7 +3924,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "5xx", NumRetries: 1, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }), ), @@ -3945,7 +3946,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "gateway-error", NumRetries: 6, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }), ), @@ -3967,7 +3968,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "gateway-error", NumRetries: 6, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }), ), @@ -3989,7 +3990,7 @@ func TestDAGInsert(t *testing.T) { RetryPolicy: &RetryPolicy{ RetryOn: "gateway-error", NumRetries: 6, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }), ), @@ -6025,6 +6026,7 @@ func TestDAGInsert(t *testing.T) { } opts := []cmp.Option{ cmp.AllowUnexported(VirtualHost{}), + cmp.AllowUnexported(timeout.Setting{}), } if diff := cmp.Diff(want, got, opts...); diff != "" { t.Fatal(diff) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index 3a6fac21bc3..0d63dde12df 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -22,6 +22,7 @@ import ( "time" envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth" + "github.com/projectcontour/contour/internal/timeout" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ) @@ -113,7 +114,7 @@ type Route struct { Websocket bool // TimeoutPolicy defines the timeout request/idle - TimeoutPolicy *TimeoutPolicy + TimeoutPolicy TimeoutPolicy // RetryPolicy defines the retry / number / timeout options for a route RetryPolicy *RetryPolicy @@ -147,12 +148,10 @@ func (r *Route) HasPathRegex() bool { type TimeoutPolicy struct { // ResponseTimeout is the timeout applied to the response // from the backend server. - // A timeout of zero implies "use envoy's default" - // A timeout of -1 represents "infinity" - ResponseTimeout time.Duration + ResponseTimeout timeout.Setting // IdleTimeout is the timeout applied to idle connections. - IdleTimeout time.Duration + IdleTimeout timeout.Setting } // RetryPolicy defines the retry / number / timeout options @@ -170,7 +169,7 @@ type RetryPolicy struct { // PerTryTimeout specifies the timeout per retry attempt. // Ignored if RetryOn is blank. - PerTryTimeout time.Duration + PerTryTimeout timeout.Setting } // MirrorPolicy defines the mirroring policy for a route. diff --git a/internal/dag/policy.go b/internal/dag/policy.go index 3c7a80e3179..8434be8bef4 100644 --- a/internal/dag/policy.go +++ b/internal/dag/policy.go @@ -21,6 +21,7 @@ import ( projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/annotation" + "github.com/projectcontour/contour/internal/timeout" "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -44,7 +45,17 @@ func retryPolicy(rp *projcontour.RetryPolicy) *RetryPolicy { if rp == nil { return nil } - perTryTimeout, _ := time.ParseDuration(rp.PerTryTimeout) + + // If PerTryTimeout is not a valid duration string, use the Envoy default + // value, otherwise use the provided value. + // TODO(sk) it might make sense to change the behavior here to be consistent + // with other timeout parsing, meaning use timeout.Parse which would result + // in a disabled per-try timeout if the input was not a valid duration. + perTryTimeout := timeout.DefaultSetting() + if perTryDuration, err := time.ParseDuration(rp.PerTryTimeout); err == nil { + perTryTimeout = timeout.DurationSetting(perTryDuration) + } + return &RetryPolicy{ RetryOn: retryOn(rp.RetryOn), RetriableStatusCodes: rp.RetriableStatusCodes, @@ -124,7 +135,7 @@ func ingressRetryPolicy(ingress *v1beta1.Ingress) *RetryPolicy { } } -func ingressTimeoutPolicy(ingress *v1beta1.Ingress) *TimeoutPolicy { +func ingressTimeoutPolicy(ingress *v1beta1.Ingress) TimeoutPolicy { response := annotation.CompatAnnotation(ingress, "response-timeout") if len(response) == 0 { // Note: due to a misunderstanding the name of the annotation is @@ -132,7 +143,10 @@ func ingressTimeoutPolicy(ingress *v1beta1.Ingress) *TimeoutPolicy { // the response body. response = annotation.CompatAnnotation(ingress, "request-timeout") if len(response) == 0 { - return nil + return TimeoutPolicy{ + ResponseTimeout: timeout.DefaultSetting(), + IdleTimeout: timeout.DefaultSetting(), + } } } // if the request timeout annotation is present on this ingress @@ -142,13 +156,16 @@ func ingressTimeoutPolicy(ingress *v1beta1.Ingress) *TimeoutPolicy { }) } -func timeoutPolicy(tp *projcontour.TimeoutPolicy) *TimeoutPolicy { +func timeoutPolicy(tp *projcontour.TimeoutPolicy) TimeoutPolicy { if tp == nil { - return nil + return TimeoutPolicy{ + ResponseTimeout: timeout.DefaultSetting(), + IdleTimeout: timeout.DefaultSetting(), + } } - return &TimeoutPolicy{ - ResponseTimeout: annotation.ParseTimeout(tp.Response), - IdleTimeout: annotation.ParseTimeout(tp.Idle), + return TimeoutPolicy{ + ResponseTimeout: timeout.Parse(tp.Response), + IdleTimeout: timeout.Parse(tp.Idle), } } diff --git a/internal/dag/policy_test.go b/internal/dag/policy_test.go index 9be3f2d1ddf..06f24f71a55 100644 --- a/internal/dag/policy_test.go +++ b/internal/dag/policy_test.go @@ -17,9 +17,10 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" - "github.com/projectcontour/contour/internal/annotation" "github.com/projectcontour/contour/internal/assert" + "github.com/projectcontour/contour/internal/timeout" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -113,7 +114,7 @@ func TestRetryPolicyIngress(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 0, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }, "no retry count, legacy per try timeout": { @@ -128,7 +129,7 @@ func TestRetryPolicyIngress(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 0, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }, "explicit 0s timeout": { @@ -143,7 +144,7 @@ func TestRetryPolicyIngress(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 0, - PerTryTimeout: 0 * time.Second, + PerTryTimeout: timeout.DefaultSetting(), }, }, "legacy explicit 0s timeout": { @@ -158,7 +159,7 @@ func TestRetryPolicyIngress(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 0, - PerTryTimeout: 0 * time.Second, + PerTryTimeout: timeout.DefaultSetting(), }, }, } @@ -166,7 +167,7 @@ func TestRetryPolicyIngress(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { got := ingressRetryPolicy(tc.i) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, got, cmp.AllowUnexported(timeout.Setting{})) }) } } @@ -203,7 +204,7 @@ func TestRetryPolicy(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 1, - PerTryTimeout: 10 * time.Second, + PerTryTimeout: timeout.DurationSetting(10 * time.Second), }, }, "explicit 0s timeout": { @@ -213,7 +214,7 @@ func TestRetryPolicy(t *testing.T) { want: &RetryPolicy{ RetryOn: "5xx", NumRetries: 1, - PerTryTimeout: 0 * time.Second, + PerTryTimeout: timeout.DefaultSetting(), }, }, "retry on": { @@ -240,7 +241,7 @@ func TestRetryPolicy(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { got := retryPolicy(tc.rp) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, got, cmp.AllowUnexported(timeout.Setting{})) }) } } @@ -248,52 +249,50 @@ func TestRetryPolicy(t *testing.T) { func TestTimeoutPolicy(t *testing.T) { tests := map[string]struct { tp *projcontour.TimeoutPolicy - want *TimeoutPolicy + want TimeoutPolicy }{ "nil timeout policy": { tp: nil, - want: nil, + want: TimeoutPolicy{}, }, "empty timeout policy": { - tp: &projcontour.TimeoutPolicy{}, - want: &TimeoutPolicy{ - ResponseTimeout: 0 * time.Second, - }, + tp: &projcontour.TimeoutPolicy{}, + want: TimeoutPolicy{}, }, "valid response timeout": { tp: &projcontour.TimeoutPolicy{ Response: "1m30s", }, - want: &TimeoutPolicy{ - ResponseTimeout: 90 * time.Second, + want: TimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(90 * time.Second), }, }, "invalid response timeout": { tp: &projcontour.TimeoutPolicy{ Response: "90", // 90 what? }, - want: &TimeoutPolicy{ + want: TimeoutPolicy{ // the documentation for an invalid timeout says the duration will // be undefined. In practice we take the spec from the // contour.heptio.com/request-timeout annotation, which is defined // to choose infinite when its valid cannot be parsed. - ResponseTimeout: -1, + ResponseTimeout: timeout.DisabledSetting(), }, }, "infinite response timeout": { tp: &projcontour.TimeoutPolicy{ Response: "infinite", }, - want: &TimeoutPolicy{ - ResponseTimeout: -1, + want: TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), }, }, "idle timeout": { tp: &projcontour.TimeoutPolicy{ Idle: "900s", }, - want: &TimeoutPolicy{ - IdleTimeout: 900 * time.Second, + want: TimeoutPolicy{ + IdleTimeout: timeout.DurationSetting(900 * time.Second), }, }, } @@ -301,7 +300,7 @@ func TestTimeoutPolicy(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { got := timeoutPolicy(tc.tp) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, got, cmp.AllowUnexported(timeout.Setting{})) }) } } @@ -352,34 +351,3 @@ func TestLoadBalancerPolicy(t *testing.T) { }) } } - -func TestParseTimeout(t *testing.T) { - tests := map[string]struct { - duration string - want time.Duration - }{ - "empty": { - duration: "", - want: 0, - }, - "infinity": { - duration: "infinity", - want: -1, - }, - "10 seconds": { - duration: "10s", - want: 10 * time.Second, - }, - "invalid": { - duration: "10", // 10 what? - want: -1, - }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - got := annotation.ParseTimeout(tc.duration) - assert.Equal(t, tc.want, got) - }) - } -} diff --git a/internal/envoy/route.go b/internal/envoy/route.go index 2372b6d0587..ae2ebb18fbc 100644 --- a/internal/envoy/route.go +++ b/internal/envoy/route.go @@ -17,7 +17,6 @@ import ( "fmt" "regexp" "sort" - "time" v2 "github.com/envoyproxy/go-control-plane/envoy/api/v2" envoy_api_v2_core "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" @@ -27,6 +26,7 @@ import ( "github.com/projectcontour/contour/internal/dag" "github.com/projectcontour/contour/internal/protobuf" "github.com/projectcontour/contour/internal/sorter" + "github.com/projectcontour/contour/internal/timeout" ) // RouteMatch creates a *envoy_api_v2_route.RouteMatch for the supplied *dag.Route. @@ -59,8 +59,8 @@ func RouteMatch(route *dag.Route) *envoy_api_v2_route.RouteMatch { func RouteRoute(r *dag.Route) *envoy_api_v2_route.Route_Route { ra := envoy_api_v2_route.RouteAction{ RetryPolicy: retryPolicy(r), - Timeout: responseTimeout(r), - IdleTimeout: idleTimeout(r), + Timeout: envoyTimeout(r.TimeoutPolicy.ResponseTimeout), + IdleTimeout: envoyTimeout(r.TimeoutPolicy.IdleTimeout), PrefixRewrite: r.PrefixRewrite, HashPolicy: hashPolicy(r), RequestMirrorPolicies: mirrorPolicy(r), @@ -131,37 +131,23 @@ func hostReplaceHeader(hp *dag.HeadersPolicy) string { return hp.HostRewrite } -func responseTimeout(r *dag.Route) *duration.Duration { - if r.TimeoutPolicy == nil { - return nil - } - return timeout(r.TimeoutPolicy.ResponseTimeout) -} - -func idleTimeout(r *dag.Route) *duration.Duration { - if r.TimeoutPolicy == nil { - return nil - } - return timeout(r.TimeoutPolicy.IdleTimeout) -} - -// timeout interprets a time.Duration with respect to -// Envoy's timeout logic. Zero durations are interpreted -// as nil, therefore remaining unset. Negative durations -// are interpreted as infinity, which is represented as -// an explicit value of 0. Positive durations behave as -// expected. -func timeout(d time.Duration) *duration.Duration { +// envoyTimeout converts a timeout.Setting to a protobuf.Duration +// that's appropriate for Envoy. In general (though there are +// exceptions), Envoy uses the following semantics: +// - not passing a value means "use Envoy default" +// - explicitly passing a 0 means "disable this timeout" +// - passing a positive value uses that value +func envoyTimeout(d timeout.Setting) *duration.Duration { switch { - case d == 0: - // no timeout specified + case d.UseDefault(): + // Don't pass a value to Envoy. return nil - case d < 0: - // infinite timeout, set timeout value to a pointer to zero which tells - // envoy "infinite timeout" + case d.IsDisabled(): + // Explicitly pass a 0. return protobuf.Duration(0) default: - return protobuf.Duration(d) + // Pass the duration value. + return protobuf.Duration(d.Duration()) } } @@ -180,9 +166,8 @@ func retryPolicy(r *dag.Route) *envoy_api_v2_route.RetryPolicy { if r.RetryPolicy.NumRetries > 0 { rp.NumRetries = protobuf.UInt32(r.RetryPolicy.NumRetries) } - if r.RetryPolicy.PerTryTimeout > 0 { - rp.PerTryTimeout = protobuf.Duration(r.RetryPolicy.PerTryTimeout) - } + rp.PerTryTimeout = envoyTimeout(r.RetryPolicy.PerTryTimeout) + return rp } diff --git a/internal/envoy/route_test.go b/internal/envoy/route_test.go index 9424bafc2bf..5d3cb37b392 100644 --- a/internal/envoy/route_test.go +++ b/internal/envoy/route_test.go @@ -24,6 +24,7 @@ import ( "github.com/projectcontour/contour/internal/assert" "github.com/projectcontour/contour/internal/dag" "github.com/projectcontour/contour/internal/protobuf" + "github.com/projectcontour/contour/internal/timeout" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -237,8 +238,8 @@ func TestRouteRoute(t *testing.T) { "single service without retry-on": { route: &dag.Route{ RetryPolicy: &dag.RetryPolicy{ - NumRetries: 7, // ignored - PerTryTimeout: 10 * time.Second, // ignored + NumRetries: 7, // ignored + PerTryTimeout: timeout.DurationSetting(10 * time.Second), // ignored }, Clusters: []*dag.Cluster{c1}, }, @@ -255,7 +256,7 @@ func TestRouteRoute(t *testing.T) { RetryPolicy: &dag.RetryPolicy{ RetryOn: "503", NumRetries: 6, - PerTryTimeout: 100 * time.Millisecond, + PerTryTimeout: timeout.DurationSetting(100 * time.Millisecond), }, Clusters: []*dag.Cluster{c1}, }, @@ -278,7 +279,7 @@ func TestRouteRoute(t *testing.T) { RetryOn: "retriable-status-codes", RetriableStatusCodes: []uint32{503, 503, 504}, NumRetries: 6, - PerTryTimeout: 100 * time.Millisecond, + PerTryTimeout: timeout.DurationSetting(100 * time.Millisecond), }, Clusters: []*dag.Cluster{c1}, }, @@ -298,8 +299,8 @@ func TestRouteRoute(t *testing.T) { }, "timeout 90s": { route: &dag.Route{ - TimeoutPolicy: &dag.TimeoutPolicy{ - ResponseTimeout: 90 * time.Second, + TimeoutPolicy: dag.TimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(90 * time.Second), }, Clusters: []*dag.Cluster{c1}, }, @@ -314,8 +315,8 @@ func TestRouteRoute(t *testing.T) { }, "timeout infinity": { route: &dag.Route{ - TimeoutPolicy: &dag.TimeoutPolicy{ - ResponseTimeout: -1, + TimeoutPolicy: dag.TimeoutPolicy{ + ResponseTimeout: timeout.DisabledSetting(), }, Clusters: []*dag.Cluster{c1}, }, @@ -330,8 +331,8 @@ func TestRouteRoute(t *testing.T) { }, "idle timeout 10m": { route: &dag.Route{ - TimeoutPolicy: &dag.TimeoutPolicy{ - IdleTimeout: 10 * time.Minute, + TimeoutPolicy: dag.TimeoutPolicy{ + IdleTimeout: timeout.DurationSetting(10 * time.Minute), }, Clusters: []*dag.Cluster{c1}, }, @@ -346,8 +347,8 @@ func TestRouteRoute(t *testing.T) { }, "idle timeout infinity": { route: &dag.Route{ - TimeoutPolicy: &dag.TimeoutPolicy{ - IdleTimeout: -1, + TimeoutPolicy: dag.TimeoutPolicy{ + IdleTimeout: timeout.DisabledSetting(), }, Clusters: []*dag.Cluster{c1}, }, diff --git a/internal/timeout/timeout.go b/internal/timeout/timeout.go new file mode 100644 index 00000000000..4bb8b693a3d --- /dev/null +++ b/internal/timeout/timeout.go @@ -0,0 +1,85 @@ +// Copyright Project Contour Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package timeout + +import "time" + +// Setting describes a timeout setting that can be exactly one of: +// disable the timeout entirely, use the default, or use a specific +// value. The zero value is a Setting representing "use the default". +type Setting struct { + val time.Duration + disabled bool +} + +// IsDisabled returns whether the timeout should be disabled entirely. +func (s Setting) IsDisabled() bool { + return s.disabled +} + +// UseDefault returns whether the default proxy timeout value should be +// used. +func (s Setting) UseDefault() bool { + return !s.disabled && s.val == 0 +} + +// Duration returns the explicit timeout value if one exists. +func (s Setting) Duration() time.Duration { + return s.val +} + +// DefaultSetting returns a Setting representing "use the default". +func DefaultSetting() Setting { + return Setting{} +} + +// DisabledSetting returns a Setting representing "disable the timeout". +func DisabledSetting() Setting { + return Setting{disabled: true} +} + +// DurationSetting returns a timeout setting with the given duration. +func DurationSetting(duration time.Duration) Setting { + return Setting{val: duration} +} + +// Parse parses string representations of timeout settings that we pass +// in various places in a standard way: +// - an empty string means "use the default". +// - any valid representation of "0" means "use the default". +// - a valid Go duration string is used as the specific timeout value. +// - any other input means "disable the timeout". +func Parse(timeout string) Setting { + // An empty string is interpreted as no explicit timeout specified, so + // use the Envoy default. + if timeout == "" { + return DefaultSetting() + } + + // Interpret "infinity" as a disabled/infinite timeout, which envoy config + // usually expects as an explicit value of 0. + if timeout == "infinity" { + return DisabledSetting() + } + + d, err := time.ParseDuration(timeout) + if err != nil { + // TODO(cmalonty) plumb a logger in here so we can log this error. + // Assuming infinite duration is going to surprise people less for + // a not-parseable duration than a implicit 15 second one. + return DisabledSetting() + } + + return DurationSetting(d) +} diff --git a/internal/timeout/timeout_test.go b/internal/timeout/timeout_test.go new file mode 100644 index 00000000000..c9b5994acb4 --- /dev/null +++ b/internal/timeout/timeout_test.go @@ -0,0 +1,63 @@ +// Copyright Project Contour Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package timeout + +import ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/projectcontour/contour/internal/assert" +) + +func TestParse(t *testing.T) { + tests := map[string]struct { + duration string + want Setting + }{ + "empty": { + duration: "", + want: DefaultSetting(), + }, + "0": { + duration: "0", + want: DefaultSetting(), + }, + "0s": { + duration: "0s", + want: DefaultSetting(), + }, + "infinity": { + duration: "infinity", + want: DisabledSetting(), + }, + "10 seconds": { + duration: "10s", + want: DurationSetting(10 * time.Second), + }, + "invalid": { + duration: "10", // 10 what? + want: DisabledSetting(), + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.want, Parse(tc.duration), cmp.AllowUnexported(Setting{})) + }) + } +} + +func TestDurationSetting(t *testing.T) { + assert.Equal(t, 10*time.Second, DurationSetting(10*time.Second).Duration()) +}