Skip to content

Commit fb02a59

Browse files
committed
fix: stop multi-server workspaces provisioning and routing over each other
Three bugs made a workspace with more than one server unsafe. Each is independently reachable today; together they make the multi-server path unusable, and replication would multiply all three. Managed services were installed on every host. `database` and `redis` are workspace-level, and configForServer spread the whole config onto each scoped copy, so setup's localhost guard fired once per server: a 3-server workspace installed and initialised Postgres three times, same user, same database name. Both now carry an `on` target and are stripped from the servers that do not host them. Cloudflare init repointed every domain at whichever host ran last. The orchestrator iterated the entire workspace app list, so DNS, ingress, firewall lockdown and audit all described apps that live elsewhere; the final CNAME upsert won and the other server's apps 404'd through the tunnel catch-all. It now requires a server-scoped config and the commands fan out. Two consequences fall out of that: sshHostname is one workspace name, so exactly one tunnel may claim it, and an explicit tunnelName shared across several tunnel-needing servers is rejected up front rather than failing halfway through on the missing-credentials guard. Servers were visited in the order they appear in the `servers` literal, and each starts only its own accessories. Declare the app server before the database server and the app deploys and health-checks before the database it depends on exists — reordering two lines silently fixed it. getServerTargets now sorts by dependsOn, declaration order breaking ties. Cycles fall back to declaration order instead of throwing, since two servers can legitimately host accessories the other consumes. The cross-server reachability warning also moves out of --dry-run into the real deploy path, resolved against the whole workspace because a per-server config cannot see other servers' accessories. runRemoteCommandForTargets had no test coverage at all — its mocked config did not even carry a `servers` key. Traversal order, per-server scoping, the includeEmpty skip, connection lifecycle and --app validation are now pinned. The abort-on-failure test documents current behaviour: the try/catch sits outside the loop, so one server failing abandons the rest with no account of what succeeded. That is left deliberately visible for the fleet work to change. One behaviour change beyond the fix: backup reads config.database, so a database is now dumped only on the server that hosts it, instead of every server attempting a pg_dump of a database only one of them has.
1 parent 7d0ca8f commit fb02a59

9 files changed

Lines changed: 523 additions & 42 deletions

File tree

src/cli/commands/cloudflare.ts

Lines changed: 75 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1-
import { runRemoteCommand } from '../runner.js';
1+
import { runRemoteCommandForTargets } from '../runner.js';
22
import { ui } from '../ui.js';
33
import { CloudflareOrchestrator } from '../../services/cloudflare.orchestrator.js';
4+
import { loadConfig } from '../../config/loader.js';
5+
import { getServerTargets, configForServer, resolveServerName } from '../../domain/servers.js';
6+
import type { ShipnodeConfig } from '../../shared/types.js';
47

58
function requireToken(): string {
69
const token = process.env['CLOUDFLARE_API_TOKEN'] ?? process.env['CF_API_TOKEN'];
@@ -11,40 +14,91 @@ function requireToken(): string {
1114
return token;
1215
}
1316

17+
/** Servers that actually host an app with a domain, and so need a tunnel. */
18+
function tunnelledServers(config: ShipnodeConfig): string[] {
19+
return getServerTargets(config)
20+
.map((target) => target.name)
21+
.filter((name) => configForServer(config, name).apps.some((app) => app.domain));
22+
}
23+
24+
/**
25+
* A tunnel name identifies one host's connector, and its credentials live on
26+
* that host. Sharing an explicit name across servers makes the second `init`
27+
* fail on the missing-credentials guard, so reject it up front with a fix
28+
* rather than halfway through the fan-out.
29+
*/
30+
function assertTunnelNameIsUnambiguous(config: ShipnodeConfig): void {
31+
const explicit = config.cloudflare?.tunnelName;
32+
if (!explicit) return;
33+
34+
const servers = tunnelledServers(config);
35+
if (servers.length <= 1) return;
36+
37+
ui.error(
38+
`cloudflare.tunnelName is set to "${explicit}", but ${servers.length} servers host apps with domains ` +
39+
`(${servers.join(', ')}). A tunnel belongs to one host. Remove tunnelName to get a per-host default ` +
40+
`(shipnode-<host>), or run cloudflare init against one server at a time.`,
41+
);
42+
process.exit(1);
43+
}
44+
1445
export async function cmdCloudflareInit(
1546
cwd: string,
1647
options: { config?: string },
1748
): Promise<void> {
18-
await runRemoteCommand(cwd, async ({ config, executor }) => {
19-
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken());
20-
await orchestrator.init();
21-
ui.success(`Cloudflare tunnel initialized.`);
22-
}, { configPath: options.config });
49+
const workspace = await loadConfig(cwd, options.config);
50+
assertTunnelNameIsUnambiguous(workspace);
51+
52+
// Only one tunnel can own the workspace's ssh hostname.
53+
const sshHostnameOwner = resolveServerName(workspace);
54+
55+
await runRemoteCommandForTargets(
56+
cwd,
57+
async ({ config, executor, serverName }) => {
58+
ui.step(`Cloudflare: ${serverName} (${config.ssh.user}@${config.ssh.host})`);
59+
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken(), {
60+
manageSshHostname: serverName === sshHostnameOwner,
61+
});
62+
await orchestrator.init();
63+
ui.success(`Cloudflare tunnel initialized on ${serverName}.`);
64+
},
65+
{ configPath: options.config },
66+
);
2367
}
2468

