Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 39: (15 comments) http://gerrit.cloudera.org:8080/#/c/21728/39//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21728/39//COMMIT_MSG@16 PS39, Line 16: Else only one of jwt or oauth is supported. Need brief explanation here as to why kerberos/ldap auth being enabled impacts jwt/oauth. http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@228 PS39, Line 228: a Nit: an http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@240 PS39, Line 240: // Enables retrieving the OAuth JWKS from the specified URL This comment does not make sense. Need to add something like "without verifying the presented TLS certificate from the server." http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@257 PS39, Line 257: in the OAuth Nit: in the OAuth token http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@259 PS39, Line 259: Custom claim 'username' This help text copies from JWT, but that help text is unhelpful. Please change this to something like "Custom claim of the token that contains the username" http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@1679 PS39, Line 1679: bool use_oauth = FLAGS_oauth_token_auth; No need to declare this variable. Just use FLAGS_oauth_token_auth directly. http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.h@33 PS39, Line 33: #include "util/jwt-util-internal.h" Unused import. http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.cc@63 PS39, Line 63: #include "util/jwt-util-internal.h" Unused import http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp@352 PS30, Line 352: fallback_to_other_auths = false; > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@367 PS30, Line 367: metrics->AddCounter("impala.webserver.total-oauth-token-auth-success", 0); > These are counter metric creation. Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@1108 PS30, Line 1108: } > The two functions are still almost exactly the same. It would be better to Had an offline discussion, and I am good with these functions being kept separate because these functions are bound to callbacks, and it ends up being clearer to have separate functions versus partially binding some function parameters (such as the startup flag values) and leaving others (such as the token and connection) to be bound at call time. http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc@696 PS35, Line 696: bearer_to > Done Done http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc@706 PS35, Line 706: } > Done Done http://gerrit.cloudera.org:8080/#/c/21728/35/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/21728/35/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java@71 PS35, Line 71: expectedAuthFailure.contains(actualAuthFailure)); > This function can be combined with the previous function since the only dif Done http://gerrit.cloudera.org:8080/#/c/21728/39/tests/custom_cluster/test_shell_oauth_auth.py File tests/custom_cluster/test_shell_oauth_auth.py: http://gerrit.cloudera.org:8080/#/c/21728/39/tests/custom_cluster/test_shell_oauth_auth.py@30 PS39, Line 30: Nit: remove extra space -- To view, visit http://gerrit.cloudera.org:8080/21728 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65dc8db917476b0f0d29b659b9fa51ebaf45b7a6 Gerrit-Change-Number: 21728 Gerrit-PatchSet: 39 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Tue, 14 Jan 2025 00:32:18 +0000 Gerrit-HasComments: Yes
