Skip to content

Commit 52a99d1

Browse files
code-ashercode-asher
committed
Improve fetching Coder binary
- Add more logging (the main goal of this commit). - Break out parts specific to the binary and add tests for those parts. - Check the version before making the request. - Avoid deleting the old binary right away, mostly to make it possible to debug why the plugin thought it was invalid. It will be cleaned up on the next download, so at most we have one. - Clean up left-over temporary binaries too. - Validate downloaded binary. - Catch error on writing (for example if we are downloading the binary to a read-only location).
1 parent 6407437 commit 52a99d1

File tree

6 files changed

+437
-176
lines changed

6 files changed

+437
-176
lines changed

‎fixtures/bin.bash‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
#!/usr/bin/env bash
2+
3+
echo '$ECHO'

‎fixtures/bin.old.bash‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#!/usr/bin/env bash
2+
3+
if [[ $* == *--output* ]] ; then
4+
>&2 echo -n '$STDERR'
5+
exit 1
6+
else
7+
echo -n '$STDOUT'
8+
fi

‎src/cliManager.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import fs from "fs/promises"
2+
import os from "os"
3+
import path from "path"
4+
import { beforeAll, describe, expect, it } from "vitest"
5+
import * as cli from "./cliManager"
6+
7+
describe("cliManager", () => {
8+
const tmp = path.join(os.tmpdir(), "vscode-coder-tests")
9+
10+
beforeAll(async () => {
11+
// Clean up from previous tests, if any.
12+
await fs.rm(tmp, { recursive: true, force: true })
13+
await fs.mkdir(tmp, { recursive: true })
14+
})
15+
16+
it("name", () => {
17+
expect(cli.name().startsWith("coder-")).toBeTruthy()
18+
})
19+
20+
it("stat", async () => {
21+
const binPath = path.join(tmp, "stat")
22+
expect(await cli.stat(binPath)).toBeUndefined()
23+
24+
await fs.writeFile(binPath, "test")
25+
expect((await cli.stat(binPath))?.size).toBe(4)
26+
})
27+
28+
it("rm", async () => {
29+
const binPath = path.join(tmp, "rm")
30+
await cli.rm(binPath)
31+
32+
await fs.writeFile(binPath, "test")
33+
await cli.rm(binPath)
34+
})
35+
36+
// TODO: CI only runs on Linux but we should run it on Windows too.
37+
it("version", async () => {
38+
const binPath = path.join(tmp, "version")
39+
await expect(cli.version(binPath)).rejects.toThrow("ENOENT")
40+
41+
const binTmpl = await fs.readFile(path.join(__dirname, "../fixtures/bin.bash"), "utf8")
42+
await fs.writeFile(binPath, binTmpl.replace("$ECHO", "hello"))
43+
await expect(cli.version(binPath)).rejects.toThrow("EACCES")
44+
45+
await fs.chmod(binPath, "755")
46+
await expect(cli.version(binPath)).rejects.toThrow("Unexpected token")
47+
48+
await fs.writeFile(binPath, binTmpl.replace("$ECHO", "{}"))
49+
await expect(cli.version(binPath)).rejects.toThrow("No version found in output")
50+
51+
await fs.writeFile(
52+
binPath,
53+
binTmpl.replace(
54+
"$ECHO",
55+
JSON.stringify({
56+
version: "v0.0.0",
57+
}),
58+
),
59+
)
60+
expect(await cli.version(binPath)).toBe("v0.0.0")
61+
62+
const oldTmpl = await fs.readFile(path.join(__dirname, "../fixtures/bin.old.bash"), "utf8")
63+
const old = (stderr: string, stdout: string): string => {
64+
return oldTmpl.replace("$STDERR", stderr).replace("$STDOUT", stdout)
65+
}
66+
67+
// Should fall back only if it says "unknown flag".
68+
await fs.writeFile(binPath, old("foobar", "Coder v1.1.1"))
69+
await expect(cli.version(binPath)).rejects.toThrow("foobar")
70+
71+
await fs.writeFile(binPath, old("unknown flag: --output", "Coder v1.1.1"))
72+
expect(await cli.version(binPath)).toBe("v1.1.1")
73+
74+
// Should trim off the newline if necessary.
75+
await fs.writeFile(binPath, old("unknown flag: --output\n", "Coder v1.1.1\n"))
76+
expect(await cli.version(binPath)).toBe("v1.1.1")
77+
78+
// Error with original error if it does not begin with "Coder".
79+
await fs.writeFile(binPath, old("unknown flag: --output", "Unrelated"))
80+
await expect(cli.version(binPath)).rejects.toThrow("unknown flag")
81+
82+
// Error if no version.
83+
await fs.writeFile(binPath, old("unknown flag: --output", "Coder"))
84+
await expect(cli.version(binPath)).rejects.toThrow("No version found")
85+
})
86+
87+
it("rmOld", async () => {
88+
const binDir = path.join(tmp, "bins")
89+
expect(await cli.rmOld(path.join(binDir, "bin1"))).toStrictEqual([])
90+
91+
await fs.mkdir(binDir, { recursive: true })
92+
await fs.writeFile(path.join(binDir, "bin.old-1"), "echo hello")
93+
await fs.writeFile(path.join(binDir, "bin.old-2"), "echo hello")
94+
await fs.writeFile(path.join(binDir, "bin.temp-1"), "echo hello")
95+
await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello")
96+
await fs.writeFile(path.join(binDir, "bin1"), "echo hello")
97+
await fs.writeFile(path.join(binDir, "bin2"), "echo hello")
98+
99+
expect(await cli.rmOld(path.join(binDir, "bin1"))).toStrictEqual([
100+
{
101+
fileName: "bin.old-1",
102+
error: undefined,
103+
},
104+
{
105+
fileName: "bin.old-2",
106+
error: undefined,
107+
},
108+
{
109+
fileName: "bin.temp-1",
110+
error: undefined,
111+
},
112+
{
113+
fileName: "bin.temp-2",
114+
error: undefined,
115+
},
116+
])
117+
118+
expect(await fs.readdir(path.join(tmp, "bins"))).toStrictEqual(["bin1", "bin2"])
119+
})
120+
121+
it("ETag", async () => {
122+
const binPath = path.join(tmp, "hash")
123+
124+
await fs.writeFile(binPath, "foobar")
125+
expect(await cli.eTag(binPath)).toBe("8843d7f92416211de9ebb963ff4ce28125932878")
126+
127+
await fs.writeFile(binPath, "test")
128+
expect(await cli.eTag(binPath)).toBe("a94a8fe5ccb19ba61c4c0873d391e987982fbbd3")
129+
})
130+
})

