From 8eadde8da538d7886f2c70f2935740c5356a798d Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Tue, 1 Oct 2024 19:09:27 -0400 Subject: [PATCH 1/6] ENG-2740 Use elapsed time for SSH retry timeout Use elapsed time to determine the SSH retry timeout instead of the number of retries. --- .vscode/launch.json | 3 +++ src/plugins/aws/ssh.ts | 9 ++++----- src/plugins/google/ssh.ts | 9 ++++----- src/plugins/ssh/index.ts | 23 +++++++---------------- src/types/ssh.ts | 2 +- 5 files changed, 19 insertions(+), 27 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 24042dc..9037cee 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -12,6 +12,9 @@ "/**" ], "program": "${workspaceFolder}/src/index.ts", + // "args": [ "login", "698375260981"], + "args": [ "ssh", "dev-instance-fabian-joya", "--sudo"], + // "args": [ "scp", "test.txt", "dev-instance-fabian-joya:test.txt", "--", "-v" ], "preLaunchTask": "tsc: build - tsconfig.json", "outFiles": [ "${workspaceFolder}/**/*.js" diff --git a/src/plugins/aws/ssh.ts b/src/plugins/aws/ssh.ts index 7c9a0e5..717a9e3 100644 --- a/src/plugins/aws/ssh.ts +++ b/src/plugins/aws/ssh.ts @@ -22,11 +22,10 @@ import { AwsSshRoleRequest, } from "./types"; -/** Maximum number of attempts to start an SSH session - * - * Each attempt consumes ~ 1 s. +/** + * It can take up to 1 minute for access to propagate on AWS, so set the time limit to 10 minutes. */ -const MAX_SSH_RETRIES = 30; +const TIME_LIMIT_MS = 10 * 60 * 1000; /** The name of the SessionManager port forwarding document. This document is managed by AWS. */ const START_SSH_SESSION_DOCUMENT_NAME = "AWS-StartSSHSession"; @@ -96,6 +95,6 @@ export const awsSshProvider: SshProvider< return undefined; }, preTestAccessPropagationArgs: () => undefined, - maxRetries: MAX_SSH_RETRIES, + timeLimit: TIME_LIMIT_MS, friendlyName: "AWS", }; diff --git a/src/plugins/google/ssh.ts b/src/plugins/google/ssh.ts index 0aa29d4..a3f356e 100644 --- a/src/plugins/google/ssh.ts +++ b/src/plugins/google/ssh.ts @@ -14,11 +14,10 @@ import { ensureGcpSshInstall } from "./install"; import { importSshKey } from "./ssh-key"; import { GcpSshPermissionSpec, GcpSshRequest } from "./types"; -/** Maximum number of attempts to start an SSH session - * - * The length of each attempt varies based on the type of error from a few seconds to < 1s +/** + * It typically takes < 1 minute for access to propagate on GCP, so set the time limit to 2 minutes. */ -const MAX_SSH_RETRIES = 120; +const TIME_LIMIT_MS = 2 * 60 * 1000; export const gcpSshProvider: SshProvider< GcpSshPermissionSpec, @@ -78,6 +77,6 @@ export const gcpSshProvider: SshProvider< } return undefined; }, - maxRetries: MAX_SSH_RETRIES, + timeLimit: TIME_LIMIT_MS, friendlyName: "Google Cloud", }; diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index 103267f..8547271 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -165,7 +165,7 @@ type SpawnSshNodeOptions = { credential?: AwsCredentials; command: string; args: string[]; - attemptsRemaining: number; + endTime: number; abortController?: AbortController; detached?: boolean; stdio: [StdioNull, StdioNull, StdioPipe]; @@ -185,14 +185,11 @@ async function spawnSshNode( return new Promise((resolve, reject) => { const provider = SSH_PROVIDERS[options.provider]; - const attemptsRemaining = options.attemptsRemaining; if (options.debug) { const gerund = options.isAccessPropagationPreTest ? "Pre-testing" : "Trying"; - print2( - `Waiting for access to propagate. ${gerund} SSH session... (remaining attempts: ${attemptsRemaining})` - ); + print2(`Waiting for access to propagate. ${gerund} SSH session...)`); } const child = spawnChildProcess( @@ -211,23 +208,17 @@ async function spawnSshNode( // In the case of ephemeral AccessDenied exceptions due to unpropagated // permissions, continually retry access until success if (!isAccessPropagated()) { - if (attemptsRemaining <= 0) { + if (options.endTime < Date.now()) { reject( - `Access did not propagate through ${provider.friendlyName} before max retry attempts were exceeded. Please contact support@p0.dev for assistance.` + `Access did not propagate through ${provider.friendlyName} in time. Please contact support@p0.dev for assistance.` ); return; } delay(RETRY_DELAY_MS) - .then(() => - spawnSshNode({ - ...options, - attemptsRemaining: attemptsRemaining - 1, - }) - ) + .then(() => spawnSshNode(options)) .then((code) => resolve(code)) .catch(reject); - return; } else if (isGoogleLoginException()) { reject(`Please login to Google Cloud CLI with 'gcloud auth login'`); @@ -387,7 +378,7 @@ const preTestAccessPropagationIfNeeded = async < stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - attemptsRemaining: sshProvider.maxRetries, + endTime: Date.now() + sshProvider.timeLimit, isAccessPropagationPreTest: true, }); } @@ -450,6 +441,6 @@ export const sshOrScp = async (args: { stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - attemptsRemaining: sshProvider.maxRetries, + endTime: Date.now() + sshProvider.timeLimit, }); }; diff --git a/src/types/ssh.ts b/src/types/ssh.ts index 738670f..758015e 100644 --- a/src/types/ssh.ts +++ b/src/types/ssh.ts @@ -66,7 +66,7 @@ export type SshProvider< preTestAccessPropagationArgs: ( cmdArgs: CommandArgs ) => CommandArgs | undefined; - maxRetries: number; + timeLimit: number; friendlyName: string; }; From b2bcf135835fa312042a9a838840cce0b4ec3469 Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Tue, 1 Oct 2024 19:13:55 -0400 Subject: [PATCH 2/6] Revert commit of .vscode/launch.json --- .vscode/launch.json | 3 --- 1 file changed, 3 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 9037cee..24042dc 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -12,9 +12,6 @@ "/**" ], "program": "${workspaceFolder}/src/index.ts", - // "args": [ "login", "698375260981"], - "args": [ "ssh", "dev-instance-fabian-joya", "--sudo"], - // "args": [ "scp", "test.txt", "dev-instance-fabian-joya:test.txt", "--", "-v" ], "preLaunchTask": "tsc: build - tsconfig.json", "outFiles": [ "${workspaceFolder}/**/*.js" From 1b645068210cb94c267a3c9e3c4ebbaf28f2ccd1 Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Tue, 1 Oct 2024 20:43:33 -0400 Subject: [PATCH 3/6] Address review comments. --- src/plugins/aws/ssh.ts | 8 +++----- src/plugins/google/ssh.ts | 8 +++----- src/plugins/ssh/index.ts | 11 ++++++++--- src/types/ssh.ts | 4 ++-- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/plugins/aws/ssh.ts b/src/plugins/aws/ssh.ts index 7c415ac..1168eaf 100644 --- a/src/plugins/aws/ssh.ts +++ b/src/plugins/aws/ssh.ts @@ -22,10 +22,8 @@ import { AwsSshRoleRequest, } from "./types"; -/** - * It can take up to 1 minute for access to propagate on AWS, so set the time limit to 10 minutes. - */ -const TIME_LIMIT_MS = 10 * 60 * 1000; +// It can take up to 8 minutes for access to propagate on AWS, so set the time limit to 10 minutes. +const PROPAGATION_TIMEOUT_LIMIT_MS = 10 * 60 * 1000; /** The name of the SessionManager port forwarding document. This document is managed by AWS. */ const START_SSH_SESSION_DOCUMENT_NAME = "AWS-StartSSHSession"; @@ -83,7 +81,7 @@ export const awsSshProvider: SshProvider< friendlyName: "AWS", - timeoutLimit: TIME_LIMIT_MS, + propagationTimeoutMs: PROPAGATION_TIMEOUT_LIMIT_MS, preTestAccessPropagationArgs: () => undefined, diff --git a/src/plugins/google/ssh.ts b/src/plugins/google/ssh.ts index 855b075..8b85666 100644 --- a/src/plugins/google/ssh.ts +++ b/src/plugins/google/ssh.ts @@ -14,10 +14,8 @@ import { ensureGcpSshInstall } from "./install"; import { importSshKey } from "./ssh-key"; import { GcpSshPermissionSpec, GcpSshRequest } from "./types"; -/** - * It typically takes < 1 minute for access to propagate on GCP, so set the time limit to 2 minutes. - */ -const TIME_LIMIT_MS = 2 * 60 * 1000; +// It typically takes < 1 minute for access to propagate on GCP, so set the time limit to 2 minutes. +const PROPAGATION_TIMEOUT_LIMIT_MS = 2 * 60 * 1000; /** * There are 7 cases of unprovisioned access in Google Cloud. @@ -73,7 +71,7 @@ export const gcpSshProvider: SshProvider< loginRequiredPattern: /You do not currently have an active account selected/, - timeoutLimit: TIME_LIMIT_MS, + propagationTimeoutMs: PROPAGATION_TIMEOUT_LIMIT_MS, preTestAccessPropagationArgs: (cmdArgs) => { if (isSudoCommand(cmdArgs)) { diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index 73ff3a0..49a2d81 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -131,7 +131,12 @@ async function spawnSshNode( const gerund = options.isAccessPropagationPreTest ? "Pre-testing" : "Trying"; - print2(`Waiting for access to propagate. ${gerund} SSH session...)`); + const remainingSeconds = ((options.endTime - Date.now()) / 1e3).toFixed( + 1 + ); + print2( + `Waiting for access to propagate. ${gerund} SSH session... (will wait up to ${remainingSeconds} seconds)` + ); } const child = spawnChildProcess( @@ -326,7 +331,7 @@ const preTestAccessPropagationIfNeeded = async < stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - endTime: Date.now() + sshProvider.timeoutLimit, + endTime: Date.now() + sshProvider.propagationTimeoutMs, isAccessPropagationPreTest: true, }); } @@ -389,6 +394,6 @@ export const sshOrScp = async (args: { stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - endTime: Date.now() + sshProvider.timeoutLimit, + endTime: Date.now() + sshProvider.propagationTimeoutMs, }); }; diff --git a/src/types/ssh.ts b/src/types/ssh.ts index 5e3ea37..e98b2c1 100644 --- a/src/types/ssh.ts +++ b/src/types/ssh.ts @@ -57,8 +57,8 @@ export type SshProvider< /** Regex match for error string indicating that CSP login is required */ loginRequiredPattern?: RegExp; - /** The timeout limit for this provider's CLI SSH command */ - timeoutLimit: number; + /** Amount of time, in ms, to wait between granting access and giving up on attempting an SSH connection */ + propagationTimeoutMs: number; /** Arguments for a pre-test command to verify access propagation prior * to actually logging in the user to the ssh session. From 9b74b7ebc64335bda5b23186223e560b77e63dd4 Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Thu, 3 Oct 2024 11:17:29 -0400 Subject: [PATCH 4/6] Update timeout for AWS. --- src/plugins/aws/ssh.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/plugins/aws/ssh.ts b/src/plugins/aws/ssh.ts index 38bc5dc..85cce00 100644 --- a/src/plugins/aws/ssh.ts +++ b/src/plugins/aws/ssh.ts @@ -22,8 +22,7 @@ import { AwsSshRoleRequest, } from "./types"; -// It can take up to 8 minutes for access to propagate on AWS, so set the time limit to 10 minutes. -const PROPAGATION_TIMEOUT_LIMIT_MS = 10 * 60 * 1000; +const PROPAGATION_TIMEOUT_LIMIT_MS = 30 * 1000; /** The name of the SessionManager port forwarding document. This document is managed by AWS. */ const START_SSH_SESSION_DOCUMENT_NAME = "AWS-StartSSHSession"; From 334cd1e69d0e1a781da97305665207608fe10bce Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Thu, 3 Oct 2024 11:25:04 -0400 Subject: [PATCH 5/6] Use one timeout for both pre-test & trying. --- src/plugins/ssh/index.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index 49a2d81..a4d04b7 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -53,6 +53,7 @@ const accessPropagationGuard = ( ) => { let isEphemeralAccessDeniedException = false; let isLoginException = false; + const beforeStart = Date.now(); child.stderr.on("data", (chunk) => { @@ -315,7 +316,8 @@ const preTestAccessPropagationIfNeeded = async < proxyCommand: string[], credential: P extends SshProvider ? C - : undefined + : undefined, + endTime: number ) => { const testCmdArgs = sshProvider.preTestAccessPropagationArgs(cmdArgs); // Pre-testing comes at a performance cost because we have to execute another ssh subprocess after @@ -331,7 +333,7 @@ const preTestAccessPropagationIfNeeded = async < stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - endTime: Date.now() + sshProvider.propagationTimeoutMs, + endTime: endTime, isAccessPropagationPreTest: true, }); } @@ -375,12 +377,15 @@ export const sshOrScp = async (args: { } } + const endTime = Date.now() + sshProvider.propagationTimeoutMs; + const exitCode = await preTestAccessPropagationIfNeeded( sshProvider, request, cmdArgs, proxyCommand, - credential + credential, + endTime ); if (exitCode && exitCode !== 0) { return exitCode; // Only exit if there was an error when pre-testing @@ -394,6 +399,6 @@ export const sshOrScp = async (args: { stdio: ["inherit", "inherit", "pipe"], debug: cmdArgs.debug, provider: request.type, - endTime: Date.now() + sshProvider.propagationTimeoutMs, + endTime: endTime, }); }; From d95ef7c7b37bb99a04e65dc895ae1d0c709cc1b0 Mon Sep 17 00:00:00 2001 From: Fabian Joya Date: Thu, 3 Oct 2024 11:53:06 -0400 Subject: [PATCH 6/6] Address review comments. --- src/plugins/ssh/index.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index 4207a6f..655eb73 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -54,8 +54,6 @@ const accessPropagationGuard = ( let isEphemeralAccessDeniedException = false; let isLoginException = false; - const beforeStart = Date.now(); - child.stderr.on("data", (chunk) => { const chunkString: string = chunk.toString("utf-8"); @@ -65,11 +63,7 @@ const accessPropagationGuard = ( chunkString.match(message.pattern) ); - if ( - match && - Date.now() <= - beforeStart + (match.validationWindowMs || DEFAULT_VALIDATION_WINDOW_MS) - ) { + if (match) { isEphemeralAccessDeniedException = true; }