Allow opening multiple gateways for the same app but different target port #51111
Conversation
This makes it work with app gateways too.
62f6763 to
ff2931c
Compare
ff2931c to
8a85f79
Compare
There was a problem hiding this comment.
This is pretty much an integration test. It started out as a unit test focused on ConnectionTrackerService, but I realized that it's just way to hard to programatically recreate everything that happens when a gateway is created and updated. So I rewrote it to use actual components that are used in the app.
Technically we could render the connection list here too and operate on that, but I don't think it's necessary, given that I don't want to spend too much extra time on this. ;f
| await user.clear(targetPortInput); | ||
| await user.type(targetPortInput, '4242'); | ||
| // We have to lose focus of that field, otherwise React is going to warn about updates not wrapped | ||
| // in act when the focus on the page changes after opening a new doc. | ||
| await user.tab(); |
There was a problem hiding this comment.
This threw me for a loop, omg. It might have to do with HTML validation or just focus changing, I haven't checked closely. I found info about user.tab() being useful here in a discussion under a library that we don't use. https://github.com/orgs/react-hook-form/discussions/4232#discussioncomment-8447514
There was a problem hiding this comment.
The problem also went away after commenting out the effect with TCP ports in DocumentGatewayApp, but ofc we want to keep it.
8a85f79 to
ff38f39
Compare
Perhaps we could do this automatically for the user when they try to activate the connection? I think we could check if the local port is not occupied by any of the active connections, and if so, clear it. IMO I would expect the app to give me a new port instead of telling me that the previous one is already in use. |
| @@ -520,6 +524,13 @@ func (s *Service) SetGatewayTargetSubresourceName(ctx context.Context, gatewayUR | |||
| return nil, trace.Wrap(err) | |||
| } | |||
|
|
|||
| if err := s.checkIfGatewayAlreadyExists(gateway.TargetURI(), CreateGatewayParams{ | |||
There was a problem hiding this comment.
There is a small bug in UI related to this feature. If I try to set the active port again, I'm getting an error. I think we should only try to make an API call on the UI side if a new port differs from the current one.
port.exists.mov
There was a problem hiding this comment.
True, we already do that for the local port I think. I thought about it when I was adding the initial handling of target port, but I haven't added that because without those buttons it wasn't easy to trigger. But with the buttons it is of course. ;)
The buttons should also be disabled for the duration of the call to change the port, even though it's usually super quick and it sits behind a mutex on the tshd side.
There was a problem hiding this comment.
Now that port buttons are gone I think I can get away with not changing this?
| // The ports are expected to be the same. We just changed doc with port 1337 to port 4242, so the | ||
| // corresponding connection has changed from conn1337 to conn4242. conn4242 got updated with the | ||
| // port set on doc1. | ||
| expect(conn4242).toBeTruthy(); | ||
| expect(conn4242.port).toEqual(conn1337.port); |
There was a problem hiding this comment.
Perhaps we could do this automatically for the user when they try to activate the connection? I think we could check if the local port is not occupied by any of the active connections, and if so, clear it. IMO I would expect the app to give me a new port instead of telling me that the previous one is already in use.
The tricky part is that we don't do this for any other connection. The expectation is that you have your collection of connections on specific ports (that you likely use with 3rd-party apps). If one of the ports is taken, you want to know about it instead of Connect changing the port automatically, so that you can keep the setup in 3rd-party apps working (as those 3rd-party apps are configured to use specific ports).
We could make an exception for multi-port TCP apps I suppose. I didn't go with this at first because I don't like creating exceptions. ;f
There was a problem hiding this comment.
But from the user's PoV it'd obviously be better than seeing "Oh this port is taken". Having a button to automatically start the gateway on some other random port would be nice too. Currently I don't think it's obvious enough that you can clear the local port field to get a random port. But I'll leave that specific improvement for some other time.
There was a problem hiding this comment.
I talked with Grzegorz and we decided to replace the list of buttons with MenuLogin that opens a list of ports that just open a new doc instead of changing the port in the current doc. The user doesn't need to see the full list of ports at all times, it's important only when making a decision when opening a new gateway.
This helps with a use case where a user on a VNet-supported platform wants to open multiple gateways for different ports.
This will let us call this function from places which do not have access to the whole app object.
| portRange => | ||
| // Filter out single-port port ranges that are equal to the current port. | ||
| portRange.endPort !== 0 || portRange.port != currentTargetPort |
There was a problem hiding this comment.
im sure youve written/read the word port so many times its lost all its meaning. just in these few things im like 🥴
There was a problem hiding this comment.
When I was working on the earlier PR which added targetPort to OfflineGateway, I started using targetPort everywhere, even in places where I really meant localPort. 🥲
Thanks for making the commits easy to review!
Np, shuffling commits around and splitting them is sooooooooo much easier with jj.
This is the last PR related to multi-port TCP app gateways in Connect.
tshd changes
First, I made sure it's not possible to create multiple gateways with the same local port in tshd. This is historically something that we've enforced through the UI for db gateways (cannot create multiple gateways for the same db + db user), but we've never enforced that in tshd. Technically there's nothing wrong with creating multiple gateways on different local ports otherwise have the same params. However, it doesn't make much sense and is harder to manage from a UX standpoint.
Connection tracking changes in UI
Second, remove
gatewayUrifromTrackedGatewayConnection. This is related to how_refreshStateinConnectionTrackerServiceworks.In short, the connections in the top left in the app are always based on connections persisted on disk and currently open gateways and documents. When you open a new gateway document and create a gateway,
_refreshStateis called. It sees that there's a newdoc.gateway, but it's not able to find a matching connection that would have the same gateway params as the doc, so it creates a new connection and stores the gateway URI inconnection.gatewayUri. Later on whenever the connection needs to refer to the gateway in any way, it just uses this field.This used to work, because given a connection, you could find the gateway using either gateway URI or gateway params saved on connections. Both fields have never changed – once you created a gateway, you weren't able to change its URI or its params through which it's identified.
However, with the advent of target port in app gateways, you actually are able to change one of the params through which a gateway is identified after the gateway was created. This means that we can no longer use gateway URI or gateway params to make a link between a connection in the top left and a gateway. We always have to use gateway params.
UI properties that come out of this
Terminology:
Approaching this problem in this way means that if you open an app gateway and change its target port, you now have two connections in the top left. One for the previous target port which is now marked as offline, and one for the new target port which is marked as online. They both share the same local port, which means if you try to activate the old one, you'll get an error and you'll have to adjust the port.
However, this is mostly going to be a problem for VNet users that are going to continue to use the traditional app gateways for multi-port apps. With VNet support enabled, the only way for you to create more than one gateway is to click three dots -> "Connect without VNet", change the target port to something else, then create another gateway (which uses the first target port from the spec). This new gateway with the default target port won't be created on the first try as the old connection still has the same local port as the original gateway that's currently running.
For users on platforms that do not support VNet (for which this whole feature is made in the first place), I don't expect it to be that much of a problem. I expect them to create gateways through the three dots menu where clicking on a specific target port creates a new gateway with a random local port, which avoids the whole issue I described above.
This should work good enough for the default case and while it could probably be improved in a way, I don't really want to invest more time in what is probably going to be a very niche use case. We don't expect people to use multi-port TCP apps that much on platforms that do not support VNet, simply because it sucks to use them without VNet. VNet is the reason we're adding multi-port support in the first place.
Considered alternatives
At first I tried to make it so that when operating on connections, we identify documents by
gatewayUriwhenever possible and I wanted to allow for multiple gateways with the same target port. However, operating on connections through "hard links" to gateways is just not feasible because of the reasons I mentioned above. When the target port is changed, the old gateway URI remains in the connection, as well as the old target port, but the gateway now uses a different target port. Connections have to always use "soft links", similar to how we've already been doing that for kube gateways or server connections.You could use hard links and then simply update the target port of the existing connection, but then you run into the problem where when restoring app state you have two connections for exactly the same gateway params and no hard links to any existing gateway (because you just started the app). So clicking on the first connection would create a new doc, but after that clicking on the second connection would just activate the existing doc. So using soft links seems to be just the better way to manage this.