Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22462 )
Change subject: IMPALA-13687: Support shared secret key for cookies ...................................................................... Patch Set 21: (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: // Create a temporary file with a random key. > Nit: this file will not get cleaned up if an ASSERT fails. Done http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@502 PS20, Line 502: vector<uint8_t> buf(buffer_size); > What happens if the thread join never happens in the AuthenticationHashFrom It'll hang. I don't see another good way to handle it. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@535 PS20, Line 535: while (retries++ < max_retries) { > Nit: There are two different ways of waiting for a hash update. Line 518 The first is expecting it not to change, so we need to use a decently long time to ensure it will probably have been updated. There is a chance of a false positive every once in awhile. The second is expecting it to change, and we can retry several times over a long period to catch this as soon as it changes while allowing for slower cases. 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 Possible error cases: EBADF An invalid file descriptor was given in one of the sets. (Perhaps a file descriptor that was already closed, or one on which an error has occurred.) However, see BUGS. EINTR A signal was caught; see signal(7). EINVAL nfds is negative or exceeds the RLIMIT_NOFILE resource limit (see getrlimit(2)). EINVAL The value contained within timeout is invalid. ENOMEM Unable to allocate memory for internal tables. I don't think any others are common enough to worry about handling. 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 continu Maybe we should retry for a bit (1 minute?) then make it a fatal error. 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 The behavior changed specifically for this in-memory test where we create multiple Webserver in the same process. They used to each instantiate their own AuthenticationHash. Now they grab it from AuthManager, which gets a default instance that's re-used. So now the hash doesn't change when you create a new Webserver. I could update this test to re-initialize AuthenticationHash in AuthManager before starting the new Webserver, but it doesn't seem very useful. 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: writeCookieSecret(keyFile); > The ctests sleep 500ms in one case and up to 2000ms in the other. Can this Done 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: // Cookie auth is expected to succeed at least once, possibly many times. > Nit: Please consider refactoring this code into its own standalone generic Done 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: writeCookieSecret(keyFile); > Same question about other sleep methods to wait for key rotations to be pic Done -- 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: 21 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: Thu, 11 Dec 2025 00:08:42 +0000 Gerrit-HasComments: Yes
