[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/12122 )

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Reviewed-on: http://gerrit.cloudera.org:8080/12122
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
Reviewed-by: Hao Hao 
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,347 insertions(+), 95 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved
  Hao Hao: Looks good to me, approved

--
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: merged
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Hao Hao (Code Review)]

Hao Hao 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 12: Code-Review+2


--
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: 12
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 19:14:29 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Alexey Serbin (Code Review)]

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 12: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@103
PS10, Line 103: VLOG(1) << Substitute("Putting new token for tab
> I suppose, but the work done under lock is pretty paltry. I've generally us
SGTM


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto@638
PS10, Line 638:  can always be expected with this response, unles
> Ah, you're right. If the response has an error, we shouldn't expect an auth
Thanks for clarification!



--
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: 12
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 05:25:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#12).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,347 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/12
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Hao Hao (Code Review)]

Hao Hao 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 11: Code-Review+1

LGTM, might need to look into the IWYU failure.


--
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: 11
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 02:19:44 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 11: Code-Review+1


--
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: 11
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 01:38:35 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#11).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,345 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/11
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 10:

(21 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@47
PS10, Line 47: asynchonous
> asynchronous
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@84
PS10, Line 84:   // Adds an authz token to the cache for 'table_id', replacing 
any that
 :   // previously existed.
 :   void Put(const std::string& table_id,
 :security::SignedTokenPB authz_token);
> Yep, indeed -- I took another look and found them there.
Ack


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@123
PS10, Line 123: table id
> nit: either 'table ID' or change 'table ID' --> 'table id' at line 130.
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@63
PS10, Line 63:
> style nit: 4 spaces
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@84
PS10, Line 84:   if (new_status.ok() && resp_.has_authz_token()) {
 : client_->data_->authz_token_cache_.Put(table_->id(), 
resp_.authz_token());
 :   }
 :   user_cb_.Run(new_status);
> What if (new_status.ok() && !resp_.has_authz_token())?  Does that situation
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@73
PS10, Line 73: void RetrieveAuthzTokenRpc::SendRpcCb(const Status& status) {
 :   Status new_status = status;
 :   // Check for generic master RPC errors.
 :   if (RetryOrReconnectIfNecessary(_status)) {
 : return;
 :   }
 :   // Unwrap and return any other application errors that may be 
returned by the
 :   // master service.
 :   if (new_status.ok() && resp_.has_error()) {
 : new_status = StatusFromPB(resp_.error().status());
 :   }
 :   if (new_status.ok() && resp_.has_authz_token()) {
 : client_->data_->authz_token_cache_.Put(table_->id(), 
resp_.authz_token());
 :   }
 :   user_cb_.Run(new_status);
 : }
 :
 : string RetrieveAuthzTokenRpc::ToString() const {
 :   return Substitute("$0 { table: '$1' ($2), attempt: $3 }", 
AsyncLeaderMasterRpc::ToString(),
 :   req_.table().table_name(), req_.table().table_id(), 
num_attempts());
 : }
> nit: could you reorder these to match the order of declaration in the .h fi
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@103
PS10, Line 103: std::lock_guard l(token_lock_);
> nit: would it make sense to use shared_lock pattern here?  For example, tha
I suppose, but the work done under lock is pretty paltry. I've generally used 
that pattern when there is more work to be done per reader or per writer; in 
this case, both readers and writers are just updating entries in a map.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@123
PS10, Line 123: rpc_and_cbs->second.emplace_back(std::move(callback));
> nit: add DCHECK(!rpc_and_cbs->second.empty()) before adding a new element (
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129
PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } })
> whoops, apparently you cannot
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143
PS10, Line 143: rpc_and_cbs.second
> I'm not sure .second is necessary here: the EraseKeyReturnValuePtr() is sup
Right, but the mapped type is the RpcAndCallbacks pair.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143
PS10, Line 143: rpc_and_cbs.second
> I take this back: I missed the fact the value in the map is std::pair.
Ack


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc@5833
PS10, Line 5833:   ASSERT_TRUE(new_data->FetchCachedAuthzToken(table_id, 
_token));
> Does it make sense to verify that storing authz token into 'new_data' does
Done


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h@637
PS10, Line 637: FRIEND_TEST(ClientTest, TestInvalidAuthorizationTokens);
> Is it still 

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Alexey Serbin (Code Review)]

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 10:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@123
PS10, Line 123: table id
nit: either 'table ID' or change 'table ID' --> 'table id' at line 130.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129
PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } })
> nit: could you get away with just { rpc, { std::move(callback) } } ?
whoops, apparently you cannot

