v6 removal of /twirp prefix makes upgrading hard

[spenczar]

The current v6_prerelease branch changes the URL scheme. It goes from /twirp/<package>.<service>/<method> to just /<package>.<service>/<method>.

Unfortunately, this is difficult to upgrade into. Consider a service which has generated v5 code today, and which has a client which vendors that generated Go code. The service wants to upgrade to generating v6 code, and they currently register their Twirp service like this:

If they regenerate their code and don't tell the client to upgrade their vendored copy, then all client calls immediately start failing, since the client is hitting the old URL, which 404s. If the client upgrade first, same thing - all 404s. The client and server need to deploy an upgrade version simultaneously. That's unacceptably painful. We need something better.

What are the options?

1. Document code changes the service owner can make

We could tell service owners that they must run their service on two URL path prefixes if they are upgrading, at least until all their clients are updated. For example, they'd need to do this:

mux := http.NewServeMux()
mux.Handle(HaberdasherPathPrefix, svc)
mux.Handle("/twirp" + HaberdasherPathPrefix, http.StripPrefix("/twirp", svc))

This is not very pleasant. It's gross legacy code, and hard to know you need to do this; I don't like solutions that require manual action from our users. It's hard to know when you can remove this code, as well. And if users already have a mux, they could get confused.

We could make this more visible by renaming the PathPrefix constant. That way, code won't compile, and hopefully the user goes and looks up what's wrong and learns that they need to make a code change. But this is a bad option; upgrading should be painless.

2. Leave /twirp as a default prefix, but allow it to be customized

We could change the branch to default to using the /twirp prefix. But if the user specifies, we could remove the prefix in generated code. This would make upgrading easy - the user would only run into trouble if they enabled a custom prefix, which is kind of their own fault. Most upgrades would be painless.

But I already rejected this approach in #48, really. My argument there was that non-customizability is a feature, not a bug. It makes third-party generators harder to get right, and complicates the spec a bit. That said, it's worth looking at how we'd do this.

Protobuf files support options. We could have something like this:

// this path depends on your local machine :(
import "path/to/twitchtv/twirp/genconfig.proto" 

service Haberdasher {
  option (twitchtv.twirp.genconfig.prefix) = "/whatever/you/want"
  rpc MakeHat(Size) returns (Hat)
}

The challenge is really that the option must be defined in some other .proto file which gets imported by the user. Twirp would have to distribute that .proto file somehow, and provide instructions on how to import it. This is tricky - protoc is not a friendly tool. But perhaps this is alright, since we hope that custom prefixes are the exceptional case, not the common one, so it's alright for them to be a little difficult.

3. Go back to requiring /twirp prefix

Finally, we could revert this change entirely. This is appealing to me - it keeps Twirp simple and the upgrade is really clean. I like that no third-party generators become incompatible, since the protocol remains the same.

But there are a few people who are using the prerelease branch today because they hate that prefix. I'd like to hear from them on their needs, once again. We kind of touched on them in #55, but that covered other topics, and I don't think we have good written reasons that removing the /twirp prefix specifically is worth the cost. I'd like to hear those arguments.

---

I lean towards option 3, in absence of clear reasons the /twirp prefix needs to be removed.

[spenczar]

cc @rynop @jonathaningram @rmasp

[rynop]

Thank you for putting thought into this.

My vote is #1, then #2. I love Twirp, but I don't want consumers knowing that I run it. Same reason you remove apache/nginx/php/blah product&version response headers from HTTP servers. Also further locks me in to Twirp (hopefully you never leave Twitch :)

FWIW, I run v5 today behind a CDN. My CDN adds the /twirp prefix for me. My Twirp clients all don't send /twirp. I'd prefer to use v6 and not have this extra config, but it's so far behind master I've been using v5 for new projects.

#1 is one line. Someone using Twirp is already a bit adventurous/skilled, so IMO its a no-op. I do this today when I'm dev'ing locally (not going through CDN that adds prefix).

I'm ok with the extra pain outlined in #2, because if this is the chosen option, I'm obviously the minority and should be expected to do extra work to get away from the default.

[marwan-at-work]

I think keeping things simple and stable is a better feature than removing /twirp. Though I'd prefer the /twirp prefix to be removed to hide a server's internals.

So my vote is either #2 or #3

For #3, I think we can maybe just document how to hackishly remove that prefix for any user who wants to do it like @rynop suggested. For example, by adding a Go middleware that just prepends the /twirp prefix.

[rmasp]

My vote is for 2. It doesn't have to be a schema item though. I'd even be happy with a protoc plugin option, like the one I wrote for a v6_prerelease compatible protoc-gen-twirp_swagger

    --twirp_swagger_out=twirp_route="":./swaggerui/rpc

I've been socializing twirp across my organization in hopes of making it a standard microservice architecture. About 1/2 the engineers I show it too ask, "Can we get rid of this /twirp thing?" I also anticipate I would get exec blowback if we decided to build open APIs or expose APIs to partners.

