-
Notifications
You must be signed in to change notification settings - Fork 2k
Combine ClustersService logout functions
#59539
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
dfd2ead
Remove clusters immediately after a logout, move `useClusterLogout` t…
gzdunek 8343c3c
Review callsites to ensure cluster is properly checked before being a…
gzdunek 0d0455b
Revert "Review callsites to ensure cluster is properly checked before…
gzdunek 16c8968
Switch to removing the cluster at the end of logout sequence
gzdunek 1d9df1e
Lint
gzdunek 8700e4d
Move `logoutWithCleanup` to `ui/ClusterLogout`
gzdunek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 69 additions & 0 deletions
69
web/packages/teleterm/src/ui/ClusterLogout/logoutWithCleanup.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /** | ||
| * Teleport | ||
| * Copyright (C) 2025 Gravitational, Inc. | ||
| * | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU Affero General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU Affero General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Affero General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| import Logger from 'teleterm/logger'; | ||
| import { IAppContext } from 'teleterm/ui/types'; | ||
| import { RootClusterUri, routing } from 'teleterm/ui/uri'; | ||
|
|
||
| /** Disposes cluster-related resources and then logs out. */ | ||
| export async function logoutWithCleanup( | ||
| ctx: IAppContext, | ||
| clusterUri: RootClusterUri | ||
| ): Promise<void> { | ||
| const logger = new Logger('logoutWithCleanup'); | ||
| // This function checks for updates, do not wait for it. | ||
| ctx.mainProcessClient | ||
| .maybeRemoveAppUpdatesManagingCluster(clusterUri) | ||
| .catch(err => { | ||
| logger.error('Failed to remove managing cluster', err); | ||
| }); | ||
|
|
||
| if (ctx.workspacesService.getRootClusterUri() === clusterUri) { | ||
| const [firstConnectedWorkspace] = ctx.workspacesService | ||
| .getConnectedWorkspacesClustersUri() | ||
| .filter(c => c !== clusterUri); | ||
| if (firstConnectedWorkspace) { | ||
| await ctx.workspacesService.setActiveWorkspace(firstConnectedWorkspace); | ||
| } else { | ||
| await ctx.workspacesService.setActiveWorkspace(null); | ||
| } | ||
| } | ||
|
|
||
| // Remove connections first, they depend both on the cluster and the workspace. | ||
| ctx.connectionTracker.removeItemsBelongingToRootCluster(clusterUri); | ||
| // Remove the workspace next, because it depends on the cluster. | ||
| ctx.workspacesService.removeWorkspace(clusterUri); | ||
|
|
||
| // If there are active ssh connections to the agent, killing it will take a few seconds. To work | ||
| // around this, kill the agent only after removing the workspace. Removing the workspace closes | ||
| // ssh tabs, so it should terminate connections to the cluster from the app. | ||
| await ctx.connectMyComputerService.killAgentAndRemoveData(clusterUri); | ||
|
|
||
| await ctx.clustersService.removeClusterGateways(clusterUri); | ||
|
|
||
| const { | ||
| params: { rootClusterId }, | ||
| } = routing.parseClusterUri(clusterUri); | ||
| await ctx.mainProcessClient.removeKubeConfig({ | ||
| relativePath: rootClusterId, | ||
| isDirectory: true, | ||
| }); | ||
|
|
||
| // Remove the cluster, it does not depend on anything. | ||
| await ctx.clustersService.logout(clusterUri); | ||
| } |
74 changes: 0 additions & 74 deletions
74
web/packages/teleterm/src/ui/ClusterLogout/useClusterLogout.ts
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
As far as I remember that PR didn't introduce
logout&removeCluster. There are two separate RPCs for that because in the alpha version of Connect it was possible to log out of a cluster without removing it from the list of clusters in the app. Similar to how you can disconnect a gateway and only then remove it from the connections.Though I think I see what you mean in the context of #24978 splitting the logout sequence into first changing
connectedto false and actually removing the cluster from the state at the very end.I understand this becomes a larger concern when ~/.tsh sharing gets implemented, right? Because at the moment I don't think there are many opportunities to trigger
ClustersService.syncRootClustersAndCatchErrorsbeyond the app start, but looking at its callsites what you described is technically possible.That was surprising to me because if I had to bet I wouldn't have said that this is the case. 😅 I think in my head I've always assumed that we've had this sweet little invariant where the existence of a workspace at least implies that a root cluster is available.
I don't know, I'm not entirely opposed to this change, it just feels like a big departure from something I've always assumed was invariant, so I do feel a bit uneasy about it.
Perhaps we should document functions returning
ClusterfromClustersServiceto note that they might return no cluster?Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, I meant the methods in
ClustersService, not the tshd RPCs. Before that PR, the logout sequence started from the logout in tshd and removing theClustersServicestate. In the PR, we switched to removing the state at the very end.Tbh, even the fix that we added could be done easier. In the comment for
ClustersService.logoutwe said:We could as well explicitly filter out that cluster when looking for the next connected workspace in
useCluserLogout:)This change isn't strictly necessary for sharing ~/.tsh, but having a single method helps make the logic a bit cleaner.
I assume that in the ideal world, it would work like that:
It still could be four steps (as we have it today), where step 2 only calls tshd and sets
.connected = false, and a separate step 4 actually removes the cluster, but that's a tighter coupling between the renderer and main process than seems necessary.One alternative is switching steps 2 and 3, so the cluster is removed at the very end. That way, it would be removed after the workspace.
However, this still doesn’t address the other issue:
ClustersService.syncRootClustersAndCatchErrorcan be triggered beyond just the app start, which could cause a mismatch between workspaces and clusters.So maybe it’s cleaner to have the null checks, unless we can guarantee that these stores are always in sync (or alternatively, prevent this function from being called beyond app initialization).
But hmm, now that I think of it, maybe it actually has more sense to switch the steps? So the renderer first needs to remove the workspace and other dependencies and then we attempt to logout in tsh and remove the cluster (and we forget about
ClustersService.syncRootClustersAndCatchError).Maybe it wasn't the majority, but we did have 17 places with null checks and something around that I added in this PR.
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.
After discussing it through DMs with Rafał, we decided to switch the steps and perform the logout at the end of the logout sequence.
It makes more sense this way, since a cluster can exist without a workspace, but not the other way around.
If the logout in tshd fails, the app will remain usable. The cluster will still appear in the profile selector, allowing the user to retry the logout or open a new workspace for it.
To address the types issues, we should pass a cluster through the workspace context, so that we won't need all these null checks.
When it comes to
ClustersService.syncRootClustersAndCatchError, it should be called only once, before creating the workspaces. I left a TODO item to fix the one incorrect usage.