From e6490eae5c60259b0bcc5a9bdfea1f93e2df7203 Mon Sep 17 00:00:00 2001 From: Hydra Date: Wed, 19 Aug 2026 19:57:01 +0300 Subject: [PATCH] fix(connections): resolve the machine a project runs on instead of comparing serverId columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regressions in 26ddd69f, both in the reachability gate it added. The co-location check compared `(source.serverId ?? null) !== (target.serverId ?? null)`, which is not a same-box test. `project.serverId` is written only in the deploy success tail (deployment-lifecycle) and never on a local deploy, so it is null for every project that has not completed a server-bound deploy — and the codebase holds that a derived `local` target, the auto-registered isLocal "This Server" row, and a plain loopback/SERVER_IP row for this host are ONE machine (`resolveTargetPlatform`, `isLocalHostRow`, `resolvesToLocalHost`): one docker daemon, therefore one set of `openship-` networks. The check read three encodings of one machine as three machines, so it refused the mainline self-hosted pair — a catalog app bound to the "This Server" row wired into a project that deploys local — and refused connect-before-deploy, which is what the app-install wizard does ("Wire declared connections BEFORE deploy so the injected env is present"). An explicit `mode: "internal"` got a 400 where it used to work; a defaulted one silently downgraded to public. The cloud check was swapped from `activeDeployment.meta.deployTarget === "cloud"` to `deriveProjectDeployTarget`, i.e. `cloudWorkspaceId` alone. deployment-lifecycle DELIBERATELY skips `setCloudWorkspaceId` for a local-orchestrated cloud deploy (`!CLOUD_MODE && deployTarget === "cloud" && buildStrategy === "local"`) because "the project MUST stay local-canonical", and the serverId persist below it is gated on `deployTarget !== "cloud"` — so for that shape BOTH columns are null by design, the gate went silent, and the serverId comparison then answered "same box". A swap where the repo already takes a union (project-resources.service: `deployTarget === "cloud" || !!cloudWorkspaceId`). `resolveProjectHost` now answers "which machine" once, for both ends: snapshot serverId first and durable column second — the order `readDeployMeta` uses, because the snapshot is where the live release actually runs and an alias only resolves next to the running container — with cloud as the union of both signals and every encoding of this box collapsed to one identity. `resolveSnapshotTarget` and `countActiveByServer` invert that order deliberately; they answer where the NEXT deploy would go, which is not this question. A project bound to nothing and never deployed resolves to `box`, not to a "don't know": `resolveSnapshotTarget` resolves exactly that shape to the host default, so its first deploy lands here. An earlier cut of this fix reported it as unknown and skipped the comparison, which the two callers that never asked for internal — the object-storage bind's mode chooser and the scope-derived default — read as "internal is available". That handed a remote source's container alias to a project about to deploy elsewhere, and a wrong guess is invisible because attachLinkedNetworks only warns. Now an unbound target is co-located with a source on this box (allowed, the wizard's case) and NOT with a source on a remote server (refused, steered to Public — which is the endpoint probeBucket just verified). Hosts are resolved only when the value carries a host, so the GH-631 credential path (a JWT, a password, a bucket name) still does no extra queries. Known gap, not fixed here: a multi-server org installing an app whose declared requirement asks explicitly for internal, where the wizard will deploy the target to the SAME remote server as the source, is now refused rather than allowed — the wizard holds `destination.serverId` twelve lines from the bundle call but does not pass it in. No shipped catalog app declares `requires`, so this is latent for custom templates; plumbing that destination through is the follow-up. The new tests fail against 26ddd69f and pass here; the eight pre-existing ones are unchanged in both, which is what makes them a regression pin rather than a rewrite. --- .../projects/project-connection.service.ts | 92 ++++++++++--- .../connection-internal-synth.test.ts | 130 +++++++++++++++++- 2 files changed, 197 insertions(+), 25 deletions(-) diff --git a/apps/api/src/modules/projects/project-connection.service.ts b/apps/api/src/modules/projects/project-connection.service.ts index 900eec35a..5e039ed15 100644 --- a/apps/api/src/modules/projects/project-connection.service.ts +++ b/apps/api/src/modules/projects/project-connection.service.ts @@ -16,12 +16,12 @@ import { getAppEndpoints, getAppConnection, getOutputPort, - deriveProjectDeployTarget, type AppTemplate, } from "@repo/core"; import { getTemplateForOrg } from "../apps/catalog-source"; import type { RequestContext } from "../../lib/request-context"; import { assertResourceInOrg } from "../../lib/controller-helpers"; +import { isLocalHostRow } from "../../lib/box-org"; import { permission } from "../../lib/permission"; import { getAppConnectionView, type AppConnectionOutput } from "../apps/app-settings.service"; import { mergeEnvVars } from "./project-env.service"; @@ -234,6 +234,63 @@ async function loadConnectionEnds( return { source, target, output, template, declaredPort: spec ? getOutputPort(spec) : null }; } +/** + * WHICH MACHINE a project's workload sits on, collapsed so that every encoding of + * "this box" compares equal. + * + * `box` covers all of them deliberately: a project with no server binding deploys to + * the host docker socket, and so does the auto-registered isLocal "This Server" row + * AND a plain loopback/SERVER_IP row for this host (deployment-runtime + * `resolveTargetPlatform`, `isLocalHostRow`). One daemon means one set of + * `openship-` networks, so treating those as different machines refuses pairs + * that are demonstrably co-located. + */ +type ProjectHost = { kind: "cloud" } | { kind: "box" } | { kind: "server"; id: string }; + +const hostKey = (h: ProjectHost): string => (h.kind === "server" ? `server:${h.id}` : h.kind); + +/** + * Resolve which machine a project's workload sits on. + * + * Snapshot serverId FIRST, durable column second — the order `readDeployMeta` uses, + * and for its reason: the snapshot is where the live release ACTUALLY runs while the + * column is where the project is bound, and an alias only resolves next to the + * running container. (`resolveSnapshotTarget` and `countActiveByServer` deliberately + * invert this — they answer "where would the NEXT deploy go", a different question.) + * + * Cloud is the UNION of both signals, mirroring project-resources.service: the + * `cloudWorkspaceId` column alone is not enough, because a self-hosted instance + * orchestrating a cloud deploy deliberately leaves it null to stay local-canonical + * (deployment-lifecycle, `isLocalOrchestratedCloud`) — for that shape the snapshot is + * the only cloud signal there is, and reading the column alone declares it local. + * + * A project bound to nothing and never deployed is NOT unknown: `resolveSnapshotTarget` + * resolves exactly that shape to the host default, so its first deploy lands on this + * box and `box` is the honest answer. Reporting it as "no idea" instead read as + * "nothing to refuse" at the call sites, which handed a remote source's alias to a + * project that was about to deploy somewhere else entirely. + */ +async function resolveProjectHost(project: Project): Promise { + const dep = project.activeDeploymentId + ? await repos.deployment.findById(project.activeDeploymentId).catch(() => null) + : null; + const meta = (dep?.meta ?? null) as { deployTarget?: string; serverId?: string } | null; + if (meta?.deployTarget === "cloud" || project.cloudWorkspaceId) return { kind: "cloud" }; + + const serverId = meta?.serverId ?? project.serverId ?? null; + if (!serverId) return { kind: "box" }; + + const row = await repos.server + .getInOrganization(serverId, project.organizationId) + .catch(() => null); + // A row we cannot read is not PROVABLY this box, so it stays its own machine — the + // failure mode of guessing "local" here is a dead alias, which is what this check + // exists to prevent. + return row && (await isLocalHostRow(row).catch(() => false)) + ? { kind: "box" } + : { kind: "server", id: serverId }; +} + /** The east-west value an internal connection should inject, or why it can't. */ type InternalResolution = { value: string } | { error: string }; @@ -243,31 +300,30 @@ type InternalResolution = { value: string } | { error: string }; * infer availability from WHICH error `createConnection` happened to throw first — * so the answer depended on the shape of an unrelated output's value. */ -function resolveInternalValue(ends: ConnectionEnds): InternalResolution { +async function resolveInternalValue(ends: ConnectionEnds): Promise { const { source, target, output, template, declaredPort } = ends; // GH-631: a non-URL value (password, token, API key, bucket name) names no host, // so internal mode has nothing to rewrite and nothing to reach — inject it // verbatim. Answered BEFORE reachability on purpose: a credential is equally - // valid whichever box the source runs on, and gating it on network topology - // would re-break the case that was reported. + // valid whichever box the source runs on, gating it on network topology would + // re-break the case that was reported, and resolving hosts costs queries this + // path has no use for. if (!isNetworkUrl(output.value)) return { value: output.value }; // Everything below hands the consumer a HOST to reach east-west, which rides on - // joining the source app's docker network at deploy (attachLinkedNetworks). - // Those networks are per-HOST, so both ends must land on the same box: a - // cloud-hosted project has no attachable shared network on this pipe yet, and - // one server cannot see another's — the alias would resolve nowhere, and the - // attach only WARNS, so the deploy goes green around a dead host. `serverId` is - // the durable binding behind `deriveProjectDeployTarget`; the per-deployment - // snapshot is re-derived, and a deploy that failed to would silently pass here. - if ( - deriveProjectDeployTarget(source) === "cloud" || - deriveProjectDeployTarget(target) === "cloud" - ) { + // joining the source app's docker network at deploy (attachLinkedNetworks). Those + // networks are per-machine and a failed attach only WARNS, so a link across two + // machines injects an alias that resolves nowhere and still deploys green. + const [sourceHost, targetHost] = await Promise.all([ + resolveProjectHost(source), + resolveProjectHost(target), + ]); + + if (sourceHost.kind === "cloud" || targetHost.kind === "cloud") { return { error: "Internal mode isn't available for a cloud-hosted app yet — use Public." }; } - if ((source.serverId ?? null) !== (target.serverId ?? null)) { + if (hostKey(sourceHost) !== hostKey(targetHost)) { return { error: "Internal mode needs both projects on the same server — they're on different servers, so use Public.", @@ -310,7 +366,7 @@ export async function internalModeAvailable( input.sourceProjectId, input.outputId, ); - return !("error" in resolveInternalValue(ends)); + return !("error" in (await resolveInternalValue(ends))); } export async function createConnection( @@ -350,7 +406,7 @@ export async function createConnection( let value = output.value; if (mode === "internal") { - const resolved = resolveInternalValue(ends); + const resolved = await resolveInternalValue(ends); if ("error" in resolved) { // An EXPLICIT internal ask gets the reason. A DEFAULTED one falls back to // public — failing a request that never asked for internal turns a working diff --git a/apps/api/test/modules/projects/connection-internal-synth.test.ts b/apps/api/test/modules/projects/connection-internal-synth.test.ts index 0d1bf4bd1..d425c8ee0 100644 --- a/apps/api/test/modules/projects/connection-internal-synth.test.ts +++ b/apps/api/test/modules/projects/connection-internal-synth.test.ts @@ -6,15 +6,20 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; * (`http://:`), tagged `internal: true`. `createConnection` must * inject that value VERBATIM in internal mode — never route it through * `toInternalUrl`, which needs a template and would return null (killing the - * link). The co-location guard still fires first, so a cloud-hosted source — or - * one on a different server, since `openship-` networks are per-host — is - * steered to Public even when it carries a synthesized internal value. + * link). The co-location guard still fires first, so a cloud-hosted source — or one + * on a genuinely different machine, since `openship-` networks are per-machine + * — is steered to Public even when it carries a synthesized internal value. What + * counts as "a different machine" is the delicate part, and most of the cases below + * are about the pairs that are NOT: every encoding of this box is one machine, and a + * project with no destination yet is not a second one. */ const h = vi.hoisted(() => ({ findById: vi.fn(), listEnvVars: vi.fn(), deploymentFindById: vi.fn(), + serverGetInOrganization: vi.fn(), + isLocalHostRow: vi.fn(), listByTarget: vi.fn(), upsert: vi.fn(), getTemplateForOrg: vi.fn(), @@ -27,10 +32,15 @@ vi.mock("@repo/db", () => ({ repos: { project: { findById: h.findById, listEnvVars: h.listEnvVars }, deployment: { findById: h.deploymentFindById }, + server: { getInOrganization: h.serverGetInOrganization }, projectConnection: { listByTarget: h.listByTarget, upsert: h.upsert }, }, })); +vi.mock("../../../src/lib/box-org", () => ({ + isLocalHostRow: h.isLocalHostRow, +})); + vi.mock("../../../src/modules/apps/catalog-source", () => ({ getTemplateForOrg: h.getTemplateForOrg, })); @@ -93,6 +103,9 @@ beforeEach(() => { h.listEnvVars.mockResolvedValue([]); h.listByTarget.mockResolvedValue([]); h.deploymentFindById.mockResolvedValue(null); + // Default: a server row that is NOT this box, so a serverId means a real remote. + h.serverGetInOrganization.mockImplementation(async (id: string) => ({ id, isLocal: false })); + h.isLocalHostRow.mockImplementation(async (row: { isLocal?: boolean }) => !!row?.isLocal); h.toInternalUrl.mockReturnValue(null); h.upsert.mockImplementation(async (row: Record) => ({ ...row, id: "conn_new" })); h.mergeEnvVars.mockResolvedValue(undefined); @@ -122,10 +135,9 @@ describe("createConnection — synthesized internal source", () => { }); it("still steers a CLOUD-hosted source to Public before injecting anything", async () => { - // Read from the DURABLE binding (`cloudWorkspaceId`, via - // deriveProjectDeployTarget), not the per-deployment meta snapshot: a project - // bound to cloud that hasn't redeployed has no snapshot to read, and used to - // sail through this guard. + // The DURABLE half of the union: a project bound to cloud that hasn't redeployed + // has no snapshot to read. The snapshot half is pinned separately below — + // neither signal alone catches every cloud shape. h.findById.mockImplementation(async (id: string) => id === "app-a" ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null } @@ -169,6 +181,110 @@ describe("createConnection — synthesized internal source", () => { expect(h.upsert).not.toHaveBeenCalled(); }); + it("treats the isLocal 'This Server' row and an unbound-but-deployed project as ONE machine", async () => { + // The regression this pins: comparing raw serverId columns called these two + // different machines. They are the same docker daemon — a project with no server + // binding deploys to the host socket, and so does the isLocal row + // (deployment-runtime `resolveTargetPlatform`) — so the same + // `openship-` networks are reachable and internal MUST be allowed. + h.serverGetInOrganization.mockImplementation(async (id: string) => ({ id, isLocal: true })); + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: "dep-a", serverId: null } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: "dep-c", serverId: "srv-local" }, + ); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ); + + expect(res.connection.mode).toBe("internal"); + expect(h.mergeEnvVars).toHaveBeenCalledWith( + "app-a", + "org1", + expect.objectContaining({ + upserts: [{ key: "DB_URL", value: "http://my-app:8080", isSecret: true }], + }), + ); + }); + + it("allows connect-before-deploy when the source is on THIS box", async () => { + // The app-install wizard wires declared connections BEFORE the first deploy. A + // project bound to nothing is not unknown — resolveSnapshotTarget resolves that + // exact shape to the host default — so its first deploy lands next to a source + // that is already here, and refusing would reject a co-located pair. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: null } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: "dep-c", serverId: null }, + ); + + const res = await createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ); + + expect(res.connection.mode).toBe("internal"); + }); + + it("still refuses connect-before-deploy when the source is on a REMOTE server", async () => { + // The other half, and the one that makes "no binding yet" a real answer rather + // than an excuse to skip the check: an unbound target deploys HERE by default, so + // a source on server A is a different machine and its alias would resolve nowhere. + // Treating the unbound end as "nothing to refuse" injected that dead alias and + // reported success. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: null } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: "dep-c", serverId: "srv-a" }, + ); + + await expect( + createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ), + ).rejects.toThrow(/same server/i); + + expect(h.mergeEnvVars).not.toHaveBeenCalled(); + expect(h.upsert).not.toHaveBeenCalled(); + }); + + it("catches a cloud source from the deployment SNAPSHOT when cloudWorkspaceId is null", async () => { + // A self-hosted instance orchestrating a cloud deploy deliberately leaves + // `cloudWorkspaceId` null to stay local-canonical (deployment-lifecycle, + // isLocalOrchestratedCloud), so the column alone reads "local" and the guard went + // silent. `meta.deployTarget` is that shape's only cloud signal. + h.findById.mockImplementation(async (id: string) => + id === "app-a" + ? { id: "app-a", name: "App A", slug: "app-a", organizationId: "org1", activeDeploymentId: null, serverId: null } + : { id: "db-c", name: "Plain App", slug: "plain-app", organizationId: "org1", appTemplateId: null, activeDeploymentId: "dep-c", serverId: null, cloudWorkspaceId: null }, + ); + h.deploymentFindById.mockResolvedValue({ + id: "dep-c", + meta: { deployTarget: "cloud", buildStrategy: "local" }, + }); + + await expect( + createConnection( + ctx, + "app-a", + { sourceProjectId: "db-c", outputId: "svc", envKey: "DB_URL", mode: "internal" }, + { defer: true }, + ), + ).rejects.toThrow(/cloud-hosted/i); + + expect(h.mergeEnvVars).not.toHaveBeenCalled(); + expect(h.upsert).not.toHaveBeenCalled(); + }); + it("allows internal for two projects on the SAME server", async () => { h.findById.mockImplementation(async (id: string) => id === "app-a"