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

docker api enforces tls from docker 1.13 onwards #80

Merged
merged 3 commits into from
Feb 24, 2017

Conversation

calvinchengx
Copy link
Contributor

For docker 1.13 onwards, not using dc.NewTLSClient will fail with malformed http response.

Also, is there an option in dockertest v3 to support docker-machine? There was an option to support docker-machine in v2 but I can't seem to find it in v3. Providing such an option will allow us to use dc.NewClientFromEnv().

For docker 1.13 onwards, not using `dc.NewTLSClient` will fail with malformed http response.

Also, is there an option in dockertest v3 to support docker-machine? There was an option to support docker-machine in v2 but I can't seem to find it in v3. Providing such an option will allow us to use `dc.NewClientFromEnv()`.
@coveralls
Copy link

coveralls commented Feb 19, 2017

Coverage Status

Changes Unknown when pulling 97abb20 on calvinchengx:patch-1 into ** on ory:v3**.

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2017

No, docker-machine is no longer supported explicitly. But you can simply point NewPool to the ip:port of the docker daemon in the docker-machine image?

@calvinchengx
Copy link
Contributor Author

Ok. But even if it doesn't support docker-machine explicitly, NewPool with the correct ip:port will still fail because NewPool uses dc.NewClient instead of dc.NewTLSClient. docker 1.13 onwards requires TLS communication.

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2017

Unfortunately, docker native (for windows) does not set up $DOCKER_CERT_PATH automatically. Any idea how to resolve this? Also, do we need TLS if we talk to a local socket?

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2017

I just ran unit tests on windows that runs docker (for windows) 1.13.1 and it still works with master. What problem are you facing exactly?

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2017

I also remember that docker for windows explicitly sets up the http endpoint because the certificate jazz is just broken on windows. And for osx and linux, docker uses (iirc) a socket unix:///var/run/docker.sock which I think does not need TLS because it's local.

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2017

Yup, when I unit test your patch on windows, this is what I get:

Error:          Expected nil, but got: : Post https://localhost:2375/images/create?fromImage=postgres&tag=9.5: http: server gave HTTP response to HTTPS client

@calvinchengx
Copy link
Contributor Author

We don't need TLS if we talk to a local socket, but we do need TLS if we talk to a remote socket (whether the remote connection is a docker-machine's docker server or a remote machine's docker server). This is now enforced.

So NewPool will probably need a flag of sorts so that library users can specify whether to use TLS or not depending on their requirements.

@aeneasr
Copy link
Member

aeneasr commented Feb 21, 2017 via email

dockertest.go Outdated

client, err := dc.NewClient(endpoint)
client, err := dc.NewClient(endpoint, cert, key, ca)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cde1a9a on calvinchengx:patch-1 into ** on ory:v3**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cde1a9a on calvinchengx:patch-1 into ** on ory:v3**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cde1a9a on calvinchengx:patch-1 into ** on ory:v3**.

@aeneasr
Copy link
Member

aeneasr commented Feb 21, 2017

coveralls, stahp

@calvinchengx
Copy link
Contributor Author

Would this pull request be merged? Is there any other issue I need to amend?

@aeneasr
Copy link
Member

aeneasr commented Feb 24, 2017

Yup :)

@aeneasr aeneasr merged commit 6f03cce into ory:v3 Feb 24, 2017
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.

None yet

3 participants