You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/04/08 00:26:46 UTC
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15681
to review the following change.
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
5 files changed, 543 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/1
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#12) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 828 insertions(+), 554 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/12
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@155
PS1, Line 155:
> nit: Is this needed? Doesn't only the SentryAuthzITestHarness need the HMS
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@300
PS1, Line 300: make_optional<const string&>(kAdminUser), cluster, client);
: CheckTable(kDatabaseName, kSecondTable,
: make_optional<const string&>(kAdminUser), cluster, client);
: return Status::OK();
> nit: maybe rename these to SetUpExternalMiniServiceOpts() and SetUpExternal
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@920
PS1, Line 920: // in Kudu and the
> If you replace this with ASSERT_STR_MATCHES, you can reuse it for ranger to
Hao said she already used the upper-case version in systest, so that's why I decided against making them consistent. Thanks for the tip.
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1031
PS1, Line 1031: {
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1151
PS1, Line 1151:
> We may want to consider using testing::Combine on some kSentry/kRanger enum
How do you feel about adding a TODO to refactor it once we support C++14? With C++14 we could templatize this.
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@645
PS3, Line 645:
> I'm a bit surprised that this compiles. Maybe I'm missing something, but th
Done
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@689
PS3, Line 689: Status StopAuthzProvider() {
: return harness_.StopAuthzProvider(cluster_);
: }
> Can we not refactor a bit to have the first argument be a MasterAuthzITestH
I think this could be done, but that would require us to rewrite the methods in the harness to expect a pointer to the cluster as an argument instead of the client and create their own client which feels weird. How about rewriting this part as well when we can templatize variables? (C++14)
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@54
PS3, Line 54: // Policy key used for searching policies_
> nit: could you mention what the fields are?
Done
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@59
PS3, Line 59: // The AuthorizationPolicy contains a set of privileges on a resource to one or
: // more users. 'items' is a vector of user-list of actions pair. This struct can
: // be used to create new Ranger policies in tests.
> nit: should mention the policy name will be based on its contents.
Done
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@191
PS3, Line 191: cies since s
> nit: "resources"?
Done
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@192
PS3, Line 192: used fo
> nit: Ranger
Done
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 20:05:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#10) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger. A later patch will
do the same for ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
8 files changed, 738 insertions(+), 461 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/10
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#16).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
This commit refactors the existing Sentry integration tests to prepare
for Ranger integration tests.
It changes HMS and Sentry integration test base classes to test
harnesses that don't inherit from Gtest to simplify inheritance when
using typed tests.
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
A src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
D src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/ts_sentry-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
10 files changed, 1,882 insertions(+), 1,303 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/16
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 13: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@515
PS13, Line 515: policy.tables.emplace_back("*");
Mentioned in previous comments, I think this can be removed.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@554
PS13, Line 554: policy_new_table.tables.emplace_back("*");
This can be removed as well.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc@573
PS13, Line 573: if (allowed_actions.empty()) {
: LOG(WARNING) << Substitute("User $0 is not authorized to perform actions $1 on table $2",
: user_name, JoinMapped(*actions, ActionPB_Name, ", "), table_name);
: return Status::NotAuthorized(kUnauthorizedAction);
: }
Should we remove this and let ranger_authz_provider handle it as well?
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 05:52:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 17:28:48 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#4).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
5 files changed, 612 insertions(+), 217 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/4
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@443
PS7, Line 443: const uni
nit: indent.
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@477
PS7, Line 477: policy.tables.emplace_back("*");
This can be removed as only Database level privilege is required.
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@491
PS7, Line 491: SleepFor(MonoDelta::FromMilliseconds(1500))
If the sleep is for waiting the polling interval to finish in Ranger client, we can even make it smaller instead of sleep here?
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@510
PS7, Line 510: policy_new_table.tables.emplace_back("*")
This can be removed.
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@540
PS7, Line 540: ParseHiveTableIdentifier
We should use ParseRangerTableIdentifier?
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/master/ranger_authz_provider.cc@199
PS7, Line 199: CHECK_OK(client_.AuthorizeActionMultipleColumns(user, ActionPB::SELECT, table_name,
: &column_names));
But the subprocess execution can also return a non OK() status?
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 22:30:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#14).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
This commit refactors the existing Sentry integration tests to prepare
for Ranger integration tests.
It changes HMS and Sentry integration test base classes to test
harnesses that don't inherit from Gtest to simplify inheritance when
using typed tests.
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
A src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
D src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/ts_sentry-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
11 files changed, 1,862 insertions(+), 1,297 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/14
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#2).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
5 files changed, 529 insertions(+), 160 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/2
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#7).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource). This broke an existing test in
mini_ranger-test which was testing persistence by expecting an error
after adding an existing policy after restarting Ranger, this test is
fetching the number of policies now instead.
These new integration tests uncovered a previously unknown bug in Ranger
authorization provider and Ranger client which is also fixed now.
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 635 insertions(+), 243 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/7
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#17).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
This commit refactors the existing Sentry integration tests to prepare
for Ranger integration tests.
It changes HMS and Sentry integration test base classes to test
harnesses that don't inherit from Gtest to simplify inheritance when
using typed tests.
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
A src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
D src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/ts_sentry-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
10 files changed, 1,881 insertions(+), 1,303 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/17
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#3).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
5 files changed, 525 insertions(+), 160 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/3
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 15:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@326
PS13, Line 326: // Always enable Kerberos, as Sentry/Ranger deployments do not make sense
> ah if it's the default, we can remove it
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@515
PS13, Line 515: policy.items.emplace_back(Policy
> Checking a bit, it seems it is IsCreateTableDone() requiring METADATA on t
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@554
PS13, Line 554: policy_new_table.tables.emplace_back("*");
> Same here.
Done
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc
File src/kudu/integration-tests/ts_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc@356
PS14, Line 356: public:
> can you make this look more like the master test with an enum? or separate
changed it to TSSentryITest without the template if that's what you meant by separating it out. Can't really remove it from here due to the rest of the refactor.
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/master/ranger_authz_provider.cc@194
PS14, Line 194: atus RangerAuthzProvider::FillTablePrivilegePB(const string& table_name,
: const string& user,
: const SchemaPB& schema_pb,
: TablePrivilegePB* pb) {
: DCHECK(pb);
: DCHECK(pb->has_table_id());
: bool authorized;
: if (IsTrustedUser(user) ||
> This should belong to https://gerrit.cloudera.org/c/15696/?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 21:00:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#8).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource). This broke an existing test in
mini_ranger-test which was testing persistence by expecting an error
after adding an existing policy after restarting Ranger, this test is
fetching the number of policies now instead.
These new integration tests uncovered a previously unknown bug in Ranger
authorization provider and Ranger client which is also fixed now.
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 637 insertions(+), 244 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/8
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#5).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
These new integration tests uncovered a previously unknown bug in Ranger
authorization provider and Ranger client which is also fixed now.
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 624 insertions(+), 238 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/5
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@155
PS1, Line 155:
nit: Is this needed? Doesn't only the SentryAuthzITestHarness need the HMS harness? If so, you could move the HMS client setup into SentryAuthzITestHarness::SetUpProvider
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@300
PS1, Line 300: virtual Status SetUpCredentials() = 0;
: virtual Status SetUpProvider(bool enable_kerberos,
: const unique_ptr<ExternalMiniCluster>& cluster) = 0;
: };
nit: maybe rename these to SetUpExternalMiniServiceOpts() and SetUpExternalServiceClients()?
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@920
PS1, Line 920:
If you replace this with ASSERT_STR_MATCHES, you can reuse it for ranger too. Alternatively, make the error messages consistent.
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1031
PS1, Line 1031: const auto table_name = Substitute("$0.$1", desc.database, desc.table_na
Remove
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1151
PS1, Line 1151:
We may want to consider using testing::Combine on some kSentry/kRanger enum, and have the constructor of these tests instantiate the correct test harness, based on the enum. If that works, that way, we can reuse these without duplicating everything.
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@645
PS3, Line 645: StartSentry
I'm a bit surprised that this compiles. Maybe I'm missing something, but these aren't defined for the Ranger harness are they? How about combine these into a more agnostic Start/StopAuthorizationService or somesuch.
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/integration-tests/master_authz-itest.cc@689
PS3, Line 689:
: // We unfortunately need to duplicate a bunch of code here as we can't
: // templatize variables and typedefs.
Can we not refactor a bit to have the first argument be a MasterAuthzITestHarness*?
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@54
PS3, Line 54: // Policy key used for searching policies_
nit: could you mention what the fields are?
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@59
PS3, Line 59: // The AuthorizationPolicy contains a set of privileges on a resource to one or
: // more users. 'items' is a vector of user-list of actions pair. This struct can
: // be used to create new Ranger policies in tests.
nit: should mention the policy name will be based on its contents.
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@191
PS3, Line 191: policy items
nit: "resources"?
http://gerrit.cloudera.org:8080/#/c/15681/3/src/kudu/ranger/mini_ranger.h@192
PS3, Line 192: Rangeer
nit: Ranger
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 01:42:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@326
PS13, Line 326: opts.num_masters = 1;
> why is the default overridden here?
ah if it's the default, we can remove it
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc
File src/kudu/integration-tests/ts_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc@356
PS14, Line 356: class TSAuthzITest : public ExternalMiniClusterITestBase {
can you make this look more like the master test with an enum? or separate this out into the next patch where you parameterize for ranger?
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:35:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 13:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@326
PS13, Line 326: opts.num_masters = 1;
why is the default overridden here?
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@350
PS13, Line 350: and any dependencies they may
: // have (e.g. HMS).
> nit: oops this is no longer true.
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@385
PS13, Line 385: // Then start Sentry and its client.
> nit: drop this comment
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@515
PS13, Line 515: policy.tables.emplace_back("*");
> Mentioned in previous comments, I think this can be removed.
Unfortunately it can't be removed, METADATA on the table level is necessary to create a table.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@518
PS13, Line 518: SleepFor(MonoDelta::FromMilliseconds(1500));
> I think there was a comment earlier asking whether we could set this to som
2x polling interval was way too low, that was the first I tried with. It seems even with a low polling interval it takes some time for the client to update the cached policies. Lowered it to 1.2s, 1s was still flaky.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@554
PS13, Line 554: policy_new_table.tables.emplace_back("*");
> This can be removed as well.
Unfortunately it can't be removed, METADATA on the table level is necessary to create a table.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@591
PS13, Line 591: &ranger_db, &ranger_table));
> nit: spacing
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@676
PS13, Line 676: optional<const string&> table_id) {
> warning: the parameter 'table_id' is copied for each invocation but only us
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@748
PS13, Line 748: boost::optional<const std::string&> user,
> warning: the parameter 'user' is copied for each invocation but only used a
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@1061
PS13, Line 1061:
: // Test that when the client passes a table identifier with the table name
: // and table ID refer to different tables, the client needs permission on
: // both tables for returning TABLE_NOT_FOUND error to avoid leaking table
: // existence.
: TEST_P(MasterAuthzITest, TestMismatchedTable) {
: const auto table_name_a = Substitute("$0.$1", kDatabaseName, kTableName);
: const auto table_name_b = Substitute("$0.$1", kDatabaseName, kSecondTable);
:
: // Log in as 'test-admin' to get the tablet ID.
: ASSERT_OK(this->cluster_->kdc()->Kinit(kAdminUser));
: shared_ptr<KuduClient> client;
: ASSERT_OK(this->cluster_->CreateClient(nullptr, &client));
: shared_ptr<KuduTable> table;
: ASSERT_OK(client->OpenTable(table_name_a, &table));
: optional<const string&> table_id_a = make_optional<const string&>(table->id());
:
: // Log back as 'test-user'.
: ASSERT_OK(this->cluster_->kdc()->Kinit(kTestUser));
:
: Status s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotAuthorized());
: ASSERT_STR_MATCHES(s.ToString(), "[Uu]nauthorized action");
:
: ASSERT_OK(this->GrantGetMetadataTablePrivilege({ kDatabaseName, kTableName }));
: s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotAuthorized());
: ASSERT_STR_MATCHES(s.ToString(), "[Uu]nauthorized action");
:
: ASSERT_OK(this->GrantGetMetadataTablePrivilege({ kDatabaseName, kSecondTable }));
: s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotFound());
: ASSERT_STR_CONTAINS(s.ToString(), "the table ID refers to a different table");
: }
> nit: oops, this should go up by the other MasterAuthzITests
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@1175
PS13, Line 1175: FALSE(s.ok());
> I think we can revert this to IsNetworkError() since this is only run for S
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/mini_ranger-test.cc
File src/kudu/ranger/mini_ranger-test.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/mini_ranger-test.cc@83
PS13, Line 83: curl.FetchURL(JoinPathSegments(ranger_.admin_url(), "service/plugins/policies/count"),
> Check for errors.
Done
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc@573
PS13, Line 573: if (allowed_actions.empty()) {
: LOG(WARNING) << Substitute("User $0 is not authorized to perform actions $1 on table $2",
: user_name, JoinMapped(*actions, ActionPB_Name, ", "), table_name);
: return Status::NotAuthorized(kUnauthorizedAction);
: }
> +1
Done
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 16:11:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#9) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
DONT_BUILD
still haven't been able to run the tests successfully
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
5 files changed, 668 insertions(+), 434 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/9
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#13) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.h
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
11 files changed, 832 insertions(+), 558 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/13
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#15).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
This commit refactors the existing Sentry integration tests to prepare
for Ranger integration tests.
It changes HMS and Sentry integration test base classes to test
harnesses that don't inherit from Gtest to simplify inheritance when
using typed tests.
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
A src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
D src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/ts_sentry-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
10 files changed, 1,852 insertions(+), 1,290 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/15
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 13: Verified+1
Unrelated flake:
BlockManagerType/TsRecoveryITest.TestNoBlockIDReuseIfMissingBlocks
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 05:07:49 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15681
to look at the new patch set (#6).
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
These new integration tests uncovered a previously unknown bug in Ranger
authorization provider and Ranger client which is also fixed now.
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 623 insertions(+), 238 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/6
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@515
PS13, Line 515: policy.tables.emplace_back("*");
> We are requesting METADATA on db level in authz provider, https://github.co
Checking a bit, it seems it is IsCreateTableDone() requiring METADATA on table level privieliege. Would you mind adding a comment here to make it clear if it is the case? Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:41:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 13:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@1151
PS1, Line 1151: zITestBase::SetUp());
> How do you feel about adding a TODO to refactor it once we support C++14? W
The latest patchset seems to be working with the approach described here.
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/7/src/kudu/integration-tests/master_authz-itest.cc@540
PS7, Line 540:
> We should use ParseRangerTableIdentifier?
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@245
PS9, Line 245:
> warning: method 'CreateKuduTable' can be made static [readability-convert-m
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@285
PS9, Line 285: KuduSchema schema;
> warning: method 'CheckTable' can be made static [readability-convert-member
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@287
PS9, Line 287: unique_ptr<KuduTableCreator> table_creator(client->NewTableCreator());
> warning: parameter 'user' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@288
PS9, Line 288: if (timeout.Initialized()) {
> warning: parameter 'cluster' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@568
PS9, Line 568: AuthorizationPolicy policy;
> warning: parameter 'cluster' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/15681/9/src/kudu/integration-tests/master_authz-itest.cc@572
PS9, Line 572: SleepFor(MonoDelta::FromMilliseconds(1500));
> warning: parameter 'cluster' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 04:58:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15681/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15681/8//COMMIT_MSG@9
PS8, Line 9: master_sentry-itest is renamed to master_authz-itest to generalize it
There will be a follow up patch to add ranger tests for ts integration test (such as ts_ranger-itest.cc)?
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 22:35:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
This commit refactors the existing Sentry integration tests to prepare
for Ranger integration tests.
It changes HMS and Sentry integration test base classes to test
harnesses that don't inherit from Gtest to simplify inheritance when
using typed tests.
master_sentry-itest is renamed to master_authz-itest to generalize it,
and most tests are changed to also run with Ranger. Those that aren't
test behavior specific to Sentry. A later patch will do the same for
ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Reviewed-on: http://gerrit.cloudera.org:8080/15681
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
A src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
D src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/ts_sentry-itest.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
10 files changed, 1,881 insertions(+), 1,303 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@515
PS13, Line 515: policy.tables.emplace_back("*");
> Unfortunately it can't be removed, METADATA on the table level is necessary
We are requesting METADATA on db level in authz provider, https://github.com/apache/kudu/blob/master/src/kudu/master/ranger_authz_provider.cc#L65. If that is the case, then it is a bug?
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@554
PS13, Line 554: policy_new_table.tables.emplace_back("*");
> Unfortunately it can't be removed, METADATA on the table level is necessary
Same here.
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/master/ranger_authz_provider.cc@194
PS14, Line 194: // AuthorizeActionMultipleColumns shouldn't return NotAuthorized at this point
: // as it only returns NotAuthorized if the table name is invalid, but the
: // previous AuthorizeActions() call would've also returned a NotAuthorized in
: // that case.
: //
: // TODO(abukor): revisit if it's worth merge this into the previous request
: RETURN_NOT_OK(client_.AuthorizeActionMultipleColumns(user, ActionPB::SELECT, table_name,
: &column_names));
This should belong to https://gerrit.cloudera.org/c/15696/?
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:16:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@208
PS1, Line 208: const unique_ptr<ExternalMiniCluster>& cluster) {
> warning: method 'GetTableLocations' can be made static [readability-convert
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@297
PS1, Line 297: virtual void TearDown() = 0;
> warning: parameter 'client' is const-qualified in the function declaration;
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/integration-tests/master_authz-itest.cc@497
PS1, Line 497: const unique_ptr<ExternalMiniCluster>& cluster) override {
> warning: parameter 'enable_kerberos' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15681/1/src/kudu/ranger/mini_ranger.cc@46
PS1, Line 46: using std::vector;
> warning: using decl 'unique_ptr' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 01:05:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#11) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
KUDU-3078 Add Ranger tests to master_authz-itest
master_sentry-itest is renamed to master_authz-itest to generalize it
and the tests were changed to also run with Ranger. A later patch will
do the same for ts_sentry-itest.
Ranger doesn't support adding new policy items (users and privileges) to
existing policies, so MiniRanger::AddPolicy stores policies in a member
variable and recreates them completely when a new item is added to an
existing policy (resource).
Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
---
M src/kudu/integration-tests/CMakeLists.txt
R src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 736 insertions(+), 466 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/15681/11
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 13:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@350
PS13, Line 350: and any dependencies they may
: // have (e.g. HMS).
nit: oops this is no longer true.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@385
PS13, Line 385: // Then start Sentry and its client.
nit: drop this comment
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@518
PS13, Line 518: SleepFor(MonoDelta::FromMilliseconds(1500));
I think there was a comment earlier asking whether we could set this to something lower. Can we? Maybe 2x the Ranger polling interval or something? Regardless, we should probably make this a constant for this harness or test.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@591
PS13, Line 591: &ranger_db, &ranger_table));
nit: spacing
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@1061
PS13, Line 1061:
: // Test that when the client passes a table identifier with the table name
: // and table ID refer to different tables, the client needs permission on
: // both tables for returning TABLE_NOT_FOUND error to avoid leaking table
: // existence.
: TEST_P(MasterAuthzITest, TestMismatchedTable) {
: const auto table_name_a = Substitute("$0.$1", kDatabaseName, kTableName);
: const auto table_name_b = Substitute("$0.$1", kDatabaseName, kSecondTable);
:
: // Log in as 'test-admin' to get the tablet ID.
: ASSERT_OK(this->cluster_->kdc()->Kinit(kAdminUser));
: shared_ptr<KuduClient> client;
: ASSERT_OK(this->cluster_->CreateClient(nullptr, &client));
: shared_ptr<KuduTable> table;
: ASSERT_OK(client->OpenTable(table_name_a, &table));
: optional<const string&> table_id_a = make_optional<const string&>(table->id());
:
: // Log back as 'test-user'.
: ASSERT_OK(this->cluster_->kdc()->Kinit(kTestUser));
:
: Status s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotAuthorized());
: ASSERT_STR_MATCHES(s.ToString(), "[Uu]nauthorized action");
:
: ASSERT_OK(this->GrantGetMetadataTablePrivilege({ kDatabaseName, kTableName }));
: s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotAuthorized());
: ASSERT_STR_MATCHES(s.ToString(), "[Uu]nauthorized action");
:
: ASSERT_OK(this->GrantGetMetadataTablePrivilege({ kDatabaseName, kSecondTable }));
: s = this->GetTableLocationsWithTableId(table_name_b, table_id_a);
: ASSERT_TRUE(s.IsNotFound());
: ASSERT_STR_CONTAINS(s.ToString(), "the table ID refers to a different table");
: }
nit: oops, this should go up by the other MasterAuthzITests
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/integration-tests/master_authz-itest.cc@1175
PS13, Line 1175: FALSE(s.ok());
I think we can revert this to IsNetworkError() since this is only run for Sentry. I updated the errors before realizing that we shouldn't be running this part of the test for Ranger anyway.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/mini_ranger-test.cc
File src/kudu/ranger/mini_ranger-test.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/mini_ranger-test.cc@83
PS13, Line 83: curl.FetchURL(JoinPathSegments(ranger_.admin_url(), "service/plugins/policies/count"),
Check for errors.
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15681/13/src/kudu/ranger/ranger_client.cc@573
PS13, Line 573: if (allowed_actions.empty()) {
: LOG(WARNING) << Substitute("User $0 is not authorized to perform actions $1 on table $2",
: user_name, JoinMapped(*actions, ActionPB_Name, ", "), table_name);
: return Status::NotAuthorized(kUnauthorizedAction);
: }
> Should we remove this and let ranger_authz_provider handle it as well?
+1
Also we should separate updates to this file into a separate patch.
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 06:51:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3078 Add Ranger tests to master authz-itest
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15681 )
Change subject: KUDU-3078 Add Ranger tests to master_authz-itest
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc
File src/kudu/integration-tests/ts_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15681/14/src/kudu/integration-tests/ts_sentry-itest.cc@356
PS14, Line 356: public:
> changed it to TSSentryITest without the template if that's what you meant b
That's a small part of it, but my main gripe the (IMO) odd semantics around cluster setup vs harness code -- how it relies on injecting test methods via this lambda instead of clarifying the boundary of what belongs in test code and what belongs in harness code. We do this in master_authz-itest with the GetClusterOpts() and SetUpCluster(HarnessEnum) methods.
--
To view, visit http://gerrit.cloudera.org:8080/15681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25dc67516cd61f0624914989f8db4c4f94d7e3bf
Gerrit-Change-Number: 15681
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 21:07:42 +0000
Gerrit-HasComments: Yes