Fixed NTP & DNS server activation bug.

2026-04-15 08:48:18 -07:00
parent d992a8b5a6
commit 0a1ae97523
5 changed files with 145 additions and 13 deletions
@@ -0,0 +1,2 @@
+frontend/node_modules
+backend/node_modules
@@ -85,17 +85,19 @@ router.get('/overview', async (_req: Request, res: Response) => {
 router.get('/dhcp', async (req: Request, res: Response) => {
  const deviceId = deviceIdParam(req, res); if (!deviceId) return;
  await withDevice(deviceId, res, async (collector) => {
-    const [v4, v6, pv4, pv6] = await Promise.allSettled([
+    const [v4, v6, pv4, pv6, ifaces] = await Promise.allSettled([
      collector.getDhcpServers('ipv4'),
      collector.getDhcpServers('ipv6'),
      collector.getDhcpPools('ipv4'),
      collector.getDhcpPools('ipv6'),
+      collector.getDhcpInterfaces(),
    ]);
    res.json({
      ipv4: v4.status === 'fulfilled' ? v4.value : [],
      ipv6: v6.status === 'fulfilled' ? v6.value : [],
      pools_v4: pv4.status === 'fulfilled' ? pv4.value : [],
      pools_v6: pv6.status === 'fulfilled' ? pv6.value : [],
+      interfaces: ifaces.status === 'fulfilled' ? ifaces.value : [],
    });
  });
 });
@@ -1771,10 +1771,17 @@ export class DeviceCollector {
      if (servers[1]) ntpConfig['secondary-ntp'] = (servers[1] as Record<string, string>)['address'] || '';
    }

+    // RouterOS 7 DNS returns booleans as "true"/"false" instead of "yes"/"no"
+    const dnsRaw = (dns[0] as Record<string, string>) || {};
+    const dnsNormalized: Record<string, string> = {};
+    for (const [k, v] of Object.entries(dnsRaw)) {
+      dnsNormalized[k] = v === 'true' ? 'yes' : v === 'false' ? 'no' : v;
+    }
+
    return {
      identity: (identity[0] as Record<string, string>)?.['name'] || '',
      ntp: ntpConfig,
-      dns: (dns[0] as Record<string, string>) || {},
+      dns: dnsNormalized,
    };
  }

@@ -2205,6 +2212,13 @@ export class DeviceCollector {

  // DHCP ────────────────────────────────────────────────────────────────────────

+  async getDhcpInterfaces(): Promise<Record<string, string>[]> {
+    const all = await this.client.execute('/interface/print', { detail: '' }).catch(() => [] as Record<string, string>[]);
+    // Exclude interface types that can't meaningfully serve as a DHCP binding
+    const excluded = new Set(['pppoe-out', 'pptp-client', 'l2tp-client', 'ovpn-client', 'sstp-client', 'lte', 'lo', 'ipip', 'eoip', 'gre', 'vxlan', 'wireguard']);
+    return all.filter(i => !excluded.has(i['type'] || ''));
+  }
+
  async getDhcpServers(protocol: 'ipv4' | 'ipv6'): Promise<Record<string, string>[]> {
    const cmd = protocol === 'ipv4' ? '/ip/dhcp-server/print' : '/ipv6/dhcp-server/print';
    return this.client.execute(cmd, { detail: '' }).catch(() => [] as Record<string, string>[]);
@@ -2267,7 +2281,13 @@ export class DeviceCollector {

  async getDnsSettings(): Promise<Record<string, string>> {
    const rows = await this.client.execute('/ip/dns/print', {}).catch(() => [] as Record<string, string>[]);
-    return rows[0] ?? {};
+    const row = rows[0] ?? {};
+    // RouterOS 7 DNS returns booleans as "true"/"false" instead of "yes"/"no"
+    const out: Record<string, string> = {};
+    for (const [k, v] of Object.entries(row)) {
+      out[k] = v === 'true' ? 'yes' : v === 'false' ? 'no' : v;
+    }
+    return out;
  }

  async setDnsSettings(settings: {
@@ -2314,13 +2334,24 @@ export class DeviceCollector {
    server: Record<string, string>;
    client: Record<string, string>;
  }> {
+    // RouterOS 7 NTP module returns boolean fields as "true"/"false" rather than
+    // the "yes"/"no" used by every other RouterOS API. Normalize here so the rest
+    // of the app can use a consistent contract.
+    const normalizeBools = (r: Record<string, string>): Record<string, string> => {
+      const out: Record<string, string> = {};
+      for (const [k, v] of Object.entries(r)) {
+        out[k] = v === 'true' ? 'yes' : v === 'false' ? 'no' : v;
+      }
+      return out;
+    };
+
    const [server, client] = await Promise.allSettled([
-      this.client.execute('/system/ntp/server/print', {}).then(r => r[0] ?? {}),
-      this.client.execute('/system/ntp/client/print', {}).then(r => r[0] ?? {}),
+      this.client.execute('/system/ntp/server/print', {}).then(r => normalizeBools(r[0] ?? {})),
+      this.client.execute('/system/ntp/client/print', {}).then(r => normalizeBools(r[0] ?? {})),
    ]);
    return {
-      server: server.status === 'fulfilled' ? server.value as Record<string, string> : {},
-      client: client.status === 'fulfilled' ? client.value as Record<string, string> : {},
+      server: server.status === 'fulfilled' ? server.value : {},
+      client: client.status === 'fulfilled' ? client.value : {},
    };
  }

@@ -2337,7 +2368,7 @@ export class DeviceCollector {
    if (settings.server_broadcast !== undefined) serverParams['broadcast'] = settings.server_broadcast ? 'yes' : 'no';
    if (settings.server_manycast !== undefined) serverParams['manycast'] = settings.server_manycast ? 'yes' : 'no';
    if (Object.keys(serverParams).length > 0) {
-      await this.client.execute('/system/ntp/server/set', serverParams).catch(() => {});
+      await this.client.execute('/system/ntp/server/set', serverParams);
    }

    const clientParams: Record<string, string> = {};
@@ -2345,7 +2376,7 @@ export class DeviceCollector {
    if (settings.client_mode) clientParams['mode'] = settings.client_mode;
    if (settings.client_servers !== undefined) clientParams['servers'] = settings.client_servers;
    if (Object.keys(clientParams).length > 0) {
-      await this.client.execute('/system/ntp/client/set', clientParams).catch(() => {});
+      await this.client.execute('/system/ntp/client/set', clientParams);
    }
  }

@@ -40,17 +40,34 @@ interface ServerFormProps {
  protocol: 'ipv4' | 'ipv6';
  existing?: NS;
  pools: NS[];
+  interfaces: NS[];
  deviceId: number;
  onClose: () => void;
+  conflictDevices?: { name: string }[];
 }

-function ServerForm({ protocol, existing, pools, deviceId, onClose }: ServerFormProps) {
+function ServerForm({ protocol, existing, pools, interfaces, deviceId, onClose, conflictDevices }: ServerFormProps) {
  const qc = useQueryClient();
  const [name, setName] = useState(existing?.['name'] || '');
  const [iface, setIface] = useState(existing?.['interface'] || '');
  const [pool, setPool] = useState(existing?.['address-pool'] || '');
  const [leaseTime, setLeaseTime] = useState(existing?.['lease-time'] || '00:10:00');

+  // Group interfaces for the dropdown
+  const vlanIfaces  = interfaces.filter(i => i['type'] === 'vlan');
+  const bridgeIfaces = interfaces.filter(i => i['type'] === 'bridge');
+  const otherIfaces  = interfaces.filter(i => i['type'] !== 'vlan' && i['type'] !== 'bridge');
+
+  function ifaceLabel(i: NS): string {
+    const base = i['name'];
+    if (i['type'] === 'vlan') {
+      const vid = i['vlan-id'];
+      const parent = i['interface'];
+      return vid ? `${base}  —  VLAN ${vid}${parent ? ` on ${parent}` : ''}` : base;
+    }
+    return i['comment'] ? `${base}  —  ${i['comment']}` : base;
+  }
+
  const save = useMutation({
    mutationFn: () => {
      const body: NS & { protocol: 'ipv4' | 'ipv6' } = {
@@ -81,7 +98,37 @@ function ServerForm({ protocol, existing, pools, deviceId, onClose }: ServerForm
          </div>
          <div>
            <label className="block text-xs font-medium text-gray-600 dark:text-slate-300 mb-1">Interface *</label>
-            <input className="input w-full" value={iface} onChange={e => setIface(e.target.value)} placeholder="bridge1" />
+            {interfaces.length > 0 ? (
+              <select className="input w-full" value={iface} onChange={e => setIface(e.target.value)}>
+                <option value="">— select interface —</option>
+                {vlanIfaces.length > 0 && (
+                  <optgroup label="VLAN Interfaces">
+                    {vlanIfaces.map(i => (
+                      <option key={i['name']} value={i['name']}>{ifaceLabel(i)}</option>
+                    ))}
+                  </optgroup>
+                )}
+                {bridgeIfaces.length > 0 && (
+                  <optgroup label="Bridge Interfaces">
+                    {bridgeIfaces.map(i => (
+                      <option key={i['name']} value={i['name']}>{ifaceLabel(i)}</option>
+                    ))}
+                  </optgroup>
+                )}
+                {otherIfaces.length > 0 && (
+                  <optgroup label="Physical / Other">
+                    {otherIfaces.map(i => (
+                      <option key={i['name']} value={i['name']}>{ifaceLabel(i)}</option>
+                    ))}
+                  </optgroup>
+                )}
+              </select>
+            ) : (
+              <input className="input w-full" value={iface} onChange={e => setIface(e.target.value)} placeholder="bridge1, vlan10, ether1" />
+            )}
+            <p className="mt-0.5 text-xs text-gray-400 dark:text-slate-500">
+              VLAN interfaces scope the DHCP server to that VLAN. Bridge interfaces serve all untagged clients.
+            </p>
          </div>
          <div>
            <label className="block text-xs font-medium text-gray-600 dark:text-slate-300 mb-1">Address Pool</label>
@@ -100,6 +147,14 @@ function ServerForm({ protocol, existing, pools, deviceId, onClose }: ServerForm
            <p className="mt-0.5 text-xs text-gray-400">Format: HH:MM:SS or e.g. 10m, 1h, 1d</p>
          </div>
        </div>
+        {!existing && conflictDevices && conflictDevices.length > 0 && (
+          <div className="px-5 pb-3">
+            <div className="flex items-start gap-2 p-3 rounded-lg bg-amber-50 dark:bg-amber-900/20 border border-amber-200 dark:border-amber-700 text-xs text-amber-700 dark:text-amber-300">
+              <AlertTriangle className="w-3.5 h-3.5 flex-shrink-0 mt-0.5" />
+              <span>Active {protocol.toUpperCase()} DHCP server(s) already running on: <strong>{conflictDevices.map(d => d.name).join(', ')}</strong>. Multiple DHCP servers on the same network may cause IP address conflicts.</span>
+            </div>
+          </div>
+        )}
        <div className="px-5 pb-4 flex items-center gap-3">
          <button
            onClick={() => save.mutate()}
@@ -358,6 +413,23 @@ export default function NetworkServicesDHCPPage() {
    onSuccess: (_d, vars) => qc.invalidateQueries({ queryKey: ['ns-leases', deviceId, vars.protocol] }),
  });

+  // Cross-device conflict detection
+  const { data: overview = [] } = useQuery({
+    queryKey: ['network-services-overview'],
+    queryFn: () => networkServicesApi.overview().then(r => r.data as Record<string, unknown>[]),
+    staleTime: 60_000,
+  });
+  const conflictDevicesV4 = overview.filter(d => {
+    if ((d.id as number) === deviceId) return false;
+    const v4 = d.dhcp_v4 as { total: number; enabled: number } | null;
+    return (v4?.enabled ?? 0) > 0;
+  });
+  const conflictDevicesV6 = overview.filter(d => {
+    if ((d.id as number) === deviceId) return false;
+    const v6 = d.dhcp_v6 as { total: number; enabled: number } | null;
+    return (v6?.enabled ?? 0) > 0;
+  });
+
  function ServerTable({ servers, protocol }: { servers: NS[]; protocol: 'ipv4' | 'ipv6' }) {
    if (servers.length === 0) {
      return <div className="px-5 py-6 text-sm text-gray-400 dark:text-slate-500 text-center">No servers configured.</div>;
@@ -388,7 +460,16 @@ export default function NetworkServicesDHCPPage() {
                  {canWrite && (
                    <td className="px-4 py-3">
                      <div className="flex items-center justify-end gap-1">
-                        <button onClick={() => toggleServer.mutate({ id: s['.id'], disabled: !disabled, protocol })} title={disabled ? 'Enable' : 'Disable'}
+                        <button onClick={() => {
+                          if (disabled) {
+                            const conflicts = protocol === 'ipv4' ? conflictDevicesV4 : conflictDevicesV6;
+                            if (conflicts.length > 0) {
+                              const names = conflicts.map(d => d.name as string).join(', ');
+                              if (!confirm(`Active ${protocol.toUpperCase()} DHCP server(s) already running on: ${names}.\n\nEnabling another DHCP server on the same network may cause IP address conflicts. Continue?`)) return;
+                            }
+                          }
+                          toggleServer.mutate({ id: s['.id'], disabled: !disabled, protocol });
+                        }} title={disabled ? 'Enable' : 'Disable'}
                          className={clsx('p-1.5 rounded-lg transition-colors', disabled ? 'text-gray-400 hover:text-green-600 hover:bg-green-50 dark:hover:bg-green-900/20' : 'text-green-600 hover:text-amber-600 hover:bg-amber-50 dark:hover:bg-amber-900/20')}>
                          <Power className="w-3.5 h-3.5" />
                        </button>
@@ -499,8 +580,10 @@ export default function NetworkServicesDHCPPage() {
          protocol={serverForm.protocol}
          existing={serverForm.existing}
          pools={serverForm.protocol === 'ipv4' ? (dhcp?.pools_v4 || []) : (dhcp?.pools_v6 || [])}
+          interfaces={dhcp?.interfaces || []}
          deviceId={deviceId}
          onClose={() => setServerForm(null)}
+          conflictDevices={serverForm.protocol === 'ipv4' ? conflictDevicesV4 as { name: string }[] : conflictDevicesV6 as { name: string }[]}
        />
      )}
      {poolForm && <PoolForm protocol={poolForm} deviceId={deviceId} onClose={() => setPoolForm(null)} />}
@@ -545,6 +628,20 @@ export default function NetworkServicesDHCPPage() {
        )}
      </div>

+      {/* Cross-device conflict warnings */}
+      {conflictDevicesV4.length > 0 && deviceId > 0 && (
+        <div className="flex items-start gap-2 p-3 rounded-lg bg-amber-50 dark:bg-amber-900/20 border border-amber-200 dark:border-amber-700 text-sm text-amber-700 dark:text-amber-300">
+          <AlertTriangle className="w-4 h-4 flex-shrink-0 mt-0.5" />
+          <span>Active IPv4 DHCP server(s) already running on: <strong>{conflictDevicesV4.map(d => d.name as string).join(', ')}</strong>. Adding another DHCP server on the same network may cause IP address conflicts.</span>
+        </div>
+      )}
+      {conflictDevicesV6.length > 0 && deviceId > 0 && (
+        <div className="flex items-start gap-2 p-3 rounded-lg bg-amber-50 dark:bg-amber-900/20 border border-amber-200 dark:border-amber-700 text-sm text-amber-700 dark:text-amber-300">
+          <AlertTriangle className="w-4 h-4 flex-shrink-0 mt-0.5" />
+          <span>Active IPv6 DHCP server(s) already running on: <strong>{conflictDevicesV6.map(d => d.name as string).join(', ')}</strong>. Adding another DHCP server on the same network may cause IP address conflicts.</span>
+        </div>
+      )}
+
      {deviceId === 0 && <div className="card p-8 text-center text-sm text-gray-400 dark:text-slate-500">Select a device above.</div>}
      {deviceId > 0 && isLoading && <div className="card p-8 text-center text-sm text-gray-400 dark:text-slate-500">Loading…</div>}
      {deviceId > 0 && error && (
@@ -520,7 +520,7 @@ export const networkServicesApi = {

  // ── DHCP ──────────────────────────────────────────────────────────────────
  getDhcp: (deviceId: number) =>
-    api.get<{ ipv4: NS[]; ipv6: NS[]; pools_v4: NS[]; pools_v6: NS[] }>(
+    api.get<{ ipv4: NS[]; ipv6: NS[]; pools_v4: NS[]; pools_v6: NS[]; interfaces: NS[] }>(
      '/network-services/dhcp', { params: { deviceId } }
    ),
  addDhcpServer: (deviceId: number, body: NS & { protocol: 'ipv4' | 'ipv6' }) =>