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

Proposed new routing format: /package/service/method #55

Closed
spenczar opened this issue Jan 24, 2018 · 36 comments
Closed

Proposed new routing format: /package/service/method #55

spenczar opened this issue Jan 24, 2018 · 36 comments

Comments

@spenczar
Copy link
Contributor

spenczar commented Jan 24, 2018

I think we could improve Twirp's routing scheme. It has two problems right now: the /twirp/ prefix, and the package.service scheme (instead of package/service).

Some people have mentioned to me that they're hesitant to use Twirp because the routes are prefixed with /twirp, which is concerning to them for several reasons:

  • Trademarks: some very large organizations don't want to take any legal risks and are concerned that "twirp" could become trademarked.
  • Feels like advertising: This was mentioned in Add support for custom route prefixes #48: putting "twirp" in all your routes feels like it's just supposed to pump Twirp's brand.
  • Homophonous with "twerp": In some Very Serious settings (like government websites), it's not okay that "twirp" sounds like "twerp", which means something like "insignificant pest."

We currently have the /twirp prefix to avoid colliding with gRPC's routes, which live at /package.service/method, and to reserve a space for future APIs (like a reflection server).

Slashes-based routes (instead of dot-based) would be preferable because lots of existing technology expects slash-based routing. Load balancers and proxies use slashes for namespaces. If Twirp used a slash after the package name, users could leverage this much more easily. This is often possible with dots, just annoying, as you have to write a (possibly pretty complex) regex.

I think we can fix both of these in one go. This would be a breaking change at the protocol level, so it's something to take very seriously, but I think we can do it without much pain. It would just take a version bump to v6.


Proposed Implementation:

I propose we use a new routing scheme: /<package>/<service>/<method>.

This does not collide with gRPC's routes, which is good. I think it gives us flexibility to put future routes in by putting them under a Twirp package. We could provide a reflection service like this one, which lists available services:

syntax = "proto3";

package twirp.meta

import "github.com/golang/protobuf/protoc-gen-go/descriptor/descriptor.proto"

service Reflector {
  rpc ListServices(ListServiceRequest) returns (ListServiceResponse);
}

message ListServiceRequest {}
message ListServiceResponse {
  repeated protobuf.ServiceDescriptorProto services = 1;
}

It would live at /twirp.meta/Reflector, which seems good to me.

Regarding compatibility: generated clients would always use the new routing scheme. Generated servers would be able to use both, in case your users have old generated clients still around. We would accomplish this by exporting two path prefixes:

const (
  HaberdasherPathPrefix = "/notreal.example/Haberdasher/"
  HaberdasherLegacyPathPrefix = "/twirp/notreal.example.Haberdasher/"
)

Users can mount both on a mux if they want to support old clients. If not, they can just use the new one.

If we do this, there are other consequences.

  • Documentation would need to be updated everywhere.
  • Server implementations in other languages would need to be updated ASAP, as new clients won't work with old servers.
  • If people have log parsing or metrics gathering code that operates on routes, they'd need to change them.

The pain of those other consequences is a good reason that we should try to get this out promptly, if we do it.

@spenczar spenczar added this to the v6 milestone Jan 24, 2018
@Xe
Copy link

Xe commented Jan 24, 2018

Terrible idea for migration simplicity: a v5 RoundTripper in a box that tries the old path if the new path doesn't work.

@cep21
Copy link

cep21 commented Jan 24, 2018

