gaurav singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 37: (13 comments) http://gerrit.cloudera.org:8080/#/c/21728/35//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21728/35//COMMIT_MSG@15 PS35, Line 15: then both jwt and oauth a > could you add a test case with both jwt and oauth enabled? This would be tricky since in tests we do not have either ldap or kerberos enabled. http://gerrit.cloudera.org:8080/#/c/21728/33/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21728/33/be/src/rpc/authentication.cc@1679 PS33, Line 1679: bool use_oauth = FLAGS_oauth_token_auth; > Do we allow oauth to be used without TLS? Only in the tests. 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; > Oh, good point. There would be a decrement method if the metric type was s 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); > Please see comment on THttpServer.cpp These are counter metric creation. 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 > Nit: rename to something more generic (such as `bearer_token`) to avoid con Done http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc@706 PS35, Line 706: } > I don't like logging a jwt auth failure automatically if there is a potenti Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@26 PS35, Line 26: > Please create a class hierarchy with a single base class (such as TestImpal Moved classes to separate files. Jwt/Oauth cannot work together in test. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@47 PS35, Line 47: "-jwt_token_auth=true -jwt_allow_without_tls=true " > Nit: The above two lines need to be indented so they line up with line 45. Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@51 PS35, Line 51: HS2_HTTP_CONNS = "impala.thrift-server.hiveserver2-http-frontend.total-connections" > Nit: The above two lines need to be indented so they line up with line 45. Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@68 PS35, Line 68: disable_log_buffering=True, > If setting up the test class hierarchy, these existing tests should not hav Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@291 PS35, Line 291: > Nit: an Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@300 PS35, Line 300: > Nit: OAuth Done http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@309 PS35, Line 309: > Nit: OAuth Done -- 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: 37 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: Mon, 13 Jan 2025 23:41:17 +0000 Gerrit-HasComments: Yes
