Connect: make logout function idempotent by gzdunek · Pull Request #60553 · gravitational/teleport

gzdunek · 2025-10-24T10:53:23Z

Contributes to #25806
Closes https://github.com/gravitational/teleport.e/issues/859

The upcoming profile watcher will need to respond to the tsh logout command. However, this command does not clean up resources that live in the tsh daemon, such as the headless watcher and cluster clients.

To clean up the cluster resources from the Electron app when we detect a logout, we can reuse the existing Logout RPC. Unfortunately, currently it returns early with an error if the profile has already been removed. To address this, we can make the method idempotent: it will ignore "not found" errors, allowing the full cleanup sequence to run regardless of the profile state.

I also combined ClusterLogout and ClusterRemove into a single RPC, since they already had some overlap. To support the different tsh logout modes, I added a RemoveProfile option.

In Connect, logging out always removes both the certificates and the YAML profile. In tsh, there are two modes:

tsh logout - removes everything.
tsh logout --proxy=... --user=... - removes only the certificates for the specified user; the YAML profile remains untouched.

If we only had a single ClusterLogout mode (removing both certificates and the profile), we would remove the profile after detecting the tsh logout --proxy=... --user=... command.

How will we detect both modes?

The profile watcher will call ListRootClusters whenever it detects a filesystem change and compare the new response with the previous one:

tsh logout — the new response will no longer contain the cluster at all.
tsh logout --proxy=... --user=... — we can check the Cluster.LoggedInUser.name field. If it was non-empty previously but is now empty, we know that the user logged out.

I also removed some cleanup functions from the renderer; they can now be invoked directly in the main process.
The main process should not rely on the renderer to clean up its own resources.

gzdunek · 2025-10-24T11:13:58Z

-        }
-      ) => {
-        const { kubeConfigsDir } = this.settings;
-        const filePath = path.join(kubeConfigsDir, options.relativePath);


Turns out this function has been broken for a long time.
The kubeconfigs are kepts in path.join(kubeConfigsDir, 'keys', profileName) not in path.join(kubeConfigsDir, profileName).

I'm wondering if we need this function at all, a kubeconfig is removed when the kube gateway closes, so here we only remove a tree of empty directories. OTOH it doesn't hurt to clean it up.

This must've been a leftover from times when we were migrating to kube gateways. Looking at git blame seems to confirm this, it was added with the initial kube support and hasn't been touched in three years. ;)

It should've been removed in #55000.

Yeah, this function was added before the kube gateways.
I will remove it.

