Todd Lipcon has posted comments on this change. Change subject: WIP [rpc] introduced per-RPC credentials policy ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/6875/3//COMMIT_MSG Commit Message: Line 7: WIP [rpc] introduced per-RPC credentials policy nit: better to say "Introduce" instead of "Introduced" for consistency with most of our messages (see https://chris.beams.io/posts/git-commit/ point 5) PS3, Line 23: but other than http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: Line 386: ++it; Can you add a comment why not breaking here? PS3, Line 411: { : UserCredentials credentials; : credentials.set_real_user(conn_id.user_credentials().real_user()); : credentials.set_authn_token((policy == CredentialsPolicy::PRIMARY_CREDENTIALS) : ? boost::none : reactor()->messenger()->authn_token()); : (*conn)->set_local_user_credentials(std::move(credentials)); : } There's a bit of a mess here, in that the Proxy object has set_user_credentials(), and UserCredentials purports to be an external-facing class, but here we're basically ignoring any token that the user might have tried to set in UserCredentials and instead overriding it with the token from the Messenger. It appears that we basically don't use Proxy::set_user_credentials() anywhere, though. Perhaps it would be best to make it an internal-facing class for KRPC, so this is a bit less messy, given it's not used anyway? Alternatively, maybe putting authn_token in UserCredentials isn't the right method, adn instead we should just pass it directly into the StartConnectionNegotiation call? It's only needed during negotiation, and once negotiated, we only need to remember how we authenticated the connection, not the actual token used, right? Line 516: it = client_conns_.erase(it); can you preserve the assertion here and ensure that we actually erase() exactly one connection in this loop? http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: PS3, Line 51: on nit: "on a" PS3, Line 59: username&password we don't actually support username/password anymore, right? -- To view, visit http://gerrit.cloudera.org:8080/6875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
