diff --git a/console/README.md b/console/README.md index 6996012..a11f608 100644 --- a/console/README.md +++ b/console/README.md @@ -38,6 +38,9 @@ npm run build # tsc + vite build → dist/ | `src/source.ts` | `Source` interface + `MockSource` (fixtures) / `TauriSource` (desktop) | | `src/render.ts` | pure `rosterHtml(deployments)` → table; `renderRoster` sets it on the DOM | | `src/main.ts` | polls `Source` every 5s and re-renders | +| `src/deploy.ts` | `[+ New fleet]` / `[+ Add instance]` wizard — collects the fields, then makes the one `deploy_provision_agent` call | +| `src/deployArgs.ts` | pure wizard-fields → `deploy_provision_agent` args (studio#111) | +| `src/fleetToml.ts` | pure `fleets.toml` block edits — the post-deploy config write | | `src/fixtures.ts` | stand-in roster data | ## Wiring to the core (slice-2) @@ -48,3 +51,27 @@ dependency. Slice-2 adds `src-tauri/` whose Rust `deploy_list` command bridges to `studio-cp::observe_services` / `observe_deployment`. Because the boundary is the read-model shape (and, later, MCP), swapping `MockSource` → `TauriSource` is the only change the UI sees. + +## Deploy credentials (studio#111) + +A deploy is the one call where the wizard's AWS answers have nowhere else to +live: `fleets.toml` is written only *after* a confirmed successful provision +(ADR-83 §7.5), so on a first create the Region + Credential profile the +identity step collected exist solely in the `deploy_provision_agent` arguments +(`src/deployArgs.ts`). + +`oab-mcp` resolves the managing credential in two steps: the fleet a call *named* +(`fleet:`) wins, else the binding whose `cluster` key matches, else the ambient +`[default]` chain. + +The wizard's deploy call matches neither key: it names no fleet, and a +console-written `[fleet.]` block declares no `cluster` key — so the +recorded pair is read by nothing and the ambient chain answers. (The console's +*read* and scale calls do name a fleet, but `target()` rejects a `fleet`-scoped +ECS call whose binding omits `cluster` — a separate, pre-existing gap; the +deploy call steps around it by passing `cluster` instead.) On a first create the +ambient chain is also what `build_default_manifest`'s VPC/subnet/security-group +discovery runs against, so a fleet created for one account/region would land in +another. Drop the fields and the wizard's answer is silently discarded. Naming +only one of the two is safe: the sidecar layers it onto whatever base binding it +found rather than substituting for it. diff --git a/console/src/deploy.ts b/console/src/deploy.ts index 7d2d6b9..81fccce 100644 --- a/console/src/deploy.ts +++ b/console/src/deploy.ts @@ -14,6 +14,7 @@ import type { Source } from "./source"; import { appendMember, appendFleetBlock, fleetBlockExists } from "./fleetToml"; +import { provisionAgentArgs, awsIdentityFor } from "./deployArgs"; type Invoke = (cmd: string, args?: Record) => Promise; @@ -99,6 +100,14 @@ function randomGreekName(): string { // "add-instance" (that only exists for "new-fleet"), so there's nothing to // re-collect from the operator — a k8s fleet's context/namespace/service // account were fixed the moment the fleet was created. +// +// studio#111: the `region`/`profile` pair rides along for the same reason. +// The sidecar resolves a fleet's managing credential by the fleet a call names +// (`fleet:`), else by a binding's `cluster` key — and this console's wizard +// names neither (it passes no `fleet`, and its writer emits no `cluster` key), +// so the recorded pair is read by nothing and the ambient `[default]` chain +// answers instead. The answer has to travel with the call (see +// `deployArgs.ts`). export type DeployMode = | { kind: "new-fleet" } | { @@ -108,6 +117,8 @@ export type DeployMode = context: string | null; namespace: string | null; expectedPrincipal: string | null; + region: string | null; + profile: string | null; }; // What the panel reports back once a deploy + fleets.toml write both succeed — @@ -663,21 +674,35 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null } deployBtn.disabled = true; setStatus(deployStatusEl, "deploying…"); + // studio#111: the AWS identity the deploy runs against has to travel with + // the call — see `deployArgs.ts`. "new-fleet" reads it off the identity + // step's own fields; "add-instance" inherits the existing fleet's + // recorded region/profile, the same place it inherits its k8s placement. + const { region: awsRegion, profile: awsProfile } = awsIdentityFor( + mode.kind === "new-fleet" + ? { kind: "new-fleet", region: regionInput.value, profile: profileInput.value } + : { kind: "add-instance", region: mode.region, profile: mode.profile }, + ); let res: { image?: string; digest?: string; objects?: number }; try { - res = await invoke("deploy_provision_agent", { - image, - name, - namespace, - api_key: apiKeyInput.value.trim() || undefined, - chat_platform: chatPlatform, - chat_bot_token: chatTokenInput.value.trim() || undefined, - chat_channel_secret: chatSecretInput.value.trim() || undefined, - acp_enabled: acpCheckbox.checked, - acp_token: acpCheckbox.checked ? acpTokenInput.value.trim() || undefined : undefined, - local_config_folder: localConfigFolder(), - ...(isK8s ? { provider: "k8s", context, expected_principal: expectedPrincipal } : {}), - }); + res = await invoke( + "deploy_provision_agent", + provisionAgentArgs({ + image, + name, + namespace, + apiKey: apiKeyInput.value, + chatPlatform, + chatBotToken: chatTokenInput.value, + chatChannelSecret: chatSecretInput.value, + acpEnabled: acpCheckbox.checked, + acpToken: acpTokenInput.value, + localConfigFolder: localConfigFolder(), + region: awsRegion, + profile: awsProfile, + k8s: k8sTarget ? { context, expectedPrincipal } : undefined, + }), + ); } catch (e) { setStatus(deployStatusEl, `deploy failed: ${errText(e)}`, "err"); deployBtn.disabled = false; @@ -698,8 +723,11 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null ? { kind: "k8s", context: context ?? null, namespace } : { kind: "ecs", - region: regionInput.value.trim() || null, - profile: profileInput.value.trim() || null, + // Same values the deploy just ran under, not a second read + // of the form — the recorded binding and the call that + // created the agent have to agree. + region: awsRegion ?? null, + profile: awsProfile ?? null, }, }) : appendMember(current.text, fleetName, service); diff --git a/console/src/deployArgs.test.ts b/console/src/deployArgs.test.ts new file mode 100644 index 0000000..a335d6a --- /dev/null +++ b/console/src/deployArgs.test.ts @@ -0,0 +1,303 @@ +// studio#111: the "+ New fleet" / "+ Add instance" wizard's single +// `deploy_provision_agent` call is where #111's create-or-redeploy branch is +// reached from — a brand-new agent (no stored manifest) gets a freshly built +// `OABServiceManifest` applied through `apply_manifests`, an existing one takes +// `redeploy`'s patch path. That decision is the sidecar's, but everything it +// needs to make it — and to build a correct first manifest — arrives in this +// one argument object, so the object is pinned here rather than left implicit +// in the panel's submit handler. +// +// The AWS-credential half is the load-bearing one (studio#111): the wizard's +// identity step already collects Region + Credential profile and writes them +// into `fleets.toml`, but oab-mcp resolves a fleet's credential by the fleet a +// call names (`fleet:`), else by a binding's `cluster` key, and this wizard +// names neither — so the sidecar falls back to the ambient `[default]` chain. +// On a first create there is no binding at all +// yet (`fleets.toml` is only written *after* a confirmed successful deploy), +// so the first manifest's VPC/subnet/security-group discovery — and the apply +// itself — would run against whatever account the ambient chain happens to +// resolve. The operator's chosen region/profile have to ride along with the +// call. + +import { describe, it, expect } from "vitest"; +import { provisionAgentArgs, awsIdentityFor } from "./deployArgs"; +import { appendFleetBlock } from "./fleetToml"; + +const ecs = { + image: "ghcr.io/openabdev/openab:0.10.0-beta.3-codex", + name: "zeus", + namespace: "default", +}; + +describe("provisionAgentArgs — the shape every provision call must have", () => { + it("always names the image, agent and namespace", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true }); + expect(args.image).toBe(ecs.image); + expect(args.name).toBe("zeus"); + expect(args.namespace).toBe("default"); + }); + + it("never sends a k8s dispatch for an ECS submit", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true }); + expect(args.provider).toBeUndefined(); + expect(args.context).toBeUndefined(); + expect(args.expected_principal).toBeUndefined(); + }); +}); + +describe("provisionAgentArgs — AWS credentials (studio#111)", () => { + it("forwards the region and credential profile the wizard collected", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + region: "ap-northeast-1", + profile: "studio-prod", + }); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); + + it("omits blank credentials rather than sending empty strings", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true, region: " ", profile: "" }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); + + it("keeps a region-only submit from inventing a profile", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true, region: "eu-west-1" }); + expect(args.region).toBe("eu-west-1"); + expect("profile" in args).toBe(false); + }); +}); + +describe("provisionAgentArgs — k8s dispatch (studio#104/#153)", () => { + const k8s = { + ...ecs, + namespace: "persephone", + acpEnabled: true, + k8s: { context: "orbstack", expectedPrincipal: "system:serviceaccount:persephone:runner" }, + }; + + it("passes provider/context/expected_principal together", () => { + const args = provisionAgentArgs(k8s); + expect(args.provider).toBe("k8s"); + expect(args.context).toBe("orbstack"); + expect(args.expected_principal).toBe("system:serviceaccount:persephone:runner"); + }); + + it("omits an unset context so the kubeconfig current-context applies", () => { + const args = provisionAgentArgs({ ...k8s, k8s: { context: undefined, expectedPrincipal: undefined } }); + expect(args.provider).toBe("k8s"); + expect("context" in args).toBe(false); + expect("expected_principal" in args).toBe(false); + }); + + it("never sends AWS credentials on a k8s submit", () => { + // A k8s pod has no AWS credential chain (studio#104/#128), so a region or + // profile left over from the identity step's AWS fields must not travel + // with the call and mislead the sidecar into resolving one anyway. + const args = provisionAgentArgs({ ...k8s, region: "ap-northeast-1", profile: "studio-prod" }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); +}); + +describe("awsIdentityFor — which source answers a submit (studio#111)", () => { + it("reads new-fleet's own identity step fields", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: "ap-northeast-1", profile: "studio-prod" }), + ).toEqual({ region: "ap-northeast-1", profile: "studio-prod" }); + }); + + it("inherits add-instance's fleet-recorded pair, not an empty form", () => { + // The panel has no AWS field group in add-instance mode, so reading the + // form there would ship "no override" for a fleet that has one recorded. + expect( + awsIdentityFor({ kind: "add-instance", region: "eu-west-1", profile: "prod-admin" }), + ).toEqual({ region: "eu-west-1", profile: "prod-admin" }); + }); + + it("treats an unrecorded fleet pair as no override at all", () => { + // Both undefined, not empty strings: the sidecar must keep resolving the + // credential itself rather than being handed a blank region/profile. + expect(awsIdentityFor({ kind: "add-instance", region: null, profile: null })).toEqual({ + region: undefined, + profile: undefined, + }); + }); + + it("trims surrounding whitespace but keeps a real value", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: " ap-northeast-1 ", profile: " studio-prod " }), + ).toEqual({ region: "ap-northeast-1", profile: "studio-prod" }); + }); + + it("drops a whitespace-only field", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: " ", profile: " studio-prod " }), + ).toEqual({ region: undefined, profile: "studio-prod" }); + }); +}); + +describe("the two seams compose — what an add-instance submit actually sends", () => { + it("takes region/profile from the fleet binding, not from empty form fields", () => { + // The panel's submit handler is exactly this composition; `mode.region` / + // `mode.profile` (the fleet `main.ts` read off `FleetConfigEntry`) have to + // be what reaches the call, since add-instance has no AWS field group. + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "add-instance", region: "eu-west-1", profile: "prod-admin" }), + }); + expect(args.region).toBe("eu-west-1"); + expect(args.profile).toBe("prod-admin"); + }); + + it("sends nothing to override with when the fleet records no identity", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "add-instance", region: null, profile: null }), + }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); + + it("still carries the new-fleet identity step's own fields through", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "new-fleet", region: "ap-northeast-1", profile: "studio-prod" }), + }); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); +}); + +describe("the deployed identity and the recorded one cannot drift", () => { + // The submit handler reads the identity once (`awsIdentityFor`) and feeds it + // to both the call and the `[fleet.]` entry. Asserted against the real + // `appendFleetBlock` output rather than a hand-rolled stand-in, because the + // thing that must not drift is what actually lands in `fleets.toml`. + const ecsEntry = (region: string | null, profile: string | null) => ({ + name: "support", + member: "oab-default-zeus", + expectedPrincipal: null, + runtime: { kind: "ecs" as const, region, profile }, + }); + + it("records exactly what it deployed under", () => { + const identity = awsIdentityFor({ + kind: "new-fleet", + region: " ap-northeast-1 ", + profile: "studio-prod", + }); + const call = provisionAgentArgs({ ...ecs, acpEnabled: true, ...identity }); + const toml = appendFleetBlock("", ecsEntry(identity.region ?? null, identity.profile ?? null)); + expect(call.region).toBe("ap-northeast-1"); + expect(toml).toContain('region = "ap-northeast-1"'); + expect(toml).toContain('profile = "studio-prod"'); + }); + + it("records a blank identity as absent, not as an empty string", () => { + // An empty `region = ""` in fleets.toml would later read as a pinned-but- + // blank region to every consumer of the file. + const identity = awsIdentityFor({ kind: "new-fleet", region: " ", profile: "" }); + expect(identity).toEqual({ region: undefined, profile: undefined }); + const toml = appendFleetBlock("", ecsEntry(identity.region ?? null, identity.profile ?? null)); + expect(toml).not.toContain("region"); + expect(toml).not.toContain("profile"); + expect(toml).toContain('members = ["oab-default-zeus"]'); + }); +}); + +describe("provisionAgentArgs — optional wizard fields", () => { + it("forwards chat platform secrets and the local config folder when set", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + chatPlatform: "line", + chatBotToken: "channel-token", + chatChannelSecret: "channel-secret", + apiKey: "sk-vendor", + localConfigFolder: "/home/op/studio-config", + }); + expect(args.chat_platform).toBe("line"); + expect(args.chat_bot_token).toBe("channel-token"); + expect(args.chat_channel_secret).toBe("channel-secret"); + expect(args.api_key).toBe("sk-vendor"); + expect(args.local_config_folder).toBe("/home/op/studio-config"); + }); + + it("drops every blank optional field instead of sending empty strings", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: false, + apiKey: " ", + chatPlatform: "", + chatBotToken: "", + chatChannelSecret: "", + acpToken: "", + localConfigFolder: "", + }); + for (const key of [ + "api_key", + "chat_platform", + "chat_bot_token", + "chat_channel_secret", + "acp_token", + "local_config_folder", + ]) { + expect(key in args).toBe(false); + } + }); + + it("trims surrounding whitespace on every optional field", () => { + // Pre-existing behavior for most of these; pinned because a trimmed + // `local_config_folder` in particular is new (it used to go over untrimmed, + // and a trailing space in a path is a different directory). + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + apiKey: " sk-vendor ", + chatPlatform: " discord ", + chatBotToken: " bot-token ", + chatChannelSecret: " channel-secret ", + acpToken: " acp-key ", + localConfigFolder: " /home/op/studio-config ", + region: " ap-northeast-1 ", + profile: " studio-prod ", + }); + expect(args.api_key).toBe("sk-vendor"); + expect(args.chat_platform).toBe("discord"); + expect(args.chat_bot_token).toBe("bot-token"); + expect(args.chat_channel_secret).toBe("channel-secret"); + expect(args.acp_token).toBe("acp-key"); + expect(args.local_config_folder).toBe("/home/op/studio-config"); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); + + it("trims the k8s placement pair too", () => { + // A hand-edited `fleets.toml` is the only in-app source of these (via + // add-instance's inherited placement), and padding on either one selects + // nothing. + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + k8s: { context: " orbstack ", expectedPrincipal: " system:serviceaccount:persephone:runner " }, + }); + expect(args.context).toBe("orbstack"); + expect(args.expected_principal).toBe("system:serviceaccount:persephone:runner"); + }); + + it("carries acp_enabled verbatim — the sidecar owns the default-when-absent rule", () => { + expect(provisionAgentArgs({ ...ecs, acpEnabled: false }).acp_enabled).toBe(false); + expect(provisionAgentArgs({ ...ecs, acpEnabled: true }).acp_enabled).toBe(true); + }); + + it("never sends an acp_token for an ACP-off agent (the sidecar ignores it, and it would be a stray secret)", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: false, acpToken: "typed-anyway" }); + expect("acp_token" in args).toBe(false); + }); +}); diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts new file mode 100644 index 0000000..4064625 --- /dev/null +++ b/console/src/deployArgs.ts @@ -0,0 +1,153 @@ +// Pure translation of the deploy wizard's collected fields into the single +// `deploy_provision_agent` argument object (`deploy.ts`'s submit handler calls +// exactly one, so the panel stays a single step — studio#111). +// +// Why this is a module and not an inline object literal: everything the +// sidecar needs to run its create-or-redeploy branch travels in that one +// object, and on the *first* deploy there is nothing else to read it from. +// `fleets.toml` — the only other carrier of the operator's answers — is +// written strictly *after* a confirmed successful provision (ADR-83 §7.5), so +// at create time the region/profile the identity step collected exist nowhere +// but here. Drop them and `oab-mcp` falls back to the ambient `[default]` +// credential chain: it resolves a fleet's credential by the fleet a call names +// (`fleet:`), else by a binding's `cluster` key, and this wizard names neither +// (it passes no `fleet`, and its writer emits no `cluster` key) — so the first +// manifest's +// `default_networking` VPC/subnet/security-group discovery (studio#111's +// create-from-scratch defaults, ported from `oabctl create`'s wizard) and the +// apply itself would both run against whichever account the ambient chain +// happens to resolve, while `fleets.toml` goes on to record the region the +// operator actually picked. +// +// Pure and side-effect-free, like `fleetToml.ts` — the DOM reads stay in +// `deploy.ts`, this only shapes what they collected. + +/** The k8s placement fields the submit sends alongside `provider: "k8s"`. */ +export interface K8sPlacement { + /** Kubeconfig context; omitted leaves the kubeconfig current-context in play. */ + context?: string; + /** `system:serviceaccount::`, or omitted for the namespace default. */ + expectedPrincipal?: string; +} + +/** Everything the wizard collected for one agent, as read off the form. */ +export interface ProvisionAgentInput { + image: string; + name: string; + namespace: string; + apiKey?: string; + chatPlatform?: string; + chatBotToken?: string; + chatChannelSecret?: string; + acpEnabled: boolean; + acpToken?: string; + localConfigFolder?: string; + /** ECS only — the identity step's Region field. */ + region?: string; + /** ECS only — the identity step's Credential profile field. */ + profile?: string; + /** Present ⇔ this submit targets k8s (studio#104/#153). */ + k8s?: K8sPlacement; +} + +/** + * Where one submit's AWS identity comes from — the same split the panel's + * `DeployMode` makes for k8s placement (studio#153), for the same reason: + * `new-fleet` collects the answers in its own identity step, `add-instance` + * inherits them from the fleet it is adding to. + */ +export type AwsIdentitySource = + | { kind: "new-fleet"; region: string; profile: string } + | { kind: "add-instance"; region: string | null; profile: string | null }; + +/** + * The AWS identity a submit should deploy under — trimmed, never invented. + * + * `add-instance` reads the *fleet's* recorded pair, not an empty form (the + * panel has no AWS field group in that mode), and a fleet with nothing + * recorded yields `undefined` for both, i.e. "no override", so the sidecar + * keeps resolving the credential itself rather than being handed blanks. + */ +export function awsIdentityFor(source: AwsIdentitySource): { + region?: string; + profile?: string; +} { + const [region, profile] = + source.kind === "new-fleet" + ? [source.region, source.profile] + : [source.region ?? "", source.profile ?? ""]; + return { region: orUndefined(region), profile: orUndefined(profile) }; +} + +/** + * Trimmed value, or `undefined` for anything blank. + * + * Trimming is a behavior change for the four fields that used to cross the wire + * verbatim — `chat_platform`, `local_config_folder`, and the k8s pair + * `context`/`expected_principal` — and is intentional in each: a padded platform + * name or kubeconfig context selects neither, a padded path names a different + * directory, and a padded `expected_principal` matches no principal. No in-app + * source produces padding (a `