Grpc-gateway: 404s using colons in the middle of the last path segment

[charleschenster]

I have the following endpoint and user_ids of the format "user:"

  rpc GetUser(GetUserRequest) returns (GetUserResponse) {
    option (google.api.http) = {
      get: "/v0/users/{user_id}"
    };
  };

I'm getting 404 status code and it doesn't hit my GetUser handler when I call this endpoint with user_ids of the format above. However, if I call the endpoint with a user_id that doesn't have a colon it (e.g. "user123") it does hit my GetUser handler. Url encoding the colon doesn't work either.

What's even more interesting is if I add another colon to the end of the path, grpc-gateway parses the user_id into the correct format that I want. ("/v0/users/user:123:" -> user_id = "user:123"). Given this data point, I believe grpc-gateway is treating colons differently in the last path segment.

For example here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-grpc-gateway/httprule/parse.go#L86 it strips off the last colon in the segment. Though this looks like the rule for parsing the endpoint path definition and not what is being used to parse incoming requests.

[yugui]

Thank you for your report.
But I don't understand what are you trying to say.

I guess probably the facts you have reported were different from your expectation, but I don't know what you expected and why you expected so. Could you explain what exactly you expected and its reason?
Also if you have not yet, please read the FAQ at first.

[charleschenster]

@yugui Thanks for the response!

Given the endpoint definition above here are the use cases with the expected and actual result

• curl /v0/users/user:123

Expected: GetUser handler receives request with user_id = 'user:123'
Actual: 404 status code is returned

• curl /v0/users/user123

Expected: GetUser handler receives request with user_id = 'user123'
Actual: (Matches expected) GetUser handler receives request with user_id = 'user123'

• curl /v0/users/user:123:

Expected: GetUser handler receives request with user_id = 'user:123:'
Actual: GetUser handler receives request with user_id = 'user:123'

[ambarc]

+1 seeing the same issue.

Colons in the URL path are part of the standard spec:

http://stackoverflow.com/questions/2053132/is-a-colon-safe-for-friendly-url-use

and there's likely some miscommunication happening at the gateway layer as it tries to speak to gRPC.

[yugui]

Colons in the URL path are part of the standard spec:

Right. But it does not always mean that our spec supports the URL.

The problem is something similar to shift/reduce conflict between a VARIABLE value and VERB in the spec at runtime. /v0/users/user:123 actually looks like a sequence path components [v0, users, user] and a verb 123.
So I doubt if we should parse user:123 as a variable value based on the fact that there's no other template /v0/users/*:123 defined because it would mean that the interpretation of a URL pattern depends on other patterns if we parsed so.

As a workaround, you can still escape colon with percent-encoding or write a custom wrapper.

[charleschenster]

Thanks @yugui

I mentioned in the first comment that I tried percent encoding the id and it didn't work. Here's the expected and actual use case here.

• curl /v0/users/user%3A123

Expected: GetUser handler receives request with user_id = 'user:123'
Actual: 404 status code is returned

It looks like here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L60 the path used should be the raw path to check for the colon. Then the path should be url decoded for the rest of the method.

Thanks for pointing me to the custom wrapper, I'll see if it'll match my needs in the meantime.

[charleschenster]

@yugui Any updates on this? As described in the previous post, the raw path should be used to check for the colon and presence of a verb.

[ghost]

Same issue here and the link to the "spec" is broken (I suppose its this file https://github.com/googleapis/googleapis/blob/master/google/api/http.proto ). But its not clean what is a verb anyway.

[hoveychen]

Any update on this issue? Path template seems to be broken by some undocumented Verb stuff.
Encoding colons with %3A doesn't fix.

[tmc]

A good first step here would be to submit or modify an executable test case that demonstrates this problem. Making contributions easy for beginner contributors is important so if you hit snags on this path please open issues to improve documentation.

[Azuka]

I was able to get around this by wrapping the mux itself with a hack handler that appends a : to the request path if there's none, because all my ids are urns:

func wrapMux(h http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if !strings.HasSuffix(r.URL.Path, "/") && !strings.HasSuffix(r.URL.Path, ":") {
            r.URL.Path += ":"
        }
        h.ServeHTTP(w, r)
    })
}

Confirmed that both paths like /users and /users/urn:user:123 work

[kolotaev]

I have the same issue, because of the usage of URN as User ID. Not ID with bare colon, nor url-encoded one doesn't match the route and results in 404.

Middleware suggested by @Azuka helps a lot, but, to my mind, this case should be considered and handled in grpc-gateway internally.

[kassiansun]

@Azuka @kolotaev I don't think URN is valid inside URL, they are exclusive forms of resource identifying.

[Azuka]

@kassiansun, I'm not technically using urns (the prefix isn't urn:): I'm using something similar to Amazon's ARNs.

[kassiansun]

Are you serious about technically, or just some kind of "speechcraft"?

Anyway, I can fork this repo and maintain it by myself, thank OSS.

[Azuka]

I'm honestly not sure what your argument is. I've never heard or read of any argument that URL paths can't contain URNs, only that a URI is either a URL or a URN. It's up to you how you want to interpret a URL path.

This isn't the place to have this argument. No one's stopping you from forking if you don't agree with the support for colons in paths (which is the issue being discussed here).

[achew22]

@kassiansun, maintainer here. Sorry it sounds like you're frustrated. That's obviously not what I want.

