Fix negated allow chunked length field

changelogs/unreleased/4152-sunjayBhatia-small.md

@@ -0,0 +1 @@
+Fix accidental negation of disableAllowChunkedLength configuration flag.

cmd/contour/serve.go

@@ -31,6 +31,7 @@ import (
 	contour_api_v1alpha1 "github.com/projectcontour/contour/apis/projectcontour/v1alpha1"
 	"github.com/projectcontour/contour/internal/annotation"
 	"github.com/projectcontour/contour/internal/contour"
+	"github.com/projectcontour/contour/internal/contourconfig"
 	"github.com/projectcontour/contour/internal/controller"
 	"github.com/projectcontour/contour/internal/dag"
 	"github.com/projectcontour/contour/internal/debug"
@@ -280,25 +281,48 @@ func (s *Server) doServe() error {
 		cipherSuites = append(cipherSuites, string(cs))
 	}
 
-	listenerConfig := xdscache_v3.NewListenerConfig(
-		contourConfiguration.Envoy.Listener.UseProxyProto,
-		contourConfiguration.Envoy.HTTPListener,
-		contourConfiguration.Envoy.HTTPSListener,
-		contourConfiguration.Envoy.Logging.AccessLogFormat,
-		contourConfiguration.Envoy.Logging.AccessLogFields,
-		contourConfiguration.Envoy.Logging.AccessLogFormatString,
-		AccessLogFormatterExtensions(contourConfiguration.Envoy.Logging.AccessLogFormat, contourConfiguration.Envoy.Logging.AccessLogFields, contourConfiguration.Envoy.Logging.AccessLogFormatString),
-		annotation.MinTLSVersion(contourConfiguration.Envoy.Listener.TLS.MinimumProtocolVersion, "1.2"),
-		config.SanitizeCipherSuites(cipherSuites),
-		contourConfiguration.Envoy.Timeouts,
-		parseDefaultHTTPVersions(contourConfiguration.Envoy.DefaultHTTPVersions),
-		!contourConfiguration.Envoy.Listener.DisableAllowChunkedLength,
-		contourConfiguration.Envoy.Network.XffNumTrustedHops,
-		contourConfiguration.Envoy.Listener.ConnectionBalancer,
-		s.log,
-	)
+	timeouts, err := contourconfig.ParseTimeoutPolicy(contourConfiguration.Envoy.Timeouts)
+	if err != nil {
+		return err
+	}
 
-	if err := s.setupRateLimitService(contourConfiguration, &listenerConfig); err != nil {
+	accessLogFormatString := ""
+	if contourConfiguration.Envoy.Logging.AccessLogFormatString != nil {
+		accessLogFormatString = *contourConfiguration.Envoy.Logging.AccessLogFormatString
+	}
+
+	listenerConfig := xdscache_v3.ListenerConfig{
+		UseProxyProto: contourConfiguration.Envoy.Listener.UseProxyProto,
+		HTTPListeners: map[string]xdscache_v3.Listener{
+			xdscache_v3.ENVOY_HTTP_LISTENER: {
+				Name:    xdscache_v3.ENVOY_HTTP_LISTENER,
+				Address: contourConfiguration.Envoy.HTTPListener.Address,
+				Port:    contourConfiguration.Envoy.HTTPListener.Port,
+			},
+		},
+		HTTPAccessLog: contourConfiguration.Envoy.HTTPListener.AccessLog,
+		HTTPSListeners: map[string]xdscache_v3.Listener{
+			xdscache_v3.ENVOY_HTTPS_LISTENER: {
+				Name:    xdscache_v3.ENVOY_HTTPS_LISTENER,
+				Address: contourConfiguration.Envoy.HTTPSListener.Address,
+				Port:    contourConfiguration.Envoy.HTTPSListener.Port,
+			},
+		},
+		HTTPSAccessLog:               contourConfiguration.Envoy.HTTPSListener.AccessLog,
+		AccessLogType:                contourConfiguration.Envoy.Logging.AccessLogFormat,
+		AccessLogFields:              contourConfiguration.Envoy.Logging.AccessLogFields,
+		AccessLogFormatString:        accessLogFormatString,
+		AccessLogFormatterExtensions: AccessLogFormatterExtensions(contourConfiguration.Envoy.Logging.AccessLogFormat, contourConfiguration.Envoy.Logging.AccessLogFields, contourConfiguration.Envoy.Logging.AccessLogFormatString),
+		MinimumTLSVersion:            annotation.MinTLSVersion(contourConfiguration.Envoy.Listener.TLS.MinimumProtocolVersion, "1.2"),
+		CipherSuites:                 config.SanitizeCipherSuites(cipherSuites),
+		Timeouts:                     timeouts,
+		DefaultHTTPVersions:          parseDefaultHTTPVersions(contourConfiguration.Envoy.DefaultHTTPVersions),
+		AllowChunkedLength:           !contourConfiguration.Envoy.Listener.DisableAllowChunkedLength,
+		XffNumTrustedHops:            contourConfiguration.Envoy.Network.XffNumTrustedHops,
+		ConnectionBalancer:           contourConfiguration.Envoy.Listener.ConnectionBalancer,
+	}
+
+	if listenerConfig.RateLimitConfig, err = s.setupRateLimitService(contourConfiguration); err != nil {
 		return err
 	}
 
@@ -506,9 +530,9 @@ func (s *Server) doServe() error {
 	return s.group.Run(context.Background())
 }
 
-func (s *Server) setupRateLimitService(contourConfiguration contour_api_v1alpha1.ContourConfigurationSpec, listenerConfig *xdscache_v3.ListenerConfig) error {
+func (s *Server) setupRateLimitService(contourConfiguration contour_api_v1alpha1.ContourConfigurationSpec) (*xdscache_v3.RateLimitConfig, error) {
 	if contourConfiguration.RateLimitService == nil {
-		return nil
+		return nil, nil
 	}
 
 	// ensure the specified ExtensionService exists
@@ -521,7 +545,7 @@ func (s *Server) setupRateLimitService(contourConfiguration contour_api_v1alpha1
 	// Using GetAPIReader() here because the manager's caches won't be started yet,
 	// so reads from the manager's client (which uses the caches for reads) will fail.
 	if err := s.mgr.GetAPIReader().Get(context.Background(), key, extensionSvc); err != nil {
-		return fmt.Errorf("error getting rate limit extension service %s: %v", key, err)
+		return nil, fmt.Errorf("error getting rate limit extension service %s: %v", key, err)
 	}
 
 	// get the response timeout from the ExtensionService
@@ -531,19 +555,17 @@ func (s *Server) setupRateLimitService(contourConfiguration contour_api_v1alpha1
 	if tp := extensionSvc.Spec.TimeoutPolicy; tp != nil {
 		responseTimeout, err = timeout.Parse(tp.Response)
 		if err != nil {
-			return fmt.Errorf("error parsing rate limit extension service %s response timeout: %v", key, err)
+			return nil, fmt.Errorf("error parsing rate limit extension service %s response timeout: %v", key, err)
 		}
 	}
 
-	listenerConfig.RateLimitConfig = &xdscache_v3.RateLimitConfig{
+	return &xdscache_v3.RateLimitConfig{
 		ExtensionService:        key,
 		Domain:                  contourConfiguration.RateLimitService.Domain,
 		Timeout:                 responseTimeout,
 		FailOpen:                contourConfiguration.RateLimitService.FailOpen,
 		EnableXRateLimitHeaders: contourConfiguration.RateLimitService.EnableXRateLimitHeaders,
-	}
-
-	return nil
+	}, nil
 }
 
 func (s *Server) setupDebugService(debugConfig contour_api_v1alpha1.DebugConfig, contourHandler *contour.EventHandler) {

internal/contourconfig/contourconfiguration.go

@@ -0,0 +1,77 @@
+// 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 contourconfig
+
+import (
+	"fmt"
+
+	contour_api_v1alpha1 "github.com/projectcontour/contour/apis/projectcontour/v1alpha1"
+	"github.com/projectcontour/contour/internal/timeout"
+)
+
+type Timeouts struct {
+	Request                       timeout.Setting
+	ConnectionIdle                timeout.Setting
+	StreamIdle                    timeout.Setting
+	MaxConnectionDuration         timeout.Setting
+	DelayedClose                  timeout.Setting
+	ConnectionShutdownGracePeriod timeout.Setting
+}
+
+func ParseTimeoutPolicy(timeoutParameters *contour_api_v1alpha1.TimeoutParameters) (Timeouts, error) {
+	var (
+		err      error
+		timeouts Timeouts
+	)
+
+	if timeoutParameters != nil {
+		if timeoutParameters.RequestTimeout != nil {
+			timeouts.Request, err = timeout.Parse(*timeoutParameters.RequestTimeout)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse request timeout: %s", err)
+			}
+		}
+		if timeoutParameters.ConnectionIdleTimeout != nil {
+			timeouts.ConnectionIdle, err = timeout.Parse(*timeoutParameters.ConnectionIdleTimeout)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse connection idle timeout: %s", err)
+			}
+		}
+		if timeoutParameters.StreamIdleTimeout != nil {
+			timeouts.StreamIdle, err = timeout.Parse(*timeoutParameters.StreamIdleTimeout)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse stream idle timeout: %s", err)
+			}
+		}
+		if timeoutParameters.MaxConnectionDuration != nil {
+			timeouts.MaxConnectionDuration, err = timeout.Parse(*timeoutParameters.MaxConnectionDuration)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse max connection duration: %s", err)
+			}
+		}
+		if timeoutParameters.DelayedCloseTimeout != nil {
+			timeouts.DelayedClose, err = timeout.Parse(*timeoutParameters.DelayedCloseTimeout)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse delayed close timeout: %s", err)
+			}
+		}
+		if timeoutParameters.ConnectionShutdownGracePeriod != nil {
+			timeouts.ConnectionShutdownGracePeriod, err = timeout.Parse(*timeoutParameters.ConnectionShutdownGracePeriod)
+			if err != nil {
+				return Timeouts{}, fmt.Errorf("failed to parse connection shutdown grace period: %s", err)
+			}
+		}
+	}
+	return timeouts, nil
+}

internal/contourconfig/contourconfiguration_test.go

@@ -0,0 +1,123 @@
+// 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 contourconfig_test
+
+import (
+	"testing"
+	"time"
+
+	contour_api_v1alpha1 "github.com/projectcontour/contour/apis/projectcontour/v1alpha1"
+	"github.com/projectcontour/contour/internal/contourconfig"
+	"github.com/projectcontour/contour/internal/timeout"
+	"github.com/stretchr/testify/require"
+	"k8s.io/utils/pointer"
+)
+
+func TestParseTimeoutPolicy(t *testing.T) {
+	testCases := map[string]struct {
+		config   *contour_api_v1alpha1.TimeoutParameters
+		expected contourconfig.Timeouts
+		errorMsg string
+	}{
+		"nil timeout parameters": {
+			config: nil,
+			expected: contourconfig.Timeouts{
+				Request:                       timeout.DefaultSetting(),
+				ConnectionIdle:                timeout.DefaultSetting(),
+				StreamIdle:                    timeout.DefaultSetting(),
+				MaxConnectionDuration:         timeout.DefaultSetting(),
+				DelayedClose:                  timeout.DefaultSetting(),
+				ConnectionShutdownGracePeriod: timeout.DefaultSetting(),
+			},
+		},
+		"timeouts not set": {
+			config: &contour_api_v1alpha1.TimeoutParameters{},
+			expected: contourconfig.Timeouts{
+				Request:                       timeout.DefaultSetting(),
+				ConnectionIdle:                timeout.DefaultSetting(),
+				StreamIdle:                    timeout.DefaultSetting(),
+				MaxConnectionDuration:         timeout.DefaultSetting(),
+				DelayedClose:                  timeout.DefaultSetting(),
+				ConnectionShutdownGracePeriod: timeout.DefaultSetting(),
+			},
+		},
+		"timeouts all set": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				RequestTimeout:                pointer.String("1s"),
+				ConnectionIdleTimeout:         pointer.String("2s"),
+				StreamIdleTimeout:             pointer.String("3s"),
+				MaxConnectionDuration:         pointer.String("infinity"),
+				DelayedCloseTimeout:           pointer.String("5s"),
+				ConnectionShutdownGracePeriod: pointer.String("6s"),
+			},
+			expected: contourconfig.Timeouts{
+				Request:                       timeout.DurationSetting(time.Second),
+				ConnectionIdle:                timeout.DurationSetting(time.Second * 2),
+				StreamIdle:                    timeout.DurationSetting(time.Second * 3),
+				MaxConnectionDuration:         timeout.DisabledSetting(),
+				DelayedClose:                  timeout.DurationSetting(time.Second * 5),
+				ConnectionShutdownGracePeriod: timeout.DurationSetting(time.Second * 6),
+			},
+		},
+		"request timeout invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				RequestTimeout: pointer.String("xxx"),
+			},
+			errorMsg: "failed to parse request timeout",
+		},
+		"connection idle timeout invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				ConnectionIdleTimeout: pointer.String("a"),
+			},
+			errorMsg: "failed to parse connection idle timeout",
+		},
+		"stream idle timeout invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				StreamIdleTimeout: pointer.String("invalid"),
+			},
+			errorMsg: "failed to parse stream idle timeout",
+		},
+		"max connection duration invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				MaxConnectionDuration: pointer.String("xxx"),
+			},
+			errorMsg: "failed to parse max connection duration",
+		},
+		"delayed close timeout invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				DelayedCloseTimeout: pointer.String("xxx"),
+			},
+			errorMsg: "failed to parse delayed close timeout",
+		},
+		"connection shutdown grace period invalid": {
+			config: &contour_api_v1alpha1.TimeoutParameters{
+				ConnectionShutdownGracePeriod: pointer.String("xxx"),
+			},
+			errorMsg: "failed to parse connection shutdown grace period",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			parsed, err := contourconfig.ParseTimeoutPolicy(tc.config)
+			if len(tc.errorMsg) > 0 {
+				require.Error(t, err, "expected error to be returned")
+				require.Contains(t, err.Error(), tc.errorMsg)
+			} else {
+				require.Nil(t, err)
+				require.Equal(t, tc.expected, parsed)
+			}
+		})
+	}
+}

