refactor: ir.TCPListener to support one to many listener to routes #1798

dboslee · 2023-08-18T21:36:11Z

What this PR does / why we need it:
This change refactors the ir.TCPListener to be named directly after the corresponding gateway listener. The tcp/tls routes map to TCPRoutes attached to the TCPListener.

Which issue(s) this PR fixes:
Fixes #1635

Signed-off-by: David Boslee <david@goteleport.com>

codecov · 2023-08-18T21:42:33Z

Codecov Report

Merging #1798 (cc2f5a7) into main (14a395a) will decrease coverage by 0.20%.
The diff coverage is 71.02%.

@@            Coverage Diff             @@
##             main    #1798      +/-   ##
==========================================
- Coverage   66.12%   65.93%   -0.20%     
==========================================
  Files          86       86              
  Lines       12868    12898      +30     
==========================================
- Hits         8509     8504       -5     
- Misses       3832     3864      +32     
- Partials      527      530       +3

Files Changed	Coverage Δ
internal/ir/zz_generated.deepcopy.go	`13.17% <5.00%> (-0.30%)`	⬇️
internal/xds/translator/translator.go	`80.00% <68.42%> (+0.07%)`	⬆️
internal/xds/translator/listener.go	`82.75% <71.42%> (ø)`
internal/ir/xds.go	`73.57% <75.00%> (-0.11%)`	⬇️
internal/gatewayapi/route.go	`88.30% <97.36%> (-0.09%)`	⬇️
internal/gatewayapi/helpers.go	`81.13% <100.00%> (ø)`
internal/gatewayapi/listener.go	`97.87% <100.00%> (+0.08%)`	⬆️

... and 2 files with indirect coverage changes

Signed-off-by: David Boslee <david@goteleport.com>

arkodg · 2023-08-18T21:53:11Z

thanks for fixing this, heads up, there are 2 PRs approved and ready to be merged #1702 #1788 that might require rebasing here.
pro tip, a quick way to generate golden test file data is to run make testdata, looks like youve already found it 🎉

Signed-off-by: David Boslee <david@goteleport.com>

arkodg · 2023-08-23T21:51:46Z

internal/gatewayapi/helpers.go

 	return fmt.Sprintf("%s/%s/%s", listener.gateway.Namespace, listener.gateway.Name, listener.Name)
 }