2569
export async function cmdCloudflareAudit(
2670
cwd: string,
2771
options: { config?: string },
2872
): Promise<void> {
29-
await runRemoteCommand(cwd, async ({ config, executor }) => {
30-
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken());
31-
const result = await orchestrator.audit();
32-
console.log(` Zone: ${result.zone.name} (${result.zone.id}) — ${result.zone.status}`);
33-
for (const app of result.apps) {
34-
console.log(` DNS ${app.domain} → localhost:${app.port}`);
35-
}
36-
console.log(` cloudflared tunnels:\n${result.tunnelList}`);
37-
console.log(` cloudflared service: ${result.service}`);
38-
}, { configPath: options.config });
73+
await runRemoteCommandForTargets(
74+
cwd,
75+
async ({ config, executor, serverName }) => {
76+
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken());
77+
const result = await orchestrator.audit();
78+
ui.heading(`Server: ${serverName} (${config.ssh.user}@${config.ssh.host})`);
79+
console.log(` Zone: ${result.zone.name} (${result.zone.id}) — ${result.zone.status}`);
80+
for (const app of result.apps) {
81+
console.log(` DNS ${app.domain} → localhost:${app.port}`);
82+
}
83+
console.log(` cloudflared tunnels:\n${result.tunnelList}`);
84+
console.log(` cloudflared service: ${result.service}`);
85+
},
86+
{ configPath: options.config },
87+
);
3988
}
4089

4190
export async function cmdCloudflareStatus(
4291
cwd: string,
4392
options: { config?: string },
4493
): Promise<void> {
45-
await runRemoteCommand(cwd, async ({ config, executor }) => {
46-
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken());
47-
const output = await orchestrator.status();
48-
console.log(output);
49-
}, { configPath: options.config });
94+
await runRemoteCommandForTargets(
95+
cwd,
96+
async ({ config, executor, serverName }) => {
97+
const orchestrator = new CloudflareOrchestrator(executor, config, requireToken());
98+
const output = await orchestrator.status();
99+
ui.heading(`Server: ${serverName} (${config.ssh.user}@${config.ssh.host})`);
100+
console.log(output);
101+
},
102+
{ configPath: options.config },
103+
);
50104
}