One advantage of the /twirp/ prefix is that it made routing and middleware layers easier to use. For example, I could forward all of /twirp/* to one middleware layer and still put other HTTP endpoints on the same http.Server

@spenczar
Copy link
Contributor Author

@cep21 That's true, you'd need to wrap each twirp service with your middleware wrapper, instead of being able to do it once.

Most programs only serve one Twirp service, but some might serve multiple. This would mean an extra line of code for each one. That seems like a pretty mild loss.

You could encapsulate it with a function that loops over your services if it's really bad, like this:

type middleware func(http.Handler) http.Handler

func mountTwirpServices(mux *http.ServeMux, svcs map[string]http.Handler, m middleware) {
  for prefix, svc := range svcs {
    mux.Handle(prefix, m(svc))
  }
}

@marioizquierdo
Copy link
Contributor

Internally at Twitch having the /twirp prefix is useful to easily tell apart twirp services from others, but yeah, it makes sense to remove this prefix on other organizations. The same can be achieved by using some common prefix on package names.

I like the proposed path schema: <package>/<service>/<method> 👍

@jonathaningram
Copy link
Contributor

I reached out to @spenczar about this via DM. Just sharing some of my thoughts on this.

As part of my dta.gov.au team, we're evaluating some API toolkits/specs/etc. that could be used in various whole of government solutions.

Whilst tooling/SKDs should mask the path most of the time, the reality is that if we build APIs that are used by other gov departments/services, it's possible that they won't or can't use any of the SDKs that would mask the path.

Instead we'd give them some sort of "documentation" for how to construct HTTP requests to call the API.

Putting something like:

Make a POST request to https://a-thing.gov.au/twirp/x.y.UserService/CreateUser

in the documentation might not be received that well even though you and I know it's just a word. It may not give confidence that your service is taking things seriously, etc.

I don't have a strong opinion on what the best format is for the path, but I wonder if at the least it could just use something else instead of /twirp (including /twirp.meta/Reflector), e.g. trpc.

@spenczar
Copy link
Contributor Author

Two new concerns with /<package>/<service>:

  • It's legal to have no package. Should we really be going to /<service> then? We run a much higher risk of colliding with other stuff.
  • Lots of .proto files are already written now in prominent projects like Envoy. There is a real risk of collision for them, as they probably didn't pick package names with this in mind. That could easily lead to collisions in the first path element, at least (imagine /envoy, for example).

In light of this, @wora suggested still using a prefix. Two options:

  • /$rpc/<package>/<service>/<method>
  • /trpc/<package>/<service>/<method>

We can be pretty sure that $rpc will not collide. Most people would think it even needs to be escaped (it doesn't). But it's a little ugly.

trpc is prettier, but has a small risk of collision, and returns us to trademark fears, advertising concerns, etc.

I lean a little towards /$rpc as a prefix to just be done with this whole thing. What do others think?

@wora
Copy link

wora commented Jan 26, 2018 via email

@fajran
Copy link

fajran commented Jan 26, 2018

I prefer to have a customizable prefix (does that what $rpc mean?). One of the use case is if we want to have multiple Twirp services in one app. Each can handle different prefix. I personally don't mind about how the path for package and service is structured.

@doapp-ryanp
Copy link

doapp-ryanp commented Jan 26, 2018

If consensus is use prefix, why not just stick with twirp or twirpc? The latter is a "cute" way of conveying both rpc and twirp.

I have no opinion on the naming, just as long as / is delimiter instead of the .s that are used today.

@spenczar
Copy link
Contributor Author

@fajran No, $rpc wouldn't be configurable. It would be a hardcoded string.

@doapp-ryanp, twirp and twirpc both have problems for Very Serious environments (see #55 (comment) above). They also raise eyebrows about trademark concerns at big companies.

@thechriswalker
Copy link

thechriswalker commented Jan 26, 2018

IMHO we should just not enforce a prefix -- as commented here you can mount your twirp service wherever you want (including at /twirp). Then it would be a runtime/compile-time decision, not code-generation time decision.

Of course, as this is a backwards incompatible change in itself (unless you do choose to mount at /twirp) it make little difference if other breaking changes occur as well - such as the change of delimiter, although in my mind that doesn't accomplish anything.

EDIT: The client would need a baseurl anyway, it would now just need the mounted prefix as well.

@spenczar
Copy link
Contributor Author

spenczar commented Jan 26, 2018

@wora, a significant problem with $rpc: because people don't expect $ to be legal in URLs, lots of tools don't handle them properly. This is especially bad because $ is a special character in PHP, regular expressions, and many shells. Putting it in a URL makes life significantly more difficult for people.

Many examples of confused people dealing with dollar signs in URLs that pop up on google:

I am sure there would be more. This is enough for me to say $ in a URL is a bad idea.

@spenczar
Copy link
Contributor Author

@thechriswalker Yes, the issue is making sure clients know the right prefix. It would be best if clients only need a hostport, not a prefix too.

Your point that the server can be mounted at a special place, and the client can include that prefix if they must, is a really good one, though. That might mean we have coverage for the exceptional cases where we would collide, and where a collision is unavoidable because changing the package is considered unacceptable.

@cep21
Copy link

cep21 commented Jan 26, 2018

Trpc feels like it had clear advantages with little downside. I think it is enough away from twirp or twitch that serious business won't mind

@thechriswalker
Copy link

@spenczar I think the difference between having the "address" of the service ashttp(s)://host:port/ vs http(s)://host:port/path is negligible. They still need one string, one piece of info, not two. I meant that they would be combined anyway. There is really 4 pieces of info there as well, as you may be running this over HTTPS or HTTP.

I am slightly biased as I am thinking of using twirp exposed over the public internet from the same web server as the rest of a web app, so would want a customizable prefix.

@spenczar
Copy link
Contributor Author

@thechriswalker, another good point re: sharing the scheme, host, and port anyway. Your use case, exposing it next to other web app routes, is one we want to support well.

@wora
Copy link

wora commented Jan 26, 2018

I agree with @thechriswalker. A client needs at least one piece of information to locate the server, and we can standardize the address as the base URL as http(s)://host:port/path. This would solve all issues raised here. I would be super happy with it.

@marioizquierdo
Copy link
Contributor

marioizquierdo commented Jan 26, 2018

I agree that Twirp services should be "mountable" on any prefix. For consistency, we can call it "baseURL" in the client (instead of "addr" or "host").

This will also solve a somewhat surprising behavior with the current router switch, that only matches requests in the root url even if the twirp httpHandler was mounted with a mux on top of another base path. I think it makes sense to asume that Twirp services are mountable on any "baseURL", and then clients receive that "baseURL" as parameter. This also makes sense for other languages, not just Go.

If for v6 we keep the URL format for Twirp in the form <package>.<Service>/<Method> and allow to mount on any url prefix at the same time, the change upgrade will be easier: to support v5 clients just mount the v6 service on /twirp. But now any new v6+ service can be mounted anywhere, which also solves the problem of collisions with other services (e.g. gRPC)

@wora
Copy link

wora commented Jan 26, 2018

Yes. We have an agreement here. I am drafting the wire protocol spec with Spencer. Stay tuned.

@spenczar
Copy link
Contributor Author

@marioizquierdo, good catch that the router's current switch doesn't permit remounting. If we advise people to mount on a separate prefix if they need to avoid collisions, we will also need to either:

  • tell them to use http.StripPrefix, or
  • route things based on the requested URL's suffix, which should be in the formal specification.

The second seems more reasonable to me.

@rhysh
Copy link
Contributor

rhysh commented Jan 28, 2018

I have a request and a suggestion for the migration, regardless of the format we choose.

First, we release a version (v5.x.0) where servers can understand both v5 and v6 URIs but where clients still send v5 URIs. An org that is currently using v5.1.0 could safely upgrade all clients and servers to v5.x.0 in any order. After finishing that migration, they could then upgrade their clients and servers to v6.0.0, again in any order.

Second, I suggest that v5 path prefixes be named "v5" rather than "legacy". Someday v6 may be legacy too. For example:

const (
  HaberdasherV6PathPrefix = "/notreal.example/Haberdasher/"
  HaberdasherV5PathPrefix = "/twirp/notreal.example.Haberdasher/"

  HaberdasherPathPrefix = HaberdasherV6PathPrefix
  // or `... = HaberdasherV5PathPrefix` during the transition
)

@spenczar
Copy link
Contributor Author

@rhysh, your suggestion on names of the consts is clearly better. We'll do it the way you described.

The two-step migration sounds good and interesting too. It's kinda weird that we'd be making a v5 release that handles v6 behavior, though. I suppose HaberdasherPathPrefix would be set to the v5 prefix in that intermediate release?

@marioizquierdo
Copy link
Contributor

@rhysh @spenczar Related to my previous comment, if we keep the current path format <package>.<Service>/<Method>, and change Twirp service routing to be based on path postfix, then there's really no need for migration on the clients side.

The v6 upgrade instructions would tell services to also mount twirp on the /twirp route for backwards compatibility.

@wora
Copy link

wora commented Jan 30, 2018

I think we should change the server code to accept both package.service/method and package/service/method, and encourage people to use later. I think the server change is very trivial and the change does offer long term value to users.

The routing can be done by different proxy layers, there is no chance for us to update them. It is a very tiny cost for us to avoid the pain for customers.

@spenczar
Copy link
Contributor Author

Right, @wora, if v6 servers accept both package/service and package.service, then users can mount at both / and /twirp to get compatibility with v5 clients.

But as @marioizquierdo points out, v6 clients can talk to v5 servers only if we generate them to use package.service.

I think we have agreement that we should drop the /twirp prefix, but let's talk more about the conversion to using a slash between package and service. The cost of this switch is that you cannot upgrade a client to v6 without first upgrading the server (either hopping through an intermediate version like @rhysh said, or all the way to v6). What are the benefits?

@Xe is the one who suggested this - could you talk a bit about the benefits of using slashes here?

@mocax
Copy link
Contributor

mocax commented Jan 30, 2018

A proto package typically represents a single API. You can write a single routing rule per package, without worrying about adding new interfaces to the same package. With the same URL length, / is much better for dispatching than .

For upgrade, we can add a boolean flag to the v6 client generator to produce the old . separator, with a transition window of 6 months. The usage of Twirp probably allows changing the spec now. Or we can stay as is forever.

@spenczar
Copy link
Contributor Author

@mocax, I really don't want to add a boolean flag. Flags make the behavior dependent on the letters somebody typed in on the command line when they generated the code rather than dependent on the proto document + the Twirp spec. That makes the system much less predictable.

If we do this, we'll just do it for clients, and servers will be backwards compatible. I agree that we should do it sooner than later, while Twirp is still young enough to handle spec changes.

@mocax
Copy link
Contributor

mocax commented Jan 30, 2018

I don't like to have flag at all. If we decide to switch, we should just do it.

@marioizquierdo
Copy link
Contributor

Yeah, no flags, because it also makes multi-language support a lot harder, then all service generators in all other languages will also need to support the flag!

In terms of backwards compatibility, it is fairy easy to make the Go server generator to support both v5 and v6 routes, but that also imposes extra burden on other languages. Should all other languages support v5 routes as well? Ideally Twirp would support scenarios like a client generated in twirp-v5 in Go talking to a service generated in twirp-v8 in Rust. In any case, it is probably early enough to consider this kind of change (actually, is now or never!!!). It is probably totally OK for the future server generator in Rust to only support v6+ clients.

@doapp-ryanp
Copy link

@spenczar close to any decision on this? My team would like to use Twirp in production, however I'm waiting for this change to occur (we are not fans of the v5 routing scheme, for reasons brought up in this thread).

@spenczar
Copy link
Contributor Author

spenczar commented Feb 14, 2018

@doapp-ryanp I think we have consensus here, we will change the routing in the way listed. We should make a PROTOCOL_v6_DRAFT.md document, or something - clearly labeled as a draft, it would be our plans for v6.

However, when it comes to actually implementing that new spec, I would like a v6 release to include streaming support. That requires two things:

I think that, for now, we do a v6-alpha release which includes just routing changes and a sketch for a possible streaming API, with very loud notices that the streaming API be broken. We'll need to maintain a separate branch until we lock in the streaming API.

So, immediate action items:

  • Make a new v6 draft of the spec, both in docs/spec.md and PROTOCOL_V6_DRAFT.md, which includes routing changes. (Add draft of v6 specification #80)
  • Make a new v6_prerelease branch.
  • Implement v6 routing in the v6_prerelease branch.
  • Add a description the new streaming protocol to the v6 spec draft.
  • Implement streaming in the v6_prerelease branch.

You should be good to use twirp after step 3, @doapp-ryanp, if you're comfortable working with a prerelease version while the streaming API gets some use. The only unstable part of the prerelease would be streaming APIs; we would promise not to break other stuff.

@nullbio
Copy link

nullbio commented Feb 15, 2018

On the subject of changing the routes, what are your thoughts on lowercasing the service and method names? It seems a bit odd to have case sensitivity in a URL.

@spenczar
Copy link
Contributor Author

@nullbio That doesn't bother me too much. It's unusual, for sure, but it shouldn't matter, as you shouldn't be seeing URLs too often anyway. If you want to discuss more, could you open an issue for that topic?

@wora
Copy link

wora commented Feb 15, 2018 via email

@marioizquierdo
Copy link
Contributor

The protocol v6 was archived. The are no current plans to update the route path formats.

For reference, there is more talk about this in #179

@marioizquierdo
Copy link
Contributor

PR #263 implements a solution for the twirp prefix in a backwards compatible way. It updates the wire protocol from:

/twirp/[<package>.]<Service>/<Method>

to:

[<prefix>]/[<package>.]<Service>/<Method>

The <prefix> is now configurable in clients and servers, using "/twirp" by default.

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

No branches or pull requests