wip unit tests

aws · Jan 29, 2024 · d5398da · d5398da
1 parent da0cd67
commit d5398da
Show file tree

Hide file tree

Showing 4 changed files with 151 additions and 106 deletions.
diff --git a/ecs-init/docker/dependencies.go b/ecs-init/docker/dependencies.go
@@ -104,10 +104,15 @@ func preferredAPIVersions() []string {
 	}
 }
 
+// knownAPIVersions returns all known docker API versions, in order from
+// oldest to newest.
 func knownAPIVersions() []string {
 	return append(backupAPIVersions(), preferredAPIVersions()...)
 }
 
+// dockerVersionCompare returns 0 if lhs and rhs are equal.
+// returns 1 if lhs > rhs
+// returns -1 if lhs < rhs
 func dockerVersionCompare(lhs, rhs string) int {
 	if lhs == rhs {
 		return 0

diff --git a/ecs-init/docker/dependencies_test.go b/ecs-init/docker/dependencies_test.go
@@ -1,6 +1,3 @@
-//go:build test
-// +build test
-
 // Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License"). You may
@@ -35,6 +32,7 @@ const immediately = time.Duration(-1)
 
 var netError = &url.Error{Err: &net.OpError{Op: "read", Net: "unix", Err: io.EOF}}
 var httpError = &docker.Error{Status: http.StatusInternalServerError, Message: "error"}
+var badRequestError = &docker.Error{Status: http.StatusBadRequest, Message: "Bad Request Error"}
 
 func TestIsNetworkErrorReturnsTrue(t *testing.T) {
 	assert.True(t, isNetworkError(netError), "Expect isNetworkError to return true if network error passed in")
@@ -48,6 +46,10 @@ func TestIsRetryablePingErrorReturnsTrue(t *testing.T) {
 	assert.True(t, isRetryablePingError(httpError), "Expect RetryablePingError to be true if httpError passed in")
 }
 
+func TestIsRetryableBadRequestErrorReturnsFalse(t *testing.T) {
+	assert.False(t, isRetryablePingError(badRequestError), "Expect RetryablePingError to be false if 'bad request' client error passed in")
+}
+
 func TestIsRetryablePingErrorReturnsFalse(t *testing.T) {
 	assert.False(t, isRetryablePingError(fmt.Errorf("error")), "Expect RetryablePingError to be true if non-http error passed in")
 }
@@ -100,10 +102,33 @@ func TestNewDockerClientDoesnotRetryOnPingNonNetworkError(t *testing.T) {
 	mockClientFactory := NewMockdockerClientFactory(ctrl)
 	mockBackoff := NewMockBackoff(ctrl)
 
-	gomock.InOrder(
-		mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(mockDockerClient, nil),
-		mockDockerClient.EXPECT().Ping().Return(fmt.Errorf("error")),
-	)
+	calls := []*gomock.Call{}
+	for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
+		t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
+		calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
+		calls = append(calls, mockDockerClient.EXPECT().Ping().Return(fmt.Errorf("error")))
+	}
+	gomock.InOrder(calls...)
+
+	_, err := newDockerClient(mockClientFactory, mockBackoff)
+	assert.Error(t, err, "Expect error when creating docker client with no retry")
+}
+
+func TestNewDockerClientDoesNotRetryOnHTTPBadRequestError(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockDockerClient := NewMockdockerclient(ctrl)
+	mockClientFactory := NewMockdockerClientFactory(ctrl)
+	mockBackoff := NewMockBackoff(ctrl)
+
+	calls := []*gomock.Call{}
+	for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
+		t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
+		calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
+		calls = append(calls, mockDockerClient.EXPECT().Ping().Return(badRequestError))
+	}
+	gomock.InOrder(calls...)
 
 	_, err := newDockerClient(mockClientFactory, mockBackoff)
 	assert.Error(t, err, "Expect error when creating docker client with no retry")
@@ -117,14 +142,17 @@ func TestNewDockerClientGivesUpRetryingOnPingNetworkError(t *testing.T) {
 	mockClientFactory := NewMockdockerClientFactory(ctrl)
 	mockBackoff := NewMockBackoff(ctrl)
 
-	gomock.InOrder(
-		mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(mockDockerClient, nil),
-		mockDockerClient.EXPECT().Ping().Return(netError),
-		mockBackoff.EXPECT().ShouldRetry().Return(true),
-		mockBackoff.EXPECT().Duration().Return(immediately),
-		mockDockerClient.EXPECT().Ping().Return(netError),
-		mockBackoff.EXPECT().ShouldRetry().Return(false),
-	)
+	calls := []*gomock.Call{}
+	for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
+		t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
+		calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
+		calls = append(calls, mockDockerClient.EXPECT().Ping().Return(netError))
+		calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(true))
+		calls = append(calls, mockBackoff.EXPECT().Duration().Return(immediately))
+		calls = append(calls, mockDockerClient.EXPECT().Ping().Return(netError))
+		calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(false))
+	}
+	gomock.InOrder(calls...)
 
 	_, err := newDockerClient(mockClientFactory, mockBackoff)
 	assert.Error(t, err, "Expect error when creating docker client with no retry")
