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 capability testing is broken with Docker >= 1.13.0 #1010

Closed
samuelkarp opened this issue Oct 9, 2017 · 7 comments
Closed

Docker capability testing is broken with Docker >= 1.13.0 #1010

samuelkarp opened this issue Oct 9, 2017 · 7 comments

Comments

@samuelkarp
Copy link
Contributor

Summary

Capability detection is broken because we're not able to detect supported Remote API versions properly since Docker 1.13.0.

Description

See moby/moby#35145

Docker stopped rejecting API requests with version numbers greater than the maximum version supported by the daemon. This happened in moby/moby@e98e4a7#diff-14051df1b5de1608aaba3a983f2a87e3 as part of moby/moby#27745 and has affected every version of Docker since 1.13.0.

This change breaks the capability detection logic used by the Amazon ECS agent. The Amazon ECS agent is designed to work with all Docker versions >= 1.5.0, and we enable or disable features depending on the capabilities of the Docker version used. We use Remote API version as we expected this to be more stable than the daemon version; the daemon version has gone through a few changes since we started (roughly semver-like to year-month plus edition). We currently call the /_ping API with a range of different versions to discover which API versions are supported by the Docker daemon, but this change makes it so that every request with an API version >= 1.12 will result in success. This is causing incorrect behavior where features are being enabled with incompatible Docker versions; see #1008 .

In https://docs.docker.com/engine/api/version-history/#v125-api-changes , I see a note about a new header returned that specifies the maximum API version of the daemon. However, I do not see a note about the change in behavior to start accepting requests that have a specified version greater than the maximum.

Steps to reproduce the issue:
Run curl --verbose --unix-socket /var/run/docker.sock http://localhost/v9999.9999/_ping

On Docker versions prior to 1.13.0, you'll see an HTTP 400 with an error message "client is newer than server"
On Docker versions 1.13.0 or greater, you'll see an HTTP 200 OK.

@aaithal
Copy link
Contributor

aaithal commented Oct 11, 2017

To make the version detection logic in the Agent more generic, let's do this:

  1. create an unversioned docker client
  2. call client.Version()
  3. if response contains MinAPIVersion, register the versions we care about between [MinAPIVersion,ApiVersion]
  4. else use the versioned client + client.Ping() logic we have today

@samuelkarp
Copy link
Contributor Author

LGTM, except I'd just use a versioned client at the default (1.17 for Linux and 1.24 for Windows) in order to make the initial /version call.

@sharanyad
Copy link
Contributor

According to today's supported docker versions detection logic, even when client ping fails, we add it to the list of supported API versions.
Code path here

@samuelkarp
Copy link
Contributor Author

@sharanyad There are actually two different things going on. We're trying to detect all API versions that the Docker daemon knows about that we also know exist, and separately from that we're trying to detect all API versions that the Docker daemon knows and we fully support. This logic exists so that we can add new logging drivers (since they'll have been introduced along with a new Docker API version) without gating on full support for the API by the agent/go-dockerclient. You'll want to look at the difference between FindKnownAPIVersions and FindSupportedAPIVersions.

@sharanyad
Copy link
Contributor

Yes, I understand the distinction between FindKnownAPIVersions and FindSupportedAPIVersions. What I intended to convey was in the findDockerVersions method, when client.Ping() fails, instead of returning err, we add the client to the list. I think the logic was modified during regression.

@samuelkarp
Copy link
Contributor Author

@sharanyad Got it, that does sound like an additional bug.

@sharanyad
Copy link
Contributor

Merged #1014 fixes this issue, hence closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants