gustavosantos/MIKROTIK-RMM
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(safety): Change Guard — device-side auto-revert for risky config changes (v0.18.0-beta)

Browse Source 
A user change can sever the manager's own path to a device with no warning and no
way back. Most recently, deleting an *unused* bond on a switch took the whole
switch offline. This adds a safety net, and fixes the bug that caused that
specific incident.

Root cause of the bond incident (DeviceCollector.deleteBond):
- Slaves were re-added to the bridge ONLY when the bond itself was a bridge
  member. For an unbridged bond the restore loop never ran, so the member ports
  were released from the bond and left in no bridge at all — the ports stopped
  forwarding and the switch dropped off the network.
- The restore in both deleteBond and createBond was wrapped in `.catch(() => {})`,
  so a failed restore was swallowed and the route still returned 200 OK.
- Restores also dropped PVID, silently returning tagged ports to VLAN 1.
Now: port settings (PVID, frame-types, ingress-filtering, edge, horizon) are
captured and carried over, and restore failures are reported instead of hidden.

Change Guard (services/changeGuard/ChangeGuard.ts):
Before a risky change the device saves a restore point and arms a scheduler that
restores it after N seconds. The change is applied, then reachability is proven
on a NEW connection; success disarms the scheduler, failure leaves the device to
restore itself and come back. Because the restore point is created before the
scheduler, a binary restore rolls back to a config containing neither the
scheduler nor the change — self-cleaning, with no revert loop.

