Set fake host header for local communication.

[Random-Liu]

For issue #189.

In this PR, I set URL.Host to a fake address "docker.sock" to solve #189.

Hope this could be in, thanks! :)

@duglin @vdemeester

[duglin]

Would it be possible to add a testcase for this to make sure we don't change it (or remove it) by accident?
Aside from that it looks good.

[Random-Liu]

@duglin Sure, will do that soon. Thanks!

[Random-Liu]

@duglin Addressed comments.

[duglin]

Even though the PR is focused on the "" case, let's make the testcase cover the "normal" host cases too. Can you please add a test variant where test.expectedHost isn't ""?

[Random-Liu]

I think the test.expectedHost is always "" now.

Now we just pass the apiPath (something without schema like /container/id/start) into http.NewRequest. (See here)

While http.NewRequest only sets request.Host when the passed in url has Schema. (See here, here and here).

[Random-Liu]

Oh, wait, let me see.

[duglin]

wouldn't using "tcp://localhost:4243" do it?

[Random-Liu]

No. The request.Host is still "" with "tcp://localhost:4243".

[duglin]

oh I also added:

req.Host = host

around like 92 in request.go

[vdemeester]

oh yep, I think you're right @duglin it would be better to use NewClient everywhere.

• renaming newMockClient to newMockTransportClient (or newMockTransport…) as it's more what it does.
• probably creating a newMockClient that does call NewClient, …

func newMockClient(tlsConfig *tls.Config, doer func(*http.Request) (*http.Response, error)) Client {
        client, _ := NewClient(test.host, "", nil, nil)

        client.transport = newMockTransport(tlsConfig, doer)

        return client
}

I would even think we should check the error on NewClient.

[Random-Liu]

@duglin, I think the main reason is that you added req.Host = host in request.go, there is nothing to do with the NewClient.

The req.Host is initialized in http.NewRequest before, and mostly it will be set to "" except the bug I mentioned above.

Now you manually added req.Host = host, of course it will be equal to req.Host.URL.

/cc @vdemeester

[duglin]

Something feels wrong here. According to this test the HTTP Host header will always be empty, which doesn't seem right to me. If it really is supposed to be blank then why include the string in the test data, just check for a static "" in the if-statement. However, having an empty header is not correct and we should fix that.

I'll try to find some time to look at this but kind of busy today....

[Random-Liu]

@duglin Yeah, you are right. If it is supposed to be blank, we should just check it with a static "". There is indeed something wrong here, wait for your result. :)

[Random-Liu]

@duglin @vdemeester Ping~ :P

[vdemeester]

LGTM 🐶

[MHBauer]

shouldn't this set req.Host? We override the header (the thing that causes the problem on the server) only in the case that we need to talk to a Host that would cause a problem. The request still needs to go to the right place.

[Random-Liu]

@MHBauer Make sense to me, will update soon.

[Random-Liu]

@MHBauer Addressed comments.

[MHBauer]

@Random-Liu thanks for the changes. I appreciate the comments as well. If I don't make sense call me on it.

I think the three way tests were better.
My understanding is the actual request will look different from the Host header in the case of sockets. We should check that.

@vdemeester @duglin Am I on to something or misinterpreting?

EDITED TO DELETE THE WRONG STUFF

[Random-Liu]

@MHBauer Unfortunately, request.Host is always "" now. With the current implementation and test case, if we print out the request.Host and request.URL.Host, the result is:

docker
/var/run/docker.sock

docker
//./pipe/docker_engine

                                    <-- The host is empty
0.0.0.0:4243

The reason is that the url we pass to http.NewRequest has no host, it's just something like /container/id/start see

[MHBauer]

Yea, I think that's an artifact of using the mock client. The request never actually gets to the golang write() method which you linked in the original issue #189. Thus the req.URL.Host -> req.Host transformation never actually happens in the testing scenario.

The change feels correct to me, but I'm not sure the test is worthwhile if it doesn't connect to an actual server and go through the whole end to end integration scenario.

[MHBauer]

Understanding it further, I would expect it to be:

        {
            "unix:///var/run/docker.sock",
            "docker",
            "/var/run/docker.sock",
        },
        {
            "npipe:////./pipe/docker_engine",
            "docker",
            "//./pipe/docker_engine",
        },
        {
            "tcp://0.0.0.0:4243",
            "",
            "0.0.0.0:4243",
        },
        {
            "tcp://localhost:4243",
            "",
            "localhost:4243",
        },
    }

[MHBauer]

LGTM for this as it is, based on my understanding.

[Random-Liu]

@MHBauer Addressed comments.

[duglin]

I agree with @MHBauer about not using a real server for the testing. It feels like we're debugging/testing the testing code and not docker. Did anyone consider using a real http server in the tests like we do here: https://github.com/docker/docker/blob/master/integration-cli/docker_cli_config_test.go#L26 ?

[calavera]

@duglin if the server part ended up in this repo it would be really easy to use as a test harness. We'd only need to provide test backends and we'd get end to end tests.

This change LGTM. I'm going to merge it as it is, but we can keep the discussion going in an issue.

[MHBauer]

Thank you for the contribution @Random-Liu!

[Random-Liu]

@MHBauer Thanks for your review and good suggestions. :)