cmd/contour: add TimeoutConfig.RequestTimeout

cmd/contour/serve.go

@@ -185,7 +185,7 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {
 		AccessLogType:                 ctx.AccessLogFormat,
 		AccessLogFields:               ctx.AccessLogFields,
 		MinimumTLSVersion:             annotation.MinTLSVersion(ctx.TLSConfig.MinimumProtocolVersion),
-		RequestTimeout:                ctx.RequestTimeout,
+		RequestTimeout:                getRequestTimeout(log, ctx),
 		ConnectionIdleTimeout:         timeout.Parse(ctx.ConnectionIdleTimeout),
 		StreamIdleTimeout:             timeout.Parse(ctx.StreamIdleTimeout),
 		MaxConnectionDuration:         timeout.Parse(ctx.MaxConnectionDuration),
@@ -467,3 +467,17 @@ func startInformer(inf k8s.InformerFactory, log logrus.FieldLogger) func(stop <-
 		return nil
 	}
 }
+
+// getRequestTimeout gets the request timeout setting from ctx.TimeoutConfig.RequestTimeout
+// if it's set, or else ctx.RequestTimeoutDeprecated if it's set, or else a default setting.
+func getRequestTimeout(log logrus.FieldLogger, ctx *serveContext) timeout.Setting {
+	if ctx.RequestTimeout != "" {
+		return timeout.Parse(ctx.RequestTimeout)
+	}
+	if ctx.RequestTimeoutDeprecated > 0 {
+		log.Warn("The request-timeout field in the Contour config file is deprecated and will be removed in a future release. Use timeout-config.request-timeout instead.")
+		return timeout.DurationSetting(ctx.RequestTimeoutDeprecated)
+	}
+
+	return timeout.DefaultSetting()
+}

cmd/contour/serve_test.go

@@ -0,0 +1,71 @@
+// 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 main
+
+import (
+	"io/ioutil"
+	"testing"
+	"time"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/projectcontour/contour/internal/assert"
+	"github.com/projectcontour/contour/internal/timeout"
+	"github.com/sirupsen/logrus"
+)
+
+func TestGetRequestTimeout(t *testing.T) {
+	tests := []struct {
+		name string
+		ctx  *serveContext
+		want timeout.Setting
+	}{
+		{
+			name: "neither field set",
+			ctx:  &serveContext{},
+			want: timeout.DefaultSetting(),
+		},
+		{
+			name: "only deprecated field set",
+			ctx:  &serveContext{RequestTimeoutDeprecated: 7 * time.Second},
+			want: timeout.DurationSetting(7 * time.Second),
+		},
+		{
+			name: "only new field set",
+			ctx: &serveContext{
+				TimeoutConfig: TimeoutConfig{
+					RequestTimeout: "70s",
+				},
+			},
+			want: timeout.DurationSetting(70 * time.Second),
+		},
+		{
+			name: "both fields set, new field takes precedence",
+			ctx: &serveContext{
+				TimeoutConfig: TimeoutConfig{
+					RequestTimeout: "70s",
+				},
+				RequestTimeoutDeprecated: 7 * time.Second,
+			},
+			want: timeout.DurationSetting(70 * time.Second),
+		},
+	}
+
+	for _, tc := range tests {
+		log := logrus.New()
+		log.Out = ioutil.Discard
+		opt := cmp.AllowUnexported(timeout.Setting{})
+
+		assert.Equal(t, tc.want, getRequestTimeout(log, tc.ctx), opt)
+	}
+}

cmd/contour/servecontext.go

