From e10ed6e1628300ee79c6fb0a1d0b8f0178720788 Mon Sep 17 00:00:00 2001
From: Shriram Rajagopalan <rshriram@users.noreply.github.com>
Date: Thu, 22 Aug 2019 00:06:26 -0400
Subject: [PATCH] VirtualInbound: dont modify filter chains in place (#16451)

Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
---
 .../core/v1alpha3/listener_builder.go         |  5 +-
 .../pilot/envoyfilter/main_test.go            | 54 ++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/pilot/pkg/networking/core/v1alpha3/listener_builder.go b/pilot/pkg/networking/core/v1alpha3/listener_builder.go
index 70884d8077b0..0a3e7e5de6f2 100644
--- a/pilot/pkg/networking/core/v1alpha3/listener_builder.go
+++ b/pilot/pkg/networking/core/v1alpha3/listener_builder.go
@@ -20,6 +20,7 @@ import (
 	"github.com/envoyproxy/go-control-plane/envoy/api/v2/listener"
 	tcp_proxy "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/tcp_proxy/v2"
 	xdsutil "github.com/envoyproxy/go-control-plane/pkg/util"
+	gogoproto "github.com/gogo/protobuf/proto"
 	"github.com/gogo/protobuf/types"
 
 	networking "istio.io/api/networking/v1alpha3"
@@ -104,7 +105,7 @@ func reduceInboundListenerToFilterChains(listeners []*xdsapi.Listener) ([]*liste
 			continue
 		}
 		for _, c := range l.FilterChains {
-			newChain, needTLSLocal := amendFilterChainMatchFromInboundListener(c, l, needTLS)
+			newChain, needTLSLocal := amendFilterChainMatchFromInboundListener(gogoproto.Clone(c).(*listener.FilterChain), l, needTLS)
 			chains = append(chains, newChain)
 			needTLS = needTLS || needTLSLocal
 		}
@@ -123,6 +124,8 @@ func (builder *ListenerBuilder) aggregateVirtualInboundListener() *ListenerBuild
 			Name: xdsutil.OriginalDestination,
 		},
 	)
+	// TODO: Trim the inboundListeners properly. Those that have been added to filter chains should
+	// be removed while those that haven't been added need to remain in the inboundListeners list.
 	filterChains, needTLS := reduceInboundListenerToFilterChains(builder.inboundListeners)
 
 	builder.virtualInboundListener.FilterChains =
diff --git a/tests/integration/pilot/envoyfilter/main_test.go b/tests/integration/pilot/envoyfilter/main_test.go
index 1a10cdc751c8..97b50c01c95b 100644
--- a/tests/integration/pilot/envoyfilter/main_test.go
+++ b/tests/integration/pilot/envoyfilter/main_test.go
@@ -57,7 +57,6 @@ spec:
     match:
       context: SIDECAR_INBOUND
       listener:
-        portNumber: 80
         filterChain:
           filter:
             name: "envoy.http_connection_manager"
@@ -75,6 +74,46 @@ spec:
             function envoy_on_response(handle)
               handle:logWarn("DEBUG RESPONSE")
             end
+  - applyTo: NETWORK_FILTER
+    match:
+      context: SIDECAR_INBOUND
+      listener:
+        portNumber: 80
+        filterChain:
+          filter:
+            name: "envoy.http_connection_manager"
+    patch:
+      operation: MERGE
+      value:
+        typed_config:
+          "@type": "type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager"
+          access_log:
+          - name: envoy.http_grpc_access_log
+            config: 
+              common_config:
+                log_name: "grpc-als-example"
+                grpc_service:
+                  envoy_grpc:
+                    cluster_name: grpc-als-cluster
+  - applyTo: CLUSTER
+    match:
+      context: SIDECAR_INBOUND
+    patch:
+      operation: ADD
+      value:
+        name: grpc-als-cluster
+        type: STRICT_DNS
+        connect_timeout: 0.25s
+        http2_protocol_options: {}
+        load_assignment:
+          cluster_name: grpc-als-cluster
+          endpoints:
+          - lb_endpoints:
+            - endpoint:
+                address:
+                  socket_address:
+                    address: 127.0.0.1
+                    port_value: 9999
 `
 
 	AppConfig = `
@@ -225,6 +264,7 @@ func checkHTTPFilter(resp *xdsapi.DiscoveryResponse) (success bool, e error) {
 	}
 
 	expectedHTTPFilters := []string{"istio_authn", "envoy.lua", "mixer", "envoy.cors", "envoy.fault", "envoy.router"}
+	expectedHTTPAccessLogFilteers := []string{"envoy.file_access_log", "envoy.http_grpc_access_log"}
 	var listenerToCheck *xdsapi.Listener
 	got := map[string]struct{}{}
 	for _, res := range resp.Resources {
@@ -256,6 +296,9 @@ func checkHTTPFilter(resp *xdsapi.DiscoveryResponse) (success bool, e error) {
 					}
 				}
 
+				if err := hcm.Validate(); err != nil {
+					return false, fmt.Errorf("invalid http connection manager: %v", err)
+				}
 				httpFiltersFound := make([]string, 0)
 				for _, httpFilter := range hcm.HttpFilters {
 					httpFiltersFound = append(httpFiltersFound, httpFilter.Name)
@@ -265,6 +308,15 @@ func checkHTTPFilter(resp *xdsapi.DiscoveryResponse) (success bool, e error) {
 						expectedHTTPFilters, httpFiltersFound)
 				}
 
+				accessLogFiltersFound := make([]string, 0)
+				for _, al := range hcm.AccessLog {
+					accessLogFiltersFound = append(accessLogFiltersFound, al.Name)
+				}
+				if !reflect.DeepEqual(expectedHTTPAccessLogFilteers, accessLogFiltersFound) {
+					return false, fmt.Errorf("excepted accesslog filters %+v, got %+v",
+						expectedHTTPAccessLogFilteers, accessLogFiltersFound)
+				}
+
 			}
 		}
 	}