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 11: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/rpc/authentication.cc@1726 PS9, Line 1726: if (!FLAGS_cookie_secret_file.empty()) { > max_cookie_lifetime_s doesn't seem to impact whether cookies are generated, That is interesting. I agree removing the 'FLAGS_max_cookie_lifetime_s > 0' part of the condition would be a good approach (which I see you already did in patch 11). http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc@306 PS9, Line 306: if (n != SHA256_DIGEST_LENGTH) { > What would make it an invalid digest? It might not be intended, but it'd li After reading this response and thinking about this more, I agree we should not worry about this situation. It's likelihood of happening is low and the consequences are irrelevant. http://gerrit.cloudera.org:8080/#/c/22462/9/be/src/util/openssl-util.cc@325 PS9, Line 325: int wd = inotify_add_watch( > When recreated, it will either trigger IN_CLOSE_WRITE or IN_MOVE_SELF. I do It's better if we read the file twice than not at all! http://gerrit.cloudera.org:8080/#/c/22462/9/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/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1060 PS9, Line 1060: TOpenSessionReq openReq = new TOpenSessionReq(); > Decided to leave this, log scraping has been a bit error-prone and we don't Ack http://gerrit.cloudera.org:8080/#/c/22462/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1066 PS9, Line 1066: } > Done Ack http://gerrit.cloudera.org:8080/#/c/22462/9/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/9/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@352 PS9, Line 352: // Cookie auth is expected to succeed at least once, possibly many times. > Moved this to a helper and changed it to ThreadLocalRandom to prioritize pe Ack http://gerrit.cloudera.org:8080/#/c/22462/9/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/9/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@186 PS9, Line 186: // there will already be some successful auth attempts. > Done Ack -- 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: 11 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: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 11 Sep 2025 20:29:35 +0000 Gerrit-HasComments: Yes
