fix: tcp listener is rejected when no route attached by zhaohuabing · Pull Request #4681 · envoyproxy/gateway

zhaohuabing · 2024-11-08T08:49:13Z

Create a default route pointing to a static cluster without endpoints when there is no TCPRoutes attached to a TCP listener. This avoids Envoy rejecting a listener without a filter chain and reporting warning.

Fixes #4680

Release Notes: Yes

Alternative consideration:
An alterantive approach could be to not create the xDS listener, but this behavior doesn't align with the Gateway semantics(which does defines a listener) and the behavior of the empty HTTP listener.

Other type of listeners:

A HTTP listener without HTTPRoute will create the xDS HTTP listener and an empty RouteConfiguration.
An UDP listener without UDPRoute won't create the xDS UDP listerner at all.

codecov · 2024-11-08T09:07:02Z

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.62%. Comparing base (8a01dd6) to head (b037abe).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/translator.go	68.42%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4681      +/-   ##
==========================================
+ Coverage   65.56%   65.62%   +0.05%     
==========================================
  Files         211      211              
  Lines       31998    32017      +19     
==========================================
+ Hits        20979    21010      +31     
+ Misses       9775     9764      -11     
+ Partials     1244     1243       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Huabing Zhao <[email protected]>

arkodg · 2024-11-08T20:54:12Z

thanks for looking into this @zhaohuabing
can you share what the flow looks like from a client perspective (e.g. using curl) when we either dont create a tcp listener vs tcp listener with dummy filter chain

zhaohuabing · 2024-11-12T05:36:03Z

thanks for looking into this @zhaohuabing can you share what the flow looks like from a client perspective (e.g. using curl) when we either dont create a tcp listener vs tcp listener with dummy filter chain

can't connect vs connection reset by peer, the latter seems make more sense as there's a listening port on Envoy for the defined Gateway listener.

no tcp listener:

curl -v http://${GATEWAY_HOST}:8080
*   Trying 172.18.0.200:8080...
* connect to 172.18.0.200 port 8080 from 172.18.0.1 port 54918 failed: Connection refused
* Failed to connect to 172.18.0.200 port 8080 after 0 ms: Couldn't connect to server
* Closing connection
curl: (7) Failed to connect to 172.18.0.200 port 8080 after 0 ms: Couldn't connect to server

dummy filter chain

