Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21780 )
Change subject: IMPALA-13312: Use client address from X-Forwarded-For Header in Ranger Audit Logs ...................................................................... Patch Set 6: Code-Review+2 (8 comments) Thanks for addressing my previous comments Abhishek! I only left 2 very minor comments in test_ranger.py. I don't have any other suggestion. http://gerrit.cloudera.org:8080/#/c/21780/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21780/3//COMMIT_MSG@15 PS3, Line 15: if the deployment environment protects against spoofing. : : Also, added a new function 'GetXFFOriginClientAddress' for parsing XFF : header with comma separated IP addresses, whi > Updated commit message mainly inspired from IMPALA-13310 Ack http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-util.h File be/src/rpc/authentication-util.h: http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-util.h@56 PS3, Line 56: Status GetXFFOriginClientAddress(const std::string_view& xff_addresses, : std::string& origin); > Thanks for pointing this out. It appears there are gaps in code and most of Thanks for the pointer! http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@117 PS3, Line 117: '--hs2_x_forw > Maybe not since it's treated as an empty header which is similar to not inc Thanks! I was just confused by this code comment ("# Invalid XFF headers") in the patch set 3. Since it has been removed in the patch set 6, I don't have any other suggestion. http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@135 PS3, Line 135: '"resType":"@table",' : '"action":"select",' : '"result":1,' : '"agent":"impala",' : r'"policy":\d,' : '"enforcer":"ranger-acl",' : '"cliIP":"%s",' : '"reqData":"%s",' : '".+":".+","logType":"RangerAudit"' : ) : > Added regex. Ack http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@135 PS3, Line 135: '"resType":"@table",' : '"action":"select",' : '"result":1,' : '"agent":"impala",' : r'"policy":\d,' : '"enforcer":"ranger-acl",' : '"cliIP":"%s",' : '"reqData":"%s",' : '".+":".+","logType":"RangerAudit"' : ) : : # Ensure audit logs were logged in coordinator logs : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("10.20.30.40", r"select count\(1\) from functional.alltypes"), : expected_count=3) : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("127.0.0.1", r"select count\(2\) from functional.alltypes"), expected_count=2) : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("foobar", r"select count\(3\) from functional.alltypes"), expected_count=1) : : @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args( : impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS) : def test_grant_revoke_with_catalog_v1(self, unique_name): : """Tests grant/revoke with catalog v1.""" : self._test_grant_revoke(unique_name, [None, "invalidate metadata", : "refresh authorization"]) : : @pytest.mark.execute_serially : @SkipIfFS.hdfs_acls : @CustomClusterTestSuite.with_args( : impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS) : def test_insert_with_catalog_v1(self, unique_name): : """ : Test that when Ranger is the authorization provider in the legacy catalog mode, : Impala does not throw an AnalysisException when an authorized user tries to execute : an INSERT query against a partitioned table of which the respective table path and : the partition path are not writable according to HDFS permission. : """ : user = getuser() : admin_client = self.create_impala_client() : unique_database = unique_name + "_db" : unique_table = unique_name + "_tbl" : partition_column = "year" : partition_value = "2008" : table_path = "test-warehouse/{0}.db/{1}".format(unique_database, unique_table) : table_partition_path = "{0}/{1}={2}"\ : .format(table_path, partition_column, partition_value) : insert_statement = "insert into {0}.{1} (name) partition ({2}) " \ : "values (\"Adam\", {3})".format(unique_database, unique_table, partition_column, : partition_value) : authz_err = "AuthorizationException: User '{0}' does not have privileges to " \ : "execute 'INSERT' on: {1}.{2}".format(user, unique_database, unique_table) : try: : admin_client.execute("drop database if exists {0} cascade" : .format(unique_database), user=ADMIN) : admin_client.execute("create database {0}".format(unique_database), user=ADMIN) : admin_client.execute("create table {0}.{1} (name string) partitioned by ({2} int)" : .format(unique_database, unique_table, partition_column), user=ADMIN) : admin_client.execute("alter table {0}.{1} add partition ({2}={3})" : .format(unique_database, unique_table, partition_column, partition_value), : user=ADMIN) : : # Change the owner user and group of the HDFS paths corresponding to the table and : # the partition so that according to Impala's FsPermissionChecker, the table is not : # > I added regex based on other review comment. Please take a look if the late Ack http://gerrit.cloudera.org:8080/#/c/21780/6/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/21780/6/tests/authorization/test_ranger.py@131 PS6, Line 131: s nit: Is my understanding correct that in patch set 6, the following becomes a single string (the sub-strings of different fields are implicitly concatenated)? If so, it may be good that we remove this "s" in the end. :-) http://gerrit.cloudera.org:8080/#/c/21780/6/tests/authorization/test_ranger.py@132 PS6, Line 132: s nit: Same as above. It may be clearer to change the name of the variable to 'expected_string_valid_xff' (to emphasize it's a single string)? http://gerrit.cloudera.org:8080/#/c/21780/6/tests/authorization/test_ranger.py@147 PS6, Line 147: assert_impalad_log_contains This is just a note for myself. Nothing has to be changed. :-) It looks like assert_log_file() used in patch set 3 were more flexible in that it tried to make sure there exists a log line that contains each field added in the list, whereas assert_impalad_log_contains() here in patch set 6 makes sure there exists a log line that contains the single line given as a whole. In the future, when the schema of a Ranger audit event changes, e.g., the relative order of fields changes, we may have to update the test here. Anyway, the current patch looks good. We don't have to change this part. -- To view, visit http://gerrit.cloudera.org:8080/21780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib784ad805c649e9576ef34f125509c904b7773ab Gerrit-Change-Number: 21780 Gerrit-PatchSet: 6 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Thu, 26 Sep 2024 18:25:29 +0000 Gerrit-HasComments: Yes
