Pass port from VNet to local proxy #49453

ravicious · 2024-11-26T10:51:23Z

Part of Multi-port TCP app access #46758.

Depends on #49047 and #49349. This PR implements the section of the RFD called Passing the port number from VNet to the client.

The way VNet used to work was that when a DNS query came in for an FQDN that matched the public address of an app in the cluster, VNet would create a virtual IP for it and a local proxy (ala tsh proxy app). Then the TCP connection would be routed through that proxy to the URI specified in the app spec.

Adding multi-port support means that if we have an app spec such as this:

- name: multi-port-example
  uri: tcp://localhost
  tcp_ports:
    - port: 5434 # Postgres
    - port: 8765 # HTTP echo

then a user should be able to start VNet and do curl http://multi-port-example.cluster.example.com:8765 on their computer. This connection should be then routed to localhost:8765 on the machine that's running the app service.

The backend part was implemented in #49047. This PR makes it so that when a TCP connection comes in, VNet reads the port and only then creates a local proxy with a proper TargetPort set in RouteToApp.

Implementation details

lib/vnet/vnet.go has NetworkStack.handleTCP which gets called on a new TCP connection. At that point, VNet grabs a tcpAppHandler, which is still created when a DNS query comes in – a handler is still associated with an FQDN of an app.

However, the underlying proxy is not created at the time when tcpAppHandler is initialized. Instead, from NetworkStack.handleTCP we pass the local port to tcpAppHandler.HandleTCPConnector which then gets or initializes a local proxy associated with that local port. The local proxy has RouteToApp.TargetPort set to the local port.

The tests are completely removed from how Teleport does routing or cert generation. So to verify that multi-port support works correctly within VNet, we verify if lib/vnet.AppProvider.ReissueAppCert is called with correct port in RouteToApp.

How to test this

First compile teleport and tsh from this branch. Then add a TCP app like in the snippet above. I use mendhak/http-https-echo Docker image for the HTTP echo.

  http-https-echo:
    container_name: teleport-local-http-https-echo
    image: mendhak/http-https-echo:35
    ports:
      - "8765:8080"

Then start VNet with tsh vnet -d and connect to either of the ports using an appropriate client. If you're using a local cluster, make sure <app-name>.<cluster-address> is not in /etc/hosts.

