Skip to content

Commit 7be8bc8

Browse files
pujitmpujitm
authored
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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 4d97e14 commit 7be8bc8

File tree

5 files changed

+97
-58
lines changed

5 files changed

+97
-58
lines changed

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

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -213,29 +213,36 @@ export class CloudService {
213213
resolve(hostname).then(([address]) => address),
214214
]);
215215

216-
if (!local.includes(network)) {
217-
// Question: should we actually throw an error, or just log a warning?
218-
//
219-
// This is usually due to cloudflare's load balancing.
220-
// if `dig +short mothership.unraid.net` shows both IPs, then this should be safe to ignore.
221-
// this.logger.warn(
222-
// `Local and network resolvers showing different IP for "${hostname}". [local="${
223-
// local ?? 'NOT FOUND'
224-
// }"] [network="${network ?? 'NOT FOUND'}"].`
225-
// );
226-
216+
/**
217+
* If either resolver returns a private IP we still treat this as a fatal
218+
* mis-configuration because the host will be unreachable from the public
219+
* Internet.
220+
*
221+
* The user likely has a PI-hole or something similar running that rewrites
222+
* the record to a private address.
223+
*/
224+
if (ip.isPrivate(local) || ip.isPrivate(network)) {
227225
throw new Error(
228-
`Local and network resolvers showing different IP for "${hostname}". [local="${
229-
local ?? 'NOT FOUND'
230-
}"] [network="${network ?? 'NOT FOUND'}"]`
226+
`"${hostname}" is being resolved to a private IP. [local="${local ?? 'NOT FOUND'}"] [network="${
227+
network ?? 'NOT FOUND'
228+
}"]`
231229
);
232230
}
233231

234-
// The user likely has a PI-hole or something similar running.
235-
if (ip.isPrivate(local))
236-
throw new Error(
237-
`"${hostname}" is being resolved to a private IP. [IP=${local ?? 'NOT FOUND'}]`
232+
/**
233+
* Different public IPs are expected when Cloudflare (or anycast) load-balancing
234+
* is in place. Log the mismatch for debugging purposes but do **not** treat it
235+
* as an error.
236+
*
237+
* It does not affect whether the server can connect to Mothership.
238+
*/
239+
if (local !== network) {
240+
this.logger.debug(
241+
`Local and network resolvers returned different IPs for "${hostname}". [local="${local ?? 'NOT FOUND'}"] [network="${
242+
network ?? 'NOT FOUND'
243+
}"]`
238244
);
245+
}
239246

240247
return { local, network };
241248
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ export class MothershipGraphqlClientService implements OnModuleInit, OnModuleDes
5757
* Initialize the GraphQL client when the module is created
5858
*/
5959
async onModuleInit(): Promise<void> {
60-
await this.createClientInstance();
6160
this.configService.getOrThrow('API_VERSION');
6261
this.configService.getOrThrow('MOTHERSHIP_GRAPHQL_LINK');
6362
}

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import { Injectable, Logger, OnApplicationBootstrap, OnModuleDestroy } from '@nestjs/common';
2+
3+
import { TimeoutCheckerJob } from '../connection-status/timeout-checker.job.js';
4+
import { MothershipConnectionService } from './connection.service.js';
5+
import { MothershipGraphqlClientService } from './graphql.client.js';
6+
import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
7+
8+
/**
9+
* Controller for (starting and stopping) the mothership stack:
10+
* - GraphQL client (to mothership)
11+
* - Subscription handler (websocket communication with mothership)
12+
* - Timeout checker (to detect if the connection to mothership is lost)
13+
* - Connection service (controller for connection state & metadata)
14+
*/
15+
@Injectable()
16+
export class MothershipController implements OnModuleDestroy, OnApplicationBootstrap {
17+
private readonly logger = new Logger(MothershipController.name);
18+
constructor(
19+
private readonly clientService: MothershipGraphqlClientService,
20+
private readonly connectionService: MothershipConnectionService,
21+
private readonly subscriptionHandler: MothershipSubscriptionHandler,
22+
private readonly timeoutCheckerJob: TimeoutCheckerJob
23+
) {}
24+
25+
async onModuleDestroy() {
26+
await this.stop();
27+
}
28+
29+
async onApplicationBootstrap() {
30+
await this.initOrRestart();
31+
}
32+
33+
/**
34+
* Stops the mothership stack. Throws on first error.
35+
*/
36+
async stop() {
37+
this.timeoutCheckerJob.stop();
38+
this.subscriptionHandler.stopMothershipSubscription();
39+
await this.clientService.clearInstance();
40+
this.connectionService.resetMetadata();
41+
this.subscriptionHandler.clearAllSubscriptions();
42+
}
43+
44+
/**
45+
* Attempts to stop, then starts the mothership stack. Throws on first error.
46+
*/
47+
async initOrRestart() {
48+
await this.stop();
49+
const { state } = this.connectionService.getIdentityState();
50+
this.logger.verbose('cleared, got identity state');
51+
if (!state.apiKey) {
52+
this.logger.warn('No API key found; cannot setup mothership subscription');
53+
return;
54+
}
55+
await this.clientService.createClientInstance();
56+
await this.subscriptionHandler.subscribeToMothershipEvents();
57+
this.timeoutCheckerJob.start();
58+
}
59+
}

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

Lines changed: 11 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,44 @@
1-
import { Inject, Injectable, Logger, OnModuleDestroy } from '@nestjs/common';
1+
import { Inject, Injectable, Logger } from '@nestjs/common';
22
import { OnEvent } from '@nestjs/event-emitter';
33

44
import { PubSub } from 'graphql-subscriptions';
55

66
import { MinigraphStatus } from '../config/connect.config.js';
7-
import { TimeoutCheckerJob } from '../connection-status/timeout-checker.job.js';
87
import { EVENTS, GRAPHQL_PUBSUB_CHANNEL, GRAPHQL_PUBSUB_TOKEN } from '../helper/nest-tokens.js';
98
import { MothershipConnectionService } from './connection.service.js';
10-
import { MothershipGraphqlClientService } from './graphql.client.js';
11-
import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
9+
import { MothershipController } from './mothership.controller.js';
1210

1311
@Injectable()
14-
export class MothershipHandler implements OnModuleDestroy {
12+
export class MothershipHandler {
1513
private readonly logger = new Logger(MothershipHandler.name);
1614
constructor(
1715
private readonly connectionService: MothershipConnectionService,
18-
private readonly clientService: MothershipGraphqlClientService,
19-
private readonly subscriptionHandler: MothershipSubscriptionHandler,
20-
private readonly timeoutCheckerJob: TimeoutCheckerJob,
16+
private readonly mothershipController: MothershipController,
2117
@Inject(GRAPHQL_PUBSUB_TOKEN)
2218
private readonly legacyPubSub: PubSub
2319
) {}
2420

25-
async onModuleDestroy() {
26-
await this.clear();
27-
}
28-
29-
async clear() {
30-
this.timeoutCheckerJob.stop();
31-
this.subscriptionHandler.stopMothershipSubscription();
32-
await this.clientService.clearInstance();
33-
this.connectionService.resetMetadata();
34-
this.subscriptionHandler.clearAllSubscriptions();
35-
}
36-
37-
async setup() {
38-
await this.clear();
39-
const { state } = this.connectionService.getIdentityState();
40-
this.logger.verbose('cleared, got identity state');
41-
if (!state.apiKey) {
42-
this.logger.warn('No API key found; cannot setup mothership subscription');
43-
return;
44-
}
45-
await this.clientService.createClientInstance();
46-
await this.subscriptionHandler.subscribeToMothershipEvents();
47-
this.timeoutCheckerJob.start();
48-
}
49-
5021
@OnEvent(EVENTS.IDENTITY_CHANGED, { async: true })
5122
async onIdentityChanged() {
5223
const { state } = this.connectionService.getIdentityState();
5324
if (state.apiKey) {
5425
this.logger.verbose('Identity changed; setting up mothership subscription');
55-
await this.setup();
26+
await this.mothershipController.initOrRestart();
5627
}
5728
}
5829

5930
@OnEvent(EVENTS.MOTHERSHIP_CONNECTION_STATUS_CHANGED, { async: true })
6031
async onMothershipConnectionStatusChanged() {
6132
const state = this.connectionService.getConnectionState();
62-
// Question: do we include MinigraphStatus.ERROR_RETRYING here?
63-
if (state && [MinigraphStatus.PING_FAILURE].includes(state.status)) {
33+
if (
34+
state &&
35+
[MinigraphStatus.PING_FAILURE, MinigraphStatus.ERROR_RETRYING].includes(state.status)
36+
) {
6437
this.logger.verbose(
6538
'Mothership connection status changed to %s; setting up mothership subscription',
6639
state.status
6740
);
68-
await this.setup();
41+
await this.mothershipController.initOrRestart();
6942
}
7043
}
7144

@@ -84,7 +57,6 @@ export class MothershipHandler implements OnModuleDestroy {
8457
await this.legacyPubSub.publish(GRAPHQL_PUBSUB_CHANNEL.OWNER, {
8558
owner: { username: 'root', url: '', avatar: '' },
8659
});
87-
this.timeoutCheckerJob.stop();
88-
await this.clear();
60+
await this.mothershipController.stop();
8961
}
9062
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { MothershipConnectionService } from './connection.service.js';
1010
import { MothershipGraphqlClientService } from './graphql.client.js';
1111
import { MothershipSubscriptionHandler } from './mothership-subscription.handler.js';
1212
import { MothershipHandler } from './mothership.events.js';
13+
import { MothershipController } from './mothership.controller.js';
1314

1415
@Module({
1516
imports: [RemoteAccessModule],
@@ -23,6 +24,7 @@ import { MothershipHandler } from './mothership.events.js';
2324
TimeoutCheckerJob,
2425
CloudService,
2526
CloudResolver,
27+
MothershipController,
2628
],
2729
exports: [],
2830
})

0 commit comments

Comments
 (0)