Allow changing terminal shell in Teleport Connect by gzdunek · Pull Request #45152 · gravitational/teleport

gzdunek · 2024-08-06T17:10:49Z

Closes #19392
Closes #20186

I have to admit that this PR has become quite big, although I think it is not that bad when reviewed commit-by-commit.
Here is a summary of how the shell switcher works:

Instead of RuntimeSettings.defaultShell we have now RuntimeSettings.availableShells and RuntimeSettings.defaultOsShellId. The shells are taken from shell.ts, where we calculate them for Unix and Windows.

The Unix shells are read from /etc/shells.
The list of Windows shells is hardcoded: I check what shells from the list: powershell.exe, pwsh.exe, cmd.exe and wsl.exe are available. I didn't do anything sophisticated here, I simply check what is available in the Path.

The default shell is stored in the app config, the shell for terminal tab is kept in the document.
When a new terminal tab is opened, I check in useDocumentTerminal.ts what is the default shell and set its ID in that document (I don't set the shellId in DocumentsService because it would have to depend on ConfigService and currently it doesn't depend on anything).
After that, I pass the shellId to the buildPtyOptions where we resolve the full Shell object and get binPath. In theory, I could send that binPath from the renderer but I didn't want to blindly spawn anything it would send.
When the user changes the active shell, I recreate the terminal document with the new URI and shellId.

This is how it looks on Windows.

I know @ravicious that you weren't a fan of the shell switcher on non-Windows platforms, and because of that, I decided to condense both of them into submenus, so they take less space.
But also I believe that we should avoid adding platform-specific features, if possible. Such features are much more difficult to develop and test (you have to run a VM) and simply it is easy to forget that we actually have them :)

changelog: The terminal shell can now be changed in Teleport Connect by right-clicking on a terminal tab. This allows using WSL (wsl.exe) if it is installed. Also, the default shell on Windows has been changed to pwsh.exe (instead of powershell.exe).

…ve it to the shell path)

…indows

avatus · 2024-08-07T19:02:16Z

I wasn't able to see the shell switcher when right clicking (on macos). From your description it seems like it should work in non-windows OSes as well yes? Am I missing something to get it open

gzdunek · 2024-08-08T08:01:46Z

Strange, did you click on a tab title? It needs to be a terminal or kube session tab.

ravicious · 2024-08-08T14:11:00Z

