Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 23: (13 comments) http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/CMakeLists.txt File src/kudu/ranger/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/CMakeLists.txt@53 PS22, Line 53: : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/bin" : "${EXECUTABLE_OUTPUT_PATH}/postgres") : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/lib" : "${EXECUTABLE_OUTPUT_PATH}/postgres-lib") : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/jdbc/postgresql.jar" : "${EXECUTABLE_OUTPUT_PATH}/postgres/") Can this be removed now that Postgres is in its own module? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h@40 PS22, Line 40: nit: maybe typedef this too as UserList so this is more self-documenting. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h@47 PS22, Line 47: : // The AuthorizationPolicy contains a set of privileges on a resource to one or : // more user. 'items' is a vector of user-list of actions pair. This struct can : // be used to create new Ranger nit: it isn't clear upon reading the comment whether the vectors here should be all the same length (e.g. each size N to specify N database->table->column resources), or should have their cross-product taken. Could you clarify? Alternatively, it seems like this was done to match the Ranger REST endpoint. Would something like this be closer to what Ranger exposes to users? It'd be nice to hide internal details of Ranger's more developer-facing endpoints, especially if it improves composability in tests. // Strings may be real values to target specific resources, "*" to assign wildcard privileges, or empty. struct RangerPrivilege { string database; string table; string column; vector<PolicyItem> actions; // btw maybe we should rename PolicyItem to UsersAndActions or something? }; struct AuthorizationPolicy { string policy_name; vector<RangerPrivilege> privileges; }; http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@61 PS22, Line 61: ranger_process_ RETURN_NOT_OK http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@195 PS22, Line 195: // Set JAVA_HOME : // Set ranger-admin-env* variables : // - RANGER_PID_DIR_PATH = /tmp (probably don't need) : // - RANGER_ADMIN_PID_NAME=rangeradmin.pid : // - RANGER_USER : // Set JAVA_OPTS and JAVA_HOME : // RANGER_ADMIN_LOG_DIR=${XAPOLICYMGR_EWS_DIR}/logs : // Set up pid file (probably don't need) : // Run tomcat server : : string classpath = ranger_classpath(); : : LOG(INFO) << "Using Probably don't need these comments anymore, but it's probably worth linking to a specific hash of Ranger's ranger-admin-services.sh http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@234 PS22, Line 234: }); You can remove these :) XXX(awong) comments are just notes to myself that I intend on addressing before pushing/de-wipping. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@256 PS22, Line 256: ststring result; : Status s = POSTToRanger("service/plugins/services", std::move(service), &result); : if (s.IsRemoteError()) { : return Status::RemoteError(Substitute("Faled to create Kudu service. Status: $0; message: $1", : s.ToString(), result.ToString())); : } : return s; : } : nit: shift back a couple spaces http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h File src/kudu/ranger/mini_ranger_configs.h: PS22: Thanks for grokking and organizing these configs! Makes it significantly more understandable/maintainable :) http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@63 PS22, Line 63: external ranger port (this is the one that actually matters) nit: could you reword this? Matters for what? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@78 PS22, Line 78: 1 nit: reorder with $0? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@139 PS22, Line 139: <property> : <name>ranger.service.shutdown.port</name> : <value>$1</value> : </property> Now that we're just using SIGTERM, do we still need this? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@224 PS22, Line 224: jdbc:log4jdbc:mysql postgresql? Does this actually matter? http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto@45 PS22, Line 45: // Whether or not Ranger should be enabled : optional bool enable_ranger = 9; : Hrm, why don't we have an enable_sentry? Will we actually use this? FWIW it would've been nice to have this patch focus solely on getting MiniRanger up and running, before plumbing it into the mini-cluster, especially if there aren't mini-cluster tests that exercise this. -- To view, visit http://gerrit.cloudera.org:8080/15483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15ab1eb8abe71c074c26b286073442882e101bc6 Gerrit-Change-Number: 15483 Gerrit-PatchSet: 23 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sun, 22 Mar 2020 23:43:27 +0000 Gerrit-HasComments: Yes
