Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens ...................................................................... Patch Set 4: (35 comments) http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@53 PS3, Line 53: MonoTime deadline > nit: In RetrieveNewAuthzToken() that's passed by const reference. Is it po Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@54 PS3, Line 54: std::str > nit: drop 'virtual' Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@58 PS3, Line 58: void Sen > nit: drop Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@72 PS3, Line 72: Cache for authz tokens received from the master. A clie > This reads vague. Could you be more specific? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@84 PS3, Line 84: for 'table_id', replacing any that : // previously existed. > It would be nice to document the reasoning behind this design decision. Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@93 PS3, Line 93: t > nit: :: Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc File src/kudu/client/authz_token_cache.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@79 PS3, Line 79: // Unwrap and return any other application errors that may be returned by the > So we don't need to check the controller status at all? Why not? It's checked in RetryOrReconnectIfNecessary() http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@82 PS3, Line 82: new_status = StatusFromPB(resp_.error().status()); > Shouldn't this also be conditioned on new_status.ok()? I wouldn't expect th Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@141 PS3, Line 141: std::lock_guard<simple_spinlock> l(rpc_lock_); > What does this return if for some reason there's no key of table_id? It calls the default constructor of the mapped type (pair in this case), so we'd hit the DCHECK below. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@254 PS3, Line 254: es 'req_' > nit: updates what? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@496 PS3, Line 496: if (batcher_->client_->data_->FetchCachedAuthzToken(table()->id(), &signed_token)) { > Can you std::move it? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@498 PS3, Line 498: } else { : // Note: this is the expected path if communi > Can it be the case when the expected authz token hasn't been fetched into t If that were the case, and the tserver is enforcing access control, then yes. This generally won't happen though since, if the master supports generating authz tokens, there should be an authz token in the cache from the call to OpenTable. I've fleshed out a comment in authz_token_cache.h to clarify the workflow a bit. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h@249 PS3, Line 249: // upon learnin > Why to allocate it on the heap? Is it possible to have an instance of Auth Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360 PS3, Line 360: = std::move(a > What if the token in the cache has expired? It will get an error from the tserver and retry. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360 PS3, Line 360: *next_req_.mutable_new_scan_request()->mutable_authz_token() = std::move(authz_token); > std::move Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@362 PS3, Line 362: this is expected > I'm not sure I understand. If FetchCachedAuthzToken() returns false, then If there's nothing in the cache, it means that OpenTable() didn't get a token from the master. The assumption then is that the tserver doesn't need a token. If this is, for some reason, not the case, and the tserver _does_ want a token, the client will try to RetrieveNewAuthzToken(), but get an error from the master and hit L197 in this file. TestAuthzTokensNotSupported tests this case. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc@103 PS3, Line 103: > If this test is re-purposed to verify the authz token-related functionality Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@142 PS3, Line 142: > nit: add two more spaces Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@153 PS3, Line 153: static void StoreAuthzToken(KuduClient* client, > Line too long? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@196 PS3, Line 196: client_builder.default_rpc_timeout(MonoDelta::FromSeconds(kRpcTimeoutSecs)); : string authn_creds; : AuthenticationCredentialsPB authn_pb; > This is just for plain authn, right? Yes http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@238 PS3, Line 238: e create > nit: table_id_ ? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@295 PS3, Line 295: LOG(INFO) << "Trying to use a bad signature..."; : string bad_signature = std::move(*new_token.mutable_signature()); : // Flip the bits of the signature. : f > Is this just zeroing the signature? If so, maybe memset() is easier to wri Mentioned in the other patch, meant to ~ instead of ^ to flip the bits of the signature. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@347 PS3, Line 347: > Clever, more terse than the multi-line if check. Ack http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@349 PS3, Line 349: We slee > Ensure? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@367 PS3, Line 367: erver should respond : // with an ERROR_UNA > nit: "it's far ahead of latest TSK's sequence number" Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@432 PS3, Line 432: // After a while, the clie > Is this needed? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@470 PS3, Line 470: ASSER > remove Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1739 PS3, Line 1739: public ::testing::WithParamInterface< > Some basic verifications that we get authz tokens when we expect Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1742 PS3, Line 1742: uthzTokenMasterT > style nit: stick the asterisk to the type, not the name of the variable. Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1752 PS3, Line 1752: }; > Could you parameterize the test on this? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1757 PS3, Line 1757: nd_req(&resp)); > Do you want to be certain on the type of the error in the response? Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1760 PS3, Line 1760: RUE(s.IsNot > BTW, are we going to check authorization for the CreateTable operation and Yes, but not with tokens. Kudu will get authorization metadata directly from an external source (e.g. Sentry, Ranger) at DDL-time. Hao has some in-flight patches for that. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto@792 PS3, Line 792: GENERATE > nit: if that's about generating authz token, why is it RETRIVE, not GENERAT Done http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@80 PS3, Line 80: master_support_authz_tokens > Why not 'master_generate_authz_tokens' or 'master_enable_authz_tokens'? It I named this to be consistent with FLAGS_master_support_connect_to_master_rpc. Mentioned it in the description. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@424 PS3, Line 424: // for the table. > Do we want to restrict this only to the leader master role? Yes, this is the case, given the check at L409. -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 07 Jan 2019 08:08:16 +0000 Gerrit-HasComments: Yes
