server/prpc: updated server according to protocol

server/prpc: updated server according to protocol

Update pRPC server to conform the protocol
https://godoc.org/github.com/luci/luci-go/common/prpc#hdr-Protocol

R=dnj@chromium.org
BUG=

[nodir, 2016-01-26 09:34:16]

Description was changed from

==========
server/prpc: updated server according to protocol

Update pRPC server to conform the protocol
https://godoc.org/github.com/luci/luci-go/common/prpc#hdr-Protocol

BUG=
==========

to

==========
server/prpc: updated server according to protocol

Update pRPC server to conform the protocol
https://godoc.org/github.com/luci/luci-go/common/prpc#hdr-Protocol

R=dnj@chromium.org
BUG=
==========

[nodir, 2016-01-26 09:34:16]

nodir@chromium.org changed reviewers:
+ dnj@chromium.org

[nodir, 2016-01-26 09:34:32]

PTAL

[dnj (Google), 2016-01-26 16:13:55]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-01-26 16:13:57]

Mostly looks good, few comments and one big ask: Can we remove as many code
paths as possible that panic? Panic in server code is an easy DoS path and
presents a fairly poor interface to users. Ideally we would never panic, even
when the input or output is malformed.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:23: csrfPrefix   = ")]}'"
I believe this should end in a newline, both for clarity and Gerrit parity.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:38: assert(len(parsed) > 0)
This is just correctness of "parseAccept". No value asserting this here.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:61: assert(f == formatBinary || f == formatJSONPB || f
== formatText)
This is just correctness. No value using panic to double-check methods. Skip the
format if we don't recognize it (default case == formatUnrecognized case). Then
no panic!

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:81: panicf("msg is nil")
nit: panic would work here, no need for "panicf".

Although since we return pRPC errors for encoding failures, maybe this should be
a status.Internal instead of a panic?

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:96: m := jsonpb.Marshaler{Indent: "\t"}
1) Don't we normally use spaces to indent JSON?
2) Why format the JSON at all here? It's extra data to send. If we want
formatted JSON, there are tools to do that on the client side.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:123: panicf("err is nil")
Default to something like Unknown if nil, rather than panic.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:163: func codeStatus(code codes.Code) int {
If it falls back to internal server error, you should remove the entries in the
map that map to internal server error.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go
File server/prpc/error.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go#ne...
server/prpc/error.go:42: func assert(condition bool) {
Can we make an effort to get rid of all instances of this, either with softer
code (gRPC/protocol error) or by removing unnecessary assertions?

Panics in core server code are an easy DoS path and, as far as I can see, add no
real value here.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go
File server/prpc/method.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go#n...
server/prpc/method.go:43: if in == nil {
Panic in response to input is bad. Let's just have this return an error.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:25: contentType string     // defaults to text/plain
Shouldn't default. Have each method set this explicitly.

Either let the method specify the headers directly (including content-type) by
having a Headers element here, or condition:
if contentType != "" {
  //set it
}
// move on

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:26: body        []byte     // must be non-nil, otherwise
write will panic.
Is an empty protobuf not acceptable? I see no need to enforce that nil => panic.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:46: if r.code == codes.Internal || r.code ==
codes.Unknown {
nit: Use switch here.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:48: logging.Errorf(c, "%s error: %s", r.code, r.body)
nit: include fields

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:65: var body []byte
nit: merge this with the block above, no need to switch on the same values
twice.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go
File server/prpc/server.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:102: r.POST(path, handle)
Do we need to support all of these methods? I thought we were only doing POST.
If not, leave the others for httprouter to 404.

Is StatusMethodNotAllowed important here? If so, we're not setting the Allow
headers accordingly. I don't think there's any value in returning
MethodNotAllowed over 404, though, so let's just not register endpoints we don't
have.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:113: res := s.respond(c, w, r, p)
nit: pull service/method out of params up front, then pass them to respond().

[nodir, 2016-01-26 17:56:40]

Patchset #3 (id:40001) has been deleted

[nodir, 2016-01-26 18:01:07]

We never panic if API input is malformed, including http requests. Look closer
to panics.
But I've removed some of them because they concern you.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:23: csrfPrefix   = ")]}'"
On 2016/01/26 16:13:56, dnj (Google) wrote:
> I believe this should end in a newline, both for clarity and Gerrit parity.

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:38: assert(len(parsed) > 0)
On 2016/01/26 16:13:56, dnj (Google) wrote:
> This is just correctness of "parseAccept". No value asserting this here.

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:61: assert(f == formatBinary || f == formatJSONPB || f
== formatText)
On 2016/01/26 16:13:56, dnj (Google) wrote:
> This is just correctness. No value using panic to double-check methods. Skip
the
> format if we don't recognize it (default case == formatUnrecognized case).
Then
> no panic!

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:81: panicf("msg is nil")
On 2016/01/26 16:13:56, dnj (Google) wrote:
> nit: panic would work here, no need for "panicf".
> 
> Although since we return pRPC errors for encoding failures, maybe this should
be
> a status.Internal instead of a panic?

err, in another CL you said that we panic with errors, not strings. This the
sole reason I have panicf. Removed it.

This can only happen if caller did not check what is passes (it does), but
whatever, returned err response

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:96: m := jsonpb.Marshaler{Indent: "\t"}
On 2016/01/26 16:13:56, dnj (Google) wrote:
> 1) Don't we normally use spaces to indent JSON?
> 2) Why format the JSON at all here? It's extra data to send. If we want
> formatted JSON, there are tools to do that on the client side.

1) \t is less bytes
2) I used to curl in the past. I don't mind formatting it in rpc. Done

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:123: panicf("err is nil")
On 2016/01/26 16:13:56, dnj (Google) wrote:
> Default to something like Unknown if nil, rather than panic.