…ster` to main process The main process should not depend on the renderer to clean up its own resources.

ravicious

Looks good, but I want to try it out and I don't have time for this today unfortunately.

ravicious · 2025-10-27T16:47:20Z

+// ClusterLogout logs the user out of the cluster and cleans up associated resources.
+// Optionally removes the profile.
+// This operation is idempotent and can be safely invoked multiple times.
+func (s *Service) ClusterLogout(ctx context.Context, uri uri.ResourceURI, removeProfile bool) error {


I don't like using bools as positional args, but doing anything else in Go when you just have one extra arg is such a PITA. 😭

Agree, I don't like it too :(

ravicious · 2025-10-27T16:52:34Z

 }

-// RemoveCluster removes cluster
-func (s *Service) RemoveCluster(ctx context.Context, uri string) error {


I was thinking that at some point we could make the profile selector behave like the connection list. In the connection list, the first click disconnects a gateway and only the subsequent one totally removes it.

Similarly, in the profile selector the first click could only log you out without removing the profile and the subsequent one would completely get rid of the cluster.

This would solve the annoying issue where after logging out you need to provide the cluster address again.

This still should be possible though since ClusterLogout now has removeProfile arg. Off the top of my head I can't tell if there are going to be some issues with leftover username in the profile or whatever. Something to think about in the future.

ravicious · 2025-10-27T16:55:08Z

-        }
-      ) => {
-        const { kubeConfigsDir } = this.settings;
-        const filePath = path.join(kubeConfigsDir, options.relativePath);


This must've been a leftover from times when we were migrating to kube gateways. Looking at git blame seems to confirm this, it was added with the initial kube support and hasn't been touched in three years. ;)

It should've been removed in #55000.

ravicious · 2025-10-27T16:59:43Z

+        .catch(error => {
+          this.logger.error('Failed to remove managing cluster', error);
+        });
+      await this.removeKubeConfig(args.clusterUri);


Maybe we shouldn't have this at all? If the app has been working fine all this time without it then why change it. ;f

This directory isn't even managed by Connect, AFAIK tsh is responsible for it. tshd gets it from here:

teleport/lib/teleterm/gateway/kube.go

Lines 59 to 61 in 9885e0e

// KubeconfigPath returns the kubeconfig path that can be used for clients to

// connect to the local proxy.

func (k *kube) KubeconfigPath() string {

* Remove `ClusterRemove` RPC, make logging out idempotent * Move calling `removeKubeConfig` and `maybeRemoveAppUpdatesManagingCluster` to main process The main process should not depend on the renderer to clean up its own resources. * Remove cleaning up kube dir * Lint

* Remove `ClusterRemove` RPC, make logging out idempotent * Move calling `removeKubeConfig` and `maybeRemoveAppUpdatesManagingCluster` to main process The main process should not depend on the renderer to clean up its own resources. * Remove cleaning up kube dir * Lint (cherry picked from commit 2d1bc7b)

* Combine `ClustersService` logout functions (#59539) * Remove clusters immediately after a logout, move `useClusterLogout` to `AppContext` * Review callsites to ensure cluster is properly checked before being accessed * Revert "Review callsites to ensure cluster is properly checked before being accessed" This reverts commit 8343c3c. * Switch to removing the cluster at the end of logout sequence * Lint * Move `logoutWithCleanup` to `ui/ClusterLogout` (cherry picked from commit de6b4ed) * Enable sending messages from main to renderer with acknowledgments (#59642) * Create awaitable sender * Review comments * Fix test and lint (cherry picked from commit 5dc76fe) * Move cluster state to main process (#59643) * Create `ClusterStore` that manages cluster state * Fix tests that mocked tshd directly * Remove IPC to notify the main process about cluster list changes * Load immer plugins in `MainProcess` * Improve comments * Refactor `useSender` * Get rid of unnecessary Map and try/catch around send * Get rid of `MainProcess.create` * Do not return early `c.proxyHost` is falsy * Add more context to test * Add missing logout handler in main process * Fix applying patches * Adjust `subscribeToClusterStore` to updated `startAwaitableSenderListener` * Crash window when sending state update fails * Extract WebContents navigation handlers and add tests for opening links * Improve error message * Initialize `ClusterStore` synchronously * Convert `lazyTshdClient` field to `getTshdClient` function, add docs * Remove unused eslint directive (cherry picked from commit a41d021) * Connect: make logout function idempotent (#60553) * Remove `ClusterRemove` RPC, make logging out idempotent * Move calling `removeKubeConfig` and `maybeRemoveAppUpdatesManagingCluster` to main process The main process should not depend on the renderer to clean up its own resources. * Remove cleaning up kube dir * Lint (cherry picked from commit 2d1bc7b) * Connect: add profile watcher (#60622) * Add profile watcher * Move `makeClusterWithOnlyProfileProperties` to `profileWatcher.ts`, improve test * Handle watched directory removal * Improve comments * Make tests faster, pass abort signal everywhere * Improve docs * Make `removing tsh directory does not break watcher` easier to understand * Make test dir per test * Improve timing in tests * Add a limit of how many events can be emitted by `fs.watch` (to break the endless stream of events on Windows when watched dir is removed), go into the polling mode only when it's expected that the watched dir was removed * Use `expect().rejects.toThrow` correctly * Deflake 'max file system events count is restricted' * Replace `makeClusterWithOnlyProfileProperties` with `mergeClusterProfileWithDetails`, move it back to `cluster.ts` * Attempt to fix tests * Clarify comment (cherry picked from commit d4e6f19) * Initialize tshdClients in MainProcess constructor (#61044) (cherry picked from commit c7a4233) * Connect: react to tsh actions by watching tsh dir (#60884) * Add `ClusterLifecycleManager` * Register handlers for adding, removing and logging out from cluster * Provide `rootCluster` in `useWorkspaceContext` The handlers in the profile watcher will proceed with updating the cluster store, even if the renderer handlers returned errors. This check protects us from a runtime error if the renderer fails to remove the workspace. * Improve docs * Move processing queue to listener * Make `will-` operations always interrupt main process actions * Improve error messages * Do not remove managing cluster when **only** logging out The app updater displays all clusters, not just those the user is logged into. * Revert "Provide `rootCluster` in `useWorkspaceContext`" This reverts commit cf76d2b. * Rename `logoutWithCleanup` to `cleanUpBeforeLogout` * Do not pass `AbortSignal` to `this.mainProcessClient.syncRootClusters` * Lint * Fix types issues * Do not stack watcher notifications (cherry picked from commit 5fa8249) * Connect: close cluster clients when profile changes (#61090) * Include expiration time in `LoggedInUser` This will allow the profile watcher to detect when the user relogged. * Display expiration time in UI * Add `ClearStaleClusterClients` RPC * Implement `ClearStaleClusterClients` * Clear stale clients when profile changes * Improve session expiration component * Move refresh button back to top * `ClearCachedStaleClientsForRoot` -> `ClearStaleCachedClientsForRoot` * `unchanged` -> `stale` * Make "closing stale clients" a subtest * Add `clientcache` test * Remove `getProfile` error wrapping * Improve comment * Convert story to controls (cherry picked from commit 6615e42) * Gracefully handle missing `current-profile` and respect `TELEPORT_PROXY` in `tsh status` (#61295) * Respect `TELEPORT_PROXY` env var in `tsh status` * Enable listing profiles if there is no active profile * Add test * Define `err` within the block where it's actually used * Handle missing current profile in `tsh logout` * Make check more explicit * Revert mistakenly commited change (cherry picked from commit 95bec3a) * Connect: switch tsh home directory to ~/.tsh (#61352) * Switch tsh home directory to ~/.tsh * Migrate old tsh home to new location, disallow updating fields outside the `state` key in app_state.json from the renderer process * Show banner about migrated tsh home * `promoteMigratedTshHome` -> `showTshHomeMigrationBanner` * `MigratedTshHomeBanner` -> `TshHomeMigrationBanner` * 'Profiles are' -> 'Profiles are now', remove unnecessary space * Fix assigning colors for new workspaces * Improve logs (cherry picked from commit 54b5f6c) * Connect: refresh resources when access changes and add tests for `ClusterLifecycleManager` (#61479) * Detect when user's access changes * Refresh resources in UI when `did-change-access` is received * Add tests for `ClusterLifecycleManager` * Add better docs for ClusterLifecycleEvent * Test assuming requests too * Improve test names (cherry picked from commit 4b00520) * Set up deep links as soon as possible (#61668) (cherry picked from commit 0b5ab6b) * Serialize IPC errors (#61665) * Serialize all enumerable error fields * Add wrappers around `ipcMain.handle` and `ipcRenderer.invoke` * Fix `Method Error.prototype.toString called on incompatible receiver undefined` * Improve docs * Lint (cherry picked from commit a1f2ae0) * Fix unrecoverable ssh cert errors in tsh/Connect (#61322) * Initialize default Username/HostLogin only in tsh * Move `Username()` from `api.go` to `tsh.go` * Remove wrong `Profile.SiteName` default * Remove resetting `SiteName` Not sure why it was needed. Perhaps to clear the default that we just removed? But even if add the default back and remove this fix, everything works. * Gracefully handle missing SSH/TLS certs * Remove unused `TeleportClient.LoadKeyForClusterWithReissue` * Revert "Move `Username()` from `api.go` to `tsh.go`" This reverts commit f7ff0ff. * Revert "Initialize default Username/HostLogin only in tsh" This reverts commit ed38bab. * When any of SSH/TLS cert is missing, return partial profile * Only log non-nil errors * Revert "Remove wrong `Profile.SiteName` default" * Revert "Remove resetting `SiteName`" This reverts commit f54ab3f. * Set `SiteName` when adding cluster * Improve comments * Add test * Fix test * Add myself to TODO * Add test for logging out with missing SSH cert * Lint (cherry picked from commit cd3c8f8) * Connect: update docs for sharing ~/.tsh directory (#61467) * Update docs for sharing ~/.tsh directory * Review comments * Lint (cherry picked from commit 19533bf) --------- Co-authored-by: ravicious <rafal.cieslak@goteleport.com>

gzdunek requested review from avatus and ravicious October 24, 2025 10:53

gzdunek added the no-changelog Indicates that a PR does not require a changelog entry label Oct 24, 2025

github-actions bot added size/sm ui labels Oct 24, 2025

github-actions bot requested review from kimlisa and rudream October 24, 2025 10:54

gzdunek removed request for kimlisa and rudream October 24, 2025 10:55

gzdunek force-pushed the gzdunek/make-logout-idempotent branch from 71af394 to b9e97d3 Compare October 24, 2025 11:09

gzdunek commented Oct 24, 2025

View reviewed changes

gzdunek added 2 commits October 24, 2025 14:19

Remove ClusterRemove RPC, make logging out idempotent

56cd13b

Move calling removeKubeConfig and `maybeRemoveAppUpdatesManagingClu…

