Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG@14 PS4, Line 14: kudu_system > Should we enforce that all non-user tables are under this namespace? Yes, but maybe when we add a second non-user table. http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@383 PS4, Line 383: using std::pair; > warning: using decl 'map' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1531 PS4, Line 1531: // Do some fix-up of any defaults specified on columns. > Would we still want this for system tables? Then the owner would be the kud Hrm, good point. The system client would be a part of the server and that should mean that the table's owner would be the service user. Re: requiring ownership on table access, I just leveraged the default authorization for the admin service, from tserver_admin.proto service TabletServerAdminService { option (kudu.rpc.default_authz_method) = "AuthorizeServiceUser"; http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1540 PS4, Line 1540: > Why don't system tables need this? I forwent these schema checks because we're passing in a statically-defined schema that we know is correct. That said, who's to say the statically-defined schema doesn't eventually change and break things. I'll move this out of this block. http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1559 PS4, Line 1559: return SetupError( > Are you planning to do this in a follow on commit? Would it be easy enough to > enforce requiring a superuser temporarily as opposed to being completely open? That's always the case with TODOs :) As it pertains to the this feature, it would have been a must-do before calling the feature done for sure, given it would have been an easy attack vector. [post-update] The change was easy, but even a seemingly a simple change can have pretty heavy implications for testing, etc. In any case, good catch. Updated so we never list the system tables, even if we own them. -- To view, visit http://gerrit.cloudera.org:8080/16124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa Gerrit-Change-Number: 16124 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 16 Jul 2020 06:05:41 +0000 Gerrit-HasComments: Yes
