Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: tcp listener is rejected when no route attached #4681

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: tcp listener is rejected when no route attached #4681

wants to merge 2 commits into from

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Nov 8, 2024 •
edited
Loading

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.

@zhaohuabing zhaohuabing requested a review from a team as a code owner November 8, 2024 08:49
@zhaohuabing zhaohuabing force-pushed the fix-tcp-listener-without-route branch 2 times, most recently from cb94a37 to 17f3476 Compare November 8, 2024 09:00
@codecov Codecov
Copy link

codecov bot commented Nov 8, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 65.56%. Comparing base (ec56a83) to head (742c946).

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.53%   65.56%   +0.02%     
==========================================
  Files         211      211              
  Lines       31954    31973      +19     
==========================================
+ Hits        20940    20962      +22     
+ Misses       9769     9766       -3     
  Partials     1245     1245

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

@zhaohuabing
fix: tcp listener is rejected when no route attached
9bcea0b 
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Nov 8, 2024

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
Copy link
Member Author

zhaohuabing commented Nov 12, 2024 •
edited
Loading

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
Copy link
Contributor

arkodg commented Nov 13, 2024 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TCP listener creation failed when there is no TCP route defined
2 participants
@zhaohuabing @arkodg