Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 8: Code-Review+2 (6 comments) http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30 PS7, Line 30: 'ALL ON default.a' > Yeah, I don't mean to bring this up here because I expect it to change the I agree that we should be open to exploring different options here wrt responses of which scope we do store in the cache. It might be a configurable parameter, I think: the lowest scope we use to send request to sentry. The 'table' scope seems to be a reasonable default. However, caching information for the higher than requested levels would complicate queuing requests for the related entities, but I think it's manageable (I think we could use some sort of the 'tokens' idea that we use in thread pools elsewhere, if I'm not confusing that with something else). As for the choice between storing just the raw Sentry responses or some sort of structures to be able to search through more effectively, I'm thinking we should opt for the latter. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@442 PS7, Line 442: Status s = sentry_authz_provider_->Authorize(scope, SentryAction::Action::ALL, : Substitute("$0.$1", db, tbl), kTestUser); > This test is not to cover that but for verifying authorizable scope filter SGTM. Thanks for the clarification. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@451 PS7, Line 451: // Scope::COLUMN is excluded since column scope for table : // authorizable doesn't make sense. > Not quite following, if COLUMN scope is passed to Authorize(), there is a D Ah, I missed that, whoops. Indeed. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@265 PS7, Line 265: yAuthorizableScope::S > Adar suggests require_grant_option, so I am going to go with that. SGTM http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279 PS7, Line 279: 3. grant option from the former implies the grant option from the latter. > I agree it is strange for cases all on column level implies rename or drop All right, that's how Sentry works and we cannot do much about that. At least, we don't support that wildcard implications for this integration, so we don't need to worry about that :) http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@49 PS7, Line 49: > The default constructor is useful for FromString(const std::string& str, Se Thanks! -- To view, visit http://gerrit.cloudera.org:8080/12500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a Gerrit-Change-Number: 12500 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Mar 2019 23:16:04 +0000 Gerrit-HasComments: Yes
