Allow users to add custom http headers when using hs2-http (#557) #558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
csringhofer
reviewed
Nov 19, 2024
impala/tests/test_http_connect.py
Outdated
| # Save the http headers from the message in a class variable. | ||
| RequestHandlerProxy.saved_headers = self.decode_raw_headers() | ||
| # Forward the http post message to Impala and get a response message. | ||
| response = requests.post(url="http://localhost:28000/cliservice", |
There was a problem hiding this comment.
The port should be configurable with env var IMPYLA_TEST_HTTP_PORT
impala/tests/test_http_connect.py
Outdated
|
|
||
| def decode_raw_headers(self): | ||
| """Decode a list of header strings into a list of tuples, each tuple containing a | ||
| key-value pair. The details of how to get the headers are differs between Python2 |
There was a problem hiding this comment.
nit: headers are differs -> are different/differs?
impala/_thrift_api.py
Outdated
| # Update HTTP headers based on the saved cookies and auth mechanism. | ||
| # Set the user-defined callback function which adds custom HTTP headers to outgoing | ||
| # messages. | ||
| def setGetUserDefinedCustomHeadersFunc(self, func): |
There was a problem hiding this comment.
Is this used anywhere?
Also, couldn't the test use this function to set the callback so that passing get_user_custom_headers_func through constructors can be avoided?
There was a problem hiding this comment.
Not used and removed.
I agree that it is ugly to have to change the constructors. But the ImpalaHttpClient is hidden away so it is hard to call methods on it, especially before connect() is called. And connect() sends messages which we might want to know about.
I welcome suggestions for better structure.
) In a modern Impala deployment hs2-http protocol is used in a system where http messages pass through one or more http proxies. Some of these proxies add their own http message headers to messages as they are forwarded. It would be useful to test Impala with some of the message headers that are added by http proxies. In particular the case where there are multiple http headers with the same name is hard to simulate with clients such as Impyla or Impala Shell. This is partly because these clients store http headers in a Python dict which does not allow duplicate keys. Extend the Impyla connect() method to add a 'get_user_custom_headers_func' parameter. This specifies a function that is called as http message headers are being written. The function should return a list of tuples, each tuple containing a key-value pair. This allows duplicate headers to be set on outgoing messages. TESTING Add test code which implements a reverse http proxy, which allows test code to access the outgoing http message headers generated by Impyla. Add a test using this proxy which validates the new feature. The new test code requires a new python package 'requests'. I think there is not away to add this requirement automatically so I added a note to README.md All tests pass on Python2 and Python3. Fix TestHS2FaultInjection to use setup_method() and teardown_method() so as to work in Python3
bartash
force-pushed
the
asherman_sketch_user_settable_headers__12
branch
from
1141dde to
14b7ec6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a modern Impala deployment hs2-http protocol is used in a system where http messages pass through one or more http proxies. Some of these proxies add their own http message headers to messages as they are forwarded. It would be useful to test Impala with some of the message headers that are added by http proxies. In particular the case where there are multiple http headers with the same name is hard to simulate with clients such as Impyla or Impala Shell. This is partly because these clients store http headers in a Python dict which does not allow duplicate keys.
Extend the Impyla connect() method to add
a 'get_user_custom_headers_func' parameter. This specifies a function that is called as http message headers are being written. The function should return a list of tuples, each tuple containing a key-value pair. This allows duplicate headers to be set on outgoing messages.
TESTING
Add test code which implements a reverse http proxy, which allows test code to access the outgoing http message headers generated by Impyla. Add a test using this proxy which validates the new feature.
The new test code requires a new python package 'requests'. I think there is not away to add this requirement automatically so I added a note to README.md
All tests pass on Python2 and Python3.
Fix TestHS2FaultInjection to use setup_method() and teardown_method() so as to work in Python3