@@ -137,20 +165,20 @@ func TestNewDockerClientGivesUpRetryingOnUnavailableSocket(t *testing.T) {
 	mockClientFactory := NewMockdockerClientFactory(ctrl)
 	mockBackoff := NewMockBackoff(ctrl)
 
-	realDockerClient, dockerClientErr := godockerClientFactory{}.NewVersionedClient("unix:///a/bad/docker.sock", dockerClientAPIVersion)
+	realDockerClient, dockerClientErr := godockerClientFactory{}.NewVersionedClient("unix:///a/bad/docker.sock", "1.25")
 
 	require.False(t, dockerClientErr != nil, "there should be no errors trying to set up a docker client (intentionally bad with nonexistent socket path")
 
-	gomock.InOrder(
-		// We use the real client to ensure we're classifying errors
-		// correctly as returned by the upstream's error handling.
-		mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(realDockerClient, dockerClientErr),
+	calls := []*gomock.Call{}
+	for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
+		t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
+		calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(realDockerClient, nil))
 		// "bad connection", retries
-		mockBackoff.EXPECT().ShouldRetry().Return(true),
-		mockBackoff.EXPECT().Duration().Return(immediately),
-		// Give up
-		mockBackoff.EXPECT().ShouldRetry().Return(false),
-	)
+		calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(true))
+		calls = append(calls, mockBackoff.EXPECT().Duration().Return(immediately))
+		calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(false))
+	}
+	gomock.InOrder(calls...)
 
 	_, err := newDockerClient(mockClientFactory, mockBackoff)
 	require.Error(t, err, "expect an error when creating docker client")
@@ -160,3 +188,77 @@ func TestNewDockerClientGivesUpRetryingOnUnavailableSocket(t *testing.T) {
 	_, isExpectedError := err.(*url.Error)
 	assert.True(t, isExpectedError, "expect net.OpError wrapped by url.Error")
 }
+
+func TestDockerVersionCompare(t *testing.T) {
+	testCases := []struct {
+		lhs            string
+		rhs            string
+		expectedResult int
+	}{
+		{
+			lhs:            "1.21",
+			rhs:            "1.24",
+			expectedResult: -1,
+		},
+		{
+			lhs:            "1.21",
+			rhs:            "1.25",
+			expectedResult: -1,
+		},
+		{
+			lhs:            "1.21",
+			rhs:            "1.44",
+			expectedResult: -1,
+		},
+		{
+			lhs:            "1.21",
+			rhs:            "1.21",
+			expectedResult: 0,
+		},
+		{
+			lhs:            "1.25",
+			rhs:            "1.25",
+			expectedResult: 0,
+		},
+		{
+			lhs:            "1.35",
+			rhs:            "1.35",
+			expectedResult: 0,
+		},
+		{
+			lhs:            "1.44",
+			rhs:            "1.44",
+			expectedResult: 0,
+		},
+		{
+			lhs:            "1.24",
+			rhs:            "1.22",
+			expectedResult: 1,
+		},
+		{
+			lhs:            "1.25",
+			rhs:            "1.22",
+			expectedResult: 1,
+		},
+		{
+			lhs:            "1.34",
+			rhs:            "1.25",
+			expectedResult: 1,
+		},
+		{
+			lhs:            "1.44",
+			rhs:            "1.21",
+			expectedResult: 1,
+		},
+		{
+			lhs:            "1.44",
+			rhs:            "1.26",
+			expectedResult: 1,
+		},
+	}
+	for _, tc := range testCases {
+		actualResult := dockerVersionCompare(tc.lhs, tc.rhs)
+		testName := fmt.Sprintf("lhs=%s rhs=%s expectedResult=%d", tc.lhs, tc.rhs, tc.expectedResult)
+		assert.Equal(t, tc.expectedResult, actualResult, testName)
+	}
+}
diff --git a/ecs-init/docker/docker_config.go b/ecs-init/docker/docker_config.go
@@ -64,6 +64,8 @@ func createHostConfig(binds []string) *godocker.HostConfig {
 		CapAdd:      caps,
 	}
 
+	// Task ENI feature requires agent to be "init" process, so set it if docker API
+	// version is new enough for this feature.
 	if dockerVersionCompare(dockerAPIVersion, enableTaskENIDockerClientAPIVersion) >= 0 {
 		hostConfig.Init = true
 	}