You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/12/23 09:50:40 UTC
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
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 <aw...@cloudera.com>
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Dec 2018 10:42:19 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:28:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Jan 2019 00:56:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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(&retrieve_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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 06 Jan 2019 05:04:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens
......................................................................
Patch Set 3:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@53
PS3, Line 53: MonoTime deadline
nit: In RetrieveNewAuthzToken() that's passed by const reference. Is it possible to unify those two notations?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@54
PS3, Line 54: virtual
nit: drop 'virtual'
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@58
PS3, Line 58: virtual
nit: drop
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@72
PS3, Line 72: Manages distribution and reacquisition of authz tokens.
This reads vague. Could you be more specific?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@84
PS3, Line 84: No checking is done to verify the
: // expiration or validity of the returned token.
It would be nice to document the reasoning behind this design decision.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@93
PS3, Line 93: :
nit: ::
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@254
PS3, Line 254: it updates
nit: updates what?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@498
PS3, Line 498: // Note: this is the expected path if communicating with an older-versioned
: // master that does not support authz tokens.
Can it be the case when the expected authz token hasn't been fetched into the cache yet? What will happen in that case? Will the operation fetch a new authz token and retry?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h@249
PS3, Line 249: std::unique_ptr
Why to allocate it on the heap? Is it possible to have an instance of AuthzTokenCache instead?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360
PS3, Line 360: = authz_token
What if the token in the cache has expired?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@362
PS3, Line 362: this is expected
I'm not sure I understand. If FetchCachedAuthzToken() returns false, then it means the cache hasn't fetched a token yet, but de-facto the master can support generating authz tokens, right? In the latter case, what will happen with the scan request -- will it be retried with authnz token when it's available?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc@103
PS3, Line 103: AuthnTokenExpireITestBase
If this test is re-purposed to verify the authz token-related functionality as well, maybe rename it to AuthTokenExpireITestBase or alike?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@142
PS3, Line 142:
nit: add two more spaces
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@196
PS3, Line 196: AuthenticationCredentialsPB authn_pb;
: authn_pb.set_real_user(user);
: CHECK(authn_pb.SerializeToString(&authn_creds));
This is just for plain authn, right?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@238
PS3, Line 238: table_id
nit: table_id_ ?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@295
PS3, Line 295: for (int i = 0; i < bad_signature.length(); i++) {
: char* first_byte = &bad_signature[i];
: *first_byte ^= *first_byte;
: }
Is this just zeroing the signature? If so, maybe memset() is easier to write and follow?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@349
PS3, Line 349: Ensured
Ensure?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@367
PS3, Line 367: it'll appear invalid
: // to the servers.
nit: "it's far ahead of latest TSK's sequence number"
Or replace 'invalid' with 'unknown'.
'invalid' seems vague.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@432
PS3, Line 432: LOG(INFO) << s.ToString();
Is this needed?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@470
PS3, Line 470: private:
remove
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1739
PS3, Line 1739: Add some basic verifications that we get tokens when we expect.
Some basic verifications that we get authz tokens when we expect
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1742
PS3, Line 1742: char *kTableName
style nit: stick the asterisk to the type, not the name of the variable.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1757
PS3, Line 1757: resp.has_error()
Do you want to be certain on the type of the error in the response?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1760
PS3, Line 1760: CreateTable
BTW, are we going to check authorization for the CreateTable operation and other DDL operations?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto@792
PS3, Line 792: RETRIEVE
nit: if that's about generating authz token, why is it RETRIVE, not GENERATE?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@80
PS3, Line 80: master_support_authz_tokens
Why not 'master_generate_authz_tokens' or 'master_enable_authz_tokens'? It seems that 'support' is kind of intrinsic property, but not a configurable one.
If you need to have it denoting something irregular for testing, why not to add 'master_disable_authz_tokens' instead and mention that it is for tests only?
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@424
PS3, Line 424: if (PREDICT_TRUE(FLAGS_master_support_authz_tokens)) {
Do we want to restrict this only to the leader master role?
--
To view, visit http://gerrit.cloudera.org:8080/12122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 07 Jan 2019 06:06:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 AsyncLeaderMasterRpc<master::GetTableSchemaRequestPB,
Doc the class too.
http://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(&retrieve_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.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@352
PS1, Line 352: // Only new scan requests require authz tokens, with the expectation that
: // further access to new scanner IDs will only be allowed for this user.
I'm not really understanding the second half of this sentence.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@356
PS1, Line 356: // Note: this is expected if attempting to connect to a cluster that does
: // not support fine-grained access control.
Do we still want to have called mutable_authz_token() though? It means we'll send an empty token instead of not sending one at all.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt@61
PS1, Line 61: ADD_KUDU_TEST(authz_token-itest)
PROCESSORS? SHARDS?
http://gerrit.cloudera.org:8080/#/c/12122/1/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/1/src/kudu/integration-tests/authn_token_expire-itest.cc@276
PS1, Line 276: ,
Missing some value here?
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:
PS1:
We should restrict tests that explicitly sleep for a second or more to KUUD_ALLOW_SLOW_TESTS.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@167
PS1, Line 167: .timeout(MonoDelta::FromSeconds(60))
The default timeout value isn't appropriate?
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@263
PS1, Line 263: for (const auto& client_func : { insert_to_table, scan_from_table }) {
Can you parameterize the test on these two?
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto@788
PS1, Line 788: // The master supports the 'RetrieveAuthzToken' RPC.
I don't see RetrieveAuthzToken listed in MasterService.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc@404
PS1, Line 404: void MasterServiceImpl::GetTableSchema(const GetTableSchemaRequestPB* req,
Would be nice to "unit" test this bit of functionality in isolation, perhaps in master-test.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@123
PS1, Line 123: const std::string&
Can you use const char* here 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: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Jan 2019 21:27:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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(&new_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<simple_spinlock> 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, &cached_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 around? For some reason, I could find this test in this change
Indeed, I think these were roughly TestInvalidAuthzTokens.
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.
Done
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
Done
http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@248
PS10, Line 248: with
> drop
Rewrote this snippet.
http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@301
PS10, Line 301: char* byte = &bad_signature[i];
: *byte = ~*byte;
> nit: I think it's possible to use less indirection-related operations here:
Done
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 ?
Done
http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@540
PS10, Line 540: shared_ptr<KuduSession> session(client_->NewSession());
> Is this needed?
Ah no, I refactored this into uselessness.
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
Done
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
> Right, so non-leaders will send back GetTableSchemaResponsePB with no authz
Ah, you're right. If the response has an error, we shouldn't expect an authz 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
Gerrit-MessageType: comment
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 00:45:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 07 Jan 2019 17:31:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 09 Jan 2019 06:31:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2543 pt 2: pass around default authz tokens
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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<master::GetTableSchemaRequestPB,
: master::GetTableSchemaResponsePB>,
> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 03 Feb 2019 06:20:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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, &cached_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<KuduSession> 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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 11 Feb 2019 08:06:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<master::GetTableSchemaRequestPB,
: master::GetTableSchemaResponsePB>,
> 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(&new_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<simple_spinlock> 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<StatusCallback>, 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 = &bad_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 GetTableSchema requests, see MasterSer
Right, so non-leaders will send back GetTableSchemaResponsePB with no authz token, right?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 11 Feb 2019 02:35:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 05:25:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens
......................................................................
Patch Set 4:
(35 comments)
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h
File src/kudu/client/authz_token_cache.h:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@53
PS3, Line 53: MonoTime deadline
> nit: In RetrieveNewAuthzToken() that's passed by const reference. Is it po
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@54
PS3, Line 54: std::str
> nit: drop 'virtual'
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@58
PS3, Line 58: void Sen
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@72
PS3, Line 72: Cache for authz tokens received from the master. A clie
> This reads vague. Could you be more specific?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@84
PS3, Line 84: for 'table_id', replacing any that
: // previously existed.
> It would be nice to document the reasoning behind this design decision.
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.h@93
PS3, Line 93: t
> nit: ::
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc
File src/kudu/client/authz_token_cache.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@79
PS3, Line 79: // Unwrap and return any other application errors that may be returned by the
> So we don't need to check the controller status at all? Why not?
It's checked in RetryOrReconnectIfNecessary()
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@82
PS3, Line 82: new_status = StatusFromPB(resp_.error().status());
> Shouldn't this also be conditioned on new_status.ok()? I wouldn't expect th
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@141
PS3, Line 141: std::lock_guard<simple_spinlock> l(rpc_lock_);
> What does this return if for some reason there's no key of table_id?
It calls the default constructor of the mapped type (pair in this case), so we'd hit the DCHECK below.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@254
PS3, Line 254: es 'req_'
> nit: updates what?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@496
PS3, Line 496: if (batcher_->client_->data_->FetchCachedAuthzToken(table()->id(), &signed_token)) {
> Can you std::move it?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@498
PS3, Line 498: } else {
: // Note: this is the expected path if communi
> Can it be the case when the expected authz token hasn't been fetched into t
If that were the case, and the tserver is enforcing access control, then yes. This generally won't happen though since, if the master supports generating authz tokens, there should be an authz token in the cache from the call to OpenTable. I've fleshed out a comment in authz_token_cache.h to clarify the workflow a bit.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/client-internal.h@249
PS3, Line 249: // upon learnin
> Why to allocate it on the heap? Is it possible to have an instance of Auth
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360
PS3, Line 360: = std::move(a
> What if the token in the cache has expired?
It will get an error from the tserver and retry.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360
PS3, Line 360: *next_req_.mutable_new_scan_request()->mutable_authz_token() = std::move(authz_token);
> std::move
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@362
PS3, Line 362: this is expected
> I'm not sure I understand. If FetchCachedAuthzToken() returns false, then
If there's nothing in the cache, it means that OpenTable() didn't get a token from the master. The assumption then is that the tserver doesn't need a token. If this is, for some reason, not the case, and the tserver _does_ want a token, the client will try to RetrieveNewAuthzToken(), but get an error from the master and hit L197 in this file. TestAuthzTokensNotSupported tests this case.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authn_token_expire-itest.cc@103
PS3, Line 103:
> If this test is re-purposed to verify the authz token-related functionality
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@142
PS3, Line 142:
> nit: add two more spaces
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@153
PS3, Line 153: static void StoreAuthzToken(KuduClient* client,
> Line too long?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@196
PS3, Line 196: client_builder.default_rpc_timeout(MonoDelta::FromSeconds(kRpcTimeoutSecs));
: string authn_creds;
: AuthenticationCredentialsPB authn_pb;
> This is just for plain authn, right?
Yes
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@238
PS3, Line 238: e create
> nit: table_id_ ?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@295
PS3, Line 295: LOG(INFO) << "Trying to use a bad signature...";
: string bad_signature = std::move(*new_token.mutable_signature());
: // Flip the bits of the signature.
: f
> Is this just zeroing the signature? If so, maybe memset() is easier to wri
Mentioned in the other patch, meant to ~ instead of ^ to flip the bits of the signature.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@347
PS3, Line 347:
> Clever, more terse than the multi-line if check.
Ack
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@349
PS3, Line 349: We slee
> Ensure?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@367
PS3, Line 367: erver should respond
: // with an ERROR_UNA
> nit: "it's far ahead of latest TSK's sequence number"
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@432
PS3, Line 432: // After a while, the clie
> Is this needed?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@470
PS3, Line 470: ASSER
> remove
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1739
PS3, Line 1739: public ::testing::WithParamInterface<
> Some basic verifications that we get authz tokens when we expect
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1742
PS3, Line 1742: uthzTokenMasterT
> style nit: stick the asterisk to the type, not the name of the variable.
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1752
PS3, Line 1752: };
> Could you parameterize the test on this?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1757
PS3, Line 1757: nd_req(&resp));
> Do you want to be certain on the type of the error in the response?
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1760
PS3, Line 1760: RUE(s.IsNot
> BTW, are we going to check authorization for the CreateTable operation and
Yes, but not with tokens. Kudu will get authorization metadata directly from an external source (e.g. Sentry, Ranger) at DDL-time. Hao has some in-flight patches for that.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master.proto@792
PS3, Line 792: GENERATE
> nit: if that's about generating authz token, why is it RETRIVE, not GENERAT
Done
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@80
PS3, Line 80: master_support_authz_tokens
> Why not 'master_generate_authz_tokens' or 'master_enable_authz_tokens'? It
I named this to be consistent with FLAGS_master_support_connect_to_master_rpc.
Mentioned it in the description.
http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master_service.cc@424
PS3, Line 424: // for the table.
> Do we want to restrict this only to the leader master role?
Yes, this is the case, given the check at L409.
--
To view, visit http://gerrit.cloudera.org:8080/12122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 07 Jan 2019 08:08:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 19:56:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 01:38:35 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 22:40:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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
Gerrit-MessageType: comment
Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565
Gerrit-Change-Number: 12122
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 01:29:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 05 Jan 2019 01:01:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 07:32:52 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<master::GetTableSchemaRequestPB,
: master::GetTableSchemaResponsePB>,
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Jan 2019 20:40:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Jan 2019 00:58:06 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 02:19:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Jan 2019 20:01:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 13 Feb 2019 19:14:29 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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: &MasterServiceProxy::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
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5852
PS1, Line 5852: // The authz token retrieval requests shouldn't send one request per table;
> Does this hold in TSAN environments with stress threads? In a pathological
Ran via dist-test with 28 stress threads with 0/1000 failures.
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: reacquire_authn_token = true;
> Nit: add a useful comment here, or drop the one from L143 if it's redundant
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@352
PS1, Line 352: if (configuration().row_format_flags() & KuduScanner::PAD_UNIXTIME_MICROS_TO_16_BYTES) {
: controller_.RequireServerFeature(TabletServerFeatures::PAD_UNIXTIME_MICR
> I'm not really understanding the second half of this sentence.
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@356
PS1, Line 356: // Only new scan requests require authz tokens. Scan continuations rely on
: // Kudu's prevention of scanner hijacking by
> Do we still want to have called mutable_authz_token() though? It means we'l
Nice catch.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt@61
PS1, Line 61: ADD_KUDU_TEST(authz_token-itest PROCESSORS 2)
> PROCESSORS? SHARDS?
We chatted a bit about this offline and I'm basing this off of that discussion. The test doesn't take anywhere near 900 seconds to run, so NUM_SHARDS is fine with the default. I'm going to base the PROCESSORS off of the value used by client-test, since they both use an IMC and don't do anything crazy with it.
http://gerrit.cloudera.org:8080/#/c/12122/1/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/1/src/kudu/integration-tests/authn_token_expire-itest.cc@276
PS1, Line 276: V
> Missing some value here?
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc
File src/kudu/integration-tests/authz_token-itest.cc:
PS1:
> We should restrict tests that explicitly sleep for a second or more to KUUD
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@94
PS1, Line 94: using security::PrivateKey;
> warning: using decl 'SecureShortDebugString' is unused [misc-unused-using-d
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@167
PS1, Line 167:
> The default timeout value isn't appropriate?
Done, pasted from client-test since these tests were there originally.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@263
PS1, Line 263: // First, let's do a sanity check that initial authz tokens allow the user to
> Can you parameterize the test on these two?
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto@788
PS1, Line 788: // EVICT_FIRST (a.k.a. 3-2-3) and the PREPARE_REPLACEMENT_BEFORE_EVICTION
> I don't see RetrieveAuthzToken listed in MasterService.
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc@404
PS1, Line 404:
> Would be nice to "unit" test this bit of functionality in isolation, perhap
Added a very basic test.
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@123
PS1, Line 123:
> Can you use const char* here instead?
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@240
PS1, Line 240: return GetNewAuthnTokenAndRetry();
> warning: redundant boolean literal in conditional return statement [readabi
Done
http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@250
PS1, Line 250: if (server != nullptr && result.status.IsTimedOut()) {
> warning: redundant boolean literal in conditional return statement [readabi
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 05 Jan 2019 00:40:26 +0000
Gerrit-HasComments: Yes