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

Add HTTP Host header to support Docker v1.12 #1550

Merged
merged 1 commit into from
Jun 23, 2016

Conversation

l0rd
Copy link
Contributor

@l0rd l0rd commented Jun 22, 2016

Host is a mandatory header in HTTP 1.1 specifications. In previous versions of Docker that header wasn't required but release v1.12 use golang 1.6.2 that makes this header mandatory.

Therefore Docker v1.12 daemon doesn't accept HTTP 1.1 requests with no Host header set (you can find some references to the discussions here).

This works:
echo -e "GET /containers/json HTTP/1.1\r\nHost: \r\n\r\n" | netcat -U /var/run/docker.sock

This doesn't work:
echo -e "GET /containers/json HTTP/1.1\r\n\r\n" | netcat -U /var/run/docker.sock

As a consequence in Che we had this nasty error message whenever we tried to create a new workspace:

org.eclipse.che.api.machine.server.exception.MachineException: Failed writing 8192 bytes

This PR is related to #1482 and #745.

@codenvy-ci
Copy link

Can one of the admins verify this patch?

@TylerJewell
Copy link

@mmorhun @garagatyi

@@ -67,6 +67,7 @@ protected DockerResponse request(String method, String path, String query, List<
for (Pair<String, ?> header : headers) {
connection.setRequestProperty(header.first, String.valueOf(header.second));
}
connection.setRequestProperty("Host", url.toString());

Choose a reason for hiding this comment

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

In that case not only host will be put in that header but host + path +query.
@l0rd Do you think it is OK or only host+port should be put in that header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garagatyi you are absolutely right. We should set it to host+port only. Although I've test it and it worked, path+query should not be there. I'll fix that.

Choose a reason for hiding this comment

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

We still can use empty value. it is valid value due to the spec.

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>
@garagatyi
Copy link

LGTM

@garagatyi
Copy link

@skabashnyuk Please take a look

@skabashnyuk
Copy link
Contributor

ok

@mmorhun
Copy link
Contributor

mmorhun commented Jun 23, 2016

LGTM

@riuvshin
Copy link
Contributor

OK

@garagatyi garagatyi merged commit 14e6d4a into eclipse-che:master Jun 23, 2016
tolusha pushed a commit that referenced this pull request Jul 5, 2016
Signed-off-by: Mario Loriedo <mloriedo@redhat.com>
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.

7 participants