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

feat: optionally send zone_name with routes #797

Merged
merged 1 commit into from
Apr 15, 2022
Merged

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Apr 14, 2022

A followup to #778, this lets you send an optional zone_name with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793

@changeset-bot
Copy link

changeset-bot bot commented Apr 14, 2022

🦋 Changeset detected

Latest commit: ae62a5f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2022

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/2173496602/npm-package-wrangler-797

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/797/npm-package-wrangler-797

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2173496602/npm-package-wrangler-797 dev path/to/script.js

@threepointone threepointone force-pushed the route-zone-name branch 2 times, most recently from aae03f0 to 8f5105e Compare April 14, 2022 16:50
@petebacondarwin
Copy link
Contributor

Can you explain a bit more about what this is fixing?
It feels to me that we are now conflating two things because of the fact that we tried to use routes to guess zones, but that in some cases that is not possible?
Is it the case that a route really has a zone associated with it that is important outside of just guessing to which zones we should deploy?

If deployment zone is actually independent of routes (i.e. when we cannot infer it) then I would feel more happy to have a separate configuration of what zones we want to deploy to...

But I feel I am missing some context here.

@threepointone
Copy link
Contributor Author

Ok, so the context here is that you can deploy to routes that are associated with completely different zone_id/zone_names. An example (and the only real one rn, but I expect others to happen later) is the ssl for saas product (https://developers.cloudflare.com/ssl/ssl-for-saas/). Here, let's say you run a saas product xyz.com, and your customer ABC wants to deploy it at xyz.abc.com. In this situation, you would deploy your worker to a route pattern "xyz.com/*", but also pass a zone_name: "xyz.abc.com" (or a different zone_id). That's the usecase, where you don't want cloudflare to use the zone_id associated with the "origin". @WalshyDev / @eidam can correct me if I'm wrong.

@eidam
Copy link
Contributor

eidam commented Apr 15, 2022

need it now, gonna give it a spin 👀

@eidam
Copy link
Contributor

eidam commented Apr 15, 2022

its working like a charm!

Published xxyy (3.32 sec)
  something.helloworkers.dev (zone name: eidam.cf)

@threepointone
Copy link
Contributor Author

Oh yay!

Copy link
Contributor

@caass caass left a comment

Choose a reason for hiding this comment

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

looks good to me, maybe we should encapsulate the routes config type in its own type Routes = block but that's mostly just preference. approved :)

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

OK happy to approve this - as it is just an addition to the previous zone_id work.

But I am a bit confused.

Is this using undocumented API? The docs here https://api.cloudflare.com/#worker-routes-update-route only accept a body containing a JSON object of the form { pattern, script } - no zone_id nor zone_name.

A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
@threepointone
Copy link
Contributor Author

Correct, the docs need to be updated.

@threepointone threepointone merged commit 67fc4fc into main Apr 15, 2022
@threepointone threepointone deleted the route-zone-name branch April 15, 2022 17:02
@threepointone
Copy link
Contributor Author

(also that's not the api we use, we instead use some newer apis which aren't documented yet)

@github-actions github-actions bot mentioned this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: routes should be able to specify zone_name
4 participants