[miniflare] Route storage/remote bindings through shared services via props#14743
Conversation
… props Move the per-binding configuration for storage and remote (mixed-mode) bindings out of per-resource workerd services and into runtime `ctx.props`, so a single shared service can serve any number of bindings. - Local KV namespaces now share one entry service; the namespace id is passed via props and resolved in object-entry.worker.ts (idFromName). - remoteProxyClientWorker() is now script-only; the connection string, binding name and trace id travel via props (buildRemoteProxyProps), read in remote-proxy-client.worker.ts and the dispatch-namespace proxy. - All remote-binding plugins emit one shared remote-proxy service instead of one per resource. - explorer.ts reads the KV namespace id from binding props rather than parsing it out of the service name.
penalosa
added
the
ci:no-changeset-required
|
|
Codeowners approval required for this PR:
Show detailed file reviewers |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Summary: Moves per-binding storage/remote config out of per-resource workerd services and into ctx.props, letting one shared service back many bindings. The local storage path is exercised by the existing KV/R2/D1 suites and the remote service-binding path by remote-bindings-access-warning.spec.ts, so the behaviour-preserving claim is reasonably supported.
Explorer registers a colliding "remote" id for remote bindings (low severity). Remote KV/R2/D1 bindings now all point at a shared service name (kv:ns:remote, r2:bucket:remote, d1:db:remote), but constructExplorerBindingMap still derives the resource id by string-stripping that name. Every remote binding of a given type therefore registers the literal id "remote", so multiple remote bindings overwrite each other in IDToBindingName, and a bogus "remote" id leaks into the explorer map (selecting it would try to open storage for a namespace literally named remote). KV reads the id from props but still falls through to the name parse for remote (yielding "remote"); the R2 block (explorer.ts:218, const bucketName = binding.r2Bucket.name.replace(/^r2:bucket:/, "")) and the D1 block (explorer.ts:177, const databaseId = innerBinding.service?.name?.replace(/^d1:db:/, "")) weren't updated at all and have the same problem. Since remote resources aren't locally browsable, the cleanest fix is to skip the shared remote service name in all three blocks. Inline suggestion below covers the KV block; please apply the equivalent !== "remote" guard to the R2 and D1 blocks.
|
Review posted successfully to PR #14743. I reviewed the PR and posted a single COMMENT review with one inline suggestion. The one actionable finding: Explorer registers a colliding Everything else checks out: the local storage path is covered by the existing KV/R2/D1 plugin suites and the remote service-binding path by |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Looks good to me, although bonk's comment seems legitimate so think it would be worth looking into it
(as far as I can tell the local explorer doesn't currently support remote bindings, but even if it doesn't right now. this issue would need to be resolved for when(/if) it eventually does)
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
b5df334 to
0c17f28
Compare
Remote KV/R2/D1 bindings now share one proxy service (`kv:ns:remote`, `r2:bucket:remote`, `d1:db:remote`), so the explorer's service-name parse collapsed every remote resource of a type to the literal id "remote" — overwriting each other in the binding map and leaking a bogus "remote" entry. Remote resources aren't supported in the local explorer, so skip them in `constructExplorerBindingMap` for all three types rather than register the shared service name. Local resources are unaffected.
0c17f28 to
9519409
Compare
3 participants
Move the per-binding configuration for storage and remote (mixed-mode) bindings out of per-resource workerd services and into runtime
ctx.props, so a single shared service can serve any number of bindings.object-entry.worker.ts(idFromName)remoteProxyClientWorker()is now script-only — the connection string, binding name and trace id travel via props (buildRemoteProxyProps), read inremote-proxy-client.worker.tsand the dispatch-namespace proxyexplorer.tsreads the KV namespace id from binding props rather than parsing it out of the service nameNo user-facing behaviour change — this is internal plumbing that lets a single service back any number of bindings, replacing the previous one-service-per-resource model.