fix(gateway)!: return error, not *StreamError, from the exported scope API

docs/adopting.md

@@ -19,6 +19,38 @@ namespaced resource. `ScopePolicy` makes the host choose explicitly.
 Keep all-namespaces access rare. It changes the size, disclosure risk, and operating cost of a stream.
 Use an `Authorizer` to pin a user to a namespace or target before any watch opens.
 
+### Errors from the exported surface are `error`
+
+`ScopeFromQuery` and `ScopePolicy.Validate` return `error`, and the concrete value is a
+`*StreamError`. Reach the wire code with `errors.As`:
+
+```go
+var serr *gateway.StreamError
+if errors.As(err, &serr) {
+	log.Printf("refused with %s: %s", serr.Code, serr.Message)
+}
+```
+
+They return the interface rather than `*StreamError` on purpose. A fallible function returning a
+concrete pointer type is the Go typed-nil trap: assign its result into a variable already declared as
+`error` and a *successful* call comes back non-nil, because an interface holding a typed nil is not
+nil. In a scope check that is a refusal you cannot explain. In an authorization check written the
+same way, with the condition inverted, it is an admission you never see.
+
+## 1b. Targets, and hosts that carry a path
+
+A `rest.Config` whose `Host` includes a path prefix works. A kcp workspace URL
+(`https://kcp.example/clusters/root:org:ws`) is an ordinary host as far as client-go is concerned, and
+the dynamic client appends `/apis/...` to it correctly. Nothing in the gateway parses, rewrites or
+second-guesses that URL: the host resolves a target to a backend, and the backend is whatever
+`rest.Config` you built.
+
+Build that URL with `kube.NewBackendForConfig(cfg)` rather than `kube.NewBackend(dynamicClient)` when
+you can. Both work, but only the former knows the address it dialed, so an upstream failure can name
+it. That matters most exactly here: get a path-prefixed URL subtly wrong (two `/clusters/` segments,
+say) and the API server answers `the server could not find the requested resource`, which is
+indistinguishable from "that CRD is not installed" until the error says which URL it asked.
+
 ## 2. Mount the same-origin cookie endpoint
 
 ```go

gateway/handler.go

@@ -108,12 +108,12 @@ func Handler(o Options) http.Handler {
 			return
 		}
 
-		scope, serr := ScopeFromQuery(r.URL.Query())
-		if serr == nil {
-			serr = o.Scopes.Validate(scope)
+		scope, err := ScopeFromQuery(r.URL.Query())
+		if err == nil {
+			err = o.Scopes.Validate(scope)
 		}
-		if serr != nil {
-			refuse(w, serr)
+		if err != nil {
+			refuse(w, asStreamError(err))
 			return
 		}
 

gateway/kube/backend.go

@@ -54,6 +54,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/rest"
 	"k8s.io/utils/ptr"
 
 	"github.com/ConfigButler/krm-stream/gateway"
@@ -73,6 +74,13 @@ const initialEventsEndAnnotation = metav1.InitialEventsAnnotationKey
 type Backend struct {
 	client dynamic.Interface
 
+	// host is what this backend dialed, for error messages ONLY — it is never sent to a browser and
+	// never parsed. An upstream error that names neither the server nor the resource path reads like
+	// "that resource does not exist", when what actually happened may be "you built the wrong URL".
+	// A kcp workspace, whose kubeconfig carries a path prefix, is exactly where the two are
+	// indistinguishable without this.
+	host string
+
 	// mu guards listThenWatch, which is a per-GroupVersion memory of "this API refused the
 	// streaming list". It is a cache of a fact about the SERVER, not about a request, so it is
 	// shared across every stream this Backend serves.
@@ -81,13 +89,54 @@ type Backend struct {
 }
 
 // NewBackend wraps a dynamic client as a gateway upstream.
+//
+// The client alone cannot say what server it points at, so errors from this constructor name the
+// resource path but not the host. Prefer NewBackendForConfig, which knows both.
 func NewBackend(client dynamic.Interface) *Backend {
 	return &Backend{
 		client:        client,
 		listThenWatch: map[schema.GroupVersion]bool{},
 	}
 }
 
+// NewBackendForConfig builds the dynamic client AND remembers the address it will dial, so that an
+// upstream failure can say where it happened.
+//
+// Path-prefixed hosts work, and this is the constructor that makes them debuggable: a kcp workspace
+// URL (https://kcp.example/clusters/root:org:ws) is an ordinary rest.Config.Host as far as client-go
+// is concerned, and the dynamic client appends /apis/... to it correctly. If you build that URL
+// wrongly — two /clusters/ segments, say — the API server answers "the server could not find the
+// requested resource", which is indistinguishable from "that CRD is not installed" until the error
+// tells you which URL it dialed. Now it does.
+func NewBackendForConfig(cfg *rest.Config) (*Backend, error) {
+	client, err := dynamic.NewForConfig(cfg)
+	if err != nil {
+		return nil, fmt.Errorf("krm-stream/kube: dynamic client for %s: %w", cfg.Host, err)
+	}
+	b := NewBackend(client)
+	b.host = cfg.Host
+	return b, nil
+}
+
+// upstream describes what a failing call was actually reaching for: the server, and the resource path
+// built from the scope. Both halves are what a "could not find the requested resource" needs before
+// it can be diagnosed rather than guessed at.
+func (b *Backend) upstream(scope gateway.Scope) string {
+	path := "/api/" + scope.Version
+	if scope.Group != "" {
+		path = "/apis/" + scope.Group + "/" + scope.Version
+	}
+	if scope.Namespace != "" {
+		path += "/namespaces/" + scope.Namespace
+	}
+	path += "/" + scope.Resource
+
+	if b.host == "" {
+		return path
+	}
+	return strings.TrimSuffix(b.host, "/") + path
+}
+
 var _ gateway.Backend = (*Backend)(nil)
 
 // Watch opens a snapshot-then-live stream for one scope.
@@ -114,7 +163,7 @@ func (b *Backend) Watch(ctx context.Context, scope gateway.Scope) (gateway.Watch
 		// is NOT ours to paper over. Falling back on every failure would turn "you may not watch
 		// Secrets" into a second, differently-worded permission denial, which is how a gateway ends
 		// up lying about why it could not open a stream.
-		return nil, fmt.Errorf("krm-stream/kube: streaming list for %s: %w", scope.Resource, err)
+		return nil, fmt.Errorf("krm-stream/kube: streaming list for %s: %w", b.upstream(scope), err)
 	}
 
 	// F6, in production. This API is aggregated (or otherwise has WatchList off); §3b is not a
@@ -181,7 +230,7 @@ func (b *Backend) rememberListThenWatch(gv schema.GroupVersion) {
 func (b *Backend) listThenWatchStream(ctx context.Context, ri dynamic.ResourceInterface, scope gateway.Scope) (gateway.Watcher, error) {
 	list, err := ri.List(ctx, selectors(scope))
 	if err != nil {
-		return nil, fmt.Errorf("krm-stream/kube: list %s: %w", scope.Resource, err)
+		return nil, fmt.Errorf("krm-stream/kube: list %s: %w", b.upstream(scope), err)
 	}
 
 	o := selectors(scope)
@@ -190,7 +239,7 @@ func (b *Backend) listThenWatchStream(ctx context.Context, ri dynamic.ResourceIn
 	w, err := ri.Watch(ctx, o)
 	if err != nil {
 		return nil, fmt.Errorf("krm-stream/kube: watch %s from resourceVersion %q: %w",
-			scope.Resource, list.GetResourceVersion(), err)
+			b.upstream(scope), list.GetResourceVersion(), err)
 	}
 
 	// The snapshot, in the shape §3a would have delivered it — including the boundary bookmark,

gateway/scope.go

@@ -32,13 +32,15 @@ var forbiddenParams = []string{"server", "apiserver", "api-server", "url", "endp
 
 // ScopeFromQuery parses and validates a scope from a URL query, or explains why it will not.
 //
-// The error is a *StreamError with CodeScopeInvalid and Terminal set: a browser's EventSource
-// reconnects on its own, so a non-terminal rejection would have it hammer a malformed scope forever.
+// It returns `error`, not `*StreamError`, and that is deliberate — see the note on ScopePolicy.Validate.
+// The concrete value IS a *StreamError with CodeScopeInvalid and Terminal set (a browser's EventSource
+// reconnects on its own, so a non-terminal rejection would have it hammer a malformed scope forever);
+// reach it with errors.As when you need the code.
 //
 // Unknown parameters are TOLERATED (a host may carry its own — the replay server has `fixture` and
 // `pace`) except for the ones in forbiddenParams, which are the ones that would mean something
 // dangerous if we were the kind of gateway that honoured them.
-func ScopeFromQuery(q url.Values) (Scope, *StreamError) {
+func ScopeFromQuery(q url.Values) (Scope, error) {
 	for _, p := range forbiddenParams {
 		for key := range q {
 			if strings.EqualFold(key, p) {
@@ -181,7 +183,29 @@ type ScopePolicy struct {
 }
 
 // Validate reports whether a scope is one this gateway will stream at all.
-func (p ScopePolicy) Validate(s Scope) *StreamError {
+//
+// # Why this returns `error` and not `*StreamError`
+//
+// Because returning a concrete pointer type from a fallible exported function is the Go typed-nil
+// trap, and this one is a security control. A caller writing the ordinary thing:
+//
+//	var err error          // …or an err already in scope from something else
+//	if err = policy.Validate(scope); err != nil {
+//		return err     // FIRES ON SUCCESS. Always.
+//	}
+//
+// gets a non-nil `error` holding a nil *StreamError, because an interface holding a typed nil is not
+// nil. Validate said yes and the caller read no. That version fails safe, and it is the lucky one: an
+// authorizer written the same way refuses everyone and someone notices in a minute. Invert the check —
+// `if err == nil { serve() }` — and the same bug admits everyone, silently, and nobody notices at all.
+//
+// So the exported surface speaks `error`, which callers cannot get wrong. The concrete value is still
+// a *StreamError, and the gateway still switches on it internally; a host that wants the wire code
+// asks for it explicitly:
+//
+//	var se *gateway.StreamError
+//	if errors.As(err, &se) { … se.Code … }
+func (p ScopePolicy) Validate(s Scope) error {
 	if s.LabelSelector != "" && !p.AllowLabelSelector {
 		return ScopeInvalid("label selectors are not enabled for this stream endpoint")
 	}

gateway/scope_conformance_test.go

@@ -2,6 +2,7 @@ package gateway_test
 
 import (
 	"encoding/json"
+	"errors"
 	"net/url"
 	"os"
 	"testing"
@@ -48,12 +49,18 @@ func TestScopeFromQueryConformance(t *testing.T) {
 				t.Fatalf("the fixture's own query does not parse: %v", err)
 			}
 
-			got, serr := gateway.ScopeFromQuery(q)
+			got, err := gateway.ScopeFromQuery(q)
 
 			if c.Code != "" {
-				if serr == nil {
+				if err == nil {
 					t.Fatalf("expected %s (%s), but the scope was ACCEPTED as %+v", c.Code, c.Because, got)
 				}
+				// errors.As, because the exported surface speaks `error` — see ScopePolicy.Validate on
+				// why. This is the pattern a host copies to get at the wire code.
+				var serr *gateway.StreamError
+				if !errors.As(err, &serr) {
+					t.Fatalf("rejection was not a *StreamError: %v", err)
+				}
 				if string(serr.Code) != c.Code {
 					t.Errorf("code = %s, want %s", serr.Code, c.Code)
 				}
@@ -65,8 +72,8 @@ func TestScopeFromQueryConformance(t *testing.T) {
 				return
 			}
 
-			if serr != nil {
-				t.Fatalf("valid scope was rejected: %v (%s)", serr, c.Because)
+			if err != nil {
+				t.Fatalf("valid scope was rejected: %v (%s)", err, c.Because)
 			}
 			if got != c.Scope {
 				t.Errorf("scope = %+v, want %+v", got, c.Scope)

gateway/scope_policy_test.go

@@ -1,6 +1,41 @@
 package gateway
 
-import "testing"
+import (
+	"net/url"
+	"testing"
+)
+
+// The typed-nil trap, pinned shut — in the exact shape that bit the first host to adopt this.
+//
+// Validate and ScopeFromQuery must return `error`, not `*StreamError`. If either ever goes back to
+// the concrete pointer type, THIS TEST STILL COMPILES and starts failing, because assigning a nil
+// *StreamError into an `error` produces a non-nil interface. That is the whole bug: the gateway says
+// yes and the caller reads no.
+//
+// It fails safe here (a refusal nobody expected) and it fails safe in an authorizer written the same
+// way (everyone refused, someone notices in a minute). Invert the check — `if err == nil { serve() }`
+// — and the identical bug admits everyone, silently. That asymmetry is why this is a test and not a
+// doc comment.
+func TestSuccessIsNilThroughTheErrorInterface(t *testing.T) {
+	policy := ScopePolicy{
+		Targets:   []string{"demo"},
+		Resources: []GroupResource{{Resource: "configmaps", Scope: ResourceScopeNamespaced}},
+	}
+	scope := Scope{Target: "demo", Version: "v1", Resource: "configmaps", Namespace: "app"}
+
+	// Declared as `error` FIRST, then assigned — which is what a host does, and what makes a concrete
+	// return type dangerous. `err := policy.Validate(...)` would hide this by inferring *StreamError.
+	var err error
+
+	if err = policy.Validate(scope); err != nil {
+		t.Fatalf("a valid scope came back non-nil through an error interface: %v — the typed-nil trap is back", err)
+	}
+
+	q := url.Values{"target": {"demo"}, "version": {"v1"}, "resource": {"configmaps"}, "namespace": {"app"}}
+	if _, err = ScopeFromQuery(q); err != nil {
+		t.Fatalf("a valid query came back non-nil through an error interface: %v — the typed-nil trap is back", err)
+	}
+}
 
 func TestScopePolicyRequiresExplicitNamespaceSemantics(t *testing.T) {
 	policy := ScopePolicy{

gateway/stream.go

@@ -497,9 +497,16 @@ func isDecimalResourceVersion(rv string) bool {
 	return true
 }
 
+// asStreamError finds the *StreamError a host meant to send, or makes one.
+//
+// The `se == nil` arm is the typed-nil trap seen from the other side. A host whose Authorizer is
+// declared to return `error` and returns a nil *StreamError hands us a non-nil error interface
+// wrapping nothing: errors.As matches, sets se to nil, and every path downstream would dereference it
+// to build the event. That is a panic in the gateway caused by a bug in a host — so it is refused as
+// an internal error instead, which is the honest description and does not take the process down.
 func asStreamError(err error) *StreamError {
 	var se *StreamError
-	if !errors.As(err, &se) {
+	if !errors.As(err, &se) || se == nil {
 		return &StreamError{Code: CodeInternal, Message: err.Error(), Terminal: true}
 	}
 	return se