Skip to content

feat!: map upstream errors, keep error text off the wire, and let the client own retries - #40

Merged
sunib merged 6 commits into
mainfrom
feat/krm-foyer-feedback
Oct 2, 2026
Merged

sunib merged 6 commits into
mainfrom
feat/krm-foyer-feedback

Conversation

@sunib

@sunib sunib commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Implements the five asks from krm-foyer's feedback, which is committed here as docs/field-reports/first-fieldreport.md. The README now points to krm-foyer for a quick start.

Based on current main (after #35 and the 0.4.0 release, #32). It contains only the two commits for this work.

Changes

1. Map Kubernetes errors, and recover from them (gateway/kube, gateway/stream.go, gateway/shared.go)

  • API-server failures become protocol codes: 403 → FORBIDDEN with Kubernetes' own message, 401 → UNAUTHENTICATED, 404 → SCOPE_INVALID, and 429, 5xx or an unreachable server → UPSTREAM_UNAVAILABLE. On a 429, Retry-After becomes retryAfterMs. A 410 still starts a new snapshot.
  • This applies on every path: opening a streaming list, the LIST and the WATCH of the list-then-watch fallback, and ERROR events on an open watch.
  • The client retries. The gateway recovers only RESYNC_REQUIRED on the same connection. Any other non-terminal error is sent with its code and hint, and then the connection closes. A new retryable_error observation reports these.
  • SharedBackend backs off a scope whose upstream won't open (1s, doubling up to 30s). However many subscribers reconnect, the upstream sees one attempt per backoff period.
  • A SubjectAccessReview that can't reach the API server gives UPSTREAM_UNAVAILABLE. That includes timed reauthorization checks, which therefore end the stream without making the error terminal.

2. Keep unexpected error text off the wire

  • A generated INTERNAL says "internal error". The raw error goes to a new Options.Diagnostics hook, with the principal and scope it belongs to.
  • StreamError has a new Cause field, which is never sent.

3. ScopePolicy.AnyResource

  • Leaves resource admission to the upstream's RBAC. The target allowlist, the label-selector setting and the refusal of API-server addresses still apply.
  • Setting both AnyResource and Resources panics in Handler, and Validate refuses every scope.
  • The docs say that binding the stream to the caller's own credential is the host's responsibility.

4. Principal can say why it failed

  • It may return a *StreamError to choose the code. Any other error is now UNAUTHENTICATED.

5. Client retry hints (@configbutler/krm-stream)

  • The managed connector waits at least Retry-After or retryAfterMs, within maxRetryDelayMs, and drops the hint once a snapshot completes.
  • onError receives the hint as a fourth argument.
  • When a non-200 body is a Kubernetes Status, onError shows its message. The read is capped at 16 KiB.

Spec

  • §4.3 now says who recovers each non-terminal error.
  • §8 and gateway conformance item 10 allow "an allowlisted target and a host-approved GVR".
  • New consumer conformance item 10: honour retryAfterMs.

Review follow-ups

  • Shared backoff after a watch opens. A retryable failure is now recorded when an established upstream watch dies, in the same critical section that removes the scope. The backoff resets only after a completed upstream snapshot, not just because a watcher was obtained. Before the fix, 100 concurrent reconnects caused 8 to 50 upstream opens; now there is 1, and the upstream's hint is honoured.
  • Refusal-body reads can be cancelled and are time-limited. The read stops when the stream closes and after 2 seconds, falling back to stream: HTTP <status>. Before the fix, close() during a quiet body never resolved.
  • SubjectAccessReview hints are kept. A throttled review keeps its Retry-After all the way to the SSE event.
  • Changelogs. The unreleased sections of all three changelogs now describe these changes.

Breaking changes

  • Principal errors, and kube.SubjectAccessReviewAuthorizer callers it can't map to a subject, now give UNAUTHENTICATED instead of FORBIDDEN.
  • The text of unexpected errors no longer reaches the browser.
  • Non-terminal errors other than RESYNC_REQUIRED now end the connection.

Testing

  • task verify passes on the rebased branch: lint, Go (-race) and TypeScript suites, wire and browser e2e, pack. The conformance goldens are unchanged.
  • New tests:
    • every error code on each of the three opening paths, and classified watch ERROR events
    • a real refused connection: the address appears only in the Cause
    • a secret string in an error appears only in Diagnostics
    • a dead upstream gets one attempt per connection, and SharedBackend's backoff holds under a reconnect storm
    • AnyResource admission and refusals
    • client hint handling and Status parsing
    • shared backoff when the watch dies after opening (Next() error and WatchError event, 100 concurrent reconnects), and reset only after a snapshot
    • refusal bodies that stay quiet, arrive partially, or are cancelled mid-read
    • a throttled SubjectAccessReview carrying retryAfterMs: 7000 to the browser
    • each follow-up test fails against the code before its fix
  • Not run: the real-cluster suite (test-cluster).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Resource access can now be delegated to Kubernetes authorization, while target and scope restrictions remain in effect.
    • Stream clients now honor server-provided retry delays when reconnecting, within their configured maximum delay.
  • Bug Fixes
    • Kubernetes API failures now receive clearer, more appropriate stream error codes and retry guidance.
    • Unexpected internal errors no longer expose underlying details in browser-facing messages.
  • Documentation
    • Updated integration guidance with a recommended Kubernetes backend and additional adoption and operations details.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 43 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4b05d0b0-ec9e-45bf-84ef-a8defc789b09

📥 Commits

Reviewing files that changed from the base of the PR and between edbdbdb and 1a0050c.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b7901de0-bae8-4d14-ab2e-d56b0202d93e

📥 Commits

Reviewing files that changed from the base of the PR and between 9aa0013 and edbdbdb.

⛔ Files ignored due to path filters (1)
  • gateway/kube/go.sum is excluded by !**/*.sum
📒 Files selected for processing (33)
  • README.md
  • docs/adopting.md
  • docs/auth.md
  • docs/client-state-model.md
  • docs/field-reports/first-fieldreport.md
  • docs/operations.md
  • docs/releasing.md
  • gateway/CHANGELOG.md
  • gateway/README.md
  • gateway/event.go
  • gateway/handler.go
  • gateway/handler_test.go
  • gateway/kube/CHANGELOG.md
  • gateway/kube/authz.go
  • gateway/kube/authz_test.go
  • gateway/kube/backend.go
  • gateway/kube/classify_test.go
  • gateway/kube/go.mod
  • gateway/observe.go
  • gateway/reauthorize.go
  • gateway/scope.go
  • gateway/seams.go
  • gateway/shared.go
  • gateway/shared_test.go
  • gateway/stream.go
  • gateway/stream_test.go
  • packages/krm-stream/CHANGELOG.md
  • packages/krm-stream/README.md
  • packages/krm-stream/src/connection.ts
  • packages/krm-stream/src/sse.ts
  • packages/krm-stream/test/connection.test.ts
  • packages/krm-stream/test/wire.test.ts
  • spec/v1.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The changes update gateway scope admission and error handling, classify Kubernetes failures, add shared-watch backoff, and pass retry hints through the managed client. Documentation and tests cover these behaviors, along with integration and release guidance.

Changes

Gateway recovery and client retry behavior

Layer / File(s) Summary
Scope admission and refusal behavior
gateway/scope.go, gateway/seams.go, gateway/handler.go, gateway/handler_test.go, gateway/event.go, docs/adopting.md, docs/auth.md, gateway/README.md, gateway/CHANGELOG.md, spec/v1.md
ScopePolicy.AnyResource delegates resource admission while retaining other scope checks. Handler refusals preserve supplied StreamError values, map other principal errors to UNAUTHENTICATED, and send raw errors to diagnostics.
Stream termination and diagnostics
gateway/stream.go, gateway/observe.go, gateway/reauthorize.go, gateway/stream_test.go, gateway/handler_test.go, gateway/CHANGELOG.md, gateway/README.md, docs/adopting.md, docs/auth.md, docs/client-state-model.md, docs/operations.md, spec/v1.md
Only RESYNC_REQUIRED continues on the same connection. Other errors end the stream. Unexpected errors use the generic wire message internal error; diagnostics retain raw errors, and observations distinguish retryable from terminal errors.
Kubernetes error classification
gateway/kube/authz.go, gateway/kube/backend.go, gateway/kube/classify_test.go, gateway/kube/authz_test.go, gateway/kube/CHANGELOG.md, gateway/kube/go.mod, docs/auth.md
Kubernetes API and transport failures map to gateway errors with causes and retry hints. SubjectAccessReview failures can return UPSTREAM_UNAVAILABLE; tests verify error codes, terminal status, retry hints, and cause handling.
Shared upstream watch backoff
gateway/shared.go, gateway/shared_test.go, gateway/CHANGELOG.md, docs/operations.md
SharedBackend applies per-scope backoff to retryable upstream failures. The delay starts at one second, doubles to a 30-second limit, and can use an upstream retry hint. A completed initial snapshot clears the backoff.
Managed client retry hints and refusal messages
packages/krm-stream/src/connection.ts, packages/krm-stream/src/sse.ts, packages/krm-stream/test/*, packages/krm-stream/README.md, packages/krm-stream/CHANGELOG.md, spec/v1.md
The managed connector uses stream and HTTP retry hints as a minimum reconnect delay within its configured cap. The SSE client classifies selected HTTP failures and reads bounded Kubernetes Status messages for refusals.
Integration and release guidance
README.md, docs/field-reports/*, docs/adopting.md, docs/releasing.md, docs/operations.md
The README recommends krm-foyer as a starting point. The field report records integration requests and proposals. Adoption and release guidance describes diagnostics and adapter dependency updates.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~50 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant ManagedConnector as connectManagedResourceStream
  participant Gateway
  participant SharedBackend
  participant KubeBackend as gateway/kube.Backend
  participant KubernetesAPI
  ManagedConnector->>Gateway: Open stream
  Gateway->>SharedBackend: Request scope watch
  SharedBackend->>KubeBackend: Open upstream watch
  KubeBackend->>KubernetesAPI: List or watch resources
  KubernetesAPI-->>KubeBackend: Return upstream failure
  KubeBackend-->>SharedBackend: Return classified error and retry hint
  SharedBackend-->>Gateway: Return upstream-unavailable error
  Gateway-->>ManagedConnector: Send error event and close stream
  ManagedConnector->>ManagedConnector: Wait at least the retry hint
  ManagedConnector->>Gateway: Reconnect
Loading

Merge Risk: ⚪ Minimal · up to edbdb

The identified concerns do not block the documented retry behavior or supported builds. The PR is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to edbdb

Authorization remains mandatory, unexpected error details are withheld, and retries are bounded. The broader resource-access option depends on correct caller credential binding, which has not been established for deployed integrations.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Enabling AnyResource expands gateway admission beyond configured resource and namespace limits on allowlisted targets. Effective exposure still depends on the authorizer, upstream credential, and projection policy. A broader backend identity combined with permissive authorization could expose collections outside the intended caller boundary, but no deployed instance of that combination was established.

Security Findings and Attack Paths

  • observed — The supplied throttled-authorization test prevents backend opening and returns a non-terminal UPSTREAM_UNAVAILABLE event with its retry hint. The stream recovery test keeps a connection-refusal detail in diagnostics without including it in the client-visible message. These tests support specific control paths, not production credential isolation.

Trust Boundaries and Controls

  • observed — Handler requires explicit principal, authorizer, and backend-resolution functions. Scope admission remains deny-by-default. AnyResource retains target and selector checks, and conflicting delegation-plus-allowlist configuration is refused.
  • observed — The Kubernetes authorizer checks both list and watch for the mapped caller subject. A failed review is not an allow. During timed reauthorization, an unreachable authority cancels the active cycle while permitting later client reconnection; it does not permit the stream to continue on the previous decision.

Resilience and Maintainability Implications

  • observed — The managed client awaits closure before reconnecting, stops on terminal errors or exhausted retry budgets, bounds retry hints by its configured delay cap, and cancels pending retry waits on abort. Snapshot synchronization clears obsolete hints, and sustained healthy operation resets the retry budget.
  • observed — HTTP refusal-message parsing has byte and time budgets and responds to cancellation. Parsed messages are accepted only from JSON Status objects, limiting resource retention while reading an unsuccessful response.

Hardening Proposals

  • proposed — Integrations enabling AnyResource should demonstrate caller-to-backend credential binding with an end-to-end isolation test, including two callers with different resource and namespace permissions. Keep an explicit resource allowlist where that binding cannot be established.
  • proposed — Consider lifecycle-driven expiration or a cardinality bound for abandoned backoff entries so an outage's failed-scope metadata does not depend on a later failure or successful retry for cleanup.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 88.64% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 19 files. (14 skipped: …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: upstream error mapping, withholding internal error text from the wire, and client-owned retries.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sunib
sunib changed the base branch from feat/shared-stream-transport to main October 2, 2026 12:51
sunib and others added 2 commits October 2, 2026 12:55
… client own retries

Implements the five asks in docs/field-reports/first-fieldreport.md from krm-foyer.

- gateway/kube maps API-server failures to protocol codes on every path (streaming
  list, fallback LIST and WATCH, watch ERROR events): 403 FORBIDDEN, 401
  UNAUTHENTICATED, 404 SCOPE_INVALID, 429/5xx/unreachable UPSTREAM_UNAVAILABLE
  with Retry-After as retryAfterMs; 410 still resyncs.
- The stream loop recovers only RESYNC_REQUIRED in band. Any other non-terminal
  error is sent with its code and hint and the connection closes; the client
  retries. SharedBackend backs off a scope whose upstream will not open.
- A generated INTERNAL says "internal error". Raw errors, and the new
  StreamError.Cause, go only to Options.Diagnostics.
- ScopePolicy.AnyResource delegates resource admission to the upstream's RBAC.
- Principal may return a *StreamError; other errors are UNAUTHENTICATED.
- The managed client honours Retry-After and retryAfterMs, and shows a
  Kubernetes Status message on a non-200 refusal.
- spec §4.3 states who recovers; §8 and conformance item 10 allow a
  host-approved GVR.
- README points to krm-foyer for a quick start.

BREAKING CHANGE: Principal errors and kube.SubjectAccessReviewAuthorizer's
unmapped callers are UNAUTHENTICATED instead of FORBIDDEN; unexpected error text
no longer reaches the browser; non-terminal errors other than RESYNC_REQUIRED
now end the connection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ads, keep SAR hints

Review follow-ups for the krm-foyer field report.

- SharedBackend records a retryable failure when an established upstream watch
  dies, atomically with forgetting the scope, so repeated watch-error events can
  no longer bypass the backoff or its hint. The backoff now resets only after a
  completed upstream snapshot, not on merely obtaining a watcher.
- The client's refusal-body read stops when the stream closes and after a
  2-second budget, falling back to "stream: HTTP <status>"; close() during the
  read no longer hangs, and a closed stream reports nothing.
- A throttled or unreachable SubjectAccessReview keeps its classified error,
  including the Retry-After hint and cause.
- Record the field-report changes in the unreleased changelogs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sunib
sunib force-pushed the feat/krm-foyer-feedback branch from 30f2dc4 to 401768b Compare October 2, 2026 12:56
sunib and others added 4 commits October 2, 2026 13:03
…urrent

recovered(key) cleared a scope's backoff without checking which watch had
recovered. Called after the scope lock is released, it could arrive after the
watch was forgotten and a replacement had failed, erasing the newer backoff and
allowing an immediate reopen. Clear it only while that watch is still the
scope's current one, under the backend lock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The adapter now uses gateway.Unauthenticated, gateway.UpstreamUnavailable and
StreamError.Cause, and relies on the stream loop closing after a retryable
error. Its go.mod still required gateway v0.2.1, so go.work hid a build failure
that every consumer would hit, and an adopter on an older core would have
retried UPSTREAM_UNAVAILABLE in a tight loop. Require the core at the pushed
commit that has both. docs/releasing.md says how to make such a change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The check treated only v0.0.0-… as a pseudo-version. When an adapter change
needs unreleased core API, Go chooses the canonical v0.4.1-0.… form once a tag
precedes the commit, and the check skipped it as "not tagged yet" without
building anything. Recognise any version ending in a timestamp and revision.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sunib
sunib merged commit 891e93e into main Oct 2, 2026
13 checks passed
@github-actions github-actions Bot mentioned this pull request Oct 2, 2026
sunib added a commit that referenced this pull request Oct 2, 2026
Release Please inserts each version below the hand-written "Unreleased" section
and leaves it in place, so released notes stayed labelled Unreleased. File them
under the version that shipped them: the #35 notes under 0.4.0, the #40 notes
under 0.5.0.

Point gateway/kube at main's 891e93e rather than the PR-branch commit
e723903. The core module is identical; the released adapter now requires a
commit on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant