Repository navigation
fix(gateway)!: return error, not *StreamError, from the exported scope API - #9
Conversation
…e API
The Go typed-nil trap, in a public API, in a security control. It cost the
first host to adopt this ten minutes:
var err error
if err = policy.Validate(scope); err != nil { ... } // FIRES ON SUCCESS
A nil *StreamError stored in an error interface is not nil, so a scope the
gateway accepted came back as a refusal — reported as the memorable
"must be one the gateway serves: <nil>".
That failure is safe only by luck. An Authorizer written the same way
refuses everyone and is noticed in a minute. Invert the check and the
identical bug admits everyone, silently.
ScopeFromQuery and ScopePolicy.Validate now return `error`. The concrete
value is still a *StreamError and the gateway still switches on it via the
asStreamError/errors.As machinery that already existed; a host that wants
the wire code asks for it explicitly. TestSuccessIsNilThroughTheErrorInterface
pins it shut: it declares `var err error` first, so it still compiles if the
signature regresses, and fails with the reporter's own message.
asStreamError is also hardened against the same trap arriving from a host:
errors.As matching a nil *StreamError used to hand every downstream path a
nil to dereference, which is a panic in the gateway caused by a bug in a
host. It is now refused as an internal error.
Second fix: kube upstream errors named neither the server they dialed nor
the resource path they built, so a mis-built path-prefixed URL (a kcp
workspace with two /clusters/ segments) reported "the server could not find
the requested resource" — indistinguishable from "that CRD is not
installed". Backend could not do better: it only ever held a
dynamic.Interface. NewBackendForConfig builds the client from a rest.Config
and remembers the host, and the three upstream wraps now name host + path.
NewBackend still works and omits the host it cannot know.
adopting.md documents errors.As, and states that a path-prefixed rest.Config
Host works — which was true, but only provable by trying it.
BREAKING CHANGE: ScopeFromQuery and ScopePolicy.Validate return error rather
than *StreamError. Callers reading .Code directly use errors.As instead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Items 1 and 4 from the gitops-api adoption report. Both are defects with no design fork, so they ship together; items 2 and 3 are public-API decisions and are being handled separately.
1. The typed-nil trap (the one to read)
ScopePolicy.ValidateandScopeFromQueryreturned*StreamError. So the ordinary Go thing:A nil
*StreamErrorin anerrorinterface is not nil. The gateway said yes; the caller read no. It surfaces asmust be one the gateway serves: <nil>.Why this outranks everything else on the list: that version fails safe by luck. An
Authorizerwritten the same careless way refuses everyone, and someone notices within a minute. Invert the condition —if err == nil { serve() }— and the identical bug admits everyone, silently, forever.Both now return
error. The concrete value is still a*StreamError, and the gateway still switches on it internally through theasStreamError/errors.Asmachinery that already existed — so this is a signature change, not a redesign. A host that wants the wire code asks for it:Pinned shut by a test that can actually fail.
TestSuccessIsNilThroughTheErrorInterfacedeclaresvar err errorfirst and then assigns, which is the shape that makes a concrete return type dangerous (err := …would hide it by inferring the pointer type). So it still compiles if the signature ever regresses, and it fails with the reporter's own<nil>message. I verified that by reverting the signature and watching it go red, then restoring it.Same trap, other direction.
asStreamErrormatched a nil*StreamErrorviaerrors.Asand handed every downstream path a nil to dereference — a panic in the gateway caused by a bug in a host whoseAuthorizerreturns a typed nil. Now refused as an internal error.4. Upstream errors that say where they went
krm-stream/kube: streaming list for configmaps: the server could not find the requested resourcenamed neither the host dialed nor the path built, so a mis-built kcp URL read exactly like "this CRD is not installed".Backendstructurally could not do better: it only ever held adynamic.Interface, which cannot say what it points at. SoNewBackendForConfig(cfg *rest.Config)builds the client and remembers the host, and the three upstream wraps now name host + resource path.NewBackend(client)still works, and omits the host it has no way to know.Wishlist item, taken
adopting.md now states that a
rest.ConfigwhoseHostcarries a path prefix works with kcp — which was true, but only provable by trying it — and documents theerrors.Aspattern.Not in this PR
Project): real.adoptSavedcurrently has no supported way to be fed, which is a bug in the shape of a missing function.onChangehas no uid): real, and wider than reported —applyStreamEventalso discardsadded,structuralandconflicts.Both are public-API shape decisions and get their own PR.
Gateway and kube suites pass;
gofmtandgo vetclean.🤖 Generated with Claude Code