-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Add basic support for target port in gateways in Connect #50912
base: master
Are you sure you want to change the base?
Conversation
The way DocumentsService.createGatewayDocument is implemented means that the targetSubresourceName property is always present, but it can be undefined.
Now that we have support for the target port in Connect's gateways, we can show the ports and then open a gateway for that specific port on click.
It was used only in tests and it made sense only for web apps anyway.
…instead of in each function that uses it.
This will be needed in tests that check target port validation.
// | ||
// Padding is used instead of margin here on purpose, so that there's no empty transparent space | ||
// between Separator and Label – otherwise clicking on that space would count as a click on | ||
// MenuList and not trigger onClick set on Separator or Label. | ||
& + ${MenuItemSectionLabel} { | ||
${MenuItemSectionSeparator} + &, &:first-child { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is overall confusing since I had to reorder the definitions and then I moved adding that extra padding-top between components. The previous selector for extra padding was also confusing, because it was defined in MenuItemSectionSeparator
, but it looked like this: & + ${MenuItemSectionLabel} {
. So even though we were in styles for MenuItemSectionSeparator
, we were setting styles for MenuItemSectionLabel
when it directly follows a separator. 🙃
It's might be easier to look at the master version and the version from this branch.
&:invalid, | ||
&:invalid:hover { | ||
border-color: ${props => | ||
props.theme.colors.interactive.solid.danger.default}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that each individual port field can be highlighted in red if the relevant request fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible for the OfflineGateway
state to "soft lock". If you create a doc for target port 1337, then close the app, then remove that port from TCP ports of the app, then reopen the app, creating the gateway will fail. You won't be able to change the target port until you remove the gateway from the connections list.
This is also something I'm going to address in another PR. But this whole thing definitely requires too much work for how niche it's going to be. 😭
im still going through this one. i plan to work on it more tomorrow and then test it locally |
changelog: Added support for using multi-port TCP apps in Teleport Connect without VNet
This PR is rather large, but I didn't want to split it into one where I do UI changes and one where I do tshd changes.
The general goal of this PR is to add support for target port to app gateways in Connect. This way people who cannot use VNet are still able to use multi-port apps through Connect. However, we assumed that this is going to be a niche use case, since using multi-port apps without VNet is going to be a pain.
tsh proxy app
already got support for--target-port
in #50429.The commits accomplish three main things and are arranged in that order:
1. Add UI for changing target port
When VNet is available, users can click three dots next to the "Connect" button of an app and select "Connect without VNet". This opens a regular app gateway – it's already implemented on master. What this PR adds is a new field called "Target Port". By default it's set to the first TCP port from the app spec.
In an upcoming PR, I want to make it so that available ports are visible in the document with the gateway.
Why is target port stored as target subresource name on the gateway?
Why is target port stored as target subresource name on the gateway?
Mistakes of the past
Historically, we didn't realize that teleterm URIs should be somewhat extended to allow them to map to structs such as
RouteToApp
orRouteToDatabase
. It didn't help that we started with database gateways which have the most auxiliary fields.When you think about it, the role of a gateway (AKA local proxy) is to route connections coming to it to the correct destination. But how do you describe this destination? Well, teleterm used URIs, but the Teleport services use the structs that I mentioned, e.g.
RouteToDatabase
. CompareRouteToDatabase
to the pattern used for db URIs:teleport/api/proto/teleport/legacy/client/proto/authservice.proto
Lines 296 to 308 in c6e442f
teleport/lib/teleterm/api/uri/uri.go
Line 31 in c6e442f
RouteToDatabase
carries much more fields than can be encoded in a URI! How do we pass them to the gateway? Well, we opted to use one field for the URI of the target and then added two more fields to pass the db username and db name:teleport/lib/teleterm/clusters/cluster_gateways.go
Lines 37 to 44 in c6e442f
But really what we should have done is introduce a URI like this:
Current situation
Target subresource name is used to store the database name of a db gateway which has some similar properties to the target port of an app gateway. Namely, it can be changed while the gateway is live. As we tried to write gateways in a target-agnostic fashion, it means that we can just put the target port into the target subresource name and stuff will… just work.
Target subresource name is already stored on the document etc. Any other alternative would require more work for what is a rather niche use case. Now that I'm finished with the basic support, I think from all the available options this is a pretty good one.
2. Allow non-VNet users to see available target ports
This PR adds the UI in the screenshot on the right. This is so that Windows and Linux users are stil able to see what ports are available. Clicking on a port opens a new gateway with that port set as the target port.
Because of how gateways work, currently it's possible to open only a single gateway per app. I'm going to change this in an upcoming PR, but I didn't want to put even more stuff in there. I want users to be able to add multiple gateways for the same app but with a different target port.
3. Pass the target port to tshd and allow changing it
Once the UI has the target port, it passes it as the target subresource name to tshd and tshd needs to put it in
RouteToApp.TargetPort
. When a gateway is created, we generate a cert that's going to have thatRouteToApp
inside and all gateway connections will be routed to that target port.I had to figure out how to change the target port while the gateway is live. Fortunately, I didn't have to think too much because Grzegorz already did it when working on reissuing kube certs after assuming a request (#50553). There he faced a similar problem where a certain action in the UI needs to result in the cert being updated.
I utilized the same solution as Grzegorz: when the subresource name is changed, I simply remove the cert from the gateway (AKA local proxy) and then I updated the gateway middleware to refresh the cert not only when it's expired, but also when it's missing. This means that the user is free to change the target port as much as they want to. Then on the next connection that comes to the gateway, the middleware will fetch a new cert with the updated target port.
How to test this
Set up a multi-port app (works with Teleport v17.1+). To simulate Connect running on a system with no VNet support, change this line:
teleport/web/packages/teleterm/src/ui/Vnet/vnetContext.tsx
Lines 83 to 86 in 2aed5bf