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 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/auth-provider.h@179 PS3, Line 179: std::unique_ptr<AuthenticationHash> hash_; > That seems like reasonable behavior. I don't recall if that's what this pat It does not appear to be doing that. http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/rpc/authentication.cc@1408 PS3, Line 1408: if (!FLAGS_cookie_secret_file.empty() && FLAGS_max_cookie_lifetime_s > 0) { > I should document why max_cookie_lifetime_s should be non-zero for this. Yes, since FLAGS_max_cookie_lifetime_s == 0 turns off cookie auth. Although, I'm sure where to document it. http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/3/be/src/util/openssl-util.cc@218 PS3, Line 218: std::lock_guard<std::mutex> l(key_lock_); > Could use a rwlock, or possibly an atomic. I thought about a rwlock but wasn't sure if it would end up being faster. The kudu class percpu_rwlock seems to be a promising option. http://gerrit.cloudera.org:8080/#/c/22462/3/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/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@987 PS3, Line 987: final String[] cookie = new String[1]; I'm not sure why this is a final String[]. Is it because the variable has to be final to be used in the cookieSaver anonymous interface implementation? http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@992 PS3, Line 992: } Please consider adding else { fail("cookie not provided") } here to assert the auth cookie is set. http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1004 PS3, Line 1004: TCLIService.Iface client = new TCLIService.Client(new TBinaryProtocol(transport)); Will this always connect over port 28000? My concern is if the connection is made over port 28001, it won't assert the same cookie can be used across coordinators since the code starting at line 1024 always connects over port 28001. http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1019 PS3, Line 1019: verifyCookieMetrics(2, 0); Move this statement outside the try block to handle the case where execAndFetch fails but does not throw an exception. http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1035 PS3, Line 1035: assertEquals(1, actualCookieAuthSuccess); Should be able to replace with a call to verifyCookieMetrics(1, 0) http://gerrit.cloudera.org:8080/#/c/22462/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1042 PS3, Line 1042: assertEquals(3, actualCookieAuthSuccess); Should be able to replace with a call to verifyCookieMetrics(2, 0) -- 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: 3 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: Fri, 11 Jul 2025 20:25:54 +0000 Gerrit-HasComments: Yes
