From 5ee6aeadc06463b3461aff62583689c221f8a913 Mon Sep 17 00:00:00 2001
From: James Peach <jpeach@vmware.com>
Date: Mon, 27 Jul 2020 14:07:55 +1000
Subject: [PATCH] internal/contour: ensure the http.Router filter is present

Ensure that the `http.Router` filter is present whenever we build a
HTTP Connection Manager. The `Router` filter is responsible for actually
forwarding HTTP requests, so omitting it leads to configurations that
are syntactically valid but otherwise inoperable.

This fixes #2733.

Signed-off-by: James Peach <jpeach@vmware.com>
---
 _integration/testsuite/run-test-case.sh |  2 +-
 internal/contour/listener.go            |  1 +
 internal/contour/listener_test.go       |  1 +
 internal/envoy/listener.go              | 23 +++++++++++++++++++++++
 internal/envoy/listener_test.go         | 16 ++++++++++++++++
 internal/featuretests/envoy.go          |  1 +
 6 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/_integration/testsuite/run-test-case.sh b/_integration/testsuite/run-test-case.sh
index a3cdb7842b9..d4661bddd98 100755
--- a/_integration/testsuite/run-test-case.sh
+++ b/_integration/testsuite/run-test-case.sh
@@ -33,5 +33,5 @@ exec "$TESTER" run \
     --param proxy.address="$ADDRESS" \
     --param proxy.http_port="$HTTP_PORT" \
     --param proxy.https_port="$HTTPS_PORT" \
-    --watch pods,secrets \
+    --watch pods,secrets,configmaps \
     "$@"
diff --git a/internal/contour/listener.go b/internal/contour/listener.go
index 4364ec216b8..392bde3b039 100644
--- a/internal/contour/listener.go
+++ b/internal/contour/listener.go
@@ -454,6 +454,7 @@ func (v *listenerVisitor) visit(vertex dag.Vertex) {
 			// Default filter chain
 			filters = envoy.Filters(
 				envoy.HTTPConnectionManagerBuilder().
+					DefaultFilters().
 					RouteConfigName(ENVOY_FALLBACK_ROUTECONFIG).
 					MetricsPrefix(ENVOY_HTTPS_LISTENER).
 					AccessLoggers(v.ListenerVisitorConfig.newSecureAccessLog()).
diff --git a/internal/contour/listener_test.go b/internal/contour/listener_test.go
index 38ffeb4d089..b38c9626c70 100644
--- a/internal/contour/listener_test.go
+++ b/internal/contour/listener_test.go
@@ -145,6 +145,7 @@ func TestListenerVisit(t *testing.T) {
 	}
 
 	fallbackCertFilter := envoy.HTTPConnectionManagerBuilder().
+		DefaultFilters().
 		MetricsPrefix(ENVOY_HTTPS_LISTENER).
 		RouteConfigName(ENVOY_FALLBACK_ROUTECONFIG).
 		AccessLoggers(envoy.FileAccessLogEnvoy(DEFAULT_HTTP_ACCESS_LOG)).
diff --git a/internal/envoy/listener.go b/internal/envoy/listener.go
index c78f8254eb0..9007a5f0625 100644
--- a/internal/envoy/listener.go
+++ b/internal/envoy/listener.go
@@ -220,11 +220,34 @@ func (b *httpConnectionManagerBuilder) AddFilter(f *http.HttpFilter) *httpConnec
 	return b
 }
 
+// Validate runs builtin validation rules against the current builder state.
+func (b *httpConnectionManagerBuilder) Validate() error {
+	filterNames := map[string]struct{}{}
+
+	for _, f := range b.filters {
+		filterNames[f.Name] = struct{}{}
+	}
+
+	// If there's no router filter, requests won't be forwarded.
+	if _, ok := filterNames[wellknown.Router]; !ok {
+		return fmt.Errorf("missing required filter %q", wellknown.Router)
+	}
+
+	return nil
+}
+
 // Get returns a new http.HttpConnectionManager filter, constructed
 // from the builder settings.
 //
 // See https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/filter/network/http_connection_manager/v2/http_connection_manager.proto.html
 func (b *httpConnectionManagerBuilder) Get() *envoy_api_v2_listener.Filter {
+	// For now, failing validation is a programmer error that
+	// the caller can't reasonably recover from. A caller that can
+	// handle this should validate manually.
+	if err := b.Validate(); err != nil {
+		panic(err.Error())
+	}
+
 	cm := &http.HttpConnectionManager{
 		CodecType: b.codec,
 		RouteSpecifier: &http.HttpConnectionManager_Rds{
diff --git a/internal/envoy/listener_test.go b/internal/envoy/listener_test.go
index 23cc8017e3c..a8e0522e008 100644
--- a/internal/envoy/listener_test.go
+++ b/internal/envoy/listener_test.go
@@ -803,3 +803,19 @@ func TestProtoNamesForVersions(t *testing.T) {
 	assert.Equal(t, ProtoNamesForVersions(HTTPVersion3), []string(nil))
 	assert.Equal(t, ProtoNamesForVersions(HTTPVersion1, HTTPVersion2), []string{"h2", "http/1.1"})
 }
+
+// TestBuilderValidation tests that validation checks that
+// DefaultFilters adds the required HTTP connection manager filters.
+func TestBuilderValidation(t *testing.T) {
+	if err := HTTPConnectionManagerBuilder().Validate(); err == nil {
+		t.Errorf("ConnectionManager with no filters passed validation")
+	}
+
+	if err := HTTPConnectionManagerBuilder().AddFilter(&http.HttpFilter{Name: "foo"}).Validate(); err == nil {
+		t.Errorf("ConnectionManager with no filters passed validation")
+	}
+
+	if err := HTTPConnectionManagerBuilder().DefaultFilters().Validate(); err != nil {
+		t.Errorf("ConnectionManager with default filters failed validation: %s", err)
+	}
+}
diff --git a/internal/featuretests/envoy.go b/internal/featuretests/envoy.go
index a5a290770c8..9ba4b2b4369 100644
--- a/internal/featuretests/envoy.go
+++ b/internal/featuretests/envoy.go
@@ -248,6 +248,7 @@ func filterchaintlsfallback(fallbackSecret *v1.Secret, peerValidationContext *da
 			alpn...),
 		envoy.Filters(
 			envoy.HTTPConnectionManagerBuilder().
+				DefaultFilters().
 				RouteConfigName(contour.ENVOY_FALLBACK_ROUTECONFIG).
 				MetricsPrefix(contour.ENVOY_HTTPS_LISTENER).
 				AccessLoggers(envoy.FileAccessLogEnvoy("/dev/stdout")).