src/cli/commands/deploy.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ export async function cmdDeploy(cwd: string, options: { dryRun?: boolean; skipBu
5252
return;
5353
}
5454

55+
// The per-server config the callback receives only carries that server's own
56+
// accessories, so cross-server dependencies are invisible from inside it.
57+
// Dependency warnings have to be resolved against the whole workspace.
58+
const workspaceConfig = config;
59+
5560
await runRemoteCommandForTargets(
5661
cwd,
5762
async ({ config, executor, serverName }) => {
@@ -66,6 +71,12 @@ export async function cmdDeploy(cwd: string, options: { dryRun?: boolean; skipBu
6671
const label = names || Object.keys(deployConfig.accessories ?? {}).join(', ');
6772
ui.step(`Deploying ${chalk.bold(label)}${serverName} (${config.ssh.user}@${config.ssh.host})`);
6873

74+
for (const deployingApp of deployConfig.apps) {
75+
for (const warning of renderDependencyWarnings(workspaceConfig, deployingApp)) {
76+
ui.warn(warning);
77+
}
78+
}
79+
6980
const deployer = new DeployService(new LoggingExecutor(executor), deployConfig);
7081
await deployer.execute(cwd, options.skipBuild ?? false);
7182

src/config/schema.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ export const HealthCheckConfigSchema = z.object({
7979
}).default({});
8080

8181
const networkDbFields = {
82+
on: z.string().min(1).optional(),
8283
host: z.string().min(1, 'Database host is required'),
8384
port: z.number().int().min(1).max(65535),
8485
name: z.string().min(1, 'Database name is required'),
@@ -87,13 +88,18 @@ const networkDbFields = {
8788
};
8889

8990
export const DatabaseConfigSchema = z.discriminatedUnion('type', [
90-
z.object({ type: z.literal('sqlite'), name: z.string().min(1, 'SQLite file path is required') }),
91+
z.object({
92+
type: z.literal('sqlite'),
93+
on: z.string().min(1).optional(),
94+
name: z.string().min(1, 'SQLite file path is required'),
95+
}),
9196
z.object({ type: z.literal('postgres'), ...networkDbFields }),
9297
z.object({ type: z.literal('mysql'), ...networkDbFields }),
9398
z.object({ type: z.literal('mongodb'), ...networkDbFields }),
9499
]).optional();
95100

96101
export const RedisConfigSchema = z.object({
102+
on: z.string().min(1).optional(),
97103
host: z.string().default('localhost'),
98104
port: z.number().int().min(1).max(65535).default(6379),
99105
password: z.string().optional(),
@@ -264,6 +270,8 @@ const ShipnodeConfigBaseSchema = z.object({
264270
Object.entries(cfg.accessories ?? {}).forEach(([name, accessory]) => {
265271
validateTarget(accessory.on, ['accessories', name, 'on']);
266272
});
273+
if (cfg.database) validateTarget(cfg.database.on, ['database', 'on']);
274+
if (cfg.redis) validateTarget(cfg.redis.on, ['redis', 'on']);
267275
const accessoryNames = new Set(Object.keys(cfg.accessories ?? {}));
268276
cfg.apps.forEach((app, index) => {
269277
for (const name of app.dependsOn ?? []) {

src/domain/servers.ts

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,69 @@ export function getServerTargetResult(
4949
return Result.ok({ name, ssh });
5050
}
5151

52+
/**
53+
* Which servers must be visited before which, derived from `dependsOn`.
54+
*
55+
* Maps a server to the set of *other* servers it needs first. An app declaring
56+
* `dependsOn: ['postgres']` where postgres lives elsewhere means that
57+
* accessory's host has to be up before this one is deployed and health-checked.
58+
* Same-server dependencies impose no ordering — the orchestrator already
59+
* starts a server's own accessories before its apps.
60+
*/
61+
function serverPrerequisites(config: ShipnodeConfig): Map<string, Set<string>> {
62+
const needs = new Map<string, Set<string>>();
63+
64+
for (const app of config.apps) {
65+
const appServer = resolveServerName(config, app.on);
66+
for (const name of app.dependsOn ?? []) {
67+
const accessory = config.accessories?.[name];
68+
if (!accessory) continue;
69+
const accessoryServer = resolveServerName(config, accessory.on);
70+
if (accessoryServer === appServer) continue;
71+
72+
const existing = needs.get(appServer) ?? new Set<string>();
73+
existing.add(accessoryServer);
74+
needs.set(appServer, existing);
75+
}
76+
}
77+
78+
return needs;
79+
}
80+
81+
/**
82+
* Every server, ordered so that a server hosting an accessory comes before the
83+
* servers whose apps depend on it.
84+
*
85+
* Declaration order is the tiebreak, so a workspace without cross-server
86+
* `dependsOn` is traversed exactly as written. Without this, ordering is purely
87+
* the order of the `servers` literal: declare the app server first and its
88+
* health check runs before the database it needs has been started.
89+
*
90+
* A dependency cycle is not an error — two servers can legitimately host
91+
* accessories the other's apps consume. Cycle members fall back to declaration
92+
* order rather than throwing.
93+
*/
5294
export function getServerTargets(config: ShipnodeConfig): ServerTarget[] {
53-
return Object.entries(config.servers).map(([name, ssh]) => ({ name, ssh }));
95+
const declared = Object.entries(config.servers).map(([name, ssh]) => ({ name, ssh }));
96+
const needs = serverPrerequisites(config);
97+
if (needs.size === 0) return declared;
98+
99+
const ordered: ServerTarget[] = [];
100+
const placed = new Set<string>();
101+
const remaining = [...declared];
102+
103+
while (remaining.length > 0) {
104+
const ready = remaining.findIndex((target) =>
105+
[...(needs.get(target.name) ?? [])].every((dep) => placed.has(dep) || !config.servers[dep]),
106+
);
107+
// -1 means everything left is in a cycle; take the first to stay deterministic.
108+
const [next] = remaining.splice(ready === -1 ? 0 : ready, 1);
109+
if (!next) break;
110+
ordered.push(next);
111+
placed.add(next.name);
112+
}
113+
114+
return ordered;
54115
}
55116

56117
export function getAppsForServer(config: ShipnodeConfig, serverName: string): ShipnodeApp[] {
@@ -63,13 +124,29 @@ export function getAccessoriesForServer(config: ShipnodeConfig, serverName: stri
63124
return Object.fromEntries(entries);
64125
}
65126

127+
/**
128+
* Managed services are provisioned on exactly one server. Returning the config
129+
* verbatim would hand `database`/`redis` to every scoped config, and `setup`
130+
* would then install Postgres once per host — same user, same database name.
131+
*/
132+
function serviceForServer<T extends { on?: string }>(
133+
config: ShipnodeConfig,
134+
service: T | undefined,
135+
serverName: string,
136+
): T | undefined {
137+
if (!service) return undefined;
138+
return resolveServerName(config, service.on) === serverName ? service : undefined;
139+
}
140+
66141
export function configForServer(config: ShipnodeConfig, serverName: string): ShipnodeConfig {
67142
const ssh = getServerTarget(config, serverName).ssh;
68143
return {
69144
...config,
70145
ssh,
71146
apps: getAppsForServer(config, serverName),
72147
accessories: getAccessoriesForServer(config, serverName),
148+
database: serviceForServer(config, config.database, serverName),
149+
redis: serviceForServer(config, config.redis, serverName),
73150
};
74151
}
75152

src/services/cloudflare.orchestrator.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,20 @@ const CFD_INGRESS_TARGET = '.cfargotunnel.com';
99
export class CloudflareOrchestrator {
1010
private api: CloudflareApi;
1111

12+
/**
13+
* `config` must be scoped to the host `executor` is connected to (see
14+
* `configForServer`). Every app in it gets a DNS record pointing at *this*
15+
* host's tunnel, so handing it the whole workspace in a multi-server setup
16+
* repoints other servers' domains at the wrong tunnel.
17+
*
18+
* `manageSshHostname` is false for every server but one: `cloudflare.sshHostname`
19+
* is a single workspace-level name and only one tunnel can own it.
20+
*/
1221
constructor(
1322
private executor: RemoteExecutor,
1423
private config: ShipnodeConfig,
1524
apiToken: string,
25+
private options: { manageSshHostname?: boolean } = {},
1626
) {
1727
this.api = new CloudflareApi(apiToken);
1828
}
@@ -85,7 +95,7 @@ export class CloudflareOrchestrator {
8595
}
8696
}
8797

88-
if (cf.sshHostname) {
98+
if (cf.sshHostname && this.options.manageSshHostname !== false) {
8999
tunnel.addIngress(cf.sshHostname, 'ssh://localhost:22');
90100
await this.api.upsertDnsRecord(zoneId, {
91101
type: 'CNAME',

src/shared/types.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,19 @@ export type DatabaseType = 'postgres' | 'mysql' | 'sqlite' | 'mongodb';
77

88
export interface SqliteDatabaseConfig {
99
type: 'sqlite';
10+
/**
11+
* Which server hosts this database. Provisioning (`shipnode setup`) only runs
12+
* on that server; every other server's scoped config drops the field. Omitted,
13+
* it resolves the same way an app's `on` does — `default`, or the sole server.
14+
*/
15+
on?: string;
1016
name: string;
1117
}
1218

1319
export interface NetworkDatabaseConfig {
1420
type: 'postgres' | 'mysql' | 'mongodb';
21+
/** Which server hosts this database. See {@link SqliteDatabaseConfig.on}. */
22+
on?: string;
1523
host: string;
1624
port: number;
1725
name: string;
@@ -96,6 +104,8 @@ export interface HealthCheckConfig {
96104
export type DatabaseConfig = SqliteDatabaseConfig | NetworkDatabaseConfig;
97105

98106
export interface RedisConfig {
107+
/** Which server hosts Redis. See {@link SqliteDatabaseConfig.on}. */
108+
on?: string;
99109
host: string;
100110
port: number;
101111
password?: string;

0 commit comments

Comments
 (0)