Skip to content

Commit

Permalink
cmd/contour: add TimeoutConfig.RequestTimeout (#2726)
Browse files Browse the repository at this point in the history
Adds a new RequestTimeout field to the TimeoutConfig struct. If
specified in the config file, this field takes precedence over
the now-deprecated RequestTimeout field in the root of the
serveContext struct.

Signed-off-by: Steve Kriss <krisss@vmware.com>
  • Loading branch information
skriss committed Jul 24, 2020
1 parent 3727300 commit e35cca7
Show file tree
Hide file tree
Showing 10 changed files with 119 additions and 57 deletions.
16 changes: 15 additions & 1 deletion cmd/contour/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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()
}
71 changes: 71 additions & 0 deletions cmd/contour/serve_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
15 changes: 13 additions & 2 deletions cmd/contour/servecontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 1 addition & 6 deletions examples/contour/01-contour-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
7 changes: 1 addition & 6 deletions examples/render/contour.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
21 changes: 4 additions & 17 deletions internal/contour/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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).
Expand Down
11 changes: 5 additions & 6 deletions internal/envoy/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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()
}
Expand Down
22 changes: 5 additions & 17 deletions internal/envoy/listener_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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,
}),
Expand All @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}),
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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,
Expand Down Expand Up @@ -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,
}),
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down
Loading

0 comments on commit e35cca7

Please sign in to comment.