Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@56 PS3, Line 56: using sentry::TSentryPrivilege; > warning: using decl 'set' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@132 PS3, Line 132: requested_authorizable, /*scope=*/nullptr, /*action=*/nullptr)); : } > Yes, I understand the reasoning here: the responses are from real Sentry in Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@147 PS3, Line 147: const char* const kTestUser = "test-user"; > warning: 'kAllActions' is a static definition in anonymous namespace; stati Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@171 PS3, Line 171: } > style nit: it seems the indent should be four spaces; a space after the col Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@191 PS3, Line 191: // The scope string is set to something other than the expected scope. > warning: parameter 'user' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@264 PS3, Line 264: : // Select a scope at which we'll mess up the privilege request's field. : AuthorizableScopesSet nonempty_fields = : SentryAuthzProvider::ExpectedNonEmptyFields(scope.scope()); : if (invalid_privilege == InvalidPrivilege::FLIPPED_FIELD) { : > syntax sugar addict's nit: could it be just Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@47 PS3, Line 47: std::string db, > warning: the parameter 'server' is copied for each invocation but only used Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@77 PS3, Line 77: > I understand that it's directly related to Sentry's terminology, but I'm no Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87 PS3, Line 87: uthorizable hierarchy > Whoops, it should have been '... the user is granted privileges on ...' Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@90 PS3, Line 90: > Is it going to be bound to the table only? No, clarified this. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@95 PS3, Line 95: e. > I'm not sure I understand why this parameter is necessary given the descrip Yeah, I'll just remove it. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325 PS3, Line 325: InsertOrDie(&expected_empt > Why not It sounds more like an English sentence as is, i.e. if (SentryPrivilegeIsWellFormed) reads as, "if the Sentry privilege is well formed ..." http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@463 PS3, Line 463: DCHECK(!d > Do you think it's worth logging a message in that case? Maybe LOG(INFO) or Done http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53 PS3, Line 53: ALL, > Not from this particular changelist, but while you are at it: maybe, move A I somewhat see the point of this, but don't feel strongly enough about it to change it here and everywhere else where this order is held. I'm going to leave it for now unless you feel strongly about it. Alternative orderings would be alphabetical, or sorted by breadth. AFAICT there isn't currently an order to this, but i might be wrong. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91 PS3, Line 91: > Is it possible to create the result AuthorizableScopesSet once and the just >Is it possible to create the result AuthorizableScopesSet once and the just >return references to those? E.g., maybe declare those as static inside the >function and populate those just once. Maybe I'm missing something but I don't think making this static would mean generating the result once. 'scope' isn't known at compile time. That said, I've went with your suggestion of defining these in the cc. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@114 PS3, Line 114: : : : : : : : : : : : : : : : : : : : > ditto Done -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 16:00:55 +0000 Gerrit-HasComments: Yes
