-
Notifications
You must be signed in to change notification settings - Fork 47
fix(console): report an unusable remote working directory before minting (CHOO-1416) #373
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
Open
abeldantas
wants to merge
2
commits into
main
Choose a base branch
from
fix/remote-dir-preflight
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
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
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
120 changes: 120 additions & 0 deletions
120
console/apps/switch-console-desktop/src/main/core/agents/remote-dir.test.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,120 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { FileSystemError, FileSystemErrorCodes } from '@main/core/fs/types'; | ||
|
|
||
| const stat = vi.hoisted(() => vi.fn()); | ||
| const close = vi.hoisted(() => vi.fn()); | ||
| const constructedWith = vi.hoisted(() => [] as string[]); | ||
|
|
||
| vi.mock('@main/core/fs/impl/ssh-fs', () => ({ | ||
| SshFileSystem: class { | ||
| constructor(_proxy: unknown, base: string) { | ||
| constructedWith.push(base); | ||
| } | ||
| stat = stat; | ||
| close = close; | ||
| }, | ||
| })); | ||
| vi.mock('@main/core/locations/location-transport', () => ({ | ||
| sshConnectionIdForHost: (host: string) => `conn:${host}`, | ||
| })); | ||
| vi.mock('@main/core/ssh/connect/connect-agent-ssh', () => ({ | ||
| ensureSshConnected: vi.fn(async () => ({})), | ||
| })); | ||
|
|
||
| const { inspectRemoteDir } = await import('./remote-dir'); | ||
|
|
||
| /** Report `paths` as existing directories and everything else as absent. */ | ||
| function existingDirs(paths: string[]) { | ||
| stat.mockImplementation(async (path: string) => | ||
| paths.includes(path) ? { path, type: 'dir' } : null | ||
| ); | ||
| } | ||
|
|
||
| const REPO_DIR = '/home/ubuntu/switch-agents/internal-deployments'; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| constructedWith.length = 0; | ||
| }); | ||
|
|
||
| describe('inspectRemoteDir', () => { | ||
| it('reports an existing directory', async () => { | ||
| existingDirs([REPO_DIR]); | ||
|
|
||
| expect(await inspectRemoteDir('host', REPO_DIR)).toEqual({ | ||
| dir: REPO_DIR, | ||
| status: 'directory', | ||
| }); | ||
| }); | ||
|
|
||
| // Long-standing behaviour, and not something this ticket should take away: | ||
| // recursive mkdir may create the working directory itself, just not its | ||
| // ancestors, so a missing leaf under an existing parent needs no intervention. | ||
| it('reports a missing directory whose parent exists as creatable', async () => { | ||
| existingDirs(['/home/ubuntu/switch-agents']); | ||
|
|
||
| expect(await inspectRemoteDir('host', REPO_DIR)).toEqual({ | ||
| dir: REPO_DIR, | ||
| status: 'creatable', | ||
| }); | ||
| }); | ||
|
|
||
| // The ticket's repro: the parent is missing too, so the write cannot recover. | ||
| it('reports a missing directory whose parent is also missing', async () => { | ||
| existingDirs(['/home/ubuntu']); | ||
|
|
||
| expect(await inspectRemoteDir('host', REPO_DIR)).toEqual({ | ||
| dir: REPO_DIR, | ||
| status: 'missing', | ||
| }); | ||
| // Opened at the host root: an FS rooted at the missing directory could not | ||
| // stat its way out to look at the parent. | ||
| expect(constructedWith).toEqual(['/']); | ||
| }); | ||
|
|
||
| it('reports a path that is a file', async () => { | ||
| stat.mockImplementation(async (path: string) => | ||
| path === REPO_DIR ? { path, type: 'file' } : null | ||
| ); | ||
|
|
||
| expect(await inspectRemoteDir('host', REPO_DIR)).toEqual({ dir: REPO_DIR, status: 'file' }); | ||
| }); | ||
|
|
||
| it('refuses a directory whose parent is a file', async () => { | ||
| stat.mockImplementation(async (path: string) => | ||
| path === '/home/ubuntu/switch-agents' ? { path, type: 'file' } : null | ||
| ); | ||
|
|
||
| expect(await inspectRemoteDir('host', REPO_DIR)).toEqual({ dir: REPO_DIR, status: 'missing' }); | ||
| }); | ||
|
|
||
| // An unreadable path is not a missing one; saying so would send the user off | ||
| // to create a directory that is already there. | ||
| it('propagates a probe failure that is not absence', async () => { | ||
| stat.mockRejectedValue( | ||
| new FileSystemError('Permission denied: /home', FileSystemErrorCodes.PERMISSION_DENIED) | ||
| ); | ||
|
|
||
| await expect(inspectRemoteDir('host', REPO_DIR)).rejects.toThrow('Permission denied'); | ||
| }); | ||
|
|
||
| it('rejects a relative path rather than resolving it against the login dir', async () => { | ||
| await expect(inspectRemoteDir('host', 'switch-agents/repo')).rejects.toThrow( | ||
| 'must be an absolute path' | ||
| ); | ||
| expect(stat).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('normalises a trailing slash', async () => { | ||
| existingDirs([REPO_DIR]); | ||
|
|
||
| expect(await inspectRemoteDir('host', `${REPO_DIR}/`)).toMatchObject({ dir: REPO_DIR }); | ||
| }); | ||
|
|
||
| it('closes the SFTP channel even when the probe throws', async () => { | ||
| stat.mockRejectedValue(new Error('boom')); | ||
|
|
||
| await expect(inspectRemoteDir('host', REPO_DIR)).rejects.toThrow('boom'); | ||
| expect(close).toHaveBeenCalled(); | ||
| }); | ||
| }); |
48 changes: 48 additions & 0 deletions
48
console/apps/switch-console-desktop/src/main/core/agents/remote-dir.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,48 @@ | ||
| import { posix as pathPosix } from 'node:path'; | ||
| import { SshFileSystem } from '@main/core/fs/impl/ssh-fs'; | ||
| import { sshConnectionIdForHost } from '@main/core/locations/location-transport'; | ||
| import { ensureSshConnected } from '@main/core/ssh/connect/connect-agent-ssh'; | ||
| import type { RemoteDirInspection } from '@shared/core/remote-hosts/remote-dir'; | ||
|
|
||
| /** | ||
| * Inspect a prospective remote working directory on `sshHost` (CHOO-1416). | ||
| * | ||
| * Only two stats, because only two things decide the outcome: whether the | ||
| * directory is there, and — if not — whether its parent is. A missing | ||
| * directory under an existing parent is created by the first credentials | ||
| * write, as it always has been; a missing parent is not, because the | ||
| * working directory's own FS is rooted at the directory and its recursive | ||
| * mkdir stops there. | ||
| * | ||
| * The FS here is opened at the host root instead, since one rooted at a | ||
| * missing directory cannot stat its way out to look at the parent. | ||
| * | ||
| * `dir` must be absolute — a relative path would resolve against whatever | ||
| * directory the SSH session happens to start in, which is not a thing the user | ||
| * chose. | ||
| * | ||
| * A path that cannot be stat'd for any reason *other* than absence (permission | ||
| * denied, dead connection) propagates rather than being reported as `missing`. | ||
| * An unreadable path is not a missing one, and saying so would send the user | ||
| * off to fix the wrong problem. | ||
| */ | ||
| export async function inspectRemoteDir(sshHost: string, dir: string): Promise<RemoteDirInspection> { | ||
| if (!pathPosix.isAbsolute(dir)) { | ||
| throw new Error(`Remote working directory must be an absolute path: ${dir}`); | ||
| } | ||
| const normalized = pathPosix.normalize(dir).replace(/\/+$/, '') || '/'; | ||
|
|
||
| const proxy = await ensureSshConnected(sshConnectionIdForHost(sshHost), sshHost); | ||
| const fs = new SshFileSystem(proxy, '/'); | ||
| try { | ||
| const entry = await fs.stat(normalized); | ||
| if (entry) { | ||
| return { dir: normalized, status: entry.type === 'dir' ? 'directory' : 'file' }; | ||
| } | ||
|
|
||
| const parent = await fs.stat(pathPosix.dirname(normalized)); | ||
| return { dir: normalized, status: parent?.type === 'dir' ? 'creatable' : 'missing' }; | ||
| } finally { | ||
| fs.close(); | ||
| } | ||
| } |
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
39 changes: 39 additions & 0 deletions
39
console/apps/switch-console-desktop/src/shared/core/remote-hosts/remote-dir.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,39 @@ | ||
| /** | ||
| * Existence model for a prospective remote working directory (CHOO-1416). | ||
| * | ||
| * A remote agent's working directory is typed as free text, so it is the one | ||
| * input in the add-agent flow that can be wrong in a way nothing else catches: | ||
| * the SSH host is probed for reachability, the server is picked from a list, | ||
| * but the directory was only ever touched at write time — by which point an | ||
| * identity had already been minted on the gateway. | ||
| */ | ||
|
|
||
| /** What an inspection found at a remote path. */ | ||
| export type RemoteDirStatus = | ||
| /** Exists and is a directory. */ | ||
| | 'directory' | ||
| /** | ||
| * Does not exist, but its parent does, so the first credentials write creates | ||
| * it — which is what already happened before this check existed. Usable. | ||
| */ | ||
| | 'creatable' | ||
| /** Exists, but is a regular file. */ | ||
| | 'file' | ||
| /** | ||
| * Neither the directory nor its parent exists. This is the failing case: a | ||
|
abeldantas marked this conversation as resolved.
Outdated
|
||
| * working directory's FS is rooted at the directory itself, and its recursive | ||
| * mkdir will not create anything above that root. | ||
| */ | ||
| | 'missing'; | ||
|
|
||
| /** The result of inspecting a prospective remote working directory. */ | ||
| export type RemoteDirInspection = { | ||
| /** The absolute path inspected, as resolved on the host. */ | ||
| dir: string; | ||
| status: RemoteDirStatus; | ||
| }; | ||
|
|
||
| /** Whether an agent can be created in this directory. */ | ||
| export function isUsableRemoteDir(inspection: RemoteDirInspection): boolean { | ||
| return inspection.status === 'directory' || inspection.status === 'creatable'; | ||
| } | ||
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.
Uh oh!
There was an error while loading. Please reload this page.