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.

Sign up for GitHub

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 4 commits into from
Oct 25, 2023
Merged

Use zone_name for deployment of custom hostnames #4232

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2787a56
Use `zone_name` for deployment of custom hostnames
romeupalos Oct 19, 2023
3378f99
Simplify getting zone id from `zone_name` based on PR comments
romeupalos Oct 24, 2023
0b0ca97
Add some more tests
penalosa Oct 25, 2023
fe6c83c
Add mock auth to tests
penalosa Oct 25, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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
10 changes: 7 additions & 3 deletions packages/wrangler/src/dev.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import * as metrics from "./metrics";
import { getAssetPaths, getSiteAssetPaths } from "./sites";
import { getAccountFromCache, loginOrRefreshIfRequired } from "./user";
import { collectKeyValues } from "./utils/collectKeyValues";
import { getHostFromRoute, getZoneForRoute, getZoneIdFromHost } from "./zones";
import {
getHostFromRoutePattern,
getZoneForRoute,
getZoneIdFromHost,
} from "./zones";
import {
DEFAULT_INSPECTOR_PORT,
DEFAULT_LOCAL_PORT,
Expand Down Expand Up @@ -664,7 +668,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) {
}
if (!zoneId && routes) {
const firstRoute = routes[0];
const zone = await getZoneForRoute(firstRoute);
const zone = await getZoneForRoute(firstRoute, true);
if (zone) {
zoneId = zone.id;
host = zone.host;
Expand All @@ -673,7 +677,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) {
} else if (!host) {
if (routes) {
const firstRoute = routes[0];
host = getHostFromRoute(firstRoute);
host = getHostFromRoutePattern(firstRoute);

// TODO(consider): do we need really need to do this? I've added the condition to throw to match the previous implicit behaviour of `new URL()` throwing upon invalid URLs, but could we just continue here without an inferred host?
if (host === undefined) {
Expand Down
47 changes: 44 additions & 3 deletions packages/wrangler/src/zones.ts
View file
Open in desktop
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 @@ -28,6 +28,42 @@ export interface Zone {
export function getHostFromRoute(route: Route): string | undefined {
let host: string | undefined;

if (typeof route === "string") {
host = getHostFromUrl(route);
} else if (typeof route === "object") {
host = getHostFromUrl(route.pattern);

if ("zone_name" in route) {
const hostFromZoneName = getHostFromUrl(route.zone_name);
if (hostFromZoneName && !host?.endsWith?.(`.${hostFromZoneName}`)) {
// Pattern is not a subdomain of zone. Use zone name as host instead.
host = hostFromZoneName;
}
}
}

return host;
}

/**
* Get the hostname on which to run a Worker.
*
* The most accurate place is usually
* `route.pattern`, as that includes any subdomains. For example:
* ```js
* {
* pattern: foo.example.com
* zone_name: example.com
* }
* ```
* However, in the case of patterns that _can't_ be parsed as a hostname
* (primarily the pattern `*/ /*`), we fall back to the `zone_name`
* (and in the absence of that return undefined).
* @param route
*/
export function getHostFromRoutePattern(route: Route): string | undefined {
let host: string | undefined;

if (typeof route === "string") {
host = getHostFromUrl(route);
} else if (typeof route === "object") {
Expand All @@ -42,14 +78,19 @@ 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);
export async function getZoneForRoute(
route: Route,
usePattern = false
): Promise<Zone | undefined> {
const host = usePattern
? getHostFromRoutePattern(route)
: getHostFromRoute(route);
const id =
typeof route === "object" && "zone_id" in route
? route.zone_id
Expand Down
Loading