Skip to content

fix(miniflare): decouple KV plugin from Secrets Store plugin to avoid service name conflicts#9106

Merged
edmundhung merged 4 commits intomainfrom
edmundhung/refactor-secret-store-plugin
May 22, 2025
Merged

fix(miniflare): decouple KV plugin from Secrets Store plugin to avoid service name conflicts#9106
edmundhung merged 4 commits intomainfrom
edmundhung/refactor-secret-store-plugin

Conversation

@edmundhung
Copy link
Copy Markdown
Member

@edmundhung edmundhung commented Apr 30, 2025

Fixes #9006.

We were relying on the KV plugin to setup the object and storage service for the internal KV namespaced used by the local sercets store binding. However, this creates an issue when the users have both a KV namespace and secret binding defined with each creating a storage service sharing the same name while having a different persist path.

This decouples kv plugin from the sercets store plugin by copying the implementation in the kv bindings with the service named customzied for the secret store plugin.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: covered by existing tests
  • Wrangler / Vite E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bugfix
  • Wrangler V3 Backport
    • TODO (before merge)
    • Wrangler PR:
    • Not necessary because: v4 feature

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 30, 2025

🦋 Changeset detected

Latest commit: 9ad0ae2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 30, 2025

A Wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-wrangler-9106
Prereleases for other packages:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-workers-bindings-extension-9106 -O ./cloudflare-workers-bindings-extension.0.0.0-vfac1e8d94.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vfac1e8d94.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-create-cloudflare-9106 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-kv-asset-handler-9106

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-miniflare-9106

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-pages-shared-9106

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-unenv-preset-9106

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-vite-plugin-9106

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-vitest-pool-workers-9106

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-workers-editor-shared-9106

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-workers-shared-9106

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/15183895082/npm-package-cloudflare-workflows-shared-9106

Note that these links will no longer work once the GitHub Actions artifact expires.

@edmundhung edmundhung added e2e Run wrangler + vite-plugin e2e tests on a PR every-os labels Apr 30, 2025
@edmundhung edmundhung marked this pull request as ready for review May 16, 2025 12:44
@edmundhung edmundhung requested a review from a team as a code owner May 16, 2025 12:44
@edmundhung edmundhung changed the title refactor(miniflare): decouple kv plugin from the sercets store plugin fix(miniflare): decouple KV plugin from Secrets Store plugin to avoid service name conflicts May 16, 2025
@edmundhung edmundhung force-pushed the edmundhung/refactor-secret-store-plugin branch from c13df86 to a2aa378 Compare May 19, 2025 09:09
Copy link
Copy Markdown
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🙂

I think this could use a test where you do check that users can use the secrets store alongside a KV binding, but maybe that's a bit of an overkill 🤔

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk May 20, 2025
@edmundhung edmundhung added this pull request to the merge queue May 22, 2025
@edmundhung edmundhung removed this pull request from the merge queue due to a manual request May 22, 2025
Comment on lines +373 to +376
# Regression test for https://github.com/cloudflare/workers-sdk/issues/9006
kv_namespaces = [
${isLocal ? `{ binding = "KV", id = "LOCAL_ONLY" }` : ""}
]
Copy link
Copy Markdown
Member Author

@edmundhung edmundhung May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a regression test in the PR and ran the same test without the fix here: https://github.com/cloudflare/workers-sdk/actions/runs/15183927642/job/42699817229?pr=9338#step:5:4996

@edmundhung edmundhung added this pull request to the merge queue May 22, 2025
Merged via the queue into main with commit e5ae13a May 22, 2025
24 checks passed
@edmundhung edmundhung deleted the edmundhung/refactor-secret-store-plugin branch May 22, 2025 12:38
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run wrangler + vite-plugin e2e tests on a PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

vite-plugin: secrets store bindings throws Secret "[some secret key]" not found

3 participants