Fixup containers ssh UX papercuts (#11578)

* Add helper function to show or hide the cursor

* Watch cli directory in wrangler "dev" script

* containers: ssh UX improvements

Fix a few papercuts with the new container ssh command:

* Provide single-letter aliases for common SSH options (`-F` to specify
  a config file, `-o` to specify options, `-i` to set the private key)
* Allow `-o` to be used multiple times for setting multiple options
* Show the cursor when the SSH process starts. The library that
  Wrangler uses to draw the interactive UI (clack) hides the cursor, so
  it is not visible when the SSH shell starts. We need to manually make
  it visible.
* Hard code some specific SSH options (ControlMaster, ControlPersist,
  UserKnownHostsFile, and StrictHostKeyChecking). These don't make sense
  to use with the way our SSH implementation works

Author: gpanders

.changeset/pretty-teams-refuse.md

@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+Fixup UX papercuts in containers SSH

.changeset/shaky-women-help.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/cli": minor
+---
+
+Add showCursor helper function to show or hide the cursor in the terminal

packages/cli/cursor.ts

@@ -0,0 +1,13 @@
+import process from "node:process";
+
+export function showCursor(show: boolean, stream = process.stderr) {
+	if (!stream.isTTY) {
+		return;
+	}
+
+	if (show) {
+		stream.write("\x1b[?25h");
+	} else {
+		stream.write("\x1b[?25l");
+	}
+}

packages/cli/index.ts

@@ -277,3 +277,4 @@ export const error = (
 };
 
 export { checkMacOSVersion } from "./check-macos-version";
+export { showCursor } from "./cursor";

packages/wrangler/package.json

@@ -55,7 +55,7 @@
 		"check:lint": "eslint . --max-warnings=0",
 		"check:type": "tsc -p ./tsconfig.json && tsc -p ./templates/tsconfig.json",
 		"clean": "rimraf wrangler-dist miniflare-dist emitted-types",
-		"dev": "pnpm run clean && concurrently -c black,blue --kill-others-on-fail false \"pnpm tsup --watch src --watch ../containers-shared/src\" \"pnpm run check:type --watch --preserveWatchOutput\"",
+		"dev": "pnpm run clean && concurrently -c black,blue --kill-others-on-fail false \"pnpm tsup --watch src --watch ../containers-shared/src --watch ../cli\" \"pnpm run check:type --watch --preserveWatchOutput\"",
 		"generate-json-schema": "node -r esbuild-register scripts/generate-json-schema.ts",
 		"start": "pnpm run build && cross-env NODE_OPTIONS=--enable-source-maps ./bin/wrangler.js",
 		"test": "dotenv -- pnpm run assert-git-version && dotenv -- vitest",

packages/wrangler/src/__tests__/containers/ssh.test.ts

@@ -27,7 +27,7 @@ describe("containers ssh", () => {
 			"wrangler containers ssh ID
 
 			POSITIONALS
-			  ID  id of the container instance  [string] [required]
+			  ID  ID of the container instance  [string] [required]
 
 			GLOBAL FLAGS
 			  -c, --config    Path to Wrangler configuration file  [string]
@@ -41,11 +41,11 @@ describe("containers ssh", () => {
 			      --cipher         Sets \`ssh -c\`: Select the cipher specification for encrypting the session  [string]
 			      --log-file       Sets \`ssh -E\`: Append debug logs to log_file instead of standard error  [string]
 			      --escape-char    Sets \`ssh -e\`: Set the escape character for sessions with a pty (default: ‘~’)  [string]
-			      --config-file    Sets \`ssh -F\`: Specify an alternative per-user ssh configuration file  [string]
-			      --pkcs11         \`Sets \`ssh -I\`: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication  [string]
-			      --identity-file  Sets \`ssh -i\`: Select a file from which the identity (private key) for public key authentication is read  [string]
+			  -F, --config-file    Sets \`ssh -F\`: Specify an alternative per-user ssh configuration file  [string]
+			      --pkcs11         Sets \`ssh -I\`: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication  [string]
+			  -i, --identity-file  Sets \`ssh -i\`: Select a file from which the identity (private key) for public key authentication is read  [string]
 			      --mac-spec       Sets \`ssh -m\`: A comma-separated list of MAC (message authentication code) algorithms, specified in order of preference  [string]
-			      --option         Sets \`ssh -o\`: Can be used to give options in the format used in the ssh configuration file  [string]
+			  -o, --option         Sets \`ssh -o\`: Set options in the format used in the ssh configuration file. May be repeated  [string]
 			      --tag            Sets \`ssh -P\`: Specify a tag name that may be used to select configuration in ssh_config  [string]"
 		`);
 	});
@@ -79,8 +79,7 @@ describe("containers ssh", () => {
 			runWrangler(`containers ssh ${instanceId}`)
 		).rejects.toThrowErrorMatchingInlineSnapshot(
 			`
-			[Error: There has been an error verifying SSH access.
-			something happened]
+			[Error: Error verifying SSH access: something happened]
 		`
 		);
 	});
@@ -109,7 +108,8 @@ describe("containers ssh", () => {
 		const mockWebSocket = new MockWebSocketServer(wsUrl);
 		await expect(runWrangler(`containers ssh ${instanceId}`)).rejects
 			.toMatchInlineSnapshot(`
-			[Error: ssh exited unsuccessfully. Is the container running?]
+			[Error: SSH exited unsuccessfully. Is the container running?
+			NOTE: SSH does not automatically wake a container or count as activity to keep a container alive]
 		`);
 
 		// We got a connection

packages/wrangler/src/containers/ssh.ts

@@ -1,5 +1,7 @@
 import { spawn } from "node:child_process";
 import { createServer } from "node:net";
+import { showCursor } from "@cloudflare/cli";
+import { bold } from "@cloudflare/cli/colors";
 import { ApiError, DeploymentsService } from "@cloudflare/containers-shared";
 import { UserError } from "@cloudflare/workers-utils";
 import { WebSocket } from "ws";
@@ -17,7 +19,7 @@ export function sshYargs(args: CommonYargsArgv) {
 	return (
 		args
 			.positional("ID", {
-				describe: "id of the container instance",
+				describe: "ID of the container instance",
 				type: "string",
 				demandOption: true,
 			})
@@ -38,16 +40,18 @@ export function sshYargs(args: CommonYargsArgv) {
 				type: "string",
 			})
 			.option("config-file", {
+				alias: "F",
 				describe:
 					"Sets `ssh -F`: Specify an alternative per-user ssh configuration file",
 				type: "string",
 			})
 			.option("pkcs11", {
 				describe:
-					"`Sets `ssh -I`: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication",
+					"Sets `ssh -I`: Specify the PKCS#11 shared library ssh should use to communicate with a PKCS#11 token providing keys for user authentication",
 				type: "string",
 			})
 			.option("identity-file", {
+				alias: "i",
 				describe:
 					"Sets `ssh -i`: Select a file from which the identity (private key) for public key authentication is read",
 				type: "string",
@@ -58,8 +62,9 @@ export function sshYargs(args: CommonYargsArgv) {
 				type: "string",
 			})
 			.option("option", {
+				alias: "o",
 				describe:
-					"Sets `ssh -o`: Can be used to give options in the format used in the ssh configuration file",
+					"Sets `ssh -o`: Set options in the format used in the ssh configuration file. May be repeated",
 				type: "string",
 			})
 			.option("tag", {
@@ -87,10 +92,18 @@ export async function sshCommand(
 		);
 	} catch (e) {
 		if (e instanceof ApiError) {
-			throw new Error(
-				"There has been an error verifying SSH access.\n" + e.body.error
-			);
+			if (e.status === 404) {
+				throw new Error(`Instance ${sshArgs.ID} not found`);
+			}
+
+			let msg = `Error verifying SSH access`;
+			if (e.body.error !== undefined) {
+				msg += `: ${e.body.error}`;
+			}
+
+			throw new Error(msg);
 		}
+
 		throw e;
 	}
 
@@ -105,17 +118,25 @@ export async function sshCommand(
 
 	await verifySshInstalled("ssh");
 
+	// Get arguments passed to the SSH command itself. yargs includes
+	// "containers" and "ssh" as the first two elements of the array, which
+	// we don't want, so we don't include those.
+	const [, , ...rest] = sshArgs._;
+
 	const child = spawn(
 		"ssh",
 		[
 			"cloudchamber@127.0.0.1",
 			"-p",
 			`${proxyAddress.port}`,
 			...buildSshArgs(sshArgs),
+			"--",
+			...rest.map((v) => v.toString()),
 		],
 		{
 			stdio: ["inherit", "inherit", "inherit"],
 			detached: true,
+			signal: proxyController.signal,
 		}
 	);
 
@@ -132,21 +153,31 @@ export async function sshCommand(
 				resolve(undefined);
 			} else {
 				reject(
-					new Error(`ssh exited unsuccessfully. Is the container running?`)
+					new Error(
+						[
+							"SSH exited unsuccessfully. Is the container running?",
+							`${bold("NOTE:")} SSH does not automatically wake a container or count as activity to keep a container alive`,
+						].join("\n")
+					)
 				);
 			}
 		});
 	});
+
+	// Ensure the cursor is visible.
+	showCursor(true);
+
 	await childKilled;
 
+	// Hide the cursor again.
+	showCursor(false);
+
 	proxyController.abort();
 }
 
 export function verifySshInstalled(sshPath: string): Promise<undefined> {
 	return new Promise<undefined>((resolve, reject) => {
-		const child = spawn(sshPath, ["-V"], {
-			detached: true,
-		});
+		const child = spawn(sshPath, ["-V"]);
 
 		let errorHandled = false;
 		child.on("close", (code) => {
@@ -161,7 +192,7 @@ export function verifySshInstalled(sshPath: string): Promise<undefined> {
 		child.on("error", (err) => {
 			if (!errorHandled) {
 				errorHandled = true;
-				reject(new Error(`verifying ssh installation failed: ${err.message}`));
+				reject(new Error(`Verifying SSH installation failed: ${err.message}`));
 			}
 		});
 	});
@@ -195,7 +226,7 @@ export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server {
 		ws.binaryType = "arraybuffer";
 
 		ws.addEventListener("error", (err) => {
-			logger.error("Web socket error:", err.error);
+			logger.error("Web socket error:", err.message);
 			inbound.end();
 			proxy.close();
 		});
@@ -228,7 +259,26 @@ export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server {
 function buildSshArgs(
 	sshArgs: StrictYargsOptionsToInterface<typeof sshYargs>
 ): string[] {
-	const flags: string[] = [];
+	const flags = [
+		// Never use a control socket.
+		"-o",
+		"ControlMaster=no",
+		"-o",
+		"ControlPersist=no",
+		// Disable writing host keys to user known hosts file.
+		"-o",
+		"UserKnownHostsFile=/dev/null",
+		// Do not perform strict host key checking: we use the same IP
+		// address to connect to every container, all of which can have
+		// separate host keys.
+		"-o",
+		"StrictHostKeyChecking=no",
+	];
+
+	// Hide warnings from SSH unless debug logging is enabled
+	if (process.env.WRANGLER_LOG !== "debug") {
+		flags.push("-o", "LogLevel=ERROR");
+	}
 
 	if (sshArgs.cipher !== undefined) {
 		flags.push("-c", sshArgs.cipher);
@@ -259,7 +309,10 @@ function buildSshArgs(
 	}
 
 	if (sshArgs.option !== undefined) {
-		flags.push("-o", sshArgs.option);
+		const options = Array.isArray(sshArgs.option)
+			? sshArgs.option
+			: [sshArgs.option];
+		options.forEach((o) => flags.push("-o", o));
 	}
 
 	if (sshArgs.tag !== undefined) {