From 4557465d956f1c146e2f3281dbb757e1b672f6a1 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Tue, 7 May 2024 09:02:55 -0700 Subject: [PATCH] Use image digest for image pulls (#4157) --- agent/dockerclient/dockerapi/docker_client.go | 38 +++++ .../dockerapi/docker_client_test.go | 108 +++++++++++++ .../dockerapi/mocks/dockerapi_mocks.go | 14 ++ agent/dockerclient/sdkclient/interface.go | 1 + .../sdkclient/mocks/sdkclient_mocks.go | 14 ++ agent/engine/common_test.go | 24 ++- agent/engine/docker_task_engine.go | 75 ++++++++- agent/engine/docker_task_engine_test.go | 152 +++++++++++++++--- agent/engine/engine_integ_test.go | 128 ++++++++++++++- agent/utils/reference/reference.go | 31 ++++ agent/utils/reference/reference_test.go | 64 ++++++++ .../ecs-agent/utils/retry/constant_backoff.go | 33 ++++ ecs-agent/utils/retry/constant_backoff.go | 33 ++++ .../utils/retry/constant_backoff_test.go | 28 ++++ 14 files changed, 704 insertions(+), 39 deletions(-) create mode 100644 agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/utils/retry/constant_backoff.go create mode 100644 ecs-agent/utils/retry/constant_backoff.go create mode 100644 ecs-agent/utils/retry/constant_backoff_test.go diff --git a/agent/dockerclient/dockerapi/docker_client.go b/agent/dockerclient/dockerapi/docker_client.go index 0710521090..60f3e54702 100644 --- a/agent/dockerclient/dockerapi/docker_client.go +++ b/agent/dockerclient/dockerapi/docker_client.go @@ -92,6 +92,10 @@ const ( pullRetryDelayMultiplier = 2 pullRetryJitterMultiplier = 0.2 + // retry settings for tagging images + tagImageRetryAttempts = 5 + tagImageRetryInterval = 100 * time.Millisecond + // pollStatsTimeout is the timeout for polling Docker Stats API; // keeping it same as streaming stats inactivity timeout pollStatsTimeout = 18 * time.Second @@ -129,6 +133,9 @@ type DockerClient interface { // PullImage pulls an image. authData should contain authentication data provided by the ECS backend. PullImage(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) DockerContainerMetadata + // TagImage tags a local image. + TagImage(ctx context.Context, source string, target string) error + // CreateContainer creates a container with the provided Config, HostConfig, and name. A timeout value // and a context should be provided for the request. CreateContainer(context.Context, *dockercontainer.Config, *dockercontainer.HostConfig, string, time.Duration) DockerContainerMetadata @@ -246,6 +253,7 @@ type dockerGoClient struct { context context.Context manifestPullBackoff retry.Backoff imagePullBackoff retry.Backoff + imageTagBackoff retry.Backoff inactivityTimeoutHandler inactivityTimeoutHandlerFunc _time ttime.Time @@ -276,6 +284,7 @@ func (dg *dockerGoClient) WithVersion(version dockerclient.DockerVersion) (Docke config: dg.config, context: dg.context, manifestPullBackoff: dg.manifestPullBackoff, + imageTagBackoff: dg.imageTagBackoff, } // Check if the version is supported _, err := versionedClient.sdkDockerClient() @@ -319,6 +328,7 @@ func NewDockerGoClient(sdkclientFactory sdkclientfactory.Factory, pullRetryJitterMultiplier, pullRetryDelayMultiplier), manifestPullBackoff: retry.NewExponentialBackoff(minimumPullRetryDelay, maximumPullRetryDelay, pullRetryJitterMultiplier, pullRetryDelayMultiplier), + imageTagBackoff: retry.NewConstantBackoff(tagImageRetryInterval), inactivityTimeoutHandler: handleInactivityTimeout, }, nil } @@ -593,6 +603,34 @@ func getRepository(image string) string { return repository } +// TagImage tags a local image. +func (dg *dockerGoClient) TagImage(ctx context.Context, source string, target string) error { + client, err := dg.sdkDockerClient() + if err != nil { + return CannotGetDockerClientError{version: dg.version, err: err} + } + + err = retry.RetryNWithBackoffCtx(ctx, dg.imageTagBackoff, tagImageRetryAttempts, func() error { + if tagErr := client.ImageTag(ctx, source, target); tagErr != nil { + logger.Error("Attempt to tag image failed", logger.Fields{ + "source": source, + "target": target, + field.Error: tagErr, + }) + return tagErr + } + return nil + }) + + if err != nil { + return fmt.Errorf("failed to tag image '%s' as '%s': %w", source, target, err) + } + if ctx.Err() != nil { + return ctx.Err() + } + return nil +} + func (dg *dockerGoClient) InspectImage(image string) (*types.ImageInspect, error) { defer metrics.MetricsEngineGlobal.RecordDockerMetric("INSPECT_IMAGE")() client, err := dg.sdkDockerClient() diff --git a/agent/dockerclient/dockerapi/docker_client_test.go b/agent/dockerclient/dockerapi/docker_client_test.go index c9df9bfd3f..ad269abf55 100644 --- a/agent/dockerclient/dockerapi/docker_client_test.go +++ b/agent/dockerclient/dockerapi/docker_client_test.go @@ -2311,3 +2311,111 @@ func TestListPluginsWithFilter(t *testing.T) { assert.Equal(t, 1, len(pluginNames)) assert.Equal(t, "name2", pluginNames[0]) } + +func TestTagImage(t *testing.T) { + someError := errors.New("some error") + tcs := []struct { + name string + source string + target string + setSDKFactoryExpectations func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) + ctx context.Context + expectedError string + expectedSleeps int + }{ + { + name: "failed to get sdkclient", + setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) { + f.EXPECT().GetDefaultClient().Return(nil, someError) + }, + expectedError: someError.Error(), + expectedSleeps: 0, + }, + { + name: "all attempts exhausted", + source: "source", + target: "target", + setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) { + client := mock_sdkclient.NewMockClient(ctrl) + client.EXPECT(). + ImageTag(gomock.Any(), "source", "target"). + Times(tagImageRetryAttempts). + Return(someError) + f.EXPECT().GetDefaultClient().Return(client, nil) + }, + ctx: context.Background(), + expectedError: "failed to tag image 'source' as 'target': " + someError.Error(), + expectedSleeps: tagImageRetryAttempts - 1, + }, + { + name: "second attempt worked", + source: "source", + target: "target", + setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) { + client := mock_sdkclient.NewMockClient(ctrl) + client.EXPECT().ImageTag(gomock.Any(), "source", "target").Return(someError) + client.EXPECT().ImageTag(gomock.Any(), "source", "target").Return(nil) + f.EXPECT().GetDefaultClient().Return(client, nil) + }, + ctx: context.Background(), + expectedSleeps: 1, + }, + { + name: "canceled context", + setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) { + client := mock_sdkclient.NewMockClient(ctrl) + f.EXPECT().GetDefaultClient().Return(client, nil) + }, + ctx: func() context.Context { + c, cancel := context.WithCancel(context.Background()) + cancel() + return c + }(), + expectedError: "context canceled", + }, + { + name: "deadline exceeded", + setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) { + client := mock_sdkclient.NewMockClient(ctrl) + f.EXPECT().GetDefaultClient().Return(client, nil) + }, + ctx: func() context.Context { + c, cancel := context.WithTimeout(context.Background(), 0) + <-c.Done() // wait for deadline to be exceeded + cancel() + return c + }(), + expectedError: "context deadline exceeded", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // Set up mocks + mockDockerSDK := mock_sdkclient.NewMockClient(ctrl) + mockDockerSDK.EXPECT().Ping(gomock.Any()).Return(types.Ping{}, nil) + sdkFactory := mock_sdkclientfactory.NewMockFactory(ctrl) + sdkFactory.EXPECT().GetDefaultClient().Return(mockDockerSDK, nil) + + // Set up docker client for testing + client, err := NewDockerGoClient(sdkFactory, defaultTestConfig(), context.Background()) + require.NoError(t, err) + // Make retries fast + client.(*dockerGoClient).imageTagBackoff = retry.NewConstantBackoff(0) + + if tc.setSDKFactoryExpectations != nil { + tc.setSDKFactoryExpectations(sdkFactory, ctrl) + } + + err = client.TagImage(tc.ctx, tc.source, tc.target) + if tc.expectedError == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tc.expectedError) + } + }) + } +} diff --git a/agent/dockerclient/dockerapi/mocks/dockerapi_mocks.go b/agent/dockerclient/dockerapi/mocks/dockerapi_mocks.go index 93a38c5554..3e79d9098c 100644 --- a/agent/dockerclient/dockerapi/mocks/dockerapi_mocks.go +++ b/agent/dockerclient/dockerapi/mocks/dockerapi_mocks.go @@ -462,6 +462,20 @@ func (mr *MockDockerClientMockRecorder) SystemPing(arg0, arg1 interface{}) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SystemPing", reflect.TypeOf((*MockDockerClient)(nil).SystemPing), arg0, arg1) } +// TagImage mocks base method. +func (m *MockDockerClient) TagImage(arg0 context.Context, arg1, arg2 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "TagImage", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// TagImage indicates an expected call of TagImage. +func (mr *MockDockerClientMockRecorder) TagImage(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TagImage", reflect.TypeOf((*MockDockerClient)(nil).TagImage), arg0, arg1, arg2) +} + // Version mocks base method. func (m *MockDockerClient) Version(arg0 context.Context, arg1 time.Duration) (string, error) { m.ctrl.T.Helper() diff --git a/agent/dockerclient/sdkclient/interface.go b/agent/dockerclient/sdkclient/interface.go index d7eaf9aa37..99d9941c85 100644 --- a/agent/dockerclient/sdkclient/interface.go +++ b/agent/dockerclient/sdkclient/interface.go @@ -55,6 +55,7 @@ type Client interface { ImagePull(ctx context.Context, refStr string, options types.ImagePullOptions) (io.ReadCloser, error) ImageRemove(ctx context.Context, imageID string, options types.ImageRemoveOptions) ([]types.ImageDeleteResponseItem, error) + ImageTag(ctx context.Context, source, target string) error Ping(ctx context.Context) (types.Ping, error) PluginList(ctx context.Context, filter filters.Args) (types.PluginsListResponse, error) VolumeCreate(ctx context.Context, options volume.CreateOptions) (volume.Volume, error) diff --git a/agent/dockerclient/sdkclient/mocks/sdkclient_mocks.go b/agent/dockerclient/sdkclient/mocks/sdkclient_mocks.go index a863746a79..9c07d9683a 100644 --- a/agent/dockerclient/sdkclient/mocks/sdkclient_mocks.go +++ b/agent/dockerclient/sdkclient/mocks/sdkclient_mocks.go @@ -353,6 +353,20 @@ func (mr *MockClientMockRecorder) ImageRemove(arg0, arg1, arg2 interface{}) *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ImageRemove", reflect.TypeOf((*MockClient)(nil).ImageRemove), arg0, arg1, arg2) } +// ImageTag mocks base method. +func (m *MockClient) ImageTag(arg0 context.Context, arg1, arg2 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ImageTag", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// ImageTag indicates an expected call of ImageTag. +func (mr *MockClientMockRecorder) ImageTag(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ImageTag", reflect.TypeOf((*MockClient)(nil).ImageTag), arg0, arg1, arg2) +} + // Info mocks base method. func (m *MockClient) Info(arg0 context.Context) (types.Info, error) { m.ctrl.T.Helper() diff --git a/agent/engine/common_test.go b/agent/engine/common_test.go index 69ea1808f5..b816a123ac 100644 --- a/agent/engine/common_test.go +++ b/agent/engine/common_test.go @@ -38,6 +38,7 @@ import ( mock_engine "github.com/aws/amazon-ecs-agent/agent/engine/mocks" "github.com/aws/amazon-ecs-agent/agent/statechange" "github.com/aws/amazon-ecs-agent/agent/utils" + referenceutil "github.com/aws/amazon-ecs-agent/agent/utils/reference" apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status" "github.com/aws/amazon-ecs-agent/ecs-agent/api/ecs/model/ecs" apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status" @@ -48,8 +49,11 @@ import ( dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/registry" "github.com/golang/mock/gomock" + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pborman/uuid" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -158,15 +162,29 @@ func validateContainerRunWorkflow(t *testing.T, createdContainerName chan<- string, assertions func(), ) { - // Prepare mock image manifest digest for test + // Prepare a test digest + testDigest, digestParseError := digest.Parse( + "sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b") + require.NoError(t, digestParseError) + + // Get expected canonical reference for the container image and test digest + canonicalImageRef, canonicalRefErr := referenceutil.GetCanonicalRef( + container.Image, testDigest.String()) + require.NoError(t, canonicalRefErr) + + // Set expectations for transition to MANIFEST_PULLED state manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil) manifestPullClient.EXPECT(). PullImageManifest(gomock.Any(), container.Image, container.RegistryAuthentication). - Return(registry.DistributionInspect{}, nil) + Return(registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil) imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - client.EXPECT().PullImage(gomock.Any(), container.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + PullImage(gomock.Any(), canonicalImageRef.String(), nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + TagImage(gomock.Any(), canonicalImageRef.String(), container.Image).Return(nil) imageManager.EXPECT().RecordContainerReference(container).Return(nil) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false) client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil) diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index 2b9c31b869..9c2136ff73 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -82,6 +82,7 @@ const ( ipamCleanupTmeout = 5 * time.Second minEngineConnectRetryDelay = 2 * time.Second maxEngineConnectRetryDelay = 200 * time.Second + tagImageTimeout = 30 * time.Second engineConnectRetryJitterMultiplier = 0.20 engineConnectRetryDelayMultiplier = 1.5 // logDriverTypeFirelens is the log driver type for containers that want to use the firelens container to send logs. @@ -1534,13 +1535,79 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta defer clearCreds() } - metadata := engine.client.PullImage(engine.ctx, container.Image, container.RegistryAuthentication, engine.cfg.ImagePullTimeout) + imageRef := container.Image + imageDigest := container.GetImageDigest() + if imageDigest != "" { + // If image digest is available then prepare a canonical reference to be used for image pull. + // This ensures that the image version referenced by the digest is pulled. + canonicalRef, err := referenceutil.GetCanonicalRef(imageRef, imageDigest) + if err != nil { + logger.Error("Failed to prepare a canonical reference. Cannot pull image.", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: imageRef, + field.ImageDigest: imageDigest, + field.Error: err, + }) + return dockerapi.DockerContainerMetadata{ + Error: dockerapi.CannotPullContainerError{ + FromError: fmt.Errorf( + "failed to prepare a canonical reference with image '%s' and digest '%s': %w", + imageRef, imageDigest, err), + }, + } + } + imageRef = canonicalRef.String() + logger.Info("Prepared a canonical reference for image pull", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: container.Image, + field.ImageDigest: imageDigest, + field.ImageRef: imageRef, + }) + } + + metadata := engine.client.PullImage(engine.ctx, imageRef, container.RegistryAuthentication, engine.cfg.ImagePullTimeout) // Don't add internal images(created by ecs-agent) into imagemanger state if container.IsInternal() { return metadata } pullSucceeded := metadata.Error == nil + + if pullSucceeded && imageRef != container.Image { + // Resolved image manifest digest was used to pull the image. + // Tag the pulled image so that it can be found using the image reference in the task. + ctx, cancel := context.WithTimeout(engine.ctx, tagImageTimeout) + defer cancel() + err := engine.client.TagImage(ctx, imageRef, container.Image) + if err != nil { + logger.Error("Failed to tag image after pull", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: container.Image, + field.ImageRef: imageRef, + field.Error: err, + }) + if errors.Is(err, context.DeadlineExceeded) { + metadata.Error = &dockerapi.DockerTimeoutError{ + Duration: tagImageTimeout, + Transition: "pulled", + } + } else { + metadata.Error = &dockerapi.CannotPullContainerError{FromError: err} + } + pullSucceeded = false + } else { + logger.Info("Successfully tagged image", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: container.Image, + field.ImageRef: imageRef, + }) + } + } + findCachedImage := false if !pullSucceeded { // If Agent failed to pull an image when @@ -1548,11 +1615,11 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta // 2. ImagePullBehavior is not set to always // search the image in local cached images if engine.cfg.DependentContainersPullUpfront.Enabled() && engine.cfg.ImagePullBehavior != config.ImagePullAlwaysBehavior { - if _, err := engine.client.InspectImage(container.Image); err != nil { + if _, err := engine.client.InspectImage(imageRef); err != nil { logger.Error("Failed to find cached image for container", logger.Fields{ field.TaskID: task.GetID(), field.Container: container.Name, - field.Image: container.Image, + field.Image: imageRef, field.Error: err, }) // Stop the task if the container is an essential container, @@ -1566,7 +1633,7 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta logger.Info("Found cached image, use it directly for container", logger.Fields{ field.TaskID: task.GetID(), field.Container: container.Name, - field.Image: container.Image, + field.Image: imageRef, }) findCachedImage = true } diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 30e5c1f0e4..d4607dc24d 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -1544,6 +1544,7 @@ func TestUpdateContainerReference(t *testing.T) { // 5 | local | enabled | prefer-cached // 6 | local | enabled | always func TestPullAndUpdateContainerReference(t *testing.T) { + testDigest := "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966" testcases := []struct { Name string ImagePullUpfront config.BooleanDefaultFalse @@ -1551,8 +1552,10 @@ func TestPullAndUpdateContainerReference(t *testing.T) { ImageState *image.ImageState ImageInspect *types.ImageInspect InspectImage bool + ImageDigest string NumOfPulledContainer int PullImageErr apierrors.NamedError + TagImageErr error }{ { Name: "DependentContainersPullUpfrontEnabledWithRemoteImage", @@ -1616,6 +1619,66 @@ func TestPullAndUpdateContainerReference(t *testing.T) { NumOfPulledContainer: 0, PullImageErr: dockerapi.CannotPullContainerError{fmt.Errorf("error")}, }, + { + Name: "upfront enabled, behavior always, pull success, tag failure", + ImagePullUpfront: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}, + ImagePullBehavior: config.ImagePullAlwaysBehavior, + ImageState: nil, + ImageInspect: nil, + ImageDigest: testDigest, + InspectImage: false, + NumOfPulledContainer: 0, + PullImageErr: nil, + TagImageErr: errors.New("some error"), + }, + { + Name: "upfront enabled, behavior always, pull success, tag timeout", + ImagePullUpfront: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}, + ImagePullBehavior: config.ImagePullAlwaysBehavior, + ImageState: nil, + ImageInspect: nil, + ImageDigest: testDigest, + InspectImage: false, + NumOfPulledContainer: 0, + PullImageErr: nil, + TagImageErr: context.DeadlineExceeded, + }, + { + Name: "upfront enabled, behavior always, pull success, tag success", + ImagePullUpfront: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}, + ImagePullBehavior: config.ImagePullAlwaysBehavior, + ImageState: nil, + ImageInspect: nil, + ImageDigest: testDigest, + InspectImage: false, + NumOfPulledContainer: 1, + PullImageErr: nil, + TagImageErr: nil, + }, + { + Name: "upfront enabled, behavior default, pull success, tag failure", + ImagePullUpfront: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}, + ImagePullBehavior: config.ImagePullDefaultBehavior, + ImageState: nil, + ImageInspect: nil, + ImageDigest: testDigest, + InspectImage: true, + NumOfPulledContainer: 1, + PullImageErr: nil, + TagImageErr: errors.New("some error"), + }, + { + Name: "upfront disabled, behavior default, pull success, tag failure", + ImagePullUpfront: config.BooleanDefaultFalse{Value: config.ExplicitlyDisabled}, + ImagePullBehavior: config.ImagePullDefaultBehavior, + ImageState: nil, + ImageInspect: nil, + ImageDigest: testDigest, + InspectImage: false, + NumOfPulledContainer: 0, + PullImageErr: nil, + TagImageErr: errors.New("some error"), + }, } for _, tc := range testcases { @@ -1634,9 +1697,10 @@ func TestPullAndUpdateContainerReference(t *testing.T) { imageName := "image" taskArn := "taskArn" container := &apicontainer.Container{ - Type: apicontainer.ContainerNormal, - Image: imageName, - Essential: true, + Type: apicontainer.ContainerNormal, + Image: imageName, + Essential: true, + ImageDigest: tc.ImageDigest, } task := &apitask.Task{ @@ -1644,11 +1708,19 @@ func TestPullAndUpdateContainerReference(t *testing.T) { Containers: []*apicontainer.Container{container}, } - client.EXPECT().PullImage(gomock.Any(), imageName, nil, gomock.Any()). + imageRef := imageName + if tc.ImageDigest != "" { + // If image digest exists then it is used to pull the image + imageRef = imageName + "@" + tc.ImageDigest + } + client.EXPECT().PullImage(gomock.Any(), imageRef, nil, gomock.Any()). Return(dockerapi.DockerContainerMetadata{Error: tc.PullImageErr}) if tc.InspectImage { - client.EXPECT().InspectImage(imageName).Return(tc.ImageInspect, nil) + client.EXPECT().InspectImage(imageRef).Return(tc.ImageInspect, nil) + } + if tc.ImageDigest != "" { + client.EXPECT().TagImage(gomock.Any(), imageRef, imageName).Return(tc.TagImageErr) } imageManager.EXPECT().RecordContainerReference(container) @@ -1656,7 +1728,20 @@ func TestPullAndUpdateContainerReference(t *testing.T) { metadata := taskEngine.pullAndUpdateContainerReference(task, container) pulledContainersMap, _ := taskEngine.State().PulledContainerMapByArn(taskArn) require.Len(t, pulledContainersMap, tc.NumOfPulledContainer) - assert.Equal(t, dockerapi.DockerContainerMetadata{Error: tc.PullImageErr}, + var expectedErr apierrors.NamedError + if tc.PullImageErr != nil { + expectedErr = tc.PullImageErr + } else if tc.TagImageErr != nil { + if tc.TagImageErr == context.DeadlineExceeded { + expectedErr = &dockerapi.DockerTimeoutError{ + Duration: tagImageTimeout, + Transition: "pulled", + } + } else { + expectedErr = &dockerapi.CannotPullContainerError{FromError: tc.TagImageErr} + } + } + assert.Equal(t, dockerapi.DockerContainerMetadata{Error: expectedErr}, metadata, "expected metadata with error") }) } @@ -4103,6 +4188,7 @@ func TestPullContainerManifest(t *testing.T) { // This function simulates the various scenarios for transition to MANIFEST_PULLED state // where the task should complete its lifecycle. func TestManifestPullTaskShouldContinue(t *testing.T) { + testImage := "my.repo/repo/image" testDigest, err := digest.Parse("sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b") require.NoError(t, err) type testcase struct { @@ -4112,13 +4198,14 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { setManifestPulledExpectations func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call - shouldPullImage bool + shouldPullImage bool + shouldPullWithoutCanonicalRef bool } tcs := []testcase{ { name: "task should continue if manifest pull succeeds and pull behavior is default", imagePullBehavior: config.ImagePullDefaultBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { @@ -4128,7 +4215,7 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { WithVersion(dockerclient.Version_1_35). Return(manifestPullClient, nil), manifestPullClient.EXPECT(). - PullImageManifest(gomock.Any(), "myimage", nil). + PullImageManifest(gomock.Any(), testImage, nil). Return( registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil), @@ -4139,7 +4226,7 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { { name: "task should continue if manifest pull fails and pull behavior is default", imagePullBehavior: config.ImagePullDefaultBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { @@ -4149,26 +4236,27 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { WithVersion(dockerclient.Version_1_35). Return(manifestPullClient, nil), manifestPullClient.EXPECT(). - PullImageManifest(gomock.Any(), "myimage", nil). + PullImageManifest(gomock.Any(), testImage, nil). Return(registry.DistributionInspect{}, dockerapi.CannotPullImageManifestError{ FromError: errors.New("some error"), }), } }, - shouldPullImage: true, + shouldPullImage: true, + shouldPullWithoutCanonicalRef: true, }, { name: "task should continue if manifest pull succeeds and pull behavior is prefer-cached", imagePullBehavior: config.ImagePullPreferCachedBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { inspectResult := &types.ImageInspect{ - RepoDigests: []string{"myimage@" + testDigest.String()}, + RepoDigests: []string{testImage + "@" + testDigest.String()}, } return []*gomock.Call{ - c.EXPECT().InspectImage("myimage").Times(2).Return(inspectResult, nil), + c.EXPECT().InspectImage(testImage).Times(2).Return(inspectResult, nil), } }, shouldPullImage: false, @@ -4176,29 +4264,30 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { { name: "task should continue if manifest pull fails and pull behavior is prefer-cached", imagePullBehavior: config.ImagePullPreferCachedBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) return []*gomock.Call{ - c.EXPECT().InspectImage("myimage").Return(nil, errors.New("some error")), + c.EXPECT().InspectImage(testImage).Return(nil, errors.New("some error")), c.EXPECT(). WithVersion(dockerclient.Version_1_35). Return(manifestPullClient, nil), manifestPullClient.EXPECT(). - PullImageManifest(gomock.Any(), "myimage", nil). + PullImageManifest(gomock.Any(), testImage, nil). Return(registry.DistributionInspect{}, dockerapi.CannotPullImageManifestError{ FromError: errors.New("some error"), }), } }, - shouldPullImage: true, + shouldPullImage: true, + shouldPullWithoutCanonicalRef: true, }, { name: "task should continue if manifest pull succeeds and pull behavior is always", imagePullBehavior: config.ImagePullAlwaysBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { @@ -4208,7 +4297,7 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { WithVersion(dockerclient.Version_1_35). Return(manifestPullClient, nil), manifestPullClient.EXPECT(). - PullImageManifest(gomock.Any(), "myimage", nil). + PullImageManifest(gomock.Any(), testImage, nil). Return( registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil), @@ -4219,18 +4308,18 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { { name: "task should continue if manifest pull succeeds and pull behavior is once", imagePullBehavior: config.ImagePullOnceBehavior, - container: &apicontainer.Container{Image: "myimage", Name: "container"}, + container: &apicontainer.Container{Image: testImage, Name: "container"}, setManifestPulledExpectations: func( ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, ) []*gomock.Call { inspectResult := &types.ImageInspect{ - RepoDigests: []string{"myimage@" + testDigest.String()}, + RepoDigests: []string{testImage + "@" + testDigest.String()}, } return []*gomock.Call{ i.EXPECT(). - GetImageStateFromImageName("myimage"). + GetImageStateFromImageName(testImage). Return(&image.ImageState{PullSucceeded: true}, true), - c.EXPECT().InspectImage("myimage").Return(inspectResult, nil), + c.EXPECT().InspectImage(testImage).Return(inspectResult, nil), } }, shouldPullImage: false, @@ -4305,11 +4394,22 @@ func TestManifestPullTaskShouldContinue(t *testing.T) { } } if tc.shouldPullImage { + expectedPullRef := tc.container.Image + if !tc.shouldPullWithoutCanonicalRef { + expectedPullRef = tc.container.Image + "@" + testDigest.String() + } transitionExpectations = append(transitionExpectations, dockerClient.EXPECT(). - PullImage(gomock.Any(), tc.container.Image, nil, gomock.Any()). + PullImage(gomock.Any(), expectedPullRef, nil, gomock.Any()). Return(dockerapi.DockerContainerMetadata{}), ) + if !tc.shouldPullWithoutCanonicalRef { + transitionExpectations = append(transitionExpectations, + dockerClient.EXPECT(). + TagImage(gomock.Any(), expectedPullRef, tc.container.Image). + Return(nil), + ) + } } transitionExpectations = append(transitionExpectations, imageManager.EXPECT().RecordContainerReference(tc.container).Return(nil), diff --git a/agent/engine/engine_integ_test.go b/agent/engine/engine_integ_test.go index 9081a7f83f..387fde6082 100644 --- a/agent/engine/engine_integ_test.go +++ b/agent/engine/engine_integ_test.go @@ -605,14 +605,18 @@ func TestManifestPulledDoesNotDependOnContainerOrdering(t *testing.T) { t.Run(fmt.Sprintf("%v", behavior), func(t *testing.T) { cfg := defaultTestConfigIntegTest() cfg.ImagePullBehavior = behavior + cfg.DockerStopTimeout = 100 * time.Millisecond taskEngine, done, _ := setup(cfg, nil, t) defer done() - first := createTestContainerWithImageAndName(testRegistryImage, "first") - first.Command = getLongRunningCommand() + image := "public.ecr.aws/docker/library/busybox:1.36.1" + first := createTestContainerWithImageAndName(image, "first") + first.Command = []string{"sh", "-c", "sleep 60"} - second := createTestContainerWithImageAndName(testRegistryImage, "second") - second.SetDependsOn([]apicontainer.DependsOn{{ContainerName: first.Name, Condition: "COMPLETE"}}) + second := createTestContainerWithImageAndName(image, "second") + second.SetDependsOn([]apicontainer.DependsOn{ + {ContainerName: first.Name, Condition: "COMPLETE"}, + }) task := &apitask.Task{ Arn: "test-arn", @@ -632,8 +636,19 @@ func TestManifestPulledDoesNotDependOnContainerOrdering(t *testing.T) { assert.Equal(t, apicontainerstatus.ContainerManifestPulled, second.GetKnownStatus()) // Assert that both containers have the right image digest populated - assert.NotEmpty(t, first.GetImageDigest()) - assert.NotEmpty(t, second.GetImageDigest()) + expectedDigest := "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966" + assert.Equal(t, expectedDigest, first.GetImageDigest()) + assert.Equal(t, expectedDigest, second.GetImageDigest()) + + // Cleanup + first.SetDesiredStatus(apicontainerstatus.ContainerStopped) + second.SetDesiredStatus(apicontainerstatus.ContainerStopped) + verifyContainerStoppedStateChange(t, taskEngine) + verifyContainerStoppedStateChange(t, taskEngine) + verifyTaskStoppedStateChange(t, taskEngine) + taskEngine.(*DockerTaskEngine).removeContainer(task, first) + taskEngine.(*DockerTaskEngine).removeContainer(task, second) + removeImage(t, image) }) } } @@ -715,3 +730,104 @@ func TestPullContainerManifestInteg(t *testing.T) { } } } + +// Tests pullContainer method pulls container image as expected with and without an image +// digest populated on the container. If an image digest is populated then pullContainer +// uses the digest to prepare a canonical reference for the image to pull the image version +// referenced by the digest. +func TestPullContainerWithAndWithoutDigestInteg(t *testing.T) { + tcs := []struct { + name string + image string + imageDigest string + }{ + { + name: "no tag no digest", + image: "public.ecr.aws/docker/library/alpine", + imageDigest: "", + }, + { + name: "tag but no digest", + image: "public.ecr.aws/docker/library/alpine:latest", + imageDigest: "", + }, + { + name: "no tag with digest", + image: "public.ecr.aws/docker/library/alpine", + imageDigest: "sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b", + }, + { + name: "tag with digest", + image: "public.ecr.aws/docker/library/alpine:3.19", + imageDigest: "sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + // Prepare task + task := &apitask.Task{Containers: []*apicontainer.Container{{Image: tc.image}}} + container := task.Containers[0] + container.SetImageDigest(tc.imageDigest) + + // Prepare task engine + cfg := defaultTestConfigIntegTest() + cfg.ImagePullBehavior = config.ImagePullAlwaysBehavior + taskEngine, done, _ := setup(cfg, nil, t) + defer done() + dockerClient := taskEngine.(*DockerTaskEngine).client + + // Remove image from the host if it exists to start from a clean slate + removeImage(t, container.Image) + + // Pull the image + pullRes := taskEngine.(*DockerTaskEngine).pullContainer(task, container) + require.NoError(t, pullRes.Error) + + // Check that the image was pulled + _, err := dockerClient.InspectImage(container.Image) + require.NoError(t, err) + + // Cleanup + removeImage(t, container.Image) + }) + } +} + +// Tests that pullContainer pulls the same image when a digest is used versus when a digest +// is not used. +func TestPullContainerWithAndWithoutDigestConsistency(t *testing.T) { + image := "public.ecr.aws/docker/library/alpine:3.19" + imageDigest := "sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b" + + // Prepare task + task := &apitask.Task{Containers: []*apicontainer.Container{{Image: image}}} + container := task.Containers[0] + + // Prepare task engine + cfg := defaultTestConfigIntegTest() + cfg.ImagePullBehavior = config.ImagePullAlwaysBehavior + taskEngine, done, _ := setup(cfg, nil, t) + defer done() + dockerClient := taskEngine.(*DockerTaskEngine).client + + // Remove image from the host if it exists to start from a clean slate + removeImage(t, container.Image) + + // Pull the image without digest + pullRes := taskEngine.(*DockerTaskEngine).pullContainer(task, container) + require.NoError(t, pullRes.Error) + inspectWithoutDigest, err := dockerClient.InspectImage(container.Image) + require.NoError(t, err) + removeImage(t, container.Image) + + // Pull the image with digest + container.SetImageDigest(imageDigest) + pullRes = taskEngine.(*DockerTaskEngine).pullContainer(task, container) + require.NoError(t, pullRes.Error) + inspectWithDigest, err := dockerClient.InspectImage(container.Image) + require.NoError(t, err) + removeImage(t, container.Image) + + // Image should be the same + assert.Equal(t, inspectWithDigest.ID, inspectWithoutDigest.ID) +} diff --git a/agent/utils/reference/reference.go b/agent/utils/reference/reference.go index 43dbe2423d..a391f5ff28 100644 --- a/agent/utils/reference/reference.go +++ b/agent/utils/reference/reference.go @@ -78,3 +78,34 @@ func GetDigestFromRepoDigests(repoDigests []string, imageRef string) (digest.Dig return "", fmt.Errorf("found no repo digest matching '%s'", imageRef) } + +// Given an image reference and a manifest digest string, returns a canonical reference +// for the image. +// If the image reference has a digest then the canonical reference will still use the provided +// manifest digest overwriting the exiting digest in the image reference. +func GetCanonicalRef(imageRef string, manifestDigest string) (reference.Canonical, error) { + parsedDigest, err := digest.Parse(manifestDigest) + if err != nil { + return nil, fmt.Errorf("failed to parse image digest '%s': %w", manifestDigest, err) + } + + parsedImageRef, err := reference.Parse(imageRef) + if err != nil { + return nil, fmt.Errorf( + "failed to parse image reference '%s': %w", imageRef, err) + } + namedImageRef, ok := parsedImageRef.(reference.Named) + if !ok { + return nil, fmt.Errorf("image reference '%s' is not a named reference, parsed as: %v", + imageRef, parsedImageRef) + } + + canonicalRef, err := reference.WithDigest(namedImageRef, parsedDigest) + if err != nil { + return nil, fmt.Errorf( + "failed to produce a canonical reference using named reference '%v' and digest '%v': %w", + namedImageRef, parsedDigest, err) + } + + return canonicalRef, nil +} diff --git a/agent/utils/reference/reference_test.go b/agent/utils/reference/reference_test.go index 7e06ba4539..7910d575bf 100644 --- a/agent/utils/reference/reference_test.go +++ b/agent/utils/reference/reference_test.go @@ -150,3 +150,67 @@ func TestGetDigestFromRepoDigests(t *testing.T) { }) } } + +func TestGetCanonicalRef(t *testing.T) { + tcs := []struct { + name string + imageRef string + manifestDigest string + expected string + expectedError string + }{ + { + name: "invalid digest", + imageRef: "alpine", + manifestDigest: "invalid digest", + expectedError: "failed to parse image digest 'invalid digest': invalid checksum digest format", + }, + { + name: "invalid image reference format", + imageRef: "invalid reference", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expectedError: "failed to parse image reference 'invalid reference': invalid reference format", + }, + { + name: "no tag", + imageRef: "alpine", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expected: "alpine@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + }, + { + name: "has tag", + imageRef: "alpine:latest", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expected: "alpine:latest@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + }, + { + name: "image reference's digest is overwritten", + imageRef: "alpine@sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expected: "alpine@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + }, + { + name: "no tag ecr", + imageRef: "public.ecr.aws/library/alpine", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expected: "public.ecr.aws/library/alpine@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + }, + { + name: "has tag ecr", + imageRef: "public.ecr.aws/library/alpine:latest", + manifestDigest: "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + expected: "public.ecr.aws/library/alpine:latest@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + canonicalRef, err := GetCanonicalRef(tc.imageRef, tc.manifestDigest) + if tc.expectedError == "" { + require.NoError(t, err) + assert.Equal(t, tc.expected, canonicalRef.String()) + } else { + assert.EqualError(t, err, tc.expectedError) + } + }) + } +} diff --git a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/utils/retry/constant_backoff.go b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/utils/retry/constant_backoff.go new file mode 100644 index 0000000000..0198c3dcfc --- /dev/null +++ b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/utils/retry/constant_backoff.go @@ -0,0 +1,33 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package retry + +import "time" + +// A simple backoff strategy that backs off the same amount of time for each iteration. +type ConstantBackoff struct { + interval time.Duration +} + +func NewConstantBackoff(interval time.Duration) ConstantBackoff { + return ConstantBackoff{interval: interval} +} + +func (cb ConstantBackoff) Duration() time.Duration { + return cb.interval +} + +func (cb ConstantBackoff) Reset() { + // Nothing to reset +} diff --git a/ecs-agent/utils/retry/constant_backoff.go b/ecs-agent/utils/retry/constant_backoff.go new file mode 100644 index 0000000000..0198c3dcfc --- /dev/null +++ b/ecs-agent/utils/retry/constant_backoff.go @@ -0,0 +1,33 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package retry + +import "time" + +// A simple backoff strategy that backs off the same amount of time for each iteration. +type ConstantBackoff struct { + interval time.Duration +} + +func NewConstantBackoff(interval time.Duration) ConstantBackoff { + return ConstantBackoff{interval: interval} +} + +func (cb ConstantBackoff) Duration() time.Duration { + return cb.interval +} + +func (cb ConstantBackoff) Reset() { + // Nothing to reset +} diff --git a/ecs-agent/utils/retry/constant_backoff_test.go b/ecs-agent/utils/retry/constant_backoff_test.go new file mode 100644 index 0000000000..ccabcdaff0 --- /dev/null +++ b/ecs-agent/utils/retry/constant_backoff_test.go @@ -0,0 +1,28 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package retry + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestConstantBackoff(t *testing.T) { + backoff := NewConstantBackoff(2 * time.Second) + for i := 0; i < 10; i++ { + assert.Equal(t, 2*time.Second, backoff.Duration()) + } +}