Dan Burkert has posted comments on this change. Change subject: server_negotiation: improve error handling ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/6154/1/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS1, Line 562: > nit: here and elsewhere with the same pattern in this patch consider using Could you expand on this? Holding a reference to an rvalue seems like a bug to me, I'm surprised it even compiles. PS1, Line 567: case security::VerificationResult::UNKNOWN_SIGNING_KEY: > I would expect this to be along with EXPIRED_TOKEN, etc. The reason is tha I was imagining that this would happen if the tablet server had been partitioned for a while, and it hadn't learned about the newest TSKs. The token validation code checks that the token is not expired quite early, so I don't think this can be caused by the token being signed by an ancient TSK which has been purged. Is there another scenario I'm not thinking of? PS1, Line 582: // This is a runtime error because there should be no way a client could : // get a signed authn token without a subject. > This is not the part of this patch, but while you are at it: I'm not sure t Done PS1, Line 610: ErrorStatusPB::FATAL_INVALID_AUTHENTICATION_TOKEN > I'm not sure this is the right error code to send back. It seems it should Done http://gerrit.cloudera.org:8080/#/c/6154/1/src/kudu/security/token_verifier.cc File src/kudu/security/token_verifier.cc: PS1, Line 130: pki > nit: in all other places it's TODO(PKI) Done PS1, Line 162: } > nit: consider adding Done -- To view, visit http://gerrit.cloudera.org:8080/6154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64f9c0f59aa608bf5078d65883a7d9a6fb186c04 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
