P2: Reference deployment manifests — k8s + prod Compose (#179) - #196
Conversation
|
Really glad to see opinionated production manifests land — the hardened pod securityContext, HPA, and Secret-based 1. CORS will silently block the mina-explorer once #184 merges. The Explorer is a cross-origin browser app ( - name: CORS_ORIGIN
# The mina-explorer (and any browser UI) is cross-origin and CANNOT reach this
# API unless its origin is allowlisted here. Comma-separate multiple origins.
# NB: the server currently defaults to '*' when unset; #184 changes that to deny.
value: 'https://explorer.example.com'(same note applies to 2. The readiness probe targets Everything else looks solid to me. |
The readiness probe targets /readiness, which only exists from 1.0.0, but the manifests pulled :latest — today that resolves to 0.0.6. Applied as-is the probe would 404 forever, no pod would reach Ready, and the Service would be left with zero endpoints: a total outage from a manifest offered as the copy-paste reference. Both manifests now pin 1.0.0 and state the requirement up front. The /metrics scrape annotations had the same dependency. CORS_ORIGIN shipped commented out, described as "leave unset for same-origin only". Unset blocks every cross-origin browser client — the mina-explorer included — and does so silently, with nothing in the server logs. Since the primary consumers are browsers, it now ships set, with the trade-off spelled out. deploy/README.md repeated the root README's claim that pointing PG_CONN at replicas buys throughput. It buys failover: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Real read scaling needs a balancer in front of Postgres. Also sets TRUST_PROXY=1 in the k8s manifest (an ingress adds a hop, and the default of 0 would bucket every client together) and corrects terminationGracePeriodSeconds from "matches" to "exceeds" SHUTDOWN_TIMEOUT_MS, which is the property that actually matters. Verified with kubectl apply --dry-run=client and docker compose config. Addresses review feedback on #196. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — both fixed in The CORS. Agreed and done — it now ships set rather than commented out, since the primary consumers are cross-origin browsers. I used Three more found while in here:
Verified with One thing to confirm: if #191 adopts the |
|
Verdict: MERGEABLE ✅ New files only, nothing overwritten, so nothing here can break a running deployment. The two points from @SanabriaRusso are genuinely addressed at What I checked
Non-blocking nits1. Readiness probe can black-hole the Service during ordinary DB slowness. A parallel review raised this; I can confirm it with one correction. livenessProbe:
httpGet:
path: /healthcheck
port: 8080
periodSeconds: 15
timeoutSeconds: 3
failureThreshold: 3
readinessProbe:
httpGet:
path: /readiness
port: 8080
periodSeconds: 10
timeoutSeconds: 3 # must be < periodSeconds
failureThreshold: 6 # 60s of DB slowness before losing endpoints
successThreshold: 1
startupProbe: # replaces initialDelaySeconds; no restart while booting
httpGet:
path: /healthcheck
port: 8080
periodSeconds: 3
failureThreshold: 20 # 60s to start60s of tolerance sits comfortably outside #182's 30s 2. No lifecycle:
preStop:
# Endpoint removal is async and concurrent with SIGTERM. Sleep long
# enough for kube-proxy/ingress to stop routing here BEFORE the app
# stops accepting connections. Node has no shell-less sleep, so use
# the sleep binary; on a distroless base use `httpGet` to a drain path.
exec:
command: ['/bin/sh', '-c', 'sleep 15']With a 15s preStop the budget becomes 15 + 10 ( terminationGracePeriodSeconds: 45 # 15s preStop + 10s drain + margin3. Applying this file a second time clobbers the operator's real # once, with your real connection string (never committed):
kubectl create secret generic archive-node-api \
--from-literal=PG_CONN='postgres://archive_api_ro:...@postgres:5432/archive'
kubectl apply -f deploy/kubernetes.yaml4. The HPA scales out at 175m CPU. HPA resources:
requests:
cpu: '500m'
memory: '512Mi'
limits:
# No CPU limit: CFS throttling hurts Node p99 more than the noisy-
# neighbour risk it prevents. The request is the scheduling guarantee.
memory: '1Gi'5. Memory sizing + no - name: NODE_OPTIONS
# Keep the V8 heap under limits.memory (1Gi) so we hit a recoverable
# heap error before the kernel OOM-kills the pod mid-request.
value: '--max-old-space-size=768'Mirror in 6. ---
apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
name: archive-node-api
spec:
minAvailable: 1
selector:
matchLabels:
app: archive-node-apiAlso drop 7. - name: RATE_LIMIT_MAX
# Per client IP per 60s, counted PER REPLICA (in-process counter,
# #185) — the effective global limit is this × replicas, and the HPA
# moves replicas 2→6. Server-side consumers (mina-explorer-api and
# its indexer) share one egress IP and land in one bucket.
value: '1200'
8. The root 9. Security context is in good shape; only additions, no gaps that matter: 10. Compose relies on the image's On your open question: yes, if #191 lands Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
The readiness probe targets /readiness, which only exists from 1.0.0, but the manifests pulled :latest — today that resolves to 0.0.6. Applied as-is the probe would 404 forever, no pod would reach Ready, and the Service would be left with zero endpoints: a total outage from a manifest offered as the copy-paste reference. Both manifests now pin 1.0.0 and state the requirement up front. The /metrics scrape annotations had the same dependency. CORS_ORIGIN shipped commented out, described as "leave unset for same-origin only". Unset blocks every cross-origin browser client — the mina-explorer included — and does so silently, with nothing in the server logs. Since the primary consumers are browsers, it now ships set, with the trade-off spelled out. deploy/README.md repeated the root README's claim that pointing PG_CONN at replicas buys throughput. It buys failover: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Real read scaling needs a balancer in front of Postgres. Also sets TRUST_PROXY=1 in the k8s manifest (an ingress adds a hop, and the default of 0 would bucket every client together) and corrects terminationGracePeriodSeconds from "matches" to "exceeds" SHUTDOWN_TIMEOUT_MS, which is the property that actually matters. Verified with kubectl apply --dry-run=client and docker compose config. Addresses review feedback on #196. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
df9af80 to
6f89b0f
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
All four round-1 defects are genuinely fixed. Approving.
Verified fixed
- preStop + grace period —
deploy/kubernetes.yaml:90-95addspreStop: exec ['/bin/sh','-c','sleep 15'],:131setsterminationGracePeriodSeconds: 45,:69-71setsSHUTDOWN_TIMEOUT_MS: '10000'. 15 s drain window + 10 s app drain = 25 s inside a 45 s grace, 20 s margin./bin/shandsleepare present (#189's base isnode:20-alpine, busybox). This closes round 1's cross-cutting gap where every rolling deploy RST'd in-flight requests — that item had no owner, and this PR takes it. - Secret split out —
deploy/secret.example.yamlis its own file,kubernetes.yamlcontains only Deployment/Service/HPA/PDB (confirmed:grep -n 'kind:'returns no Secret), anddeploy/README.md:38-41creates the secret imperatively once, then applies-f deploy/kubernetes.yaml— a single file, not a-f deploy/directory loop. A routine re-apply can no longer clobber a realPG_CONN. - HPA/resources coherent — request
cpu: '500m'with no CPU limit (:86-89, with an explanatory comment) and HPA ataverageUtilization: 70(:165) means scale-out at 350m of the request, with no CFS throttling. That is the right call for a latency-sensitive Node service. - Probe arithmetic now checks out, given #187's bounded ping (
2496d84c):- #187's default
READINESS_PING_TIMEOUT_MS = 2000, on a dedicatedmax: 1client, so the ping cannot head-of-line-block behind a 2000-block analytics query on the shared pool. readinessProbe.timeoutSeconds: 3(:109) > the 2 s ping bound → the kubelet always gets a real 200/503, never a probe-level timeout.periodSeconds: 10 × failureThreshold: 6= 60 s of continuous DB unreachability before endpoints are pulled.- #182's
PG_STATEMENT_TIMEOUT = 15000caps the worst self-inflicted stall on the shared pool at 15 s — and cannot touch the readiness client at all. 60 s > 15 s by 4×, so a wave of statement-timeout-capped slow queries can no longer correlate all replicas NotReady. That was round 1's total-outage path; it is closed. - Liveness
/healthcheck15 s × 3 = 45 s and DB-free, so a DB outage never restarts pods. Startup 3 s × 20 = 60 s boot budget.
- #187's default
- Required sibling env vars are all set —
CORS_ORIGIN: '*'(mandatory under #184's secure default, and valid because #184 setscredentials: false; without it this manifest would ship a deployment that blocks mina-explorer's browser client outright),TRUST_PROXYexplicitly set in both artifacts (mandatory under #185, which disables the limiter entirely when unset),ENABLE_METRICS: 'true'matching #191's=== 'true'gate.
Ordering, not a defect: ghcr.io/o1-labs/archive-node-api:1.0.0 does not exist yet — the current GHCR tag list is 0.0.1-test, latest, 0.0, 0, 0.0.1, 0.0.2, 0.0.4, 0.0.5, 0.0.8, 0.0.6, 0.0.9. Unlike npm (see #208), Docker publishing does work — build.yaml pushes GHCR tags on refs/tags/v* and 0.0.9 is there — so 1.0.0 appears the moment v1.0.0 is tagged. Just make sure v1.0.0 is cut from a main that already contains #187 (/readiness), #191 (/metrics), #188 (SHUTDOWN_TIMEOUT_MS), #184 and #185 — otherwise this manifest's probes 404 and its env vars are no-ops against the image it names.
Non-blocking nits
-
deploy/README.md:46repeats the incorrect claim (which I have blocked #186 on) thatTRUST_PROXY=0is "the safe default … leaving it unset behind an ingress collapses every client into a single rate-limit bucket." Under #185 as shipped there is no default:useRateLimit()returns a no-op plugin whenTRUST_PROXYis unset, so rate limiting is disabled outright, not collapsed. Suggested:…but the API disables rate limiting entirely while
TRUST_PROXYis unset (there is no safe default), so it must be set explicitly —0for a directly exposed server, or the real hop count behind a gateway. -
deploy/docker-compose.prod.ymlhas nostop_grace_period. Docker's default is 10 s, but #188'sSHUTDOWN_TIMEOUT_MSdefault is 20000 — so everydocker compose up -dredeploy SIGKILLs mid-drain, which is the same defect this PR just fixed on the k8s side. Add, mirroring the k8s reasoning:stop_grace_period: 30s
-
The
TRUST_PROXY: '1'comment (kubernetes.yaml:73-76) says "set it to the real hop count" but doesn't name the case that bites, and its first clause ("Rate limiting keys on the socket address while this is 0") describes behaviour #185 no longer has. A GKE/GCP external ALB appends two XFF entries, so1keys every client on the forwarding-rule IP — which is exactly round 1's blocker, reproduced. #185's warning text, #186 and #197 all say 2. Worth correcting here since this is the artifact people copy:# Number of proxy hops in front of the API. A single in-cluster # ingress is 1; a GCP external ALB is 2 (client IP, then # forwarding-rule IP), plus 1 per extra hop. Too low collapses every # client into one bucket; leaving it unset disables rate limiting.
-
PG_MAX_CONNECTIONSis unset, so #182's default of 10 applies: at the HPA's max of 6 replicas that is 60 connections, plus one readiness client each (#187), against a stockmax_connections = 100. Worth pinning explicitly next toNODE_OPTIONSso the sizing is visible. -
The Compose example publishes
8080:8080withENABLE_METRICS: 'true', which on a0.0.0.0-bound host makes/metricspublicly reachable. The k8s path is fine — it usesprometheus.io/*pod annotations, so Prometheus scrapes the pod IP rather than the Service. Worth a line in the Compose comment.
Downstream: none. CORS_ORIGIN: '*' keeps mina-explorer's browser SPA working. mina-explorer-api is server-side and unaffected by CORS; its { __typename } probe and 20 s client timeout are untouched. No schema or error-text surface.
The readiness probe targets /readiness, which only exists from 1.0.0, but the manifests pulled :latest — today that resolves to 0.0.6. Applied as-is the probe would 404 forever, no pod would reach Ready, and the Service would be left with zero endpoints: a total outage from a manifest offered as the copy-paste reference. Both manifests now pin 1.0.0 and state the requirement up front. The /metrics scrape annotations had the same dependency. CORS_ORIGIN shipped commented out, described as "leave unset for same-origin only". Unset blocks every cross-origin browser client — the mina-explorer included — and does so silently, with nothing in the server logs. Since the primary consumers are browsers, it now ships set, with the trade-off spelled out. deploy/README.md repeated the root README's claim that pointing PG_CONN at replicas buys throughput. It buys failover: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Real read scaling needs a balancer in front of Postgres. Also sets TRUST_PROXY=1 in the k8s manifest (an ingress adds a hop, and the default of 0 would bucket every client together) and corrects terminationGracePeriodSeconds from "matches" to "exceeds" SHUTDOWN_TIMEOUT_MS, which is the property that actually matters. Verified with kubectl apply --dry-run=client and docker compose config. Addresses review feedback on #196. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6f89b0f to
a6b9328
Compare
The readiness probe targets /readiness, which only exists from 1.0.0, but the manifests pulled :latest — today that resolves to 0.0.6. Applied as-is the probe would 404 forever, no pod would reach Ready, and the Service would be left with zero endpoints: a total outage from a manifest offered as the copy-paste reference. Both manifests now pin 1.0.0 and state the requirement up front. The /metrics scrape annotations had the same dependency. CORS_ORIGIN shipped commented out, described as "leave unset for same-origin only". Unset blocks every cross-origin browser client — the mina-explorer included — and does so silently, with nothing in the server logs. Since the primary consumers are browsers, it now ships set, with the trade-off spelled out. deploy/README.md repeated the root README's claim that pointing PG_CONN at replicas buys throughput. It buys failover: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Real read scaling needs a balancer in front of Postgres. Also sets TRUST_PROXY=1 in the k8s manifest (an ingress adds a hop, and the default of 0 would bucket every client together) and corrects terminationGracePeriodSeconds from "matches" to "exceeds" SHUTDOWN_TIMEOUT_MS, which is the property that actually matters. Verified with kubectl apply --dry-run=client and docker compose config. Addresses review feedback on #196. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a6b9328 to
90c4a67
Compare
There were no production deployment artifacts — operators had npm/Docker/Compose for dev but no opinionated manifest with probes, resource limits, and a hardened runtime. Add deploy/: - kubernetes.yaml — Deployment + Service + HPA (+ placeholder Secret) with liveness (/healthcheck) and readiness (/readiness) probes, resource requests/limits, a 2→6 CPU autoscaler, Prometheus scrape annotations for /metrics, a hardened pod securityContext (non-root, readOnlyRootFilesystem, no privilege escalation, all caps dropped, RuntimeDefault seccomp), and a 30s termination grace period matching the graceful-shutdown drain. - docker-compose.prod.yml — the published image against an external read-only Postgres, with CPU/memory caps. - README.md — usage and how this maps to the security deployment contract. Linked from the root README. References the probe/metrics endpoints delivered by the sibling P1 PRs. Closes #179. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
The readiness probe targets /readiness, which only exists from 1.0.0, but the manifests pulled :latest — today that resolves to 0.0.6. Applied as-is the probe would 404 forever, no pod would reach Ready, and the Service would be left with zero endpoints: a total outage from a manifest offered as the copy-paste reference. Both manifests now pin 1.0.0 and state the requirement up front. The /metrics scrape annotations had the same dependency. CORS_ORIGIN shipped commented out, described as "leave unset for same-origin only". Unset blocks every cross-origin browser client — the mina-explorer included — and does so silently, with nothing in the server logs. Since the primary consumers are browsers, it now ships set, with the trade-off spelled out. deploy/README.md repeated the root README's claim that pointing PG_CONN at replicas buys throughput. It buys failover: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Real read scaling needs a balancer in front of Postgres. Also sets TRUST_PROXY=1 in the k8s manifest (an ingress adds a hop, and the default of 0 would bucket every client together) and corrects terminationGracePeriodSeconds from "matches" to "exceeds" SHUTDOWN_TIMEOUT_MS, which is the property that actually matters. Verified with kubectl apply --dry-run=client and docker compose config. Addresses review feedback on #196. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
90c4a67 to
8cc2bd2
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-approving after the rebase (the previous approval was dismissed by the force-push).
Re-verified the delta against the commit I approved (a6b9328). The deploy/ manifests — the substance of this PR — are unchanged. The only difference is in README.md: the read-replica prose this PR used to correct has since landed on main via another PR, so the branch no longer carries that hunk (correct, not a drop), and the remaining churn is prettier table reformatting plus this PR's own new ## Deployment section. I diffed the documented env-var sets both ways — nothing was dropped. All checks green.
Carrying forward the round-2 verification unchanged:
preStop sleep 15+terminationGracePeriodSeconds 45+SHUTDOWN_TIMEOUT_MS 10000(25s inside 45s) — this closes round 1's unowned cross-cutting gap.- The Secret is its own file and the README applies
-f kubernetes.yaml(a single file, not a directory loop), so a re-apply cannot clobber a realPG_CONN. - HPA/resources coherent: 500m request, no CPU limit, scale at 70%.
- Probe arithmetic checks out: 2s bounded ping < 3s probe timeout; 10s × 6 = 60s tolerance vs #182's 15s
statement_timeout, a 4x margin. - Sets
CORS_ORIGINandTRUST_PROXYexplicitly — both are mandatory once #184 and #185 land.
Non-blocking:
deploy/README.md:46still callsTRUST_PROXY=0"the safe default". #185 as shipped gives it no default — unset means the limiter is not installed at all. Prose only; the manifests set the var, so applying them works. This is the same underlying misconception blocking #186 — worth fixing all of them in one pass.docker-compose.prod.ymlhas nostop_grace_period, so Docker's 10s default SIGKILLs mid-drain againstSHUTDOWN_TIMEOUT_MS 10000.deploy/kubernetes.yamlstill shipsTRUST_PROXY '1'; a GCP external ALB appends twoX-Forwarded-Forentries and needs2.- The k8s probe
failureThresholdis 6 while #187's docs say 3 — pick one.
Release ordering: this pins ghcr.io/...:1.0.0, which does not exist yet. Cut v1.0.0 only from a main that already contains #184, #185 and #188, or these manifests set env vars the image ignores.
What & why
Part of the production-readiness epic (#163). Closes #179.
There were no production deployment artifacts — operators had npm/Docker/Compose for dev but no opinionated manifest with probes, resource limits, and a hardened runtime.
Adds
deploy/kubernetes.yaml—Deployment+Service+HorizontalPodAutoscaler(+ placeholderSecret) with production defaults:/healthcheck, readiness on/readinessreadOnlyRootFilesystem, no privilege escalation, all caps dropped,RuntimeDefaultseccomp)/metricsterminationGracePeriodSeconds: 30matching the graceful-shutdown draindocker-compose.prod.yml— the published image against an external read-only Postgres, with CPU/memory caps.README.md— usage + how it maps to the security deployment contract.Linked from the root README. References the probe/metrics endpoints delivered by the sibling P1 PRs (#169/#173).
Testing
Docs/manifests only.
prettier --debug-check .clean; YAML structure validated. No application code changed.🤖 Generated with Claude Code