Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13299 )
Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@202 PS3, Line 202: // Return: true on success, false otherwise > this function can be made static, right? Done http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@872 PS3, Line 872: // TODO: add support for SPNEGO style of HTTP auth > I don't think this logic quite makes sense. Just because Kerberos is enable Right. What we should really be checking here is if kerberos and enabled and ldap isn't. In that situation, the user's expectation is clearly that all endpoints are protected by kerberos, but we don't have any way to do that for the http server, so a user who wants to have just kerberos enabled would need to disable the http server. Rather than failing on startup if a user fails to add the startup flag --hs2_http_port=0, I guess we should probably just automatically disable the http server. http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@950 PS3, Line 950: void SecureAuthProvider::SetupConnectionContext( > this can be a const reference to the shared_ptr Done http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@956 PS3, Line 956: lServerTran > consider down_cast<> from gutil/casts.h here, which does an RTTI type check Done http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@963 PS3, Line 963: > same Done http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@79 PS2, Line 79: char* path = strchr(method, ' '); > BTW, yea, I think an LDAP query per RPC is probably pretty bad considering I filed IMPALA-8584 http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp@71 PS3, Line 71: > RFC7230 says the whitespace between the : and the header value is optional: Done http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp@75 PS3, Line 75: > does this end up closing the connection immediately? If not, I'm worried ab Done http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@904 PS3, Line 904: { > nit: weird indentation Done http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@909 PS3, Line 909: "label": "HiveServer2 HTTP API Active Connections", > need to make these labels unique to HTTP (here and below) Done http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@944 PS3, Line 944: { > should we file a JIRA to add metrics for unauthorized conn attempts? or is I filed IMPALA-8583 http://gerrit.cloudera.org:8080/#/c/13299/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/13299/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@102 PS3, Line 102: // Authenticate as 'Test1Ldap' with password '12345' > worth adding a negative test with the wrong password, as well as various in Done -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mike Yoder <[email protected]> Gerrit-Reviewer: Sudhanshu Arora <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 24 May 2019 04:32:48 +0000 Gerrit-HasComments: Yes
