Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 17: (42 comments) http://gerrit.cloudera.org:8080/#/c/15483/16/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/15483/16/build-support/dist_test.py@116 PS16, Line 116: # Add Postgres and Ranger. These are symlinks to directories in thirdparty. > Nit: terminate with a period. Done http://gerrit.cloudera.org:8080/#/c/15483/16/build-support/run_dist_test.py File build-support/run_dist_test.py: http://gerrit.cloudera.org:8080/#/c/15483/16/build-support/run_dist_test.py@165 PS16, Line 165: os.path.join(bin_path, "postgres")) > Can you just merge this loop with the chrony one, and update the comment? Done http://gerrit.cloudera.org:8080/#/c/15483/16/build-support/run_dist_test.py@176 PS16, Line 176: in(ROOT, "build/dist-test-system-libs/")] > This was already done above (L154); reuse. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/mini-cluster/external_mini_cluster.cc@337 PS16, Line 337: if (opts_.enable_ranger) { > Where do we configure the Kudu Masters to talk to MiniRanger? Or is that go I was planning to do that in a follow-up, we need to configure the subprocess for that. Should I include that in this patch? http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/CMakeLists.txt File src/kudu/ranger/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/CMakeLists.txt@69 PS16, Line 69: "${EXECUTABLE_OUTPUT_PATH}/hadoop-home") > What is this comment telling us? Is this a TODO? not sure, this was copied from Andrew's abandoned patch. Removed it, it works without execute. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/CMakeLists.txt@100 PS16, Line 100: > Alphabetical sorting. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger-test.cc File src/kudu/ranger/mini_ranger-test.cc: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger-test.cc@40 PS16, Line 40: MiniRanger ranger_; > Should be ranger_ Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger-test.cc@41 PS16, Line 41: }; > Nit: empty line here. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger-test.cc@44 PS16, Line 44: PolicyItem item; > We should at least a full lifecycle (start, restart, stop, etc) Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger-test.cc@52 PS16, Line 52: policy.name = "test1"; > std::move Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@40 PS16, Line 40: typedef std::pair<std::vector<std::string>, std::vector<ActionPB>> PolicyItem; > This needs documentation, and should be called PolicyItem. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@42 PS16, Line 42: // The AuthorizationPolicy contains a set of privileges on a resource to one or > Doc. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@50 PS16, Line 50: std::vector<PolicyItem> items; > Doc. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@57 PS16, Line 57: GetT > How did you get away with this? We shouldn't be 'using' anything in this he No idea why the compiler didn't complain, but fixed it. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@64 PS16, Line 64: > Doc. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@69 PS16, Line 69: // Adds a new policy to Ranger. > Doc. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.h@94 PS16, Line 94: JoinPathSegments(ranger_home_, "ews"), java_home_, > Doc. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@53 PS16, Line 53: > What a mess. In future work, could you: Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1132 PS16, Line 1132: > Does this warrant a TODO or something? I presume we want to support Kerbero changed it to configurable http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1141 PS16, Line 1141: > Yeah, this should be the next priority, otherwise the massive number of Sen Apparently Postgres seems to be ready to use when the port is up so it works fine without this, so removed the todo and the wait. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1147 PS16, Line 1147: > What if Start() failed mid-way such that mini_pg_ has started but Ranger ha Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1148 PS16, Line 1148: > Should WARN_NOT_OK. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1157 PS16, Line 1157: > Holy moly you can't just send the thing a TERM/KILL? What happens if you do that seems to work too, changed it to send SIGTERM instead. No idea why Ranger service script chose to use this shutdown method. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1179 PS16, Line 1179: : : > Do in the constructor and make data_root_ const? Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1213 PS16, Line 1213: > Decompose this block into a helper. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1215 PS16, Line 1215: > RETURN_NOT_OK. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1223 PS16, Line 1223: > Shouldn't this be outside the "fresh install" case? Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1232 PS16, Line 1232: > What about setting up Ranger on a UNIQUE_LOOPBACK address? wouldn't work on MacOS http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1237 PS16, Line 1237: > There should be a comment somewhere explaining all of the ports that we nee Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1238 PS16, Line 1238: > Decompose all the config stuff into a helper. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1258 PS16, Line 1258: > Decompose this block into a helper. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1261 PS16, Line 1261: : > Add a symlink function to env.h and env_posix.h (and a test to env-test.cc) Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1264 PS16, Line 1264: > Link? Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1275 PS16, Line 1275: > Doesn't need to be heap allocated. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1343 PS16, Line 1343: : : : : : : : : > Decompose into a helper. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1357 PS16, Line 1357: > Pass by value and std::move both of these. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1366 PS16, Line 1366: > Duplicated from MiniPostgres; decompose into net_util somewhere. Done http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/ranger/mini_ranger.cc@1434 PS16, Line 1434: > If there's an error, will it be reflected in the HTTP status code (i.e. wil EasyCurl returns a RemoteError if the HTTP status code is not 2xx (fixed it now, it was 200 while all 2xx codes indicate success. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: PS16: > Can you unit test this change? See PasswdWebserverTest in webserver-test (w Did some refactoring of the tests, but it seems the webserver is using DIGEST authn (couldn't get BASIC to work properly at least) and Ranger is using BASIC. So I ended up refactoring EasyCurl a bit more, DIGEST and SPNEGO are tested by webserver-test, BASIC is only tested in MiniRanger. http://gerrit.cloudera.org:8080/#/c/15483/16/src/kudu/util/curl_util.h@51 PS16, Line 51: // Fetch the given URL into the provided buffer. : // Any existing data in the buffer is replaced. > These are likely to be the same with each call to FetchURL or PostToURL; co Done http://gerrit.cloudera.org:8080/#/c/15483/16/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/15483/16/thirdparty/build-thirdparty.sh@298 PS16, Line 298: ln -nsf \ > This isn't a directory, so why -R? Done http://gerrit.cloudera.org:8080/#/c/15483/16/thirdparty/patches/ranger-fixscripts.patch File thirdparty/patches/ranger-fixscripts.patch: PS16: > The patch is obviously just a limited view into the script itself, but is i Basically the script didn't allow setting XAPOLICYMGR_EWS_DIR directly and readlink -f doesn't work on Mac, so I opted to remove this part and set XAPOLICYMGR_EWS_DIR from Subprocess. -- 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: 17 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 17:33:31 +0000 Gerrit-HasComments: Yes
