From 58a5319e4522780ce08ef04af9475e1be5bcbfea Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 23 Aug 2020 11:51:55 +0100 Subject: [PATCH 1/8] add onConfigChanged for watching config changes --- src/components/cli.ts | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/components/cli.ts b/src/components/cli.ts index dac52cae..46a978a5 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -28,11 +28,20 @@ const log = logging.get("cli"); interface CliOpts> { run: (port: number, config: ConfigType|null, registration: AppServiceRegistration|null) => void; + onConfigChanged?: (config: ConfigType) => void, generateRegistration?: (reg: AppServiceRegistration, cb: (finalReg: AppServiceRegistration) => void) => void; bridgeConfig?: { affectsRegistration?: boolean; schema: string|Record; defaults: Record; + /** + * Should the config file be watched for changes. + */ + watchConfig: boolean; + /** + * How often should the watcher poll for changes on the config file. + */ + watchInterval?: number; }; registrationPath: string; enableRegistration?: boolean; @@ -51,6 +60,7 @@ interface CliArgs { } export class Cli> { + public static DEFAULT_WATCH_INTERVAL = 2500; private bridgeConfig: ConfigType|null = null; private args: CliArgs|null = null; @@ -84,6 +94,10 @@ export class Cli> { throw new Error("Requires 'run' function."); } + if (!this.opts.onConfigChanged && this.opts.bridgeConfig?.watchConfig) { + throw new Error('`bridgeConfig.watchConfig` is enabled but `onConfigChanged` is not defined'); + } + if (this.opts.enableRegistration && !this.opts.generateRegistration) { throw new Error( "Registration generation is enabled but no " + @@ -122,8 +136,8 @@ export class Cli> { /** * Run the app from the command line. Will parse sys args. */ - public run() { - this.args = nopt({ + public run(args?: CliArgs) { + this.args = args || nopt({ "generate-registration": Boolean, "config": path, "url": String, @@ -230,6 +244,24 @@ export class Cli> { } private startWithConfig(config: ConfigType|null) { + if (this.opts.bridgeConfig?.watchConfig && this.args?.config) { + log.info("Will watch config file for changes"); + fs.watchFile(this.args.config, { + persistent: false, + interval: this.opts.bridgeConfig.watchInterval || Cli.DEFAULT_WATCH_INTERVAL }, + (curr: fs.Stats) => { + log.info("Config file change detected, reloading"); + try { + const newConfig = this.loadConfig(this.args?.config); + // onConfigChanged is checked in the constructor + if (newConfig && this.opts.onConfigChanged) { + this.opts.onConfigChanged(newConfig); + } + } catch (ex) { + log.warn("Failed to reload config file:", ex); + } + }); + } this.opts.run( this.opts.port, config, AppServiceRegistration.fromObject(this.loadYaml(this.opts.registrationPath)) From 40cf52be9542ea37729c7794377816b3da750cd1 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 23 Aug 2020 12:50:07 +0100 Subject: [PATCH 2/8] Add tests for Cli --- spec/integ/cli.spec.js | 108 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 spec/integ/cli.spec.js diff --git a/spec/integ/cli.spec.js b/spec/integ/cli.spec.js new file mode 100644 index 00000000..b221e6ec --- /dev/null +++ b/spec/integ/cli.spec.js @@ -0,0 +1,108 @@ +const fs = require("fs").promises; +const os = require("os"); +const { Cli } = require("../.."); +const { Logging } = require("../.."); +const path = require('path'); + +Logging.configure(); + +let tempDir; + +const registrationFileContent = { + id: "cli-test", + url: "http://127.0.0.1:1234", + as_token: "a_as_token", + hs_token: "a_hs_token", + sender_localpart: "the_sender", + namespaces: { + users: [{ + exclusive: true, + regex: "@_the_bridge.*", + }], + aliases: [{ + exclusive: true, + regex: "@_the_bridge.*", + }], + rooms: [], + }, + rate_limited: false, + protocols: [], +}; +async function writeRegistrationFile(content=registrationFileContent, filename="registration.yaml") { + const filePath = path.join(tempDir, filename); + await fs.writeFile(filePath, JSON.stringify(content), "utf-8"); + return filePath; +} + +async function writeConfigFile(content={}) { + const filePath = path.join(tempDir, "config.yaml"); + await fs.writeFile(filePath, JSON.stringify(content), "utf-8"); + return filePath; +} + +describe("Cli", () => { + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "bridge-test")); + }); + afterEach(async () => { + await fs.rmdir(tempDir, {recursive: true}); + }); + + it("should be able to start the bridge with just a registration file", async () => { + let runCalledWith = false; + const cli = new Cli({ + enableRegistration: false, + registrationPath: await writeRegistrationFile({}, "reg.yml"), + run: (...args) => { runCalledWith = args; } + }); + cli.run(); + expect(runCalledWith[0]).toEqual(Cli.DEFAULT_PORT); + }); + + it("should be able to start the bridge with a custom port", async () => { + const port = 1234; + let runCalledWith = null; + const cli = new Cli({ + enableRegistration: false, + registrationPath: await writeRegistrationFile(), + run: (...args) => { runCalledWith = args; } + }); + cli.run({port}); + expect(runCalledWith[0]).toEqual(port); + }); + + it("should be able to start the bridge with a registration file and config file", async () => { + const configData = {"a": "var", "b": true}; + const configFile = await writeConfigFile(configData); + let runCalledWith = null; + const cli = new Cli({ + enableRegistration: false, + registrationPath: await writeRegistrationFile(), + bridgeConfig: {}, + run: (...args) => { runCalledWith = args; } + }); + cli.run({config: configFile}); + expect(runCalledWith[0]).toEqual(Cli.DEFAULT_PORT); + expect(runCalledWith[1]).toEqual(configData); + expect(runCalledWith[2].getOutput()).toEqual(registrationFileContent); + }); + + it("should detect config file changes", async () => { + const newConfigData = {"b": "var", "c": false}; + const configFile = await writeConfigFile({"a": "var", "b": true}); + let newConfigFile = null; + const cli = new Cli({ + enableRegistration: false, + registrationPath: await writeRegistrationFile(), + bridgeConfig: { watchConfig: true, watchInterval: 100 }, + onConfigChanged: (config) => { newConfigFile = config }, + run: (...args) => { } + }); + cli.run({config: configFile}); + await writeConfigFile(newConfigData); + + // Wait for changes, 3 x the interval to be safe. + await new Promise(r => setTimeout(r, 300)); + expect(newConfigFile).toEqual(newConfigData); + }); +}) From b1c1c947fb02a410fed87f2fdcb4b4debefb5d44 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 23 Aug 2020 12:50:15 +0100 Subject: [PATCH 3/8] Shuffle things around to support tests --- src/components/cli.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/cli.ts b/src/components/cli.ts index 46a978a5..949676e8 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -22,8 +22,6 @@ import { AppServiceRegistration } from "matrix-appservice"; import ConfigValidator from "./config-validator"; import * as logging from "./logging"; -const DEFAULT_PORT = 8090; -const DEFAULT_FILENAME = "registration.yaml"; const log = logging.get("cli"); interface CliOpts> { @@ -60,7 +58,9 @@ interface CliArgs { } export class Cli> { + public static DEFAULT_PORT = 8090; public static DEFAULT_WATCH_INTERVAL = 2500; + public static DEFAULT_FILENAME = "registration.yaml"; private bridgeConfig: ConfigType|null = null; private args: CliArgs|null = null; @@ -106,8 +106,8 @@ export class Cli> { } this.opts.enableLocalpart = Boolean(this.opts.enableLocalpart); - this.opts.registrationPath = this.opts.registrationPath || DEFAULT_FILENAME; - this.opts.port = this.opts.port || DEFAULT_PORT; + this.opts.registrationPath = this.opts.registrationPath || Cli.DEFAULT_FILENAME; + this.opts.port = this.opts.port || Cli.DEFAULT_PORT; } /** * Get the parsed arguments. Only set after run is called and arguments parsed. @@ -212,7 +212,7 @@ export class Cli> { private loadConfig(filename?: string): ConfigType|null { if (!filename) { return null; } - log.info("Loading config file %s", filename); + log.info("Loading config file", filename); const cfg = this.loadYaml(filename); if (!cfg || typeof cfg === "string") { throw Error("Config file " + filename + " isn't valid YAML."); From 4424f25cad6f416c1b1b7fe91650b4ca32113757 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Sun, 23 Aug 2020 12:54:31 +0100 Subject: [PATCH 4/8] lint and changelog --- changelog.d/207.feature | 1 + src/components/cli.ts | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 changelog.d/207.feature diff --git a/changelog.d/207.feature b/changelog.d/207.feature new file mode 100644 index 00000000..dd6d96f7 --- /dev/null +++ b/changelog.d/207.feature @@ -0,0 +1 @@ +Add ability to watch the bridge config for changes. \ No newline at end of file diff --git a/src/components/cli.ts b/src/components/cli.ts index 949676e8..40f15d83 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -249,7 +249,7 @@ export class Cli> { fs.watchFile(this.args.config, { persistent: false, interval: this.opts.bridgeConfig.watchInterval || Cli.DEFAULT_WATCH_INTERVAL }, - (curr: fs.Stats) => { + () => { log.info("Config file change detected, reloading"); try { const newConfig = this.loadConfig(this.args?.config); @@ -257,7 +257,8 @@ export class Cli> { if (newConfig && this.opts.onConfigChanged) { this.opts.onConfigChanged(newConfig); } - } catch (ex) { + } + catch (ex) { log.warn("Failed to reload config file:", ex); } }); From d9abc353d63b58cdacc0a20f41ad0361c81ef8b3 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 24 Aug 2020 10:00:47 +0100 Subject: [PATCH 5/8] Reload on SIGHUP --- src/components/cli.ts | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/src/components/cli.ts b/src/components/cli.ts index 40f15d83..d892ab88 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -32,14 +32,6 @@ interface CliOpts> { affectsRegistration?: boolean; schema: string|Record; defaults: Record; - /** - * Should the config file be watched for changes. - */ - watchConfig: boolean; - /** - * How often should the watcher poll for changes on the config file. - */ - watchInterval?: number; }; registrationPath: string; enableRegistration?: boolean; @@ -94,10 +86,6 @@ export class Cli> { throw new Error("Requires 'run' function."); } - if (!this.opts.onConfigChanged && this.opts.bridgeConfig?.watchConfig) { - throw new Error('`bridgeConfig.watchConfig` is enabled but `onConfigChanged` is not defined'); - } - if (this.opts.enableRegistration && !this.opts.generateRegistration) { throw new Error( "Registration generation is enabled but no " + @@ -201,17 +189,19 @@ export class Cli> { this.opts.port = this.args.port; } this.assignConfigFile(this.args.config); - this.startWithConfig(this.bridgeConfig); + this.startWithConfig(this.bridgeConfig, this.args.config); } private assignConfigFile(configFilePath: string) { const configFile = (this.opts.bridgeConfig && configFilePath) ? configFilePath : undefined; + if (!configFile) { + return null; + } const config = this.loadConfig(configFile); this.bridgeConfig = config; } - private loadConfig(filename?: string): ConfigType|null { - if (!filename) { return null; } + private loadConfig(filename: string): ConfigType { log.info("Loading config file", filename); const cfg = this.loadYaml(filename); if (!cfg || typeof cfg === "string") { @@ -243,18 +233,16 @@ export class Cli> { }); } - private startWithConfig(config: ConfigType|null) { - if (this.opts.bridgeConfig?.watchConfig && this.args?.config) { - log.info("Will watch config file for changes"); - fs.watchFile(this.args.config, { - persistent: false, - interval: this.opts.bridgeConfig.watchInterval || Cli.DEFAULT_WATCH_INTERVAL }, + private startWithConfig(config: ConfigType|null, configFilename: string) { + if (this.opts.onConfigChanged && this.opts.bridgeConfig) { + log.info("Will listen for SIGHUP"); + process.on("SIGHUP", () => { - log.info("Config file change detected, reloading"); + log.info("Got SIGHUP, reloading config file"); try { - const newConfig = this.loadConfig(this.args?.config); - // onConfigChanged is checked in the constructor - if (newConfig && this.opts.onConfigChanged) { + const newConfig = this.loadConfig(configFilename); + // onConfigChanged is checked above, but for Typescript's sake. + if (this.opts.onConfigChanged) { this.opts.onConfigChanged(newConfig); } } From 0754c6d1037b590955d82621ec872db110876848 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 24 Aug 2020 10:00:47 +0100 Subject: [PATCH 6/8] Reload on SIGHUP --- changelog.d/207.feature | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog.d/207.feature b/changelog.d/207.feature index dd6d96f7..54968a76 100644 --- a/changelog.d/207.feature +++ b/changelog.d/207.feature @@ -1 +1,2 @@ -Add ability to watch the bridge config for changes. \ No newline at end of file +The bridge can now optionally reload the config file on a `SIGHUP` signal. Developers should define the `onConfigChanged` callback +when constructing `Cli` to make use of this feature. \ No newline at end of file From 0d2b00248612913703b648ad2f15f7f12ffb96a9 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 24 Aug 2020 10:09:53 +0100 Subject: [PATCH 7/8] Fix tests --- spec/integ/cli.spec.js | 15 +++++++-------- src/components/cli.ts | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/spec/integ/cli.spec.js b/spec/integ/cli.spec.js index b221e6ec..6e72792a 100644 --- a/spec/integ/cli.spec.js +++ b/spec/integ/cli.spec.js @@ -3,7 +3,7 @@ const os = require("os"); const { Cli } = require("../.."); const { Logging } = require("../.."); const path = require('path'); - +const { defer } = require("../../lib/utils/promiseutil"); Logging.configure(); let tempDir; @@ -87,22 +87,21 @@ describe("Cli", () => { expect(runCalledWith[2].getOutput()).toEqual(registrationFileContent); }); - it("should detect config file changes", async () => { + it("should reload config on SIGHUP", async () => { const newConfigData = {"b": "var", "c": false}; const configFile = await writeConfigFile({"a": "var", "b": true}); - let newConfigFile = null; + const configDefer = defer(); const cli = new Cli({ enableRegistration: false, registrationPath: await writeRegistrationFile(), bridgeConfig: { watchConfig: true, watchInterval: 100 }, - onConfigChanged: (config) => { newConfigFile = config }, + onConfigChanged: (config) => { configDefer.resolve(config); }, run: (...args) => { } }); cli.run({config: configFile}); await writeConfigFile(newConfigData); - - // Wait for changes, 3 x the interval to be safe. - await new Promise(r => setTimeout(r, 300)); - expect(newConfigFile).toEqual(newConfigData); + // Send ourselves a signal + process.kill(process.pid, "SIGHUP"); + expect(await configDefer.promise).toEqual(newConfigData); }); }) diff --git a/src/components/cli.ts b/src/components/cli.ts index d892ab88..00b811f9 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -195,7 +195,7 @@ export class Cli> { private assignConfigFile(configFilePath: string) { const configFile = (this.opts.bridgeConfig && configFilePath) ? configFilePath : undefined; if (!configFile) { - return null; + return; } const config = this.loadConfig(configFile); this.bridgeConfig = config; From ee7b75c6225331e9d35f52ccb8b21a437acb523a Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 28 Aug 2020 13:29:04 +0100 Subject: [PATCH 8/8] Update cli.ts --- src/components/cli.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/cli.ts b/src/components/cli.ts index 00b811f9..af0b8c62 100644 --- a/src/components/cli.ts +++ b/src/components/cli.ts @@ -241,7 +241,6 @@ export class Cli> { log.info("Got SIGHUP, reloading config file"); try { const newConfig = this.loadConfig(configFilename); - // onConfigChanged is checked above, but for Typescript's sake. if (this.opts.onConfigChanged) { this.opts.onConfigChanged(newConfig); }