Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15897 )
Change subject: docs: add Ranger integration ...................................................................... Patch Set 1: (38 comments) http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc File docs/security.adoc: http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@160 PS1, Line 160: defined in Apache Sentry 2.2 and later. In addition, starting from Kudu > +1, maybe even a "WARNING:" message. Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@223 PS1, Line 223: === Apache Ranger > nit: Put this section above the Sentry section since it is preferred. Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@226 PS1, Line 226: fashion as Apache Sentry, but without `Server` scope in the hierarchy. > We should not define things by relating them to Apache Sentry. Users config Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@228 PS1, Line 228: NOTE: Ranger allows you to add separate service repositories to manage > nit: maybe also mention the Ranger config file that would need to be update Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@233 PS1, Line 233: * *Database* - indicated as a prefix of table names with the format > May preface with. "Kudu does not have the concept of a database. Therefore, Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@234 PS1, Line 234: Since Kudu's only restriction on table names is : that they be a valid UTF-8 encoded string, which remains the same with : Ranger integration, the database in this case for tables named such as : `impala::db.table` will be `impala::db`. > nit: this is a bit awkward because it's half guidance, and half example. Ma Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@237 PS1, Line 237: `impala::db.table` will be `impala::db`. > Is there more description of this "impala::" oddity in the HMS sync or Impa Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@256 PS1, Line 256: All of the actions have the same semantics as in Sentry with the exception that > Yeah, it's probably worth explicitly saying that "ALL" implies all other ac Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@265 PS1, Line 265: DATABASE > lowercase Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@266 PS1, Line 266: `SELECT` privilege on `db=a->table=*->column=*` : to match the semantics of `SELECT` > "the `SELECT` privilege" for both of these Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@271 PS1, Line 271: the following <<security.adoc#policy-for-kudu-masters,section > nit: "the following section" usually means "the section immediately followi Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@275 PS1, Line 275: NOTE: Enabling of Ranger integration does not require the cluster to be integrated > I don't think this note is required since a new user wouldn't expect this. Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@277 PS1, Line 277: Apache Hive Metastore. > nit: maybe also mention Impala, which uses the same Ranger policies as Hive Removed as Grant suggested. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@277 PS1, Line 277: is Kudu specific, and are not in sync > "are Kudu specific, and are not synchronized" Removed as Grant suggested. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@279 PS1, Line 279: remains > "remain" Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@280 PS1, Line 280: are > "is" Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@285 PS1, Line 285: e.g. Sentry or Ranger > Fine-grained authz and coarse-grained authn use different tokens, so I don' Ack http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@379 PS1, Line 379: === Configuring the Integration with Apache Ranger > nit: Put this section above the Sentry section since it is preferred. Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@381 PS1, Line 381: NOTE: Ranger is often configured with Kerberos authentication. See > Is Kerberos authentication required? No, it is not, but It is best practice to enable Kerberos in a Ranger secured cluster. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@384 PS1, Line 384: should not be enabled > nit: "can not be" as it's actually enforced. Also consider adding a similar Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@388 PS1, Line 388: Note it down > nit: "Note its path"? Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@389 PS1, Line 389: Ranger subprocess > Users probably don't know what this is, so it's probably worth elaborating. Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@391 PS1, Line 391: are > nit: that are Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@392 PS1, Line 392: names begin or end > "names that begin" Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@392 PS1, Line 392: not Ranger-compatible, which are names begin or end with a period. Also check > What happens if a user enables Ranger with incompatible table names? The authorization will be denied by default. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@393 PS1, Line 393: there are no multiple Kudu tables whose names only differ by case since : policies authorization are case-insensitive > "that there are no two table names that only differ by case, since authoriz Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@394 PS1, Line 394: comply : the requirements > "comply with the requirements" Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@397 PS1, Line 397: * Create Ranger client `ranger-hive-security.xml` configuration file, and note down > Are there Ranger docs we can link to for this configuration? Unfortunately I cannot find any appropriate Apache Ranger docs for this. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@397 PS1, Line 397: hive > Shouldn't this be `ranger-kudu-security.xml`? Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@459 PS1, Line 459: master > nit: Kudu master Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@461 PS1, Line 461: ``` > +1 Done http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@463 PS1, Line 463: --ranger_java_path=<Path where the Java binary was installed> > If `--ranger_java_path` isn't set will it use $JAVA_HOME, look in the PATH Right, but as it is clear in the flag description I don't think we need to mention again here? http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@468 PS1, Line 468: may facilitate > nit: add an explanation when this is necessary (backup, checksum scan, etc) Done. In terms of impact, I think it is clear with the statement 'The 'kudu' user is also granted access to all Kudu data', no? http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@473 PS1, Line 473: configurations > nit: also add an explanation what this is for Hmm, not sure if it is necessary as the description of the tserver_enforce_access_control flag is very thorough? I think the important thing here is to mention this needs to be set on tserver. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@476 PS1, Line 476: --tserver_enforce_access_control=true > Not related to this patch, but it might be a good idea to add some validati Ack, filed KUDU-3118 http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@486 PS1, Line 486: <property> > isn't tag.download.auth.users also necessary? or is that only for tag-based Yeah, we don't support tag-based policy yet, it would be better to add it when we have the support. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@493 PS1, Line 493: === Caching > Maybe these should live under the respective configuring sections for each Done, I separated them to two sections. Although I don't feel they belong to configuration sections as it is not necessary to config these. Let me know if you feel other wise. http://gerrit.cloudera.org:8080/#/c/15897/1/docs/security.adoc@518 PS1, Line 518: can determine > nit: to set Done -- To view, visit http://gerrit.cloudera.org:8080/15897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iad9476f18267c1e14a73f893fd812674c955eee2 Gerrit-Change-Number: 15897 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 May 2020 00:22:22 +0000 Gerrit-HasComments: Yes
