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 37: (9 comments) http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1888 PS37, Line 1888: sys.exit(1) Nit: insert blank line below this line http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1889 PS37, Line 1889: through a file Nit: through a command http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1891 PS37, Line 1891: self.oauth_client_secret = getpass.getpass("Enter OAuth Client Secret: ") Nit: insert blank line below this line http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1919 PS37, Line 1919: if "access_token" in json_body.keys(): Need an "else" clause on this "if" statement to return a meaningful error if "access_token" is not in the response body. This ensures the user is actually using OAuth instead of potentially falling back to another authentication mechanism. http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1920 PS37, Line 1920: self.oauth = json_body["access_token"] What happens when this token expires? Will a new token be obtained? If not, please open a new Jira to add in token refresh support. http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@2250 PS37, Line 2250: print("Please specify at most one authentication mechanism (-k, -l, -j or -a)", Nit, please use the Oxford Comma syntax and add a comma after -j. http://gerrit.cloudera.org:8080/#/c/22424/37/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/37/tests/custom_cluster/test_shell_jwt_auth.py@206 PS37, Line 206: @pytest.mark.execute_serially Should these new OAuth tests go in the test_shell_oauth_auth.py file? http://gerrit.cloudera.org:8080/#/c/22424/37/tests/custom_cluster/test_shell_jwt_auth.py@276 PS37, Line 276: r'connected_user \(string\) = "test-user"', expected_count=0) Need to add an assert on the Impala OAuth success/fail metrics to ensure neither has been incremented. The reason is that a missing "access_token" in the OAuth reply should be caught in the Impala shell before it ever makes a call to the Impala server. http://gerrit.cloudera.org:8080/#/c/22424/37/tests/shell/test_shell_commandline_jwt_auth.py File tests/shell/test_shell_commandline_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/37/tests/shell/test_shell_commandline_jwt_auth.py@81 PS37, Line 81: assert "Please specify at most one authentication mechanism (-k, -l, -j or -a)" \ Will need to update all of these asserts after adding the comma after -j. -- 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: 37 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Wed, 28 May 2025 20:21:13 +0000 Gerrit-HasComments: Yes