‎src/cliManager.ts‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
import { execFile, type ExecFileException } from "child_process"
2+
import * as crypto from "crypto"
3+
import { createReadStream, type Stats } from "fs"
4+
import fs from "fs/promises"
5+
import os from "os"
6+
import path from "path"
7+
import { promisify } from "util"
8+
9+
/**
10+
* Stat the path or undefined if the path does not exist. Throw if unable to
11+
* stat for a reason other than the path not existing.
12+
*/
13+
export async function stat(binPath: string): Promise<Stats | undefined> {
14+
try {
15+
return await fs.stat(binPath)
16+
} catch (error) {
17+
if ((error as NodeJS.ErrnoException)?.code === "ENOENT") {
18+
return undefined
19+
}
20+
throw error
21+
}
22+
}
23+
24+
/**
25+
* Remove the path. Throw if unable to remove.
26+
*/
27+
export async function rm(binPath: string): Promise<void> {
28+
try {
29+
await fs.rm(binPath, { force: true })
30+
} catch (error) {
31+
// Just in case; we should never get an ENOENT because of force: true.
32+
if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") {
33+
throw error
34+
}
35+
}
36+
}
37+
38+
// util.promisify types are dynamic so there is no concrete type we can import
39+
// and we have to make our own.
40+
type ExecException = ExecFileException & { stdout?: string; stderr?: string }
41+
42+
/**
43+
* Return the version from the binary. Throw if unable to execute the binary or
44+
* find the version for any reason.
45+
*/
46+
export async function version(binPath: string): Promise<string> {
47+
let stdout: string
48+
try {
49+
const result = await promisify(execFile)(binPath, ["version", "--output", "json"])
50+
stdout = result.stdout
51+
} catch (error) {
52+
// It could be an old version without support for --output.
53+
if ((error as ExecException)?.stderr?.includes("unknown flag: --output")) {
54+
const result = await promisify(execFile)(binPath, ["version"])
55+
if (result.stdout?.startsWith("Coder")) {
56+
const v = result.stdout.split(" ")[1]?.trim()
57+
if (!v) {
58+
throw new Error("No version found in output: ${result.stdout}")
59+
}
60+
return v
61+
}
62+
}
63+
throw error
64+
}
65+
66+
const json = JSON.parse(stdout)
67+
if (!json.version) {
68+
throw new Error("No version found in output: ${stdout}")
69+
}
70+
return json.version
71+
}
72+
73+
export type RemovalResult = { fileName: string; error: unknown }
74+
75+
/**
76+
* Remove binaries in the same directory as the specified path that have a
77+
* .old-* or .temp-* extension. Return a list of files and the errors trying to
78+
* remove them, when applicable.
79+
*/
80+
export async function rmOld(binPath: string): Promise<RemovalResult[]> {
81+
const binDir = path.dirname(binPath)
82+
try {
83+
const files = await fs.readdir(binDir)
84+
const results: RemovalResult[] = []
85+
for (const file of files) {
86+
const fileName = path.basename(file)
87+
if (fileName.includes(".old-") || fileName.includes(".temp-")) {
88+
try {
89+
await fs.rm(path.join(binDir, file), { force: true })
90+
results.push({ fileName, error: undefined })
91+
} catch (error) {
92+
results.push({ fileName, error })
93+
}
94+
}
95+
}
96+
return results
97+
} catch (error) {
98+
// If the directory does not exist, there is nothing to remove.
99+
if ((error as NodeJS.ErrnoException)?.code === "ENOENT") {
100+
return []
101+
}
102+
throw error
103+
}
104+
}
105+
106+
/**
107+
* Return the etag (sha1) of the path. Throw if unable to hash the file.
108+
*/
109+
export async function eTag(binPath: string): Promise<string> {
110+
const hash = crypto.createHash("sha1")
111+
const stream = createReadStream(binPath)
112+
return new Promise((resolve, reject) => {
113+
stream.on("end", () => {
114+
hash.end()
115+
resolve(hash.digest("hex"))
116+
})
117+
stream.on("error", (err) => {
118+
reject(err)
119+
})
120+
stream.on("data", (chunk) => {
121+
hash.update(chunk)
122+
})
123+
})
124+
}
125+
126+
/**
127+
* Return the binary name for the current platform.
128+
*/
129+
export function name(): string {
130+
const os = goos()
131+
const arch = goarch()
132+
let binName = `coder-${os}-${arch}`
133+
// Windows binaries have an exe suffix.
134+
if (os === "windows") {
135+
binName += ".exe"
136+
}
137+
return binName
138+
}
139+
140+
/**
141+
* Returns the Go format for the current platform.
142+
* Coder binaries are created in Go, so we conform to that name structure.
143+
*/
144+
export function goos(): string {
145+
const platform = os.platform()
146+
switch (platform) {
147+
case "win32":
148+
return "windows"
149+
default:
150+
return platform
151+
}
152+
}
153+
154+
/**
155+
* Return the Go format for the current architecture.
156+
*/
157+
export function goarch(): string {
158+
const arch = os.arch()
159+
switch (arch) {
160+
case "arm":
161+
return "armv7"
162+
case "x64":
163+
return "amd64"
164+
default:
165+
return arch
166+
}
167+
}

‎src/remote.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,14 @@ export class Remote {
465465
//
466466
// If we didn't write to the SSH config file, connecting would fail with
467467
// "Host not found".
468-
await this.updateSSHConfig(authorityParts[1], hasCoderLogs)
468+
try {
469+
await this.updateSSHConfig(authorityParts[1], hasCoderLogs)
470+
} catch (error) {
471+
// TODO: This is a bit weird, because even if we throw an error VS Code
472+
// still tries to connect. Can we stop it?
473+
this.storage.writeToCoderOutputChannel(`Failed to configure SSH: ${error}`)
474+
throw error
475+
}
469476

470477
this.findSSHProcessID().then((pid) => {
471478
// Once the SSH process has spawned we can reset the timeout.
@@ -587,9 +594,6 @@ export class Remote {
587594
binaryPath = await this.storage.fetchBinary()
588595
}
589596
}
590-
if (!binaryPath) {
591-
throw new Error("Failed to fetch the Coder binary!")
592-
}
593597

594598
const escape = (str: string): string => `"${str.replace(/"/g, '\\"')}"`
595599

0 commit comments

Comments
 (0)