diff --git a/fixtures/bin.bash b/fixtures/bin.bash new file mode 100755 index 00000000..3f8db7e6 --- /dev/null +++ b/fixtures/bin.bash @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +echo '$ECHO' diff --git a/fixtures/bin.old.bash b/fixtures/bin.old.bash new file mode 100755 index 00000000..f45bb6de --- /dev/null +++ b/fixtures/bin.old.bash @@ -0,0 +1,8 @@ +#!/usr/bin/env bash + +if [[ $* == *--output* ]] ; then + >&2 echo -n '$STDERR' + exit 1 +else + echo -n '$STDOUT' +fi diff --git a/src/cliManager.test.ts b/src/cliManager.test.ts new file mode 100644 index 00000000..b5d18f19 --- /dev/null +++ b/src/cliManager.test.ts @@ -0,0 +1,130 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" +import { beforeAll, describe, expect, it } from "vitest" +import * as cli from "./cliManager" + +describe("cliManager", () => { + const tmp = path.join(os.tmpdir(), "vscode-coder-tests") + + beforeAll(async () => { + // Clean up from previous tests, if any. + await fs.rm(tmp, { recursive: true, force: true }) + await fs.mkdir(tmp, { recursive: true }) + }) + + it("name", () => { + expect(cli.name().startsWith("coder-")).toBeTruthy() + }) + + it("stat", async () => { + const binPath = path.join(tmp, "stat") + expect(await cli.stat(binPath)).toBeUndefined() + + await fs.writeFile(binPath, "test") + expect((await cli.stat(binPath))?.size).toBe(4) + }) + + it("rm", async () => { + const binPath = path.join(tmp, "rm") + await cli.rm(binPath) + + await fs.writeFile(binPath, "test") + await cli.rm(binPath) + }) + + // TODO: CI only runs on Linux but we should run it on Windows too. + it("version", async () => { + const binPath = path.join(tmp, "version") + await expect(cli.version(binPath)).rejects.toThrow("ENOENT") + + const binTmpl = await fs.readFile(path.join(__dirname, "../fixtures/bin.bash"), "utf8") + await fs.writeFile(binPath, binTmpl.replace("$ECHO", "hello")) + await expect(cli.version(binPath)).rejects.toThrow("EACCES") + + await fs.chmod(binPath, "755") + await expect(cli.version(binPath)).rejects.toThrow("Unexpected token") + + await fs.writeFile(binPath, binTmpl.replace("$ECHO", "{}")) + await expect(cli.version(binPath)).rejects.toThrow("No version found in output") + + await fs.writeFile( + binPath, + binTmpl.replace( + "$ECHO", + JSON.stringify({ + version: "v0.0.0", + }), + ), + ) + expect(await cli.version(binPath)).toBe("v0.0.0") + + const oldTmpl = await fs.readFile(path.join(__dirname, "../fixtures/bin.old.bash"), "utf8") + const old = (stderr: string, stdout: string): string => { + return oldTmpl.replace("$STDERR", stderr).replace("$STDOUT", stdout) + } + + // Should fall back only if it says "unknown flag". + await fs.writeFile(binPath, old("foobar", "Coder v1.1.1")) + await expect(cli.version(binPath)).rejects.toThrow("foobar") + + await fs.writeFile(binPath, old("unknown flag: --output", "Coder v1.1.1")) + expect(await cli.version(binPath)).toBe("v1.1.1") + + // Should trim off the newline if necessary. + await fs.writeFile(binPath, old("unknown flag: --output\n", "Coder v1.1.1\n")) + expect(await cli.version(binPath)).toBe("v1.1.1") + + // Error with original error if it does not begin with "Coder". + await fs.writeFile(binPath, old("unknown flag: --output", "Unrelated")) + await expect(cli.version(binPath)).rejects.toThrow("unknown flag") + + // Error if no version. + await fs.writeFile(binPath, old("unknown flag: --output", "Coder")) + await expect(cli.version(binPath)).rejects.toThrow("No version found") + }) + + it("rmOld", async () => { + const binDir = path.join(tmp, "bins") + expect(await cli.rmOld(path.join(binDir, "bin1"))).toStrictEqual([]) + + await fs.mkdir(binDir, { recursive: true }) + await fs.writeFile(path.join(binDir, "bin.old-1"), "echo hello") + await fs.writeFile(path.join(binDir, "bin.old-2"), "echo hello") + await fs.writeFile(path.join(binDir, "bin.temp-1"), "echo hello") + await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello") + await fs.writeFile(path.join(binDir, "bin1"), "echo hello") + await fs.writeFile(path.join(binDir, "bin2"), "echo hello") + + expect(await cli.rmOld(path.join(binDir, "bin1"))).toStrictEqual([ + { + fileName: "bin.old-1", + error: undefined, + }, + { + fileName: "bin.old-2", + error: undefined, + }, + { + fileName: "bin.temp-1", + error: undefined, + }, + { + fileName: "bin.temp-2", + error: undefined, + }, + ]) + + expect(await fs.readdir(path.join(tmp, "bins"))).toStrictEqual(["bin1", "bin2"]) + }) + + it("ETag", async () => { + const binPath = path.join(tmp, "hash") + + await fs.writeFile(binPath, "foobar") + expect(await cli.eTag(binPath)).toBe("8843d7f92416211de9ebb963ff4ce28125932878") + + await fs.writeFile(binPath, "test") + expect(await cli.eTag(binPath)).toBe("a94a8fe5ccb19ba61c4c0873d391e987982fbbd3") + }) +}) diff --git a/src/cliManager.ts b/src/cliManager.ts new file mode 100644 index 00000000..f5bbc5f6 --- /dev/null +++ b/src/cliManager.ts @@ -0,0 +1,167 @@ +import { execFile, type ExecFileException } from "child_process" +import * as crypto from "crypto" +import { createReadStream, type Stats } from "fs" +import fs from "fs/promises" +import os from "os" +import path from "path" +import { promisify } from "util" + +/** + * Stat the path or undefined if the path does not exist. Throw if unable to + * stat for a reason other than the path not existing. + */ +export async function stat(binPath: string): Promise { + try { + return await fs.stat(binPath) + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return undefined + } + throw error + } +} + +/** + * Remove the path. Throw if unable to remove. + */ +export async function rm(binPath: string): Promise { + try { + await fs.rm(binPath, { force: true }) + } catch (error) { + // Just in case; we should never get an ENOENT because of force: true. + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + } +} + +// util.promisify types are dynamic so there is no concrete type we can import +// and we have to make our own. +type ExecException = ExecFileException & { stdout?: string; stderr?: string } + +/** + * Return the version from the binary. Throw if unable to execute the binary or + * find the version for any reason. + */ +export async function version(binPath: string): Promise { + let stdout: string + try { + const result = await promisify(execFile)(binPath, ["version", "--output", "json"]) + stdout = result.stdout + } catch (error) { + // It could be an old version without support for --output. + if ((error as ExecException)?.stderr?.includes("unknown flag: --output")) { + const result = await promisify(execFile)(binPath, ["version"]) + if (result.stdout?.startsWith("Coder")) { + const v = result.stdout.split(" ")[1]?.trim() + if (!v) { + throw new Error("No version found in output: ${result.stdout}") + } + return v + } + } + throw error + } + + const json = JSON.parse(stdout) + if (!json.version) { + throw new Error("No version found in output: ${stdout}") + } + return json.version +} + +export type RemovalResult = { fileName: string; error: unknown } + +/** + * Remove binaries in the same directory as the specified path that have a + * .old-* or .temp-* extension. Return a list of files and the errors trying to + * remove them, when applicable. + */ +export async function rmOld(binPath: string): Promise { + const binDir = path.dirname(binPath) + try { + const files = await fs.readdir(binDir) + const results: RemovalResult[] = [] + for (const file of files) { + const fileName = path.basename(file) + if (fileName.includes(".old-") || fileName.includes(".temp-")) { + try { + await fs.rm(path.join(binDir, file), { force: true }) + results.push({ fileName, error: undefined }) + } catch (error) { + results.push({ fileName, error }) + } + } + } + return results + } catch (error) { + // If the directory does not exist, there is nothing to remove. + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return [] + } + throw error + } +} + +/** + * Return the etag (sha1) of the path. Throw if unable to hash the file. + */ +export async function eTag(binPath: string): Promise { + const hash = crypto.createHash("sha1") + const stream = createReadStream(binPath) + return new Promise((resolve, reject) => { + stream.on("end", () => { + hash.end() + resolve(hash.digest("hex")) + }) + stream.on("error", (err) => { + reject(err) + }) + stream.on("data", (chunk) => { + hash.update(chunk) + }) + }) +} + +/** + * Return the binary name for the current platform. + */ +export function name(): string { + const os = goos() + const arch = goarch() + let binName = `coder-${os}-${arch}` + // Windows binaries have an exe suffix. + if (os === "windows") { + binName += ".exe" + } + return binName +} + +/** + * Returns the Go format for the current platform. + * Coder binaries are created in Go, so we conform to that name structure. + */ +export function goos(): string { + const platform = os.platform() + switch (platform) { + case "win32": + return "windows" + default: + return platform + } +} + +/** + * Return the Go format for the current architecture. + */ +export function goarch(): string { + const arch = os.arch() + switch (arch) { + case "arm": + return "armv7" + case "x64": + return "amd64" + default: + return arch + } +} diff --git a/src/remote.ts b/src/remote.ts index 8b0a7d20..4f504e79 100644 --- a/src/remote.ts +++ b/src/remote.ts @@ -465,7 +465,14 @@ export class Remote { // // If we didn't write to the SSH config file, connecting would fail with // "Host not found". - await this.updateSSHConfig(authorityParts[1], hasCoderLogs) + try { + await this.updateSSHConfig(authorityParts[1], hasCoderLogs) + } catch (error) { + // TODO: This is a bit weird, because even if we throw an error VS Code + // still tries to connect. Can we stop it? + this.storage.writeToCoderOutputChannel(`Failed to configure SSH: ${error}`) + throw error + } this.findSSHProcessID().then((pid) => { // Once the SSH process has spawned we can reset the timeout. @@ -587,9 +594,6 @@ export class Remote { binaryPath = await this.storage.fetchBinary() } } - if (!binaryPath) { - throw new Error("Failed to fetch the Coder binary!") - } const escape = (str: string): string => `"${str.replace(/"/g, '\\"')}"` diff --git a/src/storage.ts b/src/storage.ts index 70d30907..85cd34a3 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -1,16 +1,15 @@ import axios from "axios" -import { execFile } from "child_process" import { getBuildInfo } from "coder/site/src/api/api" import { Workspace } from "coder/site/src/api/typesGenerated" -import * as crypto from "crypto" -import { createReadStream, createWriteStream } from "fs" +import { createWriteStream } from "fs" import fs from "fs/promises" import { ensureDir } from "fs-extra" import { IncomingMessage } from "http" -import os from "os" import path from "path" import prettyBytes from "pretty-bytes" import * as vscode from "vscode" +import { errToStr } from "./api-helper" +import * as cli from "./cliManager" import { getHeaderCommand, getHeaders } from "./headers" // Maximium number of recent URLs to store. @@ -119,51 +118,70 @@ export class Storage { return path.join(upperDir, latestOutput[0], remoteSSH[0]) } - // fetchBinary returns the path to a Coder binary. - // The binary will be cached if a matching server version already exists. - public async fetchBinary(): Promise { - await this.cleanUpOldBinaries() + /** + * Download and return the path to a working binary. If there is already a + * working binary and it matches the server version, return that, skipping the + * download. Throw if unable to download a working binary. + */ + public async fetchBinary(): Promise { const baseURL = this.getURL() if (!baseURL) { throw new Error("Must be logged in!") } + this.output.appendLine(`Using deployment URL: ${baseURL}`) + // Get the build info to compare with the existing binary version, if any, + // and to log for debugging. const buildInfo = await getBuildInfo() - const binPath = this.binaryPath() - const exists = await this.checkBinaryExists(binPath) - const os = goos() - const arch = goarch() - let binName = `coder-${os}-${arch}` - // Windows binaries have an exe suffix! - if (goos() === "windows") { - binName += ".exe" - } - const controller = new AbortController() + this.output.appendLine(`Got server version: ${buildInfo.version}`) - if (exists) { - this.output.appendLine(`Found existing binary: ${binPath}`) - const valid = await this.checkBinaryValid(binPath) - if (!valid) { - const removed = await this.rmBinary(binPath) - if (!removed) { - vscode.window.showErrorMessage("Failed to remove existing binary!") - return undefined + // Check if there is an existing binary and whether it looks valid. If it + // is valid and matches the server, we can return early. + const binPath = this.binaryPath() + this.output.appendLine(`Using binary path: ${binPath}`) + const stat = await cli.stat(binPath) + if (stat === undefined) { + this.output.appendLine("No existing binary found, starting download") + } else { + this.output.appendLine(`Existing binary size is ${prettyBytes(stat.size)}`) + try { + const version = await cli.version(binPath) + this.output.appendLine(`Existing binary version is ${version}`) + // If we have the right version we can avoid the request entirely. + if (version === buildInfo.version) { + this.output.appendLine("Using existing binary since it matches the server version") + return binPath } + this.output.appendLine("Downloading since existing binary does not match the server version") + } catch (error) { + this.output.appendLine(`Unable to get version of existing binary: ${error}`) + this.output.appendLine("Downloading new binary instead") } } - let etag = "" - if (exists) { - etag = await this.getBinaryETag() - } + // Remove any left-over old or temporary binaries. + const removed = await cli.rmOld(binPath) + removed.forEach(({ fileName, error }) => { + if (error) { + this.output.appendLine(`Failed to remove ${fileName}: ${error}`) + } else { + this.output.appendLine(`Removed ${fileName}`) + } + }) + + // Figure out where to get the binary. + const binName = cli.name() const configSource = vscode.workspace.getConfiguration().get("coder.binarySource") const binSource = configSource && String(configSource).trim().length > 0 ? String(configSource) : "/bin/" + binName + this.output.appendLine(`Downloading binary from: ${binSource}`) - this.output.appendLine(`Using binName: ${binName}`) - this.output.appendLine(`Using binPath: ${binPath}`) - this.output.appendLine(`Using binSource: ${binSource}`) + // Ideally we already caught that this was the right version and returned + // early, but just in case set the ETag. + const etag = stat !== undefined ? await cli.eTag(binPath) : "" this.output.appendLine(`Using ETag: ${etag}`) + // Make the download request. + const controller = new AbortController() const resp = await axios.get(binSource, { signal: controller.signal, baseURL: baseURL, @@ -176,22 +194,29 @@ export class Storage { // Ignore all errors so we can catch a 404! validateStatus: () => true, }) - this.output.appendLine("Response status code: " + resp.status) + this.output.appendLine(`Got status code ${resp.status}`) switch (resp.status) { case 200: { - const contentLength = Number.parseInt(resp.headers["content-length"]) + const rawContentLength = resp.headers["content-length"] + const contentLength = Number.parseInt(rawContentLength) + if (Number.isNaN(contentLength)) { + this.output.appendLine(`Got invalid or missing content length: ${rawContentLength}`) + } else { + this.output.appendLine(`Got content length: ${prettyBytes(contentLength)}`) + } - // Ensure the binary directory exists! + // Download to a temporary file. await fs.mkdir(path.dirname(binPath), { recursive: true }) const tempFile = binPath + ".temp-" + Math.random().toString(36).substring(8) + // Track how many bytes were written. + let written = 0 + const completed = await vscode.window.withProgress( { location: vscode.ProgressLocation.Notification, - title: `Downloading the latest binary (${buildInfo.version} from ${axios.getUri( - resp.config, - )}) to ${binPath}`, + title: `Downloading ${buildInfo.version} from ${axios.getUri(resp.config)} to ${binPath}`, cancellable: true, }, async (progress, token) => { @@ -203,65 +228,81 @@ export class Storage { cancelled = true }) - let contentLengthPretty = "" - // Reverse proxies might not always send a content length! - if (!Number.isNaN(contentLength)) { - contentLengthPretty = " / " + prettyBytes(contentLength) - } + // Reverse proxies might not always send a content length. + const contentLengthPretty = Number.isNaN(contentLength) ? "unknown" : prettyBytes(contentLength) + // Pipe data received from the request to the temp file. const writeStream = createWriteStream(tempFile, { autoClose: true, mode: 0o755, }) - let written = 0 readStream.on("data", (buffer: Buffer) => { writeStream.write(buffer, () => { written += buffer.byteLength progress.report({ - message: `${prettyBytes(written)}${contentLengthPretty}`, - increment: (buffer.byteLength / contentLength) * 100, + message: `${prettyBytes(written)} / ${contentLengthPretty}`, + increment: Number.isNaN(contentLength) ? undefined : (buffer.byteLength / contentLength) * 100, }) }) }) - try { - await new Promise((resolve, reject) => { - readStream.on("error", (err) => { - reject(err) - }) - readStream.on("close", () => { - if (cancelled) { - return reject() - } - writeStream.close() - resolve() - }) + + // Wait for the stream to end or error. + return new Promise((resolve, reject) => { + writeStream.on("error", (error) => { + readStream.destroy() + reject(new Error(`Unable to download binary: ${errToStr(error, "no reason given")}`)) }) - return true - } catch (ex) { - return false - } + readStream.on("error", (error) => { + writeStream.close() + reject(new Error(`Unable to download binary: ${errToStr(error, "no reason given")}`)) + }) + readStream.on("close", () => { + writeStream.close() + if (cancelled) { + resolve(false) + } else { + resolve(true) + } + }) + }) }, ) + + // False means the user canceled, although in practice it appears we + // would not get this far because VS Code already throws on cancelation. if (!completed) { - return + this.output.appendLine("User aborted download") + throw new Error("User aborted download") } - this.output.appendLine(`Downloaded binary: ${binPath}`) - if (exists) { + + this.output.appendLine(`Downloaded ${prettyBytes(written)} bytes to ${path.basename(tempFile)}`) + + // Move the old binary to a backup location first, just in case. And, + // on Linux at least, you cannot write onto a binary that is in use so + // moving first works around that (delete would also work). + if (stat !== undefined) { const oldBinPath = binPath + ".old-" + Math.random().toString(36).substring(8) - await fs.rename(binPath, oldBinPath).catch(() => { - this.output.appendLine(`Warning: failed to rename ${binPath} to ${oldBinPath}`) - }) - await fs.rm(oldBinPath, { force: true }).catch((error) => { - this.output.appendLine(`Warning: failed to remove old binary: ${error}`) - }) + this.output.appendLine(`Moving existing binary to ${path.basename(oldBinPath)}`) + await fs.rename(binPath, oldBinPath) } + + // Then move the temporary binary into the right place. + this.output.appendLine(`Moving downloaded file to ${path.basename(binPath)}`) await fs.mkdir(path.dirname(binPath), { recursive: true }) await fs.rename(tempFile, binPath) + // For debugging, to see if the binary only partially downloaded. + const newStat = await cli.stat(binPath) + this.output.appendLine(`Downloaded binary size is ${prettyBytes(newStat?.size || 0)}`) + + // Make sure we can execute this new binary. + const version = await cli.version(binPath) + this.output.appendLine(`Downloaded binary version is ${version}`) + return binPath } case 304: { - this.output.appendLine(`Using cached binary: ${binPath}`) + this.output.appendLine("Using existing binary since server returned a 304") return binPath } case 404: { @@ -274,6 +315,8 @@ export class Storage { if (!value) { return } + const os = cli.goos() + const arch = cli.goarch() const params = new URLSearchParams({ title: `Support the \`${os}-${arch}\` platform`, body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`, @@ -281,7 +324,7 @@ export class Storage { const uri = vscode.Uri.parse(`https://github.com/coder/vscode-coder/issues/new?` + params.toString()) vscode.env.openExternal(uri) }) - return undefined + throw new Error("Platform not supported") } default: { vscode.window @@ -291,13 +334,13 @@ export class Storage { return } const params = new URLSearchParams({ - title: `Failed to download binary on \`${os}-${arch}\``, + title: `Failed to download binary on \`${cli.goos()}-${cli.goarch()}\``, body: `Received status code \`${resp.status}\` when downloading the binary.`, }) const uri = vscode.Uri.parse(`https://github.com/coder/vscode-coder/issues/new?` + params.toString()) vscode.env.openExternal(uri) }) - return undefined + throw new Error("Failed to download binary") } } } @@ -335,23 +378,6 @@ export class Storage { return path.join(this.globalStorageUri.fsPath, "url") } - public getBinaryETag(): Promise { - const hash = crypto.createHash("sha1") - const stream = createReadStream(this.binaryPath()) - return new Promise((resolve, reject) => { - stream.on("end", () => { - hash.end() - resolve(hash.digest("hex")) - }) - stream.on("error", (err) => { - reject(err) - }) - stream.on("data", (chunk) => { - hash.update(chunk) - }) - }) - } - public writeToCoderOutputChannel(message: string) { this.output.appendLine(message) // We don't want to focus on the output here, because the @@ -374,61 +400,8 @@ export class Storage { } } - private async cleanUpOldBinaries(): Promise { - const binPath = this.binaryPath() - const binDir = path.dirname(binPath) - await fs.mkdir(binDir, { recursive: true }) - const files = await fs.readdir(binDir) - for (const file of files) { - const fileName = path.basename(file) - if (fileName.includes(".old-")) { - try { - await fs.rm(path.join(binDir, file), { force: true }) - } catch (error) { - this.output.appendLine(`Warning: failed to remove ${fileName}. Error: ${error}`) - } - } - } - } - private binaryPath(): string { - const os = goos() - const arch = goarch() - let binPath = path.join(this.getBinaryCachePath(), `coder-${os}-${arch}`) - if (os === "windows") { - binPath += ".exe" - } - return binPath - } - - private async checkBinaryExists(binPath: string): Promise { - return await fs - .stat(binPath) - .then(() => true) - .catch(() => false) - } - - private async rmBinary(binPath: string): Promise { - return await fs - .rm(binPath, { force: true }) - .then(() => true) - .catch(() => false) - } - - private async checkBinaryValid(binPath: string): Promise { - return await new Promise((resolve) => { - try { - execFile(binPath, ["version"], (err) => { - if (err) { - this.output.appendLine("Check for binary corruption: " + err) - } - resolve(err === null) - }) - } catch (ex) { - this.output.appendLine("The cached binary cannot be executed: " + ex) - resolve(false) - } - }) + return path.join(this.getBinaryCachePath(), cli.name()) } private async updateSessionToken() { @@ -447,28 +420,3 @@ export class Storage { return getHeaders(url, getHeaderCommand(vscode.workspace.getConfiguration()), this) } } - -// goos returns the Go format for the current platform. -// Coder binaries are created in Go, so we conform to that name structure. -const goos = (): string => { - const platform = os.platform() - switch (platform) { - case "win32": - return "windows" - default: - return platform - } -} - -// goarch returns the Go format for the current architecture. -const goarch = (): string => { - const arch = os.arch() - switch (arch) { - case "arm": - return "armv7" - case "x64": - return "amd64" - default: - return arch - } -}