but you could get away with RpcAndCallbacks(rpc, { std::move(callback) })


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143
PS10, Line 143: rpc_and_cbs.second
> I'm not sure .second is necessary here: the EraseKeyReturnValuePtr() is sup
I take this back: I missed the fact the value in the map is std::pair.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc@5833
PS10, Line 5833:   ASSERT_TRUE(new_data->FetchCachedAuthzToken(table_id, 
_token));
Does it make sense to verify that storing authz token into 'new_data' does not 
affect token stored via 'data'?  I.e., fetch current token from 'data' and 
verify that's still 'new_token'?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h@637
PS10, Line 637: FRIEND_TEST(ClientTest, TestInvalidAuthorizationTokens);
Is it still around?  For some reason, I could find this test in this 
changelist.  Am I missing something?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@143
PS10, Line 143:   const char* kTableName = "test-table";
  :   const char* kUser = "token-user";
  :   const char* kBadUser = "bad-token-user";
nit: 'const char* x const' protects the public pointer member 'x' itself.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@207
PS10, Line 207: int
nit: the TotalCount() method of the metric returns uint64_t, why not to use 
this type for the return type of this function as well?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@248
PS10, Line 248: with
drop


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@484
PS10, Line 484: keep
  :   // failing
are always invalid ?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@540
PS10, Line 540:   shared_ptr session(client_->NewSession());
Is this needed?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc@1771
PS10, Line 1771: ::testing::Values(true, false)
nit: could use ::testing::Bool() instead



--
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: 10
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 11 Feb 2019 08:06:25 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Alexey Serbin (Code Review)]

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 10:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@47
PS10, Line 47: asynchonous
asynchronous


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@49
PS10, Line 49: class RetrieveAuthzTokenRpc : public 
AsyncLeaderMasterRpc,
> I think it would be uglier, and since 100 is the strict limit, I'm OK with
SGTM: it's just a nit, feel free to leave as is.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@84
PS10, Line 84:   // Adds an authz token to the cache for 'table_id', replacing 
any that
 :   // previously existed.
 :   void Put(const std::string& table_id,
 :security::SignedTokenPB authz_token);
> Look again? They're both there.
Yep, indeed -- I took another look and found them there.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@63
PS10, Line 63:
style nit: 4 spaces


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@84
PS10, Line 84:   if (new_status.ok() && resp_.has_authz_token()) {
 : client_->data_->authz_token_cache_.Put(table_->id(), 
resp_.authz_token());
 :   }
 :   user_cb_.Run(new_status);
What if (new_status.ok() && !resp_.has_authz_token())?  Does that situation 
need some special handling or at least DCHECK()?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@73
PS10, Line 73: void RetrieveAuthzTokenRpc::SendRpcCb(const Status& status) {
 :   Status new_status = status;
 :   // Check for generic master RPC errors.
 :   if (RetryOrReconnectIfNecessary(_status)) {
 : return;
 :   }
 :   // Unwrap and return any other application errors that may be 
returned by the
 :   // master service.
 :   if (new_status.ok() && resp_.has_error()) {
 : new_status = StatusFromPB(resp_.error().status());
 :   }
 :   if (new_status.ok() && resp_.has_authz_token()) {
 : client_->data_->authz_token_cache_.Put(table_->id(), 
resp_.authz_token());
 :   }
 :   user_cb_.Run(new_status);
 : }
 :
 : string RetrieveAuthzTokenRpc::ToString() const {
 :   return Substitute("$0 { table: '$1' ($2), attempt: $3 }", 
AsyncLeaderMasterRpc::ToString(),
 :   req_.table().table_name(), req_.table().table_id(), 
num_attempts());
 : }
nit: could you reorder these to match the order of declaration in the .h file?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@103
PS10, Line 103: std::lock_guard l(token_lock_);
nit: would it make sense to use shared_lock pattern here?  For example, that 
might make sense if multiple sessions are run concurrently for the same 
instance of KuduClient.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@123
PS10, Line 123: rpc_and_cbs->second.emplace_back(std::move(callback));
nit: add DCHECK(!rpc_and_cbs->second.empty()) before adding a new element (that 
would pair the DCHECK at line 145)?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129
PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } })
nit: could you get away with just { rpc, { std::move(callback) } } ?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143
PS10, Line 143: rpc_and_cbs.second
I'm not sure .second is necessary here: the EraseKeyReturnValuePtr() is 
supposed to the return the mapped type, i.e. vector, right?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@301
PS10, Line 301: char* byte = _signature[i];
  : *byte = ~*byte;