returned ok

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:163: func codeStatus(code codes.Code) int {
On 2016/01/26 16:13:56, dnj (Google) wrote:
> If it falls back to internal server error, you should remove the entries in
the
> map that map to internal server error.

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go
File server/prpc/error.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go#ne...
server/prpc/error.go:42: func assert(condition bool) {
On 2016/01/26 16:13:56, dnj (Google) wrote:
> Can we make an effort to get rid of all instances of this, either with softer
> code (gRPC/protocol error) or by removing unnecessary assertions?
> 
> Panics in core server code are an easy DoS path and, as far as I can see, add
no
> real value here.

I don't panic unless there is an obvious bug in server-side code, but I don't
strongly object either.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go
File server/prpc/method.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go#n...
server/prpc/method.go:43: if in == nil {
On 2016/01/26 16:13:56, dnj (Google) wrote:
> Panic in response to input is bad. Let's just have this return an error.

Note that it is not server input. "in" is passed by generated code. 

But whatever, I turned it into an internal error

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:25: contentType string     // defaults to text/plain
On 2016/01/26 16:13:56, dnj (Google) wrote:
> Shouldn't default. Have each method set this explicitly.
> 
> Either let the method specify the headers directly (including content-type) by
> having a Headers element here, or condition:
> if contentType != "" {
>   //set it
> }
> // move on

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:26: body        []byte     // must be non-nil, otherwise
write will panic.
On 2016/01/26 16:13:56, dnj (Google) wrote:
> Is an empty protobuf not acceptable? I see no need to enforce that nil =>
panic.

empty and nil are two different things, but ok because zero values are cool

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:46: if r.code == codes.Internal || r.code ==
codes.Unknown {
On 2016/01/26 16:13:56, dnj (Google) wrote:
> nit: Use switch here.

so I counted lines and switch is 1 line more, but whatever

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:48: logging.Errorf(c, "%s error: %s", r.code, r.body)
On 2016/01/26 16:13:57, dnj (Google) wrote:
> nit: include fields

Done, also included http status

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:65: var body []byte
On 2016/01/26 16:13:56, dnj (Google) wrote:
> nit: merge this with the block above, no need to switch on the same values
> twice.

Done.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go
File server/prpc/server.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:102: r.POST(path, handle)
On 2016/01/26 16:13:57, dnj (Google) wrote:
> Do we need to support all of these methods? I thought we were only doing POST.
> If not, leave the others for httprouter to 404.
> 
> Is StatusMethodNotAllowed important here? If so, we're not setting the Allow
> headers accordingly. I don't think there's any value in returning
> MethodNotAllowed over 404, though, so let's just not register endpoints we
don't
> have.

We had this discussion and decided to handle all methods and include grpc code
in the response.
Included Allow in response.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:113: res := s.respond(c, w, r, p)
On 2016/01/26 16:13:57, dnj (Google) wrote:
> nit: pull service/method out of params up front, then pass them to respond().

Done.

[nodir, 2016-01-26 18:08:13]

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/response.go...
server/prpc/response.go:48: logging.Errorf(c, "%s error: %s", r.code, r.body)
On 2016/01/26 18:01:07, nodir wrote:
> On 2016/01/26 16:13:57, dnj (Google) wrote:
> > nit: include fields
> 
> Done, also included http status

actually did not include status

[dnj, 2016-01-26 18:50:27]

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/encoding.go...
server/prpc/encoding.go:81: panicf("msg is nil")
> err, in another CL you said that we panic with errors, not strings. This the
> sole reason I have panicf. Removed it.

Ah sorry I meant something like: panic(errors.New("...")), still return errors.
Maybe make panicf => panicErr(string)? Don't really care that much, so this is a
nit.

> This can only happen if caller did not check what is passes (it does), but
> whatever, returned err response

Thanks:)

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go
File server/prpc/error.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/error.go#ne...
server/prpc/error.go:42: func assert(condition bool) {
On 2016/01/26 18:01:07, nodir wrote:
> On 2016/01/26 16:13:56, dnj (Google) wrote:
> > Can we make an effort to get rid of all instances of this, either with
softer
> > code (gRPC/protocol error) or by removing unnecessary assertions?
> > 
> > Panics in core server code are an easy DoS path and, as far as I can see,
add
> no
> > real value here.
> 
> I don't panic unless there is an obvious bug in server-side code, but I don't
> strongly object either.

Yeah it's a tough line to draw. A panic will completely reset a VM instance,
though, so it's a fairly high price to pay. If there's a non-panic alternative
(e.g., log and return Internal) it's IMO a better thing to deploy.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go
File server/prpc/method.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/method.go#n...
server/prpc/method.go:43: if in == nil {
On 2016/01/26 18:01:07, nodir wrote:
> On 2016/01/26 16:13:56, dnj (Google) wrote:
> > Panic in response to input is bad. Let's just have this return an error.
> 
> Note that it is not server input. "in" is passed by generated code. 
> 
> But whatever, I turned it into an internal error

Yep.

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go
File server/prpc/server.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:102: r.POST(path, handle)
On 2016/01/26 18:01:07, nodir wrote:
> On 2016/01/26 16:13:57, dnj (Google) wrote:
> > Do we need to support all of these methods? I thought we were only doing
POST.
> > If not, leave the others for httprouter to 404.
> > 
> > Is StatusMethodNotAllowed important here? If so, we're not setting the Allow
> > headers accordingly. I don't think there's any value in returning
> > MethodNotAllowed over 404, though, so let's just not register endpoints we
> don't
> > have.
> 
> We had this discussion and decided to handle all methods and include grpc code
> in the response.
> Included Allow in response.

Seems pointless, but OK. IMO either a client knows what it's doing (POST,
protocol, etc.) or it doesn't, and in the latter case this won't help it at all.
I see no value at all to being partially RESTful. We're abusing HTTP/POST here
to implement RPC. I don't think there's any value in trying to have conformance
in this single minor area, moreso b/c basically nothing at all will consume this
response code meaningfully.

If you're going to do this, implement a simple separate handle method that
returns StatusMethodNotAllowed.

r.POST(path, handle)
r.GET(path, handleNotAllowed)
etc.

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/encoding.go...
server/prpc/encoding.go:57: panic("cannot happen")
nit: if this cannot happen, get rid of the default case.

IMO just:

case formatUnrecognized:
  fallthrough
default:
  continue

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go...
server/prpc/response.go:52: }.Errorf(c, "%s", body)
nit: "body" is a []byte, but you're logging it as a string. This will show up as
[1, 2, 3, 4] etc. I'd either string-convert (string(body)) or log as %v if you
want the byte output.

Also include some context in the Errorf: "Masking %s error message: %s", r.code,
string(body))

[nodir, 2016-01-26 19:06:09]

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go
File server/prpc/server.go (right):

https://codereview.chromium.org/1638493004/diff/20001/server/prpc/server.go#n...
server/prpc/server.go:102: r.POST(path, handle)
On 2016/01/26 18:50:27, dnj wrote:
> On 2016/01/26 18:01:07, nodir wrote:
> > On 2016/01/26 16:13:57, dnj (Google) wrote:
> > > Do we need to support all of these methods? I thought we were only doing
> POST.
> > > If not, leave the others for httprouter to 404.
> > > 
> > > Is StatusMethodNotAllowed important here? If so, we're not setting the
Allow
> > > headers accordingly. I don't think there's any value in returning
> > > MethodNotAllowed over 404, though, so let's just not register endpoints we
> > don't
> > > have.
> > 
> > We had this discussion and decided to handle all methods and include grpc
code
> > in the response.
> > Included Allow in response.
> 
> Seems pointless, but OK. IMO either a client knows what it's doing (POST,
> protocol, etc.) or it doesn't, and in the latter case this won't help it at
all.
> I see no value at all to being partially RESTful. We're abusing HTTP/POST here
> to implement RPC. I don't think there's any value in trying to have
conformance
> in this single minor area, moreso b/c basically nothing at all will consume
this
> response code meaningfully.
> 
> If you're going to do this, implement a simple separate handle method that
> returns StatusMethodNotAllowed.
> 
> r.POST(path, handle)
> r.GET(path, handleNotAllowed)
> etc.

ah, Server.handle was exported at some point, that's why it checked method. 
Anyway, removed other methods and updated protocol.

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/encoding.go
File server/prpc/encoding.go (right):

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/encoding.go...
server/prpc/encoding.go:57: panic("cannot happen")
On 2016/01/26 18:50:27, dnj wrote:
> nit: if this cannot happen, get rid of the default case.
> 
> IMO just:
> 
> case formatUnrecognized:
>   fallthrough
> default:
>   continue

just default: continue is enough I guess

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go...
server/prpc/response.go:52: }.Errorf(c, "%s", body)
On 2016/01/26 18:50:27, dnj wrote:
> nit: "body" is a []byte, but you're logging it as a string. This will show up
as
> [1, 2, 3, 4] etc. I'd either string-convert (string(body)) or log as %v if you
> want the byte output.

