Skip to content

feat(006): deploy-chosen container port + FLEET_IMAGE_PULL_SECRET wiring#65

Merged
bartekus merged 4 commits into
mainfrom
006-fleet-port-pullsecret
Jul 23, 2026
Merged

feat(006): deploy-chosen container port + FLEET_IMAGE_PULL_SECRET wiring#65
bartekus merged 4 commits into
mainfrom
006-fleet-port-pullsecret

Conversation

@bartekus

Copy link
Copy Markdown
Contributor

What

Two gaps blocked any real in-pod fleet placement of an enrahitu chassis image, surfaced while preparing the in-pod fleet E2E (the backlog item's own parenthetical: "pull secret, port 8080"):

  1. Deploy-chosen container port. The addon's DeploySpec.port (default 4000) was never reachable from the HTTP API, while chassis images are fixed on 8080 (EXPOSE 8080, no $PORT in the entrypoint), so placing one would create every object and then hang at the 180s rollout wait on a probe aimed at a port nothing listens on. DeployRequest gains an optional port (integer 1-65535, default 4000 unchanged), persisted on FleetApp and forwarded on update, which rebuilds the Deployment spec and would otherwise silently revert a port-8080 app's probes to 4000 on its first image change. Exposed on FleetAppView and in the deploy attestation payload.

  2. FLEET_IMAGE_PULL_SECRET wiring. Spec 009 §4.4 has always listed it as deploy-set, but the Deployment never set it, so fleetImagePullSecret() resolved empty in production and placed pods carried no imagePullSecrets at all: private images could not pull. The Deployment now sets it to ghcr-pull. The spec 010 catalog entry claimed a base64 dockerconfigjson derived from GHCR_PAT; no code ever read it that way (fleet/config.ts consumes a Secret NAME, and fleet's RBAC deliberately cannot create Secrets), so the entry is corrected (user-supplied, non-secret, still required) and .env.example regenerated.

Operational notes

  • Live DB migration required before the image carrying this deploys (CoreLedger schema init is CREATE-only): ALTER TABLE "fleet_app" ADD COLUMN "port" BIGINT NOT NULL DEFAULT 4000; (precedent: the spec 011 user_account ALTER, 2026-07-22).
  • The pull Secret itself is operator-provisioned per tenant namespace (fleet RBAC grants nothing on secrets); nothing automates that yet.
  • The operator .env value for FLEET_IMAGE_PULL_SECRET predates the catalog correction and should be updated to the Secret name (ghcr-pull).
  • The governance UI DeployForm does not expose port yet; follow-up under spec 007 territory if wanted.

Verification

  • npm run typecheck clean; full vitest run 25 files / 144 passed (18 skipped Postgres arms).
  • secrets:validate (37 keys) and secrets:check green.
  • spec-spine compile/index/lint clean; couple OK, 10 paths, no drift. Specs 006/009/010 amended in the same change.

Two gaps blocked any real in-pod fleet placement of a chassis image,
surfaced while preparing the in-pod fleet E2E:

- The addon's DeploySpec.port (default 4000) was unreachable from the
  HTTP API, while enrahitu chassis images are fixed on 8080, so a
  placement would hang at the rollout wait on a probe aimed at a port
  nothing listens on. DeployRequest gains an optional validated port,
  persisted on FleetApp and forwarded on update so an image change
  cannot silently revert probes to 4000. Exposed on FleetAppView and
  the deploy attestation payload.
  Live DB needs: ALTER TABLE "fleet_app" ADD COLUMN "port" BIGINT
  NOT NULL DEFAULT 4000 (CoreLedger schema init is CREATE-only).

- Spec 009 section 4.4 always listed FLEET_IMAGE_PULL_SECRET as
  deploy-set, but the Deployment never set it, so placed pods carried
  no imagePullSecrets and private images could not pull. The deploy
  now sets it to ghcr-pull. The spec 010 catalog entry claimed it was
  a base64 dockerconfigjson derived from GHCR_PAT; no code ever read
  it that way (fleet/config.ts consumes a Secret NAME), so the entry
  is corrected (user-supplied, non-secret) and .env.example
  regenerated; secrets:validate and secrets:check stay green.

Specs 006/009/010 amended accordingly; derived shards recompiled.
@github-actions

Copy link
Copy Markdown

AI Code Review

Good — the ledger maps { type: "integer" } to Postgres BIGINT so the spec's migration SQL is consistent. The entity type is fine.


Bugs and Logic Errors

update attestation omits port
deploy's record() call was updated to include port: appPort (api.ts:180), but the update's record() call at line 258 was not:

await record("update", app, auth.userID, { tenantId: app.tenantId, app: app.name, image: img }, gated.configHash);

Port is now a meaningful deployment parameter — an update that changes from a 4000-default deployment to one explicitly using 8080 is not captured in the governance audit trail.

Existing rows silently inherit wrong port after migration
The ALTER TABLE migration sets DEFAULT 4000 for existing rows. Any apps previously placed with enrahitu chassis images (which serve on 8080) will have port = 4000 in the database after migration. The first update call on such an app will pass port: 4000 to the addon, reverting probes to the wrong port. The spec acknowledges the migration but does not address this data gap. This may be moot if no successful private-image placements exist yet (the pull-secret bug blocked them), but that assumption is not stated.

Security

No issues. FLEET_IMAGE_PULL_SECRET is correctly reclassified as a non-secret resource name. Port validation correctly rejects NaN, Infinity, and floats via Number.isInteger.

Spec Coupling

Clean. All three specs (006, 009, 010) carry amendments; .derived/ shards in both codebase-index/ and spec-registry/ are regenerated with updated hashes. No hand-edits detected.

Performance

No concerns.


Summary: Two issues worth addressing before merge — the record() attestation gap in update, and an unacknowledged data risk for the migration DEFAULT when existing rows may have been placed on port 8080.


Automated review by Claude. Diff size: 163 lines.

…vidence

The update record() now includes the persisted port, matching deploy.
The spec 006 amendment states the verified reason DEFAULT 4000 cannot
mislabel an existing 8080 placement: the live fleet_app table holds
exactly one row, terminal removed, whose placement died Forbidden
before any cluster object existed.
@bartekus

Copy link
Copy Markdown
Contributor Author

AI review acknowledgment

Both findings taken; fixes in 91dca32.

update attestation omits port: confirmed, fixed. The update record() payload now carries port: app.port, matching deploy.

Migration DEFAULT data risk: assumption verified against the live database and now stated in the spec. fleet_app holds exactly one row (the 2026-07-22 walk's probe app), in the terminal removed state, and its placement died Forbidden before any cluster object was created (the pre-PR-#62 RBAC defect), so DEFAULT 4000 cannot mislabel an existing 8080 placement. The spec 006 amendment records this evidence instead of leaving the assumption implicit. As a second guard, removed is terminal in the FleetApp state machine, so update cannot run against that row anyway.

@github-actions

Copy link
Copy Markdown

AI Code Review


Bugs

Column type mismatch: entity declares integer, spec documents BIGINT

entities.ts:40 uses @Column({ type: "integer" }) (PostgreSQL INTEGER, 32-bit). The spec amendment (specs/006-fleet/spec.md:402) documents the manual migration as:

ALTER TABLE "fleet_app" ADD COLUMN "port" BIGINT NOT NULL DEFAULT 4000

CoreLedger's schema init (run on fresh databases) generates INTEGER; the production ALTER creates BIGINT. Port numbers fit in both, but the schema diverges between any fresh provisioning and production. The entity definition should match what the database actually has — fix to { type: "bigint" } in the entity, or document the manual migration as INTEGER.

Port is permanently immutable after deploy — not documented in the API contract

DeployRequest.port is the only write path. The update endpoint reads app.port from the stored row and forwards it; there is no way to change the port without a full remove+redeploy. The JSDoc comment on DeployRequest.port (api.ts:105-110) should state this constraint explicitly. API clients that discover immutability at update time after setting the wrong port face a potentially destructive recovery path.

Security

FLEET_IMAGE_PULL_SECRET reclassified from secret = true to secret = false — operator .env cleanup needed

The catalog change is correct (it's a resource name, not a credential). The spec notes the operator .env value predates the correction and should be updated. Worth confirming there is no automation or secrets-scanning pipeline that would have been injecting the old base64 blob as this env var in production; if it was, the running container may have the wrong value and imagePullSecrets would silently resolve to a non-existent Secret name rather than failing loudly.

Operational Risk

No migration artifact — failure mode is hard crash, not graceful degradation

The required ALTER TABLE lives only in spec prose. If the image rolls before the ALTER runs, CoreLedger fails to map the missing column and the service is immediately unavailable (not a degraded mode — every request to any fleet endpoint will fail). The spec 011 precedent is cited but that precedent is itself just documented in spec prose. Consider adding a migration runbook entry or a startup check that emits a clear error if the column is absent.

Spec Coupling

Clean. All three affected specs (006, 009, 010) have amendment sections, and all six .derived/ shards (registry and codebase-index) are regenerated with updated hashes. No hand-edits to derived artifacts detected.

Performance

No concerns.


Automated review by Claude. Diff size: 170 lines.

@bartekus

Copy link
Copy Markdown
Contributor Author

AI review acknowledgment (second pass)

Column type mismatch: refuted with production evidence. CoreLedger's Postgres dialect maps { type: "integer" } to BIGINT, not INTEGER (backend/core/ledger/schema.ts:34, "64-bit; pg returns it as a string, Number()-coerced by the codec"); INTEGER is the libsql/SQLite mapping. Verified live: \d "fleet_app" shows volume_size, declared with the identical { type: "integer" }, as bigint in production. The entity and the documented ALTER ... BIGINT are consistent; fresh provisioning and production do not diverge.

Port immutability undocumented: taken. The DeployRequest.port JSDoc now states the constraint (update forwards the persisted value; changing the port means remove + redeploy). Fixed in d647b29.

Old base64 blob possibly injected in production: refuted with pod evidence. The running container's environment contains zero FLEET_IMAGE_PULL_SECRET variables (verified via exec; count 0). The omission of the variable from the Deployment was the entire defect: nothing (SOPS set, reflector, pod Secret) ever delivered the old value, so there is no wrong value to clean up in-cluster. The stale operator .env value is local-only and flagged in the spec 010 amendment.

No migration artifact / crash window: accepted, sequencing controlled. The ALTER is a one-time additive step and will be applied to the live database immediately after this merge and before the new image digest is pinned, in the same operational sequence (precedent: the spec 011 user_account ALTER). The window the finding describes does not occur in the flux-managed flow, because the image only rolls when the digest pin lands, which happens after the ALTER. A startup column-check is more machinery than this single-operator repo's migration flow warrants today; the spec prose plus the PR body is the runbook.

@github-actions

Copy link
Copy Markdown

AI Code Review

Bugs and Logic Errors

Privileged port range accepted without guard (api.ts:validatePort): ports 1–1023 pass validation but most container images can't bind them without NET_BIND_SERVICE. A caller specifying port 443 gets a pod that loops on probe failures indefinitely, indistinguishable from a slow start. Consider rejecting port < 1024 or at least documenting the restriction.

validatePort has no unit tests for boundary cases. The entity test adds port: 8080 on the happy path; boundary values (0, 1, 65535, 65536, floats, negative numbers) are untested. Edge cases like port = 0.9 pass Number.isInteger as false and are correctly rejected, but port = 1.0 would also pass — that's actually fine (Number.isInteger(1.0) is true in JS), but it's worth an explicit test.

Deployment ordering risk (acknowledged in spec, flagging for completeness): if the image rolls before the manual ALTER TABLE "fleet_app" ADD COLUMN "port" BIGINT NOT NULL DEFAULT 4000 runs, the service crashes on startup. No code-level guard exists. The spec correctly documents the backfill rationale and the one-row live state, but there is no deploy-gate or readiness check that enforces the migration precondition.

Security

FLEET_IMAGE_PULL_SECRET reclassification (secret = false) is correct. The value is a Kubernetes resource name, not a credential; the actual dockerconfigjson lives in the operator-provisioned Secret that fleet's RBAC cannot touch. This is a security improvement: the env var leaking would only reveal the Secret's name, not the credential.

No injection surface introduced. appPort is a validated integer before reaching the addon call. The pull-secret name flows from an env var, not from user input.

port field is tenant-scoped and constrained to their own pod. A tenant specifying an unusual port can only harm their own placement; no cross-tenant or host-escape vector.

Spec Coupling

All three affected specs (006, 009, 010) carry amendment sections matching the code changes. .derived/ shard hashes are updated in by-spec/ and codebase-index/ for all three specs. The shards show only hash and amendments array changes — not hand-edited content. Coupling looks clean.

Performance

Nothing of concern.


Automated review by Claude. Diff size: 172 lines.

isValidPort joins the pure helpers in ops.ts: placed pods run as a
non-root UID with no NET_BIND_SERVICE, so a port below 1024 could only
reproduce the permanent rollout hang this change exists to prevent;
reject it up front with the reason in the error. Boundary tests cover
1023/1024/65535/65536, negatives, floats, NaN, Infinity. The fleet_app
port ALTER is applied to the live database ahead of the merge, so no
deploy ordering window exists; the spec records both.
@bartekus

Copy link
Copy Markdown
Contributor Author

AI review acknowledgment (third pass)

All three items taken or closed with evidence; changes in b545299.

Privileged ports: taken, rejected up front. isValidPort (now a pure helper in ops.ts, beside isValidAppName) requires an integer in 1024-65535. Placed pods run as a non-root UID with no NET_BIND_SERVICE, so a port below 1024 could only reproduce the exact permanent rollout hang this PR exists to prevent; the API error states the reason.

Boundary tests: taken. ops.test.ts covers 1023/1024/65535/65536, 0, negatives, floats, NaN, Infinity, plus the JS 8080.0 integer-semantics case, in the same style as the existing state-machine and name tests.

Migration ordering window: closed, not just acknowledged. The ALTER TABLE "fleet_app" ADD COLUMN "port" BIGINT NOT NULL DEFAULT 4000 was applied to the live database on 2026-07-23, before this merge; verified with \d "fleet_app" (the column exists, bigint, default 4000). The old image is unaffected (its INSERTs name their columns; the default fills). There is no longer any sequence in which the new image can meet a database without the column, so no startup guard is warranted. The spec amendment records the application date.

@github-actions

Copy link
Copy Markdown

AI Code Review

Bugs and Logic Errors

BIGINT/INTEGER column type mismatch (backend/fleet/entities.ts vs spec amendment)

The entity declares @Column({ type: "integer" }) port = 4000 (PostgreSQL integer, 4 bytes), but the manual ALTER in the spec amendment uses BIGINT NOT NULL DEFAULT 4000 (8 bytes). CoreLedger schema init is CREATE-only, so the live column is BIGINT while every ORM-generated query will type-bind it as integer. TypeORM won't fail on reads (BIGINT coerces) but the schema mismatch is a latent inconsistency — the ALTER should match the entity, i.e. ALTER TABLE "fleet_app" ADD COLUMN "port" INTEGER NOT NULL DEFAULT 4000.

Port immutability enforced by omission, not by assertion

The spec calls port immutable after deploy ("changing it means remove + redeploy"), but update silently ignores a port field if a future caller tries to pass one. This is safe today because the UpdateRequest interface has no port field, but the invariant is invisible to API consumers. A 415 or explicit rejection on unexpected fields would make this contract legible.

Security

FLEET_IMAGE_PULL_SECRET reclassification is correct — a Kubernetes Secret name is not a credential; secret: false / provenance: user-supplied is the right catalog entry. No concern here.

Port value reaches the native addon without an additional clamp. isValidPort enforces 1024–65535 as an integer, which is sufficient — no injection surface is opened.

No authz gaps introduced; port is validated after the tenant write-auth check, in the correct order.

Spec Coupling

.derived/ hashes updated, not hand-edited. Amendments land in specs 006, 009, and 010, all owning specs for the touched paths. The coupling appears complete.

Minor: the spec 006 amendment documents the FleetApp.port persistence and the update forwarding behaviour, but the FleetAppView.port field (the public API response shape) is not named explicitly in any spec section. Low risk given the amendment is thorough elsewhere, but the view contract is the external surface.

Performance

No concerns — integer column, no new indexes required, O(1) validation.


Automated review by Claude. Diff size: 208 lines.

@bartekus

Copy link
Copy Markdown
Contributor Author

AI review acknowledgment (fourth pass): merging

  • BIGINT/INTEGER mismatch: previously refuted with production evidence (see the second-pass acknowledgment). CoreLedger is not TypeORM; its Postgres dialect maps { type: "integer" } to BIGINT (backend/core/ledger/schema.ts:34), and the live volume_size column, declared identically, is bigint. The applied ALTER matches what schema init would generate. This pass's first review confirmed the same mapping.
  • Update ignoring a hypothetical port field: Encore validates request bodies against the declared interface; UpdateRequest declares no port, and the immutability contract is stated on DeployRequest.port's JSDoc. Accepted as-is.
  • FleetAppView.port not named in a spec: the spec 006 amendment states "The port travels in the deploy attestation payload and the FleetAppView."

All substantive findings across the four passes are fixed or closed with evidence; merging.

@bartekus
bartekus merged commit 676b642 into main Jul 23, 2026
3 checks passed
@bartekus
bartekus deleted the 006-fleet-port-pullsecret branch July 23, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant