Skip to content

Commit a23c433

Browse files
EhabYEhabY
committed
Add logging to SshConfig
1 parent c1a339a commit a23c433

File tree

3 files changed

+55
-30
lines changed

3 files changed

+55
-30
lines changed

‎src/remote/remote.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ import { vscodeProposed } from "../vscodeProposed";
5252
import { WorkspaceMonitor } from "../workspace/workspaceMonitor";
5353

5454
import {
55-
SSHConfig,
55+
SshConfig,
5656
type SSHValues,
5757
mergeSshConfigValues,
5858
parseCoderSshOptions,
@@ -829,7 +829,7 @@ export class Remote {
829829
sshConfigFile = path.join(os.homedir(), sshConfigFile.slice(1));
830830
}
831831

832-
const sshConfig = new SSHConfig(sshConfigFile);
832+
const sshConfig = new SshConfig(sshConfigFile, this.logger);
833833
await sshConfig.load();
834834

835835
// Merge SSH config from three sources (highest to lowest priority):

‎src/remote/sshConfig.ts‎

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@ import path from "node:path";
1010

1111
import { countSubstring, renameWithRetry, tempFilePath } from "../util";
1212

13-
class SSHConfigBadFormat extends Error {}
13+
import type { Logger } from "../logging/logger";
14+
15+
class SshConfigBadFormat extends Error {}
1416

1517
interface Block {
1618
raw: string;
@@ -172,9 +174,10 @@ export function mergeSshConfigValues(
172174
return merged;
173175
}
174176

175-
export class SSHConfig {
177+
export class SshConfig {
176178
private readonly filePath: string;
177179
private readonly fileSystem: FileSystem;
180+
private readonly logger: Logger;
178181
private raw: string | undefined;
179182

180183
private startBlockComment(safeHostname: string): string {
@@ -188,16 +191,25 @@ export class SSHConfig {
188191
: `# --- END CODER VSCODE ---`;
189192
}
190193

191-
constructor(filePath: string, fileSystem: FileSystem = defaultFileSystem) {
194+
constructor(
195+
filePath: string,
196+
logger: Logger,
197+
fileSystem: FileSystem = defaultFileSystem,
198+
) {
192199
this.filePath = filePath;
200+
this.logger = logger;
193201
this.fileSystem = fileSystem;
194202
}
195203

196204
async load() {
197205
try {
198206
this.raw = await this.fileSystem.readFile(this.filePath, "utf-8");
207+
this.logger.debug("Loaded SSH config", this.filePath);
199208
} catch {
200-
// Probably just doesn't exist!
209+
this.logger.debug(
210+
"SSH config file not found, starting fresh",
211+
this.filePath,
212+
);
201213
this.raw = "";
202214
}
203215
}
@@ -213,8 +225,10 @@ export class SSHConfig {
213225
const block = this.getBlock(safeHostname);
214226
const newBlock = this.buildBlock(safeHostname, values, overrides);
215227
if (block) {
228+
this.logger.debug("Replacing SSH config block", safeHostname);
216229
this.replaceBlock(block, newBlock);
217230
} else {
231+
this.logger.debug("Appending new SSH config block", safeHostname);
218232
this.appendBlock(newBlock);
219233
}
220234
await this.save();
@@ -231,13 +245,13 @@ export class SSHConfig {
231245
const startBlockCount = countSubstring(startBlock, raw);
232246
const endBlockCount = countSubstring(endBlock, raw);
233247
if (startBlockCount !== endBlockCount) {
234-
throw new SSHConfigBadFormat(
248+
throw new SshConfigBadFormat(
235249
`Malformed config: ${this.filePath} has an unterminated START CODER VSCODE ${safeHostname ? safeHostname + " " : ""}block. Each START block must have an END block.`,
236250
);
237251
}
238252

239253
if (startBlockCount > 1 || endBlockCount > 1) {
240-
throw new SSHConfigBadFormat(
254+
throw new SshConfigBadFormat(
241255
`Malformed config: ${this.filePath} has ${startBlockCount} START CODER VSCODE ${safeHostname ? safeHostname + " " : ""}sections. Please remove all but one.`,
242256
);
243257
}
@@ -250,15 +264,15 @@ export class SSHConfig {
250264
}
251265

252266
if (startBlockIndex === -1) {
253-
throw new SSHConfigBadFormat("Start block not found");
267+
throw new SshConfigBadFormat("Start block not found");
254268
}
255269

256270
if (startBlockIndex === -1) {
257-
throw new SSHConfigBadFormat("End block not found");
271+
throw new SshConfigBadFormat("End block not found");
258272
}
259273

260274
if (endBlockIndex < startBlockIndex) {
261-
throw new SSHConfigBadFormat(
275+
throw new SshConfigBadFormat(
262276
"Malformed config, end block is before start block",
263277
);
264278
}
@@ -371,8 +385,15 @@ export class SSHConfig {
371385
tempPath,
372386
this.filePath,
373387
);
388+
this.logger.debug("Saved SSH config", this.filePath);
374389
} catch (err) {
375-
await this.fileSystem.unlink(tempPath).catch(() => undefined);
390+
await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => {
391+
this.logger.warn(
392+
"Failed to clean up temp SSH config file",
393+
tempPath,
394+
unlinkErr,
395+
);
396+
});
376397
throw new Error(
377398
`Failed to rename temporary SSH config file at ${tempPath} to ${this.filePath}: ${
378399
err instanceof Error ? err.message : String(err)
@@ -384,7 +405,7 @@ export class SSHConfig {
384405

385406
public getRaw() {
386407
if (this.raw === undefined) {
387-
throw new Error("SSHConfig is not loaded. Try sshConfig.load()");
408+
throw new Error("SshConfig is not loaded. Try sshConfig.load()");
388409
}
389410

390411
return this.raw;

‎test/unit/remote/sshConfig.test.ts‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
1-
import { it, afterEach, vi, expect, describe } from "vitest";
1+
import { it, afterEach, vi, expect, describe, beforeEach } from "vitest";
22

33
import {
4-
SSHConfig,
4+
SshConfig,
55
parseCoderSshOptions,
66
parseSshConfig,
77
mergeSshConfigValues,
88
} from "@/remote/sshConfig";
99

10+
import { createMockLogger } from "../../mocks/testHelpers";
11+
1012
// This is not the usual path to ~/.ssh/config, but
1113
// setting it to a different path makes it easier to test
1214
// and makes mistakes abundantly clear.
@@ -25,6 +27,8 @@ const mockFileSystem = {
2527
writeFile: vi.fn(),
2628
};
2729

30+
const mockLogger = createMockLogger();
31+
2832
afterEach(() => {
2933
vi.clearAllMocks();
3034
});
@@ -33,7 +37,7 @@ it("creates a new file and adds config with empty label", async () => {
3337
mockFileSystem.readFile.mockRejectedValueOnce("No file found");
3438
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" });
3539

36-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
40+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
3741
await sshConfig.load();
3842
await sshConfig.update("", {
3943
Host: "coder-vscode--*",
@@ -76,7 +80,7 @@ it("creates a new file and adds the config", async () => {
7680
mockFileSystem.readFile.mockRejectedValueOnce("No file found");
7781
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" });
7882

79-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
83+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
8084
await sshConfig.load();
8185
await sshConfig.update("dev.coder.com", {
8286
Host: "coder-vscode.dev.coder.com--*",
@@ -126,7 +130,7 @@ it("adds a new coder config in an existent SSH configuration", async () => {
126130
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
127131
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 });
128132

129-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
133+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
130134
await sshConfig.load();
131135
await sshConfig.update("dev.coder.com", {
132136
Host: "coder-vscode.dev.coder.com--*",
@@ -197,7 +201,7 @@ Host *
197201
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
198202
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 });
199203

200-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
204+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
201205
await sshConfig.load();
202206
await sshConfig.update("dev.coder.com", {
203207
Host: "coder-vscode.dev-updated.coder.com--*",
@@ -255,7 +259,7 @@ Host coder-vscode--*
255259
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
256260
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 });
257261

258-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
262+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
259263
await sshConfig.load();
260264
await sshConfig.update("dev.coder.com", {
261265
Host: "coder-vscode.dev.coder.com--*",
@@ -298,7 +302,7 @@ it("it does not remove a user-added block that only matches the host of an old c
298302
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
299303
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 });
300304

301-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
305+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
302306
await sshConfig.load();
303307
await sshConfig.update("dev.coder.com", {
304308
Host: "coder-vscode.dev.coder.com--*",
@@ -355,7 +359,7 @@ Host afterconfig
355359
HostName after.config.tld
356360
User after`;
357361

358-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
362+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
359363
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
360364
await sshConfig.load();
361365

@@ -410,7 +414,7 @@ Host afterconfig
410414
HostName after.config.tld
411415
User after`;
412416

413-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
417+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
414418
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
415419
await sshConfig.load();
416420

@@ -461,7 +465,7 @@ Host afterconfig
461465
HostName after.config.tld
462466
User after`;
463467

464-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
468+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
465469
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
466470
await sshConfig.load();
467471

@@ -511,7 +515,7 @@ Host afterconfig
511515
HostName after.config.tld
512516
User after`;
513517

514-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
518+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
515519
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
516520
await sshConfig.load();
517521

@@ -561,7 +565,7 @@ Host afterconfig
561565
HostName after.config.tld
562566
User after`;
563567

564-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
568+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
565569
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
566570
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 });
567571
await sshConfig.load();
@@ -624,7 +628,7 @@ it("override values", async () => {
624628
mockFileSystem.readFile.mockRejectedValueOnce("No file found");
625629
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" });
626630

627-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
631+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
628632
await sshConfig.load();
629633
await sshConfig.update(
630634
"dev.coder.com",
@@ -683,7 +687,7 @@ it("fails if we are unable to write the temporary file", async () => {
683687
HostName before.config.tld
684688
User before`;
685689

686-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
690+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
687691
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig);
688692
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 });
689693
mockFileSystem.writeFile.mockRejectedValueOnce(new Error("EACCES"));
@@ -714,7 +718,7 @@ it("cleans up temp file when rename fails", async () => {
714718
(err as NodeJS.ErrnoException).code = "EXDEV";
715719
mockFileSystem.rename.mockRejectedValueOnce(err);
716720

717-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
721+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
718722
await sshConfig.load();
719723
await expect(
720724
sshConfig.update("dev.coder.com", {
@@ -755,7 +759,7 @@ describe("rename retry on Windows", () => {
755759
.mockRejectedValueOnce(err)
756760
.mockResolvedValueOnce(undefined);
757761

758-
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem);
762+
const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem);
759763
await sshConfig.load();
760764
const promise = sshConfig.update("dev.coder.com", {
761765
Host: "coder-vscode.dev.coder.com--*",

0 commit comments

Comments
 (0)