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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 09 Apr 2020 16:11:19 +0000 Gerrit-HasComments: Yes