curl -v http://${GATEWAY_HOST}:8080                    
*   Trying 172.18.0.200:8080...
* Connected to 172.18.0.200 (172.18.0.200) port 8080
> GET / HTTP/1.1
> Host: 172.18.0.200:8080
> User-Agent: curl/8.5.0
> Accept: */*
> 
* Recv failure: Connection reset by peer
* Closing connection
curl: (56) Recv failure: Connection reset by peer

arkodg · 2024-11-13T02:44:47Z

hey @zhaohuabing I prefer if we dont generate an xds listener for this case
wdyt @envoyproxy/gateway-maintainers

zirain · 2024-11-19T00:27:12Z

hey @zhaohuabing I prefer if we dont generate an xds listener for this case wdyt @envoyproxy/gateway-maintainers

it would be better if possiable.

arkodg · 2024-11-19T00:55:29Z

from @haq204's comment #4680 (comment) , we may want to have a listener up for responding to TCP health checks

Signed-off-by: Huabing Zhao <[email protected]>

arkodg · 2024-11-19T03:10:05Z

internal/xds/translator/translator.go

+		// If there are no routes, add a route without a destination to the listener to create a filter chain
+		// This is needed because Envoy requires a filter chain to be present in the listener, otherwise it will reject the listener and report a warning
+		if len(tcpListener.Routes) == 0 {
+			nullRouteCluster := &clusterv3.Cluster{


hey curious, what happens if we set xdsListener.FilterChains = [] for this case, does xDS still complain ?

Envoy will complain no filter chains specified. It's a validation inside the Envoy listener impl instead of a proto type built-in validation.

https://github.com/envoyproxy/envoy/blob/79958991ffe0cf8d59a4a351646c76d672dada83/source/common/listener_manager/listener_impl.cc#L763-L770

type.googleapis.com/envoy.config.listener.v3.Listener rejected: Error adding/updating listener(s) gateway-conformance-infra/same-namespace/tls: error adding listener '0.0.0.0:10443': no filter chains specified

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-11-19T06:32:19Z

internal/xds/translator/testdata/in/xds-ir/listener-connection-limit.yaml

-    - endpoints:
-      - host: "1.2.3.4"
-        port: 50000
+  routes:


This is a bug in the listener-connection-limit test. The routes field in the TCP Listener ir is missing.
This is not directly related to the issue addressed by this PR, but an empty cluster will be added if no route defined for the test.

zhaohuabing · 2024-11-19T06:33:50Z

internal/xds/translator/testdata/in/xds-ir/listener-connection-limit.yaml

-    - endpoints:
-      - host: "1.2.3.4"
-        port: 50000
+  routes:


Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-11-19T06:48:37Z

internal/xds/translator/testdata/in/xds-ir/listener-tcp-keepalive.yaml

-    - endpoints:
-      - host: "1.2.3.4"
-        port: 50000
+  routes:


zhaohuabing · 2024-11-19T06:48:44Z

internal/xds/translator/testdata/in/xds-ir/listener-tcp-keepalive.yaml

-    - endpoints:
-      - host: "1.2.3.4"
-        port: 50000
+  routes:


Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-11-19T07:02:37Z

internal/xds/translator/testdata/in/xds-ir/tcp-endpoint-stats.yaml

-        port: 50000
-      - host: "5.6.7.8"
-        port: 50001
+  routes:


Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-11-19T07:05:51Z

internal/xds/translator/testdata/in/xds-ir/tcp-req-resp-sizes-stats.yaml

-        port: 50000
-      - host: "5.6.7.8"
-        port: 50001
+  routes:


Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-11-19T07:22:53Z

internal/xds/translator/testdata/in/extension-xds-ir/extensionpolicy-tcp-udp-http.yaml

  name: envoy-gateway/gateway-1/tcp1
  port: 10080
+  routes:
+  - destination:


Signed-off-by: Huabing Zhao <[email protected]>

arkodg

LGTM thanks !

zirain · 2024-11-20T01:11:22Z

/retest

* fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <[email protected]> * change cluter name Signed-off-by: Huabing Zhao <[email protected]> * fix listener connection limit test Signed-off-by: Huabing Zhao <[email protected]> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <[email protected]> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f99c36c) Signed-off-by: Huabing Zhao <[email protected]>

* fix: tcp listener is rejected when no route attached (#4681) * fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <[email protected]> * change cluter name Signed-off-by: Huabing Zhao <[email protected]> * fix listener connection limit test Signed-off-by: Huabing Zhao <[email protected]> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <[email protected]> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f99c36c) Signed-off-by: Huabing Zhao <[email protected]> * fix: remove backendrefs validation (#4705) * remove backendrefs validation Signed-off-by: Huabing Zhao <[email protected]> * add tests Signed-off-by: Huabing Zhao <[email protected]> * add tests Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Co-authored-by: zirain <[email protected]> (cherry picked from commit 5068698) Signed-off-by: Huabing Zhao <[email protected]> * fix: translator reports errors for existing clusters and secretes (#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <[email protected]> * fix cluster index for SP Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> * add comment Signed-off-by: Huabing Zhao <[email protected]> * remove index Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> * xds: always use `::` and `IPv4Compact` for dynamic listener (#4743) * enable IPv4Compact Signed-off-by: zirain <[email protected]> * fix xds test Signed-off-by: zirain <[email protected]> * release-notes Signed-off-by: zirain <[email protected]> * nit Signed-off-by: zirain <[email protected]> * gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 78da42c) Signed-off-by: Huabing Zhao <[email protected]> * Fix: frequent 503 errors when connecting to a Service experiencing high Pod churn (#4754) * Revert "fix: some status updates are discarded by the status updater (#4337)" This reverts commit 14830c7. Signed-off-by: Huabing Zhao <[email protected]> * store update events and process it later Signed-off-by: Huabing Zhao <[email protected]> * rename method Signed-off-by: Huabing Zhao <[email protected]> * add release note Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> * xds: use V4_PREFERRED dnsLookupFamily by default (#4745) * use Cluster_V4_PREFERRED Signed-off-by: zirain <[email protected]> * release notes Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: zirain <[email protected]> Co-authored-by: zirain <[email protected]>

* fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <[email protected]> * change cluter name Signed-off-by: Huabing Zhao <[email protected]> * fix listener connection limit test Signed-off-by: Huabing Zhao <[email protected]> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <[email protected]> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f99c36c) Signed-off-by: Guy Daich <[email protected]>

* fix: BackendTlsPolicy specify multiple targetRefs of the same service, only one will work (#4630) * add tests Signed-off-by: Huabing Zhao <[email protected]> * fix matching comparison Signed-off-by: Huabing Zhao <[email protected]> * add release note Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit 44c2f74) Signed-off-by: Guy Daich <[email protected]> * fix: tcp listener is rejected when no route attached (#4681) * fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <[email protected]> * change cluter name Signed-off-by: Huabing Zhao <[email protected]> * fix listener connection limit test Signed-off-by: Huabing Zhao <[email protected]> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <[email protected]> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f99c36c) Signed-off-by: Guy Daich <[email protected]> * Fix: frequent 503 errors when connecting to a Service experiencing high Pod churn (#4754) * Revert "fix: some status updates are discarded by the status updater (#4337)" This reverts commit 14830c7. Signed-off-by: Huabing Zhao <[email protected]> * store update events and process it later Signed-off-by: Huabing Zhao <[email protected]> * rename method Signed-off-by: Huabing Zhao <[email protected]> * add release note Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit 8ec3095) Signed-off-by: Guy Daich <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Guy Daich <[email protected]> Co-authored-by: Huabing Zhao <[email protected]>

) * fix: tcp listener is rejected when no route attached (envoyproxy#4681) * fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <[email protected]> * change cluter name Signed-off-by: Huabing Zhao <[email protected]> * fix listener connection limit test Signed-off-by: Huabing Zhao <[email protected]> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <[email protected]> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <[email protected]> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f99c36c) Signed-off-by: Huabing Zhao <[email protected]> * fix: remove backendrefs validation (envoyproxy#4705) * remove backendrefs validation Signed-off-by: Huabing Zhao <[email protected]> * add tests Signed-off-by: Huabing Zhao <[email protected]> * add tests Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Co-authored-by: zirain <[email protected]> (cherry picked from commit 5068698) Signed-off-by: Huabing Zhao <[email protected]> * fix: translator reports errors for existing clusters and secretes (envoyproxy#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <[email protected]> * fix cluster index for SP Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> * add comment Signed-off-by: Huabing Zhao <[email protected]> * remove index Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> * xds: always use `::` and `IPv4Compact` for dynamic listener (envoyproxy#4743) * enable IPv4Compact Signed-off-by: zirain <[email protected]> * fix xds test Signed-off-by: zirain <[email protected]> * release-notes Signed-off-by: zirain <[email protected]> * nit Signed-off-by: zirain <[email protected]> * gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 78da42c) Signed-off-by: Huabing Zhao <[email protected]> * Fix: frequent 503 errors when connecting to a Service experiencing high Pod churn (envoyproxy#4754) * Revert "fix: some status updates are discarded by the status updater (envoyproxy#4337)" This reverts commit 14830c7. Signed-off-by: Huabing Zhao <[email protected]> * store update events and process it later Signed-off-by: Huabing Zhao <[email protected]> * rename method Signed-off-by: Huabing Zhao <[email protected]> * add release note Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> * xds: use V4_PREFERRED dnsLookupFamily by default (envoyproxy#4745) * use Cluster_V4_PREFERRED Signed-off-by: zirain <[email protected]> * release notes Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: zirain <[email protected]> Co-authored-by: zirain <[email protected]>

zhaohuabing requested a review from a team as a code owner November 8, 2024 08:49

zhaohuabing force-pushed the fix-tcp-listener-without-route branch 2 times, most recently from cb94a37 to 17f3476 Compare November 8, 2024 09:00

fix: tcp listener is rejected when no route attached

9bcea0b

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing force-pushed the fix-tcp-listener-without-route branch from 17f3476 to 9bcea0b Compare November 8, 2024 09:11

Merge branch 'main' into fix-tcp-listener-without-route

742c946

Merge branch 'main' into fix-tcp-listener-without-route

fd72637

Signed-off-by: Huabing Zhao <[email protected]>

arkodg reviewed Nov 19, 2024

View reviewed changes

zhaohuabing added 3 commits November 18, 2024 20:12

Merge branch 'main' into fix-tcp-listener-without-route

4bb998e

change cluter name

5b6ac41

Signed-off-by: Huabing Zhao <[email protected]>

fix listener connection limit test

d2d4bd2

Signed-off-by: Huabing Zhao <[email protected]>