Add proxy support #164
Add proxy support #164
Conversation
hyper/http11/connection.py
Outdated
| if not self.proxy_host: | ||
| host = self.host | ||
| else: | ||
| port = self.proxy_host |
There was a problem hiding this comment.
This is not quite right: you assign only host in the top block and only port in the bottom block. Need to do both in both. =)
|
Yup, this is definitely on the right track! I've left some notes inline, but I'm excited to see this work progress! |
…rly, SSL connections for proxy ignored for now
|
I am fixing this test case and adding one for http2, but before I go further do you agree with this test as it is structured? |
|
I think the test is overcomplicated. I don't think we need to run something that amounts to an actual proxy: we just need to run something that knows it is pretending to be a proxy. What we really want to test is that, when we set up proxy values, we connect to the right place and then that we format the data correctly. This doesn't require a (fragile) socket proxy, just requires the "server" side of the connection to look for some slightly different stuff. |
|
Replaced it with a simpler test case. Think everything should be ready now. Take a look. |
hyper/http11/connection.py
Outdated
| else: | ||
| self.proxy_host, self.proxy_port = proxy_host, proxy_port | ||
| else: | ||
| self.proxy_host = None |
There was a problem hiding this comment.
Can we None out self.proxy_port here as well? I hate having attributes that only sometimes exist.
|
Alright @fredthomsen, this is looking really great! A few small comments inline, but we're basically there! Please also add yourself to the |
|
Made these changes; however, looks like some strange build issues relating to DNS and SSL hit the CI process. Force a re-run? |
|
Runs clean @fredthomsen! Let me do one last review. |
| if proxy_host: | ||
| if proxy_port is None: | ||
| try: | ||
| self.proxy_host, self.proxy_port = proxy_host.split(':') |
There was a problem hiding this comment.
This logic could go wrong with IPv6 addresses. I think we might need to pull this out into a helper function, because the logic is a bit tricky, but we can run on some basic heuristics: if the host ends with square bracket it doesn't contain a port. Otherwise, change the split into rsplit only allowing one split. That should work in more cases.
There was a problem hiding this comment.
Agreed this won't work with IPv6 addresses; however, the rest of the codebase or at least the http11 and http20 constructors have similar logic when just parsing the host and port. Should these all be updated as part of this effort and test cases added as well?
There was a problem hiding this comment.
Hmm, if everything else is wrong too then I'd skip it for now. Let's open a GitHub issue to track it instead: see #170.
|
Sorry @fredthomsen, a few really small notes. Shout if any point you no longer want to keep polishing this and I can do the rest of it. |
|
No worries. I am very particular as well. Don't mind doing it at all. |
|
Alright, beautiful, I'm happy with this. Thanks so much @fredthomsen! |
2 participants
Just making sure I am on the right track. Test cases are coming.