diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 3b1cdd67..091b7d01 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -292,6 +292,9 @@ export default class ControlConnection extends BaseConnection { } async handleInstanceCreateRequest(request: lib.InstanceCreateRequest) { + if ((request.config["instance.assigned_host"] ?? null) !== null) { + throw new lib.RequestError("instance.assigned_host must be set through the assign-host interface"); + } const instanceConfig = new lib.InstanceConfig("controller"); if (request.cloneFromId) { const baseInstance = this._controller.instances.get(request.cloneFromId); diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index 0736c493..adc3d7d2 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/packages/controller/src/InstanceManager.ts @@ -208,9 +208,17 @@ export default class InstanceManager { instance.config.set("instance.assigned_host", hostId); if (newHostConnection) { - await newHostConnection.send( - new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")), - ); + try { + await newHostConnection.send( + new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")), + ); + } catch (err) { + // Leave unassigned so the assign can be retried + instance.config.set("instance.assigned_host", null); + instance.status = "unassigned"; + this.records.set(instance); + throw err; + } } else { instance.status = "unassigned"; } diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index dfaf4087..72252858 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import events from "node:events"; import * as lib from "@clusterio/lib"; -import { Controller, ControlConnection, ControllerHooks } from "@clusterio/controller"; +import { Controller, ControlConnection, ControllerHooks, InstanceManager } from "@clusterio/controller"; import { MockConnector, MockController, MockLogger } from "../mock.js"; const addr = lib.Address.fromShorthand; @@ -76,6 +76,41 @@ describe("controller/src/ControlConnection", function() { }); }); + describe(".handleInstanceCreateRequest()", function() { + let mockController; + + beforeEach(function() { + mockController = new MockController(); + mockController.mockConfigEntries.set("controller.name", "Test"); + mockController.instances = new InstanceManager(new lib.SubscribableDatastore(), mockController); + }); + + async function create(config, cloneFromId) { + await ControlConnection.prototype.handleInstanceCreateRequest.call( + { _controller: mockController }, + new lib.InstanceCreateRequest(config, cloneFromId), + ); + } + + it("rejects instance.assigned_host in the config", async function() { + await assert.rejects( + create({ "instance.id": 4001, "instance.name": "c1", "instance.assigned_host": 10 }), + new lib.RequestError("instance.assigned_host must be set through the assign-host interface"), + ); + assert.equal(mockController.instances.has(4001), false); + }); + + it("creates an unassigned clone of an assigned instance", async function() { + await create({ "instance.id": 4001, "instance.name": "base" }); + mockController.instances.getMutable(4001).config.set("instance.assigned_host", 10); + + await create({ "instance.id": 4002, "instance.name": "clone", "instance.assigned_host": null }, 4001); + const clone = mockController.instances.get(4002); + assert.equal(clone.config.get("instance.name"), "clone"); + assert.equal(clone.config.get("instance.assigned_host"), null); + }); + }); + describe("close cleanup", function() { let connector; let mockController; diff --git a/test/controller/InstanceManager.test.js b/test/controller/InstanceManager.test.js index 59e05063..237b3dca 100644 --- a/test/controller/InstanceManager.test.js +++ b/test/controller/InstanceManager.test.js @@ -298,6 +298,45 @@ describe("controller/InstanceManager", function () { assert(assignRequest instanceof lib.InstanceAssignInternalRequest); assert.equal(assignRequest.instanceId, 1); }); + + it("should leave instance unassigned if the new host fails the assign", async function () { + let unassignRequest; + controller.wsServer.hostConnections.set(5, { + send: async (request) => { + unassignRequest = request; + }, + connector: { closing: false }, + }); + let failAssign = true; + let assignRequest; + controller.wsServer.hostConnections.set(6, { + send: async (request) => { + if (failAssign) { + throw new lib.RequestError("EACCES: permission denied"); + } + assignRequest = request; + }, + connector: { closing: false }, + }); + + instance.config.set("instance.assigned_host", 5); + instance.status = "stopped"; + await assert.rejects( + () => instances.assignInstance(1, 6), + { message: "EACCES: permission denied" } + ); + + assert(unassignRequest instanceof lib.InstanceUnassignInternalRequest); + const stored = instances.records.get(1); + assert.equal(stored.config.get("instance.assigned_host"), null); + assert.equal(stored.status, "unassigned"); + + failAssign = false; + await instances.assignInstance(1, 6); + assert.equal(instance.config.get("instance.assigned_host"), 6); + assert(assignRequest instanceof lib.InstanceAssignInternalRequest); + assert.equal(assignRequest.instanceId, 1); + }); }); describe(".unassignInstance()", function () {