From 7be8bc84d3831f9cea7ff62d0964612ad366a976 Mon Sep 17 00:00:00 2001 From: Pujit Mehrotra Date: Mon, 7 Jul 2025 17:14:47 -0400 Subject: [PATCH] fix(connect): mothership connection (#1464) --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1210709463978079 ## Summary by CodeRabbit * **New Features** * Improved management of cloud connection status and error handling for DNS resolution issues. * Introduced a centralized controller for managing mothership connection lifecycle and subscriptions. * **Refactor** * Streamlined event handling and resource management for mothership connections. * Consolidated connection logic to enhance reliability and maintainability. * Optimized initialization process by deferring GraphQL client creation until needed. * **Chores** * Updated module configuration to include the new controller for better dependency management. --- .../src/connection-status/cloud.service.ts | 43 ++++++++------ .../src/mothership-proxy/graphql.client.ts | 1 - .../mothership-proxy/mothership.controller.ts | 59 +++++++++++++++++++ .../src/mothership-proxy/mothership.events.ts | 50 ++++------------ .../src/mothership-proxy/mothership.module.ts | 2 + 5 files changed, 97 insertions(+), 58 deletions(-) create mode 100644 packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts diff --git a/packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts b/packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts index 6ad9f0017..343800665 100644 --- a/packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts +++ b/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 }; } diff --git a/packages/unraid-api-plugin-connect/src/mothership-proxy/graphql.client.ts b/packages/unraid-api-plugin-connect/src/mothership-proxy/graphql.client.ts index 99be2a132..e94e95203 100644 --- a/packages/unraid-api-plugin-connect/src/mothership-proxy/graphql.client.ts +++ b/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 { - await this.createClientInstance(); this.configService.getOrThrow('API_VERSION'); this.configService.getOrThrow('MOTHERSHIP_GRAPHQL_LINK'); } diff --git a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts b/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts new file mode 100644 index 000000000..237479aa3 --- /dev/null +++ b/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(); + } +} diff --git a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts b/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts index 08aac2d26..9d9860802 100644 --- a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.events.ts +++ b/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(); } } diff --git a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts b/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts index ed44a4ab7..48bf6a538 100644 --- a/packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts +++ b/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: [], })