07b0f02

…ster` to main process The main process should not depend on the renderer to clean up its own resources.

gzdunek force-pushed the gzdunek/make-logout-idempotent branch from b9e97d3 to 07b0f02 Compare October 24, 2025 12:19

avatus approved these changes Oct 24, 2025

View reviewed changes

ravicious reviewed Oct 27, 2025

View reviewed changes

ravicious self-requested a review October 27, 2025 17:04

Remove cleaning up kube dir

da2f0c7

ravicious approved these changes Oct 28, 2025

View reviewed changes

Lint

fcf47b4

gzdunek enabled auto-merge October 28, 2025 12:24

gzdunek added this pull request to the merge queue Oct 28, 2025

Merged via the queue into master with commit 2d1bc7b Oct 28, 2025
45 of 46 checks passed

gzdunek deleted the gzdunek/make-logout-idempotent branch October 28, 2025 13:39

gzdunek mentioned this pull request Nov 28, 2025

[v18] Share TELEPORT_HOME between Connect and tsh #61846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect: make logout function idempotent#60553

Connect: make logout function idempotent#60553
gzdunek merged 4 commits intomasterfrom
gzdunek/make-logout-idempotent

gzdunek commented Oct 24, 2025

Uh oh!

gzdunek Oct 24, 2025

Uh oh!

ravicious Oct 27, 2025

Uh oh!

gzdunek Oct 28, 2025

Uh oh!

ravicious left a comment

Uh oh!

ravicious Oct 27, 2025

Uh oh!

gzdunek Oct 28, 2025

Uh oh!

ravicious Oct 27, 2025

Uh oh!

ravicious Oct 27, 2025

Uh oh!

ravicious Oct 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// KubeconfigPath returns the kubeconfig path that can be used for clients to
	// connect to the local proxy.
	func (k *kube) KubeconfigPath() string {

Conversation

gzdunek commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants