Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22424 )
Change subject: IMPALA-13675: OAuth AuthN Support for Impala Shell ...................................................................... Patch Set 18: (14 comments) http://gerrit.cloudera.org:8080/#/c/22424/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22424/16//COMMIT_MSG@9 PS16, Line 9: This patch adds the support to fetch access tokens > Added: It covers the flow: client_credentials. Done http://gerrit.cloudera.org:8080/#/c/22424/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22424/18//COMMIT_MSG@22 PS18, Line 22: unit tests Nit: these are custom cluster tests, not unit tests http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1010 PS16, Line 1010: : > Done Done http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1028 PS16, Line 1028: print("Error getting OAuth access tokens", e) > Done Done http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1030 PS16, Line 1030: finally: > Added sys.exit(1) with the exception info. That should be enough to debug/d Done http://gerrit.cloudera.org:8080/#/c/22424/18/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22424/18/shell/impala_shell.py@222 PS18, Line 222: self.oauth_endpoint = options.oauth_endpoint duplicate of line 219 http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@26 PS16, Line 26: > Done Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@197 PS16, Line 197: rror" in res > Changed to OAUTH_IMPALAD_ARGS Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@199 PS16, Line 199: assert "Not connected to Impala, could not execute queries." in result.stderr > changed to oauth_auth_success. Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@207 PS16, Line 207: size=1) > Yes. However, this is just a place holder and its not verified since we do In that case, is the "--user" flag needed at all? http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@219 PS16, Line 219: -q' > Done Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@242 PS16, Line 242: impala_log_dir="{oauth_auth_success}", > Updated. Done http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py@243 PS18, Line 243: tmp_dir_placeholders=["oauth_auth_success"], Placeholders should be renamed to something like oauth_auth_failure http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py@261 PS18, Line 261: self._stop_impala_cluster() This test is not asserting anything. It needs to check the failure message and also that the login attempt never happened. -- To view, visit http://gerrit.cloudera.org:8080/22424 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84e26d54f6a53696660728efb239ffd43de4c55d Gerrit-Change-Number: 22424 Gerrit-PatchSet: 18 Gerrit-Owner: gaurav singh <gsi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: gaurav singh <gsi...@cloudera.com> Gerrit-Comment-Date: Tue, 08 Apr 2025 20:30:00 +0000 Gerrit-HasComments: Yes
