From 4c6d1e5180cb07e96abfd4b213e02d3662015060 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 12:24:14 +0000 Subject: [PATCH 1/9] fix(gateway): open shared watches outside the backend lock SharedBackend.Watch held the backend-wide mutex across the upstream Watch call, a network round trip of unknown length, and gave that call a context nobody could cancel. One API server slow to answer one opening therefore held up every other scope, and a browser that left could not release its slot. Each scope's opening is now a private entry registered under the lock, opened on its own goroutine without it, and settled under it again. Callers wait on it through their own contexts and are counted; the last to leave cancels the opening's scope-owned context and unregisters it. A successful opening attaches one subscription per remaining waiter at the moment the scope is published, so there is no instant at which an upstream watch has no owner. An abandoned opening that returns late is stopped and discarded without touching the scope map, the cache, a replacement opening or the backoff, and a cancellation caused only by waiters leaving is never recorded as an upstream failure. Snapshot framing, cache ownership, overflow recovery, failure classification, backoff and lifecycle observations are unchanged. Co-Authored-By: Claude Opus 5.5 --- docs/operations.md | 5 + gateway/README.md | 6 + gateway/shared.go | 207 ++++++++++--- gateway/shared_open_test.go | 591 ++++++++++++++++++++++++++++++++++++ 4 files changed, 770 insertions(+), 39 deletions(-) create mode 100644 gateway/shared_open_test.go diff --git a/docs/operations.md b/docs/operations.md index 2ac3870..de46bc9 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -43,6 +43,11 @@ API server's request, where a reconnect would cost every browser a new connectio snapshot. The cost is that the browser is not told why it is waiting. A rising `syncing` time, without errors, is the signal to look at API-server load. +A shared scope that is slow to open holds up only the streams waiting for that scope. Each of them +can still end with its request, and when the last one leaves, the opening is cancelled rather than +left holding a slot. A client-go watch has no response-header timeout of its own, so a stalled +opening otherwise waits until every caller gives up. + A watch that ends before it is of use (before its snapshot completes, or within a second of it) is recovered once on the same connection. A second such end in a row closes the connection with `UPSTREAM_UNAVAILABLE`, so an upstream that ends every watch early meets the client's backoff and not diff --git a/gateway/README.md b/gateway/README.md index 49c730a..c7c1bd4 100644 --- a/gateway/README.md +++ b/gateway/README.md @@ -99,6 +99,12 @@ per-caller access decisions. `SharedOptions.QueueDepth` bounds live events per slow subscriber. Overflow triggers a resnapshot; it does not permit unbounded memory growth or silently drop events. +Each scope opens its upstream watch independently: a scope that is slow to open delays only the +callers waiting for that scope. A caller waiting for an opening leaves when its own request context +ends, and the last one to leave cancels the opening. The upstream `Backend` must honour context +cancellation for that to stop its work; a result it returns after everyone left is stopped and +discarded, and cannot affect a newer opening, the cache or the backoff. + ## Operations `Options.HeartbeatInterval` controls SSE keepalives. `Observer` provides low-cardinality lifecycle diff --git a/gateway/shared.go b/gateway/shared.go index 27b9bf7..c08f1c2 100644 --- a/gateway/shared.go +++ b/gateway/shared.go @@ -87,10 +87,42 @@ type SharedBackend struct { queueDepth int observer Observer - mu sync.Mutex - scopes map[string]*sharedScope - backoff map[string]*sharedBackoff - now func() time.Time + mu sync.Mutex + scopes map[string]*sharedScope + openings map[string]*sharedOpening + backoff map[string]*sharedBackoff + now func() time.Time +} + +// sharedOpening is one scope's upstream watch while it is being opened: registered under the +// backend's lock, opened without it, and settled under it again. +// +// It exists because opening is a network round trip of unknown length, and the backend's lock is +// taken by every scope. Holding the lock across it let one slow API server hold up every unrelated +// namespace, and gave a caller nothing to abandon: its own context was not the opening's, and nothing +// else could cancel it. Now the lock covers only the bookkeeping, and an opening belongs to whoever +// is waiting for it, counted, so the last one to leave cancels it. +type sharedOpening struct { + key string + scope Scope + // ctx is the upstream watch's own context: cancelled when the last waiter abandons the opening, + // and otherwise handed on to the scope it becomes, whose last subscriber cancels it. + ctx context.Context + cancel context.CancelFunc + // done is closed, under the backend's lock, once the opening has settled for its waiters. + done chan struct{} + + // Guarded by the backend's lock. + // + // waiters counts the callers still waiting. It changes only before done is closed; after that, + // every waiter takes one of subs (or err) on its way out, however it leaves. + waiters int + // subs holds one subscription per waiter when the opening succeeds, attached to the new scope + // at the moment it is published. A waiter therefore never has to subscribe after the fact, and + // there is no instant at which the scope holds an upstream watch and nobody who will release it. + subs []*subscriber + // err is the opening's failure, shared by every waiter. + err error } // sharedBackoff is one scope's failed attempts to open its upstream watch. @@ -120,6 +152,7 @@ func NewSharedBackendWithOptions(upstream Backend, options SharedOptions) *Share queueDepth: depth, observer: options.Observer, scopes: map[string]*sharedScope{}, + openings: map[string]*sharedOpening{}, backoff: map[string]*sharedBackoff{}, now: time.Now, } @@ -135,33 +168,136 @@ var _ Backend = (*SharedBackend)(nil) // Watch joins the shared watch for this scope, opening it if this is the first subscriber. // -// The caller's context is deliberately IGNORED, and that is the one surprising line in this file. It -// is the context of ONE browser's request; the upstream watch belongs to ALL of them. Honouring it -// here would mean the whole shared watch — and everyone else's stream — dies the moment whichever -// tab happened to open it goes away. A subscriber's own lifetime is bounded by Stop() instead, and -// the last one out cancels the upstream (see leave). -func (b *SharedBackend) Watch(_ context.Context, scope Scope) (Watcher, error) { +// The caller's context bounds only the caller's WAIT, never the upstream watch. It is the context of +// ONE browser's request; the upstream watch belongs to ALL of them. Handing it to the upstream would +// mean the whole shared watch — and everyone else's stream — dies the moment whichever tab happened to +// open it goes away. So the upstream gets a context of its own, owned by the scope: +// +// - while it is opening, by everyone waiting for it. Each may give up through its own context, and +// returns at once; the last to give up cancels the opening. +// - once it is open, by its subscribers. Each one's lifetime is bounded by Stop(), and the last one +// out cancels the upstream (see leave). +// +// The backend's lock is never held across the upstream's Watch, so a scope that is slow to open holds +// up only the callers waiting for that scope. +func (b *SharedBackend) Watch(ctx context.Context, scope Scope) (Watcher, error) { + // A caller that has already gone must not start, or join, an opening it would at once abandon. + if err := ctx.Err(); err != nil { + return nil, err + } key := scopeKey(scope) b.mu.Lock() - s, ok := b.scopes[key] + if s, ok := b.scopes[key]; ok { + b.mu.Unlock() + return s.subscribe() + } + op, ok := b.openings[key] if !ok { if wait := b.backoffRemainingLocked(key); wait > 0 { b.mu.Unlock() return nil, UpstreamUnavailable("the upstream is unavailable; retry later", wait) } - var err error - s, err = b.startScope(scope, key) - if err != nil { - b.recordFailureLocked(key, err) - b.mu.Unlock() - return nil, err + op = b.beginOpeningLocked(key, scope) + } + op.waiters++ + b.mu.Unlock() + + select { + case <-op.done: + return b.collect(op) + case <-ctx.Done(): + b.abandon(op) + return nil, ctx.Err() + } +} + +// beginOpeningLocked registers a scope's opening and starts it. Called with b.mu held; the upstream +// call itself runs on its own goroutine, without the lock. +func (b *SharedBackend) beginOpeningLocked(key string, scope Scope) *sharedOpening { + ctx, cancel := context.WithCancel(context.Background()) + op := &sharedOpening{key: key, scope: scope, ctx: ctx, cancel: cancel, done: make(chan struct{})} + b.openings[key] = op + go func() { + w, err := b.upstream.Watch(ctx, scope) + b.settle(op, w, err) + }() + return op +} + +// settle publishes an opening's result to its waiters, if it still has any. +// +// An opening every waiter abandoned is discarded whole: its watcher is stopped, and it touches +// neither the scope map, nor the backoff, nor whatever opening has since replaced it. Its failure, if +// it failed, is most likely the cancellation its waiters caused, and in any case nobody asked. +func (b *SharedBackend) settle(op *sharedOpening, w Watcher, err error) { + b.mu.Lock() + if b.openings[op.key] != op { // its last waiter left, and unregistered it + b.mu.Unlock() + op.cancel() + if w != nil { + w.Stop() + } + return + } + delete(b.openings, op.key) + if err != nil { + op.cancel() + b.recordFailureLocked(op.key, err) // once, however many are waiting + op.err = err + } else { + s := newSharedScope(b, op.key, op.scope, w, op.cancel) + b.scopes[op.key] = s + // Attach every waiter before the scope is visible to anyone else, so it is never without the + // subscribers whose departure will cancel it. + s.mu.Lock() + for range op.waiters { + op.subs = append(op.subs, s.attachLocked()) } - b.scopes[key] = s + s.mu.Unlock() + go s.pump(op.ctx) } + close(op.done) b.mu.Unlock() +} + +// collect hands a waiter its share of a settled opening: the failure, or one of the subscriptions +// attached on its behalf. +func (b *SharedBackend) collect(op *sharedOpening) (Watcher, error) { + b.mu.Lock() + defer b.mu.Unlock() + if op.err != nil { + return nil, op.err + } + sub := op.subs[len(op.subs)-1] + op.subs = op.subs[:len(op.subs)-1] + return &sharedWatcher{scope: sub.scope, sub: sub}, nil +} - return s.subscribe() +// abandon withdraws a waiter whose context ended. Before the opening settles, the waiter simply stops +// being counted, and the last one cancels the opening and unregisters it, so the next Watch starts +// afresh. After it settles — the two raced — the waiter still owns a subscription, and releases it +// exactly as a subscriber would. +func (b *SharedBackend) abandon(op *sharedOpening) { + b.mu.Lock() + select { + case <-op.done: // settled first: done is closed under b.mu, so this is decided + b.mu.Unlock() + if w, err := b.collect(op); err == nil { + w.Stop() + } + return + default: + } + op.waiters-- + last := op.waiters == 0 + if last && b.openings[op.key] == op { + delete(b.openings, op.key) + } + b.mu.Unlock() + if last { + op.cancel() // the upstream's Watch returns when it honours this; settle discards what it returns + } } func (b *SharedBackend) backoffRemainingLocked(key string) time.Duration { @@ -204,22 +340,13 @@ func (b *SharedBackend) recordFailureLocked(key string, err error) { bo.until = now.Add(wait) } -// startScope opens the one upstream watch for a scope and pumps it. Called with b.mu held. -// -// The upstream watch gets its OWN context, deliberately detached from the subscriber whose arrival -// happened to open it. Tying the shared watch to one browser's request context would mean that when -// that particular tab closes, everyone else's stream dies with it — a bug that would be invisible -// with one subscriber and baffling with two. -func (b *SharedBackend) startScope(scope Scope, key string) (*sharedScope, error) { - ctx, cancel := context.WithCancel(context.Background()) - - w, err := b.upstream.Watch(ctx, scope) - if err != nil { - cancel() - return nil, err - } - - s := &sharedScope{ +// newSharedScope wraps an opened upstream watch. Its context and cancel are the opening's: the +// watch's own, detached from whichever subscriber's arrival happened to open it. Tying the shared +// watch to one browser's request context would mean that when that particular tab closes, everyone +// else's stream dies with it — a bug that would be invisible with one subscriber and baffling with +// two. +func newSharedScope(b *SharedBackend, key string, scope Scope, w Watcher, cancel context.CancelFunc) *sharedScope { + return &sharedScope{ backend: b, key: key, scope: scope, @@ -228,8 +355,6 @@ func (b *SharedBackend) startScope(scope Scope, key string) (*sharedScope, error cache: map[string]KRMObject{}, subs: map[*subscriber]struct{}{}, } - go s.pump(ctx) - return s, nil } // scopeEnd is how a shared upstream watch ended, for the backoff. @@ -374,7 +499,12 @@ func (s *sharedScope) subscribe() (Watcher, error) { // stream loop will begin a new cycle, and that cycle opens a fresh shared scope. return nil, ErrWatchClosed } + sub := s.attachLocked() + return &sharedWatcher{scope: s, sub: sub}, nil +} +// attachLocked adds one subscriber to a live scope. Called with s.mu held. +func (s *sharedScope) attachLocked() *subscriber { sub := &subscriber{ scope: s, ch: make(chan WatchEvent, s.backend.queueDepth), @@ -390,8 +520,7 @@ func (s *sharedScope) subscribe() (Watcher, error) { if s.synced { s.deliverSnapshotLocked(sub) } - - return &sharedWatcher{scope: s, sub: sub}, nil + return sub } // deliverSnapshotLocked hands one subscriber the whole cache as a snapshot, terminated by the diff --git a/gateway/shared_open_test.go b/gateway/shared_open_test.go new file mode 100644 index 0000000..a46b92c --- /dev/null +++ b/gateway/shared_open_test.go @@ -0,0 +1,591 @@ +package gateway + +import ( + "context" + "errors" + "sync" + "testing" + "time" +) + +// Opening a shared watch is a network round trip of unknown length: a client-go watch has no +// response-header timeout of its own. These tests hold the upstream inside Watch, on a barrier, and +// prove what must stay true meanwhile: other scopes proceed, every waiter can leave, the last to leave +// cancels the opening, and an opening that comes back after everyone left changes nothing. + +// gatedUpstream blocks every Watch until the test releases it, announcing each call first. +type gatedUpstream struct { + calls chan *gatedOpen + // ignoreCancel makes Watch deaf to its context, as an upstream that does not honour cancellation + // is. Its result then arrives whenever the test releases it, however late. + ignoreCancel bool +} + +func newGatedUpstream() *gatedUpstream { return &gatedUpstream{calls: make(chan *gatedOpen, 64)} } + +type gatedOpen struct { + scope Scope + ctx context.Context + release chan gatedResult + returned chan struct{} +} + +type gatedResult struct { + w Watcher + err error +} + +func (u *gatedUpstream) Watch(ctx context.Context, scope Scope) (Watcher, error) { + o := &gatedOpen{scope: scope, ctx: ctx, release: make(chan gatedResult, 1), returned: make(chan struct{})} + defer close(o.returned) + u.calls <- o + cancelled := ctx.Done() + if u.ignoreCancel { + cancelled = nil + } + select { + case r := <-o.release: + return r.w, r.err + case <-cancelled: + return nil, ctx.Err() + } +} + +// opened waits for the next upstream Watch call. +func (u *gatedUpstream) opened(t *testing.T) *gatedOpen { + t.Helper() + select { + case o := <-u.calls: + return o + case <-time.After(2 * time.Second): + t.Fatal("the upstream was never asked to open") + return nil + } +} + +// quiet asserts that no further upstream Watch call has been made. +func (u *gatedUpstream) quiet(t *testing.T) { + t.Helper() + select { + case o := <-u.calls: + t.Fatalf("an unexpected upstream opening of %+v", o.scope) + default: + } +} + +// openWatcher is the watcher an opening returns: fed by the test, and recording that it was stopped. +type openWatcher struct { + events chan WatchEvent + stopped chan struct{} + once sync.Once +} + +func newOpenWatcher() *openWatcher { + return &openWatcher{events: make(chan WatchEvent, 16), stopped: make(chan struct{})} +} + +func (w *openWatcher) Next(ctx context.Context) (WatchEvent, error) { + select { + case ev := <-w.events: + return ev, nil + case <-ctx.Done(): + return WatchEvent{}, ctx.Err() + } +} + +func (w *openWatcher) Stop() { w.once.Do(func() { close(w.stopped) }) } + +func within(t *testing.T, ch <-chan struct{}, what string) { + t.Helper() + select { + case <-ch: + case <-time.After(2 * time.Second): + t.Fatal(what) + } +} + +// watchResult is one asynchronous SharedBackend.Watch call. +type watchResult struct { + w Watcher + err error +} + +func watchAsync(ctx context.Context, b *SharedBackend, scope Scope) <-chan watchResult { + out := make(chan watchResult, 1) + go func() { + w, err := b.Watch(ctx, scope) + out <- watchResult{w, err} + }() + return out +} + +func await(t *testing.T, ch <-chan watchResult) watchResult { + t.Helper() + select { + case r := <-ch: + return r + case <-time.After(2 * time.Second): + t.Fatal("Watch did not return") + return watchResult{} + } +} + +func pending(t *testing.T, ch <-chan watchResult) { + t.Helper() + select { + case r := <-ch: + t.Fatalf("Watch returned (%v, %v) while its opening was still in progress", r.w, r.err) + default: + } +} + +// waitForWaiters blocks until an opening of scope has n callers waiting on it. Polling a condition, +// not sleeping for an ordering: it returns the moment the condition holds. +func waitForWaiters(t *testing.T, b *SharedBackend, scope Scope, n int) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for { + b.mu.Lock() + got := 0 + if op := b.openings[scopeKey(scope)]; op != nil { + got = op.waiters + } + b.mu.Unlock() + if got == n { + return + } + if time.Now().After(deadline) { + t.Fatalf("waiters = %d, want %d", got, n) + } + time.Sleep(time.Millisecond) + } +} + +func (b *SharedBackend) state() (scopes, openings int) { + b.mu.Lock() + defer b.mu.Unlock() + return len(b.scopes), len(b.openings) +} + +var otherScopeUnderTest = Scope{Version: "v1", Resource: "configmaps", Namespace: "other"} + +// The field report's first failure: one API server slow to answer one opening held the backend-wide +// lock, and with it every other scope's stream. Now an opening holds up only its own scope. +func TestASlowOpeningDoesNotHoldUpAnotherScope(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + + ctxA, cancelA := context.WithCancel(t.Context()) + defer cancelA() + a := watchAsync(ctxA, b, sharedScopeUnderTest) + openA := up.opened(t) // scope A is now inside the upstream, and stays there + + bres := watchAsync(t.Context(), b, otherScopeUnderTest) + openB := up.opened(t) + if openB.scope != otherScopeUnderTest { + t.Fatalf("opened %+v, want scope B", openB.scope) + } + wb := newOpenWatcher() + openB.release <- gatedResult{w: wb} + got := await(t, bres) + if got.err != nil { + t.Fatalf("scope B: %v", got.err) + } + defer got.w.Stop() + wb.events <- WatchEvent{Type: WatchAdded, Object: obj("uid-b", "cm-b", "1")} + wb.events <- boundary + if uids := drainSnapshot(t, got.w); !uids["uid-b"] { + t.Fatalf("scope B snapshot = %v, want uid-b", uids) + } + + pending(t, a) + cancelA() + if r := await(t, a); !errors.Is(r.err, context.Canceled) { + t.Fatalf("scope A = %v, want its caller's cancellation", r.err) + } + within(t, openA.returned, "the abandoned opening was not cancelled") +} + +// Ten tabs arriving while a scope opens are still one upstream watch, and each is attached to it. +func TestConcurrentCallersForOneScopeShareOneOpening(t *testing.T) { + up := newGatedUpstream() + var mu sync.Mutex + var opened, closed int + b := NewSharedBackendWithOptions(up, SharedOptions{Observer: ObserverFunc(func(o Observation) { + mu.Lock() + defer mu.Unlock() + switch o.Kind { + case ObservationSharedSubscriptionOpened: + opened++ + case ObservationSharedSubscriptionClosed: + closed++ + } + })}) + + var results []<-chan watchResult + for range 10 { + results = append(results, watchAsync(t.Context(), b, sharedScopeUnderTest)) + } + open := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 10) + up.quiet(t) + + w := newOpenWatcher() + open.release <- gatedResult{w: w} + var watchers []Watcher + for _, r := range results { + got := await(t, r) + if got.err != nil { + t.Fatalf("Watch: %v", got.err) + } + watchers = append(watchers, got.w) + } + up.quiet(t) + + w.events <- WatchEvent{Type: WatchAdded, Object: obj("uid-a", "cm-a", "1")} + w.events <- boundary + for i, sw := range watchers { + if uids := drainSnapshot(t, sw); !uids["uid-a"] { + t.Errorf("subscriber %d snapshot = %v, want uid-a", i, uids) + } + } + for _, sw := range watchers { + sw.Stop() + } + within(t, w.stopped, "the last subscriber left and the upstream watch stayed open") + mu.Lock() + defer mu.Unlock() + if opened != 10 || closed != 10 { + t.Errorf("subscriptions opened %d, closed %d; want 10 and 10", opened, closed) + } +} + +// A browser that leaves while its scope is opening gets its request back at once; the others keep +// waiting on the same opening, which is not cancelled on their behalf. +func TestCancellingOneWaiterLeavesTheOthersWaiting(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + + leaving, leave := context.WithCancel(t.Context()) + defer leave() + first := watchAsync(leaving, b, sharedScopeUnderTest) + open := up.opened(t) + staying := watchAsync(t.Context(), b, sharedScopeUnderTest) + waitForWaiters(t, b, sharedScopeUnderTest, 2) + + leave() + if r := await(t, first); !errors.Is(r.err, context.Canceled) { + t.Fatalf("the departing waiter got %v, want its own cancellation", r.err) + } + waitForWaiters(t, b, sharedScopeUnderTest, 1) + if open.ctx.Err() != nil { + t.Fatal("one waiter leaving cancelled an opening another still needs") + } + pending(t, staying) + + w := newOpenWatcher() + open.release <- gatedResult{w: w} + got := await(t, staying) + if got.err != nil { + t.Fatalf("the remaining waiter: %v", got.err) + } + defer got.w.Stop() + w.events <- boundary + drainSnapshot(t, got.w) + up.quiet(t) +} + +// The last waiter out cancels the opening and unregisters it, without recording a failure: nobody +// waiting is not the upstream being away. The next caller opens afresh, at once. +func TestCancellingTheLastWaiterCancelsTheOpening(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + + ctx1, cancel1 := context.WithCancel(t.Context()) + ctx2, cancel2 := context.WithCancel(t.Context()) + defer cancel1() + defer cancel2() + r1 := watchAsync(ctx1, b, sharedScopeUnderTest) + open := up.opened(t) + r2 := watchAsync(ctx2, b, sharedScopeUnderTest) + waitForWaiters(t, b, sharedScopeUnderTest, 2) + + cancel1() + await(t, r1) + cancel2() + await(t, r2) + within(t, open.ctx.Done(), "the last waiter left and the upstream opening's context was not cancelled") + within(t, open.returned, "the cancelled opening never returned") + if scopes, openings := b.state(); scopes != 0 || openings != 0 { + t.Fatalf("scopes=%d openings=%d after every waiter left, want none", scopes, openings) + } + b.mu.Lock() + _, backingOff := b.backoff[scopeKey(sharedScopeUnderTest)] + b.mu.Unlock() + if backingOff { + t.Fatal("an opening abandoned by its waiters was recorded as an upstream failure") + } + + retry := watchAsync(t.Context(), b, sharedScopeUnderTest) + again := up.opened(t) + w := newOpenWatcher() + again.release <- gatedResult{w: w} + got := await(t, retry) + if got.err != nil { + t.Fatalf("retry: %v", got.err) + } + got.w.Stop() +} + +// A caller that is already gone starts nothing, joins nothing, and holds nothing open. +func TestACancelledCallerDoesNotOpenOrJoin(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + gone, cancel := context.WithCancel(t.Context()) + cancel() + + if _, err := b.Watch(gone, sharedScopeUnderTest); !errors.Is(err, context.Canceled) { + t.Fatalf("Watch = %v, want the caller's cancellation", err) + } + up.quiet(t) + if scopes, openings := b.state(); scopes != 0 || openings != 0 { + t.Fatalf("scopes=%d openings=%d, want none", scopes, openings) + } + + // Nor does it join an opening already under way, and so cannot keep it alive. + ctx, leave := context.WithCancel(t.Context()) + r := watchAsync(ctx, b, sharedScopeUnderTest) + open := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 1) + if _, err := b.Watch(gone, sharedScopeUnderTest); !errors.Is(err, context.Canceled) { + t.Fatalf("Watch = %v, want the caller's cancellation", err) + } + waitForWaiters(t, b, sharedScopeUnderTest, 1) + leave() + await(t, r) + within(t, open.ctx.Done(), "the opening outlived its only real waiter") +} + +// A waiter's cancellation and the opening's success can land together, and either may be noticed +// first. Whichever is, nothing is left behind: no subscription, no scope, no upstream watch, and every +// attachment observed as opened is observed as closed. +func TestCancellationRacingASuccessfulOpeningOrphansNothing(t *testing.T) { + for i := range 200 { + up := newGatedUpstream() + up.ignoreCancel = true // so the released watcher always reaches the backend, which must dispose of it + var mu sync.Mutex + balance := 0 + b := NewSharedBackendWithOptions(up, SharedOptions{Observer: ObserverFunc(func(o Observation) { + mu.Lock() + defer mu.Unlock() + switch o.Kind { + case ObservationSharedSubscriptionOpened: + balance++ + case ObservationSharedSubscriptionClosed: + balance-- + } + })}) + + ctx, cancel := context.WithCancel(t.Context()) + r := watchAsync(ctx, b, sharedScopeUnderTest) + open := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 1) + + w := newOpenWatcher() + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Go(func() { <-start; open.release <- gatedResult{w: w} }) + wg.Go(func() { <-start; cancel() }) + close(start) + wg.Wait() + + if got := await(t, r); got.err == nil { + got.w.Stop() // the opening won: the caller owns a subscriber, and releases it as usual + } else if !errors.Is(got.err, context.Canceled) { + t.Fatalf("iteration %d: Watch = %v", i, got.err) + } + within(t, w.stopped, "the upstream watch was orphaned") + if scopes, openings := b.state(); scopes != 0 || openings != 0 { + t.Fatalf("iteration %d: scopes=%d openings=%d left behind", i, scopes, openings) + } + mu.Lock() + if balance != 0 { + t.Fatalf("iteration %d: %d subscriptions opened and never closed", i, balance) + } + mu.Unlock() + } +} + +// An upstream that ignores cancellation can return long after its waiters left, by which time a new +// opening may own the scope. Its result is discarded: a watcher is stopped, a failure records no +// backoff, and the newer opening is neither removed, nor populated, nor slowed. +func TestAnAbandonedOpeningReturningLateCannotTouchItsReplacement(t *testing.T) { + for name, late := range map[string]func() gatedResult{ + "with a watcher": func() gatedResult { return gatedResult{w: newOpenWatcher()} }, + "with a failure": func() gatedResult { + return gatedResult{err: UpstreamUnavailable("the API server is unavailable", time.Minute)} + }, + "with a cancellation": func() gatedResult { return gatedResult{err: context.Canceled} }, + } { + t.Run(name, func(t *testing.T) { + up := newGatedUpstream() + up.ignoreCancel = true + b := NewSharedBackend(up) + + ctx, leave := context.WithCancel(t.Context()) + abandoned := watchAsync(ctx, b, sharedScopeUnderTest) + old := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 1) + leave() + await(t, abandoned) + + replacement := watchAsync(t.Context(), b, sharedScopeUnderTest) + current := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 1) + + result := late() + old.release <- result + within(t, old.returned, "the old opening did not return") + if w, ok := result.w.(*openWatcher); ok { + within(t, w.stopped, "the late watcher was kept rather than stopped") + } + // The replacement is still registered, still waited for, and nothing is backing off. + waitForWaiters(t, b, sharedScopeUnderTest, 1) + b.mu.Lock() + scopes, backoff := len(b.scopes), len(b.backoff) + b.mu.Unlock() + if scopes != 0 || backoff != 0 { + t.Fatalf("the late result changed state: scopes=%d backoff=%d", scopes, backoff) + } + pending(t, replacement) + + w := newOpenWatcher() + current.release <- gatedResult{w: w} + got := await(t, replacement) + if got.err != nil { + t.Fatalf("replacement: %v", got.err) + } + defer got.w.Stop() + w.events <- WatchEvent{Type: WatchAdded, Object: obj("uid-new", "cm", "2")} + w.events <- boundary + if uids := drainSnapshot(t, got.w); len(uids) != 1 || !uids["uid-new"] { + t.Fatalf("snapshot = %v, want only the replacement's uid-new", uids) + } + }) + } +} + +// A genuine failure to open is classified once — one backoff, however many were waiting — and every +// waiter receives it as it is. Terminal refusals reach them the same way, and do not back off. +func TestAnOpeningFailureReachesEveryWaiterAndBacksOffOnce(t *testing.T) { + for name, tc := range map[string]struct { + err *StreamError + backsOff bool + wantDelay time.Duration + }{ + "retryable with a hint": {UpstreamUnavailable("the API server is unavailable", 5*time.Second), true, 5 * time.Second}, + "retryable": {UpstreamUnavailable("the API server is unavailable", 0), true, sharedRetryMin}, + "terminal": {Forbidden("the gateway's identity may not watch this scope"), false, 0}, + } { + t.Run(name, func(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + now := time.Unix(0, 0) + b.now = func() time.Time { return now } // read only under b.mu, after the waiters arrived + + var results []<-chan watchResult + for range 3 { + results = append(results, watchAsync(t.Context(), b, sharedScopeUnderTest)) + } + open := up.opened(t) + waitForWaiters(t, b, sharedScopeUnderTest, 3) + open.release <- gatedResult{err: tc.err} + for i, r := range results { + var se *StreamError + if got := await(t, r); !errors.As(got.err, &se) || se != tc.err { + t.Fatalf("waiter %d got %v, want the opening's own %v", i, got.err, tc.err) + } + } + if !errors.Is(open.ctx.Err(), context.Canceled) { + t.Error("a failed opening's context was left uncancelled") + } + + b.mu.Lock() + bo := b.backoff[scopeKey(sharedScopeUnderTest)] + b.mu.Unlock() + if !tc.backsOff { + if bo != nil { + t.Fatalf("a terminal refusal backed off: %+v", bo) + } + return + } + if bo == nil || bo.delay != sharedRetryMin || bo.until.Sub(now) != tc.wantDelay { + t.Fatalf("backoff = %+v, want one failure recorded, waiting %v", bo, tc.wantDelay) + } + _, err := b.Watch(t.Context(), sharedScopeUnderTest) + var se *StreamError + if !errors.As(err, &se) || se.Code != CodeUpstreamUnavailable || se.RetryAfterMs == nil { + t.Fatalf("Watch during the backoff = %v, want UPSTREAM_UNAVAILABLE with a hint", err) + } + up.quiet(t) + }) + } +} + +// Seen from the stream loop: a stream whose scope is slow to open ends when its caller leaves, does +// not hold up a stream of another scope, and does not leave the opening behind. +func TestAStreamWaitingOnASlowOpeningEndsWithItsCaller(t *testing.T) { + up := newGatedUpstream() + b := NewSharedBackend(up) + var mu sync.Mutex + var kinds []ObservationKind + g := &Gateway{ + Auth: AllowAll{}, + Clients: func(context.Context, string, Principal) (Backend, error) { return b, nil }, + Observer: ObserverFunc(func(o Observation) { + mu.Lock() + defer mu.Unlock() + if o.Scope == sharedScopeUnderTest { + kinds = append(kinds, o.Kind) + } + }), + } + + slowCtx, leave := context.WithCancel(t.Context()) + defer leave() + slowDone := make(chan error, 1) + go func() { slowDone <- g.Stream(slowCtx, nil, sharedScopeUnderTest, &lockedSink{}) }() + slowOpen := up.opened(t) + + fast := make(reauthEvents, 8) + fastCtx, stopFast := context.WithCancel(t.Context()) + defer stopFast() + fastDone := make(chan error, 1) + go func() { fastDone <- g.Stream(fastCtx, nil, otherScopeUnderTest, fast) }() + fastOpen := up.opened(t) + w := newOpenWatcher() + fastOpen.release <- gatedResult{w: w} + w.events <- boundary + awaitEvent(t, fast, EventSynced) + + leave() + select { + case err := <-slowDone: + if !errors.Is(err, context.Canceled) { + t.Fatalf("slow stream = %v, want its caller's cancellation", err) + } + case <-time.After(2 * time.Second): + t.Fatal("a stream waiting on a slow opening outlived its caller") + } + within(t, slowOpen.returned, "the slow opening was not cancelled") + stopFast() + <-fastDone + within(t, w.stopped, "the fast stream's upstream outlived it") + mu.Lock() + defer mu.Unlock() + for _, k := range kinds { + if k == ObservationRetryableError || k == ObservationTerminalError { + t.Errorf("a caller leaving was reported as %s", k) + } + } +} From 8b8a6504e9397fe1f6d92e62b53c6e9c4ed3222d Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 12:28:34 +0000 Subject: [PATCH 2/9] feat(gateway)!: require a write timeout with timed reauthorization over HTTP A timed recheck shares its subscriber's delivery gate, so it waits for the write in progress. A write to a browser that stopped reading blocks once the buffers fill, and with the default zero WriteTimeout it never ends: the recheck, and the revocation it would apply, never ran. A host that set ReauthorizationInterval alone got an interval that did not hold. Handler now panics at construction when ReauthorizationInterval is positive and WriteTimeout is zero, and ServeStream and ServeStreamProjection apply the same shared validation before any response I/O, observation, authorization or backend call. A zero write timeout remains valid without timed checks, and the transport-neutral Stream and StreamProjection keep timed checks with host-bounded sinks. The real HTTP/1.1 and TLS HTTP/2 blocked-reader test now asserts a declared test budget and that the stalled request releases its shared subscription while the healthy one continues. docs/auth.md documents the whole revocation budget in one place, and the other guides link to it. BREAKING CHANGE: HTTP hosts using timed reauthorization (Options.ReauthorizationInterval or Gateway.ReauthorizationInterval with ServeStream/ServeStreamProjection) must set a positive per-operation WriteTimeout, or Handler and the direct serving calls panic. Choose a budget per write-plus-flush, count it in the revocation budget, and verify that the mounted middleware supports flushing and write deadlines (gateway.CheckHTTPStreaming in a host test). Co-Authored-By: Claude Opus 5.5 --- docs/adopting.md | 13 ++ docs/auth.md | 68 +++++-- docs/operations.md | 9 +- gateway/README.md | 11 +- gateway/handler.go | 11 +- gateway/http_config_test.go | 197 +++++++++++++++++++ gateway/kube/examples/sharedstream/README.md | 4 +- gateway/sse.go | 26 ++- gateway/sse_test.go | 28 ++- gateway/stream.go | 8 +- 10 files changed, 347 insertions(+), 28 deletions(-) create mode 100644 gateway/http_config_test.go diff --git a/docs/adopting.md b/docs/adopting.md index 14c95de..3c7df45 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -188,6 +188,19 @@ options.Authorizer = kube.SubjectAccessReviewAuthorizer(clientset, subjectFromUs options.Clients = func(context.Context, string, gateway.Principal) (gateway.Backend, error) { return shared, nil } ``` +Rechecking on a timer bounds how long a revoked caller keeps a quiet stream. Over HTTP it needs a +write bound too, because a check waits for the write in progress; `Handler` refuses the interval +without one: + +```go +options.ReauthorizationInterval = 30 * time.Second +options.ReauthorizationTimeout = 5 * time.Second +options.WriteTimeout = 10 * time.Second +``` + +Test that your mounted middleware supports flushing and write deadlines, and plan with the +[revocation budget](auth.md#revocation-budget), which also lists what these settings do not bound. + Read [auth.md](auth.md) before using this configuration. The shared-cache authorizer is a security boundary; a per-user backend keeps Kubernetes RBAC as the direct boundary by construction. diff --git a/docs/auth.md b/docs/auth.md index c4e5297..985f796 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -76,19 +76,30 @@ verifying that caller may read the resource. The gateway rechecks authorization and projection policy on every snapshot cycle and calls `Clients` again so the host can provide refreshing credentials. Cycle-only checks do not bound revocation time -on a quiet stream. Set a timed recheck when the host needs that bound: +on a quiet stream. Set a timed recheck when the host needs that bound, together with a write bound: ```go -options.ReauthorizationInterval = 30 * time.Second -options.ReauthorizationTimeout = 5 * time.Second +options.ReauthorizationInterval = 30 * time.Second // how often each subscriber is rechecked +options.ReauthorizationTimeout = 5 * time.Second // budget for each periodic check's callbacks +options.WriteTimeout = 10 * time.Second // required over HTTP with timed checks ``` -Timed checks run per subscriber and pause that subscriber's object delivery. Denial, timeout, policy -failure or a changed projection terminates only that stream; other subscribers continue. A -SubjectAccessReview that cannot reach the API server ends the stream with a non-terminal -`UPSTREAM_UNAVAILABLE`, so the client may reconnect once it is back. Zero -interval keeps cycle-only checks; zero timeout uses 10 seconds. The bound assumes callbacks honor -context cancellation and sinks do not block indefinitely. +Timed checks run per subscriber and share that subscriber's delivery gate: while a check runs, the +subscriber receives no objects, and a check waits for the delivery already in progress. Denial, +timeout, policy failure or a changed projection terminates only that stream; other subscribers +continue. A SubjectAccessReview that cannot reach the API server ends the stream with a non-terminal +`UPSTREAM_UNAVAILABLE`, so the client may reconnect once it is back. Zero interval keeps cycle-only +checks; zero timeout uses 10 seconds. + +Library-owned HTTP serving requires a positive `WriteTimeout` with a positive +`ReauthorizationInterval`. `Handler` panics at construction without one, and direct `ServeStream` and +`ServeStreamProjection` calls panic before writing anything. A write to a browser that has stopped +reading blocks once the buffers between fill, and without a deadline it would hold the gate, and the +revocation behind it, until something else ended the request. The mounted middleware must support +flushing and write deadlines; test it with the +[capability-check recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test). +Transport-neutral `Stream` and `StreamProjection` keep timed checks with any sink: there the host +owns the sink and must bound its I/O. Checks use the principal captured at stream open. Resolve current session/account validity inside the host authorizer. Timed checks do not invoke `Clients`; credential refresh remains per snapshot cycle @@ -98,6 +109,39 @@ With 200 subscribers, a 30-second interval adds roughly 13 SubjectAccessReviews watch per subscriber), plus opening/cycle checks. Choose intervals for the host's revocation budget and API-server capacity; checks are not cached across identities. +### Revocation budget + +The revocation budget is how long a stream keeps delivering after access is withdrawn. With timed +checks over HTTP it is made of these parts, in order: + +| Part | Bounded by | What it covers | +|---|---|---| +| Decision freshness | the host | Time until the host's authorizer can see the change: session stores, identity-provider group sync, and any decision cache the host adds. The rest of the budget starts only then. | +| Timer | `ReauthorizationInterval` | Up to one interval until that subscriber's next check. Each subscriber has its own timer. | +| Gate wait | one `WriteTimeout` | The check waits for the delivery in progress, which completes or fails at its write deadline. | +| Check | `ReauthorizationTimeout` | Starts once the check holds the gate, and covers the `Authorizer` and projection-policy callbacks. | +| Termination and cleanup | one `WriteTimeout`, then the host | The refusal is written as the stream's terminal frame, a write with its own deadline. The request then returns, releasing its shared subscription. | + +Interval + check timeout + write timeout is a planning figure, not an unconditional bound. Scheduling +delay, a heartbeat or terminal write competing for the same response, callbacks that ignore +cancellation, host middleware and cleanup can all add time. Declare the total for your deployment, +then measure it under the intended load. + +Revocation ends three things at different times: + +- **New object delivery** stops when the check holds the gate. A denied check releases the gate + only after ending the stream, so no further object is written. +- **The request** returns after the terminal frame is written or fails, and cleanup completes. +- **The browser** receives the refusal only if the transport delivers it. A reader that stopped + reading may never see it, and the request ends at the write deadline instead. + +Bytes already written to the socket, or buffered by a proxy, cannot be recalled. Revocation stops +further disclosure; it does not retract earlier disclosure. + +`ReauthorizationTimeout` covers periodic checks only. Opening and snapshot-cycle authorization run +under the request's context, so give host callbacks their own deadlines. A generic `Stream` sink +takes the place of `WriteTimeout` in the table, with whatever bound the host gives it. + ## Shared-watch authorization [`SharedBackend`](../gateway/shared.go) opens one upstream watch per scope as one service identity. @@ -134,10 +178,8 @@ SelfSubjectReview helper using participant credentials, service-account SARs and fixed scope, session/token expiry and bounded HTTP delivery. Identity resolution is an example, not a public library authentication API. It does not re-resolve identity on every timed check. -`ReauthorizationTimeout` applies to periodic callbacks after acquiring the delivery gate. Opening -and cycle authorization need their own host callback deadlines. Do not put a short callback deadline -on the entire healthy stream. Write bounds limit in-flight HTTP I/O, not all gate waiting, backend -operations or callback work; declare and measure the total revocation budget under the intended load. +Do not put a short callback deadline on the entire healthy stream. Write bounds limit in-flight +HTTP I/O, not backend operations or callback work; see the [revocation budget](#revocation-budget). For 200 allowed participants, opening can issue 400 SARs plus 200 SSRs. Timers can align, recovery adds checks, and client-side throttling consumes callback budgets. Neither the example's rate settings nor Voter's reported rehearsal results are production defaults or supported-version evidence. diff --git a/docs/operations.md b/docs/operations.md index de46bc9..56b7457 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -57,7 +57,9 @@ a reconnect loop. It shows as `retryable_error`. | Control | Default | Use | |---|---:|---| -| `gateway.Options.WriteTimeout` | 0 (no deadline) | set a positive per-operation write-plus-flush budget for bounded HTTP delivery | +| `gateway.Options.WriteTimeout` | 0 (no deadline) | set a positive per-operation write-plus-flush budget for bounded HTTP delivery; required with `ReauthorizationInterval` | +| `gateway.Options.ReauthorizationInterval` | 0 (cycle checks only) | recheck each subscriber on quiet streams; see the [revocation budget](auth.md#revocation-budget) | +| `gateway.Options.ReauthorizationTimeout` | 10 seconds | bound each periodic check's callbacks once it holds the delivery gate | | `gateway.Options.HeartbeatInterval` | 20 seconds | set below the shortest proxy idle timeout | | `gateway.SharedOptions.QueueDepth` | 256 live events | tune after measuring; it bounds memory per slow subscriber | | `ScopePolicy.AllowLabelSelector` | false | enable only for an endpoint that deliberately supports caller narrowing | @@ -91,7 +93,10 @@ metrics available on the tested Kubernetes version; the shared-host fixture uses ## Bounded HTTP delivery Set `WriteTimeout` explicitly; zero preserves no library-installed deadline and negative values -are rejected. Each header, event or heartbeat write and flush shares one deadline. Successful +are rejected. A positive `ReauthorizationInterval` requires a positive `WriteTimeout`: `Handler` +panics at construction without one, and direct `ServeStream` calls panic before writing. A timed +check waits for the write in progress, so the write bound is part of the +[revocation budget](auth.md#revocation-budget). Each header, event or heartbeat write and flush shares one deadline. Successful operations clear it, so quiet streams can outlive many timeout periods. A failed operation cancels its stream and cannot be retried as a terminal frame. Flush success does not acknowledge browser receipt. Generic `Stream` sinks remain responsible for bounded I/O. diff --git a/gateway/README.md b/gateway/README.md index c7c1bd4..f6333e7 100644 --- a/gateway/README.md +++ b/gateway/README.md @@ -114,10 +114,13 @@ Observers run on stream paths, so they must return promptly. See [`docs/operations.md`](../docs/operations.md) for suggested metrics and alerts, and [`docs/adopting.md`](../docs/adopting.md) for full host wiring examples. -For shared streams, set `Options.ReauthorizationInterval` (for example, 30 seconds) and -`Options.ReauthorizationTimeout` (for example, 5 seconds). Each subscriber is rechecked independently, -including during quiet periods; denial or timeout stops only that subscriber. Zero interval keeps -cycle-only checks. See [authorization](../docs/auth.md) for callback contracts and capacity planning. +For shared streams, set `Options.ReauthorizationInterval` (for example, 30 seconds), +`Options.ReauthorizationTimeout` (for example, 5 seconds) and a positive `Options.WriteTimeout`, which +HTTP serving requires with timed checks. Each subscriber is rechecked independently, including during +quiet periods; denial or timeout stops only that subscriber. Zero interval keeps cycle-only checks. +Transport-neutral `Stream` keeps timed checks with any sink the host bounds itself. See +[authorization](../docs/auth.md) for callback contracts, capacity planning and the +[revocation budget](../docs/auth.md#revocation-budget). Use `kube.SubjectAccessReviewAuthorizer` for Kubernetes-backed subscriber authorization. `Options.WriteTimeout` bounds each HTTP header/frame/comment write plus flush. Zero installs no diff --git a/gateway/handler.go b/gateway/handler.go index cec7e8a..19366c7 100644 --- a/gateway/handler.go +++ b/gateway/handler.go @@ -82,12 +82,18 @@ type Options struct { // WriteTimeout bounds each HTTP write-plus-flush operation. Zero installs no deadline. // Positive values require a writer supporting flush and write deadlines; negative values panic. + // Required (positive) whenever ReauthorizationInterval is: see below. WriteTimeout time.Duration // ReauthorizationInterval rechecks each subscriber independently, even on quiet streams. // Zero disables timed checks; snapshot cycles always reauthorize. + // + // A positive interval requires a positive WriteTimeout, and Handler panics without one. A timed + // check waits for that subscriber's write in progress, and a write to a browser that stopped + // reading only ends at its deadline. The revocation budget is documented in docs/auth.md. ReauthorizationInterval time.Duration - // ReauthorizationTimeout bounds a timed check. Zero defaults to 10 seconds. + // ReauthorizationTimeout bounds each timed check's Authorizer and projection-policy callbacks, + // starting once the check holds the subscriber's delivery gate. Zero defaults to 10 seconds. // Authorizers and projection policies must honor context cancellation. ReauthorizationTimeout time.Duration } @@ -102,9 +108,8 @@ type Options struct { // what stops EventSource from reconnecting forever. Unsupported bounded transports instead // abort before streaming and report ObservationHTTPTransportRejected. func Handler(o Options) http.Handler { + validateHTTPServing(o.WriteTimeout, o.ReauthorizationInterval) switch { - case o.WriteTimeout < 0: - panic("krm-stream: WriteTimeout must not be negative") case o.Principal == nil: panic("krm-stream: Options.Principal is required — the library must never assume who the caller is") case o.Authorizer == nil: diff --git a/gateway/http_config_test.go b/gateway/http_config_test.go new file mode 100644 index 0000000..8637087 --- /dev/null +++ b/gateway/http_config_test.go @@ -0,0 +1,197 @@ +package gateway + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +// Timed reauthorization over library-owned HTTP needs a write bound. A timed check waits for the +// subscriber's write in progress, and a write to a reader that stopped reading never ends without a +// deadline — so a host that set an interval alone had an interval that did not hold. That combination +// is now refused as configuration, at construction and before any direct serving touches a response. + +// spyWriter records every response operation, so a test can prove none happened. +type spyWriter struct { + header http.Header + ops []string +} + +func (w *spyWriter) Header() http.Header { + w.ops = append(w.ops, "Header") + return w.header +} +func (w *spyWriter) WriteHeader(int) { w.ops = append(w.ops, "WriteHeader") } +func (w *spyWriter) Write(p []byte) (int, error) { + w.ops = append(w.ops, "Write") + return len(p), nil +} +func (w *spyWriter) FlushError() error { w.ops = append(w.ops, "Flush"); return nil } +func (w *spyWriter) SetWriteDeadline(time.Time) error { + w.ops = append(w.ops, "SetWriteDeadline") + return nil +} + +func configPanic(t *testing.T, f func()) (msg string) { + t.Helper() + defer func() { + r := recover() + if r == nil { + t.Fatal("the configuration was accepted") + } + s, ok := r.(string) + if !ok { + t.Fatalf("panic = %v (%T), want a configuration message", r, r) + } + msg = s + }() + f() + return "" +} + +func validOptions() Options { + return Options{ + Principal: func(*http.Request) (Principal, error) { return nil, nil }, + Authorizer: AllowAll{}, + Clients: func(context.Context, string, Principal) (Backend, error) { return nil, nil }, + Scopes: ScopePolicy{Targets: []string{""}, AnyResource: true}, + } +} + +func TestHandlerRefusesTimedReauthorizationWithoutAWriteTimeout(t *testing.T) { + o := validOptions() + o.ReauthorizationInterval = 30 * time.Second + msg := configPanic(t, func() { Handler(o) }) + if !strings.Contains(msg, "ReauthorizationInterval requires a positive WriteTimeout") { + t.Errorf("panic = %q, want it to name both options", msg) + } + + // The negative-timeout refusal is unchanged, and wins: it is wrong on its own. + o.WriteTimeout = -time.Second + if msg := configPanic(t, func() { Handler(o) }); !strings.Contains(msg, "must not be negative") { + t.Errorf("panic = %q, want the negative-timeout refusal", msg) + } +} + +func TestHandlerAcceptsBoundedTimedReauthorizationAndUntimedStreams(t *testing.T) { + for name, o := range map[string]func(*Options){ + "timed checks with a write timeout": func(o *Options) { o.ReauthorizationInterval, o.WriteTimeout = 30*time.Second, 5*time.Second }, + "no timed checks, no write timeout": func(*Options) {}, + "no timed checks, with write timeout": func(o *Options) { o.WriteTimeout = 5 * time.Second }, + } { + t.Run(name, func(t *testing.T) { + opts := validOptions() + o(&opts) + if Handler(opts) == nil { + t.Fatal("no handler") + } + }) + } +} + +// A host serving directly meets the same rule, and meets it before anything happens: no header, no +// write, no deadline, no stream-opening observation, no authorization, no backend. +func TestDirectHTTPServingRefusesTimedReauthorizationBeforeAnyIO(t *testing.T) { + for name, serve := range map[string]func(*Gateway, http.ResponseWriter, *http.Request){ + "ServeStream": func(g *Gateway, w http.ResponseWriter, r *http.Request) { + g.ServeStream(w, r, "alice", sharedScopeUnderTest) + }, + "ServeStreamProjection": func(g *Gateway, w http.ResponseWriter, r *http.Request) { + g.ServeStreamProjection(w, r, "alice", sharedScopeUnderTest, ProjectionSpec) + }, + } { + t.Run(name, func(t *testing.T) { + var observed, calls atomic.Int32 + g := &Gateway{ + Auth: AuthorizerFunc(func(context.Context, Principal, Scope) error { + calls.Add(1) + return nil + }), + Clients: func(context.Context, string, Principal) (Backend, error) { + calls.Add(1) + return newFakeUpstream(), nil + }, + Observer: ObserverFunc(func(Observation) { observed.Add(1) }), + ReauthorizationInterval: 30 * time.Second, + } + w := &spyWriter{header: http.Header{}} + msg := configPanic(t, func() { serve(g, w, httptest.NewRequest(http.MethodGet, "/", nil)) }) + if !strings.Contains(msg, "ReauthorizationInterval requires a positive WriteTimeout") { + t.Errorf("panic = %q", msg) + } + if len(w.ops) != 0 { + t.Errorf("the response was touched before the refusal: %v", w.ops) + } + if n := observed.Load(); n != 0 { + t.Errorf("%d observations before the refusal, want none", n) + } + if n := calls.Load(); n != 0 { + t.Errorf("%d host callbacks before the refusal, want none", n) + } + }) + } +} + +// The same direct paths still serve a stream once the write is bounded, and with timed checks off a +// zero write timeout remains valid. +func TestDirectHTTPServingAcceptsValidTimedConfigurations(t *testing.T) { + for name, tc := range map[string]struct{ interval, write time.Duration }{ + "timed checks with a write timeout": {time.Hour, time.Second}, + "no timed checks, no write timeout": {0, 0}, + } { + t.Run(name, func(t *testing.T) { + g := &Gateway{ + Auth: AuthorizerFunc(func(context.Context, Principal, Scope) error { return Forbidden("not today") }), + Clients: func(context.Context, string, Principal) (Backend, error) { return newFakeUpstream(), nil }, + WriteTimeout: tc.write, + ReauthorizationInterval: tc.interval, + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + g.ServeStream(w, r, "alice", sharedScopeUnderTest) + })) + defer srv.Close() + res, err := srv.Client().Get(srv.URL) + if err != nil { + t.Fatal(err) + } + defer func() { _ = res.Body.Close() }() + body, err := io.ReadAll(res.Body) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(body), `"code":"FORBIDDEN"`) { + t.Errorf("the stream was not served:\n%s", body) + } + }) + } +} + +// Transport-neutral streams keep timed checks without any HTTP option: their sink is the host's to +// bound. A revocation still ends the stream through it. +func TestTransportNeutralStreamsKeepTimedChecksWithoutAWriteTimeout(t *testing.T) { + var revoked atomic.Bool + g := &Gateway{ + Auth: AuthorizerFunc(func(context.Context, Principal, Scope) error { + if revoked.Load() { + return Forbidden("revoked") + } + return nil + }), + Clients: func(context.Context, string, Principal) (Backend, error) { return newFakeUpstream(), nil }, + ReauthorizationInterval: time.Millisecond, + } + events := make(reauthEvents, 32) + done := make(chan error, 1) + go func() { done <- g.StreamProjection(t.Context(), "alice", sharedScopeUnderTest, "", events) }() + awaitEvent(t, events, EventReset) + revoked.Store(true) + if ev := awaitEvent(t, events, EventError); ev.Code != CodeForbidden || !ev.Terminal { + t.Fatalf("revocation = %+v, want a terminal FORBIDDEN", ev) + } + <-done +} diff --git a/gateway/kube/examples/sharedstream/README.md b/gateway/kube/examples/sharedstream/README.md index ce5b439..97fb036 100644 --- a/gateway/kube/examples/sharedstream/README.md +++ b/gateway/kube/examples/sharedstream/README.md @@ -103,7 +103,9 @@ contention to the API server. Record real runs in `docs/facts/` with commit, versions, protocols/proxy, workload, limits, commands, results and unrun cases. The 30-second recheck, five-second check and 60-second closure target is not a library guarantee. Gate waiting, in-flight I/O, callback execution and cleanup contribute to -revocation time. A write timeout does not bound backend opening or uncooperative callbacks. +revocation time; see the [revocation budget](../../../../docs/auth.md#revocation-budget). The +five-second `WriteTimeout` is required by the timed checks, and does not bound backend opening or +uncooperative callbacks. A shared watch still transfers a snapshot to each client and consumes per-client memory, connection and browser work. Measure sustained load, peak memory/CPU, snapshot bytes and latency distributions diff --git a/gateway/sse.go b/gateway/sse.go index 38197f5..b260107 100644 --- a/gateway/sse.go +++ b/gateway/sse.go @@ -181,7 +181,9 @@ func (g *Gateway) ServeStream(w http.ResponseWriter, r *http.Request, principal } // ServeStreamProjection serves a requested projection under the gateway's policy. -// With WriteTimeout enabled, unsupported writers abort before a stream opens. +// With WriteTimeout enabled, unsupported writers abort before a stream opens. A positive +// ReauthorizationInterval requires a positive WriteTimeout; without one it panics before +// writing anything, as Handler does at construction. func (g *Gateway) ServeStreamProjection(w http.ResponseWriter, r *http.Request, principal Principal, scope Scope, projection Projection) { g.serveHTTP(w, r, func(ctx context.Context, sink *SSESink) { // A terminal frame may itself fail; delivery is not guaranteed on a failed transport. @@ -189,10 +191,28 @@ func (g *Gateway) ServeStreamProjection(w http.ResponseWriter, r *http.Request, }) } -func (g *Gateway) serveHTTP(w http.ResponseWriter, r *http.Request, run func(context.Context, *SSESink)) { - if g.WriteTimeout < 0 { +// validateHTTPServing refuses, with a configuration panic, the settings HTTP serving cannot honour. +// Handler calls it once at construction; serveHTTP calls it again before any response I/O, so a host +// calling ServeStream directly meets the same rule rather than a weaker one. +// +// Timed reauthorization needs a write bound because a timed check and object delivery share one gate +// per subscriber (see authorizationSink): the check waits for the write in progress. A browser that +// stops reading blocks that write once the buffers between fill, and with no deadline it blocks until +// something else ends the request — so the check, and the revocation it would apply, never runs. +// Transport-neutral Stream sinks are the host's to bound, and are not checked here. +func validateHTTPServing(writeTimeout, reauthorizationInterval time.Duration) { + switch { + case writeTimeout < 0: panic("krm-stream: WriteTimeout must not be negative") + case reauthorizationInterval > 0 && writeTimeout == 0: + panic("krm-stream: ReauthorizationInterval requires a positive WriteTimeout for HTTP streams — " + + "a timed check waits for the write in progress, and a write to a reader that stopped reading " + + "never ends without one") } +} + +func (g *Gateway) serveHTTP(w http.ResponseWriter, r *http.Request, run func(context.Context, *SSESink)) { + validateHTTPServing(g.WriteTimeout, g.ReauthorizationInterval) if g.WriteTimeout > 0 { if err := CheckHTTPStreaming(w); err != nil { g.observe(Observation{Kind: ObservationHTTPTransportRejected, Code: CodeInternal}) diff --git a/gateway/sse_test.go b/gateway/sse_test.go index e406b72..5d7785a 100644 --- a/gateway/sse_test.go +++ b/gateway/sse_test.go @@ -175,6 +175,13 @@ func TestSinkShortWriteAndFailedTerminalDelivery(t *testing.T) { // Exhaust real socket / HTTP2 stream flow-control buffers. The healthy HTTP2 // subscriber shares the same client transport and connection with the stalled one. +// +// The revocation case is the field report's blocked reader: a timed check cannot run while the +// stalled write holds the delivery gate, so the write timeout — now required alongside timed checks +// — is what ends it. Each case must end the stalled request within testBudget and release its shared +// subscription, while the healthy subscriber keeps receiving. Assumptions: the authorizer answers at +// once, and scheduling under the race detector stays within the allowance. testBudget is a test +// tolerance, not a published production guarantee (see docs/auth.md, Revocation budget). func TestBlockedHTTPSubscriberReleasesSharedWatch(t *testing.T) { for _, h2 := range []bool{false, true} { for _, trigger := range []string{"write-timeout", "expiry", "revocation"} { @@ -317,7 +324,26 @@ func TestBlockedHTTPSubscriberReleasesSharedWatch(t *testing.T) { revoked.Store(true) } waitTransport(t, slowDone) - t.Logf("%s to slow handler return: %s (write budget %s; 8s test tolerance)", trigger, time.Since(start), g.WriteTimeout) + elapsed := time.Since(start) + // Timer, then a gate wait of at most one write timeout, then a check of at most the + // check timeout, plus a scheduling allowance for a loaded CI runner. + testBudget := g.ReauthorizationInterval + g.WriteTimeout + g.ReauthorizationTimeout + 2*time.Second + t.Logf("%s to slow handler return: %s (write timeout %s; test budget %s)", trigger, elapsed, g.WriteTimeout, testBudget) + if elapsed > testBudget { + t.Fatalf("the stalled request took %s to end, over the %s test budget", elapsed, testBudget) + } + // Its subscription is released by the time its handler returns; the healthy one's is not. + shared.mu.Lock() + remaining := -1 + if ss := shared.scopes[scopeKey(sharedScopeUnderTest)]; ss != nil { + ss.mu.Lock() + remaining = len(ss.subs) + ss.mu.Unlock() + } + shared.mu.Unlock() + if remaining != 1 { + t.Fatalf("shared subscriptions after the stalled request ended = %d, want only the healthy one", remaining) + } b.events <- WatchEvent{Type: WatchAdded, Object: obj("later", "still-live", "2")} waitFast("still-live") if h2 && conns.Load() != 1 { diff --git a/gateway/stream.go b/gateway/stream.go index 37c50b5..dfa9d23 100644 --- a/gateway/stream.go +++ b/gateway/stream.go @@ -99,8 +99,14 @@ type Gateway struct { // ReauthorizationInterval rechecks each subscriber independently, even on quiet streams. // Zero disables timed checks; snapshot cycles always reauthorize. + // + // A timed check waits for the subscriber's delivery in progress, so it can only be as prompt as + // the sink lets it be. ServeStream and ServeStreamProjection therefore require a positive + // WriteTimeout with it, and panic before writing anything otherwise. Stream and StreamProjection + // do not: their sink is the host's, and the host must bound its I/O. ReauthorizationInterval time.Duration - // ReauthorizationTimeout bounds a timed check. Zero defaults to 10 seconds. + // ReauthorizationTimeout bounds each timed check's Authorizer and projection-policy callbacks, + // starting once the check holds the subscriber's delivery gate. Zero defaults to 10 seconds. // Authorizers and projection policies must honor context cancellation. ReauthorizationTimeout time.Duration // now is the clock that decides whether a cycle lasted long enough to count. Nil uses time.Now. From aed0f13f74706695666d442dfd6c0f8a38297da4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 12:29:38 +0000 Subject: [PATCH 3/9] docs(kube): document the SubjectAccessReview request A host that caches SubjectAccessReviewAuthorizer decisions has to key them on exactly what the adapter asks, and nothing wrote that down. The exported comment and the shared-watch authorization guide now state it: the subject's user, groups, UID and extras; list then watch, stopping at the first refusal or failure; and the scope's group, version, resource, namespace and name. Label and field selectors and subresource are not sent, so a selector does not narrow the authorization question, and target and projection are not review attributes. The guide also says what a host cache must distinguish: the complete subject and request including verb, the authority or target, and that reuse never bypasses session validity or projection checks. Tests pin the whole request for a named scope, a selector-carrying scope and an all-namespaces scope, and that the first refusal or failure stops the check. The adapter's behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 --- docs/auth.md | 31 +++++++++++++ gateway/kube/authz.go | 18 ++++++++ gateway/kube/authz_test.go | 95 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) diff --git a/docs/auth.md b/docs/auth.md index 985f796..b1150f0 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -160,6 +160,37 @@ Kubernetes. `subjectOf` supplies the API-server-resolved username, groups, UID a both `list` and `watch`. An incomplete review is refused, and an explicit `Denied` wins over `Allowed`. +### What the SubjectAccessReview asks + +A host that caches decisions has to key them on exactly what the adapter asks. For each check it +sends one SubjectAccessReview per verb, `list` then `watch`, and stops at the first refusal or +failure: + +| Field | Value | +|---|---| +| `user`, `groups`, `uid`, `extra` | The `kube.Subject` returned by the host's `SubjectFor`, unchanged. | +| `resourceAttributes.verb` | `list`, then `watch`. | +| `resourceAttributes.group`, `.version`, `.resource` | The scope's group, version and resource. | +| `resourceAttributes.namespace` | The scope's namespace; empty for an all-namespaces scope. | +| `resourceAttributes.name` | The scope's name; empty for a collection. | + +Not sent: the scope's label selector, any field selector, and a subresource. A selector narrows what +a stream delivers, but it does not narrow the authorization question: the review asks about the +whole collection. The scope's target and the stream's projection are not review attributes either; +the clientset given to the adapter decides which API server receives the review. This describes the +adapter's request, not how every Kubernetes authorizer evaluates it. + +A cache in front of the adapter must therefore: + +- key on the complete subject (user, groups, UID and extra) and the complete request, including + the verb; +- keep decisions from different API servers or targets apart, since the same request can have a + different answer elsewhere; +- never let a reused review skip the host's own checks of session validity or projection policy. + +Changing these inputs is a compatibility change for caching hosts, and is announced in the release +notes. The library does not provide a decision cache. + The service account needs `create` on `subjectaccessreviews`; `system:auth-delegator` supplies that permission. Reviews do not require impersonation rights. These are SubjectAccessReview requests, not SelfSubjectAccessReview requests. Cycle and timed checks use the same authorizer. diff --git a/gateway/kube/authz.go b/gateway/kube/authz.go index d84d238..42c2251 100644 --- a/gateway/kube/authz.go +++ b/gateway/kube/authz.go @@ -62,6 +62,24 @@ type SubjectFor func(gateway.Principal) (Subject, error) // It needs the SERVER's own client (a service account) to hold `create` on `subjectaccessreviews`, // which is the standard `system:auth-delegator` role. It does NOT need impersonate rights: this asks // a question about a user, it does not act as one. +// +// # What a review asks +// +// A host that caches decisions must key them on exactly what is asked, so this is a contract: +// +// - Subject: User, Groups, UID and Extra, exactly as subjectFor returns them. +// - Verbs: `list`, then `watch`, one review each. The first refusal or failure ends the check; +// `watch` is not asked about after `list` is refused. +// - Resource attributes: the verb and the scope's Group, Version, Resource, Namespace and Name. +// An empty Namespace asks about every namespace; an empty Name, about the whole collection. +// - Not sent: the scope's LabelSelector, any field selector, and a subresource. A selector narrows +// what the stream delivers, not the question asked: the review covers the whole collection. +// The scope's Target and the stream's projection are not resource attributes either; the client +// cs decides which API server answers. +// +// A future change to these inputs is a compatibility change for caching hosts, and will be called +// out in the release notes. This describes the adapter's request, not how a particular authorization +// policy evaluates it. func SubjectAccessReviewAuthorizer(cs kubernetes.Interface, subjectFor SubjectFor) gateway.Authorizer { return gateway.AuthorizerFunc(func(ctx context.Context, p gateway.Principal, scope gateway.Scope) error { subject, err := subjectFor(p) diff --git a/gateway/kube/authz_test.go b/gateway/kube/authz_test.go index 0c0ec92..0476eeb 100644 --- a/gateway/kube/authz_test.go +++ b/gateway/kube/authz_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "reflect" "strings" "testing" @@ -202,3 +203,97 @@ func TestAThrottledReviewKeepsItsRetryHint(t *testing.T) { } } } + +// The review's inputs are a contract: a host that caches decisions keys on exactly them, and a later +// version that sent another attribute — a label selector, say — would make such a key wrong. So this +// pins the WHOLE request, field by field, for a named scope and for a scope carrying a selector. A +// change here is a change hosts must hear about in the release notes. +func TestSubjectAccessReviewSendsExactlyTheDocumentedAttributes(t *testing.T) { + subject := kube.Subject{ + User: "alice@example.com", + Groups: []string{"devs", "system:authenticated"}, + UID: "4b1d-alice", + Extra: map[string]authzv1.ExtraValue{"scopes.example.com/tenant": {"blue"}}, + } + subjectFor := func(gateway.Principal) (kube.Subject, error) { return subject, nil } + + for name, tc := range map[string]struct { + scope gateway.Scope + want authzv1.ResourceAttributes // without its verb + }{ + "a named object in a group": { + scope: gateway.Scope{Target: "production", Group: "apps", Version: "v1", Resource: "deployments", Namespace: "app", Name: "web"}, + want: authzv1.ResourceAttributes{Group: "apps", Version: "v1", Resource: "deployments", Namespace: "app", Name: "web"}, + }, + "a collection narrowed by a label selector": { + scope: gateway.Scope{Target: "production", Version: "v1", Resource: "configmaps", Namespace: "app", LabelSelector: "tier=web"}, + // The selector is NOT asked about: the review covers the whole collection. + want: authzv1.ResourceAttributes{Version: "v1", Resource: "configmaps", Namespace: "app"}, + }, + "every namespace": { + scope: gateway.Scope{Version: "v1", Resource: "configmaps"}, + want: authzv1.ResourceAttributes{Version: "v1", Resource: "configmaps"}, + }, + } { + t.Run(name, func(t *testing.T) { + cs, asked := reviewer(func(*authzv1.SubjectAccessReview) (bool, bool) { return true, false }) + if err := kube.SubjectAccessReviewAuthorizer(cs, subjectFor).Authorize(t.Context(), "alice", tc.scope); err != nil { + t.Fatalf("Authorize: %v", err) + } + if len(*asked) != 2 { + t.Fatalf("asked %d reviews, want exactly two: list, then watch", len(*asked)) + } + for i, verb := range []string{"list", "watch"} { + ra := tc.want + ra.Verb = verb + want := authzv1.SubjectAccessReviewSpec{ + User: subject.User, Groups: subject.Groups, UID: subject.UID, Extra: subject.Extra, + ResourceAttributes: &ra, + } + got := (*asked)[i].Spec + if !reflect.DeepEqual(got, want) { + t.Errorf("review %d:\n got %+v\nwant %+v", i, got, want) + } + // Spelled out, so the omissions read as deliberate rather than incidental. + if got.ResourceAttributes.LabelSelector != nil || got.ResourceAttributes.FieldSelector != nil { + t.Errorf("review %d carried a selector: %+v", i, got.ResourceAttributes) + } + if got.ResourceAttributes.Subresource != "" || got.NonResourceAttributes != nil { + t.Errorf("review %d asked about a subresource or a non-resource URL", i) + } + } + }) + } +} + +// The adapter stops at the first refusal or failure: a caller refused `list` is never asked about +// `watch`, and the error names the refused verb. +func TestSubjectAccessReviewStopsAtTheFirstRefusal(t *testing.T) { + for name, fail := range map[string]func(*authzv1.SubjectAccessReview) (bool, bool, error){ + "refused": func(*authzv1.SubjectAccessReview) (bool, bool, error) { return false, false, nil }, + "failed": func(*authzv1.SubjectAccessReview) (bool, bool, error) { return false, false, errors.New("unreachable") }, + } { + t.Run(name, func(t *testing.T) { + cs := fake.NewSimpleClientset() + var verbs []string + cs.PrependReactor("create", "subjectaccessreviews", + func(action k8stesting.Action) (bool, runtime.Object, error) { + sar, _ := action.(k8stesting.CreateAction).GetObject().(*authzv1.SubjectAccessReview) + verbs = append(verbs, sar.Spec.ResourceAttributes.Verb) + allowed, denied, err := fail(sar) + if err != nil { + return true, nil, err + } + sar.Status = authzv1.SubjectAccessReviewStatus{Allowed: allowed, Denied: denied} + return true, sar, nil + }) + err := kube.SubjectAccessReviewAuthorizer(cs, subjectOf).Authorize(t.Context(), alice, configmapScope) + if err == nil || !strings.Contains(err.Error(), "list") { + t.Fatalf("err = %v, want a refusal naming list", err) + } + if !reflect.DeepEqual(verbs, []string{"list"}) { + t.Errorf("asked about %v, want only list", verbs) + } + }) + } +} From d7140f8926ac506b0faa91a673204024d55176f2 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 12:29:54 +0000 Subject: [PATCH 4/9] docs: add the second krm-foyer field report and proposal 0008 The field report asks for five shared-watch changes (asks 9 to 13); proposal 0008 selects ask 9, the explicit HTTP validation form of ask 10 and the documentation part of ask 13, and records why asks 11 and 12 are deferred and what would reopen them. The report's links into krm-foyer are made absolute, and a status section records what was taken. Co-Authored-By: Claude Opus 5.5 --- docs/field-reports/second-fieldreport.md | 167 ++++++++++++++ docs/proposals/0008-shared-watch-hardening.md | 208 ++++++++++++++++++ 2 files changed, 375 insertions(+) create mode 100644 docs/field-reports/second-fieldreport.md create mode 100644 docs/proposals/0008-shared-watch-hardening.md diff --git a/docs/field-reports/second-fieldreport.md b/docs/field-reports/second-fieldreport.md new file mode 100644 index 0000000..7678592 --- /dev/null +++ b/docs/field-reports/second-fieldreport.md @@ -0,0 +1,167 @@ +# krm-stream feedback from krm-foyer: shared watches + +krm-foyer now hosts `SharedBackend` with `kube.SubjectAccessReviewAuthorizer` +([roadmap step 5b](https://github.com/ConfigButler/krm-foyer/blob/main/docs/roadmap.md#order-of-work), [watches](https://github.com/ConfigButler/krm-foyer/blob/main/docs/watches.md)). This note asks +for five changes, numbered on from the [first feedback](first-fieldreport.md), whose +asks 1 to 8 krm-stream 0.5.0 and 0.6.0 took. Two (9 and 10) are about guarantees that +do not hold today in a failure case; three (11 to 13) would cut the authorization load +that sharing costs. For each we say what krm-foyer does meanwhile. Everything was +checked against krm-stream main at `5ca19ed` (gateway 0.6.0) on 2026-10-04, by reading +the source and by tests in krm-foyer that reproduce each problem. + +## Status (2026-10-04): asks 9, 10 and 13 are taken + +[Proposal 0008](../proposals/0008-shared-watch-hardening.md) selected ask 9, the "refuse the +combination" form of ask 10, and the documentation part of ask 13: + +- **Ask 9:** a shared scope now opens its upstream watch without the backend-wide lock, under a + context of its own that its waiting callers share; the last to leave cancels it. +- **Ask 10:** `Handler`, `ServeStream` and `ServeStreamProjection` refuse a positive + `ReauthorizationInterval` without a positive `WriteTimeout`, and the + [revocation budget](../auth.md#revocation-budget) is documented in one place. +- **Ask 13:** the [SubjectAccessReview request](../auth.md#what-the-subjectaccessreview-asks) is + documented on the adapter and in the guide, and tests pin it. The exported attribute builder is + deferred. + +Asks 11 and 12 are deferred; the proposal records what would reopen them. + +## The asks at a glance + +| # | Ask | Priority | What krm-foyer does meanwhile | +| --- | --- | --- | --- | +| 9 | Open a shared watch without holding the backend-wide lock, and cancel an opening nobody waits for | High | Its upstream backend returns at once and opens the watch on the first `Next` | +| 10 | Make timed rechecks independent of a blocked write, or require a write timeout with them | High | Sets `WriteTimeout` on every stream, and counts it in its revocation bound | +| 11 | Let a host trigger a recheck | Medium | Rechecks on the timer only | +| 12 | Recheck once per principal and scope, not once per subscriber | Medium | Reuses decisions for a short time in its own authorizer | +| 13 | Say which attributes the SubjectAccessReview asks about | Low | Keys its decisions on them, with a test that fails if they change | + +## Ask 9: open a shared watch without holding the backend-wide lock + +**The problem.** `SharedBackend.Watch` holds `b.mu`, the lock every scope takes, while +`startScope` calls `b.upstream.Watch`, a network round trip to the API server. The +context it passes is `context.WithCancel(context.Background())`, and nothing cancels it +until the scope exists. So one API server slow to answer one opening: + +- **holds up every other scope:** a stream of an unrelated namespace waits for the lock, + and never reaches the API server; +- **cannot be abandoned:** the browser that asked for it leaving cancels its own request, + but not the opening, so its stream slot stays taken, and so does every other waiting + stream's. + +krm-foyer reproduced both: an API server that never answers a watch of one namespace, +not even with headers, held up a stream of another namespace until the test timed out, +and the first stream's slot stayed taken after its browser left. A client-go watch has +no response-header timeout of its own, so the wait is unbounded. + +**What we suggest.** Open the upstream outside `b.mu`: record an in-flight entry for the +scope under the lock, so later subscribers of the same scope wait on it, then open +without the lock. Give the opening a context that its waiting subscribers can cancel +together: when the last one leaves before the watch opened, cancel the opening and +drop the entry. The backoff bookkeeping can stay under the lock. + +**What krm-foyer does meanwhile.** Its upstream backend's `Watch` returns a watcher at +once, and opens the real watch on the first `Next`, with the context `Watch` was given. +`pump` calls `Next` without the lock, and `leave` cancels that context when the last +subscriber goes, so the opening is the scope's alone and ends with it. An opening +failure arrives through `Next` and `die`, which already records an +`UPSTREAM_UNAVAILABLE` for the backoff. This works, but it relies on the order of calls +inside `SharedBackend`, which is not a contract. + +## Ask 10: keep timed rechecks from waiting on a blocked write + +**The problem.** Timed checks and delivery share one gate per subscriber (docs/auth.md: +"Timed checks run per subscriber and pause that subscriber's object delivery"). A write +to a browser that stopped reading blocks once the buffers between are full, and the +recheck waits behind it. `ReauthorizationTimeout` does not cover that wait. With +`WriteTimeout` zero, the default, the write blocks until something else ends the +request, so a revoked grant is never applied to that stream. krm-foyer reproduced it: a +stream whose reader stopped, its buffers filled, its grant revoked, stayed open with no +further reviews, past a bound of about 10 seconds. + +docs/auth.md does say the bound "assumes ... sinks do not block indefinitely", and the +sharedstream example sets `WriteTimeout`. But nothing ties the two together, and a host +that sets `ReauthorizationInterval` alone gets an interval that does not hold. + +**What we suggest,** either of: + +- **Refuse the combination:** `Handler` panics on `ReauthorizationInterval > 0` with + `WriteTimeout == 0`, as it does for other unsafe options, or defaults `WriteTimeout` + then; or +- **Decouple them:** run timed checks apart from delivery, and on a denial end the + request (close the connection), whatever the write in progress is doing. + +Either way, document the whole revocation budget in one place: interval, plus the check +timeout, plus one write timeout, plus cleanup, and whatever caching the host adds. + +**What krm-foyer does meanwhile.** It sets `WriteTimeout` (10 seconds, configurable) on +every stream, shared or not, and its documented bound adds it in. A test stops reading, +fills the buffers, revokes the grant, and sees the stream end within the bound. + +## Ask 11: let a host trigger a recheck + +**The problem.** Rechecks are timed, per subscriber, and only the interval can be +tuned. A shorter interval revokes sooner and costs more reviews; a longer one the +reverse. A host that learns of a change sooner (it watches RBAC's Roles and Bindings, a +webhook authorizer's policy, or its own session store) has no way to apply it before +the next tick. + +**What we suggest.** An API on the gateway to recheck now, for every subscriber or for +those matching a principal or scope, for example `Gateway.Reauthorize(func(Principal, +Scope) bool)`, through the same path as a timed check, so a denial ends only those +streams. A host could then pair an event-driven recheck (immediate for RBAC changes) +with a long periodic one as a safety net for what it cannot watch, such as group +membership from the issuer. For krm-foyer that would make the common revocation +immediate and cut the periodic review load several times over. + +**What krm-foyer does meanwhile.** Rechecks on the timer; its decision guide explains +the trade-off between the interval and the load ([watches](https://github.com/ConfigButler/krm-foyer/blob/main/docs/watches.md#load-on-the-api-server)). + +## Ask 12: recheck once per principal and scope + +**The problem.** Each subscriber has its own recheck timer. A user with nine tabs on one +scope is nine timers, each asking the same two questions, and the timers drift apart +as the tabs open at different times. The reviews then scale with streams, not with +users: with 200 users each on nine scopes, about 120 reviews a second at a 30-second +interval, against 13 for one scope. + +**What we suggest,** either of: + +- one timed check per (principal, scope), whose answer applies to every subscriber of + that pair; or +- a decision cache in `kube`, keyed on exactly the attributes a review sends, that a + host can wrap `SubjectAccessReviewAuthorizer` with, with a lifetime the host chooses + and counted in the revocation budget. Every shared host needs one, and getting the + key wrong is a disclosure, so it belongs next to the authorizer. + +**What krm-foyer does meanwhile.** Its own authorizer wraps the SubjectAccessReview +authorizer with such a cache: a decision is reused for 10 seconds for exactly the same +subject and asked attributes, two checks at once wait for one answer, and an error is +never kept. A fuzz test checks that the cache never changes an answer. Reuse still +depends on the timers falling within one lifetime of each other, which ask 12's first +form would make unnecessary. + +## Ask 13: say which attributes the review asks about + +**The problem.** A host that caches decisions has to know exactly which parts of a scope +the review asks about. Today `review` sends the group, version, resource, namespace and +name, and not the label selector (Kubernetes' `ResourceAttributes` has a `LabelSelector` +since 1.31, unused here). That is reasonable, since RBAC cannot grant by label, but it is +written down nowhere, and a later version that sends selectors would make a cache keyed +without them wrong. + +**What we suggest.** Document the attributes on `SubjectAccessReviewAuthorizer`, and +mention a change to them in the changelog as a breaking change for hosts that cache. An +exported function returning the `ResourceAttributes` for a scope and verb would let a +host key on exactly what is asked. + +**What krm-foyer does meanwhile.** Keys on those five attributes and the subject, and a +test fails if a review it observes carries a selector. + +## What we did not ask for + +- **A response-header timeout in the Kubernetes backend.** Ask 9 is the real fix; with + it, a slow opening costs only its own scope's streams, which their browsers can + abandon. +- **Sharing by default.** We agree with keeping `SharedBackend` opt-in: an identity that + may read everything, and the review load, are not costs every host should carry + unasked. krm-foyer keeps it opt-in too ([why](https://github.com/ConfigButler/krm-foyer/blob/main/docs/watches.md#why-sharing-stays-opt-in)). diff --git a/docs/proposals/0008-shared-watch-hardening.md b/docs/proposals/0008-shared-watch-hardening.md new file mode 100644 index 0000000..ecf5a4a --- /dev/null +++ b/docs/proposals/0008-shared-watch-hardening.md @@ -0,0 +1,208 @@ +# Proposal 0008 Shared watch hardening implementation plan + +**Status: selected scope implemented; asks 11 and 12 deferred.** + +This is the implementer handoff for the [second krm-foyer field report](../field-reports/second-fieldreport.md). +Implement ask 9, the explicit HTTP configuration validation option from ask 10, and the +documentation portion of ask 13. These changes repair shared-watch opening and make the +requirements for timed authorization clearer. Asks 11 and 12 are deferred; they are not +acceptance criteria for this work. + +The local source review used commit `869ea81`. The field report describes a different baseline, +`5ca19ed`, and external krm-foyer reproductions. Those reproductions have not been run in this +repository. Confirm the implementation baseline and add repository-owned tests before changing code. + +## Decisions and work order + +| Order | Deliver | Acceptance evidence | +| --- | --- | --- | +| 1 | Ask 9: independent, cancellable shared-watch opening | Deterministic concurrency tests and Go race checks | +| 2 | Ask 10: require a positive HTTP write timeout with timed reauthorization | Construction and direct-serving validation, plus blocked-reader HTTP coverage | +| 3 | Ask 13: document the actual SubjectAccessReview request | Complete request assertions and matching API and adoption documentation | + +Deliver these as separate reviewable changes. They do not depend on authorization caching, +event-driven policy integration, new wire events, or browser changes. Keep the existing save and +draft-recovery work in [proposal 0006](0006-stream-and-save-implementation-plan.md) independently +deliverable. Follow [design rules](../../CONTRIBUTING.md#design-rules) and +[release policy](../releasing.md). + +## 1 Shared watch opening + +### Required behaviour + +In [shared.go](../../gateway/shared.go), move upstream `Watch` calls outside the backend-wide +mutex. Waiters must be able to abandon an opening through their own contexts. The upstream +opening must have a scope-owned context: cancelling the first caller must not cancel an opening +that another caller still needs. + +Maintain one current opening per scope. Register an opening under the lock, perform network work +without that lock, and publish its result only if that entry is still current. Count waiting callers +so the final departure cancels and removes an unfinished opening. Waiters for unrelated scopes must +continue independently. Keep the current live subscription lifetime: once attached, `Stop` releases +the subscription and the last subscriber cancels the shared upstream. + +Handle the handoff between opening and subscription explicitly. There must be no gap in which the +opening has no waiters or subscribers but retains an upstream watch. If an abandoned upstream +returns a watcher late, stop it and discard it. Its result must not delete a replacement opening, +populate its cache, or change its backoff. + +The implementation may use a private opening entry with a completion signal and waiter accounting; +no public opening API is required. Preserve existing snapshot framing, cache ownership, queue +overflow recovery, failure classification, backoff and lifecycle observations. A cancellation caused +only by all waiters departing is not an upstream failure and must not impose outage backoff. + +### Acceptance tests + +Use channel barriers and a fake upstream that can block inside `Watch`; avoid sleep-based ordering. +Tests must cover: + +- Scope A blocks while scope B opens and delivers its snapshot. +- Concurrent callers for the same scope cause exactly one current upstream opening. +- Cancelling one waiter returns promptly without disrupting remaining waiters. +- Cancelling the final waiter cancels the upstream context, removes the opening and permits a retry. +- A caller whose context is already cancelled does not create or retain an upstream opening. +- Cancellation racing with successful opening leaves neither a watcher nor a subscription orphaned. +- An abandoned opening returning late cannot mutate a newer entry; any returned watcher is stopped. +- A genuine retryable opening failure updates backoff once, and all relevant waiters receive the + classified failure. Existing terminal failures and retry hints retain their behaviour. +- Warm-cache joins, last-subscriber cleanup, overflow and balanced observations remain correct. + +Cancellation guarantees assume the upstream honours context cancellation. Do not promise to forcibly +terminate an arbitrary host backend; ensure its late result cannot affect current state. + +## 2 HTTP configuration for timed reauthorization + +### Chosen implementation + +Require `WriteTimeout > 0` whenever `ReauthorizationInterval > 0` for library-owned HTTP serving. +Reject the invalid combination with a clear configuration panic, consistent with existing option +validation. Do not silently select a timeout. + +Validate at `Handler` construction and before any response I/O in direct `ServeStream` and +`ServeStreamProjection` calls. Share private validation where useful so these paths cannot diverge. +Retain existing rejection of negative write timeouts. A zero write timeout remains valid for HTTP +streams with timed reauthorization disabled. + +Transport-neutral `Stream` and `StreamProjection` must continue to support timed checks with +host-owned bounded sinks. Do not require an HTTP-only option for those methods. Update the +`Options` and `Gateway` comments to distinguish these paths. + +Preserve the delivery gate and existing per-subscriber authorization and projection checks. This +increment does not introduce a cancellable gate, independent authorization scheduler, or force-close +API. Positive timeout capability checks must retain their current fail-before-streaming behaviour. + +### Documentation and limits + +Update [auth.md](../auth.md), [operations.md](../operations.md), [adopting.md](../adopting.md), and +affected examples. The timed-recheck example must set both authorization and write budgets. + +Explain that `ReauthorizationTimeout` starts after gate acquisition and covers the periodic callbacks. +Describe the revocation budget in one place, including timer scheduling, gate waiting, callback +timeout, host cache freshness, and termination/cleanup. Link to it from the other guides. +Do not turn interval + callback timeout + one write timeout into an unconditional bound: +terminal delivery, competing I/O, scheduling and host cleanup can add time. Distinguish cessation of +new object delivery, request return, and browser receipt. Already buffered bytes cannot be recalled. +Opening and cycle authorization still require host callback deadlines. + +### Acceptance tests + +- `Handler` rejects timed reauthorization with zero write timeout at construction. +- Both direct HTTP entry points reject that combination before headers, observations for logical + stream opening, or backend calls. Use a writer spy to prove no response I/O occurred. +- Positive write timeout with timed checks is accepted; zero timeout without timed checks remains + accepted. Generic bounded sinks can use timed checks without an HTTP write timeout. +- Reuse or extend the real HTTP blocked-reader tests in [sse_test.go](../../gateway/sse_test.go). + Fill transport buffers, revoke access, and verify that the request terminates within a declared + test budget and releases its subscription. A healthy subscriber must remain unaffected. +- Preserve HTTP/1.1 and TLS HTTP/2 coverage, unsupported-writer rejection, quiet-stream survival, + and authorization/projection denial tests. Do not require a terminal frame to reach a stalled client. + +Record which coverage is reused and which scenario is new. State the test's callback, write and +scheduling assumptions; a generous test tolerance is not a published production guarantee. + +This is a breaking configuration change. Include a `BREAKING CHANGE:` footer explaining that HTTP +hosts using timed reauthorization must choose a positive per-operation `WriteTimeout` and verify +their mounted middleware supports deadlines and flushing. Update repository callers together. + +## 3 SubjectAccessReview documentation + +Document [SubjectAccessReviewAuthorizer](../../gateway/kube/authz.go) as currently implemented: + +- Subject inputs are username, groups, UID and extras, supplied by the host's `SubjectFor`. +- The adapter checks `list` and `watch`, stopping on the first refusal or failure. +- Resource attributes are verb, group, version, resource, namespace and name. +- Label and field selectors and subresource are not supplied. Target and projection are not SAR + resource attributes; the configured client selects the authority receiving the review. + +Add this contract to the exported API comment and shared-watch authorization guide. State that +selectors currently do not narrow the authorization question. Keep this description specific to the +adapter; do not generalize it into a guarantee about all Kubernetes authorization policies. + +Extend [authz_test.go](../../gateway/kube/authz_test.go) to assert both verbs, every transmitted +subject/resource attribute, and the omitted selector/subresource fields. Include a named scope and +a scope carrying selectors so omissions are deliberate and tested. + +Explain that a host cache must distinguish the complete subject and request, including verb, and +isolate decisions by authority or target binding. Reusing a SAR result must not bypass current host +session validity or projection checks. No cache implementation or exported attribute builder is +part of this increment. + +Future changes to the documented authorization inputs require compatibility review and explicit +release notes for caching hosts. The current documentation change does not itself change SAR +semantics. Do not manually edit generated changelogs; use conventional commits and PR details. + +## Deferred work and conditions for reconsideration + +**Ask 11:** no gateway-wide `Reauthorize(predicate)` API in this plan. Reconsider a minimal trigger +only with a concrete host integration, stream lifecycle and trigger-coalescing rules, a distinction +between scheduling and completion, and coordination with any authorization cache invalidation. +Policy watches and determining affected subjects remain host-owned. Preserve periodic checks as a +fallback, and do not promise immediate revocation from a trigger alone. + +**Ask 12:** no grouping scheduler and no decision cache in this plan. `Principal` is opaque; matching +users does not establish interchangeable sessions or projection policy. Correct the workload before +evaluating benefit: 200 users with nine tabs on one scope produce about 120 periodic SARs/s at a +30-second interval and could reduce to about 13/s with deduplication. Nine distinct scopes per user +still require about 120/s when grouping by principal and scope. These counts assume both verbs are +allowed and exclude opening and recovery checks. + +An optional kube cache can be reconsidered with measured duplicate requests, a complete key, +authority isolation, bounded memory, explicit TTL and invalidation, error handling and waiter +cancellation semantics. Deduplicating SARs must remain separate from subscriber-specific host +authorization and projection decisions. An exported attribute builder also remains deferred. + +## Verification and completion + +For each implementation PR, add focused tests proving the changed behaviour. Before opening the PR, +run `task fixtures-check`, `task test`, and `task lint` as required by CONTRIBUTING. Run the affected +Go package tests with the race detector for shared opening changes. Inspect existing test/task setup +to run each Go module correctly. No new wire fixture is needed unless implementation unexpectedly +changes the shared protocol contract; such a change requires separate scope review. + +The final handoff from the implementer must identify the tested commit, commands and results, +unrun external scenarios, compatibility change, and remaining limitations. All three selected +increments must be complete; deferred asks must not be reported as implemented. No benchmark, +external krm-foyer reproduction, universal capacity claim, or new real-cluster campaign is a merge +requirement for this bounded work. + +## Draft response to the krm foyer team + +Thanks for the concrete failure cases and the explanation of your workarounds. We are taking on +ask 9, the explicit HTTP validation option in ask 10, and documentation of the current SAR inputs +from ask 13. + +We will isolate upstream openings by scope and cancel an unfinished opening when its final waiter +leaves. For HTTP streams with timed authorization, we will require an explicit positive write timeout +and document the budget and its limits. We will document the exact review inputs and add tests +protecting them. + +We are deferring the public trigger API, grouped authorization scheduler, cache and exported +attribute builder. These need additional contracts around session identity, policy checks, +invalidation and measured duplicate load. Please distinguish nine tabs on one scope from nine +distinct scopes in the load example; grouping only removes duplicate questions for the same pair. + +Please share the opening-cancellation and blocked-reader reproductions with their tested commits and +transport settings. They will help us compare repository tests with your integration. Also include +verb and authority isolation explicitly in the cache-key description. We will provide a configuration +migration note with the timed-authorization change. This scope does not promise immediate revocation +or a universal bound for arbitrary host callbacks and sinks. From ce94d08a9ac1044395dc93811ed17176b5947f9f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 13:58:03 +0000 Subject: [PATCH 5/9] docs(kube): describe SubjectAccessReview namespace and name as copied from the scope An empty namespace also means a cluster-scoped resource, and a named scope still carries its name, so the adapter comment no longer reads an empty namespace as every namespace or the omitted selector as a whole-collection review. It says namespace and name are copied from the scope, and selectors are omitted from the authorization request. Co-Authored-By: Claude Opus 5.5 --- gateway/kube/authz.go | 11 +++++------ gateway/kube/authz_test.go | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/gateway/kube/authz.go b/gateway/kube/authz.go index 42c2251..5f6957b 100644 --- a/gateway/kube/authz.go +++ b/gateway/kube/authz.go @@ -70,12 +70,11 @@ type SubjectFor func(gateway.Principal) (Subject, error) // - Subject: User, Groups, UID and Extra, exactly as subjectFor returns them. // - Verbs: `list`, then `watch`, one review each. The first refusal or failure ends the check; // `watch` is not asked about after `list` is refused. -// - Resource attributes: the verb and the scope's Group, Version, Resource, Namespace and Name. -// An empty Namespace asks about every namespace; an empty Name, about the whole collection. -// - Not sent: the scope's LabelSelector, any field selector, and a subresource. A selector narrows -// what the stream delivers, not the question asked: the review covers the whole collection. -// The scope's Target and the stream's projection are not resource attributes either; the client -// cs decides which API server answers. +// - Resource attributes: the verb, and the scope's Group, Version and Resource. Namespace and +// name are copied from the scope. +// - Not sent: selectors are omitted from the authorization request (the scope's LabelSelector, +// and any field selector), as is a subresource. The scope's Target and the stream's projection +// are not resource attributes either; the client cs decides which API server answers. // // A future change to these inputs is a compatibility change for caching hosts, and will be called // out in the release notes. This describes the adapter's request, not how a particular authorization diff --git a/gateway/kube/authz_test.go b/gateway/kube/authz_test.go index 0476eeb..0b66553 100644 --- a/gateway/kube/authz_test.go +++ b/gateway/kube/authz_test.go @@ -227,7 +227,7 @@ func TestSubjectAccessReviewSendsExactlyTheDocumentedAttributes(t *testing.T) { }, "a collection narrowed by a label selector": { scope: gateway.Scope{Target: "production", Version: "v1", Resource: "configmaps", Namespace: "app", LabelSelector: "tier=web"}, - // The selector is NOT asked about: the review covers the whole collection. + // Selectors are omitted from the authorization request. want: authzv1.ResourceAttributes{Version: "v1", Resource: "configmaps", Namespace: "app"}, }, "every namespace": { From 203d2a19be4d37e8c42b2695231cfbfcbbec76dd Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 13:58:03 +0000 Subject: [PATCH 6/9] docs(gateway): shorten shared-watch and handler comments Keep the invariants, lock ownership and cancellation semantics; drop narratives about earlier implementations and emphatic restatements of the security rationale, which docs/auth.md carries. Comment-only: the code is unchanged. Co-Authored-By: Claude Opus 5.5 --- gateway/handler.go | 68 ++++------- gateway/shared.go | 280 ++++++++++++++------------------------------- 2 files changed, 103 insertions(+), 245 deletions(-) diff --git a/gateway/handler.go b/gateway/handler.go index 19366c7..d419ba8 100644 --- a/gateway/handler.go +++ b/gateway/handler.go @@ -7,59 +7,34 @@ import ( "time" ) -// The http.Handler the README has been promising, and which did not exist. -// -// That is worth saying plainly rather than quietly fixing: the first code block in the project's -// README called `gateway.Handler(gateway.Options{...})`, and neither identifier was real. The first -// thing a new adopter copies did not compile. What existed instead was ServeStream(w, r, principal, -// scope) — which is a fine seam, but it left every host to write the same four things by hand: parse -// the scope out of the query, decide what a principal is, check the scope against an allowlist, and -// wire the three together. Four chances to get security-relevant glue subtly wrong, per adopter. -// -// So the library now ships the glue, and it is one line to mount: -// -// mux.Handle("/resource-stream/v1", gateway.Handler(gateway.Options{ -// Principal: func(r *http.Request) (gateway.Principal, error) { return userFrom(r) }, -// Authorizer: myAuthz, -// Clients: myClientFor, -// Scopes: gateway.ScopePolicy{Targets: []string{""}, Resources: []gateway.GroupResource{{Resource: "configmaps", Scope: gateway.ResourceScopeNamespaced}}}, -// })) -// -// ServeStream stays exported. A host that wants to route, name or authorize scopes its own way still -// can — the Handler is the paved road, not a wall. +// Handler is the paved road: it parses the scope from the query, resolves the principal, checks the +// scope against the allowlist and serves the stream. ServeStream remains for hosts that route, name +// or authorize scopes their own way. -// Options configures a Handler. Everything without a default is required, and Handler PANICS if one -// is missing — at mount time, on the first line of main(), not on a request from a real user hours -// later. A stream gateway that silently defaulted its authorizer would be a vulnerability with a -// changelog entry. +// Options configures a Handler. Everything without a default is required, and Handler panics at +// construction if one is missing or an option is unsafe, so misconfiguration fails at startup rather +// than on a request. type Options struct { - // Principal answers "who is calling?" from the request — a session cookie, an mTLS peer, a - // header your ingress set. Return an error to refuse. + // Principal resolves who is calling from the request: a session cookie, an mTLS peer, a header + // your ingress set. Required; any default would be a policy decision about the host's auth system. // // Return a *StreamError to choose what the caller is told: Unauthenticated when signing in again - // will help, UpstreamUnavailable when your session store is down and the honest answer is "try - // later". Any other error is sent as UNAUTHENTICATED "not authenticated", and its text goes only - // to Diagnostics: what went wrong identifying a caller is your business, and possibly your - // internals. - // - // Required, and there is deliberately no default. Every default that could go here ("nil - // principal", "the request's Authorization header") is a policy decision about someone's auth - // system, and this library does not get to make one. + // will help, UpstreamUnavailable when your session store is down. Any other error is sent as + // UNAUTHENTICATED "not authenticated", and its text goes only to Diagnostics. Principal func(*http.Request) (Principal, error) - // Authorizer decides whether that principal may open this scope, BEFORE any watch is opened. - // Required. gateway.AllowAll{} exists, is tedious to type on purpose, and shows up in a diff. + // Authorizer decides whether that principal may open this scope, before any watch is opened. + // Required; gateway.AllowAll{} permits everything explicitly. Authorizer Authorizer // Clients resolves (target, principal) to an upstream. Required. Clients ClientFor - // Scopes is the allowlist (spec §8). Required, and deny-by-default: the zero value streams - // nothing, so forgetting it fails closed. Setting both Resources and AnyResource panics. + // Scopes is the allowlist (spec §8). The zero value streams nothing. Setting both Resources and + // AnyResource panics. Scopes ScopePolicy - // Projection defaults to ProjectionFull — the safe one. A gateway that defaulted to raw and - // streamed Secret values because someone omitted a line would have a vulnerability, not a bug. + // Projection defaults to ProjectionFull, which redacts Secret values. Projection Projection // Projections authorizes a requested projection for a principal and scope. It is optional only @@ -82,7 +57,7 @@ type Options struct { // WriteTimeout bounds each HTTP write-plus-flush operation. Zero installs no deadline. // Positive values require a writer supporting flush and write deadlines; negative values panic. - // Required (positive) whenever ReauthorizationInterval is: see below. + // Required (positive) whenever ReauthorizationInterval is. WriteTimeout time.Duration // ReauthorizationInterval rechecks each subscriber independently, even on quiet streams. @@ -100,13 +75,10 @@ type Options struct { // Handler mounts the stream on one route. // -// Every rejection — a malformed scope, a disallowed one, an unidentifiable caller — is delivered as -// a TERMINAL SSE error event over a 200, not as an HTTP status code. That is not sloppiness, it is -// the only thing that works: a browser's EventSource cannot read the body of a non-200, so a 403 -// reaches the page as an `onerror` with no detail and no reason, and the developer is left guessing. -// A terminal error event carries a code and a message the UI can actually show, and `terminal` is -// what stops EventSource from reconnecting forever. Unsupported bounded transports instead -// abort before streaming and report ObservationHTTPTransportRejected. +// Every rejection (a malformed or disallowed scope, an unidentifiable caller) is delivered as a +// terminal SSE error event over a 200, not as an HTTP status: EventSource cannot read a non-200 body, +// and `terminal` stops it reconnecting. Unsupported bounded transports instead abort before streaming +// and report ObservationHTTPTransportRejected. func Handler(o Options) http.Handler { validateHTTPServing(o.WriteTimeout, o.ReauthorizationInterval) switch { diff --git a/gateway/shared.go b/gateway/shared.go index c08f1c2..5f1f9f2 100644 --- a/gateway/shared.go +++ b/gateway/shared.go @@ -8,61 +8,26 @@ import ( "time" ) -// Fan-out: one upstream watch per SCOPE, not per browser tab. +// SharedBackend fans one upstream watch per scope out to every subscriber, so ten tabs on one +// namespace cost one watch and one warm cache, and a joining subscriber is snapshotted from memory. // -// Without this, ten tabs on the same namespace are ten watches on the API server, ten snapshots, and -// ten copies of the same object graph — and a reconnect storm (a laptop lid closing on a floor of -// them) multiplies it. With it, they are one watch and one warm cache, and a joining subscriber gets -// its reset…synced from that cache without touching the API server at all. -// -// # It is OPT-IN, and here is the thing you are opting into -// -// A shared watch can only be opened ONCE, so it can only be opened as ONE identity. That is the -// whole trade, and it must be stared at rather than glossed: -// -// - WITHOUT sharing, ClientFor hands the gateway a client acting AS the caller. Kubernetes' own -// RBAC is then the enforcement: if the caller may not watch Secrets, the API server says so, and -// no bug in this library can change that. Authorization is defence in depth. -// - WITH sharing, the upstream watch runs as ONE identity — your service account — and every -// subscriber reads from its cache. **Your Authorizer becomes the only thing standing between a -// caller and the objects.** A bug there is not a bug, it is a disclosure. -// -// So SharedBackend is not the default and never will be. A host opts in by wiring it deliberately — -// and there is a way to opt in WITHOUT giving up the boundary, which is the wiring you want: -// -// shared := gateway.NewSharedBackend(myServiceAccountBackend) // one watch, ONE identity… -// opts.Authorizer = kube.SubjectAccessReviewAuthorizer(clientset, subjectOf) // …but Kubernetes still decides -// opts.Clients = func(context.Context, string, gateway.Principal) (gateway.Backend, error) { return shared, nil } -// -// kube.SubjectAccessReviewAuthorizer asks the API server, with a SubjectAccessReview, whether THIS user may list and -// watch THIS resource here — before the subscriber is served from the shared cache. RBAC is the -// boundary again, and the sharing costs you nothing but a round-trip per snapshot cycle. See +// It is opt-in because the shared watch is opened once, as one identity. Without sharing, each +// stream acts as its caller and Kubernetes RBAC is the boundary. With it, every subscriber reads the +// service identity's cache, and the host's Authorizer is the only check between a caller and the +// objects. Pair it with kube.SubjectAccessReviewAuthorizer so Kubernetes still decides; see // docs/auth.md. -// -// The library cannot make that choice for you, because it is a choice about YOUR threat model. What -// it can do is refuse to make it silently, which is what this comment is for. -// sharedQueueDepth is how many live events one subscriber may fall behind by before the gateway gives -// up on catching it up and resnapshots it instead. -// -// A bounded queue is not a limitation here, it is the design. The alternative — an unbounded one — -// turns a single slow consumer (a backgrounded tab, a paused debugger) into unbounded memory growth -// in a process serving everyone else. When a subscriber overflows, it is handed a continuity-loss -// error, which the stream loop already knows how to recover from: a fresh reset…synced, served from -// the warm cache, costing the API server nothing. Slowness degrades into a resnapshot, never into a -// leak and never into a lie. +// sharedQueueDepth is how many live events one subscriber may fall behind by before it is +// resnapshotted from the warm cache. The bound keeps one slow consumer from growing memory without +// limit in a process serving everyone else. const sharedQueueDepth = 256 -// The backoff after a shared upstream watch fails: opening it fails with UPSTREAM_UNAVAILABLE, the open -// watch dies of it, or the watch keeps ending before it is of use. Its subscribers each reconnect on -// their own budget, so without it the API server would see one attempt per subscriber per retry. With -// it, a scope sees at most one attempt per backoff period however many subscribers are waiting, and -// the rest are told when to come back. +// The backoff after a shared upstream watch fails to open with UPSTREAM_UNAVAILABLE, dies of it, or +// keeps ending before it is of use. A scope then sees at most one attempt per backoff period, however +// many subscribers reconnect; the rest are told when to come back. // -// A watch is of use once it has completed its snapshot and stayed live for minUsefulCycle, the same -// rule the stream loop applies to one connection. Only such a watch's end resets the backoff; one -// that opens and fails at once has not recovered. One early end is tolerated, as continuity loss is -// (§5); a second in a row backs off. +// A watch is of use once it completed its snapshot and stayed live for minUsefulCycle; only such a +// watch's end resets the backoff. One early end is tolerated (§5); a second in a row backs off. const ( sharedRetryMin = time.Second sharedRetryMax = 30 * time.Second @@ -77,11 +42,11 @@ type SharedOptions struct { Observer Observer } -// SharedBackend multiplexes many consumers onto one upstream watch per scope. +// SharedBackend multiplexes many consumers onto one upstream watch per scope. It is a Backend, so it +// drops in behind the same seam as any other. // -// It IS a Backend, so it drops in behind the same seam as any other — the stream loop cannot tell the -// difference, which is exactly what "the protocol is backend-agnostic" has to mean if it means -// anything (spec §5, §6). +// b.mu guards scopes, openings and backoff, and is never held across an upstream call. Lock order is +// b.mu before a sharedScope's mu. type SharedBackend struct { upstream Backend queueDepth int @@ -94,14 +59,9 @@ type SharedBackend struct { now func() time.Time } -// sharedOpening is one scope's upstream watch while it is being opened: registered under the -// backend's lock, opened without it, and settled under it again. -// -// It exists because opening is a network round trip of unknown length, and the backend's lock is -// taken by every scope. Holding the lock across it let one slow API server hold up every unrelated -// namespace, and gave a caller nothing to abandon: its own context was not the opening's, and nothing -// else could cancel it. Now the lock covers only the bookkeeping, and an opening belongs to whoever -// is waiting for it, counted, so the last one to leave cancels it. +// sharedOpening is one scope's upstream watch while it is being opened: registered under b.mu, opened +// without it, and settled under it again. Opening is a network round trip of unknown length, so it +// must hold up only its own scope's callers, each of whom can leave; the last to leave cancels it. type sharedOpening struct { key string scope Scope @@ -117,9 +77,8 @@ type sharedOpening struct { // waiters counts the callers still waiting. It changes only before done is closed; after that, // every waiter takes one of subs (or err) on its way out, however it leaves. waiters int - // subs holds one subscription per waiter when the opening succeeds, attached to the new scope - // at the moment it is published. A waiter therefore never has to subscribe after the fact, and - // there is no instant at which the scope holds an upstream watch and nobody who will release it. + // subs holds one subscription per waiter when the opening succeeds, attached when the scope is + // published, so the scope never holds an upstream watch without someone who will release it. subs []*subscriber // err is the opening's failure, shared by every waiter. err error @@ -133,10 +92,8 @@ type sharedBackoff struct { early int } -// NewSharedBackend shares one upstream watch per scope across every consumer of it. -// -// Read the package comment above about identity before you wire this in: the upstream is opened once, -// as whatever identity `upstream` carries, and your Authorizer becomes the security boundary. +// NewSharedBackend shares one upstream watch per scope across every consumer of it. The upstream is +// opened as whatever identity `upstream` carries, so your Authorizer becomes the security boundary. func NewSharedBackend(upstream Backend) *SharedBackend { return NewSharedBackendWithOptions(upstream, SharedOptions{}) } @@ -168,20 +125,11 @@ var _ Backend = (*SharedBackend)(nil) // Watch joins the shared watch for this scope, opening it if this is the first subscriber. // -// The caller's context bounds only the caller's WAIT, never the upstream watch. It is the context of -// ONE browser's request; the upstream watch belongs to ALL of them. Handing it to the upstream would -// mean the whole shared watch — and everyone else's stream — dies the moment whichever tab happened to -// open it goes away. So the upstream gets a context of its own, owned by the scope: -// -// - while it is opening, by everyone waiting for it. Each may give up through its own context, and -// returns at once; the last to give up cancels the opening. -// - once it is open, by its subscribers. Each one's lifetime is bounded by Stop(), and the last one -// out cancels the upstream (see leave). -// -// The backend's lock is never held across the upstream's Watch, so a scope that is slow to open holds -// up only the callers waiting for that scope. +// ctx bounds only this caller's wait, never the upstream watch, which belongs to every subscriber. +// The upstream has a context of its own, cancelled while opening by the last waiter to leave, and +// once open by the last subscriber to Stop (see leave). func (b *SharedBackend) Watch(ctx context.Context, scope Scope) (Watcher, error) { - // A caller that has already gone must not start, or join, an opening it would at once abandon. + // A caller that has already gone neither starts nor joins an opening. if err := ctx.Err(); err != nil { return nil, err } @@ -225,11 +173,9 @@ func (b *SharedBackend) beginOpeningLocked(key string, scope Scope) *sharedOpeni return op } -// settle publishes an opening's result to its waiters, if it still has any. -// -// An opening every waiter abandoned is discarded whole: its watcher is stopped, and it touches -// neither the scope map, nor the backoff, nor whatever opening has since replaced it. Its failure, if -// it failed, is most likely the cancellation its waiters caused, and in any case nobody asked. +// settle publishes an opening's result to its waiters. An opening every waiter abandoned is +// discarded: its watcher is stopped, and it touches neither the scope map, the backoff, nor any +// opening that replaced it. func (b *SharedBackend) settle(op *sharedOpening, w Watcher, err error) { b.mu.Lock() if b.openings[op.key] != op { // its last waiter left, and unregistered it @@ -248,8 +194,7 @@ func (b *SharedBackend) settle(op *sharedOpening, w Watcher, err error) { } else { s := newSharedScope(b, op.key, op.scope, w, op.cancel) b.scopes[op.key] = s - // Attach every waiter before the scope is visible to anyone else, so it is never without the - // subscribers whose departure will cancel it. + // Attach every waiter before the scope is visible, so its last departure cancels it. s.mu.Lock() for range op.waiters { op.subs = append(op.subs, s.attachLocked()) @@ -274,10 +219,9 @@ func (b *SharedBackend) collect(op *sharedOpening) (Watcher, error) { return &sharedWatcher{scope: sub.scope, sub: sub}, nil } -// abandon withdraws a waiter whose context ended. Before the opening settles, the waiter simply stops -// being counted, and the last one cancels the opening and unregisters it, so the next Watch starts -// afresh. After it settles — the two raced — the waiter still owns a subscription, and releases it -// exactly as a subscriber would. +// abandon withdraws a waiter whose context ended. Before the opening settles, the waiter stops being +// counted, and the last one cancels and unregisters the opening. If it settled first, the waiter +// owns a subscription and releases it as a subscriber would. func (b *SharedBackend) abandon(op *sharedOpening) { b.mu.Lock() select { @@ -307,9 +251,8 @@ func (b *SharedBackend) backoffRemainingLocked(key string) time.Duration { return 0 } -// recordFailureLocked starts or doubles a scope's backoff after a retryable failure. Any other -// failure is not the upstream being away, and backing off would only delay the real answer; it -// leaves the backoff as it is, because it is not a recovery either. +// recordFailureLocked starts or doubles a scope's backoff after a retryable UPSTREAM_UNAVAILABLE. Any +// other failure leaves the backoff unchanged. func (b *SharedBackend) recordFailureLocked(key string, err error) { var se *StreamError if !errors.As(err, &se) || se == nil || se.Terminal || se.Code != CodeUpstreamUnavailable { @@ -340,11 +283,8 @@ func (b *SharedBackend) recordFailureLocked(key string, err error) { bo.until = now.Add(wait) } -// newSharedScope wraps an opened upstream watch. Its context and cancel are the opening's: the -// watch's own, detached from whichever subscriber's arrival happened to open it. Tying the shared -// watch to one browser's request context would mean that when that particular tab closes, everyone -// else's stream dies with it — a bug that would be invisible with one subscriber and baffling with -// two. +// newSharedScope wraps an opened upstream watch. cancel is the opening's, detached from every +// subscriber's request context. func newSharedScope(b *SharedBackend, key string, scope Scope, w Watcher, cancel context.CancelFunc) *sharedScope { return &sharedScope{ backend: b, @@ -368,10 +308,9 @@ type scopeEnd struct { cause error } -// forget drops a dead-or-empty scope so the next Watch opens a fresh one, and accounts for how it -// ended in the same critical section, so no Watch can slip in between and reopen the upstream -// without seeing the backoff. Only the scope's current watch is accounted for: an obsolete one ending -// late must not touch a newer attempt's backoff. +// forget drops a dead or empty scope and accounts for how it ended in the same critical section, so +// no Watch can reopen the upstream without seeing the backoff. An obsolete scope ending late changes +// nothing. func (b *SharedBackend) forget(key string, s *sharedScope, end scopeEnd) { b.mu.Lock() defer b.mu.Unlock() @@ -429,53 +368,28 @@ func (s *sharedScope) usefulLocked() bool { return s.synced && s.backend.now().Sub(s.syncedAt) >= minUsefulCycle } -// subscriber is one consumer's view of the shared watch: a SNAPSHOT, then a bounded queue of live -// events. -// -// # Why two queues, and not one -// -// The first version put both through the bounded channel, and it was broken in a way that only a -// realistic scope reveals: a namespace with more objects than the queue is deep (300 ConfigMaps; -// sharedQueueDepth is 256) could not be served AT ALL. subscribe() fills the queue from the warm -// cache while nobody is draining it yet, the 257th object overflows, the subscriber is told it "fell -// behind" — and the stream loop's recovery for that is to resnapshot, which does the same thing -// again. **An infinite resync loop, on a small namespace.** Both of my own tests used two objects. -// -// The two things were never the same, and conflating them was the bug: -// -// - the SNAPSHOT is the consumer's STARTING STATE. It is finite, known in advance, and dropping any -// of it is not backpressure — it is a wrong answer, and the recovery for a wrong answer is to send -// it again, forever. It gets an unbounded slice, drained first. -// - LIVE EVENTS are open-ended. A consumer that cannot keep up with them genuinely is falling -// behind, and resnapshotting it from the warm cache is exactly right. They keep the bounded -// channel, and the backpressure it exists for. +// subscriber is one consumer's view of the shared watch: a snapshot, then a bounded queue of live +// events. They are kept apart because they differ: the snapshot is the consumer's starting state, +// finite and never to be dropped, so it is an unbounded slice drained first; live events are +// open-ended, and a consumer that cannot keep up with them is resnapshotted. (Sending the snapshot +// through the bounded queue made any scope larger than the queue unservable.) type subscriber struct { scope *sharedScope // pending is the snapshot: added* and the boundary bookmark. Guarded by sharedScope.mu, and // drained by Next() before a single live event is read. pending []WatchEvent - // ready wakes a reader that is already blocked on `ch` when the snapshot lands in `pending`. - // - // It is not decoration. The snapshot no longer travels through `ch`, so a consumer that called - // Next() before the upstream finished its first cycle parks on a channel that will never carry - // the thing it is waiting for — and waits forever. (It did. Every test that subscribed before the - // boundary bookmark hung for exactly the timeout.) Buffered 1: a missed signal is impossible, - // because a reader re-checks `pending` before parking again. + // ready wakes a reader already blocked on `ch` when the snapshot lands in `pending`, which never + // travels through `ch`. Buffered 1: a reader re-checks `pending` before parking again, so no + // signal is missed. ready chan struct{} ch chan WatchEvent // awaiting is true until this subscriber has been handed its snapshot. Live events are not - // delivered to it before then — it will receive the cache instead, which ALREADY contains them. - // Forwarding both would deliver an object twice, and the second copy could be older. + // delivered before then: the cache it will receive already contains them. awaiting bool closed bool - // reason is why this subscriber's queue ended, and it lives BESIDE the queue rather than in it. - // - // The first version of this pushed the error INTO the channel — which cannot work, because the - // case where we need to send it is precisely the case where the channel is full. The reason was - // silently dropped and the consumer saw a bare close: it still recovered (a closed watch means - // resnapshot), but nothing anywhere could say WHY, which is the difference between a system you - // can operate and one you can only restart. Written before close(ch); a receiver that observes - // the close is guaranteed to see it. + // reason is why this subscriber's queue ended. It lives beside the queue, not in it, because the + // queue may be full when it ends. Written before close(ch), so a receiver that observes the close + // sees it. reason error } @@ -514,9 +428,7 @@ func (s *sharedScope) attachLocked() *subscriber { s.subs[sub] = struct{}{} s.backend.observe(Observation{Kind: ObservationSharedSubscriptionOpened, Scope: s.scope}) - // The warm cache, and the entire point of the exercise: if the upstream snapshot is already - // complete, this consumer gets its whole reset…synced now, from memory, and the API server never - // hears about it. + // A warm cache snapshots the joiner from memory, without touching the API server. if s.synced { s.deliverSnapshotLocked(sub) } @@ -524,12 +436,8 @@ func (s *sharedScope) attachLocked() *subscriber { } // deliverSnapshotLocked hands one subscriber the whole cache as a snapshot, terminated by the -// boundary bookmark. Called with s.mu held, which is what keeps it atomic with respect to live -// events: no event can slip between the snapshot and the bookmark. -// -// It CANNOT overflow, and that is the point. The snapshot is the consumer's starting state, not a -// backlog — see the subscriber comment. Its size is the size of the scope, which the operator chose -// when they allowlisted it; a 5000-object namespace costs a 5000-element slice, once, per joiner. +// boundary bookmark. Called with s.mu held, so no live event can slip between the two. It cannot +// overflow: its size is the scope's, which the operator allowlisted. func (s *sharedScope) deliverSnapshotLocked(sub *subscriber) { if sub.closed { return @@ -541,18 +449,16 @@ func (s *sharedScope) deliverSnapshotLocked(sub *subscriber) { sub.pending = append(sub.pending, WatchEvent{Type: WatchBookmark, InitialEventsEnd: true}) sub.awaiting = false - // Wake a reader that is already parked on `ch` waiting for a snapshot that will never arrive - // there. Non-blocking: the buffer holds the one signal that matters. + // Wake a reader parked on `ch`. Non-blocking: the buffer holds the one signal that matters. select { case sub.ready <- struct{}{}: default: } } -// offer enqueues without blocking. A full queue means this consumer cannot keep up, and the honest -// response is to stop trying: hand it a continuity-loss error, which the stream loop recovers from -// with a fresh snapshot off the warm cache. Blocking here would let one stalled browser stall the -// pump, and with it every other subscriber to this scope. +// offer enqueues without blocking. A full queue ends the subscriber with a continuity-loss error, +// which the stream loop recovers from with a fresh snapshot off the warm cache. Blocking would let +// one stalled browser stall the pump, and every other subscriber with it. func (sub *subscriber) offer(ev WatchEvent) bool { if sub.closed { return false @@ -568,18 +474,16 @@ func (sub *subscriber) offer(ev WatchEvent) bool { } } -// pump is the single reader of the upstream watch. One goroutine per scope, for the life of the -// upstream watch, and it is the only thing that ever touches the cache. +// pump is the single reader of the upstream watch and the only writer of the cache: one goroutine per +// scope, for the life of the upstream watch. func (s *sharedScope) pump(ctx context.Context) { defer s.watcher.Stop() for { ev, err := s.watcher.Next(ctx) if err != nil { - // Upstream ended — cleanly (reopen), or with a 410, or the context went away. Every one of - // those means the same thing to a subscriber: continuity is lost, start a new cycle. The - // scope dies here; the next Watch builds a fresh one, and N subscribers resyncing at once - // still produce exactly ONE new upstream watch. + // However the upstream ended, continuity is lost. The scope dies, and subscribers + // resyncing at once share the one fresh upstream watch the next Watch opens. s.die(err) return } @@ -588,8 +492,7 @@ func (s *sharedScope) pump(ctx context.Context) { switch ev.Type { case WatchBookmark: if ev.InitialEventsEnd { - // The upstream snapshot is complete: the cache is now a true picture of the scope, and - // everyone waiting for one can have it. + // The cache is now a true picture of the scope; hand it to everyone waiting. if !s.synced { s.syncedAt = s.backend.now() } @@ -600,14 +503,11 @@ func (s *sharedScope) pump(ctx context.Context) { } } } - // Routine bookmarks are absorbed here and never fanned out. They carry no object a - // consumer wants, and the boundary each subscriber sees is the one WE synthesize. + // Routine bookmarks are absorbed; each subscriber's boundary is synthesized above. case WatchAdded, WatchModified: - // The same partial-object guard the stream loop applies (spec §2) — and it MUST be applied - // here too, one layer lower, because this cache is REPLAYED to every future joiner. A husk - // forwarded once blanks one consumer's object; a husk CACHED is a husk served to everyone - // who arrives later, for as long as the scope lives. + // The stream loop's partial-object guard (spec §2), applied here too because the cache is + // replayed to every future joiner. if reason := partialReason(ev.Object); reason != "" { s.mu.Unlock() s.die(ResyncRequired(reason)) @@ -621,8 +521,7 @@ func (s *sharedScope) pump(ctx context.Context) { case WatchDeleted: id := identityOf(ev.Object) if id == nil { - // A degenerate tombstone: we cannot know WHICH object left. Guessing would evict the - // wrong one from a cache that is then served to everybody. + // A degenerate tombstone: guessing which object left could evict the wrong one. s.mu.Unlock() s.die(ResyncRequired("deletion tombstone carried no trustworthy uid")) return @@ -644,8 +543,7 @@ func (s *sharedScope) pump(ctx context.Context) { func (s *sharedScope) fanOutLocked(ev WatchEvent) { for sub := range s.subs { if sub.awaiting { - // It has not been handed the cache yet, and the cache already contains this event's - // effect. Sending it as well would deliver the object twice. + // Its pending snapshot already contains this event. continue } sub.offer(ev) @@ -677,10 +575,8 @@ func (s *sharedScope) die(cause error) { s.backend.forget(s.key, s, end) s.cancel() - // Whatever ended the upstream — a clean close, a 410, a cancelled context — means one thing to a - // subscriber: continuity is lost, start a new cycle. Say it as a non-terminal RESYNC_REQUIRED so - // the stream loop announces it and resnapshots, rather than tearing the browser's connection down. - // A typed error is passed on as it is, with its code, terminal flag, message and hint. + // An untyped end is lost continuity: report it as a non-terminal RESYNC_REQUIRED so the stream + // loop resnapshots on the same connection. A typed error is passed on unchanged. if closed { cause = ResyncRequired("the shared upstream watch ended; a new snapshot cycle follows") } @@ -689,17 +585,15 @@ func (s *sharedScope) die(cause error) { defer s.mu.Unlock() s.dead = true for sub := range s.subs { - // The event first (best-effort: a full queue means this consumer is already being resynced for - // having fallen behind), then the close, which carries the reason regardless. + // The event is best-effort (a full queue is already resyncing); the close carries the reason. sub.offer(WatchEvent{Type: WatchError, Err: cause}) sub.end(cause) } s.subs = map[*subscriber]struct{}{} } -// leave removes one subscriber, and tears the whole scope down when the last one goes. Nobody is -// watching, so nothing should be watched: a shared watch that outlives its audience is a leak with -// a cache attached. +// leave removes one subscriber, and tears the scope and its upstream watch down when the last one +// goes. func (s *sharedScope) leave(sub *subscriber) { s.mu.Lock() delete(s.subs, sub) @@ -725,8 +619,7 @@ type sharedWatcher struct { once sync.Once } -// nextPending pops one snapshot event, if any remain. `pending` is written under the scope's lock, so -// it is read under it too — the critical section is a slice index, and it is not held across a block. +// nextPending pops one snapshot event, if any remain, under the scope's lock that guards `pending`. func (w *sharedWatcher) nextPending() (WatchEvent, bool) { w.scope.mu.Lock() defer w.scope.mu.Unlock() @@ -741,8 +634,7 @@ func (w *sharedWatcher) nextPending() (WatchEvent, bool) { func (w *sharedWatcher) Next(ctx context.Context) (WatchEvent, error) { for { - // The snapshot first, to exhaustion, before a single live event. It is the consumer's starting - // state, and it is not allowed to be dropped, truncated or interleaved. + // The whole snapshot first, before any live event. if ev, ok := w.nextPending(); ok { return ev, nil } @@ -754,9 +646,7 @@ func (w *sharedWatcher) Next(ctx context.Context) (WatchEvent, error) { continue // the snapshot landed in `pending`; go read it case ev, ok := <-w.sub.ch: if !ok { - // Drained, and closed. `reason` was written before the close, so observing the close - // guarantees we see it — and it is what turns "your stream restarted" into "your stream - // restarted BECAUSE you fell behind", which is the only version anyone can act on. + // Drained and closed. `reason` was written before the close, so it is visible here. if w.sub.reason != nil { return WatchEvent{}, w.sub.reason } @@ -771,13 +661,9 @@ func (w *sharedWatcher) Stop() { w.once.Do(func() { w.scope.leave(w.sub) }) } -// scopeKey is the identity of a shared watch: two consumers share an upstream exactly when they are -// asking the same question of the same cluster. -// -// Every field participates, including the label selector — a selector changes WHICH objects are in -// the snapshot, so two selectors are two scopes, and merging them would hand a consumer objects it -// did not ask for. `\x1f` (unit separator) joins them: it cannot occur in any of these values, so no -// combination of legal fields can be made to collide with another. +// scopeKey is the identity of a shared watch. Every field participates, including the label +// selector, which changes which objects are in the snapshot. `\x1f` (unit separator) cannot occur in +// any field, so no two scopes collide. func scopeKey(s Scope) string { return strings.Join([]string{ s.Target, s.Group, s.Version, s.Resource, s.Namespace, s.Name, s.LabelSelector, From a482938a80360cc1ad37de1e97bd5f8a968b926c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 13:58:03 +0000 Subject: [PATCH 7/9] docs: mark completed proposals 0007 and 0008 as historical Both still read as active instructions. Their status now says they are historical implementation plans, not adopter guidance, says what "current" referred to in 0007, and links the guides that are current. Co-Authored-By: Claude Opus 5.5 --- docs/proposals/0007-shared-stream-host-integration.md | 9 +++++++-- docs/proposals/0008-shared-watch-hardening.md | 8 +++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/proposals/0007-shared-stream-host-integration.md b/docs/proposals/0007-shared-stream-host-integration.md index 4ebc3b5..a3ec480 100644 --- a/docs/proposals/0007-shared-stream-host-integration.md +++ b/docs/proposals/0007-shared-stream-host-integration.md @@ -1,7 +1,12 @@ # Proposal 0007: Shared-stream transport correctness -**Status: implemented; unreleased.** The transport, observation and example increments below have -landed. The manual real-cluster scenario has been run at 2 and 200 identities and recorded in +**Status: historical implementation plan; implemented and released in 0.4.0.** It is kept as the +record of what was decided and why. Its instructions are not adopter guidance, and "current" below +describes the code at the time (`e450334`), not today's. For current guidance see +[authorization](../auth.md), [operations](../operations.md#bounded-http-delivery) and the +[shared ConfigMap host](../../gateway/kube/examples/sharedstream/README.md). + +The transport, observation and example increments below have landed. The manual real-cluster scenario has been run at 2 and 200 identities and recorded in [docs/facts/shared-host-rehearsal.md](../facts/shared-host-rehearsal.md); 200 subscribers cost exactly one additional API-server watch. That is one run on one disposable cluster, without a proxy, browsers or resource limits, and is not a capacity guarantee. diff --git a/docs/proposals/0008-shared-watch-hardening.md b/docs/proposals/0008-shared-watch-hardening.md index ecf5a4a..80f5997 100644 --- a/docs/proposals/0008-shared-watch-hardening.md +++ b/docs/proposals/0008-shared-watch-hardening.md @@ -1,6 +1,12 @@ # Proposal 0008 Shared watch hardening implementation plan -**Status: selected scope implemented; asks 11 and 12 deferred.** +**Status: historical implementation plan; selected scope implemented, asks 11 and 12 deferred.** +It is kept as the record of what was decided and why; its instructions were for the implementer and +are not adopter guidance. For current guidance see +[session validity and timed checks](../auth.md#session-validity-and-timed-checks), the +[revocation budget](../auth.md#revocation-budget), +[what the SubjectAccessReview asks](../auth.md#what-the-subjectaccessreview-asks) and +[shared backends](../../gateway/README.md#shared-backends). This is the implementer handoff for the [second krm-foyer field report](../field-reports/second-fieldreport.md). Implement ask 9, the explicit HTTP configuration validation option from ask 10, and the From 6d78211f2ae190e87ac0c5012f41d11d07020b2a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 13:58:03 +0000 Subject: [PATCH 8/9] docs: make adoption the single starting point and tighten the guides Each user-facing guide now gives the recommended action, the constraints needed to use it safely, and links to deeper reference. - adopting: lead with one complete host and browser recipe, with an explicit WriteTimeout and a session-expiry deadline; timed reauthorization and sharing are explicit opt-ins whose authorizer checks current session validity; error inspection, AnyResource, path prefixes, bearer tokens and keyed lists move to a reference section after it. - auth: the revocation table no longer claims one WriteTimeout bounds gate waiting (it bounds individual writes); drop the simplified arithmetic for one instruction to measure; remove wiring duplicated from adopting and the rehearsal details. - saving: one sequence (capture, conditional PATCH, 204 and watch echo, guarded GET on 409), response-object alternatives last, planned work out of the user flow, and "What the guard rejects" renamed to "What ValidateMergePatch rejects". - gateway README: a package overview, a minimal example and guide links. - operations: controls, alerts and troubleshooting, including the kube error mapping that previously lived only in the gateway README. - releasing: lead with the release steps; the 0.6.0 incident moves into the recovery section. Inbound anchors are preserved or updated. Co-Authored-By: Claude Opus 5.5 --- docs/adopting.md | 267 +++++++++--------- docs/auth.md | 211 ++++++-------- docs/operations.md | 114 ++++---- ...006-stream-and-save-implementation-plan.md | 2 +- docs/releasing.md | 147 +++++----- docs/saving.md | 218 +++++++------- examples/README.md | 6 +- gateway/README.md | 136 +++------ 8 files changed, 487 insertions(+), 614 deletions(-) diff --git a/docs/adopting.md b/docs/adopting.md index 3c7df45..615fe29 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -1,90 +1,21 @@ # Adopting krm-stream -This is the shortest safe route from an existing Go application to a browser KRM stream. The library -owns the read path. Your application owns identity, authorization policy, Kubernetes credentials, and -writes. +This is the recommended route from an existing Go application to a live KRM view in the browser. The +library owns the read path. Your application owns identity, authorization policy, Kubernetes +credentials and writes. -## 1. Declare scopes precisely +## 1. Mount the stream endpoint -An empty namespace has two Kubernetes meanings: a cluster-scoped resource, or every namespace for a -namespaced resource. `ScopePolicy` makes the host choose explicitly. - -| Resource kind | `GroupResource` | Caller namespace | Result | -|---|---|---|---| -| one namespace of ConfigMaps | `Scope: ResourceScopeNamespaced` | `app` | watches only `app` | -| all namespaces of ConfigMaps | `Scope: ResourceScopeNamespaced, AllowAllNamespaces: true` | empty | watches all namespaces | -| Namespaces | `Scope: ResourceScopeCluster` | empty | watches cluster-scoped objects | -| Namespaces with `namespace=app` | `Scope: ResourceScopeCluster` | `app` | refused | - -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. - -### Delegating resource admission to Kubernetes - -A host whose endpoint should expose whatever Kubernetes lets each user read, as a `/k8s` proxy does, -has no resource list to keep. `AnyResource` says so explicitly: - -```go -Scopes: gateway.ScopePolicy{ - Targets: []string{""}, - AnyResource: true, // admission is left to the upstream's own authorization -}, -``` - -Any resource the scope syntax accepts then passes, with or without a namespace. Without one, the API -server decides: the list of a cluster-scoped resource, or every namespace for a namespaced one, which -RBAC allows only with a cluster-wide grant. The target allowlist, `AllowLabelSelector`, the refusal of -API-server addresses and the `Authorizer` all still apply. - -`AnyResource` is safe only when `Clients` returns a backend built from the caller's own credential. -The library cannot check that: a `kube.Backend` can carry a service account's client, and a wrapper -can hide a `SharedBackend`. With either, every resource that identity can read is open to everyone -your `Authorizer` admits. Prove the binding in the host's own tests. - -### 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) -} -``` - -### Errors on the wire, and the detail behind them - -A `StreamError` you return keeps its `Message`; its `Cause` never reaches the wire. Any other error -reaches the browser as `INTERNAL` with the message `internal error`. Set `Options.Diagnostics` to see -the raw error, with the principal and scope it belongs to, and log what your policy allows: - -```go -Diagnostics: func(d gateway.Diagnostic) { - slog.Warn("stream error", "code", d.Code, "resource", d.Scope.Resource, "err", redact(d.Err)) -}, -``` - -`Principal` may return a `*StreamError` too: `gateway.Unauthenticated` when signing in again will -help, or `gateway.UpstreamUnavailable` when your session store is down. Any other `Principal` error -is sent as `UNAUTHENTICATED` "not authenticated". - -### Targets with a path prefix - -A `rest.Config.Host` may include a path prefix, such as a kcp workspace URL -(`https://kcp.example/clusters/root:org:ws`). The dynamic client preserves it when constructing API -requests. Use `kube.NewBackendForConfig(cfg)` to include the endpoint in upstream errors, which reach -`Options.Diagnostics` and never the browser; -`kube.NewBackend(dynamicClient)` also works when a dynamic client already exists. Build that client on -`kube.HTTPClientFor(cfg)` (`dynamic.NewForConfigAndClient(cfg, httpClient)`): client-go follows -redirects and carries the configured credential along, so a client holding a user's token must refuse -them, as `NewBackendForConfig`'s does. - -## 2. Mount the same-origin cookie endpoint +Each browser stream acts as its caller: the backend is built from that user's credential, so +Kubernetes RBAC stays the boundary. The session cookie identifies the caller, and the request ends +when the session does. ```go import ( + "context" + "log/slog" "net/http" + "time" "k8s.io/client-go/rest" @@ -93,12 +24,12 @@ import ( ) func mount(mux *http.ServeMux, restConfigFor func(*User) *rest.Config) { - mux.Handle("/resource-stream/v1", gateway.Handler(gateway.Options{ + stream := gateway.Handler(gateway.Options{ // Your session cookie -> your application user. krm-stream never sees a token. Principal: func(r *http.Request) (gateway.Principal, error) { return userFromSession(r) }, - // Deny before opening a watch; also rechecked on every recovery cycle. + // Deny before opening a watch; rechecked on every snapshot cycle. Authorizer: gateway.AuthorizerFunc(func(ctx context.Context, p gateway.Principal, s gateway.Scope) error { user := p.(*User) if s.Target != user.Target || s.Namespace != user.Namespace { @@ -106,7 +37,7 @@ func mount(mux *http.ServeMux, restConfigFor func(*User) *rest.Config) { } return nil }), - // Build a backend acting as this user. Kubernetes RBAC remains the boundary. + // A backend acting as this user. Clients: func(_ context.Context, _ string, p gateway.Principal) (gateway.Backend, error) { return kube.NewBackendForConfig(restConfigFor(p.(*User))) }, @@ -117,23 +48,57 @@ func mount(mux *http.ServeMux, restConfigFor func(*User) *rest.Config) { {Group: "apps", Resource: "deployments", Scope: gateway.ResourceScopeNamespaced}, }, }, - Projection: gateway.ProjectionFull, - })) + Projection: gateway.ProjectionFull, + WriteTimeout: 10 * time.Second, // bound each write to a browser that stops reading + Diagnostics: func(d gateway.Diagnostic) { + slog.Warn("stream error", "code", d.Code, "resource", d.Scope.Resource, "err", redact(d.Err)) + }, + }) + mux.Handle("/resource-stream/v1", endWithSession(stream)) +} + +// endWithSession ends a stream when the caller's session expires. +func endWithSession(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if expiry, ok := sessionExpiry(r); ok { + ctx, cancel := context.WithDeadline(r.Context(), expiry) + defer cancel() + r = r.WithContext(ctx) + } + next.ServeHTTP(w, r) + }) } ``` -The browser uses `connectManagedResourceStream` for this route. Fetch sends its same-origin session -cookie and the managed connection provides bounded recovery after network failures and sequence gaps. +`WriteTimeout` needs a response writer that supports flushing and write deadlines. Check your mounted +middleware with the +[capability test](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test); +an unsupported writer aborts the request before streaming. The +[shared-host example](../gateway/kube/examples/sharedstream/handler.go) shows a complete session +resolver that uses the earlier of session and token expiry. + +## 2. Declare scopes precisely + +An empty namespace has two Kubernetes meanings: a cluster-scoped resource, or every namespace of a +namespaced resource. `ScopePolicy` makes the host choose explicitly. + +| Resource kind | `GroupResource` | Caller namespace | Result | +|---|---|---|---| +| one namespace of ConfigMaps | `Scope: ResourceScopeNamespaced` | `app` | watches only `app` | +| all namespaces of ConfigMaps | `Scope: ResourceScopeNamespaced, AllowAllNamespaces: true` | empty | watches all namespaces | +| Namespaces | `Scope: ResourceScopeCluster` | empty | watches cluster-scoped objects | +| Namespaces with `namespace=app` | `Scope: ResourceScopeCluster` | `app` | refused | + +Keep all-namespaces access rare: it changes the size, disclosure risk and operating cost of a stream. +Use the `Authorizer` to pin a user to a namespace or target before any watch opens. -## 3. Browser client +## 3. Connect the browser ```ts import { - defaultPolicy, LiveResourceStore, connectManagedResourceStream, resourceStreamURL, - withOpenAPIKeyedLists, } from "@configbutler/krm-stream"; const store = new LiveResourceStore(); @@ -151,67 +116,111 @@ const connection = connectManagedResourceStream(url, store, { store.subscribe(() => render(store)); ``` -For a bearer-token client, use `connectManagedResourceStream(url, store, { headers: { Authorization: ... } })`. -That is useful for a non-browser client or an intentionally token-bearing browser application; the -same-origin cookie route is the safer browser default. The host must enforce HTTPS for bearer-token -requests, including the resolved destination of relative URLs and any redirects. Validate the -trusted endpoint before supplying credentials and enforce the same policy in a custom fetch wrapper. -The stream connectors delegate transport to fetch; they do not enforce a credential transport policy. +Fetch sends the same-origin session cookie, and the managed connection recovers on its own after +network failures and sequence gaps. -For Deployment or CRD editing, a host may opt into OpenAPI-declared associative-list merging without -exposing schemas to the browser: +Saving is the host's: see [saving](saving.md). A quiet stream can still hold an older write version; +see [why a quiet stream can reject a save](saving.md#why-a-quiet-stream-can-still-reject-a-save). -```ts -const store = new LiveResourceStore(withOpenAPIKeyedLists(defaultPolicy, deploymentSchema)); -``` +## 4. Optional: recheck quiet streams on a timer -`deploymentSchema` is the structural schema for that exact GroupVersionKind. Only lists marked -`x-kubernetes-list-type: map` with `x-kubernetes-list-map-keys` are merged by key; every other list -stays safely atomic. +Every snapshot cycle reauthorizes, but a quiet stream can go a long time without one. To bound how +long a revoked caller keeps it, opt into timed checks, and make the `Authorizer` check that the +caller's session is still valid, since the principal is the one captured when the stream opened: + +```go +options.ReauthorizationInterval = 30 * time.Second +options.ReauthorizationTimeout = 5 * time.Second +options.WriteTimeout = 10 * time.Second // required with timed checks; Handler panics without it +``` -A quiet stream can still hold an older write version. See -[why a quiet stream can reject a save](saving.md#why-a-quiet-stream-can-still-reject-a-save) -for the illustrated flow and the host's recovery responsibilities. +Plan with the [revocation budget](auth.md#revocation-budget). -## 4. Share watches only with Kubernetes-backed authorization +## 5. Optional: share watches, with Kubernetes-backed authorization -`SharedBackend` saves upstream watches but runs as one service identity. Pair it with -`kube.SubjectAccessReviewAuthorizer` so Kubernetes still decides whether each caller may list and watch the scope. +`SharedBackend` opens one upstream watch per scope for every caller, as one service identity. Your +`Authorizer` is then the only check between a caller and the cached objects, so pair it with +`kube.SubjectAccessReviewAuthorizer`, which asks Kubernetes whether each caller may list and watch +the scope: ```go shared := gateway.NewSharedBackendWithOptions(serviceAccountBackend, gateway.SharedOptions{ QueueDepth: 512, - Observer: metrics, + Observer: metrics, }) options.Authorizer = kube.SubjectAccessReviewAuthorizer(clientset, subjectFromUser) options.Clients = func(context.Context, string, gateway.Principal) (gateway.Backend, error) { return shared, nil } ``` -Rechecking on a timer bounds how long a revoked caller keeps a quiet stream. Over HTTP it needs a -write bound too, because a check waits for the write in progress; `Handler` refuses the interval -without one: +Read [shared-watch authorization](auth.md#shared-watch-authorization) first. The +[shared ConfigMap host](../gateway/kube/examples/sharedstream/README.md) is a tested composition with +session expiry, timed checks and a write timeout. + +Next: [saving](saving.md) for the host-owned write path and [operations](operations.md) for +monitoring and limits. + +## Reference + +### Errors and diagnostics + +A `StreamError` you return keeps its `Message`; its `Cause` never reaches the wire. Any other error +reaches the browser as `INTERNAL` with the message `internal error`. `Options.Diagnostics` receives +the raw error with its principal and scope; log what your policy allows. + +`Principal` may return `gateway.Unauthenticated` when signing in again will help, or +`gateway.UpstreamUnavailable` when your session store is down. Any other `Principal` error is sent as +`UNAUTHENTICATED` "not authenticated". + +`ScopeFromQuery` and `ScopePolicy.Validate` return `error` whose concrete value is a `*StreamError`; +reach its code with `errors.As`. + +### Delegating resource admission to Kubernetes + +An endpoint that should expose whatever Kubernetes lets each user read, as a `/k8s` proxy does, can +replace the resource list with `AnyResource`: ```go -options.ReauthorizationInterval = 30 * time.Second -options.ReauthorizationTimeout = 5 * time.Second -options.WriteTimeout = 10 * time.Second +Scopes: gateway.ScopePolicy{ + Targets: []string{""}, + AnyResource: true, // admission is left to the upstream's own authorization +}, ``` -Test that your mounted middleware supports flushing and write deadlines, and plan with the -[revocation budget](auth.md#revocation-budget), which also lists what these settings do not bound. +Any resource the scope syntax accepts then passes, with or without a namespace, and the API server +decides. The target allowlist, `AllowLabelSelector`, the refusal of API-server addresses and the +`Authorizer` still apply. -Read [auth.md](auth.md) before using this configuration. The shared-cache authorizer is a security -boundary; a per-user backend keeps Kubernetes RBAC as the direct boundary by construction. +`AnyResource` is safe only when `Clients` returns a backend built from the caller's own credential. +The library cannot check that: with a service account's client or a hidden `SharedBackend`, every +resource that identity can read is open to everyone your `Authorizer` admits. Prove the binding in +your own tests. -Next: [saving.md](saving.md) for the host-owned write path and [operations.md](operations.md) for -stream monitoring and limits. +### Targets with a path prefix -## Tested shared host and middleware +A `rest.Config.Host` may include a path prefix, such as a kcp workspace URL +(`https://kcp.example/clusters/root:org:ws`); the dynamic client preserves it. +`kube.NewBackendForConfig(cfg)` includes the endpoint in upstream errors, which reach +`Options.Diagnostics` and never the browser. To use `kube.NewBackend(dynamicClient)` instead, build +the client on `kube.HTTPClientFor(cfg)` (`dynamic.NewForConfigAndClient(cfg, httpClient)`), which +refuses redirects so a user's token is not carried along. + +### Bearer-token clients + +`connectManagedResourceStream(url, store, { headers: { Authorization: ... } })` suits a non-browser +client or an intentionally token-bearing browser application; the same-origin cookie route is the +safer browser default. The connectors delegate transport to fetch and enforce no credential policy, +so the host must require HTTPS for the resolved destination, including redirects, before supplying +credentials. -Use the [compiled shared ConfigMap host](../gateway/kube/examples/sharedstream/README.md) to compose -participant identity resolution, service-account sharing, per-subscriber authorization, session -expiry and `WriteTimeout`. Its [middleware capability recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test) -checks the actual wrapped server writer in a host test. `CheckHTTPStreaming` must run before -streaming: it emits no response, but clears the existing write deadline. A positive timeout requires -flush and deadline support; unsupported transports abort and can cause browser reconnects. +### Keyed lists for Deployment and CRD editing + +A host may opt into OpenAPI-declared associative-list merging without exposing schemas to the browser: + +```ts +const store = new LiveResourceStore(withOpenAPIKeyedLists(defaultPolicy, deploymentSchema)); +``` + +`deploymentSchema` is the structural schema for that exact GroupVersionKind. Only lists marked +`x-kubernetes-list-type: map` with `x-kubernetes-list-map-keys` are merged by key; every other list +stays atomic. diff --git a/docs/auth.md b/docs/auth.md index b1150f0..e2ef334 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -8,11 +8,14 @@ host's scope and projection decisions. How Kubernetes checks a caller depends on | Per-user | The caller's token or an impersonated user | Resolve the caller, authorize the scope and supply their client. | | Shared | One service identity | Authorize every subscriber before serving cached objects; use `kube.SubjectAccessReviewAuthorizer` for Kubernetes RBAC decisions. | +Prefer a per-user backend: Kubernetes RBAC is then the boundary by construction. The wiring for both +is in [adopting](adopting.md). + ## Browser sessions -Use a same-origin session cookie for browser applications. The host handles OIDC with its identity -provider, keeps the tokens server-side and issues a secure session cookie. The managed fetch connector -sends that cookie; native `EventSource` can use the same route. +Use a same-origin session cookie. The host handles OIDC with its identity provider, keeps the tokens +server-side and issues a secure session cookie. The managed fetch connector sends that cookie; native +`EventSource` can use the same route. ```mermaid sequenceDiagram @@ -35,48 +38,37 @@ sequenceDiagram S-->>B: Projected SSE stream or terminal error ``` -Native `EventSource` cannot send an `Authorization` header. Use `connectManagedResourceStream` with -explicit headers for an intentionally token-bearing client. The host must enforce trusted HTTPS -endpoints and redirect handling; the connector delegates those transport decisions to fetch. -See [adoption](adopting.md#3-browser-client). +A token-bearing client uses `connectManagedResourceStream` with explicit headers, because native +`EventSource` cannot send an `Authorization` header. The host must then enforce trusted HTTPS +endpoints and redirect handling; see [adoption](adopting.md#3-connect-the-browser). ## Host seams -```go -gateway.Handler(gateway.Options{ - Principal: sessionUser, - Authorizer: authorizeScope, - Clients: func(_ context.Context, target string, p gateway.Principal) (gateway.Backend, error) { - return kube.NewBackendForConfig(restConfigFor(target, p.(*User))) - }, - Scopes: scopePolicy, -}) -``` - - `Principal` resolves the request to an opaque application identity. Return a `*StreamError` to choose the refusal (`UNAUTHENTICATED`, `UPSTREAM_UNAVAILABLE`); any other error is sent as `UNAUTHENTICATED` without its text. -- `Authorizer` denies unauthorized scopes before a watch opens and on subsequent checks. -- `Clients` is a `ClientFor` callback supplying the backend for that identity and target. - `kube.NewBackendForConfig` builds a client that refuses redirects, since client-go would - otherwise carry the caller's token to wherever a redirect points. A host that builds its own - dynamic client for `kube.NewBackend` must build it on `kube.HTTPClientFor(cfg)` for the same - reason. +- `Authorizer` denies unauthorized scopes before a watch opens and on every later check. +- `Clients` supplies the backend for that identity and target. Build it with + `kube.NewBackendForConfig`, which refuses redirects: client-go would otherwise carry the caller's + token to wherever a redirect points. A dynamic client for `kube.NewBackend` must be built on + `kube.HTTPClientFor(cfg)` for the same reason. - `Scopes` allowlists targets and resources, or with `AnyResource` leaves resource admission to the - API server's RBAC for a backend that acts as the caller. A browser cannot supply a raw API-server - URL. + API server's RBAC for a backend that acts as the caller. A browser cannot supply an API-server URL. -A per-user backend can use the user's bearer token or Kubernetes impersonation. Impersonation -requires explicit host credentials with impersonation rights. Scope and disclosure policy remain -host-owned in either case; a projection does not grant permission to read or write a resource. -Redaction is an additional disclosure restriction for an authorized caller, never a substitute for -verifying that caller may read the resource. +A per-user backend can use the user's bearer token or Kubernetes impersonation; impersonation needs +host credentials with impersonation rights. A projection does not grant permission to read or write a +resource: redaction restricts disclosure to a caller who is already authorized. -## Long streams, short tokens +## Session validity and timed checks -The gateway rechecks authorization and projection policy on every snapshot cycle and calls `Clients` -again so the host can provide refreshing credentials. Cycle-only checks do not bound revocation time -on a quiet stream. Set a timed recheck when the host needs that bound, together with a write bound: +The gateway rechecks authorization and projection policy on every snapshot cycle, and calls `Clients` +again so the host can supply refreshed credentials. Checks use the principal captured when the stream +opened, so resolve current session and account validity inside the `Authorizer`. End the request when +the session or token expires: the [shared-host example](../gateway/kube/examples/sharedstream/handler.go) +puts the earlier of the two on the request context as a deadline. + +A quiet stream may go a long time without a new cycle. To bound how long a revoked caller keeps it, +recheck on a timer: ```go options.ReauthorizationInterval = 30 * time.Second // how often each subscriber is rechecked @@ -84,133 +76,90 @@ options.ReauthorizationTimeout = 5 * time.Second // budget for each periodic c options.WriteTimeout = 10 * time.Second // required over HTTP with timed checks ``` -Timed checks run per subscriber and share that subscriber's delivery gate: while a check runs, the -subscriber receives no objects, and a check waits for the delivery already in progress. Denial, -timeout, policy failure or a changed projection terminates only that stream; other subscribers -continue. A SubjectAccessReview that cannot reach the API server ends the stream with a non-terminal -`UPSTREAM_UNAVAILABLE`, so the client may reconnect once it is back. Zero interval keeps cycle-only -checks; zero timeout uses 10 seconds. - -Library-owned HTTP serving requires a positive `WriteTimeout` with a positive -`ReauthorizationInterval`. `Handler` panics at construction without one, and direct `ServeStream` and -`ServeStreamProjection` calls panic before writing anything. A write to a browser that has stopped -reading blocks once the buffers between fill, and without a deadline it would hold the gate, and the -revocation behind it, until something else ended the request. The mounted middleware must support -flushing and write deadlines; test it with the -[capability-check recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test). -Transport-neutral `Stream` and `StreamProjection` keep timed checks with any sink: there the host -owns the sink and must bound its I/O. +Each subscriber is checked on its own timer. Denial, timeout, policy failure or a changed projection +ends only that stream. A SubjectAccessReview that cannot reach the API server ends it with a +non-terminal `UPSTREAM_UNAVAILABLE`, so the client reconnects later. Zero interval keeps cycle-only +checks; zero timeout uses 10 seconds. Timed checks do not call `Clients`. -Checks use the principal captured at stream open. Resolve current session/account validity inside the -host authorizer. Timed checks do not invoke `Clients`; credential refresh remains per snapshot cycle -or inside the supplied client. +A timed check shares the subscriber's delivery gate: while it runs, that subscriber receives no +objects, and it waits for the delivery in progress. A write to a browser that has stopped reading +blocks once the buffers fill, so HTTP serving requires a positive `WriteTimeout` with timed checks. +`Handler` panics at construction without one, and `ServeStream` and `ServeStreamProjection` panic +before writing. Test that your mounted middleware supports flushing and write deadlines with the +[capability-check recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test). +`Stream` and `StreamProjection` allow timed checks with any sink; the host bounds that sink's I/O. -With 200 subscribers, a 30-second interval adds roughly 13 SubjectAccessReviews per second (list and -watch per subscriber), plus opening/cycle checks. Choose intervals for the host's revocation budget -and API-server capacity; checks are not cached across identities. +Each check costs the host's authorizer a call. With `SubjectAccessReviewAuthorizer`, 200 subscribers +on a 30-second interval add about 13 reviews per second (list and watch each), plus opening and +cycle checks. Choose the interval for both your revocation budget and API-server capacity. ### Revocation budget -The revocation budget is how long a stream keeps delivering after access is withdrawn. With timed -checks over HTTP it is made of these parts, in order: +The time from withdrawing access until a stream stops delivering is made of these parts: | Part | Bounded by | What it covers | |---|---|---| -| Decision freshness | the host | Time until the host's authorizer can see the change: session stores, identity-provider group sync, and any decision cache the host adds. The rest of the budget starts only then. | -| Timer | `ReauthorizationInterval` | Up to one interval until that subscriber's next check. Each subscriber has its own timer. | -| Gate wait | one `WriteTimeout` | The check waits for the delivery in progress, which completes or fails at its write deadline. | -| Check | `ReauthorizationTimeout` | Starts once the check holds the gate, and covers the `Authorizer` and projection-policy callbacks. | -| Termination and cleanup | one `WriteTimeout`, then the host | The refusal is written as the stream's terminal frame, a write with its own deadline. The request then returns, releasing its shared subscription. | - -Interval + check timeout + write timeout is a planning figure, not an unconditional bound. Scheduling -delay, a heartbeat or terminal write competing for the same response, callbacks that ignore -cancellation, host middleware and cleanup can all add time. Declare the total for your deployment, -then measure it under the intended load. +| Decision freshness | the host | Until the authorizer can see the change: session stores, group sync, any decision cache. | +| Timer | `ReauthorizationInterval` | Up to one interval until that subscriber's next check. | +| Gate wait | not bounded as a whole | Waits for current delivery; `WriteTimeout` bounds individual HTTP writes, not total gate waiting. | +| Check | `ReauthorizationTimeout` | Starts once the check holds the gate; covers the `Authorizer` and projection policy. | +| Termination | `WriteTimeout`, then the host | The refusal is written as the terminal frame; the request then returns and releases its subscription. | -Revocation ends three things at different times: +Measure the total revocation latency for your deployment under its intended load. -- **New object delivery** stops when the check holds the gate. A denied check releases the gate - only after ending the stream, so no further object is written. -- **The request** returns after the terminal frame is written or fails, and cleanup completes. -- **The browser** receives the refusal only if the transport delivers it. A reader that stopped - reading may never see it, and the request ends at the write deadline instead. +Revocation ends three things at different times. New object delivery stops when a denying check holds +the gate. The request returns once the terminal frame is written or fails. The browser sees the +refusal only if the transport delivers it; a reader that stopped reading may never see it. Bytes +already written to the socket or buffered by a proxy cannot be recalled. -Bytes already written to the socket, or buffered by a proxy, cannot be recalled. Revocation stops -further disclosure; it does not retract earlier disclosure. - -`ReauthorizationTimeout` covers periodic checks only. Opening and snapshot-cycle authorization run -under the request's context, so give host callbacks their own deadlines. A generic `Stream` sink -takes the place of `WriteTimeout` in the table, with whatever bound the host gives it. +`ReauthorizationTimeout` covers periodic checks only. Opening and snapshot-cycle checks run under the +request's context, so give host callbacks their own deadlines, but not a short deadline on the whole +healthy stream. ## Shared-watch authorization [`SharedBackend`](../gateway/shared.go) opens one upstream watch per scope as one service identity. -The host's `Authorizer` is then the only access check between a subscriber and the cached objects: -an overly permissive authorizer exposes the service identity's data to that subscriber. This is why -sharing is opt-in. Every subscriber must be authorized independently before receiving the cache: - -```go -shared := gateway.NewSharedBackend(serviceAccountBackend) -opts.Authorizer = kube.SubjectAccessReviewAuthorizer(clientset, subjectOf) -opts.Clients = func(context.Context, string, gateway.Principal) (gateway.Backend, error) { return shared, nil } -``` +The host's `Authorizer` is then the only check between a subscriber and the cached objects, which is +why sharing is opt-in. Authorize every subscriber with `kube.SubjectAccessReviewAuthorizer`, so +Kubernetes still decides whether that caller may list and watch the scope. -The [`SubjectAccessReviewAuthorizer`](../gateway/kube/authz.go) adapter delegates the decision to -Kubernetes. `subjectOf` supplies the API-server-resolved username, groups, UID and extras. The adapter checks -both `list` and `watch`. An incomplete review is refused, and an explicit `Denied` wins over -`Allowed`. +The adapter refuses an incomplete review, and an explicit `Denied` wins over `Allowed`. The service +account needs `create` on `subjectaccessreviews` (`system:auth-delegator`), not impersonation rights. +These are SubjectAccessReview requests, not SelfSubjectAccessReview requests. Opening, cycle and timed +checks use the same authorizer. ### What the SubjectAccessReview asks -A host that caches decisions has to key them on exactly what the adapter asks. For each check it -sends one SubjectAccessReview per verb, `list` then `watch`, and stops at the first refusal or -failure: +For each check the adapter sends one review per verb, `list` then `watch`, and stops at the first +refusal or failure: | Field | Value | |---|---| | `user`, `groups`, `uid`, `extra` | The `kube.Subject` returned by the host's `SubjectFor`, unchanged. | | `resourceAttributes.verb` | `list`, then `watch`. | -| `resourceAttributes.group`, `.version`, `.resource` | The scope's group, version and resource. | -| `resourceAttributes.namespace` | The scope's namespace; empty for an all-namespaces scope. | -| `resourceAttributes.name` | The scope's name; empty for a collection. | +| `resourceAttributes.group`, `.version`, `.resource` | Copied from the scope. | +| `resourceAttributes.namespace`, `.name` | Copied from the scope. | -Not sent: the scope's label selector, any field selector, and a subresource. A selector narrows what -a stream delivers, but it does not narrow the authorization question: the review asks about the -whole collection. The scope's target and the stream's projection are not review attributes either; -the clientset given to the adapter decides which API server receives the review. This describes the -adapter's request, not how every Kubernetes authorizer evaluates it. +Selectors are omitted from the authorization request: the label selector, any field selector, and +the subresource are not sent, so a selector does not narrow the question. The scope's target and the +stream's projection are not review attributes; the clientset given to the adapter decides which API +server receives the review. This describes the adapter's request, not how every Kubernetes authorizer +evaluates it. -A cache in front of the adapter must therefore: +A host that caches decisions must: -- key on the complete subject (user, groups, UID and extra) and the complete request, including - the verb; -- keep decisions from different API servers or targets apart, since the same request can have a - different answer elsewhere; -- never let a reused review skip the host's own checks of session validity or projection policy. +- key on the complete subject (user, groups, UID and extra) and the complete request, including the + verb; +- keep decisions from different API servers or targets apart; +- never let a reused review skip its own checks of session validity or projection policy. Changing these inputs is a compatibility change for caching hosts, and is announced in the release notes. The library does not provide a decision cache. -The service account needs `create` on `subjectaccessreviews`; `system:auth-delegator` supplies that -permission. Reviews do not require impersonation rights. These are SubjectAccessReview requests, -not SelfSubjectAccessReview requests. Cycle and timed checks use the same authorizer. +The [shared ConfigMap host](../gateway/kube/examples/sharedstream/README.md) is a tested composition +of participant identity resolution, service-account reviews, session expiry and bounded HTTP +delivery. Its identity helper is an example, not a library API. ## Save boundary -The host owns writes, CSRF protection, audit and write authorization. Before a merge PATCH, call -`gateway.ValidateMergePatch` with the effective projection and current object, and include the -captured UID and resourceVersion preconditions. Project any resource returned to the browser. -See [saving](saving.md) for the complete flow. - -## Tested shared-host composition - -The [shared ConfigMap host](../gateway/kube/examples/sharedstream/README.md) demonstrates a local -SelfSubjectReview helper using participant credentials, service-account SARs and data access, -fixed scope, session/token expiry and bounded HTTP delivery. Identity resolution is an example, -not a public library authentication API. It does not re-resolve identity on every timed check. - -Do not put a short callback deadline on the entire healthy stream. Write bounds limit in-flight -HTTP I/O, not backend operations or callback work; see the [revocation budget](#revocation-budget). -For 200 allowed participants, opening can issue 400 SARs plus 200 SSRs. Timers can align, recovery -adds checks, and client-side throttling consumes callback budgets. Neither the example's rate settings -nor Voter's reported rehearsal results are production defaults or supported-version evidence. +The host owns writes, CSRF protection, audit and write authorization. See [saving](saving.md). diff --git a/docs/operations.md b/docs/operations.md index 56b7457..10e1617 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -1,7 +1,7 @@ # Operating krm-stream -Long-lived streams need a small runbook. The gateway exposes low-cardinality `Observer` callbacks so -hosts can increment Prometheus counters or emit structured logs without inspecting resources. +The gateway reports low-cardinality lifecycle signals through `Observer`, so hosts can count them +without inspecting resources: ```go metrics := gateway.ObserverFunc(func(o gateway.Observation) { @@ -10,11 +10,10 @@ metrics := gateway.ObserverFunc(func(o gateway.Observation) { ``` Do not block in `Observe`; it runs on the stream or shared-watch goroutine. Never label a metric with -object name, UID, principal, patch contents, or an error message. +an object name, UID, principal, patch contents, scope value or error message. -Error text has its own hook, `Options.Diagnostics`, because it can name internal addresses and URLs. -The browser receives only the protocol code and a message chosen for it; an unexpected error is sent -as `INTERNAL` "internal error". Decide what to log there, and redact it. +Error text goes to `Options.Diagnostics` instead, because it can name internal addresses and URLs; the +browser receives only the protocol code and a message chosen for it. Redact before logging. ## Signals to alert on @@ -29,29 +28,28 @@ as `INTERNAL` "internal error". Decide what to log there, and redact it. | stream count / snapshot duration | connection pressure or oversized scopes | narrow namespaces/selectors; avoid accidental all-namespaces watches | | unorderable `resourceVersion` terminal errors | an unsupported or aggregated API does not meet strict ordering | use `OrderingLenient` only after accepting the reduced monotonicity guarantee | -## When the API server pushes back - -client-go retries on its own before the gateway sees an error. A 429 from Priority and Fairness, or -any 5xx, that carries a `Retry-After` header is retried up to ten times at the pace that header sets. -Only when client-go gives up does the gateway map the answer, to `UPSTREAM_UNAVAILABLE` with -`retryAfterMs`, and close the connection for the browser's client to retry. - -While client-go waits, the browser holds an open connection with heartbeats and no snapshot yet: the -managed connector reports `syncing`, not an error. krm-stream keeps client-go's default on purpose. -The API server sets the pace, so it is not a tight loop, and the wait happens once per stream at the -API server's request, where a reconnect would cost every browser a new connection and a fresh -snapshot. The cost is that the browser is not told why it is waiting. A rising `syncing` time, -without errors, is the signal to look at API-server load. - -A shared scope that is slow to open holds up only the streams waiting for that scope. Each of them -can still end with its request, and when the last one leaves, the opening is cancelled rather than -left holding a slot. A client-go watch has no response-header timeout of its own, so a stalled -opening otherwise waits until every caller gives up. - -A watch that ends before it is of use (before its snapshot completes, or within a second of it) is -recovered once on the same connection. A second such end in a row closes the connection with -`UPSTREAM_UNAVAILABLE`, so an upstream that ends every watch early meets the client's backoff and not -a reconnect loop. It shows as `retryable_error`. +## Troubleshooting upstream errors + +`gateway/kube` maps API-server answers to protocol codes: + +| API server | Browser receives | +|---|---| +| 403 | `FORBIDDEN`, with Kubernetes' message | +| 401 | `UNAUTHENTICATED` | +| 404 | `SCOPE_INVALID` | +| 429, 5xx, unreachable | `UPSTREAM_UNAVAILABLE`, with `Retry-After` as `retryAfterMs`; the connection closes and the client retries | + +- **Streams stuck in `syncing` without errors.** client-go retries a 429 or 5xx that carries + `Retry-After` up to ten times before the gateway sees it, and the browser waits with heartbeats + meanwhile. Look at API-server load. +- **`retryable_error` from watches that end early.** A watch that ends before its snapshot completes, + or within a second of it, is recovered once on the same connection; a second in a row closes the + connection with `UPSTREAM_UNAVAILABLE`. Look for an upstream that ends every watch. +- **One shared scope slow to open.** It holds up only the streams waiting for that scope, and each + ends with its request. A client-go watch has no response-header timeout of its own, so look at that + scope's API server. +- **A shared scope refused with `retryAfterMs` while others work.** `SharedBackend` backs off a scope + whose upstream will not open, so its subscribers' reconnects do not stampede. ## Runtime controls @@ -67,51 +65,43 @@ a reconnect loop. It shows as `retryable_error`. | `Gateway.Ordering` | strict | keep strict on supported Kubernetes; use lenient only for known aggregated APIs | | `gateway.Options.Diagnostics` | nil (discarded) | receive the raw error behind each error event; redact before logging | -Snapshot object and byte limits remain a host-level scope policy concern. The gateway refuses to guess a -safe universal cap: object size, useful namespace size, and recovery behavior are product-specific. -Measure snapshot size and duration per allowed scope before opening broad all-namespaces endpoints. +The gateway sets no snapshot object or byte limit: what is safe depends on the product. Measure +snapshot size and duration per allowed scope before opening broad all-namespaces endpoints. ## Counting lifetimes `stream_opened`/`stream_closed` count `StreamProjection` entry/return, including authorization failure, but exclude HTTP identity/scope refusals before entry. `shared_subscription_opened` and `shared_subscription_closed` count active attachments, including warm-cache joins. Overflow, scope -death or leave ends an attachment once; repeated `Stop` does not count again. Cleanup may finish -later. A resnapshot can replace an attachment on one logical stream without another API watch. +death or leave ends an attachment once; repeated `Stop` does not count again. A resnapshot can +replace an attachment on one logical stream without another API watch. Callbacks are synchronous and may run concurrently, including under shared locks. Return promptly; -do not panic or reenter the gateway. Update gauge counters synchronously rather than through a lossy -exporter queue. Opens precede matching closes, with no global order across independent lifetimes. -Ignore unknown kinds. Scope values are not safe metric labels. See the tested -[counter mapping](../gateway/kube/examples/sharedstream/metrics.go). - -These gauges do not count HTTP requests still resolving identity or upstream watch handles. Keep -host metrics for those needs. Measure physical API-server WATCH requests independently with the -metrics available on the tested Kubernetes version; the shared-host fixture uses +do not panic or reenter the gateway. Update gauges synchronously rather than through a lossy exporter +queue. Opens precede matching closes, with no global order across lifetimes. Ignore unknown kinds. +See the tested [counter mapping](../gateway/kube/examples/sharedstream/metrics.go). + +These gauges do not count HTTP requests still resolving identity, or upstream watch handles. Measure +physical API-server WATCH requests separately; the shared-host fixture uses `apiserver_longrunning_requests` filtered to the resource and `verb="WATCH"`. ## Bounded HTTP delivery -Set `WriteTimeout` explicitly; zero preserves no library-installed deadline and negative values -are rejected. A positive `ReauthorizationInterval` requires a positive `WriteTimeout`: `Handler` -panics at construction without one, and direct `ServeStream` calls panic before writing. A timed -check waits for the write in progress, so the write bound is part of the -[revocation budget](auth.md#revocation-budget). Each header, event or heartbeat write and flush shares one deadline. Successful -operations clear it, so quiet streams can outlive many timeout periods. A failed operation cancels -its stream and cannot be retried as a terminal frame. Flush success does not acknowledge browser -receipt. Generic `Stream` sinks remain responsible for bounded I/O. - -The transport owns write deadlines while serving and attempts to clear them at exit. It cannot -retrieve and restore a host's previous deadline. A whole-response `http.Server.WriteTimeout` is -not a per-operation substitute for a long-lived stream. Earlier request deadlines constrain I/O; -request cancellation may still wait for the current operation's bound. Backend opening/cleanup, -authorization gate waiting, encoding and host callbacks are outside that I/O budget. - -With a positive bound, unsupported writers abort before streaming and emit `http_transport_rejected`. -No unbounded diagnostic frame is attempted; browser reconnects are possible. Test the actual mounted -middleware using the [capability-check recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test). -HTTP/1.1 sockets and TLS HTTP/2 (including two streams on one connection) are tested, along with -transparent `Unwrap` and opaque wrappers. Other middleware/proxy combinations need host tests. +Set `WriteTimeout` explicitly. It is required with timed reauthorization: `Handler` panics at +construction without it, and `ServeStream` panics before writing. Each header, event or heartbeat +write and its flush share one deadline, cleared after success, so a quiet stream outlives many +timeout periods. A failed write ends its stream. Flush success does not acknowledge browser receipt. + +The library owns write deadlines while serving and clears them at exit; it cannot restore a host's +previous deadline. A whole-response `http.Server.WriteTimeout` is no substitute on a long-lived +stream. The bound covers I/O only, not backend opening, gate waiting, encoding or host callbacks. A +generic `Stream` sink is the host's to bound. + +With a positive bound, an unsupported writer aborts before streaming and emits +`http_transport_rejected`. Test your mounted middleware with the +[capability-check recipe](../gateway/kube/examples/sharedstream/README.md#middleware-capability-test). +HTTP/1.1, TLS HTTP/2 (including two streams on one connection), and transparent and opaque wrappers +are tested; other middleware and proxy combinations need host tests. For authorization bursts and the limits of the 200-subscriber profile, see the [shared-host capacity guide](../gateway/kube/examples/sharedstream/README.md#verification-and-capacity). diff --git a/docs/proposals/0006-stream-and-save-implementation-plan.md b/docs/proposals/0006-stream-and-save-implementation-plan.md index 5a988fc..53926ff 100644 --- a/docs/proposals/0006-stream-and-save-implementation-plan.md +++ b/docs/proposals/0006-stream-and-save-implementation-plan.md @@ -144,7 +144,7 @@ separately stall an authorization callback until its context expires. Pass only stream terminates within the declared closure limit, measured from revocation/session expiry or the start of the stalled check, respectively. Verify callbacks honor the configured check deadline and sinks have bounded completion times. Exercise quiet and active streams; cycle-only checks cannot -meet a bounded quiet-stream revocation target. See [authorization lifecycle](../auth.md#long-streams-short-tokens) +meet a bounded quiet-stream revocation target. See [authorization lifecycle](../auth.md#session-validity-and-timed-checks) for configuration and host responsibilities. Version-only events, independent content/delivery switches, downstream replay, write tickets, diff --git a/docs/releasing.md b/docs/releasing.md index 23235a7..0f4f529 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -1,22 +1,32 @@ # Releasing -If npm rejects an upload because the version already exists, the release job checks registry -metadata again for a bounded period. It succeeds only when that version's SHA-512 integrity matches -the validated tarball. Other publish failures, unavailable metadata and mismatched artifacts still -fail. Registry reads request cache revalidation. - -The 0.6.0 release demonstrated two distinct delays/failures: its first release attempt failed when -the Go checksum database returned `unknown revision gateway/v0.6.0` shortly after tagging. A rerun -later passed and published npm 0.6.0. A subsequent push attempted the same npm version and was -rejected as a duplicate. Do not bypass Go checksum verification or bump the version to recover these -cases; verify the public release state first. See the -[release run, including its attempts](https://github.com/ConfigButler/krm-stream/actions/runs/37061239061) -and the [duplicate upload failure](https://github.com/ConfigButler/krm-stream/actions/runs/37065350963). - Releases are generated from conventional commits on `main`. Release Please opens a release pull request with version bumps and changelog entries; merging that pull request creates the release. Nothing in it needs editing by hand. +## Release steps + +1. Merge pull requests to `main` with conventional commit titles (see below). +2. Before merging the release pull request, run: + + ```bash + task fixtures-check + task test + task lint + task build-client + ``` + + and confirm that it has passed CI and that npm trusted publishing is configured for + `@configbutler/krm-stream`. +3. Merge the release pull request. `release.yml` then: + 1. lets Release Please create the tags and select the release commit; + 2. validates and packs that commit once in CI, including a clean consumer of the public Go tag; + 3. publishes to npm the tarball produced by that CI run. + +If a step fails, see [recovering a failed release](#recovering-a-failed-release). + +## Commits and changelogs + The changelogs are written entirely from commit subjects and `BREAKING CHANGE:` footers. Do not add sections to a `CHANGELOG.md` yourself: Release Please never removes them, so an "Unreleased" section would ship labelled unreleased. CI refuses one. Put the detail in the commit body or the PR @@ -30,10 +40,14 @@ description, which the changelog links to. | `docs:`, `refactor:` | No version bump; included in the changelog | | `chore:`, `test:`, `ci:`, `build:`, `style:` | No version bump | +Before 1.0, remove superseded API names and forwarding packages instead of maintaining compatibility +shims. Record each removal and its replacement in release notes, and update repository callers, +examples and documentation together. Changes to wire semantics still require explicit spec and +conformance review. + ## Artifacts -The gateway core, Kubernetes adapter, and official npm client are released in lockstep. A release -produces these tags and packages: +The gateway core, Kubernetes adapter, and official npm client are released in lockstep: | Artifact | Published as | |---|---| @@ -48,74 +62,59 @@ request rewrites that line to the version being released (its `x-release-please- so a released adapter always resolves at least its own core. The matching `replace` in `go.work` moves with it: a release PR names a core version that is not tagged until it merges, and that replace is what lets the adapter build in it. `task release-rehearsal`, run by `task verify` and CI, builds the -adapter in exactly that state. Do not edit either line, and do not pin a -pseudo-version when an adapter change needs unreleased core API: `go.work` wires the two modules -together in this checkout, and CI's `a stranger can go get this` job builds an unreleased adapter -with the core from the same commit. Someone using an unreleased commit does the same: +adapter in exactly that state. Do not edit either line, and do not pin a pseudo-version when an +adapter change needs unreleased core API: `go.work` wires the two modules together in this checkout, +and CI's `a stranger can go get this` job builds an unreleased adapter with the core from the same +commit. Someone using an unreleased commit does the same: ```sh go get github.com/ConfigButler/krm-stream/gateway/kube@ github.com/ConfigButler/krm-stream/gateway@ ``` -Before 1.0, remove superseded API names and forwarding packages instead of maintaining compatibility -shims. Record each removal and its replacement in release notes, and update repository callers, -examples and documentation together. Changes to wire semantics still require explicit spec and -conformance review. - ## Publishing setup -`release.yml` follows one path: - -1. Release Please creates tags and selects the release commit. -2. CI validates and packs that commit once, including a clean consumer of the public Go tag. -3. npm publishes the tarball produced by that CI run. - -If there is no pending npm release, CI checks the triggering push and publishing is skipped. -Tags are created before release CI; npm publication waits for every CI job to pass. A failed CI -run can therefore leave GitHub/Go tags present without an npm package. Fix source problems in a -new release rather than moving public tags. - Configure npm trusted publishing for `ConfigButler/krm-stream`, workflow `release.yml`. No long-lived `NPM_TOKEN` is required. Configure only the maintained scoped package. -## Before merging a release PR - -```bash -task fixtures-check -task test -task lint -task build-client -``` - -Confirm that the release PR has passed CI and that the npm trusted-publisher configuration exists -for `@configbutler/krm-stream`. - -## Retry a failed release - -Use **Re-run failed jobs** on the original Actions run, or: - -```bash -gh run rerun RUN_ID --failed -``` - -If npm publication fails, this retries publishing with the **same tested tarball**. Successful -preparation and CI jobs are reused: no new commit, tag, build, or release version is needed. If -npm accepted the upload before the job failed, the retry detects the existing version and succeeds -without publishing again. A failed CI job must pass before publishing can start. - -Tarballs are retained for 30 days, matching [GitHub's job retry window](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs). -If the artifact was deleted or expired, an explicit recovery run rebuilds and validates once: - -```bash -gh workflow run release.yml --ref main -f version=0.3.0 -``` - -Recovery fails on registry errors and refuses to move npm's `latest` backwards. Do not delete or -move release tags, rename an older tarball, or remove the version checks to recover a failure. - -## Closely spaced merges +If there is no pending npm release, CI checks the triggering push and publishing is skipped. Tags are +created before release CI, and npm publication waits for every CI job to pass, so a failed CI run can +leave GitHub/Go tags present without an npm package. Release Please reads live `main`, so an older push can create a newer release commit's tags. -Preparation resolves all component tags to one immutable commit **before** CI starts. CI checks -out that commit and publishes its artifact, so the triggering push's older package cannot be -mistaken for the release. Publication does not depend on a tag being newly created in that run. +Preparation resolves all component tags to one immutable commit **before** CI starts. CI checks out +that commit and publishes its artifact, so the triggering push's older package cannot be mistaken for +the release. Publication does not depend on a tag being newly created in that run. + +## Recovering a failed release + +First verify the public release state: which tags exist, whether the Go module resolves, and which +npm version is published. Then: + +- **A job failed.** Use **Re-run failed jobs** on the original Actions run, or + `gh run rerun RUN_ID --failed`. Successful preparation and CI jobs are reused; no new commit, tag, + build or version is needed. A failed CI job must pass before publishing can start. +- **npm publication failed.** The rerun publishes the **same tested tarball**. If npm accepted the + upload before the job failed, npm rejects it as a duplicate; the job then rechecks registry + metadata for a bounded period, with cache revalidation, and succeeds only when that version's + SHA-512 integrity matches the validated tarball. Other publish failures, unavailable metadata and + mismatched artifacts still fail. +- **The Go checksum database does not know the new tag yet** (`unknown revision gateway/vX.Y.Z` + shortly after tagging). Wait and rerun; do not bypass checksum verification. +- **The tarball has expired.** Tarballs are kept for 30 days, matching + [GitHub's job retry window](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs). + An explicit recovery run rebuilds and validates once: + + ```bash + gh workflow run release.yml --ref main -f version=0.3.0 + ``` + + Recovery fails on registry errors and refuses to move npm's `latest` backwards. + +Fix source problems in a new release. Do not delete or move release tags, bump the version, rename an +older tarball, or remove the version checks to recover a failure. + +The 0.6.0 release hit both delays: its +[first attempt](https://github.com/ConfigButler/krm-stream/actions/runs/37061239061) failed on the +checksum database and passed on rerun, and a +[later push](https://github.com/ConfigButler/krm-stream/actions/runs/37065350963) was rejected as a +duplicate npm upload. diff --git a/docs/saving.md b/docs/saving.md index d148e79..39f9115 100644 --- a/docs/saving.md +++ b/docs/saving.md @@ -1,11 +1,17 @@ # Saving edits safely -krm-stream is a read library. Your application owns its HTTP save endpoint, audit policy, and -Kubernetes client. The client store produces a narrow RFC 7386 JSON merge patch; use -`gateway.ValidateMergePatch` immediately before sending it to Kubernetes. +krm-stream is a read library. Your application owns its HTTP save endpoint, audit policy and +Kubernetes client. The recommended sequence is: -Use the complete [conditional-save example](../examples/conditional-save/README.md): it includes a -compilable host endpoint, client reconciliation, race tests, and a real-cluster 409 test. +1. the browser captures a save intent; +2. the host validates it and sends a conditional Kubernetes merge PATCH; +3. the host answers 204, and the watch echoes the write back into the store; +4. on 409, the browser reconciles a guarded, projected GET, and the person saves again deliberately. + +The [conditional-save example](../examples/conditional-save/README.md) implements all of it: a +compilable host endpoint, client reconciliation, race tests and a real-cluster 409 test. + +## Capture the intent ```ts const intent = store.captureSave(uid); @@ -13,32 +19,23 @@ const intent = store.captureSave(uid); if (intent) await hostSave(intent); ``` -The host validates the patch and adds `metadata.uid` and `metadata.resourceVersion` from that intent -before sending a Kubernetes merge PATCH. It must not substitute the version from a newer GET. -Kubernetes performs the version check atomically with the write. Propagate a real Kubernetes 409 -as HTTP 409; do not turn all upstream errors into 502. +The patch is a narrow RFC 7386 JSON merge patch. Serialize saves per editor. -A narrow patch limits which fields are written; it does **not** protect against concurrent edits. -JSON merge patch replaces arrays in full, including arrays the client reconciles by keyed items. -See Kubernetes' [conditional update guidance](https://kubernetes.io/docs/reference/using-api/api-concepts/#updates-to-existing-resources). +## Send a conditional PATCH -On 409, keep the draft and reconcile a fresh, complete **projected** GET. Capture the response guard -before starting that GET: +Call `gateway.ValidateMergePatch` with the effective projection and the current object immediately +before sending the patch. Add `metadata.uid` and `metadata.resourceVersion` from the intent, never the +version from a newer GET: Kubernetes checks the version atomically with the write. Propagate a real +Kubernetes 409 as HTTP 409; do not turn all upstream errors into 502. -```ts -const reconcile = store.captureReconciliation(uid); -const { object, redactedPaths } = await hostRead(); -reconcile(object, { redactedPaths }); -// false can also mean snapshot recovery or unknown redaction paths. Do not force it. -``` +A narrow patch limits which fields are written; it does **not** protect against concurrent edits — +the version precondition does. JSON merge patch replaces arrays in full, including arrays the client +reconciles by keyed items. See Kubernetes' +[conditional update guidance](https://kubernetes.io/docs/reference/using-api/api-concepts/#updates-to-existing-resources). +Do not use whole-object `PUT`: projected objects are intentionally incomplete, and a `PUT` can delete +fields the browser never saw. -Local edits made during the request survive reconciliation. Render the current draft and conflicts; -let the user review before capturing another save intent. Serialize saves per editor. A host GET must -be a most-recent Kubernetes read with no intermediary cache and the same projection as the stream. -Do not apply a response for a replacement UID to the old editor. Deleted-object drafts are removed by -the store; archive them outside the store if the application needs recovery after deletion. - -## What the guard rejects +### What ValidateMergePatch rejects - a value declared in `redacted`, including deletion of a parent map such as `data: null`; - `metadata.managedFields` and the last-applied-configuration annotation, which every projection @@ -46,9 +43,42 @@ the store; archive them outside the store if the application needs recovery afte - `status` under `krm-spec/v1`; - a non-object or malformed JSON merge patch. -It does not grant write permission, choose a projection, fetch the object, issue a PATCH, or implement -optimistic concurrency. Those stay with the host. Do not use whole-object `PUT`: projected objects are -intentionally incomplete, and a `PUT` can delete fields the browser never saw. +It does not grant write permission, choose a projection, fetch the object, issue a PATCH or implement +optimistic concurrency. Those stay with the host. + +## Answer 204 or a receipt and let the watch echo it + +The object a Kubernetes write returns is a *raw* object: `managedFields`, the last-applied annotation, +`status`, and the Secret values your projection withholds. Returning it would hand the browser, +through your save endpoint, exactly what the stream refuses to send. + +Answer 204 instead. The write comes back down the stream as an ordinary `modified` event: projected, +redacted and three-way merged into the draft the person is still holding. Dirty state is derived from +`draft` versus `server`, so there is nothing to clear or adopt. + +A **receipt-only HTTP 200** is also valid: define and validate a receipt schema containing only +acknowledgment fields, and keep it out of the resource store. The example editor accepts any +successful status but does not parse a receipt; add that parsing in the host if you need it. For a Git-backed workflow, Kubernetes +write acceptance, CommitRequest acceptance and an observed Git commit are separate milestones; a +receipt must not imply all three. + +## On 409, reconcile and save again + +Keep the draft and reconcile a fresh, complete **projected** GET. Capture the reconciliation guard +before starting that GET: + +```ts +const reconcile = store.captureReconciliation(uid); +const { object, redactedPaths } = await hostRead(); +reconcile(object, { redactedPaths }); +// false can also mean snapshot recovery or unknown redaction paths. Do not force it. +``` + +The host GET must be a most-recent Kubernetes read, with no intermediary cache, under the same +projection as the stream. Local edits made during the request survive reconciliation. Render the +current draft and any conflicts, and let the person review before capturing a new intent. Do not +retry the old patch with a newer version, and do not apply a response for a replacement UID to the +old editor. ## Why a quiet stream can still reject a save @@ -74,11 +104,8 @@ sequenceDiagram H-->>S: Report stale version, follow save recovery flow ``` -The displayed server content is right even though the held version is older. The local draft is the -person's proposed change; it is not part of stream convergence. In this conditional merge-PATCH -flow, a 409 signals a failed version precondition, not necessarily a disagreement at an editable -field. Follow the [save outcomes](#what-the-person-editing-sees) to refresh, reconcile and capture a -newly reviewed intent. +The displayed content is right even though the held version is older. A 409 here signals a failed +version precondition, not necessarily a disagreement at an editable field. | Final upstream change | What the gateway delivers | What the browser holds | |---|---|---| @@ -86,99 +113,51 @@ newly reviewed intent. | Status only | No event under `krm-spec/v1`; an update under `krm-full/v1` | Spec view can keep an older RV; full view receives live status | | Secret token rotates | Under `krm-full/v1` or `krm-spec/v1`, an update with a higher redaction revision | The token remains withheld; its change is visible | -The RV labels above are illustrative opaque strings. Neither RV nor event `seq` provides downstream -resume: a new connection starts a complete snapshot. The -[normative contract](../spec/v1.md#6-ordering-delivery--the-state-guarantee) defines the exact comparison -and its guarantee at each delivered stream position; it does not promise zero transport latency. +The RV labels are illustrative opaque strings. Neither RV nor event `seq` provides resume: a new +connection starts a complete snapshot. The +[normative contract](../spec/v1.md#6-ordering-delivery--the-state-guarantee) defines the exact +guarantee. Sustained invisible churn can prevent save progress. An accepted projected GET advances the base -without requiring a snapshot. Render actual draft conflicts separately; when none exist, explain -the refreshed base and offer a newly captured save. If reconciliation is refused, preserve the draft -and recover before writing again. -Do not remove concurrency protection or blindly retry the old patch with a newer version. +without a snapshot. When no field conflicts exist, explain the refreshed base and offer a new save. ## What the person editing sees -These presentations map to the copyable conditional editor's outcomes. The host owns wording, -authentication and any receipt or Git workflow; connection state is not a save guarantee. +These presentations map to the conditional editor's outcomes. The host owns wording, authentication +and any receipt or Git workflow; connection state is not a save guarantee. | Situation / outcome | Suggested presentation | Host action | |---|---|---| | `version-stale`, no field conflicts | “Configuration refreshed. Your edits are intact; review and save again.” | Capture a new intent on the next deliberate Save. Do not show an empty conflict panel. | | `draft-conflict` | Show local and current values at each conflicting field. | Offer explicit resolution through store APIs; keep the rest of the form visible. | -| Connection retrying or `recovering` | “Reconnecting. Your unsaved changes are still here.” | Disable writes until live; after a refused GET, the example requires a later accepted guarded read before another write. | +| Connection retrying or `recovering` | “Reconnecting. Your unsaved changes are still here.” | Disable writes until live; after a refused GET, require a later accepted guarded read before another write. | | `saved`, watch confirmation pending | “Saved to Kubernetes; waiting for live confirmation.” | Preserve later typing. Track any receipt separately from draft state. | | Session expiry or access denial | Explain sign-in or access outcome. | Handle identity-scoped recovery; do not retry terminal auth failures indefinitely or label them field conflicts. | -| `unavailable`, deleted/recreated UID | “This configuration was removed. A replacement must be opened separately.” | Offer copy-out from a previously retained recovery copy; never apply the old draft automatically to the replacement. | - -“Unsaved changes are still here” applies while the UID remains in the store. `removeResource` and -snapshot pruning discard deleted-object drafts. If recovery after deletion matters, retain a detached -copy as edits change, **before** removal; observing a missing UID is too late to read its old draft. -Keep recovery copies scoped to the original identity and UID, with a host-defined lifetime and cleanup. -They are for recovery, not a second draft to reconcile against incoming snapshots. An executed -subscription recipe, including edit-time capture and pruning, remains -[planned work](proposals/0006-stream-and-save-implementation-plan.md#deletion-recovery-copy). - -For explicit keep-local resolution, the planned tested recipe is tracked in -[proposal 0006](proposals/0006-stream-and-save-implementation-plan.md#explicit-keep-local-resolution). -There is currently no dedicated keep-local helper; avoid a second application conflict registry. - -## Answer 204 or a receipt and let the watch echo it +| `unavailable`, deleted/recreated UID | “This configuration was removed. A replacement must be opened separately.” | Offer copy-out from a retained recovery copy; never apply the old draft to the replacement. | -The object a Kubernetes write returns is a *raw* object: `managedFields`, the last-applied annotation, -`status`, and the Secret values your projection withholds. Writing it to the response hands the -browser, through your own save endpoint, exactly what the stream is designed to refuse. The save -endpoint is not covered by the projection unless you cover it. - -You do not need to. The write reaches the API server, the watch sees it, and it comes back down the -stream as an ordinary `modified` event: projected, redacted, and three-way merged into the draft the -user is still holding. Dirty state is derived from `draft` versus `server`, so there is nothing to -clear and nothing to adopt. The echo settles it. - -A **receipt-only HTTP 200** is also valid: define and validate a host receipt schema containing only -intended acknowledgment fields, and keep it separate from the resource store. The copyable editor -accepts successful HTTP status but does not parse a receipt; add parsing in the host when needed. -For a Git-backed workflow, Kubernetes write acceptance, CommitRequest acceptance and an observed Git -commit are separate milestones. A receipt must not imply that all three have happened. - -## If you must answer with the object - -Prefer 204. If a host returns an object, capture `store.captureReconciliation(uid)` **before** the -save request and apply its projected response through that guard. This prevents a delayed response -from overwriting a newer watch event, and preserves local edits made while saving. - -`store.adoptSaved(object)` remains available for synchronous adoption and newly created objects. It -is unguarded and must not receive delayed responses that can race the watch. Never adopt a raw -Kubernetes response: project it first and provide the correct redaction metadata. Hosts exposing -redacted resources can return `redactedPaths` directly from `gateway.Project`. The guard retains -known stream revisions for paths still present and removes paths absent from that list. Omitted -redaction metadata preserves existing protections. Unknown paths reject the entire response: open a -later authoritative upsert for that UID or a fresh stream snapshot before retrying reconciliation. -Never invent revision counters for a GET. -An explicit `redacted` array is still supported when the host has authoritative stream revisions. +“Unsaved changes are still here” holds while the UID remains in the store. `removeResource` and +snapshot pruning discard a deleted object's draft, so if recovery after deletion matters, keep a +detached copy as edits change, **before** removal. Scope it to the original identity and UID, with a +host-defined lifetime; it is for recovery, not a second draft to reconcile. Resolve conflicts through +the store's APIs rather than a second application conflict registry. ## Creating and deleting whole objects -A create and a delete are host writes exactly as a save is, and they stay host-side for the same -reasons: RBAC, attribution, and — for a create body — validation all live on the server. The client -stages the *intent*; your endpoint performs the *write*. See -[client state model](client-state-model.md#creating-and-deleting-whole-objects) for the client half — -the store keys on uid and has no merge for these, so the consumer aggregates staged create/delete with -`changes()` into one review list. +A create and a delete are host writes exactly as a save is: the client stages the *intent*, your +endpoint performs the *write*. See +[client state model](client-state-model.md#creating-and-deleting-whole-objects) for the client half. The host must: - Authorize the operation and pin the target, resource kind, namespace and name. -- Validate create bodies against the allowed fields and schema before calling Kubernetes. A create - has no existing object for `ValidateMergePatch` to compare; that helper validates merge patches. -- Bind a delete to the intended UID with a Kubernetes delete precondition so a replacement object - under the same name is not removed by an old request. -- Return 204 or a receipt and let the watch reflect the result. Project any returned resource before - sending it to the browser, and preserve meaningful Kubernetes error categories. +- Validate create bodies against the allowed fields and schema. `ValidateMergePatch` validates merge + patches only. +- Bind a delete to the intended UID with a delete precondition, so a replacement object under the + same name is not removed by an old request. +- Answer 204 or a receipt and let the watch reflect the result, preserving meaningful Kubernetes + error categories. -For an authorized delete, preserve the UID captured when the user selected the object; do not -replace it with a newer GET's UID. This fragment uses the host's caller-scoped `dynamic.Interface`, -validated resource/namespace/name, and the request context: +Preserve the UID captured when the person selected the object, not a newer GET's: ```go // metav1: k8s.io/apimachinery/pkg/apis/meta/v1 @@ -192,10 +171,23 @@ if err != nil { } ``` -This binds the delete to object identity. A host that also requires unchanged content can add a -captured resourceVersion precondition. For the complete GET/PATCH save path, use the [compiled -conditional-save handler](../gateway/kube/examples/conditionalsave/handler.go). +A host that also requires unchanged content can add a captured resourceVersion precondition. The host +clears its own pending-create/delete entries when a write succeeds; see the +[client state model](client-state-model.md#reflecting-the-result). For the complete GET/PATCH path, +use the [compiled conditional-save handler](../gateway/kube/examples/conditionalsave/handler.go). -The host also clears its own pending-create/delete entries when a write succeeds. The store does not -own those staging lists. See the [client state model](client-state-model.md#reflecting-the-result) -for synchronous adoption and optimistic-delete caveats. +## If you must answer with the object + +Prefer 204. If a host returns an object, capture `store.captureReconciliation(uid)` **before** the +save request and apply the projected response through that guard. It keeps a delayed response from +overwriting a newer watch event, and preserves local edits made while saving. + +`store.adoptSaved(object)` remains for synchronous adoption and newly created objects. It is +unguarded and must not receive delayed responses that can race the watch. Never adopt a raw +Kubernetes response: project it first and provide the redaction metadata. Hosts exposing redacted +resources can return `redactedPaths` directly from `gateway.Project`. The guard keeps known stream +revisions for paths still present and removes paths absent from that list; omitted redaction +metadata preserves existing protections. Unknown paths reject the whole response: wait for a later +authoritative upsert for that UID or a fresh snapshot before retrying. Never invent revision counters +for a GET. An explicit `redacted` array is still supported when the host has authoritative stream +revisions. diff --git a/examples/README.md b/examples/README.md index 0a0a503..f141a48 100644 --- a/examples/README.md +++ b/examples/README.md @@ -3,11 +3,11 @@ The checked browser example in [`vanilla-browser/`](vanilla-browser/) runs against the replay gateway. For host integration patterns, use these small recipes: -- [Same-origin cookie application](../docs/adopting.md#2-mount-the-same-origin-cookie-endpoint): managed +- [Same-origin cookie application](../docs/adopting.md#1-mount-the-stream-endpoint): managed fetch stream, session cookie, dynamic client acting as the user. -- [Bearer-token fetch client](../docs/adopting.md#3-browser-client): `connectManagedResourceStream` with an +- [Bearer-token fetch client](../docs/adopting.md#bearer-token-clients): `connectManagedResourceStream` with an explicit `Authorization` header for a deliberate token-bearing client. -- [Shared backend with SubjectAccessReview](../docs/adopting.md#4-share-watches-only-with-kubernetes-backed-authorization): +- [Shared backend with SubjectAccessReview](../docs/adopting.md#5-optional-share-watches-with-kubernetes-backed-authorization): one service-account watch plus Kubernetes SubjectAccessReviews for each subscriber. [Conditional save](conditional-save/README.md) composes a managed connection with atomic save capture, diff --git a/gateway/README.md b/gateway/README.md index f6333e7..98853b0 100644 --- a/gateway/README.md +++ b/gateway/README.md @@ -1,84 +1,45 @@ # Go gateway The `gateway` module converts a Kubernetes-style `Backend` watch into the KRM resource-stream -protocol. It is transport-neutral until `Handler` or `ServeStream` adds SSE framing. - -The package has no Kubernetes client dependency. Use [`gateway/kube`](kube/) when `client-go` is the -right upstream for the host application. - -## Required host seams - -`gateway.Handler` requires four deliberate choices: - -| Option | Host responsibility | -|---|---| -| `Principal` | Resolve the HTTP request to an application principal. | -| `Authorizer` | Allow or deny the normalized scope before a watch opens. | -| `Clients` | Return a backend acting as the caller, or an explicitly shared backend. | -| `Scopes` | Allowlist targets, and allowlist group/resource combinations or delegate them to Kubernetes. | - -The zero `ScopePolicy` denies every request. The gateway never accepts an API-server URL or a -credential from a browser request. +protocol. It is transport-neutral until `Handler` or `ServeStream` adds SSE framing. It has no +Kubernetes client dependency; [`gateway/kube`](kube/) provides the `client-go` backend and the +SubjectAccessReview authorizer. ```go handler := gateway.Handler(gateway.Options{ - Principal: principalFromSession, - Authorizer: authorizeScope, + Principal: principalFromSession, // who is calling + Authorizer: authorizeScope, // may they open this scope? Clients: func(_ context.Context, target string, p gateway.Principal) (gateway.Backend, error) { - return kube.NewBackendForConfig(restConfigFor(target, p)) + return kube.NewBackendForConfig(restConfigFor(target, p)) // acting as the caller }, - Scopes: gateway.ScopePolicy{ + Scopes: gateway.ScopePolicy{ // the allowlist; the zero value denies everything Targets: []string{"production"}, Resources: []gateway.GroupResource{ {Group: "", Resource: "configmaps", Scope: gateway.ResourceScopeNamespaced}, - {Group: "", Resource: "namespaces", Scope: gateway.ResourceScopeCluster}, }, }, - Projection: gateway.ProjectionFull, + Projection: gateway.ProjectionFull, + WriteTimeout: 10 * time.Second, }) ``` -## Scope policy - -An empty namespace is not a wildcard by default. - -| Resource declaration | Request namespace | Result | -|---|---|---| -| `ResourceScopeNamespaced` | non-empty | one namespace | -| `ResourceScopeNamespaced` with `AllowAllNamespaces: true` | empty | all namespaces | -| `ResourceScopeCluster` | empty | cluster-scoped resource | -| `ResourceScopeCluster` | non-empty | refused | - -`ScopePolicy.AllowLabelSelector` is also false by default. Enabling selectors permits caller -narrowing only; a host should still constrain selector complexity at its HTTP boundary. +`Handler` panics at construction on a missing seam or an unsafe option. The gateway never accepts an +API-server URL or a credential from a browser request. ## Stream behavior -Every snapshot cycle emits `reset`, zero or more `added` events, then `synced`. Live updates are -complete-object replacements. A recoverable upstream discontinuity emits `RESYNC_REQUIRED` and starts -a new cycle on the same SSE connection. Any other non-terminal error, such as `UPSTREAM_UNAVAILABLE`, -is sent with its `retryAfterMs` and then closes the connection: the client owns that retry. Terminal -errors are the final event and close the connection. - -An unexpected error reaches the browser as `INTERNAL` with the message `internal error`. Its text, -and the `Cause` of any `StreamError`, go only to `Options.Diagnostics`, so the host decides what to -log and what to redact. `gateway/kube` maps API-server failures to protocol codes: 403 to -`FORBIDDEN` with Kubernetes' own message, 401 to `UNAUTHENTICATED`, 404 to `SCOPE_INVALID`, and 429, -5xx or an unreachable server to `UPSTREAM_UNAVAILABLE` (with `Retry-After` as `retryAfterMs`). -`SharedBackend` backs off a scope whose upstream will not open, so its subscribers' reconnects do -not add up to a stampede. +Every snapshot cycle emits `reset`, zero or more `added` events, then `synced`; live updates are +complete-object replacements. Lost upstream continuity emits `RESYNC_REQUIRED` and starts a new cycle +on the same connection. Any other error ends the connection: a non-terminal one such as +`UPSTREAM_UNAVAILABLE` carries `retryAfterMs` and the client retries; a terminal one is the final +event. Unexpected error text goes only to `Options.Diagnostics`. -The gateway absorbs Kubernetes-specific mechanics including bookmarks, relists, 410 responses, -partial metadata objects, and ambiguous deletion tombstones. The normative details are in -[`spec/v1.md`](../spec/v1.md). +The gateway absorbs bookmarks, relists, 410 responses, partial metadata objects and ambiguous +deletion tombstones. `Gateway.Ordering` defaults to strict decimal `resourceVersion` ordering for +Kubernetes 1.35+, preserving per-object monotonicity within a cycle; use `OrderingLenient` only for an +upstream whose versions cannot be ordered. The normative details are in [`spec/v1.md`](../spec/v1.md). -`Gateway.Ordering` defaults to strict decimal `resourceVersion` ordering. It targets Kubernetes 1.35+ -and preserves per-object monotonicity within a snapshot cycle. Use `OrderingLenient` only for an -upstream whose versions cannot be ordered and only after accepting that reduced guarantee. - -## Projections and saves - -The built-in projections are: +## Projections | Projection | Purpose | |---|---| @@ -86,49 +47,22 @@ The built-in projections are: | `krm-full/v1` | Removes metadata noise and redacts Secret values. | | `krm-spec/v1` | The full view without status-driven browser churn. | -The gateway never writes. A host save handler should read the current object under the same identity -and projection decision, then call `ValidateMergePatch` before issuing a Kubernetes merge patch. See -[`docs/saving.md`](../docs/saving.md). +The gateway never writes. `ValidateMergePatch` checks a browser's patch against the projection before +the host sends it; see [saving](../docs/saving.md). ## Shared backends -`SharedBackend` multiplexes one upstream watch per normalized scope. It is opt-in because the shared -watch uses one identity. Pair it with `kube.SubjectAccessReviewAuthorizer` when Kubernetes should continue making -per-caller access decisions. - -`SharedOptions.QueueDepth` bounds live events per slow subscriber. Overflow triggers a resnapshot; -it does not permit unbounded memory growth or silently drop events. - -Each scope opens its upstream watch independently: a scope that is slow to open delays only the -callers waiting for that scope. A caller waiting for an opening leaves when its own request context -ends, and the last one to leave cancels the opening. The upstream `Backend` must honour context -cancellation for that to stop its work; a result it returns after everyone left is stopped and -discarded, and cannot affect a newer opening, the cache or the backoff. - -## Operations +`SharedBackend` is opt-in: it shares one upstream watch per scope, opened as one identity, so your +`Authorizer` becomes the access boundary. Each scope opens independently, and a caller waiting for +an opening can leave with its request; the last to leave cancels it. A slow subscriber is +resnapshotted after `SharedOptions.QueueDepth` live events rather than buffered without bound. See +[shared-watch authorization](../docs/auth.md#shared-watch-authorization). -`Options.HeartbeatInterval` controls SSE keepalives. `Observer` provides low-cardinality lifecycle -signals for stream opens, cycles, emitted/suppressed events, resyncs, overflows, and terminal errors. -Observers run on stream paths, so they must return promptly. +## Guides -See [`docs/operations.md`](../docs/operations.md) for suggested metrics and alerts, and -[`docs/adopting.md`](../docs/adopting.md) for full host wiring examples. - -For shared streams, set `Options.ReauthorizationInterval` (for example, 30 seconds), -`Options.ReauthorizationTimeout` (for example, 5 seconds) and a positive `Options.WriteTimeout`, which -HTTP serving requires with timed checks. Each subscriber is rechecked independently, including during -quiet periods; denial or timeout stops only that subscriber. Zero interval keeps cycle-only checks. -Transport-neutral `Stream` keeps timed checks with any sink the host bounds itself. See -[authorization](../docs/auth.md) for callback contracts, capacity planning and the -[revocation budget](../docs/auth.md#revocation-budget). -Use `kube.SubjectAccessReviewAuthorizer` for Kubernetes-backed subscriber authorization. - -`Options.WriteTimeout` bounds each HTTP header/frame/comment write plus flush. Zero installs no -library deadline; the [shared-host example](kube/examples/sharedstream/README.md) chooses five seconds. -An unsupported writer aborts with `http_transport_rejected` before a logical stream opens. Use -`CheckHTTPStreaming` in a mounted middleware test; it clears the write deadline without writing. - -`stream_closed` balances logical stream opens; `shared_subscription_opened` and -`shared_subscription_closed` describe active attachments, not upstream watch counts. The observer -is synchronous and concurrent: update counters promptly and do not reenter the gateway. -See [operations](../docs/operations.md) for exact counting and timeout contracts. +| Guide | For | +|---|---| +| [Adopting](../docs/adopting.md) | The recommended host and browser wiring, start to finish. | +| [Authorization](../docs/auth.md) | Backend choice, session validity, timed checks and the revocation budget. | +| [Saving](../docs/saving.md) | The host-owned conditional write path. | +| [Operations](../docs/operations.md) | Runtime controls, observations, alerts and bounded HTTP delivery. | From 253a6b311a5c55151f5831685349bfa1cf49e382 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 4 Oct 2026 14:09:49 +0000 Subject: [PATCH 9/9] docs: qualify the 204 watch echo in the saving guide The gateway suppresses an update whose projected view is unchanged, so a write that leaves it unchanged (an admission webhook restoring a patched field, say) has no echo. Say so, and point to the guarded projected GET for that case. Raised by CodeRabbit. Co-Authored-By: Claude Opus 5.5 --- docs/saving.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/saving.md b/docs/saving.md index 39f9115..d8c334a 100644 --- a/docs/saving.md +++ b/docs/saving.md @@ -52,15 +52,18 @@ The object a Kubernetes write returns is a *raw* object: `managedFields`, the la `status`, and the Secret values your projection withholds. Returning it would hand the browser, through your save endpoint, exactly what the stream refuses to send. -Answer 204 instead. The write comes back down the stream as an ordinary `modified` event: projected, -redacted and three-way merged into the draft the person is still holding. Dirty state is derived from -`draft` versus `server`, so there is nothing to clear or adopt. +Answer 204 instead. When the write changes the projected view, it comes back down the stream as an +ordinary `modified` event: projected, redacted and three-way merged into the draft the person is +still holding. Dirty state is derived from `draft` versus `server`, so there is nothing to clear or +adopt. A write that leaves the projected view unchanged (a mutating admission webhook can restore a +patched field, for example) produces no event; if no echo arrives, reconcile with a guarded projected +GET, as [on 409](#on-409-reconcile-and-save-again), before treating the draft as saved. A **receipt-only HTTP 200** is also valid: define and validate a receipt schema containing only acknowledgment fields, and keep it out of the resource store. The example editor accepts any -successful status but does not parse a receipt; add that parsing in the host if you need it. For a Git-backed workflow, Kubernetes -write acceptance, CommitRequest acceptance and an observed Git commit are separate milestones; a -receipt must not imply all three. +successful status but does not parse a receipt; add that parsing in the host if you need it. For a +Git-backed workflow, Kubernetes write acceptance, CommitRequest acceptance and an observed Git commit +are separate milestones; a receipt must not imply all three. ## On 409, reconcile and save again