Merge release-2026.02.01 into release-2026.06.01 - #42
Merged
hhuuggoo merged 11 commits intoJul 28, 2026
Conversation
Land phoebe on release-2026.02.01
…on DDL The live-Postgres conformance test (TestIntegration_*/TestConformance_*) proved the rater's production SQL computes correct money — but against a hand-copied `schemaDDL` constant, not the migration that actually ships. The migration comments said "keep the two in sync"; that sync was enforced by a human reading two files. A column-type or index-expression drift between the copy and the migration would pass green here and break in prod — exactly the silent wrong-money failure this test exists to prevent. Replace the hand-copied constant with `ratingSchemaDDL(t)`, which loads the real migration files (migrations/0001_billing_event.sql then 0002_rating.sql, the production apply order — base_model is declared in 0001 and re-added IF NOT EXISTS in 0002_rating, a harmless overlap). The schema now CANNOT drift from production: if a migration changes, the conformance test runs against the new shape automatically. The old constant is retained, renamed `schemaDDLReference`, as a readable in-file description only (not applied — editing it edits a comment). The CI integration-test job (postgres:16 service, `make integration-test`) was already wired but tested a copy; it now tests the real schema on every PR. Verified: builds with -tags=integration, go vet clean, the full conformance suite passes against a throwaway postgres:16 loading the migration DDL, and the non-integration unit suite is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rating: run integration conformance against the REAL migration DDL
…e-id label
The cached/chain resolver strategies had their cache machinery built but their
LookupFunc was a STUB (conventionLookup) that fell back to a naming-template
guess. That guess can't work for Saturn inference: the model's Service name is
pd-{identity5}-{name}-{id}, which embeds the owning group and endpoint name —
neither of which phoebe receives. So static/convention were walking-skeleton
modes; the real multi-model design (cached/chain) was never wired.
Implement the real LookupFunc (internal/registry/k8s.go): resolve a deployment
id to its model Service by the saturncloud.io/resource-id label, read off the
in-cluster Kubernetes API. That label's value IS X-Saturn-Resource-Id (Saturn
stamps it on every inference Service via basic_resource_labels), so it's an
exact join key — no name reconstruction, no new Atlas API, no new headers. It's
self-correcting: the Service NAME template can change Atlas-side without breaking
phoebe.
- Select `resource-id=<id>,service-type=internal` — the service-type clause is
load-bearing: a deployment's ssh Service shares the resource-id label, so
without it the match is ambiguous. (Tested.)
- Port: prefer the port named "8000" (Route.port_name == str(container_port), and
the vLLM serve port is 8000 == Deployment.proxy_port); single-port fallback;
error rather than silently pick a wrong port on a multi-port Service. (Tested.)
- 0 matches → ErrNotFound (CachedResolver negative-caches, short TTL, so a new
model is reachable fast). API error / ambiguous / no-served-port → transient
error (NOT cached, retried). (Tested.)
- Wire k8sLookup into buildResolver for cached/chain (replacing conventionLookup).
chain keeps convention as a fallback ONLY for k8s-API-unreachable.
- New config registry.k8sNamespace (Saturn: "main-namespace"); required for
cached/chain. Fixed the misleading placeholder convention comment in
settings.example.yaml so nobody flips strategy:convention and gets silent 404s.
- Adds client-go (in-cluster client). Unit-tested with the fake clientset — no
cluster needed; 8 named tests covering the invariants + negatives + attacks.
ROUTING CONTRACT (one-way door, for Hugo's review): phoebe now depends on two
Atlas-owned k8s conventions for inference routing — (1) inference Services carry
saturncloud.io/resource-id + service-type=internal; (2) the served port is 8000.
Both hold today; this promotes them to a documented contract so an Atlas refactor
can't silently break routing. No Atlas change required to ship this.
Deploy note: the interceptor pod needs an RBAC Role granting get/list on services
in main-namespace (a saturn-k8s chart change, separate PR).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
registry: real control-plane resolver (k8s Service lookup by resource-id label)
phoebe no longer decides where to forward. The backend identity now rides on the request as the X-Saturn-Upstream header, which Atlas injects on each inference deployment's per-subdomain IngressRoute — Atlas knows the real Service for that exact deployment when it builds the route, so the address is authoritative, not a guess. WHY (security): phoebe sits behind auth-server, which authorizes a request for a SPECIFIC deployment. If phoebe re-derives the upstream and gets it wrong, an authorized-for-X request is served by engine Y — a confused deputy that silently breaks the resource-scoping auth-server enforced. The old resolver had three strategies for this: static (ignored the resource entirely), convention (GUESSED the Service DNS from a name template — could misroute authenticated traffic to a live wrong model), and a k8s-label lookup. All of them made phoebe a routing authority it should never have been. Atlas already solved routing at route-build time (one IngressRoute per subdomain → the deployment's own Service); the resolver reimplemented that, worse. So: forward to the address on the envelope, fail closed if it's missing. - proxy: read X-Saturn-Upstream, parse (bare host:port → http, or full URL), forward there. Absent/unparseable/hostless → 502, REFUSE. There is no other source of a target, so misrouting authenticated traffic is structurally impossible, not merely discouraged. Test TestProxyNoUpstreamFailsClosed pins it: no header → 502, ZERO billing events. - Delete internal/registry wholesale (static/convention/cached/chain, LookupFunc, the LRU, the k8s client) and its config surface (defaultUpstream, the registry block). Drops the k8s.io/client-go dependency. - identity: add HeaderUpstream with the anti-spoof contract note. Contracts: - NEW trusted header X-Saturn-Upstream (value: host:port or scheme://host:port), the routing authority. MUST be in Traefik's ForwardAuth authResponseHeaders allowlist (strip-then-inject) so a client cannot spoof its own upstream — same discipline as the identity headers. (saturn-k8s + Atlas changes, separate PRs.) - REMOVED config: defaultUpstream and the entire registry: block. A deployed config still setting them is ignored (unknown yaml keys), not rejected. - phoebe is now USELESS until Atlas injects the header — these land together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ttery fix) Two CONFIRMED battery findings on the routing-authority parse boundary, both ledger-endorsed SSRF-surface items: - A non-http scheme (https/ftp/gopher://...) passed through and was forwarded with that transport — e.g. https:// makes phoebe attempt a TLS handshake to a plain-HTTP in-cluster engine (502), and gopher:// is forwarded verbatim. - A path/query/fragment (host:8000/foo) passed through, and NewSingleHostReverseProxy PREPENDS the upstream path onto the request path — silently rewriting where every request lands on the engine (path smuggling). parseUpstream now rejects a non-http scheme and any path/query/fragment, in addition to the existing empty/unparseable/hostless checks — fail closed (→ 502, zero billing events), the same contract as a missing header. Defense-in-depth at the documented chokepoint: Atlas injects a clean bare host:port and the header is anti-spoofed, but the parse boundary enforces its own contract loudly. Adds TestParseUpstream (by-attack matrix): accepts host:port / http://host:port (trimmed); rejects https/gopher/ftp schemes, path/query/fragment smuggling, and empty/hostless input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The resolver deletion left an 'upstream *url.URL' param on the Server-building test helpers (newIOLogServer, newIOLogServerWithLog, newTestServerWithSettings, newTestServerE) — the Server no longer takes an upstream (routing is via the X-Saturn-Upstream header), so the param is unused and revive failed lint. Rename each to _ (the request helpers that actually USE upstream are unchanged). Verified clean with golangci-lint v1.64.8 (CI's version): exit 0 repo-wide. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route by X-Saturn-Upstream header; delete the resolver
Conflicts resolved in favor of release-2026.06.01, which already carries the 2026.02.01 upstream-header hardening in evolved form (parseUpstreamHeader, strict superset: also rejects userinfo). The older branch's TestParseUpstream unit test is brought forward adapted to the renamed function, with a userinfo reject case added. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REnqDYkurGAGSDZY641gHc
The auto-merge combined 2026.02.01's real-migration-DDL loader with six newer-branch tests still calling the deleted schemaDDL constant, and the loader's file paths predate the golang-migrate .up.sql/.down.sql rename. Point the six call sites at ratingSchemaDDL(t) and load the .up.sql files. Verified with go test -tags=integration against a live Postgres. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REnqDYkurGAGSDZY641gHc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Forward-merge of
release-2026.02.01intorelease-2026.06.01. Commits brought forward:Conflicts resolved
All conflicts were the same shape: PR #30's upstream-header hardening already landed on
release-2026.06.01in evolved, strictly-stronger form (parseUpstreamHeader— same checks plus userinfo rejection, reading the upstream off the identity struct).internal/proxy/proxy.go,internal/identity/identity.go,internal/e2e/e2e_test.go— kept therelease-2026.06.01side (superset of the older change; identity.go's conflict was only the newerHeaderOrgIDblock the older branch lacked).internal/proxy/proxy_test.go— kept the newer handler-level tests AND brought forward the older branch'sTestParseUpstreamunit test, renamed/retargeted toparseUpstreamHeader(it adds accept-case coverage — bare host:port, explicit http, trimming — that the newer tests lacked), with a userinfo reject case added.go.mod/go.sum— kept the newer side;go mod tidyconfirms nothing in the merged tree needs the older branch's testify-transitive additions.Verified:
go build ./...,go vet ./...,gofmt -lclean,go test ./...all packages ok.Contracts
No new contracts — forward-merge of commits already reviewed on
release-2026.02.01. TheX-Saturn-Upstreamrouting contract (host:port grammar, fail-closed 502) is unchanged; the newer branch's stricter variant wins everywhere.https://claude.ai/code/session_01REnqDYkurGAGSDZY641gHc