Alexey Serbin 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 3: (27 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 possible to unify those two notations? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@54 PS3, Line 54: virtual nit: drop 'virtual' http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@58 PS3, Line 58: virtual nit: drop http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@72 PS3, Line 72: Manages distribution and reacquisition of authz tokens. This reads vague. Could you be more specific? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@84 PS3, Line 84: No checking is done to verify the : // expiration or validity of the returned token. It would be nice to document the reasoning behind this design decision. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@93 PS3, Line 93: : nit: :: 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: it updates nit: updates what? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@498 PS3, Line 498: // Note: this is the expected path if communicating with an older-versioned : // master that does not support authz tokens. Can it be the case when the expected authz token hasn't been fetched into the cache yet? What will happen in that case? Will the operation fetch a new authz token and retry? 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: std::unique_ptr Why to allocate it on the heap? Is it possible to have an instance of AuthzTokenCache instead? 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: = authz_token What if the token in the cache has expired? 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 it means the cache hasn't fetched a token yet, but de-facto the master can support generating authz tokens, right? In the latter case, what will happen with the scan request -- will it be retried with authnz token when it's available? 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: AuthnTokenExpireITestBase If this test is re-purposed to verify the authz token-related functionality as well, maybe rename it to AuthTokenExpireITestBase or alike? 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 http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@196 PS3, Line 196: AuthenticationCredentialsPB authn_pb; : authn_pb.set_real_user(user); : CHECK(authn_pb.SerializeToString(&authn_creds)); This is just for plain authn, right? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@238 PS3, Line 238: table_id nit: table_id_ ? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@295 PS3, Line 295: for (int i = 0; i < bad_signature.length(); i++) { : char* first_byte = &bad_signature[i]; : *first_byte ^= *first_byte; : } Is this just zeroing the signature? If so, maybe memset() is easier to write and follow? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@349 PS3, Line 349: Ensured Ensure? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@367 PS3, Line 367: it'll appear invalid : // to the servers. nit: "it's far ahead of latest TSK's sequence number" Or replace 'invalid' with 'unknown'. 'invalid' seems vague. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@432 PS3, Line 432: LOG(INFO) << s.ToString(); Is this needed? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@470 PS3, Line 470: private: remove 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: Add some basic verifications that we get tokens when we expect. Some basic verifications that we get authz tokens when we expect http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1742 PS3, Line 1742: char *kTableName style nit: stick the asterisk to the type, not the name of the variable. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1757 PS3, Line 1757: resp.has_error() Do you want to be certain on the type of the error in the response? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1760 PS3, Line 1760: CreateTable BTW, are we going to check authorization for the CreateTable operation and other DDL operations? 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: RETRIEVE nit: if that's about generating authz token, why is it RETRIVE, not GENERATE? 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 seems that 'support' is kind of intrinsic property, but not a configurable one. If you need to have it denoting something irregular for testing, why not to add 'master_disable_authz_tokens' instead and mention that it is for tests only? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@424 PS3, Line 424: if (PREDICT_TRUE(FLAGS_master_support_authz_tokens)) { Do we want to restrict this only to the leader master role? -- 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: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 07 Jan 2019 06:06:33 +0000 Gerrit-HasComments: Yes
