Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

RFC: Multi-Route support #866

Closed
ashleymichal opened this issue Nov 12, 2019 · 13 comments
Closed

RFC: Multi-Route support #866

ashleymichal opened this issue Nov 12, 2019 · 13 comments

Comments

@ashleymichal
Copy link
Contributor

ashleymichal commented Nov 12, 2019

MultiRoute Support

Progress so far

Lots of great thought put into this PR: #224, as well as some great points about problems mapping routes to Wrangler projects outlined below.

The problem(s)

null Workers

The biggest hurdle we have to handle in Wrangler is “off” routes; routes that are set to a “null” or “no-op” worker. This is tricky because:

  1. routes are set at the zone-level, but they can point to any Worker published to the account level. A “many-to-many” relationship.
  2. Wrangler projects are scoped to a single Worker which can be deployed to many routes across many zones, including workers.dev. This is a one-to-many relationship. But when defining routes, we need to be able to deal with those “null” workers, which are not represented as a Worker in Wrangler, but rather just an “off” state.

You can’t just compose all the routes for the project, either, because it is likely that there are other scripts on the zone that are not represented in the toml.

How is this handled elsewhere?

The Cloudflare Dashboard

The Cloudflare Zoned Dashboard UI actually presents a user with all of the routes configured on their zone, which includes potentially many workers, including routes pointing at a null Worker. This approach would be awkward in the context of Wrangler, which is generally concerned with a single worker script, not all scripts associated with an account.

Cloudflare Serverless Plugin

The Serverless Plugin doesn't currently concern itself with deleting old routes on deploy; however a later addition to the underlying tooling library includes a function for removing a route, so I imagine some subsequent steps used by the developing team to handle removing routes.
I believe that implementing this much is sufficient for a first pass at this feature; it will unblock loads of people.

Proposed solutions/workarounds

PHASE 1: PUT all routes in toml (Parity with Serverless plugin)

This will unblock most users. The downside is it potentially leaves behind unwanted routes, but the route option does that today, too. This can support both on and off routes, with the same downsides. Users will need to handle conflicts in the dashboard (we can provide a quick link by using their zone id to fill in the appropriate url).

PHASE 1.5: Sync "on" routes (one step above parity with Serverless plugin)

We can be reasonably certain that if a user publishes a script, any routes associated with that script name that are not in their toml can be removed.

PHASE 2: Guide users through handling conflicts

Any route specified in a user's toml, but associated with a different script, even a null worker, might cause unnecessary problems for users with complex routing tables. There are a couple of ways to provide users with the appropriate feedback:

wrangler publish --dry-run

NOTE: This is a straw man. The wording and presentation of this information is fully up for debate, but I'd like to focus on the concept, namely that by querying the api for the list of routes, Wrangler can provide the user with the information they need to make decisions about their deployments.

Passing a --dry-run flag to wrangler publish could do the work of diffing the existing routes against the specified routes, outputting a formatted list of anticipated modifications and conflicts:

For example, let's say we are deploying a worker called cache to a zone with the following routes already deployed:

"https://vsnginx.workers-tooling.cf":                     "cache"
"https://vsnginx.workers-tooling.cf/cache":               "cache"
"https://workers-tooling.cf/snippets/cache":              "cache"
"https://workers-tooling.cf/snippets/cache*":             "cache"
"qr.workers-tooling.cf/":                                 "qr-generator"
"*serve-assets.workers-tooling.cf/*":                     "serve-assets"
"workers-tooling.cf/foo/*":                               "hello-world-dev-foo"

Let's say I define the following routes in my wrangler.toml (for the purpose of this illustration, I will ignore the on/off concept and just include a list of routes):

routes = [
    "https://vsnginx.workers-tooling.cf",
    "https://workers-tooling.cf/snippets/cache",
    "https://workers-tooling.cf/snippets/cache*",
    "qr.workers-tooling.cf/",
    "*serve-assets.workers-tooling.cf/*",
    "vsnginx.workers-tooling.cf/*",
    "workers-tooling.cf/foo/*"
]