- Works entirely over the RouterOS API. This matters: every recovery artifact
  today requires SSH, so devices without SSH credentials had no rollback target
  at all (cf. #86).
- Modes: `binary` (exact restore incl. deleted interfaces, reboots) or `script`
  (/import, no reboot but additive). Configurable via change_guard_* settings.
- Per-device Redis lock, `device_change_guards` audit table, and startup
  reconciliation for guards left pending by a manager restart.
- Degrades gracefully: if protection can't be armed the change still runs and the
  response says it was unprotected.

Wired into bond create/delete, bridge vlan-filtering, and /ip/service. Disabling
the service the manager is connected through is now refused outright (409) — the
mgmt-service signal already existed but was confined to the read-only posture
endpoint.

Also: replaces the bare confirm() on "Destroy Bond" with a ChangeGuardDialog that
explains the risk and the protection, and reports the outcome (including "the
device is restoring itself"). Fixes getAppSettings omitting
config_snapshot_enabled/interval, which made the v0.17.4 snapshot toggle a no-op.

Verified on real hardware (hAP-class switch): capability probe confirmed
/system/backup/save and scheduler add/remove work over the API; a guarded change
armed, applied, verified and committed leaving zero schedulers and zero files on
the device; disabling the api service was refused with 409. 133 backend tests
pass, including 7 guard state-machine tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2GT-Rich
2026-08-03 11:52:07 -07:00
co-authored by Claude Opus 4.8
parent 5895e905b7
commit 3a18cb1d91
13 changed files with 886 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/services/mikrotik/DeviceCollector.ts
+72 -16
View File
@@ -2715,16 +2715,40 @@ export class DeviceCollector {
return policy;
}
/**
* Bridge-port attributes that must survive a port being re-created. PVID above
* all: re-adding a port without it silently drops the port back to VLAN 1, which
* breaks tagged management even though the port looks bridged again.
*/
private static readonly BRIDGE_PORT_CARRY = [
'pvid', 'frame-types', 'ingress-filtering', 'edge', 'horizon', 'point-to-point',
] as const;
private static carryBridgePortAttrs(port: Record<string, string> | undefined): Record<string, string> {
const out: Record<string, string> = {};
if (!port) return out;
for (const key of DeviceCollector.BRIDGE_PORT_CARRY) {
const value = port[key];
if (value !== undefined && value !== '') out[key] = value;
}
return out;
}
async createBond(name: string, slaves: string[], mode: string, opts: {
lacpRate?: string; hashPolicy?: string; mtu?: number; minLinks?: number;
}): Promise<void> {
// Remove each slave from any bridge, recording the first bridge found
// Remove each slave from any bridge, recording the first bridge found along
// with its port settings so the bond can inherit them.
let originalBridge: string | null = null;
let carry: Record<string, string> = {};
for (const slave of slaves) {
const bridgePorts = await this.client.execute('/interface/bridge/port/print', {}, [`?interface=${slave}`]).catch(() => []);
for (const bp of bridgePorts) {
if (bp['.id']) {
if (!originalBridge) originalBridge = bp['bridge'] ?? null;
if (!originalBridge) {
originalBridge = bp['bridge'] ?? null;
carry = DeviceCollector.carryBridgePortAttrs(bp);
}
await this.client.execute('/interface/bridge/port/remove', { '.id': bp['.id'] });
}
}
@@ -2735,12 +2759,23 @@ export class DeviceCollector {
if (opts.mtu) params['mtu'] = String(opts.mtu);
if (opts.minLinks != null) params['min-links'] = String(opts.minLinks);
await this.client.execute('/interface/bonding/add', params);
// Add the new bond interface to the same bridge the slaves were removed from
// Put the new bond into the bridge the slaves came out of. If this fails the
// member ports are left unbridged — never swallow it, the device may be
// unreachable and the caller must know.
if (originalBridge) {
await this.client.execute('/interface/bridge/port/add', {
bridge: originalBridge,
interface: name,
}).catch(() => {});
try {
await this.client.execute('/interface/bridge/port/add', {
bridge: originalBridge,
interface: name,
...carry,
});
} catch (err) {
throw new Error(
`Bond '${name}' was created, but adding it to bridge '${originalBridge}' failed: ` +
`${(err as Error).message}. The member ports (${slaves.join(', ')}) are no longer bridged ` +
`and may not forward traffic — check the device.`
);
}
}
}
@@ -2763,22 +2798,43 @@ export class DeviceCollector {
const id = list[0]?.['.id'];
if (!id) throw new Error(`Bond '${name}' not found on device`);
const slaves = (list[0]?.['slaves'] ?? '').split(',').map((s: string) => s.trim()).filter(Boolean);
// Check if the bond is itself a bridge member
// Capture the bond's bridge membership *and its port settings* before removing
// it, so the freed slaves can inherit them (notably PVID).
const bondBridgePorts = await this.client.execute('/interface/bridge/port/print', {}, [`?interface=${name}`]).catch(() => []);
const bridgeName = bondBridgePorts[0]?.['bridge'] ?? null;
if (bondBridgePorts[0]?.['.id']) {
await this.client.execute('/interface/bridge/port/remove', { '.id': bondBridgePorts[0]['.id'] });
const bondPort = bondBridgePorts[0];
const bridgeName = bondPort?.['bridge'] ?? null;
const carry = DeviceCollector.carryBridgePortAttrs(bondPort);
if (bondPort?.['.id']) {
await this.client.execute('/interface/bridge/port/remove', { '.id': bondPort['.id'] });
}
await this.client.execute('/interface/bonding/remove', { '.id': id });
// Re-add each slave to the bridge the bond was in
// Removing a bond releases its slaves as standalone interfaces. If the bond was
// bridged, put each slave back in that bridge with the same port settings —
// otherwise the physical ports stop forwarding and the device can drop off the
// network. A failure here is exactly how a switch goes silently offline, so it
// is reported rather than swallowed.
const failures: string[] = [];
if (bridgeName) {
for (const slave of slaves) {
await this.client.execute('/interface/bridge/port/add', {
bridge: bridgeName,
interface: slave,
}).catch(() => {});
try {
await this.client.execute('/interface/bridge/port/add', {
bridge: bridgeName,
interface: slave,
...carry,
});
} catch (err) {
failures.push(`${slave} (${(err as Error).message})`);
}
}
}
if (failures.length > 0) {
throw new Error(
`Bond '${name}' was removed, but restoring bridge membership on '${bridgeName}' failed for: ` +
`${failures.join('; ')}. Those ports may not forward traffic — check the device.`
);
}
}
// ─── Hardware Health ──────────────────────────────────────────────────────