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"