fix: route deployment fallback for restricted permission API tokens #1188
Conversation
🦋 Changeset detectedLatest commit: 70e220a 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 |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2461310367/npm-package-wrangler-1188You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1188/npm-package-wrangler-1188Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2461310367/npm-package-wrangler-1188 dev path/to/script.js |
petebacondarwin
force-pushed
the
route-permission-fallback
branch
from
69fa0b6 to
1fae2c3
Compare
This makes it available to use elsewhere in the code.
While we wait for changes to the CF API to support API tokens that do not have "All Zone" permissions, this change provides a workaround for most scenarios. If the bulk-route request fails with an authorization error, then we fallback to the Wrangler 1 approach, which sends individual route updates via a zone-based endpoint. Fixes cloudflare#651
petebacondarwin
force-pushed
the
route-permission-fallback
branch
from
1fae2c3 to
7e11bf8
Compare
petebacondarwin
changed the title
| continue; | ||
| } else { | ||
| throw new Error( | ||
| `The route with pattern "${routePattern}" is already associated with another worker called "${knownScript}".` |
There was a problem hiding this comment.
I suspect the api will already throw an error if this is true , so I wonder whether we should throw an error here
There was a problem hiding this comment.
It is true that the API will throw an error but this will only contain the error code 10020 and the message workers.api.error.duplicate_route [code: 10020].
We would need to add code to catch errors when making the API call, then to filter out that error, and even then we would need to do a look up on the allRoutes map in order to work out the name of the worker that currently holds this route.
So we would end up with about the same amount of code, we would be relying upon parsing the error message from the API and still need to work out the worker name in order to provide a helpful error message. In addition the API would get hit unnecessarily.
So I don't think we should make the suggested change here.
There was a problem hiding this comment.
Moreover, waiting for the API to fail might result in some routes being deployed and some not. By failing early we are consistently only deploying routes if all are valid.
packages/wrangler/src/publish.ts
Outdated
| async function publishRoutes( | ||
| workerUrl: string, | ||
| routes: Route[], | ||
| scriptName: string, | ||
| notProd: boolean | ||
| ): Promise<string[]> { |
There was a problem hiding this comment.
This function signature is a bit weird, how about:
scriptName: string,
routes: Route[],
{
accountId:string,
legacyEnv: boolean,
env: string | undefined
}We'll also have to make a helper to generate workerUrl, that should also be reused in publishCustomDomains() (which should also change it s signature)
There was a problem hiding this comment.
Such a refactoring is a larger effort outside of the scope of this PR.
workerUrl is used in a number of places throughout the code of this file.
Let's consider that in a follow up.
I will rearrange these parameters to make it a bit less weird for now.
packages/wrangler/src/publish.ts
Outdated
| routes: Route[], | ||
| scriptName: string, | ||
| notProd: boolean |
There was a problem hiding this comment.
like above, I think the signature here should be
scriptName: string,
routes: Route[],
{
legacyEnv: boolean,
env: string | undefined
}
It could be worth reviewing this commit by commit, given that quite a few lines changed.
Fixes #651