Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )
Change subject: [sentry] SentryAction ...................................................................... Patch Set 1: (14 comments) Discussed with Dan offline, and we agreed that it is sufficient to only add SentryAction for checking if an action can imply another since Sentry provides API to query privileges filtered by authorizables. Updated this patch and removed SentryPrivilege. 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 M Done 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 Removed. 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? Removed. http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@79 PS1, Line 79: SentryPrivilege > const ref Done 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()` a Not really as the authorizable is in a linear hierarchy, but removing this logic anyway. 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& Done http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@43 PS1, Line 43: "ALL" > kWildCardAll Done 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 Done 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. Done 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. Done 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 Done 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? I don't think we will normally do that, just adding the logic in case this ever happen. 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 Removed. 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 ==? Done -- 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: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Sun, 14 Oct 2018 00:39:52 +0000 Gerrit-HasComments: Yes
