Repository navigation
feat(gateway): expose workspace service route snapshots and watches - #4941
TaylorMutch wants to merge 3 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-4941.docs.buildwithfern.com/openshell |
c724036 to
932643d
Compare
932643d to
6c83b66
Compare
6c83b66 to
755e4d1
Compare
755e4d1 to
0944ea2
Compare
0944ea2 to
8504067
Compare
8504067 to
feac6b4
Compare
d43f1ae to
b8e0f49
Compare
088c1c3 to
facbf94
Compare
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. rust-sdk — No explicit risk acceptance found
2. go-sdk — No explicit risk acceptance found
3. typescript-sdk — No explicit risk acceptance foundRegenerating
Review details
Advisory review; publication gates are unchanged. |
7a4aaca to
bce60fb
Compare
b65481b to
9cf95fd
Compare
9cf95fd to
2f2a42f
Compare
2f2a42f to
7423a9c
Compare
|
/ok to test 7423a9c |
7423a9c to
9a4abc9
Compare
|
/ok to test 9a4abc9 |
|
/ok to test 2178482 |
2178482 to
6d23a9b
Compare
There was a problem hiding this comment.
The inline comments cover the main findings. The first one, on collect (snapshots failing under lease churn), is the one I'd fix before merge.
Suggestion: a separate service. Putting the two RPCs on service OpenShell means every generated OpenShell server implementation has to add them: the 8 mock stubs in this PR, and the Rust SDK break the compatibility bot flagged. service SandboxUpdates was split out for exactly this reason (proto/openshell.proto:879-881). A ServiceDiscovery service with the same messages would avoid that. It's cheapest to do before #4956 onward and external consumers bind the RPC path.
Follow-ups and nits:
- Token expiry during a watch: the watch rechecks workspace membership every second, but not token expiry or revocation, so a revoked token can keep reading for up to 60 s. Other gateway streams also check authority only when the stream opens, so this probably belongs in a broader stream-auth follow-up rather than this PR.
- User docs: the server README lists the limits (1,000 records, 4,096 routes, 4 MiB, 32 watches, 1 s refresh).
configuration.mdxlists neither the limits nor the error codes clients see when they hit them (RESOURCE_EXHAUSTED,UNAVAILABLE). - Duplicated hostname format: discovery formats
{workspace}--{sandbox}[--{service}]itself (service_discovery.rs:210-214), duplicatingservice_routing.rs:203-207. A shared helper plus a test that the published hostname equals the served one would keep them in sync. Thelabel.len() > 63branch can't trigger, because names are capped at 19 characters (61 at most). - Watch loop: the closed/deadline
select!appears three times. Wrapping the loop in onetokio::time::timeout(WATCH_LIFETIME, …)and selecting ontx.closed()around the refresh and send would be shorter and easier to follow. - Double decode: stored routes are decoded twice per record, once in
is_currentand again incollect. - Visibility: the
discovery_watchesfield onServerStateispub, andservice_advertisement::testsbecamepub modto share a fixture.pub(crate)and acfg(test)test-support module would be narrower.
| async fn collect(state: &ServerState, workspace: &str) -> Result<ServiceRouteSnapshot, Status> { | ||
| // Verify the installation collection did not change across the scan. | ||
| // This is a bounded verified view, not a cross-resource transaction. | ||
| for _ in 0..2 { |
There was a problem hiding this comment.
Blocker: discovery fails on every call once a workspace has about 50 installations.
Lease renewal rewrites every installation every 5 s (REPORT_INTERVAL, driven from supervisor_session.rs:2935), and each put_if bumps resource_version. Validating one record with is_current costs about 2.7 ms here, because it builds a full config snapshot. A scan of N records therefore takes about N × 2.7 ms. Once that exceeds 5 s / N, some record always changes between the two records() calls, both attempts fail, and the call returns UNAVAILABLE. A watch's initial snapshot fails the same way, so watchers never connect, and proxies drop every route when the 15 s expiry passes.
I reproduced this with a scratch test (debug build, SQLite). It renewed N installation records at the 5 s cadence and called snapshot() in a loop for 8 s:
| Installations | Snapshots failed |
|---|---|
| 10 | 0% |
| 25 | 0% |
| 50 | 100% (28/28) |
| 100 | 100% |
A release build or Postgres moves the threshold, but the failure rate still grows with N².
Suggested fix: drop the second scan and the retry. Each record is already validated on its own, and the revision already covers expiry. If you still want a membership check, compare IDs only, not resource versions. A regression test with renewals running during the scan would cover it.
| } | ||
| let refresh = async { | ||
| authorize(&state, &principal, &workspace).await?; | ||
| snapshot(&state, &workspace).await |
There was a problem hiding this comment.
Total discovery cost has no bound. Each watch reruns the full validated collection every second. That is two list scans plus is_current per record (sandbox read, owner read, pool check, config snapshot build). The revision includes expires_at, so on a busy workspace it changes almost every tick, and every watcher receives a full snapshot of up to 4 MiB nearly every second. Unary GetServiceRouteSnapshot has no concurrency cap beyond the 10 s timeout.
Rough numbers: at about 2.7 ms per record (debug, SQLite), 50 installations × 32 watches is about 4.5 s of store work per second. On a local gateway with an empty store, 50 snapshot loops plus 32 watches raised ListSandboxes p99 from 5 ms to 74 ms. That measurement is noisy, because the load ran on the same host.
Suggestion: share one collection per workspace per refresh tick across watchers, or cache the snapshot for the tick, and bound concurrent unary snapshots.
| let permit = state | ||
| .discovery_watches | ||
| .clone() | ||
| .try_acquire_owned() |
There was a problem hiding this comment.
One user can take the whole watch pool. discovery_watches is a single 32-permit semaphore per gateway process, with no per-user or per-workspace share. A user in one workspace can hold all 32 watches and reopen them every minute. Ingress for every other workspace on that replica then gets RESOURCE_EXHAUSTED. I confirmed that the 33rd watch is refused immediately. I didn't demonstrate the cross-workspace starvation itself, because my test gateway had only one identity.
Suggestion: a per-principal cap (for example 4) and a per-workspace cap under the global one.
| )) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Tests miss the documented limits and the identity hash. No test covers the 4,096-route limit or the response-size limit. No test covers the changed-during-collection retry either; that is the path that fails under lease churn in the comment on collect. installation_version stability is checked for lease renewal, but no test shows that it changes when an identity input changes (supervisor instance, connection or ownership epoch, host, port, route bytes). That is the field proxies key on. A table test over those inputs, plus one test per limit, would pin them down.
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
…nsion Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
6d23a9b to
fcb9be2
Compare
Summary
Expose accepted supervisor installations through GetServiceRouteSnapshot and WatchServiceRoutes for one explicit authorized workspace. Independent proxies and infrastructure controllers consume exact hostnames and trusted supervisor endpoints through the same API.
Based on main at
cb836ea9d.Related Issue
Closes #4667. Part of RFC 0017.
Changes
Gate snapshot and watch RPCs on
supervisor_app_listener.enabled; disabled listeners returnFAILED_PRECONDITIONbefore collecting routes or allocating a watch.Publish accepted installation records from feat(supervisor): advertise accepted application route installations #4940, which resolves trusted supervisor endpoints through the private
ResolveSupervisorAppEndpointcompute RPC. External consumers use the public discovery RPCs; ordinaryGetSandboxobservations do not resolve routes.Return complete replacements with installation versions, declaration revisions, expiry, and current owner endpoints.
Recheck session, runtime, credential epoch, and shared pool authority before publishing routes.
Bound collection, watches, buffering, send time, and lifetime; periodically reauthorize watches and fail closed on storage errors.
Document client cache expiry and stable installation versions across lease-only renewal.
Release Notes
Discovery and Kubernetes request flow
Discovery returns an application
hostnameand a trusted supervisorendpoint(host and port), together with installation identity and expiry.
nameis thesandbox route's service name; an empty value identifies the unnamed/default
service. It is not a Kubernetes Service resource name.
The endpoint is the owning supervisor Pod IP and app-listener port. Consumers
preserve the application hostname as TLS SNI and HTTP Host/authority. Multiple
routes can share one supervisor endpoint; the hostname selects the sandbox
instance and service. The authoritative public application URL is separate and
can use a different public port.
flowchart TB subgraph Control["Control plane: build and publish discovery"] SUP["Supervisor<br/>Reports installed routes against a lease"] GW["OpenShell gateway<br/>Validates installation and ownership"] KD["Kubernetes compute driver<br/>ResolveSupervisorAppEndpoint"] KUBE["Kubernetes objects + supervisor ownership<br/>Find and validate owning supervisor Pod"] API["Discovery API<br/>hostname + Pod IP:8443 + expiry"] CONSUMER["Sandbox proxy or<br/>example Kubernetes HTTPRoute controller"] SUP -->|"Installed-route advertisement"| GW GW -->|"Resolve trusted endpoint"| KD KD --> KUBE KUBE -->|"Validated Pod IP and listener port"| KD KD --> GW GW --> API API -->|"Snapshot / watch"| CONSUMER end subgraph Data["Application data plane"] CLIENT["Client<br/>https://team--demo--web.apps.example.com"] EDGE["Configured proxy or Kubernetes Gateway<br/>Public listener, e.g. port 443"] LISTENER["Owning supervisor Pod<br/>10.42.1.27:8443<br/>TLS app listener"] DISPATCH["Select sandbox instance + named service<br/>using SNI / Host"] CONNECTOR["Selected instance's sandbox connector"] APP["Application inside sandbox<br/>127.0.0.1:8080"] CLIENT --> EDGE EDGE -->|"Connect to discovered endpoint<br/>preserve application SNI / Host"| LISTENER LISTENER --> DISPATCH DISPATCH --> CONNECTOR CONNECTOR --> APP end CONSUMER -.->|"Configure forwarding"| EDGEAddresses, domain names, and ports above are illustrative. Discovery describes
accepted installations, not application health or external ingress readiness.
The Kubernetes driver resolves trusted endpoints; a separate consumer configures
HTTPRoutes or proxy forwarding. Application traffic bypasses the OpenShell
gateway. The proxy and example controller are separate consumers, not part of
this PR's implementation.
Testing
Live Kubernetes validation preceded integration of the latest listener hardening;
focused Rust regressions validate that final integration. New-head CI is pending.
Qualification limits
Live HA validation exercised supervisor process/Pod loss on a local k3d cluster. Whole-node failure, real network partitions, metric-driven HPA, sustained production load, and published release artifacts remain unqualified. Grant-file projection contributes to revocation propagation time. Main now includes #4842 for gateway-mediated hashed hostnames. Direct application-listener/discovery hostnames remain readable; extending hashed naming to that path and installation-status presentation (#4670) remain separate work.
The example Kubernetes HTTPRoute controller is a deferred compatibility POC outside this stack.
Checklist
PR dependency order: #4941 → #4956 → #4963 → #4964 → #4957.