Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster ...................................................................... Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322 PS2, Line 322: which synchronizes the notification logs with the Sentry : // service. So that Sentry service is > Yeah. Ah, I think my confusion is in separating out the rules for the HMS vs the rules for Kudu. Maybe it's worth stating explicitly that it's verifying that changes to Kudu metadata entries in the HMS are performed by `kudu` (or whatever user it actually is). "authorization metadata operations" is a bit ambiguous IMO since it can refer to authorization for DDL/DML that starts in Kudu and ends up in the HMS, and those DDL/DML operations do need to be authorized, but are referring to a different user. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79 PS8, Line 79: sentry_service_principal nit: perhaps DCHECK this isn't empty? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229 PS8, Line 229: - hive.sentry.conf.url : // Configuration URL of the Sentry authorization plugin in the HMS. : // nit: should be a part of kHiveSentryFileTemplate? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310 PS8, Line 310: - hive.metastore.filter.hook : // Configures the HMS to use the Sentry plugin for filtering : // out information user has no privileges to access for operations : // as SHOWTABLES and SHOWDATABASES. Does this mean that we'll only see HMS notifications that the `kudu` user has Sentry privileges for? Or is this specific to SHOWTABLES and SHOWDATABASES (SHOW TABLES and SHOW DATABASES?) requests to the HMS? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321 PS8, Line 321: Configures the HMS to use the Sentry post-event listener : // which synchronizes the notification logs with the Sentry : // service. So that Sentry service is aware of events such : // as table rename or the table location update, and accordingly : // updates internally. nit reword a bit: "Configures the HMS to use the Sentry post-event listener, which synchronizes the Sentry service with the HMS via Sentry's notification log listener. The Sentry service will be made aware of events like table renames and update itself accordingly." Also, what is a "table location update"? It's not the location of a Kudu table, is it? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95 PS8, Line 95: Substitute("$0.$1", kDatabaseName, kTableName) nit: kTableIdentifier http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231 PS8, Line 231: 3. Restart the Sentry service on the same port, reconfigured to talk : // to the HMS's port. It's been a while since we talked about it, but is it not possible that another process will claim this port? If so, why not? Mind putting the answer in a comment either here or around GetBindIpForExternalServer? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64 PS8, Line 64: case EXTERNAL_SERVER: : idx = kServersMaxNum / 3 + index; Somewhat of a nit: this is giving the larger partition to the masters, when really, it's the tablet servers that we would expect to dominate the idx-space in a larger EMC deployment (if we wanted to test such large clusters). http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114 PS8, Line 114: 127.0.0.1 Why doesn't this need to be the HMS URIs? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@264 PS8, Line 264: kudu,hive Should this depend on IsHmsEnabled()? http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@402 PS3, Line 402: : // The '-Ffn' flag gets lsof to output something like: : // p5801 : // f548 : // n127.0.0.1:43954->127.0.0.1:43617 : // f549 > The output format didn't change, but the way we parse the output is only go Right, but before didn't it expect (n*) within the first three lines? And otherwise it'd be an error? Why are we able to relax that in the way described by this output? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc@411 PS8, Line 411: In each pair, : // the first half of the pair is file descriptor number, we ignore it. nit: wrap the line? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/util/test_util.cc@436 PS8, Line 436: lines[index] nit: cur_line? -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 11 Dec 2018 01:30:28 +0000 Gerrit-HasComments: Yes