+        friendlyName: 'Command Prompt (cmd.exe)',
+      },
+      {
+        id: 'wsl.exe',


With wsl.exe as the default shell, env vars are not set properly. Both the TELEPORT env vars as well as including tsh in Path. Connecting to a new kube cluster also doesn't properly show the welcome message and KUBECONFIG isn't set.

With wsl.exe as the default shell, env vars are not set properly.

Ah that's right, shame on me for not checking that. Fortunately, the fix seems to be easy, we need to set a special WSLENV variable. I only need to put all variables we want to set in it.

Connecting to a new kube cluster also doesn't properly show the welcome message

This is caused by ConPTY, I have a fix here.

I fixed the problem with passing env vars to WSL, the only thing that doesn't work is using tsh from Path.
However, it seems to me that it doesn't work in regular PowerShell too, I will look into that tomorrow.

This issue is a little weird and I'm not sure where is the problem exactly.
We prepend tsh.exe to the Path in

teleport/web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.ts

Line 246 in ffaf668

env[pathName] = [settings.binDir, env[pathName]]

I noticed that if I change Path to path for win32, everything works (tsh.exe is available in every shell). But why? On master we use Path and tsh is prepended correctly.

Btw, there was already confusion in regard to Path on Windows in node.js nodejs/node#20605.

Perhaps we should just switch to path on Windows? When I run node in a Windows terminal and type process.env, there's a path property in the returned object.

Did you check it on a packaged version of the app? Because I definitely did it only in the dev version which does not prepend anything to path.

Yeah, under WSL we need to execute tsh.exe https://stackoverflow.com/a/47663269.

But now I'm really confused, why doesn't it work for me when pathName is path? I see that I don't have /mnt/c/teleport/web/packages/teleterm/build/release/win-unpacked/resources/bin in the $PATH (same for any other shell).
Maybe something is broken on my machine? I will try on a different one.

I just remembered that someone posted their workaround for using tsh in WSL: #9849 (comment)

As for the Path issues, nothing comes to my mind other than adding console.log and checking what's up. I suspect it's on our side, not necessarily the system, unless they changed how env vars work as a part of regular system update, which I guess is not that likely.

Okay, I just tested this on a clean OS and it worked. I was able to use tsh/tsh.exe in powershell.exe. pwsh.exe, cmd.exe and tsh.exe in WSL.
So it seems that something was wrong with my previous machine, but I have no idea what :(
I think we can mark this issue as resolved.

I just remembered that someone posted their workaround for using tsh in WSL: #9849 (comment)

I'm not sure if I see value in replacing tsh.exe with tsh. I think the official convention is to use [app-name].exe when running Win32 apps from WSL:
https://learn.microsoft.com/en-us/windows/wsl/filesystems#run-windows-tools-from-linux

Also, the users can install tsh in WSL, I think renaming tsh.exe to tsh would lead to confusion.

As for the Path issues, nothing comes to my mind other than adding console.log and checking what's up. I suspect it's on our side, not necessarily the system, unless they changed how env vars work as a part of regular system update, which I guess is not that likely.

OH, I'VE GOT IT!
The problem was that I have for some reason the "user" and "os" PATH env var called differently:

This resulted in the PATH variable being called path, not Path in the process.env object. Node.js handles this by having its own setters and getters for PATH, so no matter what property you set, the single underlying value is updated.
Unfortunately, I missed that we merge many env vars by spreading the objects, so these setters/getters were lost.
As a result, in the env object I had both path and Path (and the value of Path was ignored).
To fix this problem, we only need to find a property with the correct name in the env object and update it.

ravicious · 2024-08-08T14:47:33Z

I haven't looked at the code that closely, mostly just left comments about UX.

… the custom shell

…nderer

# Conflicts: # web/packages/teleterm/src/mainProcess/mainProcess.ts # web/packages/teleterm/src/services/config/appConfigSchema.ts

…e from

ravicious · 2024-08-22T12:50:17Z

+
+/** A utility function that allows us to mock `process.env` in tests. */
+export function getNodeProcessEnv() {
+  return process.env;


I'm not sure how well this is going to work long term but let's try it. 😏 Usually a better option would be to pass process.env from above so that you can substitute it in tests with whatever you want.

I wanted to argue, but I can't really find any real benefit of it, compared to passing process.env from above lol

It's a minor thing but I changed it.

ravicious · 2024-08-22T12:53:52Z

+    // Add `shellId` before going further.
+    let docWithDefaultShell: types.DocumentTerminal;
+    if (
+      (doc.kind === 'doc.terminal_shell' || doc.kind === 'doc.gateway_kube') &&
+      !doc.shellId
+    ) {
+      docWithDefaultShell = {
+        ...doc,
+        shellId: ctx.configService.get('terminal.shell').value,
+      };
+      documentsService.update(doc.uri, docWithDefaultShell);
+    }


Is this to update any existing documents which do not have this field set? It'd have been better to do this in some kind of a migration when starting the app, but I think we've never got around to adding such stuff for app_state.json.

Could we add // DELETE IN 18.0.0?

Is this to update any existing documents which do not have this field set?

No, the purpose of it was to add a default shell to any new "shell" document (in DocumentsService we set shellId: '').
That shellId is needed later in useDocumentTerminal and in the context menu, to mark the correct option.
The problem was that DocumentsService depends only on some setters and getters from WorkspacesService. I wasn't sure about adding ConfigService there (which we would call in newTerminalDocument), so I came up with this late initialization.
But actually, after we added returning the resolved shell from buildPtyOptions (and updating the document with it), we can get rid of it. This should be probably even more correct (than updating the document with the default shell), because we don't actually know what shell we will get from buildPtyOptions, so we can update the document after we get it.

Ah, I was too fast with it.
I realized that relying only on the shell returned from createPtyProcess won't work if it throws an error. Then the shellId is remains empty and in the context menu the first option is checked (at least on macOS, looks like it by default selects the first option if nothing is selected).
Because of that, I decided to revert it.

Ideally, when createPtyProcess throws an error, we should catch it, wrap in a custom object, and return something like {error, shell}.
Unfortunately, I don't have time for it, so I'd stay with what we have now (I improved the comment).

…inal`

…om above

…mentTerminal`" This reverts commit 52f0485.

ravicious

Thanks for addressing all the feedback. This was a bunch of surprisingly tricky changes!

It might be worthwhile to keep #9849 in mind. I don't know what expectations people running WSL might have towards Connect, but it might turn out that we did not think of something that they're used to, etc.

gzdunek · 2024-08-23T09:46:35Z

It might be worthwhile to keep #9849 in mind. I don't know what expectations people running WSL might have towards Connect, but it might turn out that we did not think of something that they're used to, etc.

Agree!

public-teleport-github-review-bot · 2024-08-23T10:19:06Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v16	Failed

* Get available shells for the system and store them in `RuntimeSettings` * Add a config option for `terminal.shell` * Add `shellId` to terminal documents and pty commands * Pass the `shellId` from pty command to pty process options (and resolve it to the shell path) * Allow changing the active and default shell from the context menu * Show active shell in the tab name, drop "Terminal" from the name on Windows * Fix resolving shell env for `csh` * Do not read unix shells from `/etc/shells`, add a config property for the custom shell * Add a whitelist of config properties that can be modified from the renderer * Allow selecting custom shell * Return back the actually opened shell and creation status * Pass variables to WSL via WSLENV * Do not change the default shell when selecting a custom shell * Show profile switcher only when there is more than one shell to choose from * Show shell name in the "Default Shell" context menu as sublabel * Remove comment about sync setup of `setUpDeepLinks` * Replace `MainTabContextMenuOptions` type * Improve types in `subscribeToTabContextMenuEvent` * Use early return * Fix condition in the schema for `terminal.shell` * Expand the comment for `preventAutoPromiseResolveOnMenuClose` * Render available shells without an array * `openedShell` -> `shell` * Add tests for resolving shell * Remove `PtyOptions` * Preserve user defined WSLENV * Remove the check for `fs.access(customShellPath)` * Mock `process.env` instead of mutating it * Fix remaining `openedShell` -> `shell` * Fix `PATH` issue on Windows * Remove updating a document with the default shell in `useDocumentTerminal` * Add `canDocChangeShell` * Omit the default shell name on Linux too * Convert `buildPtyOptions` parameters to object, pass `process.env` from above * Revert "Remove updating a document with the default shell in `useDocumentTerminal`" This reverts commit 52f0485. * Improve comments * Throw when the document doesn't exist (cherry picked from commit 806dcda)

gzdunek added 7 commits August 6, 2024 17:12

Get available shells for the system and store them in RuntimeSettings

257c965

Add a config option for terminal.shell

e5fd3b7

Add shellId to terminal documents and pty commands

74fb5cf

Pass the shellId from pty command to pty process options (and resol…

431b0f7

…ve it to the shell path)

Allow changing the active and default shell from the context menu

1fbed56

Show active shell in the tab name, drop "Terminal" from the name on W…

74bdc2b

…indows

Fix resolving shell env for csh

baedfa8

gzdunek added the backport/branch/v16 label Aug 6, 2024

gzdunek requested review from avatus and ravicious August 6, 2024 17:10

github-actions Bot added size/md ui labels Aug 6, 2024

github-actions Bot requested review from rudream and ryanclark August 6, 2024 17:11

gzdunek removed request for rudream and ryanclark August 6, 2024 17:11

ravicious reviewed Aug 8, 2024

View reviewed changes

gzdunek added 7 commits August 13, 2024 16:16

Do not read unix shells from /etc/shells, add a config property for…

03d7b16

… the custom shell

Add a whitelist of config properties that can be modified from the re…

5e3e407

…nderer

Allow selecting custom shell

58b78f4

Return back the actually opened shell and creation status

563541f

Pass variables to WSL via WSLENV

ffaf668

Merge branch 'master' into gzdunek/shell-switcher

9b017fd

# Conflicts: # web/packages/teleterm/src/mainProcess/mainProcess.ts # web/packages/teleterm/src/services/config/appConfigSchema.ts

Do not change the default shell when selecting a custom shell

e767729

gzdunek requested a review from ravicious August 13, 2024 15:39

ravicious reviewed Aug 14, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/mainProcess/contextMenus/tabContextMenu.ts

Comment thread web/packages/teleterm/src/mainProcess/contextMenus/tabContextMenu.ts

Comment thread web/packages/teleterm/src/mainProcess/contextMenus/tabContextMenu.ts Outdated

Show profile switcher only when there is more than one shell to choos…

ac675c9

…e from

gzdunek added 5 commits August 22, 2024 12:28

Remove PtyOptions

9b5b4b2

Preserve user defined WSLENV

6cbd5ea

Remove the check for fs.access(customShellPath)

0efbc78

Mock process.env instead of mutating it

1f14c0f

Fix remaining openedShell -> shell

e38f2c4

ravicious reviewed Aug 22, 2024

View reviewed changes

gzdunek added 6 commits August 22, 2024 15:53

Fix PATH issue on Windows

3f042d1

Remove updating a document with the default shell in `useDocumentTerm…

52f0485

…inal`

Add canDocChangeShell

cc84c82

Omit the default shell name on Linux too

6c2af71

Convert buildPtyOptions parameters to object, pass process.env fr…

0ec349b

…om above

Revert "Remove updating a document with the default shell in `useDocu…

870fe43

…mentTerminal`" This reverts commit 52f0485.

gzdunek requested a review from ravicious August 23, 2024 07:23

ravicious approved these changes Aug 23, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.ts

Comment thread web/packages/teleterm/src/ui/services/workspacesService/documentsService/types.ts

Comment thread web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts

gzdunek added 2 commits August 23, 2024 11:39

Improve comments

d45ef0d

Throw when the document doesn't exist

ef3f622

gzdunek enabled auto-merge August 23, 2024 09:46

gzdunek added this pull request to the merge queue Aug 23, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 23, 2024

gzdunek added this pull request to the merge queue Aug 23, 2024

Merged via the queue into master with commit 806dcda Aug 23, 2024

gzdunek deleted the gzdunek/shell-switcher branch August 23, 2024 10:18

gzdunek mentioned this pull request Aug 23, 2024

[v16] Allow changing terminal shell in Teleport Connect #45734

Merged

gzdunek mentioned this pull request Sep 16, 2024

Restore the previous order of shell arguments in resolveShellEnv #46631

Merged

gzdunek mentioned this pull request Dec 18, 2024

Show kube cluster name at the beginning of kube tabs #50376

Merged

Conversation

gzdunek commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus commented Aug 7, 2024

Uh oh!

gzdunek commented Aug 8, 2024

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!

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!

Uh oh!

Uh oh!

ravicious commented Aug 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gzdunek commented Aug 23, 2024

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Aug 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gzdunek commented Aug 6, 2024 •

edited

Loading