From 58be1028f9e2a1429227b866749947beccd8f206 Mon Sep 17 00:00:00 2001 From: Cameron Sparr Date: Mon, 29 Jan 2024 11:11:58 -0800 Subject: [PATCH] wip unit tests --- ecs-init/docker/dependencies.go | 5 + ecs-init/docker/dependencies_test.go | 152 ++++++++++++++++++++++----- ecs-init/docker/docker_test.go | 98 +++-------------- 3 files changed, 149 insertions(+), 106 deletions(-) diff --git a/ecs-init/docker/dependencies.go b/ecs-init/docker/dependencies.go index d18986f3461..ad675273d47 100644 --- 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 index 66a630b77c8..2cf256e7dc2 100644 --- 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_test.go b/ecs-init/docker/docker_test.go index 0fa1a11802f..270e17becbd 100644 --- a/ecs-init/docker/docker_test.go +++ b/ecs-init/docker/docker_test.go @@ -1,6 +1,3 @@ -//go:build test -// +build test - // Copyright 2015-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may @@ -31,16 +28,13 @@ import ( "github.com/stretchr/testify/assert" ) -// expectedAgentBinds is the total number of agent host config binds. +// defaultExpectedAgentBinds is the total number of agent host config binds. // Note: Change this value every time when a new bind mount is added to // agent for the tests to pass const ( - expectedAgentBindsUnspecifiedPlatform = 20 - expectedAgentBindsSuseUbuntuPlatform = 18 + defaultExpectedAgentBinds = 20 ) -var expectedAgentBinds = expectedAgentBindsUnspecifiedPlatform - func TestIsAgentImageLoadedListFailure(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -203,7 +197,7 @@ func TestStartAgentNoEnvFile(t *testing.T) { mockFS.EXPECT().ReadFile(config.InstanceConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockFS.EXPECT().ReadFile(config.AgentConfigFile()).Return(nil, errors.New("test error")).AnyTimes() mockDocker.EXPECT().CreateContainer(gomock.Any()).Do(func(opts godocker.CreateContainerOptions) { - validateCommonCreateContainerOptions(opts, t) + validateCommonCreateContainerOptions(t, opts, defaultExpectedAgentBinds) }).Return(&godocker.Container{ ID: containerID, }, nil) @@ -221,7 +215,11 @@ func TestStartAgentNoEnvFile(t *testing.T) { } } -func validateCommonCreateContainerOptions(opts godocker.CreateContainerOptions, t *testing.T) { +func validateCommonCreateContainerOptions( + t *testing.T, + opts godocker.CreateContainerOptions, + expectedAgentBinds int, +) { if opts.Name != "ecs-agent" { t.Errorf("Expected container Name to be %s but was %s", "ecs-agent", opts.Name) } @@ -252,17 +250,7 @@ func validateCommonCreateContainerOptions(opts godocker.CreateContainerOptions, hostCfg := opts.HostConfig - // for hosts that do not have cert directories explicity mounted, ignore - // host cert directory configuration. - // TODO (adnxn): ideally, these should be behind build flags. - // https://github.com/aws/amazon-ecs-init/issues/131 - if certDir := config.HostPKIDirPath(); certDir == "" { - expectedAgentBinds = expectedAgentBindsSuseUbuntuPlatform - } - - if len(hostCfg.Binds) != expectedAgentBinds { - t.Errorf("Expected exactly %d elements to be in Binds, but was %d", expectedAgentBinds, len(hostCfg.Binds)) - } + assert.Len(t, hostCfg.Binds, expectedAgentBinds) binds := make(map[string]struct{}) for _, binding := range hostCfg.Binds { binds[binding] = struct{}{} @@ -317,9 +305,7 @@ func validateCommonCreateContainerOptions(opts godocker.CreateContainerOptions, } func expectKey(key string, input map[string]struct{}, t *testing.T) { - if _, ok := input[key]; !ok { - t.Errorf("Expected %s to be defined", key) - } + assert.Contains(t, input, key) } func TestStartAgentEnvFile(t *testing.T) { @@ -341,7 +327,7 @@ func TestStartAgentEnvFile(t *testing.T) { mockFS.EXPECT().ReadFile(config.InstanceConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockFS.EXPECT().ReadFile(config.AgentConfigFile()).Return([]byte(envFile), nil).AnyTimes() mockDocker.EXPECT().CreateContainer(gomock.Any()).Do(func(opts godocker.CreateContainerOptions) { - validateCommonCreateContainerOptions(opts, t) + validateCommonCreateContainerOptions(t, opts, defaultExpectedAgentBinds) cfg := opts.Config envVariables := make(map[string]struct{}) @@ -378,11 +364,11 @@ func TestStartAgentWithGPUConfig(t *testing.T) { envFile := "\nECS_ENABLE_GPU_SUPPORT=true\n" containerID := "container id" + expectedAgentBinds := defaultExpectedAgentBinds expectedAgentBinds += 1 defer func() { MatchFilePatternForGPU = FilePatternMatchForGPU - expectedAgentBinds = expectedAgentBindsUnspecifiedPlatform }() MatchFilePatternForGPU = func(pattern string) ([]string, error) { return []string{"/dev/nvidia0", "/dev/nvidia1"}, nil @@ -394,7 +380,7 @@ func TestStartAgentWithGPUConfig(t *testing.T) { mockFS.EXPECT().ReadFile(config.InstanceConfigFile()).Return([]byte(envFile), nil).AnyTimes() mockFS.EXPECT().ReadFile(config.AgentConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockDocker.EXPECT().CreateContainer(gomock.Any()).Do(func(opts godocker.CreateContainerOptions) { - validateCommonCreateContainerOptions(opts, t) + validateCommonCreateContainerOptions(t, opts, expectedAgentBinds) var found bool for _, bind := range opts.HostConfig.Binds { if bind == gpu.GPUInfoDirPath+":"+gpu.GPUInfoDirPath { @@ -452,7 +438,7 @@ func TestStartAgentWithGPUConfigNoDevices(t *testing.T) { mockFS.EXPECT().ReadFile(config.InstanceConfigFile()).Return([]byte(envFile), nil).AnyTimes() mockFS.EXPECT().ReadFile(config.AgentConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockDocker.EXPECT().CreateContainer(gomock.Any()).Do(func(opts godocker.CreateContainerOptions) { - validateCommonCreateContainerOptions(opts, t) + validateCommonCreateContainerOptions(t, opts, defaultExpectedAgentBinds) cfg := opts.Config envVariables := make(map[string]struct{}) @@ -837,12 +823,12 @@ func TestStartAgentWithExecBinds(t *testing.T) { hostResourcesRootDir + ":" + containerResourcesRootDir + readOnly, hostConfigDir + ":" + containerConfigDir, } - expectedAgentBinds += len(expectedExecBinds) + expectedAgentBinds := defaultExpectedAgentBinds + expectedAgentBinds += len(expectedExecBinds) // bind mount for the config folder is already included in expectedAgentBinds since it's always added expectedExecBinds = append(expectedExecBinds, hostConfigDir+":"+containerConfigDir) defer func() { - expectedAgentBinds = expectedAgentBindsUnspecifiedPlatform isPathValid = defaultIsPathValid }() @@ -852,7 +838,7 @@ func TestStartAgentWithExecBinds(t *testing.T) { mockFS.EXPECT().ReadFile(config.InstanceConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockFS.EXPECT().ReadFile(config.AgentConfigFile()).Return(nil, errors.New("not found")).AnyTimes() mockDocker.EXPECT().CreateContainer(gomock.Any()).Do(func(opts godocker.CreateContainerOptions) { - validateCommonCreateContainerOptions(opts, t) + validateCommonCreateContainerOptions(t, opts, expectedAgentBinds) // verify that exec binds are added assert.Subset(t, opts.HostConfig.Binds, expectedExecBinds) @@ -985,56 +971,6 @@ func TestDefaultIsPathValid(t *testing.T) { } } -func TestGetCredentialsFetcherSocketBind(t *testing.T) { - testCases := []struct { - name string - credentialsFetcherHostFromEnv string - credentialsFetcherHostFromConfigFile string - expectedBind string - }{ - { - name: "No Credentials Fetcher host from env", - credentialsFetcherHostFromEnv: "", - credentialsFetcherHostFromConfigFile: "dummy", - expectedBind: "/var/credentials-fetcher/socket/credentials_fetcher.sock:/var/credentials-fetcher/socket/credentials_fetcher.sock", - }, - { - name: "Invalid Credentials Fetcher host from env", - credentialsFetcherHostFromEnv: "invalid", - credentialsFetcherHostFromConfigFile: "dummy", - expectedBind: "/var/credentials-fetcher/socket/credentials_fetcher.sock:/var/credentials-fetcher/socket/credentials_fetcher.sock", - }, - { - name: "Credentials Fetcher from env, no Credentials Fetcher from config file", - credentialsFetcherHostFromEnv: "unix:///var/credentials-fetcher/socket/credentials_fetcher.sock", - credentialsFetcherHostFromConfigFile: "", - expectedBind: "/var/credentials-fetcher/socket/credentials_fetcher.sock:/var/credentials-fetcher/socket/credentials_fetcher.sock", - }, - { - name: "Credentials Fetcher from env, invalid Credentials Fetcher from config file", - credentialsFetcherHostFromEnv: "unix:///var/credentials-fetcher/socket/credentials_fetcher.sock", - credentialsFetcherHostFromConfigFile: "invalid", - expectedBind: "/var/credentials-fetcher/socket/credentials_fetcher.sock:/var/credentials-fetcher/socket/credentials_fetcher.sock", - }, - { - name: "Credentials Fetcher host from env, Credentials Fetcher from config file", - credentialsFetcherHostFromEnv: "unix:///var/credentials-fetcher/socket/credentials_fetcher.sock.1", - credentialsFetcherHostFromConfigFile: "unix:///var/credentials-fetcher/socket/credentials_fetcher.sock.1", - expectedBind: "/var/credentials-fetcher/socket/credentials_fetcher.sock.1:/var/credentials-fetcher/socket/credentials_fetcher.sock.1", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - os.Setenv("CREDENTIALS_FETCHER_HOST", tc.credentialsFetcherHostFromEnv) - defer os.Unsetenv("CREDENTIALS_FETCHER_HOST") - - bind, _ := getCredentialsFetcherSocketBind() - assert.Equal(t, tc.expectedBind, bind) - }) - } -} - func TestInstanceDomainJoined(t *testing.T) { execCommand = fakeExecCommand execLookPath = func(name string) (string, error) {