First, if you want to fork, please do. This code is open source so that anyone can take it, change it, and put their own spin on it. However, multiple incompatible implementations is not my favorite outcome. Could you help me understand what problems you have and what you would do in your proposed fork? Maybe that's something we could support in mainline either directly, through a plug-in/middleware/flag.

Looking forward to hearing another opinion!

[kolotaev]

I don't think URN is valid inside URL, they are exclusive forms of resource identifying.

@kassiansun, I won't argue that URN and URL are meant to be somewhat mutually orthogonal and, to some extent, it's strange to use URN inside URL.
But to my mind, there are no restrictions on what to use in your URLs, apart from what allowed by RFC-1738. And according to it colon : may be a reserved character in certain schemes. In case of grpc-gateway, as far as I understand, we are talking about http/s: scheme, where colon is reserved and thus must be URL-encoded. But the problem is that URL even with URL-encoded colon doesn't match the route in current implementation.

Sorry for the late reply.

[kassiansun]

In case of grpc-gateway, colon : is intend for custom method, that' why we should not allow for this kind of usage.

Anyway, there're so many people arguing against this, and some clever guys digging the old and incorrect grpc documentation have found the standing point for this change.

I will not argue for this anymore, forking is much easier.

[johanbrandhorst]

@kassiansun as discussed in #760, we're likely to revert the fix for this change so you can use :verb for custom verbs as you say.

[johanbrandhorst]

I will reopen this for discussion once #761 has been merged. @jfhamlin sorry for this, but we're going to release 1.5.1 with a revert of this. If you still require this functionality we can consider adding a generator option. What do you think?

[johanbrandhorst]

Reopening because fix was reverted with #761.

[jfhamlin]

No worries, @johanbrandhorst. I do require this functionality, so let's talk about it. I actually mentioned this backcompat issue in my pull request description. I should have forced the discussion there, though. I've repeated the relevant part of the PR description below.

To remark quickly on #760, it's actually the exact example from the description of my PR 😬. Re your comment on #760:

It also seems like the wrong thing to support this tbh, I'm just disappointed we didn't discover it earlier and didn't break users.

I tried to make a case in #708 that the fix was in fact a step _towards_ the letter of the spec — I still think that's right. The problem isn't that #708 prevents custom verbs from working, it's that the order of the rules is such that grpc-gateway prefers the verbless match. If we fixed grpc-gateway to implement the specified "last one wins" behavior (see below), then #760 would work as expected.

I think the best solution would be to put "last one wins" behavior and the fix from #708 behind a generator flag — this maintains backwards compatibility and makes available (what I think is) the correct rule-matching behavior.

PR Description

There is a backwards compatibility issue here: this can change the behavior of existing rule sets. For example:

    option(google.api.http) = {
      get: "/users/{user_id}"
    };
    option(google.api.http) = {
      get: "/users/{user_id}:averb"
    };

Before this change, only the second rule matched a request for /users/123:averb. After this change, both rules match (correctly, by my understanding of https://github.com/googleapis/googleapis/blob/master/google/api/http.proto). Note that the spec describes a deterministic way of breaking the tie: // **NOTE:** All service configuration rules follow "last one wins" order. grpc-ecosystem/grpc-gateway's implementation looks buggy on this score, since it accepts the first matching handler.

[johanbrandhorst]

Thanks @jfhamlin it's clear us maintainers didn't understand the full situation. You've made a good case for introducing this as a flag, as you say, since we can't break backwards compatibility, even for a case where the gateway is doing the wrong thing. Another thing to add to the list of our v2 design changes.

Would you be interested in drafting a PR to get this in?

[Tommy-42]

Hi guys,

We have this use case in our API where our aliases can contains a : but it shouldn't be treated as verb I think

For example :

proto : 
  get: "/alias/{alias}"
curl : 
  localhost:8080/alias/toto:tata:tutu

708 was fixing the problem

[johanbrandhorst]

Hi @Tommy-42. Yes, we want to get in the best of both worlds, the "last one wins" as suggested by @jfhamlin. Unfortunately, because it breaks backwards compatibility, until we release v2 we will have to hide it behind a generator flag. Would you be interested in contributing a fix for this?

[Azuka]

@kassiansun, sorry I didn't realize the use-case for this. Looks like we're working towards supporting both. Thanks everyone.

[Tommy-42]

hi @johanbrandhorst ,
I can try something, But I cannot promise you anything as I don't know if I will have time to work on this saddly ( a bit overwhelm this time )

[johanbrandhorst]

We'd like to make this the default behaviour in v2, please come forward if you'd like to contribute this!

[HirdrWit]

Old problems require modern solutions. You can add allow_colon_final_segments=true to your grpc-gateway_out in the generate file.
I.e.
--grpc-gateway_out=logtostderr=true,allow_colon_final_segments=true:generated \
This works with protoc2:2.0.0 >

[johanbrandhorst]

Yeah, but we want to remove the need for this to be a flag and enable it by default.

[alexandrem]

I confirm that the grpc-gateway_out argument allow_colon_final_segments=true works well with the latest v2.

It's a good enough workaround for my use case, but I think it would be nice to have this as some kind of option to the gateway runtime mux server.

[johanbrandhorst]

Any reason we can't just make it the default behaviour?

[alexandrem]

Having it as the default would be even better, it'd have saved me some trouble :)

[johanbrandhorst]

This was made the default behaviour in v2: https://github.com/grpc-ecosystem/grpc-gateway/pull/1384