Alexey Serbin has posted comments on this change. Change subject: [rpc] introduce per-RPC credentials policy ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: PS5, Line 163: SatisfiesCredentialsPolicy > yea, I think my main complaint about this is that we're calling SatisfiesCr We discussed this on-line (kudu-security Slack channel) and decided to add accessor for 'credentials_policy_' member. Line 215: conn->set_remote_features(client_negotiation.take_server_features()); > I think we're being pessimistic here in a very certain case: We discussed that on Slack kudu-security channel and the decision is to keep it as is (keep it simple). The issue with AuthenticationType is that it's not known before actual negotiation has happened, but we need to know about the policy of any connection prior to that because we want to find appropriate connection in ReactorThread::FindOrStartConnection(). Also, we discussed 'upgrading' the policy de facto as soon as the connection has been negotiated, but that seems to bring some additional complexity to fix a very corner case when authentication token expires on a connection which has been negotiated using the ANY_CREDENTIALS policy, but the actually there were no authentication token available to use, so the connection ended up to be satisfying the PRIMARY_CREDENTIALS policy. http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: PS5, Line 368: for (auto it = range.first; it != range.second; ++it) { : const auto& c = it->second; : if (c->scheduled_for_shutdown()) { : // This connection is not to be used for any new calls. : continue; : } : if (!c->SatisfiesCredentialsPolicy(cred_policy)) { : // Do not use a connection with a non-compliant credentials policy. : // Instead, open a new one, while marking the former as scheduled for : // shutdown. This process converges: any connection that satisfies the : // PRIMARY_CREDENTIALS policy automatically satisfies the ANY_CREDENTIALS : // policy as well. The idea is to keep only one usable connection identified : // by the specified 'conn_id'. : c->set_scheduled_for_shutdown(); : continue; : } : : // If the test-only 'one-connection-per-RPC' mode is enabled, connections : // are re-established at every RPC call. : if (PREDICT_FALSE(FLAGS_rpc_reopen_outbound_connections)) { : c->set_scheduled_for_shutdown(); : continue; : } : } : // Second pass: shutdown idle connections to the target destination if they : // have been marked. Non-idle connections will be taken care of later by the : // idle connection scanner. : scoped_refptr<Connection> found_conn; : for (auto it = range.first; it != range.second;) { : const auto& c = it->second; : if (c->scheduled_for_shutdown()) { : if (c->Idle()) { : DCHECK_EQ(Connection::CLIENT, c->direction()); : c->Shutdown(Status::NetworkError("connection is closed due to non-reuse policy")); : it = client_conns_.erase(it); : continue; : } : } else { : DCHECK(!found_conn); : found_conn = c; : // Appropriate connection is found; continue further to take care of the : // rest of connections marked for shutdown by the first pass above. : } : ++it; : } > I think these loops can be simplified by combining them: I thought breaking this into two parts would give the code better readability, so it was intentional. I see that's not the case. Fine, I'll update this piece. http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: PS5, Line 295: PRC > RPC Done -- 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: 5 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