@@ -118,8 +118,11 @@ type serveContext struct {
 	// be set in the config file.
 	TimeoutConfig `yaml:"timeouts,omitempty"`
 
-	// RequestTimeout sets the client request timeout globally for Contour.
-	RequestTimeout time.Duration `yaml:"request-timeout,omitempty"`
+	// RequestTimeoutDeprecated sets the client request timeout globally for Contour.
+	//
+	// Deprecated: this field has been replaced with TimeoutConfig.RequestTimeout,
+	// and will be removed in a future release.
+	RequestTimeoutDeprecated time.Duration `yaml:"request-timeout,omitempty"`
 
 	// Should Contour register to watch the new service-apis types?
 	// By default this value is false, meaning Contour will not do anything with any of the new
@@ -237,6 +240,14 @@ type LeaderElectionConfig struct {
 
 // TimeoutConfig holds various configurable proxy timeout values.
 type TimeoutConfig struct {
+	// RequestTimeout sets the client request timeout globally for Contour. Note that
+	// this is a timeout for the entire request, not an idle timeout. Omit or set to
+	// "infinity" to disable the timeout entirely.
+	//
+	// See https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#envoy-api-field-config-filter-network-http-connection-manager-v2-httpconnectionmanager-request-timeout
+	// for more information.
+	RequestTimeout string `yaml:"request-timeout,omitempty"`
+
 	// ConnectionIdleTimeout defines how long the proxy should wait while there are
 	// no active requests (for HTTP/1.1) or streams (for HTTP/2) before terminating
 	// an HTTP connection. Set to "infinity" to disable the timeout entirely.

examples/contour/01-contour-config.yaml

@@ -12,12 +12,6 @@ data:
     # path to kubeconfig (if not running inside a k8s cluster)
     # kubeconfig: /path/to/.kube/config
     #
-    # Client request timeout to be passed to Envoy
-    # as the connection manager request_timeout.
-    # Defaults to 0, which Envoy interprets as disabled.
-    # Note that this is the timeout for the whole request,
-    # not an idle timeout.
-    # request-timeout: 0s
     # disable HTTPProxy permitInsecure field
     disablePermitInsecure: false
     tls:
@@ -71,6 +65,7 @@ data:
     #
     # The following shows the default proxy timeout settings.
     # timeouts:
+    #   request-timeout: infinity
     #   connection-idle-timeout: 60s
     #   stream-idle-timeout: 5m
     #   max-connection-duration: infinity

examples/render/contour.yaml

@@ -46,12 +46,6 @@ data:
     # path to kubeconfig (if not running inside a k8s cluster)
     # kubeconfig: /path/to/.kube/config
     #
-    # Client request timeout to be passed to Envoy
-    # as the connection manager request_timeout.
-    # Defaults to 0, which Envoy interprets as disabled.
-    # Note that this is the timeout for the whole request,
-    # not an idle timeout.
-    # request-timeout: 0s
     # disable HTTPProxy permitInsecure field
     disablePermitInsecure: false
     tls:
@@ -105,6 +99,7 @@ data:
     #
     # The following shows the default proxy timeout settings.
     # timeouts:
+    #   request-timeout: infinity
     #   connection-idle-timeout: 60s
     #   stream-idle-timeout: 5m
     #   max-connection-duration: infinity

internal/contour/listener.go

@@ -17,7 +17,6 @@ import (
 	"path"
 	"sort"
 	"sync"
-	"time"
 
 	v2 "github.com/envoyproxy/go-control-plane/envoy/api/v2"
 	envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth"
@@ -97,7 +96,7 @@ type ListenerVisitorConfig struct {
 	AccessLogFields []string
 
 	// RequestTimeout configures the request_timeout for all Connection Managers.
-	RequestTimeout time.Duration
+	RequestTimeout timeout.Setting
 
 	// ConnectionIdleTimeout configures the common_http_protocol_options.idle_timeout for all
 	// Connection Managers.
@@ -204,18 +203,6 @@ func (lvc *ListenerVisitorConfig) newSecureAccessLog() []*envoy_api_v2_accesslog
 	}
 }
 
-// requestTimeout sets any durations in lvc.RequestTimeout <0 to 0 so that Envoy ends up with a positive duration.
-// for the request_timeout value we are passing, there are only two valid values:
-// 0 - disabled
-// >0 duration - the timeout.
-// The value may be unset, but we always set it to 0.
-func (lvc *ListenerVisitorConfig) requestTimeout() time.Duration {
-	if lvc.RequestTimeout < 0 {
-		return 0
-	}
-	return lvc.RequestTimeout
-}
-
 // minTLSVersion returns the requested minimum TLS protocol
 // version or envoy_api_v2_auth.TlsParameters_TLSv1_1 if not configured.
 func (lvc *ListenerVisitorConfig) minTLSVersion() envoy_api_v2_auth.TlsParameters_TlsProtocol {
@@ -324,7 +311,7 @@ func visitListeners(root dag.Vertex, lvc *ListenerVisitorConfig) map[string]*v2.
 			RouteConfigName(ENVOY_HTTP_LISTENER).
 			MetricsPrefix(ENVOY_HTTP_LISTENER).
 			AccessLoggers(lvc.newInsecureAccessLog()).
-			RequestTimeout(lvc.requestTimeout()).
+			RequestTimeout(lvc.RequestTimeout).
 			ConnectionIdleTimeout(lvc.ConnectionIdleTimeout).
 			StreamIdleTimeout(lvc.StreamIdleTimeout).
 			MaxConnectionDuration(lvc.MaxConnectionDuration).
@@ -399,7 +386,7 @@ func (v *listenerVisitor) visit(vertex dag.Vertex) {
 					RouteConfigName(path.Join("https", vh.VirtualHost.Name)).
 					MetricsPrefix(ENVOY_HTTPS_LISTENER).
 					AccessLoggers(v.ListenerVisitorConfig.newSecureAccessLog()).
-					RequestTimeout(v.ListenerVisitorConfig.requestTimeout()).
+					RequestTimeout(v.ListenerVisitorConfig.RequestTimeout).
 					ConnectionIdleTimeout(v.ListenerVisitorConfig.ConnectionIdleTimeout).
 					StreamIdleTimeout(v.ListenerVisitorConfig.StreamIdleTimeout).
 					MaxConnectionDuration(v.ListenerVisitorConfig.MaxConnectionDuration).
@@ -457,7 +444,7 @@ func (v *listenerVisitor) visit(vertex dag.Vertex) {
 					RouteConfigName(ENVOY_FALLBACK_ROUTECONFIG).
 					MetricsPrefix(ENVOY_HTTPS_LISTENER).
 					AccessLoggers(v.ListenerVisitorConfig.newSecureAccessLog()).
-					RequestTimeout(v.ListenerVisitorConfig.requestTimeout()).
+					RequestTimeout(v.ListenerVisitorConfig.RequestTimeout).
 					ConnectionIdleTimeout(v.ListenerVisitorConfig.ConnectionIdleTimeout).
 					StreamIdleTimeout(v.ListenerVisitorConfig.StreamIdleTimeout).
 					MaxConnectionDuration(v.ListenerVisitorConfig.MaxConnectionDuration).

internal/envoy/listener.go

@@ -130,7 +130,7 @@ type httpConnectionManagerBuilder struct {
 	routeConfigName               string
 	metricsPrefix                 string
 	accessLoggers                 []*accesslog.AccessLog
-	requestTimeout                time.Duration
+	requestTimeout                timeout.Setting
 	connectionIdleTimeout         timeout.Setting
 	streamIdleTimeout             timeout.Setting
 	maxConnectionDuration         timeout.Setting
@@ -168,9 +168,8 @@ func (b *httpConnectionManagerBuilder) AccessLoggers(loggers []*accesslog.Access
 	return b
 }
 
-// RequestTimeout sets the active request timeout on the connection
-// manager. If not specified or set to 0, this timeout is disabled.
-func (b *httpConnectionManagerBuilder) RequestTimeout(timeout time.Duration) *httpConnectionManagerBuilder {
+// RequestTimeout sets the active request timeout on the connection manager.
+func (b *httpConnectionManagerBuilder) RequestTimeout(timeout timeout.Setting) *httpConnectionManagerBuilder {
 	b.requestTimeout = timeout
 	return b
 }
@@ -244,12 +243,12 @@ func (b *httpConnectionManagerBuilder) Get() *envoy_api_v2_listener.Filter {
 		},
 		UseRemoteAddress: protobuf.Bool(true),
 		NormalizePath:    protobuf.Bool(true),
-		RequestTimeout:   protobuf.Duration(b.requestTimeout),
 
 		// issue #1487 pass through X-Request-Id if provided.
 		PreserveExternalRequestId: true,
 		MergeSlashes:              true,
 
+		RequestTimeout:    envoyTimeout(b.requestTimeout),
 		StreamIdleTimeout: envoyTimeout(b.streamIdleTimeout),
 		DrainTimeout:      envoyTimeout(b.connectionShutdownGracePeriod),
 	}
@@ -289,7 +288,7 @@ func HTTPConnectionManager(routename string, accesslogger []*accesslog.AccessLog
 		RouteConfigName(routename).
 		MetricsPrefix(routename).
 		AccessLoggers(accesslogger).
-		RequestTimeout(requestTimeout).
+		RequestTimeout(timeout.DurationSetting(requestTimeout)).
 		DefaultFilters().
 		Get()
 }

internal/envoy/listener_test.go

@@ -305,17 +305,16 @@ func TestHTTPConnectionManager(t *testing.T) {
 	tests := map[string]struct {
 		routename                     string
 		accesslogger                  []*envoy_api_v2_accesslog.AccessLog
-		requestTimeout                time.Duration
+		requestTimeout                timeout.Setting
 		connectionIdleTimeout         timeout.Setting
 		streamIdleTimeout             timeout.Setting
 		maxConnectionDuration         timeout.Setting
 		connectionShutdownGracePeriod timeout.Setting
 		want                          *envoy_api_v2_listener.Filter
 	}{
 		"default": {
-			routename:      "default/kuard",
-			accesslogger:   FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout: 0,
+			routename:    "default/kuard",
+			accesslogger: FileAccessLogEnvoy("/dev/stdout"),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
 				ConfigType: &envoy_api_v2_listener.Filter_TypedConfig{
@@ -356,7 +355,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 					}),
@@ -366,7 +364,7 @@ func TestHTTPConnectionManager(t *testing.T) {
 		"request timeout of 10s": {
 			routename:      "default/kuard",
 			accesslogger:   FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout: 10 * time.Second,
+			requestTimeout: timeout.DurationSetting(10 * time.Second),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
 				ConfigType: &envoy_api_v2_listener.Filter_TypedConfig{
@@ -417,7 +415,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 		"connection idle timeout of 90s": {
 			routename:             "default/kuard",
 			accesslogger:          FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout:        0,
 			connectionIdleTimeout: timeout.DurationSetting(90 * time.Second),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
@@ -461,7 +458,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 					}),
@@ -471,7 +467,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 		"stream idle timeout of 90s": {
 			routename:         "default/kuard",
 			accesslogger:      FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout:    0,
 			streamIdleTimeout: timeout.DurationSetting(90 * time.Second),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
@@ -513,7 +508,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 						StreamIdleTimeout:         protobuf.Duration(90 * time.Second),
@@ -524,7 +518,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 		"max connection duration of 90s": {
 			routename:             "default/kuard",
 			accesslogger:          FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout:        0,
 			maxConnectionDuration: timeout.DurationSetting(90 * time.Second),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
@@ -568,7 +561,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 					}),
@@ -578,7 +570,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 		"when max connection duration is disabled, it's omitted": {
 			routename:             "default/kuard",
 			accesslogger:          FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout:        0,
 			maxConnectionDuration: timeout.DisabledSetting(),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
@@ -620,17 +611,15 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 					}),
 				},
 			},
 		},
-		"connection shutdown grace period of 90s": {
+		"drain timeout of 90s": {
 			routename:                     "default/kuard",
 			accesslogger:                  FileAccessLogEnvoy("/dev/stdout"),
-			requestTimeout:                0,
 			connectionShutdownGracePeriod: timeout.DurationSetting(90 * time.Second),
 			want: &envoy_api_v2_listener.Filter{
 				Name: wellknown.HTTPConnectionManager,
@@ -672,7 +661,6 @@ func TestHTTPConnectionManager(t *testing.T) {
 						AccessLog:                 FileAccessLogEnvoy("/dev/stdout"),
 						UseRemoteAddress:          protobuf.Bool(true),
 						NormalizePath:             protobuf.Bool(true),
-						RequestTimeout:            protobuf.Duration(0),
 						PreserveExternalRequestId: true,
 						MergeSlashes:              true,
 						DrainTimeout:              protobuf.Duration(90 * time.Second),