Deprecate passing DEFAULT proxy mode to HTTPCore

[yeraydiazdiaz]

Prompted by encode/httpcore#65 (comment)

I'd suggest that a good tack onto getting there would be to start with a PR to httpx that no longer uses the DEFAULT mode anywhere. That'd help us thrash out & review any implementation changes required in httpx, without actually changing any API in httpcore or httpx.

The DEFAULT proxy mode is used to signal httpcore's proxy connection pools to use forwarding or tunnelling based on the request URL.

However, Client.dispatcher_for_url uses Client.proxies to pick a dispatcher based on the request URL, at which point HTTPX is able to choose between one proxy mode or the other. The only case where we want to delay the decision to httpcore is when the user has specified a single proxy string or URL or an all-prefixed proxy config key in the proxies dict effectively indicating both the proxy should be used for both HTTP and HTTPS.

This PR removes the use of all-prefixed proxy config keys in Client.proxies expanding them into http and https keys and mapping them to separate Proxy instances with FORWARD_ONLY and TUNNEL_ONLY modes respectively.

[yeraydiazdiaz]

😞 I'm getting different linting results locally than in CI.

[tomchristie]

Okay. Most likely due to different package versions. I’d drop your virtualenv and reinstall. Really we ought to be pining our deps.

[yeraydiazdiaz]

I've rebuilt and it's now consistent, the problem is black and isort don't agree with each other.

Maybe we should remove the isort check?

[cansarigol]

Hey, @yeraydiazdiaz nice job! I was wondering if you deliberately left the mode default value of Proxy as 'DEFAULT' to show the users who use Proxy class without mode?

[yeraydiazdiaz]

I wasn't sure at the time so and was hoping to discuss it in this PR but forgot to leave a TODO comment or something so thanks for bringing it up.

Thinking about it now I'm leaning towards leaving it because Proxy is part of the public API and the mode is part of it. In my mind we expose Proxy for advanced users that understand the implications of using one mode or the other so the warning seems like the right choice to nudge them into setting one.

That being said this PR will pass the Proxy as is, without changing its mode, to httpcore which was the point of removing it in the first place, so maybe we should prevent that by internally change the mode?

[yeraydiazdiaz]

Actually, I think a better flow would be to not warn at all and change the semantics of the proxy mode: DEFAULT is a valid value for Proxy alone and means "I don't care, do what you think it's best". In which case we change the mode in the proxy_map as required.

That way we keep the Proxy mode separate from proxy dispatch's mode and don't force the choice on users who might need to use a Proxy instance to set proxy headers but don't know or care about forward vs. tunnelling.

[cansarigol]

I agree with you. I thought that a None option can be included. That way we can warn the users who use default mode explicitly. Others can be changed silently.

diff --git a/httpx/_config.py b/httpx/_config.py
index d8c8651..d0af1cb 100644
--- a/httpx/_config.py
+++ b/httpx/_config.py
@@ -317,14 +317,14 @@ class PoolLimits:
 
 class Proxy:
     def __init__(
-        self, url: URLTypes, *, headers: HeaderTypes = None, mode: str = "DEFAULT",
+        self, url: URLTypes, *, headers: HeaderTypes = None, mode: str = None,
     ):
         url = URL(url)
         headers = Headers(headers)
 
         if url.scheme not in ("http", "https"):
             raise ValueError(f"Unknown scheme for proxy URL {url!r}")
-        if mode not in ("DEFAULT", "FORWARD_ONLY", "TUNNEL_ONLY"):
+        if mode is not None and mode not in ("DEFAULT", "FORWARD_ONLY", "TUNNEL_ONLY"):
             raise ValueError(f"Unknown proxy mode {mode!r}")
 
         if url.username or url.password:
@@ -343,7 +343,7 @@ class Proxy:
                 "DEFAULT proxy mode is deprecated, please use "
                 "TUNNEL_ONLY or FORWARD_ONLY."
             )
-        self.mode = mode
+        self.mode = mode or "DEFAULT"
 
     def build_auth_header(self, username: str, password: str) -> str:
         userpass = (username.encode("utf-8"), password.encode("utf-8"))

[tomchristie]

If we are going to go this route, then something that might? make sense at the httpcore level would be to expose the proxying functionality as AsyncTunnelProxy and AsyncForwardProxy classes, rather than a two-way switchable mode on a single class.

[yeraydiazdiaz]

Yup, that'd be good. It would be a breaking change in the API though, shall we defer to a later PR? Or are we confident enough on the change to do it now eyeing 0.13?

[tomchristie]

Milestoning for 1.0 discussion.

I feel like this could probably use a bit of "from the top" talk through of what the motivation is here. Since we've worked through things a bit now I guess the synopsis at this point would be...

• Finalize this PR, so we're either mounting a tunnel or a forward proxy in each case, never a single proxy in "default" mode.
• Switch to httpcore.AsyncForwardProxy(...) and httpcore.AsyncTunnelProxy(...), rather than having "mode=..." in httpcore.

I guess I can see the value in that from the perspective of keeping the httpcore API/behaviour really minimal & explicit.

Just to get my bearing a bit here...

• Does it ever make sense to use forward mode on https, or tunnel mode on http?
• How would we most succinctly describe the motivation in this change, and do we think it's necessary?

[yeraydiazdiaz]

How would we most succinctly describe the motivation in this change, and do we think it's necessary?

To me there's two motivations:

1. Being safe by default, following our warning in the proxying docs: "To make sure that proxies cannot read your traffic, and even if the proxy_url uses HTTPS, it is recommended to use HTTPS and tunnel requests if possible."
2. Simplifying httpcore by removing the logic behind choosing one mode or the other to httpx.

Does it ever make sense to use forward mode on https, or tunnel mode on http?

I don't have a ton of experience in this area but I think it's about allowing the choice but being safe by default when people don't know/care. From my limited experience it seems most proxies implement both tunneling and forwarding.

[tomchristie]

I think we probably want to punt this out of 0.13, since I think this design decision impacts on a bunch of other stuff that I'd like us to work through super carefully first. For example, it makes the httpcore proxy transport asymmetrical with the urllib3 proxy transport.

We might even want to talk through the Transport API with the hip folks too, before coming to a final 1.0 decision on this.

[yeraydiazdiaz]

Sure thing, moving to 1.0.

[tomchristie]

Okay this was worth investigating, but at this point I'm going to suggest that we consider punting on this?

I think it's beneficial that httpx.Proxy(...) ends up mapping onto a single transport instance, since it helps developers build a more consistent internal idea of how the Client is like to work. If we did want separate TunnelProxy and ForwardProxy transport classes, then I'd probably also want to advocate for dropping the "DEFAULT" and enforcing an explicit style, like {"http": httpx.Proxy(.... TUNNEL), "https": httpx.Proxy(..., FORWARC)}, which I don't think is a usability win.

What do we think?

[florimondmanca]

Never been 100% sure I fully groked the debate about dropping DEFAULT proxy mode, to be honest 😆 But…

It looks like the original motivation in encode/httpcore#65 was about "does choosing between FORWARD/TUNNEL belong to HTTPCore of high-level clients?".

• In my mind it's not horribly off that HTTPCore would handle this particular decision, since it's still at a "how do we transport information to the server" level.
• Looks like there are definite cons in terms of implementation on the high-level client side.

What I do think though, regardless of this (or maybe as a pre-requisite?) is that our documentation on tunnel vs forward is not super clear right now. I think we ought to have a dedicated "FORWARD vs TUNNEL" section, which clearly explains what each approach does, and then provides key insights for the user to decide between both. (Ideally they shouldn't have to choose anything because the defaults would be sensible and secure enough?)