agent: Refactoring, adding changelog

CHANGELOG.md

@@ -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
+[#1010](https://github.com/aws/amazon-ecs-agent/issues/1010)
 
 ## 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

agent/engine/dockerclient/dockerclientfactory.go

@@ -23,9 +23,13 @@ import (
 
 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
@@ -116,11 +120,11 @@ 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 min version and api version, then use it to return
-	// Docker versions
+	// if the client version returns a MinAPIVersion and APIVersion, then use it to return
+	// all the Docker clients between MinAPIVersion and APIVersion
 	clients, err := findDockerVersionsfromMinMaxVersions(endpoint)
 	if err != nil {
-		log.Debugf("Unable to get Docker clients from min API version: %v", err)
+		log.Infof("Unable to get Docker clients from min API version: %v", err)
 	} else {
 		return clients
 	}
@@ -156,36 +160,40 @@ func findDockerVersionsfromMinMaxVersions(endpoint string) (map[DockerVersion]do
 
 	clients := make(map[DockerVersion]dockeriface.Client)
 	// check if the docker.Env obj has MinAPIVersion key
-	if clientVersion.Exists(minAPIVersionKey) {
-		minAPIVersion := clientVersion.Get(minAPIVersionKey)
-		apiVersion := clientVersion.Get(apiVersionKey)
-		for _, version := range getKnownAPIVersions() {
-			dockerClient, err := getDockerClientForVersion(endpoint, string(version), minAPIVersion, apiVersion)
-			if err != nil {
-				log.Debugf("Unable to get Docker client for version %s: %v", version, err)
-				continue
-			}
-			clients[version] = dockerClient
+	if !clientVersion.Exists(minAPIVersionKey) {
+		return nil, errors.New("min API version not found in client version")
+	}
+	minAPIVersion := clientVersion.Get(minAPIVersionKey)
+	apiVersion := clientVersion.Get(apiVersionKey)
+	for _, version := range getKnownAPIVersions() {
+		dockerClient, err := getDockerClientForVersion(endpoint, string(version), minAPIVersion, apiVersion)
+		if err != nil {
+			log.Infof("Unable to get Docker client for version %s: %v", version, err)
+			continue
 		}
-		return clients, nil
+		clients[version] = dockerClient
 	}
-	return nil, errors.New("min API version not found in client version")
+	return clients, nil
 }
 
 func getDockerClientForVersion(
 	endpoint string,
 	version string,
 	minAPIVersion string,
 	apiVersion string) (dockeriface.Client, error) {
-	lessThanMinCheck := "<"+minAPIVersion
-	moreThanMaxCheck := ">"+apiVersion
-	minVersionCheck, minErr := utils.Version(version).Matches(lessThanMinCheck)
-	maxVersionCheck, maxErr := utils.Version(version).Matches(moreThanMaxCheck)
-	if minErr != nil {
-		return nil, errors.Wrapf(minErr, "error while comparing version %s with minAPIVersion %s", version, minAPIVersion)
+	// Adding patch number zero to Docker versions to reuse the existing semver
+	// comparator
+	// TODO: remove this logic later when non-semver comparator is implemented
+	version += zeroPatch
+	lessThanMinCheck := "<" + minAPIVersion + zeroPatch
+	moreThanMaxCheck := ">" + apiVersion + zeroPatch
+	minVersionCheck, err := utils.Version(version).Matches(lessThanMinCheck)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error while comparing version %s with minAPIVersion %s", version, minAPIVersion)
 	}
-	if maxErr != nil {
-		return nil, errors.Wrapf(minErr, "error while comparing version %s with apiVersion %s", version, apiVersion)
+	maxVersionCheck, err := utils.Version(version).Matches(moreThanMaxCheck)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error while comparing version %s with apiVersion %s", version, apiVersion)
 	}
 	// do not add the version when it is less than min api version or greater
 	// than api version

agent/utils/compare_versions.go

@@ -20,9 +20,6 @@ import (
 	"strings"
 )
 
-// zeroPatch is a string to append patch number zero if the major minor version lacks it
-const zeroPatch = ".0"
-
 type Version string
 
 type semver struct {
@@ -43,8 +40,7 @@ type semver struct {
 // * <x.y.z -- Matches a version less than the selector version
 // * x.y.z,a.b.c -- Matches if the version matches either of the two selector versions
 func (lhs Version) Matches(selector string) (bool, error) {
-	lhsVer := appendIfNoPatch(string(lhs))
-	lhsVersion, err := parseSemver(lhsVer)
+	lhsVersion, err := parseSemver(string(lhs))
 	if err != nil {
 		return false, err
 	}
@@ -63,29 +59,25 @@ func (lhs Version) Matches(selector string) (bool, error) {
 	}
 
 	if strings.HasPrefix(selector, ">=") {
-		rhs := appendIfNoPatch(selector[2:])
-		rhsVersion, err := parseSemver(rhs)
+		rhsVersion, err := parseSemver(selector[2:])
 		if err != nil {
 			return false, err
 		}
 		return compareSemver(lhsVersion, rhsVersion) >= 0, nil
 	} else if strings.HasPrefix(selector, ">") {
-		rhs := appendIfNoPatch(selector[1:])
-		rhsVersion, err := parseSemver(rhs)
+		rhsVersion, err := parseSemver(selector[1:])
 		if err != nil {
 			return false, err
 		}
 		return compareSemver(lhsVersion, rhsVersion) > 0, nil
 	} else if strings.HasPrefix(selector, "<=") {
-		rhs := appendIfNoPatch(selector[2:])
-		rhsVersion, err := parseSemver(rhs)
+		rhsVersion, err := parseSemver(selector[2:])
 		if err != nil {
 			return false, err
 		}
 		return compareSemver(lhsVersion, rhsVersion) <= 0, nil
 	} else if strings.HasPrefix(selector, "<") {
-		rhs := appendIfNoPatch(selector[1:])
-		rhsVersion, err := parseSemver(rhs)
+		rhsVersion, err := parseSemver(selector[1:])
 		if err != nil {
 			return false, err
 		}
@@ -136,16 +128,6 @@ func parseSemver(version string) (semver, error) {
 	return result, nil
 }
 
-// TODO: remove this logic once non-semver comparator is implemented
-func appendIfNoPatch(version string) string {
-	versionParts := strings.Split(version, ".")
-	// Only major and minor versions are present
-	if len(versionParts) == 2 {
-		version += zeroPatch
-	}
-	return version
-}
-
 // compareSemver compares two semvers, 'lhs' and 'rhs', and returns -1 if lhs is less
 // than rhs, 0 if they are equal, and 1 lhs is greater than rhs
 func compareSemver(lhs, rhs semver) int {