From 184b547bd2e2e1a2886a41fa0fe3e91cffc28c2e Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 10 Apr 2022 11:27:37 -0400 Subject: [PATCH] feat: optionally send zone_id with a route This enables optionally passing a route as `{pattern: string, zone_id: string}`. There are scenarios where we need to explicitly pass a zone_id to the api, so this is enables that. Some nuance: our toml parser unfortunately doesn't let you have mixed types in arrays, so if you have a single route with this special shape, then you'll have to do it for all of them. We can replace it with something more recent, but I'll do that in a subsequent PR. Further nuance: The errors from the api aren't super useful when invalid values are passed, but that's something to further work on. This also fixes some types in our cli parsing. Fixes https://github.com/cloudflare/wrangler2/issues/774 --- .changeset/dull-jobs-return.md | 13 ++ .../src/__tests__/configuration.test.ts | 31 ++++- packages/wrangler/src/__tests__/dev.test.tsx | 15 +++ .../wrangler/src/__tests__/publish.test.ts | 91 +++++++++++++- packages/wrangler/src/config/config.ts | 4 +- packages/wrangler/src/config/environment.ts | 4 +- packages/wrangler/src/config/validation.ts | 111 +++++++++++++++--- packages/wrangler/src/index.tsx | 57 +++++---- packages/wrangler/src/publish.ts | 38 +++--- 9 files changed, 295 insertions(+), 69 deletions(-) create mode 100644 .changeset/dull-jobs-return.md diff --git a/.changeset/dull-jobs-return.md b/.changeset/dull-jobs-return.md new file mode 100644 index 000000000000..b99e3c84651d --- /dev/null +++ b/.changeset/dull-jobs-return.md @@ -0,0 +1,13 @@ +--- +"wrangler": patch +--- + +feat: optionally send zone_id with a route + +This enables optionally passing a route as `{pattern: string, zone_id: string}`. There are scenarios where we need to explicitly pass a zone_id to the api, so this is enables that. + +Some nuance: The errors from the api aren't super useful when invalid values are passed, but that's something to further work on. + +This also fixes some types in our cli parsing. + +Fixes https://github.com/cloudflare/wrangler2/issues/774 diff --git a/packages/wrangler/src/__tests__/configuration.test.ts b/packages/wrangler/src/__tests__/configuration.test.ts index 5b3ea1049c64..f738d4e76bb7 100644 --- a/packages/wrangler/src/__tests__/configuration.test.ts +++ b/packages/wrangler/src/__tests__/configuration.test.ts @@ -553,7 +553,12 @@ describe("normalizeAndValidateConfig()", () => { compatibility_date: "2022-01-01", compatibility_flags: ["FLAG1", "FLAG2"], workers_dev: false, - routes: ["ROUTE_1", "ROUTE_2"], + routes: [ + "ROUTE_1", + "ROUTE_2", + { pattern: "ROUTE3", zone_id: "ZONE_ID_3" }, + "ROUTE_4", + ], jsx_factory: "JSX_FACTORY", jsx_fragment: "JSX_FRAGMENT", tsconfig: "path/to/tsconfig", @@ -630,7 +635,21 @@ describe("normalizeAndValidateConfig()", () => { compatibility_date: 333, compatibility_flags: [444, 555], workers_dev: "BAD", - routes: [666, 777], + routes: [ + 666, + 777, + // this one's valid + "example.com/*", + { pattern: 123, zone_id: "zone_id_1" }, + { pattern: "route_2", zone_id: 123 }, + { pattern: "route_3" }, + { zone_id: "zone_id_4" }, + { pattern: undefined }, + { pattern: "route_5", zone_id: "zone_id_5", some_other_key: 123 }, + // this one's actually valid, but let's make sure + // it gets included in the error anyway + { pattern: "route_6", zone_id: "zone_id_6" }, + ], route: 888, jsx_factory: 999, jsx_fragment: 1000, @@ -655,8 +674,8 @@ describe("normalizeAndValidateConfig()", () => { expect(diagnostics.hasWarnings()).toBe(false); expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` "Processing wrangler configuration: - - Expected \\"route\\" to be of type string but got 888. - - Expected \\"routes\\" to be of type string array but got [666,777]. + - Expected \\"route\\" to be either a string or an object with shape {pattern: string, zone_id: string}, but got 888. + - Expected \\"routes\\" to be an array of either strings or objects with the shape {pattern: string, zone_id: string}, but these weren't valid: [666,777,{\\"pattern\\":123,\\"zone_id\\":\\"zone_id_1\\"},{\\"pattern\\":\\"route_2\\",\\"zone_id\\":123},{\\"pattern\\":\\"route_3\\"},{\\"zone_id\\":\\"zone_id_4\\"},{},{\\"pattern\\":\\"route_5\\",\\"zone_id\\":\\"zone_id_5\\",\\"some_other_key\\":123}]. - Expected exactly one of the following fields [\\"routes\\",\\"route\\"]. - Expected \\"workers_dev\\" to be of type boolean but got \\"BAD\\". - Expected \\"build.command\\" to be of type string but got 1444. @@ -1797,8 +1816,8 @@ describe("normalizeAndValidateConfig()", () => { "Processing wrangler configuration: - \\"env.ENV1\\" environment configuration - - Expected \\"route\\" to be of type string but got 888. - - Expected \\"routes\\" to be of type string array but got [666,777]. + - Expected \\"route\\" to be either a string or an object with shape {pattern: string, zone_id: string}, but got 888. + - Expected \\"routes\\" to be an array of either strings or objects with the shape {pattern: string, zone_id: string}, but these weren't valid: [666,777]. - Expected exactly one of the following fields [\\"routes\\",\\"route\\"]. - Expected \\"workers_dev\\" to be of type boolean but got \\"BAD\\". - Expected \\"build.command\\" to be of type string but got 1444. diff --git a/packages/wrangler/src/__tests__/dev.test.tsx b/packages/wrangler/src/__tests__/dev.test.tsx index 37b3b1f73bc1..e22e395ca638 100644 --- a/packages/wrangler/src/__tests__/dev.test.tsx +++ b/packages/wrangler/src/__tests__/dev.test.tsx @@ -218,6 +218,21 @@ describe("wrangler dev", () => { ); }); + it("should, when provided, use a configured zone_id", async () => { + writeWranglerToml({ + main: "index.js", + routes: [ + { pattern: "https://some-domain.com/*", zone_id: "some-zone-id" }, + ], + }); + fs.writeFileSync("index.js", `export default {};`); + await runWrangler("dev"); + expect((Dev as jest.Mock).mock.calls[0][0].zone).toEqual({ + host: "some-domain.com", + id: "some-zone-id", + }); + }); + it("given a long host, it should use the longest subdomain that resolves to a zone", async () => { writeWranglerToml({ main: "index.js", diff --git a/packages/wrangler/src/__tests__/publish.test.ts b/packages/wrangler/src/__tests__/publish.test.ts index a5ff5ac24b57..4168ddead13f 100644 --- a/packages/wrangler/src/__tests__/publish.test.ts +++ b/packages/wrangler/src/__tests__/publish.test.ts @@ -311,6 +311,91 @@ describe("publish", () => { await runWrangler("publish ./index"); }); + it("should publish to a route with a pattern/zone_id combo", async () => { + writeWranglerToml({ + routes: [ + "some-example.com/some-route/*", + { pattern: "*a-boring-website.com", zone_id: "54sdf7fsda" }, + { pattern: "example.com/some-route/*", zone_id: "JGHFHG654gjcj" }, + "more-examples.com/*", + ], + }); + writeWorkerSource(); + mockUpdateWorkerRequest({ enabled: false }); + mockUploadWorkerRequest({ expectedType: "esm" }); + mockPublishRoutesRequest({ + routes: [ + "some-example.com/some-route/*", + { pattern: "*a-boring-website.com", zone_id: "54sdf7fsda" }, + { pattern: "example.com/some-route/*", zone_id: "JGHFHG654gjcj" }, + "more-examples.com/*", + ], + }); + await runWrangler("publish ./index"); + expect(std).toMatchInlineSnapshot(` + Object { + "err": "", + "out": "Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + some-example.com/some-route/* + *a-boring-website.com (zone: 54sdf7fsda) + example.com/some-route/* (zone: JGHFHG654gjcj) + more-examples.com/*", + "warn": "", + } + `); + }); + + it("should publish to a route with a pattern/zone_id combo (service environments)", async () => { + writeWranglerToml({ + env: { + staging: { + routes: [ + "some-example.com/some-route/*", + { pattern: "*a-boring-website.com", zone_id: "54sdf7fsda" }, + { pattern: "example.com/some-route/*", zone_id: "JGHFHG654gjcj" }, + "more-examples.com/*", + ], + }, + }, + }); + mockSubDomainRequest(); + writeWorkerSource(); + mockUpdateWorkerRequest({ + enabled: false, + env: "staging", + legacyEnv: false, + }); + mockUploadWorkerRequest({ + expectedType: "esm", + env: "staging", + legacyEnv: false, + }); + mockPublishRoutesRequest({ + routes: [ + "some-example.com/some-route/*", + { pattern: "*a-boring-website.com", zone_id: "54sdf7fsda" }, + { pattern: "example.com/some-route/*", zone_id: "JGHFHG654gjcj" }, + "more-examples.com/*", + ], + env: "staging", + legacyEnv: false, + }); + await runWrangler("publish ./index --legacy-env false --env staging"); + expect(std).toMatchInlineSnapshot(` + Object { + "err": "", + "out": "Uploaded test-name (staging) (TIMINGS) + Published test-name (staging) (TIMINGS) + some-example.com/some-route/* + *a-boring-website.com (zone: 54sdf7fsda) + example.com/some-route/* (zone: JGHFHG654gjcj) + more-examples.com/*", + "warn": "", + } + `); + }); + it("should publish to legacy environment specific routes", async () => { writeWranglerToml({ routes: ["example.com/some-route/*"], @@ -3829,7 +3914,7 @@ function mockPublishRoutesRequest({ env = undefined, legacyEnv = false, }: { - routes: string[]; + routes: (string | { pattern: string; zone_id: string })[]; env?: string | undefined; legacyEnv?: boolean | undefined; }) { @@ -3849,7 +3934,9 @@ function mockPublishRoutesRequest({ } expect(JSON.parse(body as string)).toEqual( - routes.map((pattern) => ({ pattern })) + routes.map((route) => + typeof route !== "object" ? { pattern: route } : route + ) ); return null; } diff --git a/packages/wrangler/src/config/config.ts b/packages/wrangler/src/config/config.ts index 92d89847746a..8b7717e3ca29 100644 --- a/packages/wrangler/src/config/config.ts +++ b/packages/wrangler/src/config/config.ts @@ -183,9 +183,9 @@ export interface DevConfig { upstream_protocol: "https" | "http"; /** - * Host to forward requests to, defaults to the zone of project + * Host to forward requests to, defaults to the host of the first route of project */ - host: string | undefined; + host: (string | { pattern: string; zone_id: string }) | undefined; } export type RawDevConfig = Partial; diff --git a/packages/wrangler/src/config/environment.ts b/packages/wrangler/src/config/environment.ts index 9a7eedd1e778..f939d1291407 100644 --- a/packages/wrangler/src/config/environment.ts +++ b/packages/wrangler/src/config/environment.ts @@ -74,7 +74,7 @@ interface EnvironmentInheritable { * * @inheritable */ - routes: string[] | undefined; + routes: (string | { pattern: string; zone_id: string })[] | undefined; /** * A route that your worker should be published to. Literally @@ -85,7 +85,7 @@ interface EnvironmentInheritable { * * @inheritable */ - route: string | undefined; + route: (string | { pattern: string; zone_id: string }) | undefined; /** * Path to a custom tsconfig diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts index 7663966bd9ee..7c8bd7c5b0de 100644 --- a/packages/wrangler/src/config/validation.ts +++ b/packages/wrangler/src/config/validation.ts @@ -461,6 +461,97 @@ function normalizeAndValidateModulePaths( return mapping; } +/** + * Check whether a value has the shape of a route, which can be a string + * or an object that looks like {pattern: string, zone_id: string } + */ +function isValidRouteValue(item: unknown): boolean { + return ( + !!item && + (typeof item === "string" || + (typeof item === "object" && + hasProperty(item, "pattern") && + typeof item.pattern === "string" && + hasProperty(item, "zone_id") && + typeof item.zone_id === "string" && + Object.keys(item).length === 2)) + ); +} + +/** + * Validate that the field is a route. + */ +const isRoute: ValidatorFn = (diagnostics, field, value) => { + if (value !== undefined && !isValidRouteValue(value)) { + diagnostics.errors.push( + `Expected "${field}" to be either a string or an object with shape {pattern: string, zone_id: string}, but got ${JSON.stringify( + value + )}.` + ); + return false; + } + return true; +}; + +/** + * Validate that the field is an array of routes. + */ +const isRouteArray: ValidatorFn = (diagnostics, field, value) => { + if (value === undefined) { + return true; + } + if (!Array.isArray(value)) { + diagnostics.errors.push( + `Expected "${field}" to be an array but got ${JSON.stringify(value)}.` + ); + return false; + } + const invalidRoutes = []; + for (const item of value) { + if (!isValidRouteValue(item)) { + invalidRoutes.push(item); + } + } + if (invalidRoutes.length > 0) { + diagnostics.errors.push( + `Expected "${field}" to be an array of either strings or objects with the shape {pattern: string, zone_id: string}, but these weren't valid: ${JSON.stringify( + invalidRoutes + )}.` + ); + } + return invalidRoutes.length === 0; +}; + +function validateRoute( + diagnostics: Diagnostics, + topLevelEnv: Environment | undefined, + rawEnv: RawEnvironment +): Config["route"] { + return inheritable( + diagnostics, + topLevelEnv, + rawEnv, + "route", + isRoute, + undefined + ); +} + +function validateRoutes( + diagnostics: Diagnostics, + topLevelEnv: Environment | undefined, + rawEnv: RawEnvironment +): Config["routes"] { + return inheritable( + diagnostics, + topLevelEnv, + rawEnv, + "routes", + all(isRouteArray, isMutuallyExclusiveWith(rawEnv, "route")), + undefined + ); +} + /** * Validate top-level environment configuration and return the normalized values. */ @@ -526,22 +617,10 @@ function normalizeAndValidateEnvironment( experimental(diagnostics, rawEnv, "unsafe"); - const route = inheritable( - diagnostics, - topLevelEnv, - rawEnv, - "route", - isString, - undefined - ); - const routes = inheritable( - diagnostics, - topLevelEnv, - rawEnv, - "routes", - all(isStringArray, isMutuallyExclusiveWith(rawEnv, "route")), - undefined - ); + const route = validateRoute(diagnostics, topLevelEnv, rawEnv); + + const routes = validateRoutes(diagnostics, topLevelEnv, rawEnv); + const workers_dev = inheritable( diagnostics, topLevelEnv, diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx index f38da3135211..b08da5f3f16c 100644 --- a/packages/wrangler/src/index.tsx +++ b/packages/wrangler/src/index.tsx @@ -648,8 +648,9 @@ export async function main(argv: string[]): Promise { }) .option("compatibility-flags", { describe: "Flags to use for compatibility checks", - type: "array", alias: "compatibility-flag", + type: "string", + array: true, }) .option("latest", { describe: "Use the latest version of the worker runtime", @@ -671,7 +672,8 @@ export async function main(argv: string[]): Promise { .option("routes", { describe: "Routes to upload", alias: "route", - type: "array", + type: "string", + array: true, }) .option("host", { type: "string", @@ -751,8 +753,7 @@ export async function main(argv: string[]): Promise { } const upstreamProtocol = - (args["upstream-protocol"] as "http" | "https") || - config.dev.upstream_protocol; + args["upstream-protocol"] || config.dev.upstream_protocol; if (upstreamProtocol === "http") { console.warn( "Setting upstream-protocol to http is not currently implemented.\n" + @@ -810,19 +811,29 @@ export async function main(argv: string[]): Promise { (args.routes && args.routes[0]) || config.route || (config.routes && config.routes[0]); + + let zoneId: string | undefined = + typeof hostLike === "object" ? hostLike.zone_id : undefined; + const host = - typeof hostLike === "string" ? getHost(hostLike) : undefined; - let zoneId: string | undefined; + typeof hostLike === "string" + ? getHost(hostLike) + : typeof hostLike === "object" + ? getHost(hostLike.pattern) + : undefined; + const hostPieces = typeof host === "string" ? host.split(".") : undefined; - while (hostPieces && hostPieces.length > 1) { + + while (!zoneId && hostPieces && hostPieces.length > 1) { zoneId = await getZoneId(hostPieces.join(".")); - if (zoneId) break; hostPieces.shift(); } + if (host && !zoneId) { throw new Error(`Could not find zone for ${hostLike}`); } + zone = typeof zoneId === "string" && typeof host === "string" ? { @@ -846,12 +857,7 @@ export async function main(argv: string[]): Promise { jsxFragment={args["jsx-fragment"] || config.jsx_fragment} tsconfig={args.tsconfig ?? config.tsconfig} upstreamProtocol={upstreamProtocol} - localProtocol={ - // The typings are not quite clever enough to handle element accesses, only property accesses, - // so we need to cast here. - (args["local-protocol"] as "http" | "https") || - config.dev.local_protocol - } + localProtocol={args["local-protocol"] || config.dev.local_protocol} enableLocalPersistence={ args["experimental-enable-local-persistence"] || false } @@ -875,8 +881,7 @@ export async function main(argv: string[]): Promise { args["compatibility-date"] )} compatibilityFlags={ - (args["compatibility-flags"] as string[]) || - config.compatibility_flags + args["compatibility-flags"] || config.compatibility_flags } usageModel={config.usage_model} bindings={{ @@ -945,8 +950,9 @@ export async function main(argv: string[]): Promise { }) .option("compatibility-flags", { describe: "Flags to use for compatibility checks", - type: "array", alias: "compatibility-flag", + type: "string", + array: true, }) .option("latest", { describe: "Use the latest version of the worker runtime", @@ -976,12 +982,14 @@ export async function main(argv: string[]): Promise { .option("triggers", { describe: "cron schedules to attach", alias: ["schedule", "schedules"], - type: "array", + type: "string", + array: true, }) .option("routes", { describe: "Routes to upload", alias: "route", - type: "array", + type: "string", + array: true, }) .option("jsx-factory", { describe: "The function that is called for each JSX element", @@ -1039,7 +1047,7 @@ export async function main(argv: string[]): Promise { compatibilityDate: args.latest ? new Date().toISOString().substring(0, 10) : args["compatibility-date"], - compatibilityFlags: args["compatibility-flags"] as string[], + compatibilityFlags: args["compatibility-flags"], triggers: args.triggers, jsxFactory: args["jsx-factory"], jsxFragment: args["jsx-fragment"], @@ -1126,7 +1134,7 @@ export async function main(argv: string[]): Promise { const accountId = await requireAuth(config); const cliFilters: TailCLIFilters = { - status: args.status as ("ok" | "error" | "canceled")[], + status: args.status as ("ok" | "error" | "canceled")[] | undefined, header: args.header, method: args.method, samplingRate: args["sampling-rate"], @@ -1255,10 +1263,7 @@ export async function main(argv: string[]): Promise { ip={config.dev.ip} public={undefined} compatibilityDate={getDevCompatibilityDate(config)} - compatibilityFlags={ - (args["compatibility-flags"] as string[]) || - config.compatibility_flags - } + compatibilityFlags={config.compatibility_flags} usageModel={config.usage_model} bindings={{ kv_namespaces: config.kv_namespaces?.map( @@ -1493,7 +1498,7 @@ export async function main(argv: string[]): Promise { return ( typeof e === "object" && e !== null && - (e as { code: 10007 }).code === 10007 + (e as { code: number }).code === 10007 ); } diff --git a/packages/wrangler/src/publish.ts b/packages/wrangler/src/publish.ts index 622c24991e39..0f773ce8703f 100644 --- a/packages/wrangler/src/publish.ts +++ b/packages/wrangler/src/publish.ts @@ -22,12 +22,12 @@ type Props = { compatibilityDate: string | undefined; compatibilityFlags: string[] | undefined; assetPaths: AssetPaths | undefined; - triggers: (string | number)[] | undefined; - routes: (string | number)[] | undefined; + triggers: string[] | undefined; + routes: string[] | undefined; legacyEnv: boolean | undefined; - jsxFactory: undefined | string; - jsxFragment: undefined | string; - tsconfig: undefined | string; + jsxFactory: string | undefined; + jsxFragment: string | undefined; + tsconfig: string | undefined; experimentalPublic: boolean; }; @@ -302,11 +302,13 @@ export default async function publish(props: Props): Promise { if (routes.length > 0) { deployments.push( fetchResult(`${workerUrl}/routes`, { - // TODO: PATCH will not delete previous routes on this script, - // whereas PUT will. We need to decide on the default behaviour - // and how to configure it. + // Note: PUT will delete previous routes on this script. method: "PUT", - body: JSON.stringify(routes.map((pattern) => ({ pattern }))), + body: JSON.stringify( + routes.map((route) => + typeof route !== "object" ? { pattern: route } : route + ) + ), headers: { "Content-Type": "application/json", }, @@ -314,10 +316,18 @@ export default async function publish(props: Props): Promise { if (routes.length > 10) { return routes .slice(0, 9) - .map(String) + .map((route) => + typeof route === "string" + ? route + : `${route.pattern} (zone: ${route.zone_id})` + ) .concat([`...and ${routes.length - 10} more routes`]); } - return routes.map(String); + return routes.map((route) => + typeof route === "string" + ? route + : `${route.pattern} (zone: ${route.zone_id})` + ); }) ); } @@ -327,15 +337,13 @@ export default async function publish(props: Props): Promise { if (triggers && triggers.length) { deployments.push( fetchResult(`${workerUrl}/schedules`, { - // TODO: Unlike routes, this endpoint does not support PATCH. - // So technically, this will override any previous schedules. - // We should change the endpoint to support PATCH. + // Note: PUT will override previous schedules on this script. method: "PUT", body: JSON.stringify(triggers.map((cron) => ({ cron }))), headers: { "Content-Type": "application/json", }, - }).then(() => triggers.map(String)) + }).then(() => triggers.map((trigger) => `schedule: ${trigger}`)) ); }