Multi-port support in VNet (#49453) backported to branch/v17.

zmb3 · 2024-11-26T15:55:35Z

lib/vnet/app_resolver.go

@@ -109,7 +111,7 @@ type TCPAppResolver struct {
 func NewTCPAppResolver(appProvider AppProvider, opts ...tcpAppResolverOption) (*TCPAppResolver, error) {
 	r := &TCPAppResolver{
 		appProvider: appProvider,
-		slog:        slog.With(teleport.ComponentKey, "VNet.AppResolver"),
+		log:         logutils.NewPackageLogger(teleport.ComponentKey, "VNet.AppResolver"),


Isn't NewPackageLogger meant to be used for package-level vars?

I forgot that this package even already defines a package-level log, I'm going to use it here then.

zmb3 · 2024-11-26T15:57:31Z

lib/vnet/app_resolver.go

 		return nil, ErrNoTCPHandler
 	}
 	if len(resp.Resources) == 0 {
 		// Didn't find any matching app, forward the request upstream.
 		return nil, ErrNoTCPHandler
 	}
 	app := resp.Resources[0].GetApp()
-	appHandler, err := r.newTCPAppHandler(ctx, profileName, leafClusterName, app)
+	appHandler, err := r.newTCPAppHandler(ctx, log, profileName, leafClusterName, app)


Hmmm - I don't love passing log in here because newTCPAppHandler also has access to r.log and it gets confusing to try to understand where you should be passing a logger around and where you should be using the logger on the method receiver.

Yeah, I know. But this log already has all the extra fields specified and they're useful both in the context of resolveTCPHandlerForCluster, as well as within a TCP app handler itself. I didn't want to repeat them twice, but I suppose doing so would at least reduce the confusion around which logger to use. So I guess that would be the way forward?

zmb3 · 2024-11-26T15:59:19Z

lib/vnet/vnet_test.go

+	const defaultPort = 123
+	if port == 0 {
+		port = defaultPort
+	}


Suggested change

const defaultPort = 123

if port == 0 {

port = defaultPort

}

port = cmp.Or(port, 123)

zmb3 · 2024-11-26T15:59:29Z

lib/vnet/vnet_test.go

@@ -235,8 +241,14 @@ func (n noUpstreamNameservers) UpstreamNameservers(ctx context.Context) ([]strin
 	return nil, nil
 }

+type appSpec struct {
+	// publicAddr is used bothas the name of the app and its public address in the final spec.


Suggested change

// publicAddr is used bothas the name of the app and its public address in the final spec.

// publicAddr is used both as the name of the app and its public address in the final spec.

nklaassen

I tried this out and it works great! This is sweet

lib/vnet/vnet_test.go

lib/vnet/app_resolver.go

zmb3 · 2024-12-02T16:49:16Z

lib/vnet/app_resolver.go

+	app              types.Application
+	portToLocalProxy map[uint16]*alpnproxy.LocalProxy
+	// mu guards access to portToLocalProxy.
+	mu sync.Mutex


Consider grouping mu with the fields it protects.

Suggested change

app types.Application

portToLocalProxy map[uint16]*alpnproxy.LocalProxy

// mu guards access to portToLocalProxy.

mu sync.Mutex

app types.Application

// mu guards access to the fields that follow

mu sync.Mutex

portToLocalProxy map[uint16]*alpnproxy.LocalProxy

Oops, forgot to address this. But I think spelling it out explicitly in the comment might be slightly better – at least you can see which fields it protects when you do a lookup on the field in your editor.

Use libutils.NewPackageLogger, call it log instead of slog which makes it harder to use the imported default slog logger instead of the one from a struct. Move creation of logger within TCPAppResolver.resolveTCPHandlerForCluster

public-teleport-github-review-bot · 2024-12-03T15:10:40Z

@ravicious See the table below for backport results.

Branch	Result
branch/v17	Failed

* Prepare app specs in tests for specifying TCP ports * Refactor logging in lib/vnet/app_resolver.go Use libutils.NewPackageLogger, call it log instead of slog which makes it harder to use the imported default slog logger instead of the one from a struct. Move creation of logger within TCPAppResolver.resolveTCPHandlerForCluster * Pass port from VNet to local proxy * Don't create another package logger * Don't pass logger to newTCPAppHandler * Fix typo in comment * Explicitly pass port to dialHost * Convert multi-line definitions of simple appSpecs to single-line * Add TODO comment about validating local port * Empty commit to trigger CI

ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Nov 26, 2024

ravicious requested a review from nklaassen November 26, 2024 10:51

github-actions bot requested review from EdwardDowling and rosstimothy November 26, 2024 10:51

github-actions bot added the size/md label Nov 26, 2024

ravicious removed the request for review from rosstimothy November 26, 2024 10:52

ravicious mentioned this pull request Nov 26, 2024

Multi-port TCP app access #46758

Open

ravicious added a commit that referenced this pull request Nov 26, 2024

Update version to 17.1.0-dev.ravicious.1

d148723

Multi-port support in VNet (#49453) backported to branch/v17.

zmb3 reviewed Nov 26, 2024

View reviewed changes

nklaassen reviewed Nov 26, 2024

View reviewed changes

lib/vnet/vnet_test.go Outdated Show resolved Hide resolved

lib/vnet/vnet_test.go Outdated Show resolved Hide resolved

lib/vnet/vnet_test.go Outdated Show resolved Hide resolved

lib/vnet/app_resolver.go Show resolved Hide resolved

ravicious requested review from zmb3 and nklaassen November 27, 2024 16:10

nklaassen approved these changes Nov 27, 2024

View reviewed changes

lib/vnet/app_resolver.go Show resolved Hide resolved

ravicious added the do-not-merge label Nov 29, 2024

zmb3 approved these changes Dec 2, 2024

View reviewed changes

ravicious removed the request for review from EdwardDowling December 3, 2024 09:57

ravicious mentioned this pull request Dec 3, 2024

Surface target ports in tsh and Connect #49688

Merged

ravicious added backport/branch/v17 and removed do-not-merge labels Dec 3, 2024

ravicious added 9 commits December 3, 2024 15:21

Prepare app specs in tests for specifying TCP ports

f8e3f96

Refactor logging in lib/vnet/app_resolver.go

8003240

Use libutils.NewPackageLogger, call it log instead of slog which makes it harder to use the imported default slog logger instead of the one from a struct. Move creation of logger within TCPAppResolver.resolveTCPHandlerForCluster

Pass port from VNet to local proxy

b5480b5

Don't create another package logger

1586063

Don't pass logger to newTCPAppHandler

bf000af

Fix typo in comment

a55c568

Explicitly pass port to dialHost

de78e1a

Convert multi-line definitions of simple appSpecs to single-line

3d4340e

Add TODO comment about validating local port

f880b58

ravicious force-pushed the r7s/multi-port-vnet branch from daa9898 to f880b58 Compare December 3, 2024 14:21

ravicious requested a review from klizhentas as a code owner December 3, 2024 14:21

ravicious requested review from russjones, r0mant, fheinecke, camscale, tcsc, rosstimothy and codingllama as code owners December 3, 2024 14:21

ravicious changed the base branch from r7s/multi-port-vnet-base to master December 3, 2024 14:21

ravicious removed request for klizhentas, russjones, r0mant, fheinecke, camscale, tcsc, rosstimothy and codingllama December 3, 2024 14:21

ravicious enabled auto-merge December 3, 2024 14:21

Empty commit to trigger CI

188bb9c

ravicious added this pull request to the merge queue Dec 3, 2024

Merged via the queue into master with commit 899ae26 Dec 3, 2024
39 checks passed

ravicious deleted the r7s/multi-port-vnet branch December 3, 2024 15:09

ravicious mentioned this pull request Dec 3, 2024

[v17] Add multi-port support for TCP apps #49711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass port from VNet to local proxy #49453

Pass port from VNet to local proxy #49453

ravicious commented Nov 26, 2024 •

edited

Loading

zmb3 Nov 26, 2024

ravicious Nov 26, 2024

zmb3 Nov 26, 2024

ravicious Nov 26, 2024

zmb3 Nov 26, 2024

zmb3 Nov 26, 2024

nklaassen left a comment

zmb3 Dec 2, 2024

ravicious Dec 3, 2024

public-teleport-github-review-bot bot commented Dec 3, 2024

	// publicAddr is used bothas the name of the app and its public address in the final spec.
	// publicAddr is used both as the name of the app and its public address in the final spec.

Pass port from VNet to local proxy #49453

Pass port from VNet to local proxy #49453

Conversation

ravicious commented Nov 26, 2024 • edited Loading

Implementation details

How to test this

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklaassen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Dec 3, 2024

ravicious commented Nov 26, 2024 •

edited

Loading