gateway icon indicating copy to clipboard operation
gateway copied to clipboard

Refactor Xds Listener for TLSRoutes

Open arkodg opened this issue 11 months ago • 14 comments

Description:

Today, the TLSRoute name is incorporated into the Xds Listener name so when 1 of many TLS Routes gets deleted, the Xds Listener gets recreated which is not great.

Instead,

  • Create 1 IR TCP Listener per Gateway Listener
  • Map TLSRoutes context into the same IR TCP Listener, maybe by creating an array of IR TCP Routes
  • Create a unique Filter Chain match for every IR TCP Route, using SNIs as the unique differentiator

thanks @dboslee for catching this

Relates to https://github.com/envoyproxy/gateway/issues/1634#issuecomment-1626357641

arkodg avatar Jul 08 '23 00:07 arkodg

@arkodg Can I take over this ?

NomadXD avatar Jul 14 '23 04:07 NomadXD

go for it @NomadXD !

arkodg avatar Jul 18 '23 20:07 arkodg

@arkodg I understand the issue here with having ir.TCPListener.Name as the listener name. But when we retrieve a Listener, we just use the address and the port.

https://github.com/envoyproxy/gateway/blob/3215e2d6eefe8f550db3bc0d3ec2d1427532156e/internal/xds/translator/translator.go#L228

And then we create a FilterChain with a TCPProxy filter with a FilterChainMatch based on SNI.

https://github.com/envoyproxy/gateway/blob/3215e2d6eefe8f550db3bc0d3ec2d1427532156e/internal/xds/translator/listener.go#L221

So just removing the ir.TCPListener.Name from the Listener would fix the issue. Isn't it so ?

NomadXD avatar Jul 21 '23 08:07 NomadXD

@NomadXD we need a Name to create a unique xds listener (in case none exist and findXdsListenerByHostPort returns null)

arkodg avatar Jul 21 '23 15:07 arkodg

@arkodg Yes, for that we can use a combination of address and port. For example 172.17.0.3 and 9090 creates a listener with name listener_172-17-0-3_9090. AFAIU this is the only change required to fix this issue. Isn't it ? Will try this and send a PR

NomadXD avatar Jul 26 '23 03:07 NomadXD

@envoyproxy/gateway-maintainers should the Xds Listener name be based off the IR Listener Name or the Listening Address and Port as suggested above ?

arkodg avatar Jul 27 '23 17:07 arkodg

hey @NomadXD, @zirain @AliceProxy and I discussed this in the community meeting today and we prefer mapping the xds Listener name to the IR name to improve debuggability of mapping output resources to input resources, can you help implement the suggestions from https://github.com/envoyproxy/gateway/issues/1635#issue-1794562791 ?

arkodg avatar Aug 09 '23 03:08 arkodg

I can implement this if needed, @NomadXD let me know if you still plan on taking this or not.

dboslee avatar Aug 17 '23 19:08 dboslee

@dboslee Sorry I've been busy lately with work and couldn't look into this. You can go ahead and do it.

NomadXD avatar Aug 18 '23 01:08 NomadXD

thanks @dboslee ! assigning this to you

arkodg avatar Aug 18 '23 03:08 arkodg

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

github-actions[bot] avatar Sep 17 '23 04:09 github-actions[bot]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

github-actions[bot] avatar Nov 25 '23 20:11 github-actions[bot]

@arkodg Please assign this issue to me and I'll work based on your PR. Then I'll take #3163 after this issue is done.

aoledk avatar Apr 24 '24 07:04 aoledk

/assign aoledk

zirain avatar Apr 24 '24 07:04 zirain