-func irTLSListenerName(listener *ListenerContext, tlsRoute *TLSRouteContext) string {
+func irTLSRouteName(listener *ListenerContext, tlsRoute *TLSRouteContext) string {


does listener need to be part of the name, can this helper look more like irRouteName w/o the the matchIdx

arkodg · 2023-08-23T21:52:49Z

internal/gatewayapi/helpers.go

 	return fmt.Sprintf("%s/%s/%s/%s", listener.gateway.Namespace, listener.gateway.Name, listener.Name, tlsRoute.Name)
 }

-func irTCPListenerName(listener *ListenerContext, tcpRoute *TCPRouteContext) string {
+func irTCPRouteName(listener *ListenerContext, tcpRoute *TCPRouteContext) string {


does listener need to be part of the name, can this helper look more like irRouteName w/o the the matchIdx

arkodg · 2023-08-23T21:55:25Z

internal/gatewayapi/listener.go

@@ -119,6 +119,13 @@ func (t *Translator) ProcessListeners(gateways []*GatewayContext, xdsIR XdsIRMap
 					irListener.Hostnames = append(irListener.Hostnames, "*")
 				}
 				gwXdsIR.HTTP = append(gwXdsIR.HTTP, irListener)
+			case v1beta1.TCPProtocolType, v1beta1.TLSProtocolType:


dont we need to set irListener.Hostnames for TLS ?
https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1beta1.Listener

arkodg · 2023-08-23T21:59:38Z

internal/ir/xds.go

@@ -679,7 +679,16 @@ type TCPListener struct {
 	Address string `json:"address" yaml:"address"`
 	// Port on which the service can be expected to be accessed by clients.
 	Port uint32 `json:"port" yaml:"port"`
-	// TLS holds information for configuring TLS on a listener
+	// Routes associated with TCP traffic to the listener.
+	Routes []*TCPRoute `json:"routes" yaml:"routes"`


Suggested change

Routes []*TCPRoute `json:"routes" yaml:"routes"`

Routes []*TCPRoute `json:"routes,omitempty" yaml:"routes,omitempty"`

arkodg · 2023-08-23T22:26:27Z

thanks @dboslee, looks mostly good, added some comments

dboslee · 2023-09-07T15:17:39Z

Was OOO, I will circle back to this in the next few days.

arkodg · 2023-09-12T19:20:37Z

np thanks @dboslee

arkodg · 2023-10-10T00:37:54Z

@dboslee checking in to see if you are still actively working on this PR

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

github-actions · 2023-11-09T04:02:09Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg · 2024-04-25T19:58:42Z

closing in favor of #3271

* Refactor IR creation for TLSRoute Takes forward PR #1798 Fixes: #1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * pass lint Signed-off-by: Dingkang Li <dingkang1743@gmail.com> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Dingkang Li <dingkang1743@gmail.com> Co-authored-by: Arko Dasgupta <arko@tetrate.io>

Add ir.TCPRoute and refactor ir.TCPListener

7b57a54

Signed-off-by: David Boslee <david@goteleport.com>

dboslee requested a review from a team as a code owner August 18, 2023 21:36

run make go.testdata.complete

a00e935

Signed-off-by: David Boslee <david@goteleport.com>

Ronnie-personal mentioned this pull request Aug 19, 2023

e2e: reload test #1683

Open

dboslee changed the title ~~refactor: ir.TLSListener to support one to many listener to routes~~ refactor: ir.TCPListener to support one to many listener to routes Aug 21, 2023

Merge remote-tracking branch 'origin/main' into david/tcp-listener

649d275

Signed-off-by: David Boslee <david@goteleport.com>

arkodg mentioned this pull request Aug 22, 2023

use / instead of - in IR Route Name #1814

Merged

Merge branch 'main' into david/tcp-listener

a2b21da

arkodg reviewed Aug 23, 2023

View reviewed changes

Merge branch 'main' into david/tcp-listener

cc2f5a7

arkodg added a commit to arkodg/gateway that referenced this pull request Oct 17, 2023

Refactor IR creation for TLSRoute

fede451

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg mentioned this pull request Oct 17, 2023

fix: Refactor IR creation for TLSRoute #1976

Closed

arkodg added a commit to arkodg/gateway that referenced this pull request Oct 26, 2023

Refactor IR creation for TLSRoute

9da3f2d

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

github-actions bot added the stale label Nov 9, 2023

arkodg added a commit to arkodg/gateway that referenced this pull request Nov 14, 2023

Refactor IR creation for TLSRoute

9d49777

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg added a commit to arkodg/gateway that referenced this pull request Nov 15, 2023

Refactor IR creation for TLSRoute

3bb8a1a

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

aoledk pushed a commit to aoledk/gateway that referenced this pull request Apr 25, 2024

Refactor IR creation for TLSRoute

6093b59

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

aoledk pushed a commit to aoledk/gateway that referenced this pull request Apr 25, 2024

Refactor IR creation for TLSRoute

05b9d6d

Takes forward PR envoyproxy#1798 Fixes: envoyproxy#1635 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

aoledk mentioned this pull request Apr 25, 2024

refactor: refactor TCP IR #3271

Merged

arkodg closed this Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: ir.TCPListener to support one to many listener to routes #1798

refactor: ir.TCPListener to support one to many listener to routes #1798

dboslee commented Aug 18, 2023

codecov bot commented Aug 18, 2023 •

edited

Loading

arkodg commented Aug 18, 2023 •

edited

Loading

arkodg Aug 23, 2023

arkodg Aug 23, 2023

arkodg Aug 23, 2023

arkodg Aug 23, 2023

arkodg commented Aug 23, 2023

dboslee commented Sep 7, 2023

arkodg commented Sep 12, 2023

arkodg commented Oct 10, 2023

github-actions bot commented Nov 9, 2023

arkodg commented Apr 25, 2024

	Routes []*TCPRoute `json:"routes" yaml:"routes"`
	Routes []*TCPRoute `json:"routes,omitempty" yaml:"routes,omitempty"`

refactor: ir.TCPListener to support one to many listener to routes #1798

refactor: ir.TCPListener to support one to many listener to routes #1798

Conversation

dboslee commented Aug 18, 2023

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

arkodg commented Aug 18, 2023 • edited Loading

arkodg Aug 23, 2023

Choose a reason for hiding this comment

arkodg Aug 23, 2023

Choose a reason for hiding this comment

arkodg Aug 23, 2023

Choose a reason for hiding this comment

arkodg Aug 23, 2023

Choose a reason for hiding this comment

arkodg commented Aug 23, 2023

dboslee commented Sep 7, 2023

arkodg commented Sep 12, 2023

arkodg commented Oct 10, 2023

github-actions bot commented Nov 9, 2023

arkodg commented Apr 25, 2024

codecov bot commented Aug 18, 2023 •

edited

Loading

arkodg commented Aug 18, 2023 •

edited

Loading