Alexey Serbin has posted comments on this change. Change subject: server_negotiation: improve error handling ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/6154/1//COMMIT_MSG Commit Message: PS1, Line 12: rpc nit: RPC PS1, Line 14: master nit: master server 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 const Status& s = ...; PS1, Line 567: case security::VerificationResult::UNKNOWN_SIGNING_KEY: I would expect this to be along with EXPIRED_TOKEN, etc. The reason is that in rare situations the server might be not in sync with the master in terms of fresh TSK (consider the very first startup of the cluster or the startup of the cluster after very long delay when all existing TSKs expired). That looks more like a transient error, and getting a new authn token might help. 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 this is valid reasoning to issue RuntimeError() instead of NotAuthorized -- the token comes from the external party (client), not directly from us. Consider replacing with NotAuthorized here as well. If not and the reasoning is valid, then the NotAuthorized above should be replaced with RuntimeError as well. 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 have been ErrorStatusPB::FATAL_UNAUTHORIZED or we need an additional cert-specific code else. 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) consider using the same upper-case pattern PS1, Line 162: } nit: consider adding return "<unreachable>"; It helps to get rid of warnings from older versions of the gcc compiler. -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