internal/featuretests/v3/timeouts_test.go

@@ -19,6 +19,7 @@ import (
 
 	envoy_discovery_v3 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
 	contour_api_v1 "github.com/projectcontour/contour/apis/projectcontour/v1"
+	"github.com/projectcontour/contour/internal/contourconfig"
 	envoy_v3 "github.com/projectcontour/contour/internal/envoy/v3"
 	"github.com/projectcontour/contour/internal/fixture"
 	"github.com/projectcontour/contour/internal/timeout"
@@ -76,10 +77,12 @@ func TestTimeoutsNotSpecified(t *testing.T) {
 
 func TestNonZeroTimeoutsSpecified(t *testing.T) {
 	withTimeouts := func(conf *xdscache_v3.ListenerConfig) {
-		conf.ConnectionIdleTimeout = timeout.DurationSetting(7 * time.Second)
-		conf.StreamIdleTimeout = timeout.DurationSetting(70 * time.Second)
-		conf.MaxConnectionDuration = timeout.DurationSetting(700 * time.Second)
-		conf.ConnectionShutdownGracePeriod = timeout.DurationSetting(7000 * time.Second)
+		conf.Timeouts = contourconfig.Timeouts{
+			ConnectionIdle:                timeout.DurationSetting(7 * time.Second),
+			StreamIdle:                    timeout.DurationSetting(70 * time.Second),
+			MaxConnectionDuration:         timeout.DurationSetting(700 * time.Second),
+			ConnectionShutdownGracePeriod: timeout.DurationSetting(7000 * time.Second),
+		}
 	}
 
 	rh, c, done := setup(t, withTimeouts)