Running wrangler publish --dry-run should output the following table illustrating how this will affect the user's routing table:

M "https://vsnginx.workers-tooling.cf":                     "cache"
D "https://vsnginx.workers-tooling.cf/cache":               "cache"
M "https://workers-tooling.cf/snippets/cache":              "cache"
M "https://workers-tooling.cf/snippets/cache*":             "cache"
C "qr.workers-tooling.cf/":                                 "qr-generator"
C "*serve-assets.workers-tooling.cf/*":                     "serve-assets"
A "vsnginx.workers-tooling.cf/*":                           "cache"
C "workers-tooling.cf/foo/*":                               "hello-world-dev-foo"

`M` = `maintained` - this route already points to this script
`D` = `deleted` - this route points to this script but is not in the toml and so will be deleted
`C` = `conflicts` - this route currently points to a different script and may break it
`A` = `added` - this route does not currently exist and will be added

This shows me that I have three routes that would potentially break other workers running on my zone (those marked C). Which brings us to...

wrangler publish --overwrite-routes

On a normal wrangler publish, wrangler should make the same check against the routes list as in the dry run, and throw an error if any conflicts exist, printing the offending route patterns to the console. This gives the user the option to either update their routes list, or run wrangler publish again, passing an --overwrite-routes flag that will point the conflicting flags to the project script; effectively this is the user saying "I know those routes exist, go ahead and update them anyway".

PHASE 3: Route subcommands

We can optionally give the user the ability to handle routes individually by providing utilities for CRUD operations on routes, as well as listing routes. I would defer this in favor of the flag until it is requested, but it does give users more granular control over their routing tables.

What about those pesky "null" workers?

For a phase 1, we can simply add "off" routes the same way we would "on" routes; that is, just make the same kind of PUT request and let the cards land where they will. I believe this will fail if the route is already pointing to another worker, including the current worker. We could nudge this in the direction of being okay with changing a route from being "on" for this worker to being "off", but not the other way around.

For phase 2, we can treat null workers very similarly to just another conflicting worker:

v deployed | in toml > Worker "on" Worker "off"
route points to this worker Stay the same Update
route points to null worker Conflict Stay the same
route points to other worker Conflict Conflict

Where a conflict will cause wrangler publish to bail unless passed the --overwrite-routes flag.

Open Questions

All or nothing vs one-at-a-time

The above solution assumes that it is better to fail the entire deploy than to deploy only to some routes, which requires that we query the API for the list of routes before taking action. I'm not stoked about this; I would rather build this into the API, but at the moment we are just trying to unblock folks and see if this kind of an approach solves their problems. If we think it's acceptable to just try every route and return the errors to the user, that is still an option (this is how Serverless works), and the fact is that under the hood we will still have to individually POST or PUT every specified route in series rather than defining them in bulk; however defining the interface as a bulk put now makes transitioning the implementation smoother in the future.

How far do we take this?

I've proposed three phases to this work; the first phase gives us parity with the Serverless Plugin, but without the level of support of the Cloudflare Dashboard UI. This would unblock many users, but may cause problems with more complex multiroute use cases.

The second phase offers what I consider to be the minimum of protection against unexpected behavior. I am happy to write a proof of concept of this solution, but I think it will take more development and is perhaps best suited to a later revision.

The third phase gives routes first class status in wrangler. This one feels like overkill at the moment.

Solved Problems

Wrangler environments

The route key in the wrangler.toml cannot be inherited by other environments, since inheriting would stomp the original deployment. routes should likewise not be inherited, but be required as an explicit configuration.

@exvuma
Copy link
Contributor

exvuma commented Nov 12, 2019

All or nothing vs one-at-a-time

