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.

Sign up for GitHub

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

dockerclient: Agent supported Docker versions from client min,API Versions #1014

Merged
merged 1 commit into from
Oct 17, 2017
Merged

dockerclient: Agent supported Docker versions from client min,API Versions #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

## UNRELEASED
* Feature - Support for provisioning Tasks with ENIs
* Bug - Fixed a bug where unsupported Docker API client versions could be registered
[#1014](https://github.com/aws/amazon-ecs-agent/pull/1014)

## 1.14.5
* Enhancement - Retry failed container image pull operations [#975](https://github.com/aws/amazon-ecs-agent/pull/975)
* Enhancement - Set read and write timeouts for websocket connectons [#993](https://github.com/aws/amazon-ecs-agent/pull/993)
* Enhancement - Add support for the SumoLogic Docker log driver plugin
[#992](https://github.com/aws/amazon-ecs-agent/pull/992)
[#992](https://github.com/aws/amazon-ecs-agent/pull/992)
* Bug - Fixed a memory leak issue when submitting the task state change [#967](https://github.com/aws/amazon-ecs-agent/pull/967)
* Bug - Fixed a race condition where a container can be created twice when agent restarts. [#939](https://github.com/aws/amazon-ecs-agent/pull/939)
* Bug - Fixed an issue where `microsoft/windowsservercore:latest` was not
Expand Down
78 changes: 70 additions & 8 deletions agent/engine/dockerclient/dockerclientfactory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,23 @@
package dockerclient

import (
"errors"

"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface"
"github.com/aws/amazon-ecs-agent/agent/utils"
log "github.com/cihub/seelog"
docker "github.com/fsouza/go-dockerclient"
"github.com/pkg/errors"
)

const (
// minAPIVersionKey is the docker.Env key for min API version
// This is supported in Docker API versions >=1.25
// https://docs.docker.com/engine/api/version-history/#v125-api-changes
minAPIVersionKey = "MinAPIVersion"
// apiVersionKey is the docker.Env key for API version
apiVersionKey = "ApiVersion"
// zeroPatch is a string to append patch number zero if the major minor version lacks it
zeroPatch = ".0"
)
// Factory provides a collection of docker remote clients that include a
// recommended client version as well as a set of alternative supported
// docker clients.
Expand Down Expand Up @@ -110,19 +120,71 @@ func (f *factory) getClient(version DockerVersion) (dockeriface.Client, error) {
// findDockerVersions loops over all known API versions and finds which ones
// are supported by the docker daemon on the host
func findDockerVersions(endpoint string) map[DockerVersion]dockeriface.Client {
// if the client version returns a MinAPIVersion and APIVersion, then use it to return
// all the Docker clients between MinAPIVersion and APIVersion, else try pinging
// the clients in getKnownAPIVersions
var minAPIVersion, apiVersion string
// get a Docker client with the default supported version
client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would find this easier to read if it were refactored a bit and the conditions were inverted, something like this:

func findDockerVersionsFromAPI(endpoint string) (int, int, bool) {
	client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
	if err != nil {
		seelog.Debug("Failed to query the version API", err)
		return "", "", false
	}

	clientVersion, err := client.Version()
	if err != nil {
		seelog.Debug("Failed to extract version", err)
		return "", "", false
	}

	if !clientVersion.Exists(minAPIVersionKey) || !clientVersion.Exists(apiVersionKey) {
		seelog.Debug("Failed to extract minimum and maximum version")
		return "", "", false
	}

	return clientVersion.Get(minAPIVersionKey), clientVersion.Get(apiVersionKey), true
}

This way the error conditions are more clear and there's less to keep in mind as you're reading through.

clientVersion, err := client.Version()
if err == nil {
// check if the docker.Env obj has MinAPIVersion key
if clientVersion.Exists(minAPIVersionKey) {
minAPIVersion = clientVersion.Get(minAPIVersionKey)
}
// check if the docker.Env obj has APIVersion key
if clientVersion.Exists(apiVersionKey) {
apiVersion = clientVersion.Get(apiVersionKey)
}
}
}

clients := make(map[DockerVersion]dockeriface.Client)
for _, version := range getKnownAPIVersions() {
client, err := newVersionedClient(endpoint, string(version))
dockerClient, err := getDockerClientForVersion(endpoint, string(version), minAPIVersion, apiVersion)
if err != nil {
log.Infof("Error while creating client: %v", err)
log.Infof("Unable to get Docker client for version %s: %v", version, err)
continue
}
clients[version] = dockerClient
}
return clients
}

err = client.Ping()
func getDockerClientForVersion(
endpoint string,
version string,
minAPIVersion string,
apiVersion string) (dockeriface.Client, error) {
if (minAPIVersion != "" && apiVersion != "") {
// Adding patch number zero to Docker versions to reuse the existing semver
// comparator
// TODO: remove this logic later when non-semver comparator is implemented
versionWithPatch := version + zeroPatch
lessThanMinCheck := "<" + minAPIVersion + zeroPatch
moreThanMaxCheck := ">" + apiVersion + zeroPatch
minVersionCheck, err := utils.Version(versionWithPatch).Matches(lessThanMinCheck)
if err != nil {
return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to get min version: %s", minAPIVersion)
}
maxVersionCheck, err := utils.Version(versionWithPatch).Matches(moreThanMaxCheck)
if err != nil {
log.Infof("Failed to ping with Docker version %s: %v", version, err)
return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to get max version: %s", apiVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be MinAPIVersion?

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, this means something was wrong with apiVersion and the comparison check failed.

}
// do not add the version when it is less than min api version or greater
// than api version
if minVersionCheck || maxVersionCheck {
return nil, errors.Errorf("version detection using MinAPIVersion: unsupported version: %s", version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be MinAPIVersion again ?

Copy link
Contributor Author

@sharanyad sharanyad Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, we are simply logging the version from knownVersions , which does not fall in the range [minAPIVersion, apiVersion]

}
clients[version] = client
}
return clients
client, err := newVersionedClient(endpoint, string(version))
if err != nil {
return nil, errors.Wrapf(err, "version detection check: unable to create Docker client for version: %s", version)
}
err = client.Ping()
if err != nil {
return nil, errors.Wrapf(err, "version detection check: failed to ping with Docker version: %s", string(version))
}
return client, nil
}
62 changes: 60 additions & 2 deletions agent/engine/dockerclient/dockerclientfactory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface/mocks"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
docker "github.com/fsouza/go-dockerclient"
)

const expectedEndpoint = "expectedEndpoint"
Expand All @@ -35,7 +36,8 @@ func TestGetDefaultClientSuccess(t *testing.T) {
if version == string(getDefaultVersion()) {
mockClient = expectedClient
}
mockClient.EXPECT().Ping()
mockClient.EXPECT().Version().Return(&docker.Env{}, nil).AnyTimes()
mockClient.EXPECT().Ping().AnyTimes()

return mockClient, nil
}
Expand All @@ -59,7 +61,8 @@ func TestFindSupportedAPIVersions(t *testing.T) {
// Ensure that agent pings all known versions of Docker API
for i := 0; i < len(allVersions); i++ {
mockClients[string(allVersions[i])] = mock_dockeriface.NewMockClient(ctrl)
mockClients[string(allVersions[i])].EXPECT().Ping()
mockClients[string(allVersions[i])].EXPECT().Version().Return(&docker.Env{}, nil).AnyTimes()
mockClients[string(allVersions[i])].EXPECT().Ping().AnyTimes()
}

// Define the function for the mock client
Expand Down Expand Up @@ -92,3 +95,58 @@ func TestVerifyAgentVersions(t *testing.T) {
assert.True(t, isKnown(agentVersion))
}
}

func TestFindSupportedAPIVersionsFromMinAPIVersions(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

agentVersions := getAgentVersions()
allVersions := getKnownAPIVersions()

// Set up the mocks and expectations
mockClients := make(map[string]*mock_dockeriface.MockClient)

// Ensure that agent pings all known versions of Docker API
for i := 0; i < len(allVersions); i++ {
mockClients[string(allVersions[i])] = mock_dockeriface.NewMockClient(ctrl)
mockClients[string(allVersions[i])].EXPECT().Version().Return(&docker.Env{"MinAPIVersion=1.12","ApiVersion=1.27"}, nil).AnyTimes()
mockClients[string(allVersions[i])].EXPECT().Ping().AnyTimes()
}

// Define the function for the mock client
// For simplicity, we will pretend all versions of docker are available
newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
return mockClients[version], nil
}

factory := NewFactory(expectedEndpoint)
actualVersions := factory.FindSupportedAPIVersions()

assert.Equal(t, len(agentVersions), len(actualVersions))
for i := 0; i < len(actualVersions); i++ {
assert.Equal(t, agentVersions[i], actualVersions[i])
}
}

func TestCompareDockerVersionsWithMinAPIVersion(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
minAPIVersion := "1.12"
apiVersion := "1.27"
versions := []string{"1.11", "1.29"}
rightVersion := "1.25"

for _, version := range versions {
_, err := getDockerClientForVersion("endpoint", version, minAPIVersion, apiVersion)
assert.EqualError(t, err, "version detection using MinAPIVersion: unsupported version: " + version)
}

mockClients := make(map[string]*mock_dockeriface.MockClient)
newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
mockClients[version] = mock_dockeriface.NewMockClient(ctrl)
mockClients[version].EXPECT().Ping()
return mockClients[version], nil
}
client, _ := getDockerClientForVersion("endpoint", rightVersion, minAPIVersion, apiVersion)
assert.Equal(t, mockClients[rightVersion], client)
}
4 changes: 3 additions & 1 deletion agent/engine/dockerclient/dockerclientfactory_unix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface/mocks"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
docker "github.com/fsouza/go-dockerclient"
)

func TestGetClientCached(t *testing.T) {
Expand All @@ -29,7 +30,8 @@ func TestGetClientCached(t *testing.T) {

newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
mockClient := mock_dockeriface.NewMockClient(ctrl)
mockClient.EXPECT().Ping()
mockClient.EXPECT().Version().Return(&docker.Env{}, nil).AnyTimes()
mockClient.EXPECT().Ping().AnyTimes()
return mockClient, nil
}

Expand Down
6 changes: 4 additions & 2 deletions agent/engine/dockerclient/dockerclientfactory_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface/mocks"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
docker "github.com/fsouza/go-dockerclient"
)

func TestGetClientMinimumVersion(t *testing.T) {
Expand All @@ -31,10 +32,11 @@ func TestGetClientMinimumVersion(t *testing.T) {

newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
mockClient := mock_dockeriface.NewMockClient(ctrl)
if version == string(MinDockerAPIWindows) {
if version == string(minDockerAPIVersion) {
mockClient = expectedClient
}
mockClient.EXPECT().Ping()
mockClient.EXPECT().Version().Return(&docker.Env{}, nil).AnyTimes()
mockClient.EXPECT().Ping().AnyTimes()
return mockClient, nil
}

Expand Down
4 changes: 4 additions & 0 deletions agent/engine/dockerclient/versionsupport_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface"
)

const (
// minDockerAPIVersion is the min Docker API version supported by agent
minDockerAPIVersion = Version_1_17
)
// GetClient on linux will simply return the cached client from the map
func (f *factory) GetClient(version DockerVersion) (dockeriface.Client, error) {
return f.getClient(version)
Expand Down
8 changes: 4 additions & 4 deletions agent/engine/dockerclient/versionsupport_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface"
)

const MinDockerAPIWindows = Version_1_24
const minDockerAPIVersion = Version_1_24

// GetClient will replace some versions of Docker on Windows. We need this because
// agent assumes that it can always call older versions of the docker API.
func (f *factory) GetClient(version DockerVersion) (dockeriface.Client, error) {
for _, v := range getWindowsReplaceableVersions() {
if v == version {
version = MinDockerAPIWindows
version = minDockerAPIVersion
break
}
}
Expand All @@ -48,10 +48,10 @@ func getWindowsReplaceableVersions() []DockerVersion {

// getAgentVersions for Windows should return all of the replaceable versions plus additional versions
func getAgentVersions() []DockerVersion {
return append(getWindowsReplaceableVersions(), MinDockerAPIWindows)
return append(getWindowsReplaceableVersions(), minDockerAPIVersion)
}

// getDefaultVersion returns agent's default version of the Docker API
func getDefaultVersion() DockerVersion {
return MinDockerAPIWindows
return minDockerAPIVersion
}