Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/common/schema.h@552 PS7, Line 552: std::vector<ColumnId> column_ids() const { > Can you return a cref of col_ids_ instead? It'll make some operations (like Done http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc File src/kudu/tserver/tablet_server_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc@444 PS7, Line 444: const int kNumKeys = 5; : const int kNumVals = 5; : const char* kScanTableName = "scan-table-name"; : const char* kScanTabletId = "scan-tablet-id"; : const char* kScanUser = "good-guy"; > All of these can be declared as static. And if Alexey is going to review th Done http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_server_authorization-test.cc@509 PS7, Line 509: pb.set_read_mode(READ_AT_SNAPSHOT); > Why this? A fault tolerant scan must be a snapshot scan, according to HandleNewScanRequest: // Ordered scans must be at a snapshot so that we perform a serializable read (which can be // resumed). Otherwise, this would be read committed isolation, which is not resumable. Added a small comment, but not sure how useful it is, other than noting that this is needed for the scan type. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@427 PS4, Line 427: return false; > Regarding IS_DELETED, it's kind of murky. In theory you could add it to any I think there are a couple of approaches worth considering: - Tightly couple diff scan and ORDERED scan mode. Depending on how the deduplication of ghost rows turns out, it seems like this might be necessary anyway? Can a user get garbage back if they try a diff scan in UNORDERED mode? In which case, we should add the appropriate checks at the tablet service endpoints. - Don't worry about it -- no scan is allowed without any scan privileges, and so whatever a user _is_ authorized to scan, it seems reasonable to assume they also have the privilege to effectively scan on the history of those columns. This I think is a bit fuzzy, and would be worth checking with an expert if we think this is the right option. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1533 PS4, Line 1533: if (!GetRequiredColumnPrivilegesForNewScan(scan_pb, schema, &required_column_privileges)) { > I see your point re: not just being projected columns. Yeah, my issue is th Got it, yeah I named it required_column_privileges. Lmk if you think it'd still be confusing with less context -- I think it's fine, but I do want it to be understandable for someone coming into this part of the codebase. http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/7/src/kudu/tserver/tablet_service.cc@409 PS7, Line 409: vector<ColumnId> cols = schema.column_ids(); : *required_column_privileges = unordered_set<ColumnId>(cols.begin(), cols.end()); > If column_ids() returns a cref, you could combine these two lines without l Done -- To view, visit http://gerrit.cloudera.org:8080/11753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905 Gerrit-Change-Number: 11753 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 18 Mar 2019 05:20:06 +0000 Gerrit-HasComments: Yes
