Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use zone_name for deployment of custom hostnames #4232

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/rich-schools-fry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"wrangler": minor
---

fix: use `zone_name` to determine a zone when the pattern is a custom hostname

In Cloudflare for SaaS, custom hostnames of third party domain owners can be used in Cloudflare.
Workers are allowed to intercept these requests based on the routes configuration.
Before this change, the same logic used by `wrangler dev` was used in `wrangler deploy`, which caused wrangler to fail with:

✘ [ERROR] Could not find zone for [partner-saas-domain.com]
114 changes: 114 additions & 0 deletions packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,84 @@ describe("deploy", () => {
`);
});

it("should deploy to a route with a SaaS domain", async () => {
writeWranglerToml({
workers_dev: false,
routes: [
{
pattern: "partner.com/*",
zone_name: "owned-zone.com",
},
],
});
writeWorkerSource();
mockUpdateWorkerRequest({ enabled: false });
mockUploadWorkerRequest({ available_on_subdomain: false });
mockGetZones("owned-zone.com", [{ id: "owned-zone-id-1" }]);
mockGetWorkerRoutes("owned-zone-id-1");
mockPublishRoutesRequest({
routes: [
{
pattern: "partner.com/*",
zone_name: "owned-zone.com",
},
],
});
await runWrangler("deploy ./index");
expect(std).toMatchInlineSnapshot(`
Object {
"debug": "",
"err": "",
"info": "",
"out": "Total Upload: xx KiB / gzip: xx KiB
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
partner.com/* (zone name: owned-zone.com)
Current Deployment ID: Galaxy-Class",
"warn": "",
}
`);
});

it("should deploy to a route with a SaaS subdomain", async () => {
writeWranglerToml({
workers_dev: false,
routes: [
{
pattern: "subdomain.partner.com/*",
zone_name: "owned-zone.com",
},
],
});
writeWorkerSource();
mockUpdateWorkerRequest({ enabled: false });
mockUploadWorkerRequest({ available_on_subdomain: false });
mockGetZones("owned-zone.com", [{ id: "owned-zone-id-1" }]);
mockGetWorkerRoutes("owned-zone-id-1");
mockPublishRoutesRequest({
routes: [
{
pattern: "subdomain.partner.com/*",
zone_name: "owned-zone.com",
},
],
});
await runWrangler("deploy ./index");
expect(std).toMatchInlineSnapshot(`
Object {
"debug": "",
"err": "",
"info": "",
"out": "Total Upload: xx KiB / gzip: xx KiB
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
subdomain.partner.com/* (zone name: owned-zone.com)
Current Deployment ID: Galaxy-Class",
"warn": "",
}
`);
});

it("should deploy to a route with a pattern/{zone_id|zone_name} combo (service environments)", async () => {
writeWranglerToml({
env: {
Expand Down Expand Up @@ -8883,6 +8961,42 @@ function mockUnauthorizedPublishRoutesRequest({
);
}

function mockGetZones(domain: string, zones: { id: string }[] = []) {
msw.use(
rest.get("*/zones", (req, res, ctx) => {
expect([...req.url.searchParams.entries()]).toEqual([["name", domain]]);

return res(
ctx.status(200),
ctx.json({
success: true,
errors: [],
messages: [],
result: zones,
})
);
})
);
}

function mockGetWorkerRoutes(zoneId: string) {
msw.use(
rest.get("*/zones/:zoneId/workers/routes", (req, res, ctx) => {
expect(req.params.zoneId).toEqual(zoneId);

return res(
ctx.status(200),
ctx.json({
success: true,
errors: [],
messages: [],
result: [],
})
);
})
);
}

