refactoring, modify changelog

CHANGELOG.md

@@ -3,7 +3,7 @@
 ## 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)
+[#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)

agent/engine/dockerclient/dockerclientfactory.go

@@ -121,50 +121,27 @@ func (f *factory) getClient(version DockerVersion) (dockeriface.Client, error) {
 // 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
-	clients, err := findDockerVersionsfromMinMaxVersions(endpoint)
-	if err != nil {
-		log.Infof("Unable to get Docker clients from min API version: %v", err)
-	} else {
-		return clients
-	}
-	clients = make(map[DockerVersion]dockeriface.Client)
-	for _, version := range getKnownAPIVersions() {
-		client, err := newVersionedClient(endpoint, string(version))
-		if err != nil {
-			log.Infof("Error while creating client: %v", err)
-			continue
-		}
-
-		err = client.Ping()
-		if err != nil {
-			log.Infof("Failed to ping with Docker version %s: %v", version, err)
-			continue
-		}
-		clients[version] = client
-	}
-	return clients
-}
-
-func findDockerVersionsfromMinMaxVersions(endpoint string) (map[DockerVersion]dockeriface.Client, error) {
+	// all the Docker clients between MinAPIVersion and APIVersion, else try pinging
+	// the clients in getKnownAPIVersions
+	minAPIVersion := ""
+	apiVersion := ""
 	// get a Docker client with the default supported version
 	client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
-	if err != nil {
-		return nil, errors.Wrap(err, "min API version check: error while creating default Docker client")
-	}
-
-	clientVersion, err := client.Version()
-	if err != nil {
-		return nil, errors.Wrap(err, "min API version check: error while getting client version")
+	if err == nil {
+		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)
-	// check if the docker.Env obj has MinAPIVersion key
-	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 {
@@ -173,36 +150,42 @@ func findDockerVersionsfromMinMaxVersions(endpoint string) (map[DockerVersion]do
 		}
 		clients[version] = dockerClient
 	}
-	return clients, nil
+	return clients
 }
 
 func getDockerClientForVersion(
 	endpoint string,
 	version string,
 	minAPIVersion string,
 	apiVersion string) (dockeriface.Client, error) {
-	// 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)
+	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 {
+			return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to get max version: %s", apiVersion)
+		}
+		// 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)
+		}
 	}
-	maxVersionCheck, err := utils.Version(versionWithPatch).Matches(moreThanMaxCheck)
+	client, err := newVersionedClient(endpoint, string(version))
 	if err != nil {
-		return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to get max version: %s", apiVersion)
-	}
-	// 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)
+		return nil, errors.Wrapf(err, "version detection check: unable to create Docker client for version: %s", version)
 	}
-	client, err := newVersionedClient(endpoint, string(version))
+	err = client.Ping()
 	if err != nil {
-		return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to create Docker client for version: %s", version)
+		return nil, errors.Wrapf(err, "version detection check: failed to ping with Docker version: %s", string(version))
 	}
 	return client, nil
 }

agent/engine/dockerclient/dockerclientfactory_test.go

@@ -62,7 +62,7 @@ func TestFindSupportedAPIVersions(t *testing.T) {
 	for i := 0; i < len(allVersions); i++ {
 		mockClients[string(allVersions[i])] = mock_dockeriface.NewMockClient(ctrl)
 		mockClients[string(allVersions[i])].EXPECT().Version().Return(&docker.Env{}, nil).AnyTimes()
-		mockClients[string(allVersions[i])].EXPECT().Ping()
+		mockClients[string(allVersions[i])].EXPECT().Ping().AnyTimes()
 	}
 
 	// Define the function for the mock client
@@ -144,6 +144,7 @@ func TestCompareDockerVersionsWithMinAPIVersion(t *testing.T) {
 	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)