nit: I think it's possible to use less indirection-related operations here:

auto& byte = bad_signature[i];
byte = ~byte;


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto@638
PS10, Line 638: A token can always be expected with this response
> Non-leader masters do not respond to 

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@49
PS10, Line 49: class RetrieveAuthzTokenRpc : public 
AsyncLeaderMasterRpc,
> style nit: it would be nice if it's possible to fit it into 80 characters,
I think it would be uglier, and since 100 is the strict limit, I'm OK with 
going over 80, unless you feel strongly about it.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@84
PS10, Line 84:   // Adds an authz token to the cache for 'table_id', replacing 
any that
 :   // previously existed.
 :   void Put(const std::string& table_id,
 :security::SignedTokenPB authz_token);
> I'm not sure I saw the implementation for this method and the Fetch() metho
Look again? They're both there.


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto@638
PS10, Line 638: A token can always be expected with this response
> Do non-leader masters also generate authz tokens and send them with the res
Non-leader masters do not respond to GetTableSchema requests, see 
MasterServiceImpl::GetTableSchema() in master/master_service.cc.



--
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: 10
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 03 Feb 2019 06:20:23 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Alexey Serbin (Code Review)]

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 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@49
PS10, Line 49: class RetrieveAuthzTokenRpc : public 
AsyncLeaderMasterRpc,
style nit: it would be nice if it's possible to fit it into 80 characters, 
maybe put 'public AsyncLeaderMasterRpc...' on a new line?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@84
PS10, Line 84:   // Adds an authz token to the cache for 'table_id', replacing 
any that
 :   // previously existed.
 :   void Put(const std::string& table_id,
 :security::SignedTokenPB authz_token);
I'm not sure I saw the implementation for this method and the Fetch() method as 
well (at least I could not find it in the corresponding .cc file).  Am I 
missing something here?


http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto@638
PS10, Line 638: A token can always be expected with this response
Do non-leader masters also generate authz tokens and send them with the 
response?



--
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: 10
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Jan 2019 20:40:16 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 10: Code-Review+1


--
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: 10
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Jan 2019 00:58:06 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc@463
PS8, Line 463: // Enforce access control, and make elections much more 
frequent.
> These all have an effect _after_ the cluster has been started?
Good callout, a couple of them do affect state set at table create time (e.g. 
the periodic timers).



--
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: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Jan 2019 00:56:31 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#10).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,335 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/10
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Hao Hao (Code Review)]

Hao Hao 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 9: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287
PS6, Line 287: after being told go retrieve a new token
> It's verified by the fact that we got a new token at L292, and there are VL
Sounds good.



-- 
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: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:28:37 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 9: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc@463
PS8, Line 463: // Enforce access control, and make elections much more 
frequent.
These all have an effect _after_ the cluster has been started?



--
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: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 22:40:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#9).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,332 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/9
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Hao Hao (Code Review)]

Hao Hao 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 6: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287
PS6, Line 287: after being told go retrieve a new token
Is there a way to verify this? e.g. a log?



--
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: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 19:56:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287
PS6, Line 287: after being told go retrieve a new token
> Is there a way to verify this? e.g. a log?
It's verified by the fact that we got a new token at L292, and there are VLOG 
messages in the token cache when new tokens are retrieved.



--
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: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 20:01:49 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#7).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,332 insertions(+), 96 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/7
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 6: Code-Review+1


