Merge pull request projectcontour#23 from phylake/idle-timeout-tweak

cap upstream and downstream idle timeouts to 1h
lrouquette · Mar 3, 2020 · 315de01 · 315de01
2 parents c0503c4 + 8e2a849
commit 315de01
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 15 deletions.
diff --git a/ADOBE_CHANGELOG.md b/ADOBE_CHANGELOG.md
@@ -16,6 +16,11 @@ v{C major}.{C minor}.{C fix}-{A major}.{A minor}.{A fix}-adobe
 
 # Log
 
+## v1.1.0-2.4.0-adobe
+
+- 1 hour limit on upstream and downstream idle timeouts
+- upstream limit moved from Envoy
+
 ## v1.1.0-2.3.0-adobe
 
 - set `merge_slashes=true` on HTTP connection manager

diff --git a/adobe/adobe.go b/adobe/adobe.go
@@ -13,7 +13,7 @@ import (
 
 // ignore properties added/removed by our customization
 var ignoreProperties = []cmp.Option{
-	cmpopts.IgnoreFields(v2.Cluster{}, "CircuitBreakers", "DrainConnectionsOnHostRemoval"),
+	cmpopts.IgnoreFields(v2.Cluster{}, "CommonHttpProtocolOptions", "CircuitBreakers", "DrainConnectionsOnHostRemoval"),
 	cmpopts.IgnoreFields(v2.RouteConfiguration{}, "RequestHeadersToAdd"),
 	cmpopts.IgnoreFields(envoy_api_v2_core.HealthCheck_HttpHealthCheck{}, "ExpectedStatuses"),
 	cmpopts.IgnoreFields(envoy_api_v2_route.RouteAction{}, "RetryPolicy", "Timeout", "IdleTimeout", "HashPolicy"),

diff --git a/apis/contour/v1beta1/ingressroute.go b/apis/contour/v1beta1/ingressroute.go
@@ -64,7 +64,7 @@ type Route struct {
 
 	Timeout *Duration `json:"timeout,omitempty"`
 
-	IdleTimeout *Duration `json:"idleTimeout,omitempty"` // Deprecated: do not use.
+	IdleTimeout *Duration `json:"idleTimeout,omitempty"`
 }
 
 // TimeoutPolicy define the attributes associated with timeout

diff --git a/internal/dag/builder.go b/internal/dag/builder.go
@@ -18,11 +18,13 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"time"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/api/networking/v1beta1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
+	"github.com/golang/protobuf/ptypes"
 	"github.com/google/go-cmp/cmp"
 	ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1"
 	projcontour "github.com/projectcontour/contour/apis/projectcontour/v1"
@@ -979,10 +981,17 @@ func (b *Builder) processIngressRoutes(sw *ObjectStatusWriter, ir *ingressroutev
 				HashPolicy:      route.HashPolicy,
 				PerFilterConfig: route.PerFilterConfig,
 			}
-			// TODO(bcook) deprecate this
+
 			if route.IdleTimeout != nil {
-				r.IdleTimeout = &route.IdleTimeout.Duration
+				if d, err := ptypes.Duration(&route.IdleTimeout.Duration); err == nil {
+					if d > time.Hour {
+						r.IdleTimeout = ptypes.DurationProto(time.Hour)
+					} else {
+						r.IdleTimeout = &route.IdleTimeout.Duration
+					}
+				}
 			}
+
 			if route.Timeout != nil {
 				r.Timeout = &route.Timeout.Duration
 			}
@@ -1018,9 +1027,21 @@ func (b *Builder) processIngressRoutes(sw *ObjectStatusWriter, ir *ingressroutev
 					UpstreamValidation: uv,
 					Protocol:           s.Protocol,
 				}
+
 				if service.IdleTimeout != nil {
-					c.IdleTimeout = &service.IdleTimeout.Duration
+					if d, err := ptypes.Duration(&service.IdleTimeout.Duration); err == nil {
+						if d > time.Hour {
+							c.IdleTimeout = ptypes.DurationProto(time.Hour)
+						} else {
+							c.IdleTimeout = &service.IdleTimeout.Duration
+						}
+					}
+				}
+
+				if c.IdleTimeout == nil {
+					c.IdleTimeout = ptypes.DurationProto(58 * time.Second)
 				}
+
 				r.Clusters = append(r.Clusters, c)
 			}
 

diff --git a/internal/dag/cache.go b/internal/dag/cache.go
@@ -154,16 +154,6 @@ func (kc *KubernetesCache) Insert(obj interface{}) bool {
 		if class == "" || class != kc.ingressClass() {
 			return false
 		}
-		// Adobe - route.IdleTimeout is deprecated - emit warning if found
-		for _, r := range obj.Spec.Routes {
-			if r.IdleTimeout != nil {
-				om := obj.GetObjectMeta()
-				kc.WithField("name", om.GetName()).
-					WithField("namespace", om.GetNamespace()).
-					WithField("kind", k8s.KindOf(obj)).
-					Warning("using deprecated property IdleTimeout on Route")
-			}
-		}
 		m := toMeta(obj)
 		if kc.ingressroutes == nil {
 			kc.ingressroutes = make(map[Meta]*ingressroutev1.IngressRoute)

diff --git a/internal/e2e/adobe_test.go b/internal/e2e/adobe_test.go
@@ -1212,6 +1212,9 @@ func TestAdobeClusterCircuitBreakersDrainConnections(t *testing.T) {
 	c := cluster("default/ws/80/da39a3ee5e", "default/ws", "default_ws_80")
 	c.CircuitBreakers = adobe.CircuitBreakers
 	c.DrainConnectionsOnHostRemoval = true
+	c.CommonHttpProtocolOptions = &envoy_api_v2_core.HttpProtocolOptions{
+		IdleTimeout: protobuf.Duration(58 * time.Second),
+	}
 
 	protos := []proto.Message{c}
 
@@ -1294,16 +1297,25 @@ func TestAdobeClusterLbPolicy(t *testing.T) {
 	cCookie.CircuitBreakers = adobe.CircuitBreakers
 	cCookie.DrainConnectionsOnHostRemoval = true
 	cCookie.LbPolicy = v2.Cluster_ROUND_ROBIN
+	cCookie.CommonHttpProtocolOptions = &envoy_api_v2_core.HttpProtocolOptions{
+		IdleTimeout: protobuf.Duration(58 * time.Second),
+	}
 
 	cRingHash := cluster("default/ws-ringhash/80/40633a6ca9", "default/ws-ringhash", "default_ws-ringhash_80")
 	cRingHash.CircuitBreakers = adobe.CircuitBreakers
 	cRingHash.DrainConnectionsOnHostRemoval = true
 	cRingHash.LbPolicy = v2.Cluster_RING_HASH
+	cRingHash.CommonHttpProtocolOptions = &envoy_api_v2_core.HttpProtocolOptions{
+		IdleTimeout: protobuf.Duration(58 * time.Second),
+	}
 
 	cMagLev := cluster("default/ws-maglev/80/843e4ded8f", "default/ws-maglev", "default_ws-maglev_80")
 	cMagLev.CircuitBreakers = adobe.CircuitBreakers
 	cMagLev.DrainConnectionsOnHostRemoval = true
 	cMagLev.LbPolicy = v2.Cluster_MAGLEV
+	cMagLev.CommonHttpProtocolOptions = &envoy_api_v2_core.HttpProtocolOptions{
+		IdleTimeout: protobuf.Duration(58 * time.Second),
+	}
 
 	protos := []proto.Message{cCookie, cMagLev, cRingHash} //ordered
 
@@ -1374,6 +1386,9 @@ func TestAdobeClusterHealthcheck(t *testing.T) {
 			},
 		},
 	}
+	c.CommonHttpProtocolOptions = &envoy_api_v2_core.HttpProtocolOptions{
+		IdleTimeout: protobuf.Duration(58 * time.Second),
+	}
 
 	protos := []proto.Message{c}