Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17759 )
Change subject: IMPALA-10846: Skip Authentication for connection with trusted auth header ...................................................................... Patch Set 1: (8 comments) This looks good, I have a few suggestions for cleanup and clarity. http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/rpc/authentication.cc@135 PS1, Line 135: // This flag defines a HTTP header which indicates a connection is trusted. This comment duplicates the flag description (below), probably we should just have the flag description. http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/rpc/authentication.cc@145 PS1, Line 145: "If set as non empty string, Impala will look for this header in the connection " I think the header is in the http headers, rather than the connections string? http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/rpc/authentication.cc@146 PS1, Line 146: "string. If the header presents, Impala will skip authentication and extract the " Nit: "If the header is present" http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/rpc/authentication.cc@151 PS1, Line 151: "connections are authenticated before the requests lands on Impala."); Nit: land http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.h File be/src/util/webserver.h: http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.h@204 PS1, Line 204: /// Returns true if the connection has a valid username set in the request's the Nit: delete the last "the" http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.h@222 PS1, Line 222: bool GetUsernameFromAuthHeader(struct sq_connection* connection, Add a simple description http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17759/1/be/src/util/webserver.cc@887 PS1, Line 887: err_msg = Substitute("Error parsing basic authentication token from: $0 Error: $1", Nit: should be "error" so that if TrustedDomainCheck prints the message it will say "but error ..." http://gerrit.cloudera.org:8080/#/c/17759/1/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/17759/1/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@286 PS1, Line 286: What happens with the trusted auth header and a bad password? The main description of the flag says the password *may* be blank, does that imply it is ignored? -- To view, visit http://gerrit.cloudera.org:8080/17759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7103012bc9cf9dfdc9c184af3a281526d8127a31 Gerrit-Change-Number: 17759 Gerrit-PatchSet: 1 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Aug 2021 21:39:45 +0000 Gerrit-HasComments: Yes
