-
Notifications
You must be signed in to change notification settings - Fork 745
fix(sagemaker): improve SSH configuration error handling for SageMakare remote connections #8328
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ export const ConnectFromRemoteWorkspaceMessage = | |
| 'Unable to establish new remote connection. Your last active VS Code window is connected to a remote workspace. To open a new SageMaker Studio connection, select your local VS Code window and try again.' | ||
|
|
||
| export const InstanceTypeError = 'InstanceTypeError' | ||
| export const SshConfigError = 'SshConfigError' | ||
|
|
||
| export const InstanceTypeMinimum = 'ml.t3.large' | ||
|
|
||
|
|
@@ -46,6 +47,10 @@ export const InstanceTypeNotSelectedMessage = (spaceName: string) => { | |
| export const RemoteAccessRequiredMessage = | ||
| 'This space requires remote access to be enabled.\nWould you like to restart the space and connect?\nAny unsaved work will be lost.' | ||
|
|
||
| export const SshConfigErrorMessage = () => { | ||
| return `Unable to connect. ~/.ssh/config contains invalid SSH configuration. Fix the errors to continue.` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we write the "~" character in other error messages? Windows doesn't use it so we shouldn't explicitly write this. We can possibly show it if we have confirmed it is applicable to the user's environment. Alternatively, we display the exact path, though it may be a bit too long. |
||
| } | ||
|
|
||
| export const SmusDeeplinkSessionExpiredError = { | ||
| title: 'Session Disconnected', | ||
| message: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { ToolkitError } from '../../shared/errors' | |
| import { SagemakerSpaceNode } from './explorer/sagemakerSpaceNode' | ||
| import { sleep } from '../../shared/utilities/timeoutUtils' | ||
| import { SagemakerUnifiedStudioSpaceNode } from '../../sagemakerunifiedstudio/explorer/nodes/sageMakerUnifiedStudioSpaceNode' | ||
| import { SshConfigError, SshConfigErrorMessage } from './constants' | ||
|
|
||
| const logger = getLogger('sagemaker') | ||
|
|
||
|
|
@@ -96,6 +97,23 @@ export async function prepareDevEnvConnection( | |
| if (config.isErr()) { | ||
| const err = config.err() | ||
| logger.error(`sagemaker: failed to add ssh config section: ${err.message}`) | ||
|
|
||
| if (err instanceof ToolkitError && err.code === 'SshCheckFailed') { | ||
| const sshConfigPath = path.join(os.homedir(), '.ssh', 'config') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use the |
||
|
|
||
| // Extracting line number from SSH error message is the best-effort. | ||
| // SSH error formats are not standardized and may vary across implementations. | ||
| await vscode.window | ||
| .showErrorMessage(SshConfigErrorMessage(), { modal: true, detail: err.message }, 'Open SSH Config') | ||
| .then((resp) => { | ||
| if (resp === 'Open SSH Config') { | ||
| void vscode.window.showTextDocument(vscode.Uri.file(sshConfigPath)) | ||
| } | ||
| }) | ||
|
|
||
| throw new ToolkitError('SSH configuration error', { code: SshConfigError, cancelled: true }) | ||
| } | ||
|
|
||
| throw err | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,7 +85,18 @@ export class SshConfig { | |
| protected async matchSshSection() { | ||
| const result = await this.checkSshOnHost() | ||
| if (result.exitCode !== 0) { | ||
| return Result.err(result.error ?? new Error(`ssh check against host failed: ${result.exitCode}`)) | ||
| // Format stderr error message | ||
| let errorMessage = result.stderr?.trim() || `ssh check against host failed: ${result.exitCode}` | ||
| const sshConfigPath = getSshConfigPath() | ||
| errorMessage = errorMessage.replace(new RegExp(`${sshConfigPath}:\\s*`, 'g'), '').trim() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could benefit from a comment explaining the purpose. |
||
|
|
||
| if (result.error) { | ||
| // System level error | ||
| return Result.err(ToolkitError.chain(result.error, errorMessage)) | ||
| } | ||
|
|
||
| // SSH ran but returned error exit code | ||
| return Result.err(new ToolkitError(errorMessage, { code: 'SshCheckFailed' })) | ||
| } | ||
| const matches = result.stdout.match(this.proxyCommandRegExp) | ||
| return Result.ok(matches?.[0]) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "type": "Bug Fix", | ||
| "description": "SageMaker: Enhanced SSH configuration error handling to show user-friendly modal dialogs with line numbers and an \"Open SSH Config\" button for quick fixes." | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are some suggestions regarding change log entries:
Here's an alternative wording (AI-assisted):
|
||
| } | ||
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.
When I read this method name, I generate many questions in my head.
Part of the problem is that this method is making a decision (made clear by the word should), which realistically can't be made without context that isn't provided to the method. An example of such context is the comment which says "Suppress errors that were already shown to user". That is something this method cannot possibly know.
To improve, we can simply rename the method (e.g.,
isUserConfigurationError,isUserCancelledError), or use error metadata (isUserCancelledError()is an existing helper method in the repository for this use case).