Skip to content

Commit

Permalink
internal/timeout: extract package for handling timeouts (projectconto…
Browse files Browse the repository at this point in the history 
…ur#2698)

Adds the internal/timeout package, which exposes a structured
type for representing timeouts, and updates existing code to use
this new package.

Signed-off-by: Steve Kriss <krisss@vmware.com>
  • Loading branch information
@skriss
skriss authored Jul 22, 2020
1 parent e02ca4c commit 2d36cc8
Show file tree
Hide file tree
Showing 11 changed files with 266 additions and 192 deletions.
6 changes: 3 additions & 3 deletions internal/annotation/annotations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
46 changes: 0 additions & 46 deletions internal/annotation/timeout.go
View file

This file was deleted.

10 changes: 5 additions & 5 deletions internal/assert/assert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
50 changes: 26 additions & 24 deletions internal/dag/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 ¯\_(ツ)_/¯.
},
}),
),
Expand All @@ -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 ¯\_(ツ)_/¯.
},
}),
),
Expand All @@ -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 ¯\_(ツ)_/¯.
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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(),
},
}),
),
Expand All @@ -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(),
},
}),
),
Expand All @@ -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(),
},
}),
),
Expand Down Expand Up @@ -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),
},
}),
),
Expand All @@ -3901,7 +3902,7 @@ func TestDAGInsert(t *testing.T) {
RetryPolicy: &RetryPolicy{
RetryOn: "5xx",
NumRetries: 6,
PerTryTimeout: 0,
PerTryTimeout: timeout.DefaultSetting(),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand All @@ -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),
},
}),
),
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 5 additions & 6 deletions internal/dag/dag.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
33 changes: 25 additions & 8 deletions internal/dag/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Expand Down Expand Up @@ -124,15 +135,18 @@ 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
// request timeout, but it is actually applied as a timeout on
// 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
Expand All @@ -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),
}
}

Expand Down
Loading

0 comments on commit 2d36cc8

Please sign in to comment.