grpc: propagate error and http status to caller when received HTTP method is not POST

[ejona86]

It came up during discussion that grpc-go may not be verifying that the HTTP method is POST. grpc-go should fail RPCs if the method is anything but POST. While there is an experimental GET protocol (which isn't really making any progress toward stabilization), it is quite different and wouldn't be supported by grpc-go without explicit support.

[menghanl]

fixed by #4241

[ejona86]

Reopening because the fix doesn't communicate the failure to the remote appropriately.

[dfawley]

What is the appropriate way to communicate the failure?

[ejona86]

An HTTP response with HTTP status code. You can include a gRPC status code if you want, but the main expected case is "the client isn't speaking grpc" so it doesn't matter much.

[ejona86]

Java right now responds with HTTP 405, INTERNAL gRPC status, and a text/plain response. HTTP 501 would also be appropriate, although earlier versions of HTTP required supporting GET+HEAD and so claimed 405 was more appropriate for those methods.

In no way do I claim Java's implementation is ideal. It was just much better than what was there before.

[bdhess]

Is there more context on why this change is needed? It took me a long time to root cause why a package update to google.golang.org/grpc broke clients.

Turns out that gRPC-C++ has a ClientContext settings for idempotency which causes the HTTP method to be PUT instead of POST.

I guess that setting is marked experimental -- is the experiment not moving forward?

[ejona86]

@bdhess, why were you using PUT?

I'm surprised PUT is still there in C++. That feature died. It was to allow things like QUIC to do 0-RTT transmission of the RPC, which can be replayed. However, it was later determined that "idempotent" isn't the same as "safe to replay by an attacker" and so PUT was nerfed in QUIC and became no faster than POST.

The work was supplanted by GET work which is still 0-RTT, but I think that was recently stripped from C++ because it was incomplete, hadn't moved forward in years, and was a maintenance burden. I'm surprised PUT is still there, but it is probably because it has low maintenance cost. There's still some interest in GET, but adding the code back was seen to be better than maintaining it in limbo.

There was no GET/PUT testing with Go. That work was all focusing on mobile so Java/C++ were the only things impacted (and client-side-only in Java). It never got out of testing though; it wouldn't be considered production ready. PUT is simple enough it probably mostly worked, but the compatibility story is completely missing. I know GET server-side support was broken in C++ because the server didn't verify the HTTP method matched what was allowed by the RPC method. That's probably less of a concern for PUT, but probably still a bad idea.

[bdhess]

Ack, thanks for the explanation. Our code was calling that setter in grpc::ClientContext mostly because it seemed like a not-unreasonable thing to mark idempotent operations as such -- not necessarily for a MITMer to replay, but for client side retry logic akin to the standard gRPC retry policy. I'm not sure we ever noticed the 'experimental' comment until now.

[ejona86]

@bdhess, makes sense. That's one of the half-baked things about it actually. Java, for example, also allows setting a method as idempotent (generally copied by code generator from the proto definition), but it wouldn't trigger the PUT logic because it was unclear if the server supported PUT. Instead, it can just be used locally by interceptors and the like. The API really gives no indication that it could cause compatibility issues, and it should.

[zasweq]

Fixed by #5364.