From fb4a5e40f32f9ad3dbd3933c80daf78a61a2ccd7 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Sat, 18 May 2024 03:33:55 +0100 Subject: [PATCH 1/4] Add tests for loopbacks --- test/integration/index.js | 6 +++++ test/integration/routing.js | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/test/integration/index.js b/test/integration/index.js index 22450cbd..e9584a5c 100644 --- a/test/integration/index.js +++ b/test/integration/index.js @@ -189,6 +189,12 @@ before(async function() { format: new lib.TerminalFormat(), })); + // If fast test is enabled then output that it is + // eslint-disable-next-line node/no-process-env + if (process.env.FAST_TEST) { + console.log("FAST_TEST is present in env, slow tests will be skipped."); + } + await fs.remove(instancesDir); await fs.remove(modsDir); await fs.remove(databaseDir); diff --git a/test/integration/routing.js b/test/integration/routing.js index 6881f079..22c5e0ad 100644 --- a/test/integration/routing.js +++ b/test/integration/routing.js @@ -5,6 +5,9 @@ const lib = require("@clusterio/lib"); const { Host, Instance, InstanceConnection } = require("@clusterio/host"); const { ControlConnection, Controller, HostConnection, InstanceInfo } = require("@clusterio/controller"); +// Although there are no imports, the file has side-effects such as "before" which can effect tests +require("./index"); + const addr = lib.Address.fromShorthand; class Control extends lib.Link { } @@ -419,4 +422,45 @@ describe("Integration of link routing", function() { }); } }); + + describe("event should not be routable on loopback", function() { + it("should not loopback on controller", async function() { + // Sending an event never returns a promise, so it throws rather than rejects like a request + assert.throws( + () => send(get("controller"), addr("controller"), new TestEvent()), + new Error("controller as dst is not supported.") + ); + }); + + for (const srcName of ["hostA", "instanceA1", "controlA"]) { + it(`should not loopback on ${srcName}`, async function() { + const src = get(srcName); + const dstAddr = src.connector.src; + assert.throws( + () => send(src, dstAddr, new TestEvent()), + new Error(`Message would return back to sender ${dstAddr}.`) + ); + }); + } + }); + + describe("request should not be routable on loopback", function() { + it("should not loopback on controller", async function() { + await assert.rejects( + () => send(get("controller"), addr("controller"), new TestRequest()), + new Error("controller as dst is not supported.") + ); + }); + + for (const srcName of ["hostA", "instanceA1", "controlA"]) { + it(`should not loopback on ${srcName}`, async function() { + const src = get(srcName); + const dstAddr = src.connector.src; + await assert.rejects( + () => send(src, dstAddr, new TestRequest()), + new Error(`Message would return back to sender ${dstAddr}.`) + ); + }); + } + }); }); From 0c9e91b9533881e1748ce16bc1a10168c4f58814 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Sat, 18 May 2024 03:40:18 +0100 Subject: [PATCH 2/4] Fix error message on instance loopback attempt --- packages/host/src/Host.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/host/src/Host.ts b/packages/host/src/Host.ts index 75e80ffa..2c806c42 100644 --- a/packages/host/src/Host.ts +++ b/packages/host/src/Host.ts @@ -140,7 +140,7 @@ export class HostRouter { return false; } origin.connector.sendResponseError( - new lib.ResponseError("Instance is not running."), message.src + new lib.ResponseError(msg || "Instance is not running."), message.src ); } return true; From d1fa5e67d48a972579c5d3c43c4984151a6a5697 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Sat, 18 May 2024 18:42:53 +0100 Subject: [PATCH 3/4] Improve error throwing for sending events --- packages/controller/src/Controller.ts | 25 ++++++++++++++++--------- packages/lib/src/link/link.ts | 6 ++++++ test/integration/routing.js | 25 +++++-------------------- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index e1e53293..f256da32 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -1351,10 +1351,10 @@ export default class Controller { throw Error(`Unknown type ${requestOrEvent.constructor.type}.`); } - async sendRequest(request: any, dst: lib.Address) { + sendRequest(request: any, dst: lib.Address) { let connection; if (dst.type === lib.Address.controller) { - throw new Error("controller as dst is not supported."); + throw new Error(`Message would return back to sender ${dst}.`); } else if (dst.type === lib.Address.control) { connection = this.wsServer.controlConnections.get(dst.id); @@ -1389,24 +1389,33 @@ export default class Controller { sendEvent(event: lib.Event, dst: lib.Address) { let connection; if (dst.type === lib.Address.controller) { - throw new Error("controller as dst is not supported."); + throw new Error(`Message would return back to sender ${dst}.`); } else if (dst.type === lib.Address.control) { connection = this.wsServer.controlConnections.get(dst.id); + if (!connection) { + throw new Error("Target control connection does not exist."); + } } else if (dst.type === lib.Address.instance) { let instance = this.instances.get(dst.id); if (!instance) { - return; + throw new Error(`Instance with ID ${dst.id} does not exist`); } let hostId = instance.config.get("instance.assigned_host"); if (hostId === null) { - return; + throw new Error("Instance is not assigned to a host"); } connection = this.wsServer.hostConnections.get(hostId); + if (!connection) { + throw new Error("Host containing instance is not connected"); + } } else if (dst.type === lib.Address.host) { connection = this.wsServer.hostConnections.get(dst.id); + if (!connection) { + throw new Error("Host is not connected"); + } } else if (dst.type === lib.Address.broadcast) { if (dst.id === lib.Address.control) { @@ -1424,7 +1433,7 @@ export default class Controller { } } else { - throw new Error(`Unexpected broacdast target ${dst.id}`); + throw new Error(`Unexpected broadcast target ${dst.id}`); } return; @@ -1432,9 +1441,7 @@ export default class Controller { throw new Error(`Unknown address type ${dst.type}`); } - if (connection) { - connection.sendEvent(event, dst); - } + connection.sendEvent(event, dst); } async sendToHostByInstanceId(request: Req & lib.Request): Promise; diff --git a/packages/lib/src/link/link.ts b/packages/lib/src/link/link.ts index 1d3d3381..ae91a4c8 100644 --- a/packages/lib/src/link/link.ts +++ b/packages/lib/src/link/link.ts @@ -538,6 +538,9 @@ export class Link { if (!entry) { throw new Error(`Attempt to send unregistered Request ${request.constructor.name}`); } + if (this.connector.src.equals(dst)) { + throw new Error(`Message would return back to sender ${dst}.`); + } if (this.validateSent) { entry.requestFromJSON(JSON.parse(JSON.stringify(request))); } @@ -572,6 +575,9 @@ export class Link { if (!entry) { throw new Error(`Attempt to send unregistered Event ${event.constructor.name}`); } + if (this.connector.src.equals(dst)) { + throw new Error(`Message would return back to sender ${dst}.`); + } if (this.validateSent) { entry.eventFromJSON(JSON.parse(JSON.stringify(event))); } diff --git a/test/integration/routing.js b/test/integration/routing.js index 22c5e0ad..7c5b204c 100644 --- a/test/integration/routing.js +++ b/test/integration/routing.js @@ -424,18 +424,10 @@ describe("Integration of link routing", function() { }); describe("event should not be routable on loopback", function() { - it("should not loopback on controller", async function() { - // Sending an event never returns a promise, so it throws rather than rejects like a request - assert.throws( - () => send(get("controller"), addr("controller"), new TestEvent()), - new Error("controller as dst is not supported.") - ); - }); - - for (const srcName of ["hostA", "instanceA1", "controlA"]) { + for (const srcName of ["controller", "hostA", "instanceA1", "controlA"]) { it(`should not loopback on ${srcName}`, async function() { const src = get(srcName); - const dstAddr = src.connector.src; + const dstAddr = src instanceof Controller ? addr("controller") : src.connector.src; assert.throws( () => send(src, dstAddr, new TestEvent()), new Error(`Message would return back to sender ${dstAddr}.`) @@ -445,18 +437,11 @@ describe("Integration of link routing", function() { }); describe("request should not be routable on loopback", function() { - it("should not loopback on controller", async function() { - await assert.rejects( - () => send(get("controller"), addr("controller"), new TestRequest()), - new Error("controller as dst is not supported.") - ); - }); - - for (const srcName of ["hostA", "instanceA1", "controlA"]) { + for (const srcName of ["controller", "hostA", "instanceA1", "controlA"]) { it(`should not loopback on ${srcName}`, async function() { const src = get(srcName); - const dstAddr = src.connector.src; - await assert.rejects( + const dstAddr = src instanceof Controller ? addr("controller") : src.connector.src; + assert.throws( () => send(src, dstAddr, new TestRequest()), new Error(`Message would return back to sender ${dstAddr}.`) ); From ebc0298115d1abd7f78bcefda9b3f84bae854770 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Sat, 18 May 2024 18:48:08 +0100 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dd176b9..099eb639 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,20 @@ Many thanks to the following for contributing to this release: [@username](https://github.com/username) --> +## Version 2.0.0-alpha.19 + +### Changes + +- Fixed IPC errors reporting undefined when no stack trace was present [#624](https://github.com/clusterio/clusterio/pull/624). +- Fixed loopback routing for instances reporting the wrong error message [#625](https://github.com/clusterio/clusterio/pull/625). + +### Breaking Changes + +- Previously silent errors for controller.sendEvent now throw exceptions [#625](https://github.com/clusterio/clusterio/pull/625). + +Many thanks to the following for contributing to this release: +[@Cooldude2606](https://github.com/Cooldude2606) + ## Version 2.0.0-alpha.18 ### Features