Skip to content
This repository was archived by the owner on Nov 23, 2019. It is now read-only.

Set fake host header for local communication. #190

Merged
merged 1 commit into from
Apr 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions client/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ func (cli *Client) sendClientRequest(ctx context.Context, method, path string, q
}

req, err := cli.newRequest(method, path, query, body, headers)
if cli.proto == "unix" || cli.proto == "npipe" {
// For local communications, it doesn't matter what the host is. We just
// need a valid and meaningful host name. (See #189)
req.Host = "docker"
}
req.URL.Host = cli.addr
req.URL.Scheme = cli.transport.Scheme()

Expand Down
77 changes: 77 additions & 0 deletions client/request_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package client

import (
"bytes"
"fmt"
"io/ioutil"
"net/http"
"strings"
"testing"

"golang.org/x/net/context"
)

// TestSetHostHeader should set fake host for local communications, set real host
// for normal communications.
func TestSetHostHeader(t *testing.T) {
testURL := "/test"
testCases := []struct {
host string
expectedHost string
expectedURLHost string
}{
{
"unix:///var/run/docker.sock",
"docker",
"/var/run/docker.sock",
},
{
"npipe:////./pipe/docker_engine",
"docker",
"//./pipe/docker_engine",
},
{
"tcp://0.0.0.0:4243",
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, wait, let me see.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I also added:

req.Host = host

around like 92 in request.go

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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. :)

"0.0.0.0:4243",
},
{
"tcp://localhost:4243",
"",
"localhost:4243",
},
}

for c, test := range testCases {
proto, addr, basePath, err := ParseHost(test.host)
if err != nil {
t.Fatal(err)
}

client := &Client{
transport: newMockClient(nil, func(req *http.Request) (*http.Response, error) {
if !strings.HasPrefix(req.URL.Path, testURL) {
return nil, fmt.Errorf("Test Case #%d: Expected URL %q, got %q", c, testURL, req.URL)
}
if req.Host != test.expectedHost {
return nil, fmt.Errorf("Test Case #%d: Expected host %q, got %q", c, test.expectedHost, req.Host)
}
if req.URL.Host != test.expectedURLHost {
return nil, fmt.Errorf("Test Case #%d: Expected URL host %q, got %q", c, test.expectedURLHost, req.URL.Host)
}
return &http.Response{
StatusCode: http.StatusOK,
Body: ioutil.NopCloser(bytes.NewReader(([]byte("")))),
}, nil
}),
proto: proto,
addr: addr,
basePath: basePath,
}

_, err = client.sendRequest(context.Background(), "GET", testURL, nil, nil, nil)
if err != nil {
t.Fatal(err)
}
}
}