In that case I might be forced in to #4: fork twirp and start a schism. That would be a last resort, for sure.

[marioizquierdo]

Option 1 doesn't look that bad to me. Is a major update and the instructions are clear: to update you need to support both with and without the prefix.

Option 2 looks decent. Although I think the prefix should default to empty. New Twirp users should not have to worry about the prefix. It feels more intuitive moving forward to have no prefix by default. The upgrade instructions in this case would require to explicitly add the prefix option to "/twirp". This option doesn't need to be respected in other language implementations, because the easy way out is to just add that prefix in the baseURL when instantiating clients.

Option 3 seems like the easy thing to do now, but has the issue of always having to answer the same question over and over again forever and ever. Maybe we can automate that by adding a new page in the documentation, with some known solutions for it. Maybe also maintain a no-prefix branch?

[jonathaningram]

I agree that keeping Twirp (and generally anything) customisation-free is good, so I'd probably prefer option 2 the least.

Regarding option 1. Putting aside anything to do with Twirp, APIs are often retired (after some fair warning and time hopefully), and I think service owners need to cater for this no matter if they use Twirp or not. I have retired certain endpoints and it was relatively easy to track the customers/clients that were calling the deprecated endpoints and give them a nudge to upgrade. With that in mind, option 1 would probably be my preferred option. It's a little gross, but not too difficult for the service owner to implement, along with good comments and commit messages.

Regarding option 3. As @rynop says, I think the /twirp prefix is the only thing that really looks like vendor lock-in. Correct me if I'm wrong, but everything else in Twirp looks pretty generic and so one could reasonably remove Twirp from the equation and clients nor the service owner would be none the wiser. I do love that Twirp can be used "just like a REST API" where you could hand out CURL commands and let customers implement their own clients. However, I don't think that is Twirp's primary use case: rather it's an edge case and so I think I'm reverting a little on my first opinion that this prefix is a deal-breaker. I think I'd still prefer option 1 but could live with option 3.

Thanks @spenczar for taking the time on this.

[khlbrg]

We are not running Twirp in production right now, but we are on our way to make Twirp a big part of our ecosystem. So we are not affected by this decision, although, coming as a new user I would prefer option #1.

[lwc]

For better or worse, we use a https://internal.gateway/<service>/ approach to routing our services, so we're already juggling http.StripPrefixs in various places.

Option 1 would get my vote :)

[dpolansky]

I don't think removing the /twirp URL path prefix is the right decision for Twirp.

Introducing this breaking change in a new major version of Twirp could affect some of its users too significantly. Some organizations that use Twirp would need to migrate hundreds of services for a change that may not have any business value. Even worse, the breaking change could result in unsuccessful migrations that may cause impact to services. I don't think this change would serve any purpose at Twitch, and Twitch is a sufficiently large user of Twirp to experience a significant amount of disruption from the change.

That being said, I do understand the need to change the path prefix for security or cosmetic purposes, and Twirp should have a sufficient answer for that use case even if it's not supported directly in Twirp's spec.

This problem seems solvable with code generation as a protoc plugin. I would prefer a plugin in comparison to a protobuf option because I think it is more clear to users that the mechanism is not supported by Twirp's spec. I also think it's appropriate to push the added complexity onto the users that have a need to deviate from Twirp's spec if it avoids breaking changes.

If there is interest for such a plugin, I am interested in gathering requirements for it and collaborating on an implementation. I'm interested in changing my view on this issue if presented with compelling arguments as well.

cc @rmasp

[marioizquierdo]

Twirp is not going to change the path format in the short term. The Twirp team has archived the draft for protocol v6.

However, it is still possible to implement a Twirp service without the /twirp prefix (or a different prefix) by using middleware and adapting the clients to call the right path.

As an example, this is how to build a Twirp service without the "/twirp" prefix.

Use middleware to strip the prefix in the service:

mux := http.NewServeMux()
mux.Handle(MyTwirpPathPrefix, http.StripPrefix("/twirp", svc))

All clients need to be configured to call into the URL without the prefix, which is different for different implementation or languages (this is the main reason why Twirp protocol can not be updated easily). For a Go client, a roundtripper can be used on the http.Client:

type StripPrefixTransport struct {
	Prefix string
	Transport http.RoundTripper
}

func (t *stripPrefixTransport) RoundTrip(r *http.Request) (*http.Response, error) {
	r = requestWithStripPrefix(r, t.Prefix)
	return t.Transport.RoundTrip(r)
}

func requestWithStripPrefix(r *http.Request, prefix string) r *http.Request {
	p := strings.TrimPrefix(r.URL.Path, prefix)
	if len(p) == len(r.URL.Path) {
		return r
	}
	r2 := new(Request)
	*r2 = *r
	r2.URL = new(url.URL)
	*r2.URL = *r.URL
	r2.URL.Path = p
	return r2
}

The StripPrefixTransport could be used on the *http.Client as Transport property.