Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22462 )
Change subject: IMPALA-13687: Support shared secret key for cookies ...................................................................... Patch Set 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@488 PS20, Line 488: string key_path = Substitute("/tmp/auth_key_$0", getpid()); Nit: this file will not get cleaned up if an ASSERT fails. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@502 PS20, Line 502: // On exit the destructor will halt and join with the reload thread. What happens if the thread join never happens in the AuthenticationHashFromFile destructor? Will the test eventually time out and fail? http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@535 PS20, Line 535: } Nit: There are two different ways of waiting for a hash update. Line 518 sleeps for 500 milliseconds while this code retries up to 2 seconds. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc@357 PS20, Line 357: LOG(ERROR) << "select() failed in reload thread: " << strerror(errno); Are there any other cases where a fail during select() would be recoverable? Should we try a few times before breaking? http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc@358 PS20, Line 358: break; If this break is hit, how will the end users know? Everything will continue to function normally until after a key rotation happens and an incoming connection with an auth hash created on another coordinator does not have an alternate means of identifying itself (such as basic auth). It could take quite awhile for this type of situation to happen. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/webserver-test.cc File be/src/util/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/webserver-test.cc@a603 PS20, Line 603: I'm not understanding why this was removed. Unless the cookie_secret_file flag is set, the coordinator should get a new hash each time it starts, and I don't see where the cookie_secret_flag is set. http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1052 PS20, Line 1052: Thread.sleep(1000); // Wait for the key to be reloaded. The ctests sleep 500ms in one case and up to 2000ms in the other. Can this sleep be unified with the ctests or possibly wait for the "Loaded authentication key from" log message to appear? http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@344 PS20, Line 344: assertTrue("Did not authenticate within timeout", successReached); Nit: Please consider refactoring this code into its own standalone generic function. While I don't see any other JUnit test that waits for a particular metric value, it would still be nice to have for the future. http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@205 PS20, Line 205: Thread.sleep(1000); // Wait for the key to be reloaded. Same question about other sleep methods to wait for key rotations to be picked up by the coordinator -- To view, visit http://gerrit.cloudera.org:8080/22462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e2345f771608069407e9dcf7ed4697fc0214e7 Gerrit-Change-Number: 22462 Gerrit-PatchSet: 20 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 09 Dec 2025 23:22:50 +0000 Gerrit-HasComments: Yes
