docker-py breaks with some configurations of Requests v2.12.2

[Lukasa]

Reported by @graingert.

The patch filed by @tiran in #3713 seems to have broken docker-py. They're using a custom URL scheme (http+docker), which we previously applied URL preparation to but now do not. This has therefore busted what they were up to in v2.12.2.

I'm not immediately sure that we have a good way out of this. Prior to this patch docker-py users were probably at risk of encountering issues because the HTTP requests to the Docker API can be routed across unix domain sockets, which may entirely fail to contain hostnames and also fail to IDNA-encode. So I'm not sure that we don't need docker-py to route around this a different way.

[Lukasa]

Corresponding docker-py issue is docker/docker-py#1321.

[Lukasa]

What is unclear to me is exactly why this skip is happening. @graingert, if you can run your example under pdb can you set a breakpoint at requests.sessions.Session.request and run print url when it breaks?

[graingert]

@Lukasa can do

[graingert]

@Lukasa it's 'http+docker://localunixsocket/v1.24/images/create' but that gets handled by the mounted docker.transport.unixconn.UnixAdapter

[Lukasa]

Heh, nevermind, I think I know what this is.

The only difference between the two Requests versions is this. In 2.12.1:

Python 2.7.12 (default, Oct 13 2016, 13:16:42) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.35)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> r = requests.Request('GET', 'http+docker://localunixsocket/v1.24/images/create')
>>> p = r.prepare()
>>> p.url
'http+docker://localunixsocket/v1.24/images/create'

In 2.12.2

Python 2.7.12 (default, Oct 13 2016, 13:16:42) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.35)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> r = requests.Request('GET', 'http+docker://localunixsocket/v1.24/images/create')
>>> p = r.prepare()
>>> p.url
u'http+docker://localunixsocket/v1.24/images/create'

Specifically, the issue is that when Requests processes the URL it ends up as a bytestring. When it does not, it ends up as whatever type was passed in: in the case of Python 3, unicode. That then trips something up lower down in the stack (probably in the docker-py adapter) which expects only bytes there.

[Lukasa]

Yup, the criminal is http.client: the auto-adding of Host headers is only done if url.startswith('http'), which in Python 3 is not true for bytes URLs. This is a pretty dumb failure mode.

[nateprewitt]

Just as a little background here, #3713 was raised because unix sockets were hitting idna. The bypass (#3695) had addressed (at least from my tests) the socket path issues already. I didn't think it was worth noting because it seemed #3713 was providing some helpful constraint tightening, beyond fixing the immediate idna issue. As far as I can tell, avoiding that tightened bailout point would likely fix this issue.

[Lukasa]

So, Requests can potentially address this with a feature: we could have a registry of schemes that we should just treat like HTTP (that is, we should assume that all our URL-processing logic can handle). That would allow docker-py to register "http+docker" as something that makes sense.

However, given that docker-py are already lying to us by setting a hostname (hi there localunixsocket), why don't docker-py just use the URL http://localunixsocket and mount their adapter at that full-string?

[graingert]

@Lukasa someone might come along and register the gtld localunixsocket

[Lukasa]

@graingert That's fine, it won't matter. The adapter is selected based on a longest-prefix match, so the docker-py adapter will still be selected and knows it doesn't have to do a DNS lookup.

[Lukasa]

Ok, so here's the thing: now that we're not processing the URL everything is off the table. Query parameters for example, are not handled any longer. This also probably breaks some docker-py stuff.

[Lukasa]

@graingert has pointed out that in situations like this we should probably throw an exception if we were supposed to add any query parameters to the request.

[graingert]

Yup, the criminal is http.client: the auto-adding of Host headers is only done if url.startswith('http'), which in Python 3 is not true for bytes URLs. This is a pretty dumb failure mode.

It's not the missing Host header, the request still works fine without it. (currently it's set to 'Host: localhost' which doesn't mean anything to the docker process)

The issue is the params processing of the URL.

[Lukasa]

Yeah, so once again, docker-py can get out of this by changing the way they handle their adapters. However, I'm not sure Requests should be processing schemes it doesn't understand, and our previous escape hatch wasn't great.

[graingert]

@Lukasa should this handling be moved to the adaptor?

[Lukasa]

Which handling?

[graingert]

*URL processing

[Lukasa]

No, I don't think so. I think it's right where it is. We can discuss whether there is a sensible feature addition that allows for Requests adapters to register scheme-specific URL processing logic, but that can't be shoved in for a little while and has to clear quite a high bar.

[Lukasa]

Closing in favour of #3735.