Hi Enrico, all,
> We have the last patch that is waiting for review: > https://github.com/apache/zookeeper/pull/1134/ > > https://issues.apache.org/jira/browse/ZOOKEEPER-1112 Add support for C > client for SASL authentication > > It is a very big patch on the C client and it needs careful review and it > is introducing a lot of changes (many LOCs). I agree that the patch is substantial. I tried, however, to minimize the impact on the existing client code in order to avoid regressions when SASL support is compiled out or disabled. I would encourage reviewers to question my choices, either on this list or on the "pull request"—but perhaps a small "review guide" can help in the meantime: M zookeeper-client/zookeeper-client-c/src/zookeeper.c The most critical change I was not able to avoid lies in 'zookeeper.c': I was forced to split the "bottom half" of the session establishment logic to a 'finalize_session_establishment' procedure. That code still is invoked "synchronously" from the same location when SASL is disabled or inactive—but emulating the Java client causes that finalization to happen after (asynchronous) SASL negociation. The code in 'zookeeper.c' now also tests 'is_sasl_auth_in_progress' from a few locations. This is analoguous to what happens in the Java client. These tests should (hopefully!) be relatively innocuous as the answer is trivially false unless SASL has been compiled in *and* requested. The rest of the changes are fairly straightforward: M zookeeper-client/zookeeper-client-c/src/zk_adaptor.h Adds an optional 'zoo_sasl_client_t *' to 'zhandle'. This changes the size of the struct, but should otherwise have no effect unless SASL is requested. M zookeeper-client/zookeeper-client-c/include/proto.h Just adds a missing #define. M zookeeper-client/zookeeper-client-c/include/zookeeper.h Adds new definitions, but does not modify existing ones. A zookeeper-client/zookeeper-client-c/src/zk_sasl.c A zookeeper-client/zookeeper-client-c/src/zk_sasl.h New files, contain the bulk of the SASL logic. These files are not even compiled in when SASL support is disabled. M zookeeper-client/zookeeper-client-c/src/cli.c Useful for interactive testing of C SASL auth, or as a simple example, but not expected to impact "production" deployments. A zookeeper-client/zookeeper-client-c/tests/TestSASLAuth.cc Tests added (file used to be called TestServerRequireClientSASLAuth.cc) M README_packaging.md M dev/docker/Dockerfile M pom.xml A tools/cmake/Modules/FindCyrusSASL.cmake M zookeeper-client/zookeeper-client-c/CMakeLists.txt M zookeeper-client/zookeeper-client-c/LICENSE M zookeeper-client/zookeeper-client-c/Makefile.am M zookeeper-client/zookeeper-client-c/README M zookeeper-client/zookeeper-client-c/configure.ac M zookeeper-client/zookeeper-client-c/pom.xml M zookeeper-client/zookeeper-client-c/tests/zkServer.sh Misc. configuration/detection logic, documentation updates, LICENCE updates, notes, etc. > Personally I am not able to do a real review of C code and it would need > another committer to be accepted. > > I am sorry, I think it will be a great enhancement and the community will > love it (and Perl/Python bindings will benefit as well), but IMHO it can > wait 3.6.1 or 3.7.0 > We are not in a hurry. Note that I already have working SASL updates to the Perl client in my pipe :) They still need a round of cleanups, but are otherwise ready to go. Waiting for 3.6.1 would not be too bad. But we would really like to see this land in 3.6.x to avoid carrying such an "unofficial patch" in production, where it would only get reviews limited by a singular point of view. > So if no reviewer chimes in within a couple of days I will postpone that > issue to the next release. > > I am continuing to validate branch-3.6 before sending a formal VOTE > > Cheers and season greetings to every one ! > Enrico Cheers, and season greetings! Damien Diederen
