feat(network): Route proxy-mode fields + persistence (pg/sqlite/mongo) + migrations - #32
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The new proxy-mode fields widened the struct column layout, leaving gofmt field alignment and tagalign tag alignment stale in network/ and store/mongo, and the new postgres round-trip test missing cuddling whitespace before its if statements. Whitespace only - no tag values, field names or logic changed.
octopus's path_mode (Strip|Passthrough) is the same axis as the StripPrefix bool ctrlplane already persists, already exposes on the public AddRoute request DTO, and already renders in the dashboard. Carrying both meant two columns encoding one decision, and no answer for a route that set StripPrefix=true alongside path_mode=passthrough. So StripPrefix stays the only path field. The annotation emitter maps true to "strip" and false to "passthrough", which is what the words mean anyway. Nothing about existing routes changes: today the emitter writes annotations only when StripPrefix is true, and octopus defaults an annotated route to Strip. That leaves four genuinely new fields: rewrite_redirects, rewrite_cookie_path, upstream_origin, tls_verify. All four are read by proxy_spec_from_annotations in octopus-k8s. Migrations shrink to match.
TLSVerify is the one proxy field whose safe value is true, which is why AddRouteRequest carries it as *bool rather than bool. An older client that never sends the key has to come out with verification on. Invert that condition by accident and every route created by such a client silently stops checking upstream certificates, with nothing failing. The table covers all three inputs: unset, explicit true, explicit false. Verified it catches the regression by flipping the production condition to `!= nil && *req.TLSVerify` first, which failed only the unset case, then restoring it.
The four proxy fields were create-only. You could stand a route up against an external origin but never rotate it, never turn redirect rewriting on for a route that already existed, and never restore verification after turning it off. UpdateRouteRequest now carries all four as pointers, so an omitted key still leaves the stored value alone. UpstreamOrigin is a *string rather than a string because the two cases differ: nil leaves the origin, and a pointer to "" clears it and sends the route back to its in-cluster backend. Clearing the origin also resets TLSVerify to true. Octopus only reads tls_verify when an origin is set, so a route sitting at tls_verify=false with no origin is dead state. Nothing surfaces it, nothing fails, and the next person to point that route at an origin inherits unverified TLS without ever asking for it. The reset wins over an explicit tls_verify=false in the same request, because honouring that would recreate the exact state the invariant exists to prevent. Turning verification off again works fine once a new origin is set. Tests cover all five orderings plus the two carry-over flags. Watched them fail first against the unimplemented block.
juicycleff
force-pushed
the
feat/route-proxy-fields
branch
from
September 3, 2026 03:41
6cf1d03 to
ee3daf0
Compare
Route has carried ServiceName and Hostname for a while, but no store ever wrote them. The three model-based stores map through hand-written routeModel structs, and neither field had a column, so both were dropped on insert and came back empty on the next read. Nothing errored. A route created against a named service in a multi-service instance quietly fell back to Main, and a host-scoped route widened onto every host sharing the wildcard listener. The memory and badger stores never had the problem, since one clones the struct and the other marshals it as JSON. That is what hid this. Every unit test in the repo runs against memory. Two additive columns per SQL store, both defaulting to empty, which is the behaviour existing routes already have. Mongo needs no migration. Round-trip tests now sit next to each mapper, because the mapping is copied out by hand three times and a field added to the domain struct will never fail to compile here.
Hostname was create-only. You could scope a route to a single host when you made it, and after that you were stuck: no way to move a workspace onto a new API hostname, no way to widen a route back onto the shared listener. ServiceName already worked this way, so the field slots into the same pointer contract. nil leaves the stored value alone, a pointer assigns, and a pointer to "" clears it. Clearing matters here. An empty hostname is not a missing value, it is the route answering on every host the gateway listener serves, so the difference between "leave it" and "unscope it" has to survive the wire. The table covers all four cases and asserts on the persisted copy as well as the returned route. Watched them fail first.
You could not create a route targeting a named service over HTTP, and you could not scope one to a host, even though AddRoute has accepted both for a while. The API DTOs replicate the body fields by hand, because network.AddRouteRequest claims the instance_id json tag that the path parameter needs, and the two sides had drifted apart. POST now takes service_name and hostname. PATCH takes both as pointers, so an omitted key still leaves the stored value alone. This also lands the proxy-field wiring that was sitting uncommitted in the tree, so strip_prefix, rewrite_redirects, rewrite_cookie_path, upstream_origin and tls_verify reach the service layer from both endpoints too. The mapping moves out of the handlers into toAddRouteRequest and toUpdateRouteRequest, which makes it testable without a forge.Context. Two tests guard it. One reflects over the json tags on both structs and fails on any field that one side accepts and the other does not, which is the exact drift that caused this. The other builds a fully populated DTO and fails on any zero value in the result, which catches the other half of the problem: a field that exists on both sides but never gets assigned. That second failure is the nastier one over HTTP, since the schema advertises the field, clients send it, and the server drops it without a word.
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.
What this does
Routes gain four proxy-mode fields, persisted across postgres, sqlite and mongo. twinos reads them to emit
octopus.io/*annotations on the HTTPRoute it builds, and octopus uses those to decide whether to rewrite redirects, where to send the request, and whether to check the upstream certificate. The gateway side of this shipped in octopus#3.RewriteRedirectsre-adds a stripped prefix toLocationheaders. Without it a backend mounted at/sends browsers outside the gateway prefix, and they 404.RewriteCookiePathdoes the same forSet-Cookiepath attributes.UpstreamOriginpoints the route at an absolutescheme://host[:port]outside the cluster.TLSVerifycontrols certificate verification against that origin.All four are read by
proxy_spec_from_annotationsinoctopus-k8s.What changed since the first version of this PR
There was a fifth field,
PathMode, carrying""/strip/passthrough, and it turned out to be the same axis as theStripPrefixbool that a Route already persists, already exposes onAddRouteRequest, and already renders in the dashboard. Two columns for one decision. Worse, nothing said what should happen to a route that setStripPrefix=truenext topath_mode=passthrough, and once both are columns every reader has to answer that question for itself.So
StripPrefixstays the only path field. The annotation emitter maps true tostripand false topassthrough, which is what the words mean anyway. Existing routes don't change: twinos writes annotations only whenStripPrefixis true today, and octopus defaults an annotated route toStrip.The proxy fields were also create-only in the first version, so you could stand a route up against an external origin but never rotate it, and never turn redirect rewriting on for a route that already existed.
UpdateRouteRequestnow carries all four.TLSVerify
This is the one field whose zero value is the unsafe one, so it's handled from both sides.
On create,
AddRouteRequest.TLSVerifyis a*bool. An older client that never sends the key comes out with verification on, not off.On update, clearing
UpstreamOriginresetsTLSVerifyto true. Octopus only readstls_verifywhen an origin is set, so a route sitting attls_verify=falsewith no origin is dead state: nothing surfaces it and nothing fails, right up until someone points that route at a new origin months later and inherits unverified TLS without ever asking for it. The reset wins over an explicittls_verify=falsein the same request, because honouring that would rebuild the exact state the invariant exists to prevent. Turning verification back off works fine once a new origin is set.UpstreamOriginis a*stringon the update request so you can tell "leave it alone" (nil) apart from "clear it and go back to the in-cluster backend" (pointer to""). With a plain string those two collapse into the same zero value, and the reset would fire on every update that never mentioned the origin, quietly turning verification back on under an operator who hadn't touched it.Persistence
Model fields and both mapping directions for postgres, sqlite and mongo. Migrations are additive: postgres
20240101000026as one comma-joinedALTER, sqlite20240101000020as four single-column ones, with text defaulting to'', bools to false, andtls_verifyto TRUE. Mongo is schemaless, so it gets no migration.Existing routes read back unchanged. No existing column is touched.
Testing
go build ./...,go vet ./...,go test ./...andgolangci-lint run ./...are all clean.TestRouteModel_ProxyFieldsRoundTripsets every field includingTLSVerify:false, to prove it is not defaulted away, then walks it through both mapping directions.TestAddRoute_TLSVerifyDefaultsTruecovers unset, explicit true and explicit false.TestUpdateRoute_ClearingOriginResetsTLSVerifycovers five orderings of origin and verify. A second test covers the two carry-over flags.Both new behaviour tests were watched failing first, the update one against the unimplemented block and the create one against a deliberately inverted condition, so we know they catch the regression they describe rather than just passing next to it.
Still open
twinos needs a consumer change to emit the new annotations, gated on a tagged release of this. Today it derives
path-modeandrewrite-redirectsfromStripPrefixalone, and pins ctrlplane v1.6.1.The HTTP API layer is behind the library DTO, and it already was before this PR:
AddRouteAPIRequestdoesn't carryStripPrefix, so you can PATCH it but can't set it at creation, and the four new fields have the same gap. That is a separate change, and twinos imports ctrlplane as a Go library so it isn't blocked on it.