Simplify secret:bulk api via script settings #4179
Conversation
🦋 Changeset detectedLatest commit: 4653073 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
matthewdavidrodgers
force-pushed
the
matthewrodgers/bulk-secrets
branch
from
cfddc65
to
334d53d
Compare
|
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/6629624085/npm-package-wrangler-4179You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6629624085/npm-package-wrangler-4179Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6629624085/npm-package-wrangler-4179 dev path/to/script.jsAdditional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6629624085/npm-package-cloudflare-pages-shared-4179Note that these links will no longer work once the GitHub Actions artifact expires.
| Please ensure constraints are pinned, and |
Codecov Report
@@ Coverage Diff @@
## main #4179 +/- ##
==========================================
- Coverage 75.32% 75.32% -0.01%
==========================================
Files 223 223
Lines 12280 12294 +14
Branches 3177 3180 +3
==========================================
+ Hits 9250 9260 +10
- Misses 3030 3034 +4
|
matthewdavidrodgers
force-pushed
the
matthewrodgers/bulk-secrets
branch
2 times, most recently
from
94563b6
to
cb6ef4c
Compare
Firing PUTs to the secret api in parallel has never been a great solution -
each request independently needs to lock the script, so running in parallel
is at best just as bad as running serially.
Luckily, we have the script settings PATCH api now, which can update the
settings for a script (including secret bindings) at once, which means we
don't need any parallelization. However this api doesn't work with a
partial list of bindings, so we have to fetch the current bindings and
merge in with the new secrets before PATCHing. We can however just omit
the value of the binding (i.e. only provide the name and type) which
instructs the config service to inherit the existing value, which
simplifies this as well. Note that we don't use the bindings in your
current wrangler.toml, as you could be in a draft state, and it makes
sense as a user that a bulk secrets update won't update anything else.
Instead, we use script settings api again to fetch the current state of
your bindings.
This simplified implementation means the operation can only fail or
succeed, rather than succeeding in updating some secrets but failing for
others. In order to not introduce breaking changes for logging output,
the language around "${x} secrets were updated" or "${x} secrets failed"
is kept, even if it doesn't make much sense anymore.
matthewdavidrodgers
force-pushed
the
matthewrodgers/bulk-secrets
branch
from
cb6ef4c
to
4653073
Compare
|
I have a slight concern here around race conditions with pulling down settings and then uploading them. Of course, this is what the dashboard does too, so it's not a major concern, but I wonder if it might be possible to support a way to patch the set of bindings accessible to a worker, rather than just replacing them? |
|
it's true, we open up to race conditions here possibly, but A) the race condition can only be with bindings, not script content or any other metadata and B) yeah, the dash is already subject to much worse race conditions. given that we frequently see bulk secret updates take 20s+ in EWC, this feels worth it to me |
Firing PUTs to the secret api in parallel has never been a great solution - each request independently needs to lock the script, so running in parallel is at best just as bad as running serially.
Luckily, we have the script settings PATCH api now, which can update the settings for a script (including secret bindings) at once, which means we don't need any parallelization. However this api doesn't work with a partial list of bindings, so we have to fetch the current bindings and merge in with the new secrets before PATCHing. We can however just omit the value of the binding (i.e. only provide the name and type) which instructs the config service to inherit the existing value, which simplifies this as well. Note that we don't use the bindings in your current wrangler.toml, as you could be in a draft state, and it makes sense as a user that a bulk secrets update won't update anything else. Instead, we use script settings api again to fetch the current state of your bindings.
This simplified implementation means the operation can only fail or succeed, rather than succeeding in updating some secrets but failing for others. In order to not introduce breaking changes for logging output, the language around "${x} secrets were updated" or "${x} secrets failed" is kept, even if it doesn't make much sense anymore.
Author has included the following, where applicable:
Reviewer is to perform the following, as applicable:
Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr reviewso future reviewers can take inspiration and learn from it.