diff --git a/docs/adopting.md b/docs/adopting.md index 14c95de..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 -## 3. Browser client +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. Connect the browser ```ts import { - defaultPolicy, LiveResourceStore, connectManagedResourceStream, resourceStreamURL, - withOpenAPIKeyedLists, } from "@configbutler/krm-stream"; const store = new LiveResourceStore(); @@ -151,54 +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: -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. +```go +options.ReauthorizationInterval = 30 * time.Second +options.ReauthorizationTimeout = 5 * time.Second +options.WriteTimeout = 10 * time.Second // required with timed checks; Handler panics without it +``` + +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 } ``` -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. +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 -Next: [saving.md](saving.md) for the host-owned write path and [operations.md](operations.md) for -stream monitoring and limits. +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`. -## Tested shared host and middleware +### 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`: -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. +```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, and the API server +decides. The target allowlist, `AllowLabelSelector`, the refusal of API-server addresses and the +`Authorizer` still apply. + +`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. + +### 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. +`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. + +### 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 c4e5297..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,109 +38,128 @@ 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 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. -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. +## Session validity and timed checks -## Long streams, short tokens +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. -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: +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 -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. +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`. + +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. + +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 time from withdrawing access until a stream stops delivering is made of these parts: + +| Part | Bounded by | What it covers | +|---|---|---| +| 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. | + +Measure the total revocation latency for your deployment under its intended load. -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. +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. -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. +`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: +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. -```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 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 + +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` | Copied from the scope. | +| `resourceAttributes.namespace`, `.name` | Copied from the scope. | + +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 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; +- never let a reused review skip its own checks of session validity or projection policy. -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`. +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. - -`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. -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/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/operations.md b/docs/operations.md index 2ac3870..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,30 +28,36 @@ 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 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 | 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 | @@ -60,48 +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. 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/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 new file mode 100644 index 0000000..80f5997 --- /dev/null +++ b/docs/proposals/0008-shared-watch-hardening.md @@ -0,0 +1,214 @@ +# Proposal 0008 Shared watch hardening implementation plan + +**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 +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. 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..d8c334a 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,45 @@ 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. 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. + +## 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 +107,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 +116,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 +174,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 49c730a..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. +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`. -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. +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). -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). - -`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,40 +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. - -## 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) 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. -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. | diff --git a/gateway/handler.go b/gateway/handler.go index cec7e8a..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,29 +57,31 @@ 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. 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 } // 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 { - 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/authz.go b/gateway/kube/authz.go index d84d238..5f6957b 100644 --- a/gateway/kube/authz.go +++ b/gateway/kube/authz.go @@ -62,6 +62,23 @@ 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 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 +// 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..0b66553 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"}, + // Selectors are omitted from the authorization request. + 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) + } + }) + } +} 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/shared.go b/gateway/shared.go index 27b9bf7..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,20 +42,46 @@ 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 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 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 + // 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 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 } // sharedBackoff is one scope's failed attempts to open its upstream watch. @@ -101,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{}) } @@ -120,6 +109,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 +125,123 @@ 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) { +// 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 neither starts nor joins an opening. + 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. 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 + 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, so its last departure cancels 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() +} - return s.subscribe() +// 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 +} + +// 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 { + 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 { @@ -171,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 { @@ -204,22 +283,10 @@ 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. 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, key: key, scope: scope, @@ -228,8 +295,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. @@ -243,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() @@ -304,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 } @@ -374,7 +413,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), @@ -384,23 +428,16 @@ func (s *sharedScope) subscribe() (Watcher, error) { 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) } - - return &sharedWatcher{scope: s, sub: sub}, nil + return sub } // 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 @@ -412,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 @@ -439,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 } @@ -459,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() } @@ -471,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)) @@ -492,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 @@ -515,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) @@ -548,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") } @@ -560,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) @@ -596,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() @@ -612,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 } @@ -625,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 } @@ -642,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, 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) + } + } +} 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.