Skip to content

Commit

Permalink
fix: use account id for listing zones
Browse files Browse the repository at this point in the history
Fixes #4944

Trying. to fetch `/zones` fails when it spans more than 500 zones. The fix to use an account id when doing so. This patch passes the account id to the zones call, threading it through all the functions that require it.
  • Loading branch information
threepointone committed Jun 28, 2024
1 parent 7ed675e commit 61d6424
Show file tree
Hide file tree
Showing 14 changed files with 149 additions and 57 deletions.
9 changes: 9 additions & 0 deletions .changeset/grumpy-bikes-prove.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"wrangler": patch
---

fix: use account id for listing zones

Fixes https://github.com/cloudflare/workers-sdk/issues/4944

Trying to fetch `/zones` fails when it spans more than 500 zones. The fix to use an account id when doing so. This patch passes the account id to the zones call, threading it through all the functions that require it.
5 changes: 4 additions & 1 deletion packages/wrangler/e2e/helpers/wrangler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export class WranglerLongLivedCommand extends LongLivedCommand {
}

async waitForReady(): Promise<{ url: string }> {
const match = await this.readUntil(/Ready on (?<url>https?:\/\/.*)/, 5_000);
const match = await this.readUntil(
/Ready on (?<url>https?:\/\/.*)/,
10_000
);
return match.groups as { url: string };
}

Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/e2e/versions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ describe("versions deploy", { timeout: TIMEOUT }, () => {
├ Finding latest stable Worker Version to rollback to
? Please provide an optional message for this rollback (120 characters max)?
? Please provide an optional message for this rollback (120 characters max)
🤖 Using default value in non-interactive context: Rollback via e2e test
├ WARNING You are about to rollback to Worker Version 00000000-0000-0000-0000-000000000000.
Expand Down Expand Up @@ -425,7 +425,7 @@ describe("versions deploy", { timeout: TIMEOUT }, () => {
│ Tag: e2e-upload
│ Message: Upload via e2e test
? Please provide an optional message for this rollback (120 characters max)?
? Please provide an optional message for this rollback (120 characters max)
🤖 Using default value in non-interactive context: Rollback to old version
├ WARNING You are about to rollback to Worker Version 00000000-0000-0000-0000-000000000000.
Expand Down
11 changes: 9 additions & 2 deletions packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10400,12 +10400,19 @@ function mockUnauthorizedPublishRoutesRequest({
);
}

function mockGetZones(domain: string, zones: { id: string }[] = []) {
function mockGetZones(
domain: string,
zones: { id: string }[] = [],
accountId = "some-account-id"
) {
msw.use(
http.get("*/zones", ({ request }) => {
const url = new URL(request.url);

expect([...url.searchParams.entries()]).toEqual([["name", domain]]);
expect([...url.searchParams.entries()]).toEqual([
["name", domain],
["account.id", accountId],
]);

return HttpResponse.json(
{
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/__tests__/rollback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe("rollback", () => {
mockPostDeployment();

mockPrompt({
text: "Please provide a message for this rollback (120 characters max, optional)?",
text: "Please provide an optional message for this rollback (120 characters max)",
result: "Test rollback",
});

Expand Down Expand Up @@ -168,7 +168,7 @@ describe("rollback", () => {
mockPostDeployment(true);

mockPrompt({
text: "Please provide a message for this rollback (120 characters max, optional)?",
text: "Please provide an optional message for this rollback (120 characters max)",
result: "Test rollback",
});

Expand Down
56 changes: 42 additions & 14 deletions packages/wrangler/src/__tests__/zones.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,21 @@ import { getHostFromUrl, getZoneForRoute } from "../zones";
import { mockAccountId, mockApiToken } from "./helpers/mock-account-id";
import { msw } from "./helpers/msw";

function mockGetZones(domain: string, zones: { id: string }[] = []) {
function mockGetZones(
domain: string,
zones: { id: string }[] = [],
accountId = "some-account-id"
) {
msw.use(
http.get(
"*/zones",
({ request }) => {
const url = new URL(request.url);

expect([...url.searchParams.entries()]).toEqual([["name", domain]]);
expect([...url.searchParams.entries()]).toEqual([
["name", domain],
["account.id", accountId],
]);

return HttpResponse.json(
{
Expand Down Expand Up @@ -67,16 +74,22 @@ describe("Zones", () => {
describe("getZoneForRoute", () => {
test("string route", async () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(await getZoneForRoute("example.com/*")).toEqual({
expect(
await getZoneForRoute({
route: "example.com/*",
accountId: "some-account-id",
})
).toEqual({
host: "example.com",
id: "example-id",
});
});

test("string route (not a zone)", async () => {
mockGetZones("wrong.com", []);
await expect(getZoneForRoute("wrong.com/*")).rejects
.toMatchInlineSnapshot(`
await expect(
getZoneForRoute({ route: "wrong.com/*", accountId: "some-account-id" })
).rejects.toMatchInlineSnapshot(`
[Error: Could not find zone for \`wrong.com\`. Make sure the domain is set up to be proxied by Cloudflare.
For more details, refer to https://developers.cloudflare.com/workers/configuration/routing/routes/#set-up-a-route]
`);
Expand All @@ -86,7 +99,10 @@ describe("Zones", () => {
// when a zone_id is provided in the route
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({ pattern: "example.com/*", zone_id: "other-id" })
await getZoneForRoute({
route: { pattern: "example.com/*", zone_id: "other-id" },
accountId: "some-account-id",
})
).toEqual({
host: "example.com",
id: "other-id",
Expand All @@ -98,8 +114,11 @@ describe("Zones", () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "some.third-party.com/*",
zone_id: "other-id",
route: {
pattern: "some.third-party.com/*",
zone_id: "other-id",
},
accountId: "some-account-id",
})
).toEqual({
host: "some.third-party.com",
Expand All @@ -111,8 +130,11 @@ describe("Zones", () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "example.com/*",
zone_name: "example.com",
route: {
pattern: "example.com/*",
zone_name: "example.com",
},
accountId: "some-account-id",
})
).toEqual({
host: "example.com",
Expand All @@ -123,8 +145,11 @@ describe("Zones", () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "subdomain.example.com/*",
zone_name: "example.com",
route: {
pattern: "subdomain.example.com/*",
zone_name: "example.com",
},
accountId: "some-account-id",
})
).toEqual({
host: "subdomain.example.com",
Expand All @@ -135,8 +160,11 @@ describe("Zones", () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "some.third-party.com/*",
zone_name: "example.com",
route: {
pattern: "some.third-party.com/*",
zone_name: "example.com",
},
accountId: "some-account-id",
})
).toEqual({
host: "some.third-party.com",
Expand Down
22 changes: 18 additions & 4 deletions packages/wrangler/src/deploy/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,13 @@ export async function publishRoutes(
workerUrl,
scriptName,
notProd,
}: { workerUrl: string; scriptName: string; notProd: boolean }
accountId,
}: {
workerUrl: string;
scriptName: string;
notProd: boolean;
accountId: string;
}
): Promise<string[]> {
try {
return await fetchResult(`${workerUrl}/routes`, {
Expand All @@ -897,7 +903,11 @@ export async function publishRoutes(
if (isAuthenticationError(e)) {
// An authentication error is probably due to a known issue,
// where the user is logged in via an API token that does not have "All Zones".
return await publishRoutesFallback(routes, { scriptName, notProd });
return await publishRoutesFallback(routes, {
scriptName,
notProd,
accountId,
});
} else {
throw e;
}
Expand All @@ -911,7 +921,11 @@ export async function publishRoutes(
*/
async function publishRoutesFallback(
routes: Route[],
{ scriptName, notProd }: { scriptName: string; notProd: boolean }
{
scriptName,
notProd,
accountId,
}: { scriptName: string; notProd: boolean; accountId: string }
) {
if (notProd) {
throw new UserError(
Expand All @@ -932,7 +946,7 @@ async function publishRoutesFallback(
const activeZones = new Map<string, string>();
const routesToDeploy = new Map<string, string>();
for (const route of routes) {
const zone = await getZoneForRoute(route);
const zone = await getZoneForRoute({ route, accountId });
if (zone) {
activeZones.set(zone.id, zone.host);
routesToDeploy.set(
Expand Down
5 changes: 4 additions & 1 deletion packages/wrangler/src/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,10 @@ export async function validateDevServerSettings(
// we want it to exit the Wrangler process early to allow the user to fix it. Calling it here forces
// the error to be thrown where it will correctly exit the Wrangler process
if (args.remote) {
await getZoneIdForPreview(host, routes);
const accountId =
args.accountId ?? config.account_id ?? getAccountFromCache()?.id;
assert(accountId, "Account ID must be provided for remote dev");
await getZoneIdForPreview({ host, routes, accountId });
}
const initialIp = args.ip || config.dev.ip;
const initialIpListenCheck = initialIp === "*" ? "0.0.0.0" : initialIp;
Expand Down
6 changes: 5 additions & 1 deletion packages/wrangler/src/dev/remote.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,11 @@ export async function getWorkerAccountAndContext(props: {
};

// What zone should the realish preview for this Worker run on?
const zoneId = await getZoneIdForPreview(props.host, props.routes);
const zoneId = await getZoneIdForPreview({
host: props.host,
routes: props.routes,
accountId: props.accountId,
});

const workerContext: CfWorkerContext = {
env: props.env,
Expand Down
9 changes: 6 additions & 3 deletions packages/wrangler/src/tail/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,14 @@ export async function tailHandler(args: TailArgs) {

let scriptName;

const accountId = await requireAuth(config);

// Worker names can't contain "." (and most routes should), so use that as a discriminator
if (args.worker?.includes(".")) {
scriptName = await getWorkerForZone(args.worker);
scriptName = await getWorkerForZone({
worker: args.worker,
accountId,
});
if (args.format === "pretty") {
logger.log(`Connecting to worker ${scriptName} at route ${args.worker}`);
}
Expand All @@ -115,8 +120,6 @@ export async function tailHandler(args: TailArgs) {
);
}

const accountId = await requireAuth(config);

const cliFilters: TailCLIFilters = {
status: args.status as ("ok" | "error" | "canceled")[] | undefined,
header: args.header,
Expand Down
9 changes: 7 additions & 2 deletions packages/wrangler/src/triggers/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export default async function triggersDeploy(props: Props): Promise<void> {
// to bother with all the error handling tomfoolery.
const routesWithOtherBindings: Record<string, string[]> = {};
for (const route of routes) {
const zone = await getZoneForRoute(route);
const zone = await getZoneForRoute({ route, accountId });
if (!zone) {
continue;
}
Expand Down Expand Up @@ -194,7 +194,12 @@ export default async function triggersDeploy(props: Props): Promise<void> {
// Update routing table for the script.
if (routesOnly.length > 0) {
deployments.push(
publishRoutes(routesOnly, { workerUrl, scriptName, notProd }).then(() => {
publishRoutes(routesOnly, {
workerUrl,
scriptName,
notProd,
accountId,
}).then(() => {
if (routesOnly.length > 10) {
return routesOnly
.slice(0, 9)
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/versions/rollback/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export async function versionsRollbackHandler(args: VersionsRollbackArgs) {
}));

const message = await prompt(
"Please provide an optional message for this rollback (120 characters max)?",
"Please provide an optional message for this rollback (120 characters max)",
{
defaultValue: args.message ?? "Rollback",
}
Expand Down
Loading

0 comments on commit 61d6424

Please sign in to comment.