Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )
Change subject: [sentry] SentryPrivilege ...................................................................... Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc File src/kudu/sentry/sentry_privilege-test.cc: http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc@65 PS1, Line 65: db_select.set_action(SentryPrivilege::SELECT); : ASSERT_TRUE(db_all.Imply(db_select)); : : SentryPrivilege db_insert = db_all; : db_insert.set_action(SentryPrivilege::INSERT); : ASSERT_TRUE(db_all.Imply(db_insert)); : : SentryPrivilege db_update = db_all; : db_update.set_action(SentryPrivilege::UPDATE); : ASSERT_TRUE(db_all.Imply(db_update)); : : SentryPrivilege db_delete = db_all; : db_delete.set_action(SentryPrivilege::DELETE); : ASSERT_TRUE(db_all.Imply(db_delete)); : : SentryPrivilege db_alter = db_all; : db_alter.set_action(SentryPrivilege::ALTER); : ASSERT_TRUE(db_all.Imply(db_alter)); : : SentryPrivilege db_create = db_all; : db_create.set_action(SentryPrivilege::CREATE); : ASSERT_TRUE(db_all.Imply(db_create)); : : SentryPrivilege db_drop = db_all; : db_drop.set_action(SentryPrivilege::DROP); : ASSERT_TRUE(db_all.Imply(db_drop)); : : SentryPrivilege db_owner = db_all; : db_owner.set_action(SentryPrivilege::OWNER); : ASSERT_TRUE(db_all.Imply(db_owner)); : : SentryPrivilege db_metadata = db_all; : db_metadata.set_action(SentryPrivilege::METADATA); : ASSERT_TRUE(db_all.Imply(db_metadata)); Mind writing this as a loop over all the Action types? Also the OWNER and METADATA asserts below. http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc@147 PS1, Line 147: SentryPrivilege column_insert(table_all); : column_insert.set_column(kColumn); : column_insert.set_action(SentryPrivilege::INSERT); May be worth investing in a constructor to which we can set the action and authorizables? http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h File src/kudu/sentry/sentry_privilege.h: http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@32 PS1, Line 32: Privilege CommonPrivilege? http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@79 PS1, Line 79: SentryPrivilege const ref http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@100 PS1, Line 100: void set_table(std::string table) { Should this also check that `column_` is empty? Same for `set_database()` and `table_`? http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc File src/kudu/sentry/sentry_privilege.cc: http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@39 PS1, Line 39: & nit: string& http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@43 PS1, Line 43: "ALL" kWildCardAll http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@44 PS1, Line 44: kWildCardAll kWildCard? Might be a missing test case somewhere http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@71 PS1, Line 71: const Action& Since this is an enum, we don't need the const ref. http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@76 PS1, Line 76: database_(""), : table_(""), : column_("") These aren't needed. http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@95 PS1, Line 95: CHECK(!database_.empty()); : CHECK(!table_.empty()); What is enforcing these? Is it possible that we're given a sentry privilege that is malformed? We might want to handle malformed privileges anyway (e.g. by returning a Status) instead of crashing http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@102 PS1, Line 102: this == &other Do we ever expect to compare a SentryPrivilege object with its own pointer? http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@111 PS1, Line 111: : if (!ImplyAuthorizable(server(), other.server())) { : return false; : } : : if (!ImplyAction(action(), other.action())) { : return false; : } : : if (!database().empty() && : !ImplyAuthorizable(database(), other.database())) { : return false; : } : : if (!table().empty() && : !ImplyAuthorizable(table(), other.table())) { : return false; : } : : if (!column().empty() && : !ImplyAuthorizable(column(), other.column())) { : return false; : } It's probably worth commenting why this is ordered this way. Also maybe put the ImplyAction() first so all the Authorizables are together? http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@144 PS1, Line 144: boost::iequals(required_authorizable, kWildCardSome) kWildCardSome is already case insensitive, no? Just use ==? -- To view, visit http://gerrit.cloudera.org:8080/11656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49 Gerrit-Change-Number: 11656 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 11 Oct 2018 17:04:59 +0000 Gerrit-HasComments: Yes