function mockPublishRoutesFallbackRequest(route: {
pattern: string;
script: string;
Expand Down
16 changes: 0 additions & 16 deletions packages/wrangler/src/__tests__/dev.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -369,22 +369,6 @@ describe("wrangler dev", () => {
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should fail for non-existing zones", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@penalosa
I've removed this UT, since it does not seem to be correct when having Cloudflare for SaaS product in mind.
Please, let me know if it is ok to remove it.

writeWranglerToml({
main: "index.js",
routes: [
{
pattern: "https://subdomain.does-not-exist.com/*",
zone_name: "exists.com",
},
],
});
await fs.promises.writeFile("index.js", `export default {};`);
await expect(runWrangler("dev --remote")).rejects.toEqual(
new Error("Could not find zone for subdomain.does-not-exist.com")
);
});

it("should fail for non-existing zones, when falling back from */*", async () => {
writeWranglerToml({
main: "index.js",
Expand Down
101 changes: 100 additions & 1 deletion packages/wrangler/src/__tests__/zones.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,23 @@
import { getHostFromUrl } from "../zones";
import { rest } from "msw";
import { getHostFromUrl, getZoneForRoute } from "../zones";
import { msw } from "./helpers/msw";
function mockGetZones(domain: string, zones: { id: string }[] = []) {
msw.use(
rest.get("*/zones", (req, res, ctx) => {
expect([...req.url.searchParams.entries()]).toEqual([["name", domain]]);

return res.once(
ctx.status(200),
ctx.json({
success: true,
errors: [],
messages: [],
result: zones,
})
);
})
);
}

describe("Zones", () => {
describe("getHostFromUrl", () => {
Expand Down Expand Up @@ -35,4 +54,84 @@ describe("Zones", () => {
expect(getHostFromUrl(pattern)).toBe(host);
});
});
describe("getZoneForRoute", () => {
test("string route", async () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(await getZoneForRoute("example.com/*")).toEqual({
host: "example.com",
id: "example-id",
});
});

test("string route (not a zone)", async () => {
mockGetZones("wrong.com", []);
await expect(
getZoneForRoute("wrong.com/*")
).rejects.toMatchInlineSnapshot(
`[Error: Could not find zone for wrong.com]`
);
});
test("zone_id route", async () => {
// example-id and other-id intentionally different to show that the API is not called
// 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" })
).toEqual({
host: "example.com",
id: "other-id",
});
});
test("zone_id route (custom hostname)", async () => {
// example-id and other-id intentionally different to show that the API is not called
// when a zone_id is provided in the route
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "some.third-party.com/*",
zone_id: "other-id",
})
).toEqual({
host: "some.third-party.com",
id: "other-id",
});
});

test("zone_name route (apex)", async () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "example.com/*",
zone_name: "example.com",
})
).toEqual({
host: "example.com",
id: "example-id",
});
});
test("zone_name route (subdomain)", async () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "subdomain.example.com/*",
zone_name: "example.com",
})
).toEqual({
host: "subdomain.example.com",
id: "example-id",
});
});
test("zone_name route (custom hostname)", async () => {
mockGetZones("example.com", [{ id: "example-id" }]);
expect(
await getZoneForRoute({
pattern: "some.third-party.com/*",
zone_name: "example.com",
})
).toEqual({
host: "some.third-party.com",
id: "example-id",
});
});
});
});
18 changes: 11 additions & 7 deletions packages/wrangler/src/zones.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

@romeupalos thanks for this PR! Unfortunately I don't think this is the right fix for this issue, since we don't want to split the implementations of dev and deploy. I think a simpler option would be to change the implementation of getZoneForRoute to

export async function getZoneForRoute(route: Route): Promise<Zone | undefined> {
	const host = getHostFromRoute(route);
	let id: string | undefined;

	if (typeof route === "object" && "zone_id" in route) {
		id = route.zone_id;
	} else if (typeof route === "object" && "zone_name" in route) {
		id = await getZoneIdFromHost(route.zone_name);
	} else if (host) {
		id = await getZoneIdFromHost(host);
	}

	return id && host ? { id, host } : undefined;
}

Does that work for your use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@penalosa I've incorporated your code suggestion and updated the PR.
Please, let me know if it looks good to be merged.

Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,24 @@ export function getHostFromRoute(route: Route): string | undefined {
}

/**
* Try to compute the a zone ID and host name for one or more routes.
* Try to compute the a zone ID and host name for a route.
*
* When we're given a route, we do 2 things:
* - We try to extract a host from it
* - We try to get a zone id from the host
*/
export async function getZoneForRoute(route: Route): Promise<Zone | undefined> {
const host = getHostFromRoute(route);
const id =
typeof route === "object" && "zone_id" in route
? route.zone_id
: host
? await getZoneIdFromHost(host)
: undefined;
let id: string | undefined;

if (typeof route === "object" && "zone_id" in route) {
id = route.zone_id;
} else if (typeof route === "object" && "zone_name" in route) {
id = await getZoneIdFromHost(route.zone_name);
} else if (host) {
id = await getZoneIdFromHost(host);
}

return id && host ? { id, host } : undefined;
}

Expand Down
Loading