fix(connect): mothership connection

packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts

@@ -213,29 +213,36 @@ export class CloudService {
             resolve(hostname).then(([address]) => address),
         ]);
 
-        if (!local.includes(network)) {
-            // Question: should we actually throw an error, or just log a warning?
-            //
-            // This is usually due to cloudflare's load balancing.
-            // if `dig +short mothership.unraid.net` shows both IPs, then this should be safe to ignore.
-            // this.logger.warn(
-            //     `Local and network resolvers showing different IP for "${hostname}". [local="${
-            //         local ?? 'NOT FOUND'
-            //     }"] [network="${network ?? 'NOT FOUND'}"].`
-            // );
-
+        /**
+         * If either resolver returns a private IP we still treat this as a fatal
+         * mis-configuration because the host will be unreachable from the public
+         * Internet.
+         *
+         * The user likely has a PI-hole or something similar running that rewrites
+         * the record to a private address.
+         */
+        if (ip.isPrivate(local) || ip.isPrivate(network)) {
             throw new Error(
-                `Local and network resolvers showing different IP for "${hostname}". [local="${
-                    local ?? 'NOT FOUND'
-                }"] [network="${network ?? 'NOT FOUND'}"]`
+                `"${hostname}" is being resolved to a private IP. [local="${local ?? 'NOT FOUND'}"] [network="${
+                    network ?? 'NOT FOUND'
+                }"]`
             );
         }
 
-        // The user likely has a PI-hole or something similar running.
-        if (ip.isPrivate(local))
-            throw new Error(
-                `"${hostname}" is being resolved to a private IP. [IP=${local ?? 'NOT FOUND'}]`
+        /**
+         * Different public IPs are expected when Cloudflare (or anycast) load-balancing
+         * is in place. Log the mismatch for debugging purposes but do **not** treat it
+         * as an error.
+         *
+         * It does not affect whether the server can connect to Mothership.
+         */
+        if (local !== network) {
+            this.logger.debug(
+                `Local and network resolvers returned different IPs for "${hostname}". [local="${local ?? 'NOT FOUND'}"] [network="${
+                    network ?? 'NOT FOUND'
+                }"]`
             );
+        }
 
         return { local, network };
     }

packages/unraid-api-plugin-connect/src/mothership-proxy/graphql.client.ts

@@ -57,7 +57,6 @@ export class MothershipGraphqlClientService implements OnModuleInit, OnModuleDes
      * Initialize the GraphQL client when the module is created
      */
     async onModuleInit(): Promise<void> {
-        await this.createClientInstance();
         this.configService.getOrThrow('API_VERSION');
         this.configService.getOrThrow('MOTHERSHIP_GRAPHQL_LINK');
     }

packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts

@@ -0,0 +1,59 @@
+import { Injectable, Logger, OnApplicationBootstrap, OnModuleDestroy } from '@nestjs/common';
+
+import { TimeoutCheckerJob } from '../connection-status/timeout-checker.job.js';
+import { MothershipConnectionService } from './connection.service.js';
+import { MothershipGraphqlClientService } from './graphql.client.js';
+import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
+
+/**
+ * Controller for (starting and stopping) the mothership stack:
+ * - GraphQL client (to mothership)
+ * - Subscription handler (websocket communication with mothership)
+ * - Timeout checker (to detect if the connection to mothership is lost)
+ * - Connection service (controller for connection state & metadata)
+ */
+@Injectable()
+export class MothershipController implements OnModuleDestroy, OnApplicationBootstrap {
+    private readonly logger = new Logger(MothershipController.name);
+    constructor(
+        private readonly clientService: MothershipGraphqlClientService,
+        private readonly connectionService: MothershipConnectionService,
+        private readonly subscriptionHandler: MothershipSubscriptionHandler,
+        private readonly timeoutCheckerJob: TimeoutCheckerJob
+    ) {}
+
+    async onModuleDestroy() {
+        await this.stop();
+    }
+
+    async onApplicationBootstrap() {
+        await this.initOrRestart();
+    }
+
+    /**
+     * Stops the mothership stack. Throws on first error.
+     */
+    async stop() {
+        this.timeoutCheckerJob.stop();
+        this.subscriptionHandler.stopMothershipSubscription();
+        await this.clientService.clearInstance();
+        this.connectionService.resetMetadata();
+        this.subscriptionHandler.clearAllSubscriptions();
+    }
+
+    /**
+     * Attempts to stop, then starts the mothership stack. Throws on first error.
+     */
+    async initOrRestart() {
+        await this.stop();
+        const { state } = this.connectionService.getIdentityState();
+        this.logger.verbose('cleared, got identity state');
+        if (!state.apiKey) {
+            this.logger.warn('No API key found; cannot setup mothership subscription');
+            return;
+        }
+        await this.clientService.createClientInstance();
+        await this.subscriptionHandler.subscribeToMothershipEvents();
+        this.timeoutCheckerJob.start();
+    }
+}

packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts

@@ -1,71 +1,44 @@
-import { Inject, Injectable, Logger, OnModuleDestroy } from '@nestjs/common';
+import { Inject, Injectable, Logger } from '@nestjs/common';
 import { OnEvent } from '@nestjs/event-emitter';
 
 import { PubSub } from 'graphql-subscriptions';
 
 import { MinigraphStatus } from '../config/connect.config.js';
-import { TimeoutCheckerJob } from '../connection-status/timeout-checker.job.js';
 import { EVENTS, GRAPHQL_PUBSUB_CHANNEL, GRAPHQL_PUBSUB_TOKEN } from '../helper/nest-tokens.js';
 import { MothershipConnectionService } from './connection.service.js';
-import { MothershipGraphqlClientService } from './graphql.client.js';
-import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
+import { MothershipController } from './mothership.controller.js';
 
 @Injectable()
-export class MothershipHandler implements OnModuleDestroy {
+export class MothershipHandler {
     private readonly logger = new Logger(MothershipHandler.name);
     constructor(
         private readonly connectionService: MothershipConnectionService,
-        private readonly clientService: MothershipGraphqlClientService,
-        private readonly subscriptionHandler: MothershipSubscriptionHandler,
-        private readonly timeoutCheckerJob: TimeoutCheckerJob,
+        private readonly mothershipController: MothershipController,
         @Inject(GRAPHQL_PUBSUB_TOKEN)
         private readonly legacyPubSub: PubSub
     ) {}
 
-    async onModuleDestroy() {
-        await this.clear();
-    }
-
-    async clear() {
-        this.timeoutCheckerJob.stop();
-        this.subscriptionHandler.stopMothershipSubscription();
-        await this.clientService.clearInstance();
-        this.connectionService.resetMetadata();
-        this.subscriptionHandler.clearAllSubscriptions();
-    }
-
-    async setup() {
-        await this.clear();
-        const { state } = this.connectionService.getIdentityState();
-        this.logger.verbose('cleared, got identity state');
-        if (!state.apiKey) {
-            this.logger.warn('No API key found; cannot setup mothership subscription');
-            return;
-        }
-        await this.clientService.createClientInstance();
-        await this.subscriptionHandler.subscribeToMothershipEvents();
-        this.timeoutCheckerJob.start();
-    }
-
     @OnEvent(EVENTS.IDENTITY_CHANGED, { async: true })
     async onIdentityChanged() {
         const { state } = this.connectionService.getIdentityState();
         if (state.apiKey) {
             this.logger.verbose('Identity changed; setting up mothership subscription');
-            await this.setup();
+            await this.mothershipController.initOrRestart();
         }
     }
 
     @OnEvent(EVENTS.MOTHERSHIP_CONNECTION_STATUS_CHANGED, { async: true })
     async onMothershipConnectionStatusChanged() {
         const state = this.connectionService.getConnectionState();
-        // Question: do we include MinigraphStatus.ERROR_RETRYING here?
-        if (state && [MinigraphStatus.PING_FAILURE].includes(state.status)) {
+        if (
+            state &&
+            [MinigraphStatus.PING_FAILURE, MinigraphStatus.ERROR_RETRYING].includes(state.status)
+        ) {
             this.logger.verbose(
                 'Mothership connection status changed to %s; setting up mothership subscription',
                 state.status
             );
-            await this.setup();
+            await this.mothershipController.initOrRestart();
         }
     }
 
@@ -84,7 +57,6 @@ export class MothershipHandler implements OnModuleDestroy {
         await this.legacyPubSub.publish(GRAPHQL_PUBSUB_CHANNEL.OWNER, {
             owner: { username: 'root', url: '', avatar: '' },
         });
-        this.timeoutCheckerJob.stop();
-        await this.clear();
+        await this.mothershipController.stop();
     }
 }

packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts

@@ -10,6 +10,7 @@ import { MothershipConnectionService } from './connection.service.js';
 import { MothershipGraphqlClientService } from './graphql.client.js';
 import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
 import { MothershipHandler } from './mothership.events.js';
+import { MothershipController } from './mothership.controller.js';
 
 @Module({
     imports: [RemoteAccessModule],
@@ -23,6 +24,7 @@ import { MothershipHandler } from './mothership.events.js';
         TimeoutCheckerJob,
         CloudService,
         CloudResolver,
+        MothershipController,
     ],
     exports: [],
 })