--
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: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 07:32:52 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 6:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h@134
PS5, Line 134: authz_rpcs_;
> nit: why not name it 'authz_rpcs_' to match 'authz_tokens_'?
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc@466
PS5, Line 466:  Parse the server part
> nit: use StoreAuthzToken() instead?
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc@5853
PS5, Line 5853:   
client_->data_->RetrieveAuthzTokenAsync(client_table_.get(), 
s.AsStatusCallback(),
> Is it possible to change the logic to create a new client and only retrieve
That's possible, we could also not run RetrieveAuthzTokenAsync if there is a 
token in the cache. I'm going to hold off on it though since I don't think that 
adds to the test coverage and would make this test more complicated.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc
File src/kudu/integration-tests/auth_token_expire-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@143
PS5, Line 143: i
> nit: extra space.
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@254
PS5, Line 254: p
> nit: space.
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@184
PS5, Line 184: Insert
> nit: 'Inserts' to aligned with other comments style.
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@205
PS5, Line 205: Get
> nit: 'Gets'
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@281
PS5, Line 281:
> Can you add a comment to explain what is the expected behavior in this case
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@352
PS5, Line 352:  auto& e : er
> Nit: got two spaces here.
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@362
PS5, Line 362:
> Why not parameterized the functor here as well?
Done


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@369
PS5, Line 369: yPB tsk;
> Should a similar test be exist for authn token as well?
A similar test exists for authn tokens in security-unknown-tsk-itest. I chose 
to put this test here because the logic differed enough that parameterizing 
security-unknown-tsk-itest would have been a lot of work and I think would've 
made the test harder to follow.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@477
PS5, Line 477: token_ratio = 1.0;
> Will a similar also apply for authn tokens? Also,  wondering how you consid
auth_token_expire-itest has some tests that explicitly test for expired tokens 
/ token reacquisition during master leader changes (e.g. 
MultiMasterIdleConnectionsITest), which is similar to this (invalid tokens 
during master election storms). I opted to not reuse some of the existing tests 
because parameterizing them would be non-trivial and make them harder to 
understand (and I found some tests a bit confusing to start with, given a few 
of them are targeting very specific error scenarios).

auth_token_expire-itest tests generally target expiration of tokens. 
authz_token-itest targets other error scenarios.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@523
PS5, Line 523:   // Client should have no problems connecting to an old cluster.
 :   ASSERT_OK(Inse
> What happens if a old client try to communicate with servers that require a
That'd be the same as sending requests with no authz tokens, so the client 
would get an error. That's harder to test, since I'm not maintaining the old 
behavior of the client through some config flag, but 
https://gerrit.cloudera.org/c/11751/10/src/kudu/tserver/tablet_server_authorization-test.cc#259
 shows the behavior of using the proxy with no token.



--
To view, visit http://gerrit.cloudera.org:8080/12122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#6).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,331 insertions(+), 96 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/6
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Hao Hao (Code Review)]

Hao Hao 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 5:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h@134
PS5, Line 134: retrieve_authz_rpcs_
nit: why not name it 'authz_rpcs_' to match 'authz_tokens_'?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc@466
PS5, Line 466: authz_token_cache_.Put
nit: use StoreAuthzToken() instead?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc@5853
PS5, Line 5853:   
client_->data_->RetrieveAuthzTokenAsync(client_table_.get(), 
s.AsStatusCallback(),
Is it possible to change the logic to create a new client and only retrieve 
authz token when there isn't one cached? So that we can have a deterministic 
result that there is only one RPC being sent in this case?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc
File src/kudu/integration-tests/auth_token_expire-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@143
PS5, Line 143:
nit: extra space.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@254
PS5, Line 254:
nit: space.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@184
PS5, Line 184: Insert
nit: 'Inserts' to aligned with other comments style.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@205
PS5, Line 205: Get
nit: 'Gets'


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@281
PS5, Line 281:   LOG(INFO) << "Trying to use the wrong user's token...";
Can you add a comment to explain what is the expected behavior in this case? 
Especially why operations in L285 is expected to succeed.


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@362
PS5, Line 362: ASSERT_OK(ScanFromTable(client_table_.get()));
Why not parameterized the functor here as well?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@369
PS5, Line 369: TestUnknownTsk
Should a similar test be exist for authn token as well?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@477
PS5, Line 477: TestMasterElectionStorms
Will a similar also apply for authn tokens? Also,  wondering how you consider 
what should be tested in auth_token_expire-itest and what should be in 
authz_token-itest?


http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@523
PS5, Line 523: // Ensures the client can still communicate with servers that do 
not support
 : // authz tokens.
What happens if a old client try to communicate with servers that require authz 
tokens?



--
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: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 09 Jan 2019 06:31:27 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@352
PS5, Line 352: that  expired
Nit: got two spaces here.



--
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: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 07 Jan 2019 17:31:58 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#5).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/auth_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,335 insertions(+), 97 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/5
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 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(), _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 

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#4).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
D src/kudu/integration-tests/authn_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,244 insertions(+), 607 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/4
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Alexey Serbin (Code Review)]

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(_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 = _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 

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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:

(8 comments)

The test failure looks relevant.

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:   if (new_status.ok() && resp_.has_error()) {
So we don't need to check the controller status at all? Why not?


http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@82
PS3, Line 82:   if (resp_.has_authz_token()) {
Shouldn't this also be conditioned on new_status.ok()? I wouldn't expect the 
server to return an error and for there to be a token in the response, but 
defensive programming...


http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@141
PS3, Line 141: auto rpc_and_cbs = 
EraseKeyReturnValuePtr(_authz_rpcs_, table_id);
What does this return if for some reason there's no key of table_id?


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@496
PS3, Line 496: *req_.mutable_authz_token() = signed_token;
Can you std::move it?


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:   
*next_req_.mutable_new_scan_request()->mutable_authz_token() = authz_token;
std::move


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@153
PS3, Line 153:   static void StoreAuthzToken(KuduClient* client, const string& 
table_id, const SignedTokenPB& token) {
Line too long?


http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@347
PS3, Line 347:   SKIP_IF_SLOW_NOT_ALLOWED();
Clever, more terse than the multi-line if check.


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@1752
PS3, Line 1752:   for (bool supports_authz : { true, false }) {
Could you parameterize the test on this?



--
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 06 Jan 2019 05:04:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12122/2/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12122/2/src/kudu/integration-tests/authz_token-itest.cc@154
PS2, Line 154: client->data_->StoreAuthzToken(table_id, std::move(token));
> warning: passing result of std::move() as a const reference argument; no mo
Done


http://gerrit.cloudera.org:8080/#/c/12122/2/src/kudu/integration-tests/authz_token-itest.cc@288
PS2, Line 288:   ASSERT_FALSE(MessageDifferencer::Equals(bad_token, new_token));
> warning: 'bad_token' used after it was moved [bugprone-use-after-move]
Done



--
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: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 05 Jan 2019 01:01:54 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#3).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/authn_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,302 insertions(+), 80 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/3
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12122

to look at the new patch set (#2).

Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/authn_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/util/test_util.h
20 files changed, 1,302 insertions(+), 80 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/2
--
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: newpatchset
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 2:

(33 comments)

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@47
PS1, Line 47: // An asynchonous RPC that retrieves a new authz token for a 
table and puts it
> Doc the class too.
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@87
PS1, Line 87:  se
> Not an argument.
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@105
PS1, Line 105: const Status& status);
> Nit: empty line before.
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@106
PS1, Line 106:
> Sentence fragment?
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@54
PS1, Line 54: using master::MasterFeatures;
> warning: using decl 'SecureShortDebugString' is unused [misc-unused-using-d
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@65
PS1, Line 65:  
::GetTableSchemaAsync, "RetrieveAuthzToken",
> Why isn't the synchronous GetTableSchema method in client-internal.cc not b
This class is dependent on knowing the table ID up front. This is the case for 
all the places it's currently used, and isn't so for GetTableSchema. You're 
right that if we had the tablet ID, we could use this in GetTableSchema and 
forego the explicit Put() call in client-internal.cc


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@74
PS1, Line 74: void RetrieveAuthzTokenRpc::SendRpcCb(const Status& status) {
:   Status new_status = status;
> Shouldn't we only set one of these? Probably just the ID?
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@80
PS1, Line 80: w_status = StatusFromP
> Is this prefixing necessary?
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@85
PS1, Line 85:   }
> This seems risky from a forward compatibility perspective: what if there ar
Yeah, this is actually not used. I added handling for this case in 
scanner-internal.cc and retriable_rpc.h (for WriteRpc), and changed the outcome 
-- if we are retrieving an authz token in response to the tserver requiring a 
new authz token, as we are here, the master must support authz tokens. I.e. the 
client should just spit out an error.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@92
PS1, Line 92: }
> Do we need to do more error checking here? Or has that been done for us alr
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@93
PS1, Line 93:
> Could be moved into the scope below, or even combined with L95.
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@138
PS1, Line 138:   {
> Not EmplaceOrDie here? Didn't we just prove that there's no such entry in r
Done. It was having trouble deducing the type of the mapped Args for some 
reason, but this should work (calling the constructor explicitly).


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@151
PS1, Line 151: } // namespace client
> Would EraseKeyReturnValuePtr work here? It would be nice to find an alterna
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@384
PS1, Line 384:   switch (err->code()) {
> Nit: FWIW, I don't think this is necessary when the fallthrough is obvious
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@493
PS1, Line 493: void WriteRpc::FetchCachedAuthzToken() {
> See my comment in scanner-internal.cc.
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5799
PS1, Line 5799: data
> cache
Done


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5826
PS1, Line 5826: oken(table
> Parallely?
Self-conscious about this due to 
https://www.quora.com/Is-using-the-word-parallelly-grammatically-correct so 
changed the name.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5833
PS1, Line 5833: TEST_F(ClientTest, TestRetrieveAuthzTokenInParallel) {
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Ack

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Adar Dembo (Code Review)]

Adar Dembo 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 1:

(28 comments)

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@47
PS1, Line 47: class RetrieveAuthzTokenRpc : public 
AsyncLeaderMasterRpchttp://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@87
PS1, Line 87:  'table_id'
Not an argument.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@105
PS1, Line 105:   // Authorization tokens stored for each table, indexed by the 
table id. Note
Nit: empty line before.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@106
PS1, Line 106: users of the cache.
Sentence fragment?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@65
PS1, Line 65: RetrieveAuthzTokenRpc::RetrieveAuthzTokenRpc(const KuduTable* 
table,
Why isn't the synchronous GetTableSchema method in client-internal.cc not built 
around this async primitive?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@74
PS1, Line 74:   req_.mutable_table()->set_table_name(table_->name());
:   req_.mutable_table()->set_table_id(table_->id());
Shouldn't we only set one of these? Probably just the ID?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@80
PS1, Line 80: AsyncLeaderMasterRpc::
Is this prefixing necessary?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@85
PS1, Line 85:   rpc.error_response()->unsupported_feature_flags_size() > 0) 
{
This seems risky from a forward compatibility perspective: what if there are 
new feature flags that we do care about in the future? Is there a way to 
identify that it was just RETRIEVE_AUTHZ_TOKEN that was unsupported, and treat 
that as OK, while failing the rest?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@92
PS1, Line 92:
Do we need to do more error checking here? Or has that been done for us already?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@93
PS1, Line 93:   auto* token_cache = client_->data_->authz_token_cache_.get();
Could be moved into the scope below, or even combined with L95.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@138
PS1, Line 138: EmplaceOrUpdate(_authz_rpcs_, table_id, { rpc, { 
std::move(callback) } });
Not EmplaceOrDie here? Didn't we just prove that there's no such entry in 
retrieve_authz_rpcs_?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@151
PS1, Line 151: auto* rpc_and_cbs = FindOrNull(retrieve_authz_rpcs_, 
table_id);
Would EraseKeyReturnValuePtr work here? It would be nice to find an alternative 
such that there aren't two lookups (oen in FindOrNull and one in erase). Maybe 
raw iterators are the way to go.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@384
PS1, Line 384:   FALLTHROUGH_INTENDED;
Nit: FWIW, I don't think this is necessary when the fallthrough is obvious 
(i.e. a case that does nothing).


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@493
PS1, Line 493:   if 
(!batcher_->client_->data_->FetchCachedAuthzToken(table()->id(), 
req_.mutable_authz_token())) {
See my comment in scanner-internal.cc.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5799
PS1, Line 5799: cahce
cache


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5826
PS1, Line 5826: Parallelly
Parallely?


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5852
PS1, Line 5852:   ASSERT_LT(num_reqs, kThreads);
Does this hold in TSAN environments with stress threads? In a pathological case 
it could be possible for each of the 20 threads to run serially and thus for 
there to be no grouping at all. Unlikely, but possible.


http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@147
PS1, Line 147: case ScanRpcStatus::RPC_INVALID_AUTHORIZATION_TOKEN:
Nit: add a useful comment here, or drop the one from L143 if it's redundant.

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

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 1: Verified+1

The failure seems unrelated:
  BlockManagerType/TsRecoveryITest.TestNoBlockIDReuseIfMissingBlocks/1


--
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: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Dec 2018 10:42:19 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens

[Andrew Wong (Code Review)]

Andrew Wong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12122


Change subject: KUDU-2543 pt 2: pass around default authz tokens
..

KUDU-2543 pt 2: pass around default authz tokens

Adds authz token generation to the master's GetTableSchema endpoint,
with which clients can authorize themselves for specific tables. A
client will cache these tokens and use them appropriately for RPCs that
need them (e.g. Writes and Scans), reacquiring them when receiving word
that they are expired.

This is tested in the following ways:
- unit tests for the new client-side cache for authz tokens
- parameterized the token expiration test for authn and authz tokens to
  have varying token expirations, testing when authn tokens expire but
  not authz tokens, and vice versa
- added various tests to ensure the client behaves correctly in various
  non-happy cases

Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
---
M src/kudu/client/CMakeLists.txt
A src/kudu/client/authz_token_cache.cc
A src/kudu/client/authz_token_cache.h
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/authn_token_expire-itest.cc
A src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
18 files changed, 1,235 insertions(+), 70 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/1
--
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: newchange
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong