tsh proxy app: Add support for multi-port TCP apps#50429
Conversation
| // newLocalProxyAppWithPortMapping creates a new generic app proxy. Unlike newLocalProxyApp, it | ||
| // accepts a specific port mapping as an argument. | ||
| func newLocalProxyAppWithPortMapping(tc *client.TeleportClient, appInfo *appInfo, portMapping client.PortMapping, insecure bool) *localProxyApp { | ||
| func newLocalProxyAppWithPortMapping(ctx context.Context, tc *client.TeleportClient, appInfo *appInfo, app types.Application, portMapping client.PortMapping, insecure bool) (*localProxyApp, error) { |
There was a problem hiding this comment.
I don't quite understand the idea behind appInfo. In theory, it has a GetApp method which lazily fetches the app or returns it if it was already fetched. However, to execute that method the callsite needs to have access to api/client.GetResourcesClient. Because of that, I opted for passing types.Application in addition to appInfo.
There was a problem hiding this comment.
You're right, appInfo isn't really useful here. We just need proto.RouteToApp and profile from it for some
apps. I think we should remove it localProxyApp and add those two fields.
Alternatively, just set appInfo.app = app instead of adding a duplicate field.
There was a problem hiding this comment.
I replaced appInfo on localProxyApp with profile and routeToApp.
github-actions
Bot
added
size/md
tsh
| return PortMapping{}, nil | ||
| } | ||
|
|
||
| parts := strings.SplitN(rawPorts, ":", 2) |
There was a problem hiding this comment.
strings.Cut is almost always better than strings.SplitN(2) IMO.
There was a problem hiding this comment.
Oh TIL, I didn't know about it. With strings.SplitN, I always forget if SplitN(1) or SplitN(2) does the equivalent of strings.Cut.
| // As tsh proxy app is not the recommended way to access multi-port apps, let's bail out of | ||
| // using the existing cert and instead always generate a new one. |
There was a problem hiding this comment.
Will the new port-specific certificate get stored? If not, and multi-port certs are never supposed to be stored, should we delete the stale on-disk certificate?
There was a problem hiding this comment.
Local proxies always keep generated certs in memory, so the new port-specific cert will not be stored on disk. Multi-port certs can be stored. This might be useful in a situation where someone cannot use neither VNet nor local proxies. I think there's no harm in not touching the existing cert that's on disk. It's also not necessarily stale, the user might be using it for some other purposes than setting up a local proxy.
Joerger
left a comment
Joerger
left a comment
There was a problem hiding this comment.
Only minor comments, Approving since I'll be OOO
| // when running "ssh" commands with a tsh "ProxyCommand". | ||
| func mustLoginSetEnv(t *testing.T, s *suite, args ...string) (tshHome, kubeConfig string) { | ||
| tshHome, kubeConfig = mustLogin(t, s, args...) | ||
| // deprecated: Create a new helper that depends on mustLogin instead which requires migrating from |
There was a problem hiding this comment.
| // deprecated: Create a new helper that depends on mustLogin instead which requires migrating from | |
| // TODO(ravicious): Create a new helper that depends on mustLogin instead which requires migrating from |
Did you intend to follow up on this TODO in this PR or just leave a note?
There was a problem hiding this comment.
Just leave a note. testenv.MakeTestServer has been there for quite some time, so I just wanted to make sure that the next person who's going to copy paste some test structure for another tsh test is aware that functions that depend on newTestSuite are deprecated too.
|
/excludeflake * |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
@ravicious See the table below for backport results.
|
…0429) * Pass around PortMapping rather than port as string * Pass target port to local proxy * Validate target port * Accept target-port flag in tsh app login * Don't reuse certs for multi-port apps * Add a test for multi-port tsh proxy app * Use strings.Cut instead of strings.SplitN * Remove appInfo from localProxyApp * Remove unused method appInfo.appLocalCAPath
4 participants
This PR adds support for multi-port apps to
tsh proxy app, so that users on platforms where VNet is not available can still access those apps. The target port can be specified by appending:<target portto the--port flag, e.g.,tsh proxy app foo --port 3000:8080See the relevant section from the RFD.I also added support for the
--target-portflag totsh app login, mostly just so that if for whatever reason someone needs a cert for a specific port, they can get one. However,tsh proxy appis not going to use that cert for multi-port apps. Doing so would require implementing a lot of logic around this. We'd need to consider both the target port passed through argv and the target port included in the cert. Astsh proxy appis not the recommended way to access multi-port apps, I made it so that when pointed at a multi-port app,tsh proxy appignores the app cert on disk and instead always fetches a new one.If the target port is skipped, the connection is routed to the first TCP port in the app spec. This is similar to a connection made through a client that doesn't know about
TargetPortat all.Arguably, the output of
tsh app logincould include something like--port 0:8765in the proxy instructions. But I'm not sure if it's worth the effort, given thattsh app loginis an obscure escape hatch rather than a recommended way of interacting with multi-port apps.Another not so great thing is that because of the format of the port flag, if you want to start a proxy on a random port to a specific target port, you need to pass something like
--port 0:8765. We could support--target-porton top of that, but I feel like it'd be another case of adding too much complexity to a niche use case.changelog: Added support for multi-port TCP apps to
tsh proxy app