I agree. I'd love to see us start fixing issues like this closer to the API level. I think the less wrangler can be a wrapper for an API that doesn't do what it should, the better. With that, I don't think we should build a wrapper around the API to perform bulk route uploads. I think we should build the simplest solution first (Phase 1) and see if we can work with Core for a new endpoint.

The level of detail on Phase 2/3 design on this RFC is fantastic if we were to build it today, but I think we should get an MVP out (Phase 1) and see what errors users find themselves coming across. It's better to move fast, collect user feedback, and improve even if it means less than perfect initial DX. Though I agree users may get some abandoned routes, we shouldn't assume that will be a major issue and start to build out 1.5. After we collect some feedback, we should maybe start building 1.5 and come back to the detail outlined in Phase 2/3.

@gabbifish
Copy link
Contributor

gabbifish commented Nov 18, 2019

I'm really excited about this! <3
Some thoughts:

  • I suppose I have some nits about the wording in phase 2 for --dry-run (I love this flag btw). Should maintained -> unchanged and conflict -> overwrite? I feel like using overwrite is especially meaningful because it's the only type of conflict a user would experience when publishing workers to pre-existing routes.
  • I like the idea of putting phases 1 and 1.5 together for a release. I feel like they are a good pair; rolling out an initial phase of multiroute support with sync capabilities would be super compelling for users by itself!
  • The all-or-nothing approach makes sense; I like the atomicity of it. Something we'll need to consider is whether the routes API allows for an atomic route upload; right now it looks like each upload has to happen individually, which is not ideal (esp. if an route upload fails... do we have to go back and unroll all the previous route uploads in that wrangler publish invocation?) https://api.cloudflare.com/#worker-routes-update-route

@EverlastingBugstopper
Copy link
Contributor

I'd say that phase 1 and 1.5 together seem muy bueno, and that if we were only doing phase 1, we'd likely want to have "delete" functionality. However, "delete" functionality doesn't really make as much sense as syncing. I think that 1.5 is MVP for this feature, as I would not expect a route to continue to exist after removing it from my manifest.

@ashleymichal
Copy link
Contributor Author

@gabbifish that's the comment, yeah, is that whatever wrangler does to provide the appearance of atomicity will likely be a leaky abstraction over what the API currently supports. We might be able to convince the platform team to prioritize a bulk upload endpoint (a Serverless framework deploy of a worker with lots of routes hammers on the API for little good reason), but the design needs to be worked out first. The best we can do at this juncture to implement 1.5 is to query the existing routes for the zone and do our best, but with the added complexity of "null worker" zones, handling a work flow that involves multiple clients will be tricky.

@balupton
Copy link

FWIW I'm still using the workers editor on than cloudflare website to do my workers rather than wrangler or terraform due to this. As all my workers run over multiple routes over multiple domains. I also wish the cloudflare web editor supported typescript out of the box.

@stale
Copy link

stale bot commented Jul 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Sep 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 11, 2020
@balupton
Copy link

Issue is not solved.

@stale stale bot removed the wontfix label Sep 11, 2020
@ashleymichal
Copy link
Contributor Author

@balupton we did ship some multiroute support a few versions ago, however not every feature listed here was included, just the ability to deploy to many routes on the same zone. which version of wrangler are you currently running, and is there a specific feature that you are missing to do what you want with it?

@initplatform
Copy link

I am wondering how to exclude certain routes in the wrangler.toml.

Given the example here in the docs to exclude cat.png -> https://developers.cloudflare.com/workers/platform/routes#matching-behavior

*example.com/images/cat.png -> <no script>
*example.com/images/*       -> worker-script

Doesn't appear as though this is supported in the wrangler.toml

@initplatform
Copy link

@ashleymichal should I open up a new issue for this ^^

@Electroid
Copy link
Contributor

Made a new issue in #1817 for exclude route support, otherwise, we support the rest of the proposal here.

@alexandrukis
Copy link

Are there any plans for supporting #1817?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants