Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial start on HTTP tunnel proxies #223

Closed
wants to merge 3 commits into from
Closed

Initial start on HTTP tunnel proxies #223

wants to merge 3 commits into from

Conversation

sethmlarson
Copy link
Contributor

@sethmlarson sethmlarson commented Aug 17, 2019

Closes #201. This is my initial stab at implement HTTP tunnel proxy as a Dispatcher. Will need to implement the proxies argument for the Client to match Requests.

I'm wondering what the best way to test this implementation would be.

@@ -178,7 +178,8 @@ def merge_headers(
)
except HTTPError as exc:
# Add the original request to any HTTPError
exc.request = request
if exc.request is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to not replace the request property on a ProxyError which is different than the original request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be OK with freezing that bit of information as a code comment. :-)

httpx/concurrency.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

Crackin' stuff!
Not a detailed review, but some initial thoughts:

  • start_tls doesn't appear to be getting called into anywhere yet - where will that end up being plugged in?
  • It's not clear to me on first pass if it makes more sense to have the make-the-connection implementation take effect on the Dispatcher class, or if it'd actually be cleaner to have the "Make a CONNECT request" be implemented within an alternate HTTPConnection.
  • If possible we ought to prefer composing implementations rather than inheriting them, since we're being a bit fuzzy about reaching into details of the subclass that we might prefer to be able to treat as private API. (Ie. in this cases that'd mean having a dispatcher class which contains an instance of ConnectionPool rather than inheriting from it.)

It'll probably be easier to fully review once start_tls, and the actually proxy configuration via-the-client stuff is all plumbed in.

@sethmlarson sethmlarson changed the title Initial start on HTTP tunnel proxies Initial start on HTTP tunnel proxies [WIP] Aug 19, 2019
@sethmlarson sethmlarson changed the title Initial start on HTTP tunnel proxies [WIP] Initial start on HTTP tunnel proxies Aug 26, 2019
@tomchristie
Copy link
Member

What's the proposed API for this once it's linked up to the Client?

@sethmlarson
Copy link
Contributor Author

After I implement the two separate proxy types (forward and tunnel) the interface for clients will be the same as in Requests:

proxies={"http": "http://proxy"} or proxies={"http://google.com": "http://proxy"}

or if you need more control:

proxies={"http": httpx.HTTPProxy("http://proxy", proxy_headers={"Proxy-Authorization": "...")}

The default HTTPProxy is a hybrid between these two where HTTP requests are sent via forwarding and HTTPS requests are sent via tunneling. Maybe I can merge the implementation of these two separate proxies into one an use arguments to control behavior like httpx.HTTPProxy(..., proxy_mode=httpx.ProxyMode.ALWAYS_TUNNEL) or the like.

@tomchristie
Copy link
Member

Maybe I can merge the implementation of these two separate proxies into one

From an API point of view I think that having just HTTPProxy(...) and ConnectionPool(...) as our documented dispatcher classes is nicer than having two different class implementations for proxying yeah. (With a mode control available if neccessary?)

@sethmlarson
Copy link
Contributor Author

You're right, I'll merge the two. What are your thoughts on getting the two separate implementations committed and then merging along with wiring up the Client as it's own PR?

Makes it easier to test the behavior of the specific proxy modes.

@tomchristie
Copy link
Member

What are your thoughts on getting the two separate implementations committed and then merging along with wiring up the Client as it's own PR?

Probably would be okay with that since the dispatcher interface is documented already, yeah.

We should probably start exanding out the "Advanced" section of the docs tho.

@sethmlarson
Copy link
Contributor Author

Closed in favor of #259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement HTTP Tunnel Proxy
3 participants