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 <[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 06:51:52 +0000 Gerrit-HasComments: Yes