%s converts bytes to string https://play.golang.org/p/m39fvwX43z
 
> Also include some context in the Errorf: "Masking %s error message: %s",
r.code,
> string(body))

You are suggesting all our internal rpc errors to have this prefix. The main
point here is not masking (although we do it), but the error itself.

[dnj, 2016-01-26 19:26:38]

lgtm w/ some nits

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go
File server/prpc/response.go (right):

https://codereview.chromium.org/1638493004/diff/60001/server/prpc/response.go...
server/prpc/response.go:52: }.Errorf(c, "%s", body)
On 2016/01/26 19:06:09, nodir wrote:
> On 2016/01/26 18:50:27, dnj wrote:
> > nit: "body" is a []byte, but you're logging it as a string. This will show
up
> as
> > [1, 2, 3, 4] etc. I'd either string-convert (string(body)) or log as %v if
you
> > want the byte output.
> 
> %s converts bytes to string https://play.golang.org/p/m39fvwX43z

Oh cool, didn't know that!

> > Also include some context in the Errorf: "Masking %s error message: %s",
> r.code,
> > string(body))
> 
> You are suggesting all our internal rpc errors to have this prefix. The main
> point here is not masking (although we do it), but the error itself.

Wait, this is a logged error (shows up in our logs), not a returned error. I
just think "Something went wrong" showing up in the log is less useful than,
"Masked Internal/Unknown error: Something went wrong". Especially b/c searching
for "where did this error occur in the code?" is excruciatingly difficult when
there is no text string context.

But I'll leave this up to you as a nit.

https://codereview.chromium.org/1638493004/diff/80001/server/prpc/decoding.go
File server/prpc/decoding.go (right):

https://codereview.chromium.org/1638493004/diff/80001/server/prpc/decoding.go...
server/prpc/decoding.go:82: // no body, no message
Is this true? A completely zero-value protobuf would be zero length, no? Do we
need to check this, anyway? If there is insufficient body, the unmarshalling
will fail below.

https://codereview.chromium.org/1638493004/diff/80001/server/prpc/decoding.go...
server/prpc/decoding.go:99: panic("cannot happen")
nit: I'd prefer that you either don't check this case (b/c it can't happen) or
return a protocol error here instead of panic. I understand that this can never
happen, but having a panic inline with user request handling is a bad practice
in general.

[dnj, 2016-01-27 00:08:46]

Taking ownership, will continue here:
https://codereview.chromium.org/1636873006