diff --git a/README.md b/README.md index 6107d9e9218..699aa6a2faf 100644 --- a/README.md +++ b/README.md @@ -199,6 +199,7 @@ additional details on each available environment variable. | `ECS_APPARMOR_CAPABLE` | `true` | Whether AppArmor is available on the container instance. | `false` | `false` | | `ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION` | 10m | Default time to wait to delete containers for a stopped task (see also `ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION_JITTER`). If set to less than 1 second, the value is ignored. | 3h | 3h | | `ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION_JITTER` | 1h | Jitter value for the task engine cleanup wait duration. When specified, the actual cleanup wait duration time for each task will be the duration specified in `ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION` plus a random duration between 0 and the jitter duration. | blank | blank | +| `ECS_MANIFEST_PULL_TIMEOUT` | 10m | Timeout before giving up on fetching image manifest for a container image. | 1m | 1m | | `ECS_CONTAINER_STOP_TIMEOUT` | 10m | Instance scoped configuration for time to wait for the container to exit normally before being forcibly killed. | 30s | 30s | | `ECS_CONTAINER_START_TIMEOUT` | 10m | Timeout before giving up on starting a container. | 3m | 8m | | `ECS_CONTAINER_CREATE_TIMEOUT` | 10m | Timeout before giving up on creating a container. Minimum value is 1m. If user sets a value below minimum it will be set to min. | 4m | 4m | diff --git a/agent/api/container/container.go b/agent/api/container/container.go index c77dab614e7..493fcb97fe5 100644 --- a/agent/api/container/container.go +++ b/agent/api/container/container.go @@ -23,6 +23,7 @@ import ( "time" resourcestatus "github.com/aws/amazon-ecs-agent/agent/taskresource/status" + referenceutil "github.com/aws/amazon-ecs-agent/agent/utils/reference" apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status" apierrors "github.com/aws/amazon-ecs-agent/ecs-agent/api/errors" "github.com/aws/amazon-ecs-agent/ecs-agent/credentials" @@ -1521,3 +1522,17 @@ func (c *Container) GetImageName() string { containerImage := strings.Split(c.Image, ":")[0] return containerImage } + +// Checks if the container has a resolved image manifest digest. +// Always returns false for internal containers as those are out-of-scope of digest resolution. +func (c *Container) DigestResolved() bool { + return !c.IsInternal() && c.GetImageDigest() != "" +} + +// Checks if the container's image requires manifest digest resolution. +// Manifest digest resolution is required if the container's image reference does not +// have a digest. +// Always returns false for internal containers as those are out-of-scope of digest resolution. +func (c *Container) DigestResolutionRequired() bool { + return !c.IsInternal() && referenceutil.GetDigestFromImageRef(c.Image) == "" +} diff --git a/agent/api/container/container_test.go b/agent/api/container/container_test.go index 8ee4f036cbc..5252bfefdba 100644 --- a/agent/api/container/container_test.go +++ b/agent/api/container/container_test.go @@ -1342,3 +1342,28 @@ func getContainer(hostConfig string, credentialSpecs []string) *Container { c.CredentialSpecs = credentialSpecs return c } + +func TestDigestResolved(t *testing.T) { + t.Run("never resolved for internal container", func(t *testing.T) { + assert.False(t, (&Container{Type: ContainerServiceConnectRelay}).DigestResolved()) + }) + t.Run("digest resolved if it is populated", func(t *testing.T) { + assert.True(t, (&Container{ImageDigest: "digest"}).DigestResolved()) + }) + t.Run("digest not resolved if it is not populated", func(t *testing.T) { + assert.False(t, (&Container{}).DigestResolved()) + }) +} + +func TestDigestResolutionRequired(t *testing.T) { + t.Run("never required for internal containers", func(t *testing.T) { + assert.False(t, (&Container{Type: ContainerServiceConnectRelay}).DigestResolutionRequired()) + }) + t.Run("required if not found in image reference", func(t *testing.T) { + assert.True(t, (&Container{Image: "alpine"}).DigestResolutionRequired()) + }) + t.Run("not required if found in image reference", func(t *testing.T) { + image := "ubuntu@sha256:ed6d2c43c8fbcd3eaa44c9dab6d94cb346234476230dc1681227aa72d07181ee" + assert.False(t, (&Container{Image: image}).DigestResolutionRequired()) + }) +} diff --git a/agent/api/statechange.go b/agent/api/statechange.go index 7df2b984082..c937541ba12 100644 --- a/agent/api/statechange.go +++ b/agent/api/statechange.go @@ -130,7 +130,7 @@ func NewTaskStateChangeEvent(task *apitask.Task, reason string) (TaskStateChange return event, ErrShouldNotSendEvent{task.Arn} } taskKnownStatus := task.GetKnownStatus() - if !taskKnownStatus.BackendRecognized() { + if taskKnownStatus != apitaskstatus.TaskManifestPulled && !taskKnownStatus.BackendRecognized() { return event, errors.Errorf( "create task state change event api: status not recognized by ECS: %v", taskKnownStatus) @@ -140,6 +140,14 @@ func NewTaskStateChangeEvent(task *apitask.Task, reason string) (TaskStateChange "create task state change event api: status [%s] already sent", taskKnownStatus.String()) } + if taskKnownStatus == apitaskstatus.TaskManifestPulled && !task.HasAContainerWithResolvedDigest() { + return event, ErrShouldNotSendEvent{ + fmt.Sprintf( + "create task state change event api: status %s not eligible for backend reporting as"+ + " no digests were resolved", + apitaskstatus.TaskManifestPulled.String()), + } + } event = TaskStateChange{ TaskARN: task.Arn, @@ -161,11 +169,21 @@ func NewContainerStateChangeEvent(task *apitask.Task, cont *apicontainer.Contain return event, err } contKnownStatus := cont.GetKnownStatus() - if !contKnownStatus.ShouldReportToBackend(cont.GetSteadyStateStatus()) { + if contKnownStatus != apicontainerstatus.ContainerManifestPulled && + !contKnownStatus.ShouldReportToBackend(cont.GetSteadyStateStatus()) { return event, ErrShouldNotSendEvent{fmt.Sprintf( "create container state change event api: status not recognized by ECS: %v", contKnownStatus)} } + if contKnownStatus == apicontainerstatus.ContainerManifestPulled && !cont.DigestResolved() { + // Transition to MANIFEST_PULLED state is sent to the backend only to report a resolved + // image manifest digest. No need to generate an event if the digest was not resolved + // which could happen due to various reasons. + return event, ErrShouldNotSendEvent{fmt.Sprintf( + "create container state change event api:"+ + " no need to send %s event as no resolved digests were found", + apicontainerstatus.ContainerManifestPulled.String())} + } if cont.GetSentStatus() >= contKnownStatus { return event, ErrShouldNotSendEvent{fmt.Sprintf( "create container state change event api: status [%s] already sent for container %s, task %s", @@ -196,7 +214,7 @@ func newUncheckedContainerStateChangeEvent(task *apitask.Task, cont *apicontaine TaskArn: task.Arn, ContainerName: cont.Name, RuntimeID: cont.GetRuntimeID(), - Status: contKnownStatus.BackendStatus(cont.GetSteadyStateStatus()), + Status: containerStatusChangeStatus(contKnownStatus, cont.GetSteadyStateStatus()), ExitCode: cont.GetKnownExitCode(), PortBindings: portBindings, ImageDigest: cont.GetImageDigest(), @@ -206,6 +224,27 @@ func newUncheckedContainerStateChangeEvent(task *apitask.Task, cont *apicontaine return event, nil } +// Maps container known status to a suitable status for ContainerStateChange. +// +// Returns ContainerRunning if known status matches steady state status, +// returns knownStatus if it is ContainerManifestPulled or ContainerStopped, +// returns ContainerStatusNone for all other cases. +func containerStatusChangeStatus( + knownStatus apicontainerstatus.ContainerStatus, + steadyStateStatus apicontainerstatus.ContainerStatus, +) apicontainerstatus.ContainerStatus { + switch knownStatus { + case steadyStateStatus: + return apicontainerstatus.ContainerRunning + case apicontainerstatus.ContainerManifestPulled: + return apicontainerstatus.ContainerManifestPulled + case apicontainerstatus.ContainerStopped: + return apicontainerstatus.ContainerStopped + default: + return apicontainerstatus.ContainerStatusNone + } +} + // NewManagedAgentChangeEvent creates a new managedAgent change event to convey managed agent state changes // returns error if the state change doesn't need to be sent to the ECS backend. func NewManagedAgentChangeEvent(task *apitask.Task, cont *apicontainer.Container, managedAgentName string, reason string) (ManagedAgentStateChange, error) { @@ -322,24 +361,6 @@ func (change *TaskStateChange) SetTaskTimestamps() { } } -// ShouldBeReported checks if the statechange should be reported to backend -func (change *TaskStateChange) ShouldBeReported() bool { - // Events that should be reported: - // 1. Normal task state change: RUNNING/STOPPED - // 2. Container state change, with task status in CREATED/RUNNING/STOPPED - // The task timestamp will be sent in both of the event type - // TODO Move the Attachment statechange check into this method - if change.Status == apitaskstatus.TaskRunning || change.Status == apitaskstatus.TaskStopped { - return true - } - - if len(change.Containers) != 0 { - return true - } - - return false -} - func (change *TaskStateChange) ToFields() logger.Fields { fields := logger.Fields{ "eventType": "TaskStateChange", @@ -494,8 +515,11 @@ func buildContainerStateChangePayload(change ContainerStateChange) (*ecsmodel.Co statechange.ImageDigest = aws.String(change.ImageDigest) } - stat := change.Status.String() - if stat != apicontainerstatus.ContainerStopped.String() && stat != apicontainerstatus.ContainerRunning.String() { + // TODO: This check already exists in NewContainerStateChangeEvent and shouldn't be repeated here; remove after verifying + stat := change.Status + if stat != apicontainerstatus.ContainerManifestPulled && + stat != apicontainerstatus.ContainerStopped && + stat != apicontainerstatus.ContainerRunning { logger.Warn("Not submitting unsupported upstream container state", logger.Fields{ field.Status: stat, field.ContainerName: change.ContainerName, @@ -503,10 +527,11 @@ func buildContainerStateChangePayload(change ContainerStateChange) (*ecsmodel.Co }) return nil, nil } - if stat == "DEAD" { - stat = apicontainerstatus.ContainerStopped.String() + // TODO: This check is probably redundant as String() method never returns "DEAD"; remove after verifying + if stat.String() == "DEAD" { + stat = apicontainerstatus.ContainerStopped } - statechange.Status = aws.String(stat) + statechange.Status = aws.String(stat.BackendStatusString()) if change.ExitCode != nil { exitCode := int64(aws.IntValue(change.ExitCode)) diff --git a/agent/api/statechange_test.go b/agent/api/statechange_test.go index 11fe5f3b506..ab60047158f 100644 --- a/agent/api/statechange_test.go +++ b/agent/api/statechange_test.go @@ -27,51 +27,13 @@ import ( "github.com/aws/amazon-ecs-agent/agent/engine/execcmd" apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status" "github.com/aws/amazon-ecs-agent/ecs-agent/api/ecs/model/ecs" + ecsmodel "github.com/aws/amazon-ecs-agent/ecs-agent/api/ecs/model/ecs" apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status" "github.com/aws/aws-sdk-go/aws" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestShouldBeReported(t *testing.T) { - cases := []struct { - status apitaskstatus.TaskStatus - containerChange []ContainerStateChange - result bool - }{ - { // Normal task state change to running - status: apitaskstatus.TaskRunning, - result: true, - }, - { // Normal task state change to stopped - status: apitaskstatus.TaskStopped, - result: true, - }, - { // Container changed while task is not in steady state - status: apitaskstatus.TaskCreated, - containerChange: []ContainerStateChange{ - {TaskArn: "taskarn"}, - }, - result: true, - }, - { // No container change and task status not recognized - status: apitaskstatus.TaskCreated, - result: false, - }, - } - - for _, tc := range cases { - t.Run(fmt.Sprintf("task change status: %s, container change: %t", tc.status, len(tc.containerChange) > 0), - func(t *testing.T) { - taskChange := TaskStateChange{ - Status: tc.status, - Containers: tc.containerChange, - } - - assert.Equal(t, tc.result, taskChange.ShouldBeReported()) - }) - } -} - func TestSetTaskTimestamps(t *testing.T) { t1 := time.Now() t2 := t1.Add(time.Second) @@ -495,3 +457,374 @@ func getTestContainerStateChange() ContainerStateChange { return testContainerStateChange } + +func TestNewTaskStateChangeEvent(t *testing.T) { + tcs := []struct { + name string + task *apitask.Task + reason string + expected TaskStateChange + expectedError string + }{ + { + name: "internal tasks are never reported", + task: &apitask.Task{IsInternal: true, Arn: "arn"}, + expectedError: "should not send events for internal tasks or containers: arn", + }, + { + name: "manifest_pulled state is not reported if there are no resolved digests", + task: &apitask.Task{ + Arn: "arn", + KnownStatusUnsafe: apitaskstatus.TaskManifestPulled, + Containers: []*apicontainer.Container{ + {ImageDigest: ""}, + {ImageDigest: "digest", Type: apicontainer.ContainerCNIPause}, + }, + }, + expectedError: "should not send events for internal tasks or containers:" + + " create task state change event api: status MANIFEST_PULLED not eligible" + + " for backend reporting as no digests were resolved", + }, + { + name: "manifest_pulled state is reported", + task: &apitask.Task{ + Arn: "arn", + KnownStatusUnsafe: apitaskstatus.TaskManifestPulled, + Containers: []*apicontainer.Container{{ImageDigest: "digest"}}, + }, + expected: TaskStateChange{TaskARN: "arn", Status: apitaskstatus.TaskManifestPulled}, + }, + { + name: "created state is not reported", + task: &apitask.Task{Arn: "arn", KnownStatusUnsafe: apitaskstatus.TaskCreated}, + expectedError: "create task state change event api: status not recognized by ECS: CREATED", + }, + { + name: "running state is reported", + task: &apitask.Task{Arn: "arn", KnownStatusUnsafe: apitaskstatus.TaskRunning}, + expected: TaskStateChange{TaskARN: "arn", Status: apitaskstatus.TaskRunning}, + }, + { + name: "stopped state is reported", + task: &apitask.Task{Arn: "arn", KnownStatusUnsafe: apitaskstatus.TaskRunning}, + reason: "container stopped", + expected: TaskStateChange{ + TaskARN: "arn", Status: apitaskstatus.TaskRunning, Reason: "container stopped", + }, + }, + { + name: "already sent status is not reported again", + task: &apitask.Task{ + Arn: "arn", + KnownStatusUnsafe: apitaskstatus.TaskManifestPulled, + SentStatusUnsafe: apitaskstatus.TaskManifestPulled, + }, + expectedError: "create task state change event api: status [MANIFEST_PULLED] already sent", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + res, err := NewTaskStateChangeEvent(tc.task, tc.reason) + if tc.expectedError == "" { + require.NoError(t, err) + tc.expected.Task = tc.task + assert.Equal(t, tc.expected, res) + } else { + assert.EqualError(t, err, tc.expectedError) + } + }) + } +} + +func TestNewContainerStateChangeEvent(t *testing.T) { + tcs := []struct { + name string + task *apitask.Task + reason string + expected ContainerStateChange + expectedError string + }{ + { + name: "internal containers are not reported", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + {Name: "container", Type: apicontainer.ContainerCNIPause}, + }, + }, + expectedError: "should not send events for internal tasks or containers: container", + }, + { + name: "MANIFEST_PULLED state is reported if digest was resolved", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "digest", + KnownStatusUnsafe: apicontainerstatus.ContainerManifestPulled, + }, + }, + }, + expected: ContainerStateChange{ + TaskArn: "arn", + ContainerName: "container", + Status: apicontainerstatus.ContainerManifestPulled, + ImageDigest: "digest", + }, + }, + { + name: "MANIFEST_PULLED state not is not reported if digest was not resolved", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "", + KnownStatusUnsafe: apicontainerstatus.ContainerManifestPulled, + }, + }, + }, + expectedError: "should not send events for internal tasks or containers:" + + " create container state change event api:" + + " no need to send MANIFEST_PULLED event" + + " as no resolved digests were found", + }, + { + name: "PULLED state is not reported", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "digest", + KnownStatusUnsafe: apicontainerstatus.ContainerPulled, + }, + }, + }, + expectedError: "should not send events for internal tasks or containers:" + + " create container state change event api: " + + "status not recognized by ECS: PULLED", + }, + { + name: "RUNNING state is reported", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "digest", + KnownStatusUnsafe: apicontainerstatus.ContainerRunning, + }, + }, + }, + expected: ContainerStateChange{ + TaskArn: "arn", + ContainerName: "container", + Status: apicontainerstatus.ContainerRunning, + ImageDigest: "digest", + }, + }, + { + name: "STOPPED state is reported", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "digest", + KnownStatusUnsafe: apicontainerstatus.ContainerStopped, + }, + }, + }, + reason: "container stopped", + expected: ContainerStateChange{ + TaskArn: "arn", + ContainerName: "container", + Status: apicontainerstatus.ContainerStopped, + ImageDigest: "digest", + Reason: "container stopped", + }, + }, + { + name: "already sent state is not reported again", + task: &apitask.Task{ + Arn: "arn", + Containers: []*apicontainer.Container{ + { + Name: "container", + ImageDigest: "digest", + KnownStatusUnsafe: apicontainerstatus.ContainerRunning, + SentStatusUnsafe: apicontainerstatus.ContainerRunning, + }, + }, + }, + expectedError: "should not send events for internal tasks or containers:" + + " create container state change event api:" + + " status [RUNNING] already sent for container container, task arn", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + res, err := NewContainerStateChangeEvent(tc.task, tc.task.Containers[0], tc.reason) + if tc.expectedError == "" { + require.NoError(t, err) + tc.expected.Container = tc.task.Containers[0] + assert.Equal(t, tc.expected, res) + } else { + assert.EqualError(t, err, tc.expectedError) + } + }) + } +} + +func TestContainerStatusChangeStatus(t *testing.T) { + // Mapped status is ContainerStatusNone when container status is ContainerStatusNone + var containerStatus apicontainerstatus.ContainerStatus + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerStatusNone) + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerStatusNone) + + // Mapped status is ContainerManifestPulled when container status is ContainerManifestPulled + containerStatus = apicontainerstatus.ContainerManifestPulled + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerManifestPulled) + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerManifestPulled) + + // Mapped status is ContainerStatusNone when container status is ContainerPulled + containerStatus = apicontainerstatus.ContainerPulled + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerStatusNone) + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerStatusNone) + + // Mapped status is ContainerStatusNone when container status is ContainerCreated + containerStatus = apicontainerstatus.ContainerCreated + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerStatusNone) + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerStatusNone) + + containerStatus = apicontainerstatus.ContainerRunning + // Mapped status is ContainerRunning when container status is ContainerRunning + // and steady state is ContainerRunning + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerRunning) + // Mapped status is ContainerStatusNone when container status is ContainerRunning + // and steady state is ContainerResourcesProvisioned + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerStatusNone) + + containerStatus = apicontainerstatus.ContainerResourcesProvisioned + // Mapped status is ContainerRunning when container status is ContainerResourcesProvisioned + // and steady state is ContainerResourcesProvisioned + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerRunning) + + // Mapped status is ContainerStopped when container status is ContainerStopped + containerStatus = apicontainerstatus.ContainerStopped + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerRunning), + apicontainerstatus.ContainerStopped) + assert.Equal(t, + containerStatusChangeStatus(containerStatus, apicontainerstatus.ContainerResourcesProvisioned), + apicontainerstatus.ContainerStopped) +} + +func TestBuildContainerStateChangePayload(t *testing.T) { + tcs := []struct { + name string + change ContainerStateChange + expected *ecsmodel.ContainerStateChange + expectedError string + }{ + { + name: "fails when no container name", + change: ContainerStateChange{}, + expectedError: "container state change has no container name", + }, + { + name: "no result no error when container state is unsupported", + change: ContainerStateChange{ + ContainerName: "container", + Status: apicontainerstatus.ContainerStatusNone, + }, + expected: nil, + }, + { + name: "MANIFEST_PULLED state maps to PENDING", + change: ContainerStateChange{ + ContainerName: "container", + Container: &apicontainer.Container{}, + Status: apicontainerstatus.ContainerManifestPulled, + ImageDigest: "digest", + }, + expected: &ecsmodel.ContainerStateChange{ + ContainerName: aws.String("container"), + ImageDigest: aws.String("digest"), + NetworkBindings: []*ecs.NetworkBinding{}, + Status: aws.String("PENDING"), + }, + }, + { + name: "RUNNING maps to RUNNING", + change: ContainerStateChange{ + ContainerName: "container", + Container: &apicontainer.Container{}, + Status: apicontainerstatus.ContainerRunning, + ImageDigest: "digest", + RuntimeID: "runtimeid", + }, + expected: &ecsmodel.ContainerStateChange{ + ContainerName: aws.String("container"), + ImageDigest: aws.String("digest"), + RuntimeId: aws.String("runtimeid"), + NetworkBindings: []*ecs.NetworkBinding{}, + Status: aws.String("RUNNING"), + }, + }, + { + name: "STOPPED maps to STOPPED", + change: ContainerStateChange{ + ContainerName: "container", + Container: &apicontainer.Container{}, + Status: apicontainerstatus.ContainerStopped, + ImageDigest: "digest", + RuntimeID: "runtimeid", + ExitCode: aws.Int(1), + }, + expected: &ecsmodel.ContainerStateChange{ + ContainerName: aws.String("container"), + ImageDigest: aws.String("digest"), + RuntimeId: aws.String("runtimeid"), + ExitCode: aws.Int64(1), + NetworkBindings: []*ecs.NetworkBinding{}, + Status: aws.String("STOPPED"), + }, + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + res, err := buildContainerStateChangePayload(tc.change) + if tc.expectedError == "" { + require.NoError(t, err) + assert.Equal(t, tc.expected, res) + } else { + assert.EqualError(t, err, tc.expectedError) + } + }) + } +} diff --git a/agent/api/task/task.go b/agent/api/task/task.go index c7f5b96b518..9ef69bd8987 100644 --- a/agent/api/task/task.go +++ b/agent/api/task/task.go @@ -3721,3 +3721,14 @@ func (task *Task) IsRunning() bool { return taskStatus == apitaskstatus.TaskRunning } + +// Checks if the task has at least one container with a successfully +// resolved image manifest digest. +func (task *Task) HasAContainerWithResolvedDigest() bool { + for _, c := range task.Containers { + if c.DigestResolved() { + return true + } + } + return false +} diff --git a/agent/api/task/task_test.go b/agent/api/task/task_test.go index b38e4630ea2..d331728b8e7 100644 --- a/agent/api/task/task_test.go +++ b/agent/api/task/task_test.go @@ -5431,3 +5431,21 @@ func TestIsManagedDaemonTask(t *testing.T) { }) } } + +func TestHasAContainerWithResolvedDigest(t *testing.T) { + t.Run("false if no containers with a resolved digest", func(t *testing.T) { + task := &Task{ + Containers: []*apicontainer.Container{{}}, + } + assert.False(t, task.HasAContainerWithResolvedDigest()) + }) + t.Run("true if there is a container with a resolved digest", func(t *testing.T) { + task := &Task{ + Containers: []*apicontainer.Container{ + {}, + {ImageDigest: "digest"}, + }, + } + assert.True(t, task.HasAContainerWithResolvedDigest()) + }) +} diff --git a/agent/config/config.go b/agent/config/config.go index 97737c9ef7b..ea75b838515 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -549,6 +549,7 @@ func environmentConfig() (Config, error) { DeleteNonECSImagesEnabled: parseBooleanDefaultFalseConfig("ECS_ENABLE_UNTRACKED_IMAGE_CLEANUP"), TaskCPUMemLimit: parseBooleanDefaultTrueConfig("ECS_ENABLE_TASK_CPU_MEM_LIMIT"), DockerStopTimeout: parseDockerStopTimeout(), + ManifestPullTimeout: parseManifestPullTimeout(), ContainerStartTimeout: parseContainerStartTimeout(), ContainerCreateTimeout: parseContainerCreateTimeout(), DependentContainersPullUpfront: parseBooleanDefaultFalseConfig("ECS_PULL_DEPENDENT_CONTAINERS_UPFRONT"), @@ -625,6 +626,7 @@ func (cfg *Config) String() string { "PollingMetricsWaitDuration: %v, "+ "ReservedMem: %v, "+ "TaskCleanupWaitDuration: %v, "+ + "ManifestPullTimeout: %v, "+ "DockerStopTimeout: %v, "+ "ContainerStartTimeout: %v, "+ "ContainerCreateTimeout: %v, "+ @@ -644,6 +646,7 @@ func (cfg *Config) String() string { cfg.PollingMetricsWaitDuration, cfg.ReservedMemory, cfg.TaskCleanupWaitDuration, + cfg.ManifestPullTimeout, cfg.DockerStopTimeout, cfg.ContainerStartTimeout, cfg.ContainerCreateTimeout, diff --git a/agent/config/config_test.go b/agent/config/config_test.go index 7b1c08c6679..4d0ee41ceea 100644 --- a/agent/config/config_test.go +++ b/agent/config/config_test.go @@ -127,6 +127,7 @@ func TestEnvironmentConfig(t *testing.T) { defer setTestEnv("ECS_CLUSTER", "myCluster")() defer setTestEnv("ECS_RESERVED_PORTS_UDP", "[42,99]")() defer setTestEnv("ECS_RESERVED_MEMORY", "20")() + defer setTestEnv("ECS_MANIFEST_PULL_TIMEOUT", "85s")() defer setTestEnv("ECS_CONTAINER_STOP_TIMEOUT", "60s")() defer setTestEnv("ECS_CONTAINER_START_TIMEOUT", "5m")() defer setTestEnv("ECS_CONTAINER_CREATE_TIMEOUT", "4m")() @@ -176,6 +177,7 @@ func TestEnvironmentConfig(t *testing.T) { assert.Contains(t, conf.ReservedPortsUDP, uint16(42)) assert.Contains(t, conf.ReservedPortsUDP, uint16(99)) assert.Equal(t, uint16(20), conf.ReservedMemory) + assert.Equal(t, 85*time.Second, conf.ManifestPullTimeout) expectedDurationDockerStopTimeout, _ := time.ParseDuration("60s") assert.Equal(t, expectedDurationDockerStopTimeout, conf.DockerStopTimeout) expectedDurationContainerStartTimeout, _ := time.ParseDuration("5m") @@ -361,6 +363,14 @@ func TestInvalidFormatContainerCreateTimeout(t *testing.T) { assert.Equal(t, defaultContainerCreateTimeout, conf.ContainerCreateTimeout, "Wrong value for ContainerCreateTimeout") } +func TestInvalidFormatManifestPullTimeout(t *testing.T) { + defer setTestRegion()() + defer setTestEnv("ECS_MANIFEST_PULL_TIMEOUT", "invalid")() + conf, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + assert.NoError(t, err) + assert.Equal(t, defaultManifestPullTimeout, conf.ManifestPullTimeout) +} + func TestInvalidFormatDockerInactivityTimeout(t *testing.T) { defer setTestRegion()() defer setTestEnv("ECS_IMAGE_PULL_INACTIVITY_TIMEOUT", "invalid")() @@ -402,6 +412,14 @@ func TestZeroValueContainerCreateTimeout(t *testing.T) { assert.Equal(t, defaultContainerCreateTimeout, conf.ContainerCreateTimeout, "Wrong value for ContainerCreateTimeout") } +func TestZeroValueManifestPullTimeout(t *testing.T) { + defer setTestRegion()() + defer setTestEnv("ECS_MANIFEST_PULL_TIMEOUT", "0s")() + conf, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + assert.NoError(t, err) + assert.Equal(t, defaultManifestPullTimeout, conf.ManifestPullTimeout) +} + func TestInvalidValueContainerStartTimeout(t *testing.T) { defer setTestRegion()() defer setTestEnv("ECS_CONTAINER_START_TIMEOUT", "-10s")() @@ -418,6 +436,14 @@ func TestInvalidValueContainerCreateTimeout(t *testing.T) { assert.Equal(t, minimumContainerCreateTimeout, conf.ContainerCreateTimeout, "Wrong value for ContainerCreataeTimeout") } +func TestInvalidValueManifestPullTimeout(t *testing.T) { + defer setTestRegion()() + defer setTestEnv("ECS_MANIFEST_PULL_TIMEOUT", "-10s")() + conf, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + assert.NoError(t, err) + assert.Equal(t, minimumManifestPullTimeout, conf.ManifestPullTimeout) +} + func TestZeroValueDockerPullInactivityTimeout(t *testing.T) { defer setTestRegion()() defer setTestEnv("ECS_DOCKER_PULL_INACTIVITY_TIMEOUT", "0s")() diff --git a/agent/config/config_unix.go b/agent/config/config_unix.go index 5adf8ec02cc..19968a827dc 100644 --- a/agent/config/config_unix.go +++ b/agent/config/config_unix.go @@ -46,6 +46,10 @@ const ( // Default cgroup memory system root path, this is the default used if the // path has not been configured through ECS_CGROUP_PATH defaultCgroupPath = "/sys/fs/cgroup" + // minimumManifestPullTimeout is the minimum timeout allowed for manifest pulls + minimumManifestPullTimeout = 30 * time.Second + // defaultManifestPullTimeout is the default timeout for manifest pulls + defaultManifestPullTimeout = 1 * time.Minute // defaultContainerStartTimeout specifies the value for container start timeout duration defaultContainerStartTimeout = 3 * time.Minute // minimumContainerStartTimeout specifies the minimum value for starting a container @@ -70,6 +74,7 @@ func DefaultConfig() Config { ReservedMemory: 0, AvailableLoggingDrivers: []dockerclient.LoggingDriver{dockerclient.JSONFileDriver, dockerclient.NoneDriver}, TaskCleanupWaitDuration: DefaultTaskCleanupWaitDuration, + ManifestPullTimeout: defaultManifestPullTimeout, DockerStopTimeout: defaultDockerStopTimeout, ContainerStartTimeout: defaultContainerStartTimeout, ContainerCreateTimeout: defaultContainerCreateTimeout, diff --git a/agent/config/config_unix_test.go b/agent/config/config_unix_test.go index 84d803766ef..552f634eb68 100644 --- a/agent/config/config_unix_test.go +++ b/agent/config/config_unix_test.go @@ -46,6 +46,7 @@ func TestConfigDefault(t *testing.T) { assert.Equal(t, 30*time.Second, cfg.DockerStopTimeout, "Default docker stop container timeout set incorrectly") assert.Equal(t, 3*time.Minute, cfg.ContainerStartTimeout, "Default docker start container timeout set incorrectly") assert.Equal(t, 4*time.Minute, cfg.ContainerCreateTimeout, "Default docker create container timeout set incorrectly") + assert.Equal(t, 1*time.Minute, cfg.ManifestPullTimeout, "Default manifest pull timeout set incorrectly") assert.False(t, cfg.PrivilegedDisabled.Enabled(), "Default PrivilegedDisabled set incorrectly") assert.Equal(t, []dockerclient.LoggingDriver{dockerclient.JSONFileDriver, dockerclient.NoneDriver}, cfg.AvailableLoggingDrivers, "Default logging drivers set incorrectly") diff --git a/agent/config/config_windows.go b/agent/config/config_windows.go index 803f0937556..a8873d6e372 100644 --- a/agent/config/config_windows.go +++ b/agent/config/config_windows.go @@ -57,6 +57,10 @@ const ( dnsPort = 53 // NetBIOS over TCP/IP netBIOSPort = 139 + // minimumManifestPullTimeout is the minimum timeout allowed for manifest pulls + minimumManifestPullTimeout = 30 * time.Second + // defaultManifestPullTimeout is the default timeout for manifest pulls + defaultManifestPullTimeout = 1 * time.Minute // defaultContainerStartTimeout specifies the value for container start timeout duration defaultContainerStartTimeout = 8 * time.Minute // minimumContainerStartTimeout specifies the minimum value for starting a container @@ -121,6 +125,7 @@ func DefaultConfig() Config { ReservedMemory: 0, AvailableLoggingDrivers: []dockerclient.LoggingDriver{dockerclient.JSONFileDriver, dockerclient.NoneDriver, dockerclient.AWSLogsDriver}, TaskCleanupWaitDuration: DefaultTaskCleanupWaitDuration, + ManifestPullTimeout: defaultManifestPullTimeout, DockerStopTimeout: defaultDockerStopTimeout, ContainerStartTimeout: defaultContainerStartTimeout, ContainerCreateTimeout: defaultContainerCreateTimeout, diff --git a/agent/config/parse.go b/agent/config/parse.go index 886389d1209..11059c83f23 100644 --- a/agent/config/parse.go +++ b/agent/config/parse.go @@ -94,6 +94,19 @@ func parseDockerStopTimeout() time.Duration { return dockerStopTimeout } +func parseManifestPullTimeout() time.Duration { + var timeout time.Duration + parsedTimeout := parseEnvVariableDuration("ECS_MANIFEST_PULL_TIMEOUT") + if parsedTimeout >= minimumManifestPullTimeout { + timeout = parsedTimeout + } else if parsedTimeout != 0 { + // Parsed timeout too low + timeout = minimumManifestPullTimeout + seelog.Warnf("Discarded invalid value for manifest pull timeout, parsed as: %v", parsedTimeout) + } + return timeout +} + func parseContainerStartTimeout() time.Duration { var containerStartTimeout time.Duration parsedStartTimeout := parseEnvVariableDuration("ECS_CONTAINER_START_TIMEOUT") diff --git a/agent/config/types.go b/agent/config/types.go index 03fe9d2d8d4..e9eec8abf85 100644 --- a/agent/config/types.go +++ b/agent/config/types.go @@ -108,6 +108,9 @@ type Config struct { // This doesn't reserve memory usage on the instance ReservedMemory uint16 + // ManifestPullTimeout is the amount of time to wait for a manifest pull + ManifestPullTimeout time.Duration + // DockerStopTimeout specifies the amount of time before a SIGKILL is issued to // containers managed by ECS DockerStopTimeout time.Duration diff --git a/agent/dockerclient/dockerapi/docker_client.go b/agent/dockerclient/dockerapi/docker_client.go index ac659d59dc2..60f3e547027 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 @@ -270,10 +278,13 @@ func (dg *dockerGoClient) WithVersion(version dockerclient.DockerVersion) (Docke versionedClient := &dockerGoClient{ sdkClientFactory: dg.sdkClientFactory, version: version, + ecrClientFactory: dg.ecrClientFactory, auth: dg.auth, + ecrTokenCache: dg.ecrTokenCache, config: dg.config, context: dg.context, manifestPullBackoff: dg.manifestPullBackoff, + imageTagBackoff: dg.imageTagBackoff, } // Check if the version is supported _, err := versionedClient.sdkDockerClient() @@ -317,6 +328,7 @@ func NewDockerGoClient(sdkclientFactory sdkclientfactory.Factory, pullRetryJitterMultiplier, pullRetryDelayMultiplier), manifestPullBackoff: retry.NewExponentialBackoff(minimumPullRetryDelay, maximumPullRetryDelay, pullRetryJitterMultiplier, pullRetryDelayMultiplier), + imageTagBackoff: retry.NewConstantBackoff(tagImageRetryInterval), inactivityTimeoutHandler: handleInactivityTimeout, }, nil } @@ -371,6 +383,10 @@ func (dg *dockerGoClient) PullImageManifest( }) if err != nil { + if errors.Is(err, context.DeadlineExceeded) { + timeoutErr := &DockerTimeoutError{time.Since(startTime), "MANIFEST_PULLED"} + return registry.DistributionInspect{}, timeoutErr + } return registry.DistributionInspect{}, wrapManifestPullErrorAsNamedError(imageRef, err) } if ctxErr := ctx.Err(); ctxErr != nil { @@ -587,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 c9df9bfd3fb..ad269abf553 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 93a38c55547..3e79d9098ca 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 d7eaf9aa37c..99d9941c858 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 a863746a79c..9c07d9683a2 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_integ_test.go b/agent/engine/common_integ_test.go index afe803a678a..eeac803a941 100644 --- a/agent/engine/common_integ_test.go +++ b/agent/engine/common_integ_test.go @@ -141,6 +141,20 @@ func loggerConfigIntegrationTest(logfile string) string { return config } +func verifyContainerManifestPulledStateChange(t *testing.T, taskEngine TaskEngine) { + stateChangeEvents := taskEngine.StateChangeEvents() + event := <-stateChangeEvents + assert.Equal(t, apicontainerstatus.ContainerManifestPulled, event.(api.ContainerStateChange).Status, + "Expected container to be at MANIFEST_PULLED state") +} + +func verifyTaskManifestPulledStateChange(t *testing.T, taskEngine TaskEngine) { + stateChangeEvents := taskEngine.StateChangeEvents() + event := <-stateChangeEvents + assert.Equal(t, apitaskstatus.TaskManifestPulled, event.(api.TaskStateChange).Status, + "Expected task to reach MANIFEST_PULLED state") +} + func verifyContainerRunningStateChange(t *testing.T, taskEngine TaskEngine) { stateChangeEvents := taskEngine.StateChangeEvents() event := <-stateChangeEvents @@ -148,6 +162,13 @@ func verifyContainerRunningStateChange(t *testing.T, taskEngine TaskEngine) { "Expected container to be RUNNING") } +func verifyTaskRunningStateChange(t *testing.T, taskEngine TaskEngine) { + stateChangeEvents := taskEngine.StateChangeEvents() + event := <-stateChangeEvents + assert.Equal(t, apitaskstatus.TaskRunning, event.(api.TaskStateChange).Status, + "Expected task to be RUNNING") +} + func verifyContainerRunningStateChangeWithRuntimeID(t *testing.T, taskEngine TaskEngine) { stateChangeEvents := taskEngine.StateChangeEvents() event := <-stateChangeEvents @@ -178,6 +199,14 @@ func verifyContainerStoppedStateChange(t *testing.T, taskEngine TaskEngine) { "Expected container to be STOPPED") } +func verifyContainerStoppedStateChangeWithReason(t *testing.T, taskEngine TaskEngine, reason string) { + stateChangeEvents := taskEngine.StateChangeEvents() + event := <-stateChangeEvents + assert.Equal(t, event.(api.ContainerStateChange).Status, apicontainerstatus.ContainerStopped, + "Expected container to be STOPPED") + assert.Equal(t, reason, event.(api.ContainerStateChange).Reason) +} + func verifyContainerStoppedStateChangeWithRuntimeID(t *testing.T, taskEngine TaskEngine) { stateChangeEvents := taskEngine.StateChangeEvents() event := <-stateChangeEvents diff --git a/agent/engine/common_test.go b/agent/engine/common_test.go index cc0ff397287..b8bf2d93113 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" @@ -46,14 +47,19 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/cihub/seelog" 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 ( containerID = "containerID" waitTaskStateChangeDuration = 2 * time.Minute + testDigest = digest.Digest("sha256:ed6d2c43c8fbcd3eaa44c9dab6d94cb346234476230dc1681227aa72d07181ee") ) var ( @@ -106,6 +112,9 @@ func verifyTaskIsRunning(stateChangeEvents <-chan statechange.Event, task *apita if taskEvent.TaskARN != task.Arn { continue } + if taskEvent.Status == apitaskstatus.TaskManifestPulled { + continue + } if taskEvent.Status == apitaskstatus.TaskRunning { return nil } @@ -146,6 +155,7 @@ func waitForTaskStoppedByCheckStatus(task *apitask.Task) { // removed matches with the generated container name during cleanup operation in the // test. func validateContainerRunWorkflow(t *testing.T, + ctrl *gomock.Controller, container *apicontainer.Container, task *apitask.Task, imageManager *mock_engine.MockImageManager, @@ -156,8 +166,29 @@ func validateContainerRunWorkflow(t *testing.T, createdContainerName chan<- string, assertions func(), ) { + // 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{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) @@ -260,6 +291,7 @@ func addTaskToEngine(t *testing.T, assert.NoError(t, err) taskEngine.AddTask(sleepTask) + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForRunningEvents(t, taskEngine.StateChangeEvents()) // Wait for all events to be consumed prior to moving it towards stopped; we // don't want to race the below with these or we'll end up with the "going @@ -294,6 +326,18 @@ func waitForRunningEvents(t *testing.T, stateChangeEvents <-chan statechange.Eve } } +// waitForManifestPulledEvents waits for a task to emit 'MANIFEST_PULLED' events for a container +// and the task +func waitForManifestPulledEvents(t *testing.T, stateChangeEvents <-chan statechange.Event) { + event := <-stateChangeEvents + assert.Equal(t, apicontainerstatus.ContainerManifestPulled, event.(api.ContainerStateChange).Status, + "Expected MANIFEST_PULLED state to be emitted for the container") + + event = <-stateChangeEvents + assert.Equal(t, apitaskstatus.TaskManifestPulled, event.(api.TaskStateChange).Status, + "Expected MANIFEST_PULLED state to be emitted for the task") +} + // waitForStopEvents waits for a task to emit 'STOPPED' events for a container // and the task func waitForStopEvents(t *testing.T, stateChangeEvents <-chan statechange.Event, verifyExitCode, execEnabled bool) { diff --git a/agent/engine/docker_image_manager.go b/agent/engine/docker_image_manager.go index a6841ce797b..a52f67558a1 100644 --- a/agent/engine/docker_image_manager.go +++ b/agent/engine/docker_image_manager.go @@ -159,8 +159,12 @@ func (imageManager *dockerImageManager) RecordContainerReference(container *apic return err } container.ImageID = imageInspected.ID - imageDigest := imageManager.fetchRepoDigest(imageInspected, container) - container.SetImageDigest(imageDigest) + // For older Docker versions imageDigest is not populated during transition to + // MANIFEST_PULLED state. Populate it here if that's the case. + if container.GetImageDigest() == "" { + imageDigest := imageManager.fetchRepoDigest(imageInspected, container) + container.SetImageDigest(imageDigest) + } added := imageManager.addContainerReferenceToExistingImageState(container) if !added { imageManager.addContainerReferenceToNewImageState(container, imageInspected.Size) diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index c62b2e8d3f4..870d0cedd9c 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -45,6 +45,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/taskresource/credentialspec" "github.com/aws/amazon-ecs-agent/agent/taskresource/firelens" "github.com/aws/amazon-ecs-agent/agent/utils" + referenceutil "github.com/aws/amazon-ecs-agent/agent/utils/reference" "github.com/aws/amazon-ecs-agent/ecs-agent/api/appnet" apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status" apierrors "github.com/aws/amazon-ecs-agent/ecs-agent/api/errors" @@ -59,6 +60,7 @@ import ( ep "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/docker/docker/api/types" dockercontainer "github.com/docker/docker/api/types/container" + "github.com/opencontainers/go-digest" "github.com/pkg/errors" ) @@ -80,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. @@ -978,7 +981,7 @@ func (engine *DockerTaskEngine) EmitTaskEvent(task *apitask.Task, reason string) event, err := api.NewTaskStateChangeEvent(task, reason) if err != nil { if _, ok := err.(api.ErrShouldNotSendEvent); ok { - logger.Debug(err.Error()) + logger.Debug(err.Error(), logger.Fields{field.TaskID: task.GetID()}) } else { logger.Error("Unable to create task state change event", logger.Fields{ field.TaskID: task.GetID(), @@ -1238,12 +1241,134 @@ func (engine *DockerTaskEngine) GetDaemonManagers() map[string]dm.DaemonManager func (engine *DockerTaskEngine) pullContainerManifest( task *apitask.Task, container *apicontainer.Container, ) dockerapi.DockerContainerMetadata { - // Currently a no-op - logger.Debug("Manifest pull is currently a no-op", logger.Fields{ - field.TaskID: task.GetID(), - field.Container: container.Name, - field.Image: container.Image, + if !container.DigestResolutionRequired() { + // Digest resolution not required + // (internal container or already has digest in image reference) + logger.Info("Digest resolution not required", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Image: container.Image, + }) + return dockerapi.DockerContainerMetadata{} + } + // AppNet Agent container image is managed at start up so it is not in-scope for digest resolution. + // (it uses the same image as AppNet Relay container) + if task.IsServiceConnectEnabled() && container == task.GetServiceConnectContainer() { + return dockerapi.DockerContainerMetadata{} + } + + var imageManifestDigest digest.Digest + if !engine.imagePullRequired(engine.cfg.ImagePullBehavior, container, task.GetID()) { + // Image pull is not required for the container so we will use a locally available + // image for the container. Get digest from a locally available image. + imgInspect, err := engine.client.InspectImage(container.Image) + if err != nil { + logger.Error("Failed to inspect image to find repo digest", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Image: container.Image, + }) + return dockerapi.DockerContainerMetadata{ + Error: dockerapi.CannotPullImageManifestError{FromError: err}, + } + } + if len(imgInspect.RepoDigests) == 0 { + // Image was not pulled from a registry, so the user must have cached it on the + // host directly. Skip digest resolution for this case as there is no digest. + logger.Info("No repo digest found", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Image: container.Image, + }) + return dockerapi.DockerContainerMetadata{} + } + parsedDigest, err := referenceutil.GetDigestFromRepoDigests( + imgInspect.RepoDigests, container.Image) + if err != nil { + logger.Error("Failed to find a repo digest matching the image", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Image: container.Image, + "repoDigests": imgInspect.RepoDigests, + }) + return dockerapi.DockerContainerMetadata{ + Error: dockerapi.CannotPullImageManifestError{ + FromError: fmt.Errorf("failed to find a repo digest matching '%s'", container.Image), + }, + } + } + imageManifestDigest = parsedDigest + logger.Info("Fetched image manifest digest for container from local image inspect", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.ImageDigest: imageManifestDigest.String(), + field.Image: container.Image, + }) + } else { + // Digest should be resolved by calling the registry + + // Technically, API version 1.30 is enough to call Distribution API to fetch image manifests + // from registries. However, Docker engine versions between 17.06 (API version 1.30) and + // 17.12 (API version 1.35) support image pulls from v1 registries + // (using `disable-legacy-registry` option) whereas Distribution API does not work against + // v1 registries. So, to be safe, we will only attempt digest resolution if Docker engine + // version is >= 17.12 (API version 1.35). + supportedAPIVersion := dockerclient.GetSupportedDockerAPIVersion(dockerclient.Version_1_35) + client, err := engine.client.WithVersion(supportedAPIVersion) + if err != nil { + logger.Warn( + "Failed to find a supported API version that supports manifest pulls. Skipping digest resolution.", + logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + "requiredAPIVersion": supportedAPIVersion, + field.Error: err, + }) + return dockerapi.DockerContainerMetadata{} + } + + // Set registry auth credentials if required and clear them when no longer needed + clearCreds, authError := engine.setRegistryCredentials(container, task) + if authError != nil { + logger.Error("Failed to set registry auth credentials", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Error: authError, + }) + return dockerapi.DockerContainerMetadata{Error: authError} + } + if clearCreds != nil { + defer clearCreds() + } + + ctx, cancel := context.WithTimeout(engine.ctx, engine.cfg.ManifestPullTimeout) + defer cancel() + distInspect, manifestPullErr := client.PullImageManifest( + ctx, container.Image, container.RegistryAuthentication) + if manifestPullErr != nil { + logger.Error("Failed to fetch image manifest from registry", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.Image: container.Image, + field.Error: manifestPullErr, + }) + return dockerapi.DockerContainerMetadata{Error: manifestPullErr} + } + imageManifestDigest = distInspect.Descriptor.Digest + logger.Info("Fetched image manifest digest for container from registry", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.ImageDigest: imageManifestDigest.String(), + field.Image: container.Image, + }) + } + + logger.Debug("Setting image digest on container", logger.Fields{ + field.TaskARN: task.Arn, + field.ContainerName: container.Name, + field.ImageDigest: imageManifestDigest.String(), }) + container.SetImageDigest(imageManifestDigest.String()) return dockerapi.DockerContainerMetadata{} } @@ -1312,7 +1437,7 @@ func (engine *DockerTaskEngine) imagePullRequired(imagePullBehavior config.Image // by inspecting the image. _, err := engine.client.InspectImage(container.Image) if err != nil { - logger.Info("Image inspect returned error, going to pull image for container", logger.Fields{ + logger.Info("Image inspect returned error, manifest and image pull required", logger.Fields{ field.TaskID: taskId, field.Container: container.Name, field.Image: container.Image, @@ -1397,53 +1522,88 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta return dockerapi.DockerContainerMetadata{Error: TaskStoppedBeforePullBeginError{task.Arn}} } - // Set the credentials for pull from ECR if necessary - if container.ShouldPullWithExecutionRole() { - executionCredentials, ok := engine.credentialsManager.GetTaskCredentials(task.GetExecutionCredentialsID()) - if !ok { - logger.Error("Unable to acquire ECR credentials to pull image for container", logger.Fields{ - field.TaskID: task.GetID(), - field.Container: container.Name, - field.Image: container.Image, + // Set registry auth credentials if required and clearCreds them when no longer needed + clearCreds, err := engine.setRegistryCredentials(container, task) + if err != nil { + return dockerapi.DockerContainerMetadata{Error: err} + } + if clearCreds != nil { + defer clearCreds() + } + + 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.CannotPullECRContainerError{ - FromError: errors.New("engine ecr credentials: not found"), + 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, + }) + } - iamCredentials := executionCredentials.GetIAMRoleCredentials() - container.SetRegistryAuthCredentials(iamCredentials) - // Clean up the ECR pull credentials after pulling - defer container.SetRegistryAuthCredentials(credentials.IAMRoleCredentials{}) + 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 - // Apply registry auth data from ASM if required - if container.ShouldPullWithASMAuth() { - if err := task.PopulateASMAuthData(container); err != nil { - logger.Error("Unable to acquire Docker registry credentials to pull image for container", logger.Fields{ + 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, }) - return dockerapi.DockerContainerMetadata{ - Error: dockerapi.CannotPullContainerAuthError{ - FromError: errors.New("engine docker private registry credentials: not found"), - }, + 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, + }) } - defer container.SetASMDockerAuthConfig(types.AuthConfig{}) } - metadata := engine.client.PullImage(engine.ctx, container.Image, 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 findCachedImage := false if !pullSucceeded { // If Agent failed to pull an image when @@ -1451,11 +1611,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, @@ -1469,7 +1629,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 } @@ -1486,6 +1646,53 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta return metadata } +// Sets image registry auth credentials for the container. +// Returns a cleanup function that may be called to clear the credentials from the container +// when they are no longer needed. The cleanup function is `nil` if no credentials are needed +// for the container. +func (engine *DockerTaskEngine) setRegistryCredentials( + container *apicontainer.Container, task *apitask.Task, +) (func(), apierrors.NamedError) { + var cleanup func() + + // Set the credentials for pull from ECR if necessary + if container.ShouldPullWithExecutionRole() { + executionCredentials, ok := engine.credentialsManager.GetTaskCredentials(task.GetExecutionCredentialsID()) + if !ok { + logger.Error("Unable to acquire ECR credentials to pull image for container", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: container.Image, + }) + return nil, dockerapi.CannotPullECRContainerError{ + FromError: errors.New("engine ecr credentials: not found"), + } + } + + iamCredentials := executionCredentials.GetIAMRoleCredentials() + container.SetRegistryAuthCredentials(iamCredentials) + cleanup = func() { container.SetRegistryAuthCredentials(credentials.IAMRoleCredentials{}) } + } + + // Apply registry auth data from ASM if required + if container.ShouldPullWithASMAuth() { + if err := task.PopulateASMAuthData(container); err != nil { + logger.Error("Unable to acquire Docker registry credentials to pull image for container", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: container.Name, + field.Image: container.Image, + field.Error: err, + }) + return nil, dockerapi.CannotPullContainerAuthError{ + FromError: errors.New("engine docker private registry credentials: not found"), + } + } + cleanup = func() { container.SetASMDockerAuthConfig(types.AuthConfig{}) } + } + + return cleanup, nil +} + func (engine *DockerTaskEngine) updateContainerReference(pullSucceeded bool, container *apicontainer.Container, taskId string) { err := engine.imageManager.RecordContainerReference(container) if err != nil { diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 627b53770db..abea4639bc6 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -33,6 +33,7 @@ import ( mock_asm_factory "github.com/aws/amazon-ecs-agent/agent/asm/factory/mocks" "github.com/aws/amazon-ecs-agent/agent/config" "github.com/aws/amazon-ecs-agent/agent/data" + "github.com/aws/amazon-ecs-agent/agent/dockerclient" "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi" mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks" "github.com/aws/amazon-ecs-agent/agent/ecscni" @@ -57,12 +58,14 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/credentials" "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/aws/aws-sdk-go/aws" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" + "github.com/docker/docker/api/types/registry" "github.com/golang/mock/gomock" "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" @@ -115,6 +118,10 @@ func TestResourceContainerProgression(t *testing.T) { client.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes() + // Mock versioned docker client to be used for transition to MANIFEST_PULLED state + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + + expectedCanonicalRef := sleepContainer.Image + "@" + testDigest.String() // Hierarchical memory accounting is always enabled in CgroupV2 and no controller file exists to configure it if config.CgroupV2 { gomock.InOrder( @@ -122,7 +129,18 @@ func TestResourceContainerProgression(t *testing.T) { mockControl.EXPECT().Exists(gomock.Any()).Return(false), mockControl.EXPECT().Create(gomock.Any()).Return(nil), imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), - client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), sleepContainer.Image, nil). + Return(registry.DistributionInspect{ + Descriptor: ocispec.Descriptor{Digest: testDigest}, + }, nil), + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, sleepContainer.Image). + Return(nil), imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil), imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false), client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), @@ -153,7 +171,18 @@ func TestResourceContainerProgression(t *testing.T) { mockControl.EXPECT().Create(gomock.Any()).Return(nil), mockIO.EXPECT().WriteFile(cgroupMemoryPath, gomock.Any(), gomock.Any()).Return(nil), imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), - client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), sleepContainer.Image, nil). + Return(registry.DistributionInspect{ + Descriptor: ocispec.Descriptor{Digest: testDigest}, + }, nil), + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, sleepContainer.Image). + Return(nil), imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil), imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false), client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), @@ -300,7 +329,7 @@ func TestResourceContainerProgressionFailure(t *testing.T) { sleepContainer := sleepTask.Containers[0] sleepContainer.TransitionDependenciesMap = make(map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet) - sleepContainer.BuildResourceDependency("cgroup", resourcestatus.ResourceCreated, apicontainerstatus.ContainerPulled) + sleepContainer.BuildResourceDependency("cgroup", resourcestatus.ResourceCreated, apicontainerstatus.ContainerManifestPulled) mockControl := mock_control.NewMockControl(ctrl) taskID := sleepTask.GetID() @@ -408,7 +437,7 @@ func TestTaskCPULimitHappyPath(t *testing.T) { } for _, container := range sleepTask.Containers { - validateContainerRunWorkflow(t, container, sleepTask, imageManager, + validateContainerRunWorkflow(t, ctrl, container, sleepTask, imageManager, client, &roleCredentials, &containerEventsWG, eventStream, containerName, func() { metadataManager.EXPECT().Create(gomock.Any(), gomock.Any(), @@ -675,7 +704,21 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { // parallel. The dependency graph enforcement comes into effect for CREATED transitions. // Hence, do not enforce the order of invocation of these calls imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}) + + // Prepare mock image manifest digest for test + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), sleepContainer.Image, sleepContainer.RegistryAuthentication). + Return(registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil) + + expectedCanonicalRef := sleepContainer.Image + "@" + testDigest.String() + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, sleepContainer.Image). + Return(nil) imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil) imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false) @@ -843,8 +886,15 @@ func TestPauseContainerHappyPath(t *testing.T) { cniClient.EXPECT().SetupNS(gomock.Any(), gomock.Any(), gomock.Any()).Return(nsResult, nil), ) + // Mock versioned docker client to be used during transition to MANIFEST_PULLED state + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + // For the other container imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + dockerClient.EXPECT().WithVersion(dockerclient.Version_1_35).Times(2).Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()).Times(2). + Return(registry.DistributionInspect{}, nil) dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(2) @@ -1057,6 +1107,11 @@ func TestContainersWithServiceConnect(t *testing.T) { // For the other container imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + dockerClient.EXPECT().WithVersion(dockerclient.Version_1_35).Times(2).Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()).Times(2). + Return(registry.DistributionInspect{}, nil) dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(2) @@ -1255,6 +1310,11 @@ func TestContainersWithServiceConnect_BridgeMode(t *testing.T) { // For SC and sleep container - those calls can happen in parallel // Note that SC container won't trigger image-related calls as AppNet container images are cached and managed by Agent imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + dockerClient.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()). + Return(registry.DistributionInspect{}, nil) dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(1) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(1) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(1) diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 480a2972ce1..852b3891b29 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -67,6 +67,8 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" mock_ttime "github.com/aws/amazon-ecs-agent/ecs-agent/utils/ttime/mocks" + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/secretsmanager" @@ -75,6 +77,7 @@ import ( "github.com/docker/docker/api/types" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" + "github.com/docker/docker/api/types/registry" "github.com/golang/mock/gomock" "github.com/pborman/uuid" "github.com/stretchr/testify/assert" @@ -274,7 +277,7 @@ func TestBatchContainerHappyPath(t *testing.T) { }() for _, container := range sleepTask.Containers { - validateContainerRunWorkflow(t, container, sleepTask, imageManager, + validateContainerRunWorkflow(t, ctrl, container, sleepTask, imageManager, client, &roleCredentials, &containerEventsWG, eventStream, containerName, func() { metadataManager.EXPECT().Create(gomock.Any(), gomock.Any(), @@ -374,7 +377,7 @@ func TestRemoveEvents(t *testing.T) { }() for _, container := range sleepTask.Containers { - validateContainerRunWorkflow(t, container, sleepTask, imageManager, + validateContainerRunWorkflow(t, ctrl, container, sleepTask, imageManager, client, nil, &containerEventsWG, eventStream, containerName, func() { }) @@ -444,7 +447,19 @@ func TestStartTimeoutThenStart(t *testing.T) { client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil) for _, container := range sleepTask.Containers { imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - client.EXPECT().PullImage(gomock.Any(), container.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}) + 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{ + Descriptor: ocispec.Descriptor{Digest: testDigest}, + }, nil) + client.EXPECT(). + PullImage(gomock.Any(), container.Image+"@"+testDigest.String(), nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + TagImage(gomock.Any(), container.Image+"@"+testDigest.String(), container.Image). + Return(nil) imageManager.EXPECT().RecordContainerReference(container) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false) @@ -468,6 +483,7 @@ func TestStartTimeoutThenStart(t *testing.T) { assert.NoError(t, err) stateChangeEvents := taskEngine.StateChangeEvents() taskEngine.AddTask(sleepTask) + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForStopEvents(t, taskEngine.StateChangeEvents(), false, false) // Now surprise surprise, it actually did start! @@ -504,7 +520,7 @@ func TestSteadyStatePoll(t *testing.T) { // set up expectations for each container in the task calling create + start for _, container := range sleepTask.Containers { - validateContainerRunWorkflow(t, container, sleepTask, imageManager, + validateContainerRunWorkflow(t, ctrl, container, sleepTask, imageManager, client, nil, &containerEventsWG, eventStream, containerName, func() { }) @@ -531,6 +547,7 @@ func TestSteadyStatePoll(t *testing.T) { err := taskEngine.Init(ctx) // start the task engine assert.NoError(t, err) taskEngine.AddTask(sleepTask) // actually add the task we created + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForRunningEvents(t, taskEngine.StateChangeEvents()) containerMap, ok := taskEngine.(*DockerTaskEngine).State().ContainerMapByArn(sleepTask.Arn) assert.True(t, ok) @@ -581,6 +598,12 @@ func TestStopWithPendingStops(t *testing.T) { defer discardEvents(stateChangeEvents)() + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil).MaxTimes(2) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(2). + Return(registry.DistributionInspect{}, nil) + pullDone := make(chan bool) pullInvoked := make(chan bool) client.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Do(func(w, x, y, z interface{}) { @@ -780,7 +803,18 @@ func TestTaskTransitionWhenStopContainerTimesout(t *testing.T) { } for _, container := range sleepTask.Containers { imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - client.EXPECT().PullImage(gomock.Any(), container.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}) + 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{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil) + expectedCanonicalRef := container.Image + "@" + testDigest.String() + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, container.Image). + Return(nil) imageManager.EXPECT().RecordContainerReference(container) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false) client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil) @@ -813,6 +847,7 @@ func TestTaskTransitionWhenStopContainerTimesout(t *testing.T) { go taskEngine.AddTask(sleepTask) // wait for task running + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForRunningEvents(t, taskEngine.StateChangeEvents()) // Set the task desired status to be stopped and StopContainer will be called updateSleepTask := testdata.LoadTask("sleep5") @@ -841,10 +876,23 @@ func TestTaskTransitionWhenStopContainerReturnsUnretriableError(t *testing.T) { mockTime.EXPECT().Now().Return(time.Now()).AnyTimes() mockTime.EXPECT().After(gomock.Any()).AnyTimes() containerEventsWG := sync.WaitGroup{} + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) for _, container := range sleepTask.Containers { + expectedCanonicalRef := container.Image + "@" + testDigest.String() gomock.InOrder( imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), - client.EXPECT().PullImage(gomock.Any(), container.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), container.Image, container.RegistryAuthentication). + Return( + registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, + nil), + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, container.Image). + Return(nil), imageManager.EXPECT().RecordContainerReference(container), imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false), client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), @@ -888,6 +936,7 @@ func TestTaskTransitionWhenStopContainerReturnsUnretriableError(t *testing.T) { go taskEngine.AddTask(sleepTask) // wait for task running + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForRunningEvents(t, taskEngine.StateChangeEvents()) containerEventsWG.Wait() // Set the task desired status to be stopped and StopContainer will be called @@ -917,10 +966,21 @@ func TestTaskTransitionWhenStopContainerReturnsTransientErrorBeforeSucceeding(t containerStoppingError := dockerapi.DockerContainerMetadata{ Error: dockerapi.CannotStopContainerError{errors.New("Error stopping container")}, } + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) for _, container := range sleepTask.Containers { + expectedCanonicalRef := container.Image + "@" + testDigest.String() gomock.InOrder( imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), - client.EXPECT().PullImage(gomock.Any(), container.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), container.Image, container.RegistryAuthentication). + Return(registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, nil), + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}), + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, container.Image). + Return(nil), imageManager.EXPECT().RecordContainerReference(container), imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false), // Simulate successful create container @@ -945,6 +1005,7 @@ func TestTaskTransitionWhenStopContainerReturnsTransientErrorBeforeSucceeding(t go taskEngine.AddTask(sleepTask) // wait for task running + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) waitForRunningEvents(t, taskEngine.StateChangeEvents()) // Set the task desired status to be stopped and StopContainer will be called updateSleepTask := testdata.LoadTask("sleep5") @@ -1515,6 +1576,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 @@ -1522,8 +1584,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", @@ -1587,6 +1651,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 { @@ -1605,9 +1729,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{ @@ -1615,11 +1740,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) @@ -1627,7 +1760,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") }) } @@ -2261,6 +2407,11 @@ func TestContainerProgressParallize(t *testing.T) { imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).AnyTimes() imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).AnyTimes() client.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + client.EXPECT().WithVersion(dockerclient.Version_1_35).Times(2).Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()).Times(2). + Return(registry.DistributionInspect{}, nil) client.EXPECT().PullImage(gomock.Any(), fastPullImage, gomock.Any(), gomock.Any()) client.EXPECT().PullImage(gomock.Any(), slowPullImage, gomock.Any(), gomock.Any()).Do( func(ctx interface{}, image interface{}, auth interface{}, timeout interface{}) { @@ -2318,10 +2469,13 @@ func TestContainerProgressParallize(t *testing.T) { taskEngine.Init(ctx) taskEngine.AddTask(testTask) - // Expect the fast pulled container to be running firs + // Expect the fast pulled container to be running first fastPullContainerRunning := false for event := range stateChangeEvents { containerEvent, ok := event.(api.ContainerStateChange) + if ok && containerEvent.Status == apicontainerstatus.ContainerManifestPulled { + continue + } if ok && containerEvent.Status == apicontainerstatus.ContainerRunning { if containerEvent.ContainerName == fastPullImage { fastPullContainerRunning = true @@ -2334,6 +2488,9 @@ func TestContainerProgressParallize(t *testing.T) { } taskEvent, ok := event.(api.TaskStateChange) + if ok && taskEvent.Status == apitaskstatus.TaskManifestPulled { + continue + } if ok && taskEvent.Status == apitaskstatus.TaskRunning { break } @@ -3868,3 +4025,891 @@ func TestCreateContainerWithExecAgent(t *testing.T) { }) } } + +func TestPullContainerManifest(t *testing.T) { + type testcase struct { + name string + containerType apicontainer.ContainerType + containerName string + image string + registryAuthData *apicontainer.RegistryAuthenticationData + serviceConnectConfig *serviceconnect.Config + taskResources map[string][]taskresource.TaskResource + imagePullBehavior config.ImagePullBehaviorType + setDockerClientExpectations func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) + expectedResult dockerapi.DockerContainerMetadata + expectedDigest string + } + + someError := errors.New("some error") + testDigest, err := digest.Parse("sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b") + require.NoError(t, err) + tcs := []testcase{ + { + name: "no-op for CNI pause container", + containerType: apicontainer.ContainerCNIPause, + }, + { + name: "no-op for namespace pause container", + containerType: apicontainer.ContainerNamespacePause, + }, + { + name: "no-op for service connect relay container", + containerType: apicontainer.ContainerServiceConnectRelay, + }, + { + name: "no-op for managed daemon container", + containerType: apicontainer.ContainerManagedDaemon, + }, + { + name: "no-op for service connect container", + serviceConnectConfig: &serviceconnect.Config{ContainerName: "my-sc-container"}, + containerName: "my-sc-container", + }, + { + name: "digest is not resolved if already available in image reference", + image: "public.ecr.aws/library/alpine@" + testDigest.String(), + }, + { + name: "image pull not required - image inspect fails", + image: "myimage", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + d.EXPECT().InspectImage("myimage").Return(nil, nil) // no error the first time + d.EXPECT().InspectImage("myimage").Return(nil, someError) // error the second time + }, + expectedResult: dockerapi.DockerContainerMetadata{ + Error: dockerapi.CannotPullImageManifestError{FromError: someError}, + }, + }, + { + name: "image pull not required - inspected image has no repo digests", + image: "myimage", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + inspectResult := &types.ImageInspect{} + d.EXPECT().InspectImage("myimage").Times(2).Return(inspectResult, nil) + }, + expectedResult: dockerapi.DockerContainerMetadata{}, + }, + { + name: "image pull not required - repo digest invalid", + image: "myimage", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + inspectResult := &types.ImageInspect{RepoDigests: []string{"invalid"}} + d.EXPECT().InspectImage("myimage").Times(2).Return(inspectResult, nil) + }, + expectedResult: dockerapi.DockerContainerMetadata{ + Error: dockerapi.CannotPullImageManifestError{ + FromError: errors.New("failed to find a repo digest matching 'myimage'"), + }, + }, + }, + { + name: "image pull not required - repo digest valid", + image: "myimage", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + inspectResult := &types.ImageInspect{ + RepoDigests: []string{"myimage@" + testDigest.String()}, + } + d.EXPECT().InspectImage("myimage").Times(2).Return(inspectResult, nil) + }, + expectedDigest: testDigest.String(), + }, + { + name: "image pull required - required docker API version unsupported", + image: "myimage", + imagePullBehavior: config.ImagePullAlwaysBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + d.EXPECT().WithVersion(dockerclient.Version_1_35).Return(nil, someError) + }, + expectedResult: dockerapi.DockerContainerMetadata{}, + }, + func() testcase { + manifestPullError := dockerapi.CannotPullImageManifestError{FromError: someError} + return testcase{ + name: "image pull required - manifest pull from registry failed", + image: "myimage", + imagePullBehavior: config.ImagePullAlwaysBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + versioned := mock_dockerapi.NewMockDockerClient(c) + versioned.EXPECT(). + PullImageManifest(gomock.Any(), "myimage", nil). + Return(registry.DistributionInspect{}, manifestPullError) + d.EXPECT().WithVersion(dockerclient.Version_1_35).Return(versioned, nil) + }, + expectedResult: dockerapi.DockerContainerMetadata{Error: manifestPullError}, + } + }(), + { + name: "image pull required - manifest pull from public registry succeeded", + image: "myimage", + imagePullBehavior: config.ImagePullAlwaysBehavior, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + versioned := mock_dockerapi.NewMockDockerClient(c) + versioned.EXPECT(). + PullImageManifest(gomock.Any(), "myimage", nil). + Return( + registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, + nil) + d.EXPECT().WithVersion(dockerclient.Version_1_35).Return(versioned, nil) + }, + expectedDigest: testDigest.String(), + }, + func() testcase { + dockerAuthConfig := types.AuthConfig{Username: "user", Password: "pass"} + asmAuthRes := &asmauth.ASMAuthResource{} + asmAuthRes.PutASMDockerAuthConfig("key", dockerAuthConfig) + return testcase{ + name: "image pull required - manifest pull from private registry", + image: "myimage", + imagePullBehavior: config.ImagePullAlwaysBehavior, + registryAuthData: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeASM, + ASMAuthData: &apicontainer.ASMAuthData{CredentialsParameter: "key"}, + }, + taskResources: map[string][]taskresource.TaskResource{ + asmauth.ResourceName: {asmAuthRes}, + }, + setDockerClientExpectations: func(c *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + expectedRegistryAuthData := &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeASM, + ASMAuthData: &apicontainer.ASMAuthData{CredentialsParameter: "key"}, + } + expectedRegistryAuthData.ASMAuthData.SetDockerAuthConfig(dockerAuthConfig) + versioned := mock_dockerapi.NewMockDockerClient(c) + versioned.EXPECT(). + PullImageManifest(gomock.Any(), "myimage", expectedRegistryAuthData). + Return( + registry.DistributionInspect{ + Descriptor: ocispec.Descriptor{Digest: digest.Digest(testDigest.String())}, + }, + nil) + d.EXPECT().WithVersion(dockerclient.Version_1_35).Return(versioned, nil) + }, + expectedDigest: testDigest.String(), + } + }(), + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + cfg := &config.Config{ImagePullBehavior: tc.imagePullBehavior} + ctrl, dockerClient, _, taskEngine, _, _, _, _ := mocks(t, context.Background(), cfg) + defer ctrl.Finish() + + if tc.setDockerClientExpectations != nil { + tc.setDockerClientExpectations(ctrl, dockerClient) + } + engine, ok := taskEngine.(*DockerTaskEngine) + require.True(t, ok) + + container := &apicontainer.Container{ + Image: tc.image, Type: tc.containerType, Name: tc.containerName, + RegistryAuthentication: tc.registryAuthData, + } + task := &apitask.Task{ + Containers: []*apicontainer.Container{container}, + ResourcesMapUnsafe: tc.taskResources, + ServiceConnectConfig: tc.serviceConnectConfig, + } + result := engine.pullContainerManifest(task, container) + assert.Equal(t, tc.expectedResult, result) + assert.Equal(t, tc.expectedDigest, container.GetImageDigest()) + }) + } +} + +// 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 { + name string + imagePullBehavior config.ImagePullBehaviorType + container *apicontainer.Container + setManifestPulledExpectations func( + ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, + ) []*gomock.Call + 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: 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(). + WithVersion(dockerclient.Version_1_35). + Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), testImage, nil). + Return( + registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, + nil), + } + }, + shouldPullImage: true, + }, + { + name: "task should continue if manifest pull fails and pull behavior is default", + imagePullBehavior: config.ImagePullDefaultBehavior, + 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(). + WithVersion(dockerclient.Version_1_35). + Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), testImage, nil). + Return(registry.DistributionInspect{}, dockerapi.CannotPullImageManifestError{ + FromError: errors.New("some error"), + }), + } + }, + 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: testImage, Name: "container"}, + setManifestPulledExpectations: func( + ctrl *gomock.Controller, c *mock_dockerapi.MockDockerClient, i *mock_engine.MockImageManager, + ) []*gomock.Call { + inspectResult := &types.ImageInspect{ + RepoDigests: []string{testImage + "@" + testDigest.String()}, + } + return []*gomock.Call{ + c.EXPECT().InspectImage(testImage).Times(2).Return(inspectResult, nil), + } + }, + shouldPullImage: false, + }, + { + name: "task should continue if manifest pull fails and pull behavior is prefer-cached", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + 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(testImage).Return(nil, errors.New("some error")), + c.EXPECT(). + WithVersion(dockerclient.Version_1_35). + Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), testImage, nil). + Return(registry.DistributionInspect{}, dockerapi.CannotPullImageManifestError{ + FromError: errors.New("some error"), + }), + } + }, + shouldPullImage: true, + shouldPullWithoutCanonicalRef: true, + }, + { + name: "task should continue if manifest pull succeeds and pull behavior is always", + imagePullBehavior: config.ImagePullAlwaysBehavior, + 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(). + WithVersion(dockerclient.Version_1_35). + Return(manifestPullClient, nil), + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), testImage, nil). + Return( + registry.DistributionInspect{Descriptor: ocispec.Descriptor{Digest: testDigest}}, + nil), + } + }, + shouldPullImage: true, + }, + { + name: "task should continue if manifest pull succeeds and pull behavior is once", + imagePullBehavior: config.ImagePullOnceBehavior, + 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{testImage + "@" + testDigest.String()}, + } + return []*gomock.Call{ + i.EXPECT(). + GetImageStateFromImageName(testImage). + Return(&image.ImageState{PullSucceeded: true}, true), + c.EXPECT().InspectImage(testImage).Return(inspectResult, nil), + } + }, + shouldPullImage: false, + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + // Set up config + cfg := config.DefaultConfig() + cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled + cfg.ImagePullBehavior = tc.imagePullBehavior + + // A test task + task := &apitask.Task{ + Containers: []*apicontainer.Container{tc.container}, + Arn: testTaskARN, + DesiredStatusUnsafe: apitaskstatus.TaskRunning, + } + + // Set up task engine and mocks + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ctrl, dockerClient, mockTime, taskEngine, _, imageManager, _, serviceConnectManager := + mocks(t, ctx, &cfg) + defer ctrl.Finish() + serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes().Return("") + serviceConnectManager.EXPECT(). + LoadImage(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes() + + // time.Now() is called to record certain timestamps but we don't care about + // that for this test + mockTime.EXPECT().Now().AnyTimes().Return(time.Now()) + + // Set expectations on mocks for container transition to CREATED and RUNNING + eventStream := make(chan dockerapi.DockerContainerChangeEvent) + dockerClient.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) + imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + + transitionExpectations := []*gomock.Call{} + + // Expectations for transition to MANIFEST_PULLED state + transitionExpectations = append(transitionExpectations, + tc.setManifestPulledExpectations(ctrl, dockerClient, imageManager)...) + + // Expectations for transition to PULLED state + if tc.imagePullBehavior == config.ImagePullOnceBehavior { + // If testing 'once' pull behavior, expect interaction with ImageManager + // when transitioning to PULLED state + transitionExpectations = append(transitionExpectations, + imageManager.EXPECT(). + GetImageStateFromImageName(tc.container.Image). + Return(&image.ImageState{PullSucceeded: true}, true), + ) + } + if tc.imagePullBehavior == config.ImagePullPreferCachedBehavior { + // If testing 'prefer-cached' pull behavior, expect image inspect during + // transition to PULLED state + if tc.shouldPullImage { + transitionExpectations = append(transitionExpectations, + dockerClient.EXPECT(). + InspectImage(tc.container.Image). + Return(nil, errors.New("some error")), + ) + } else { + transitionExpectations = append(transitionExpectations, + dockerClient.EXPECT().InspectImage(tc.container.Image).Return(&types.ImageInspect{ + RepoDigests: []string{tc.container.Image + testDigest.String()}, + }, nil), + ) + } + } + if tc.shouldPullImage { + expectedPullRef := tc.container.Image + if !tc.shouldPullWithoutCanonicalRef { + expectedPullRef = tc.container.Image + "@" + testDigest.String() + } + transitionExpectations = append(transitionExpectations, + dockerClient.EXPECT(). + 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), + imageManager.EXPECT().GetImageStateFromImageName(tc.container.Image).Return(nil, false), + ) + + // Rest of the expectations + var dockerEventsSent sync.WaitGroup + transitionExpectations = append(transitionExpectations, + // Expectations for transition to CREATED + dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), + dockerClient.EXPECT(). + CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Do( + func(ctx interface{}, config *dockercontainer.Config, y interface{}, + containerName string, z time.Duration, + ) { + dockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) + dockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), + + // Expectations for transition to RUNNING + dockerClient.EXPECT(). + StartContainer(gomock.Any(), containerID, cfg.ContainerStartTimeout). + Do( + func(ctx interface{}, id string, timeout time.Duration) { + dockerEventsSent.Wait() + dockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) + dockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), + ) + + gomock.InOrder(transitionExpectations...) + + // Start the task + err := taskEngine.Init(context.Background()) + require.NoError(t, err) + taskEngine.AddTask(task) + + // Wait for the task to reach RUNNING + if !tc.shouldPullWithoutCanonicalRef { + // MANIFEST_PULLED event is emitted only if image digest is resolved + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) + } + waitForRunningEvents(t, taskEngine.StateChangeEvents()) + dockerEventsSent.Wait() + + // Expectations for cleanup + cleanup := make(chan time.Time) + mockTime.EXPECT().After(gomock.Any()).Return(cleanup).MinTimes(1) + containerMap, ok := taskEngine.(*DockerTaskEngine).State().ContainerMapByArn(task.Arn) + require.True(t, ok) + dockerContainer, ok := containerMap[task.Containers[0].Name] + require.True(t, ok) + dockerClient.EXPECT(). + RemoveContainer( + gomock.Any(), dockerContainer.DockerID, dockerclient.RemoveContainerTimeout). + Return(nil) + imageManager.EXPECT().RemoveContainerReferenceFromImageState(gomock.Any()).Return(nil) + + // Simulate container exit + eventStream <- dockerapi.DockerContainerChangeEvent{ + Status: apicontainerstatus.ContainerStopped, + DockerContainerMetadata: dockerapi.DockerContainerMetadata{ + DockerID: containerID, + ExitCode: aws.Int(0), + }, + } + + // StopContainer might be invoked if the test execution is slow, during + // the cleanup phase. Account for that. + dockerClient.EXPECT().StopContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( + dockerapi.DockerContainerMetadata{DockerID: containerID}).AnyTimes() + + // Wait for task to stop + waitForStopEvents(t, taskEngine.StateChangeEvents(), false, false) + + // trigger cleanup, this ensures all the goroutines were finished + task.SetSentStatus(apitaskstatus.TaskStopped) // Needed to unblock cleanup + cleanup <- time.Now() + for { + tasks, _ := taskEngine.(*DockerTaskEngine).ListTasks() + if len(tasks) == 0 { + break + } + time.Sleep(5 * time.Millisecond) + } + }) + } +} + +// This function simulates cases where manifest pull for the task container fails and the +// task should stop. +func TestManifestPullFailuresTaskShouldStop(t *testing.T) { + someError := errors.New("some error") + manifestPullErr := dockerapi.CannotPullImageManifestError{FromError: someError} + tcs := []struct { + name string + imagePullBehavior config.ImagePullBehaviorType + container *apicontainer.Container + setImageManagerExpectations func(i *mock_engine.MockImageManager) + setDockerClientExpectations func(ctrl *gomock.Controller, d *mock_dockerapi.MockDockerClient) + expectedApplyingError error + }{ + { + name: "image pull behavior always", + container: &apicontainer.Container{Image: "myimage", Name: "container"}, + imagePullBehavior: config.ImagePullAlwaysBehavior, + setDockerClientExpectations: func(ctrl *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), "myimage", nil). + Return(registry.DistributionInspect{}, manifestPullErr) + d.EXPECT(). + WithVersion(dockerclient.Version_1_35). + Return(manifestPullClient, nil) + }, + expectedApplyingError: manifestPullErr, + }, + { + name: "image pull behavior once - image found in cache but not on host", + container: &apicontainer.Container{Image: "myimage", Name: "container"}, + imagePullBehavior: config.ImagePullOnceBehavior, + setImageManagerExpectations: func(i *mock_engine.MockImageManager) { + i.EXPECT(). + GetImageStateFromImageName("myimage"). + Return(&image.ImageState{PullSucceeded: true}, true) + }, + setDockerClientExpectations: func(ctrl *gomock.Controller, d *mock_dockerapi.MockDockerClient) { + d.EXPECT().InspectImage("myimage").Return(nil, someError) + }, + expectedApplyingError: dockerapi.CannotPullImageManifestError{FromError: someError}, + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + // Set up config + cfg := config.DefaultConfig() + cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled + cfg.ImagePullBehavior = tc.imagePullBehavior + + // A test task + task := &apitask.Task{ + Containers: []*apicontainer.Container{tc.container}, + Arn: testTaskARN, + DesiredStatusUnsafe: apitaskstatus.TaskRunning, + } + + // Set up task engine and mocks + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ctrl, dockerClient, mockTime, taskEngine, _, imageManager, _, serviceConnectManager := + mocks(t, ctx, &cfg) + defer ctrl.Finish() + serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes() + serviceConnectManager.EXPECT(). + LoadImage(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes() + eventStream := make(chan dockerapi.DockerContainerChangeEvent) + dockerClient.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) + cleanup := make(chan time.Time) + if tc.setImageManagerExpectations != nil { + tc.setImageManagerExpectations(imageManager) + } + if tc.setDockerClientExpectations != nil { + tc.setDockerClientExpectations(ctrl, dockerClient) + } + + // We don't care about interaction with time for this test + mockTime.EXPECT().Now().AnyTimes().Return(time.Now()) + mockTime.EXPECT().After(gomock.Any()).Return(cleanup).AnyTimes() + + // Start the task + err := taskEngine.Init(context.Background()) + require.NoError(t, err) + taskEngine.AddTask(task) + + // Verify that the task fails and the error is captured in the container. + verifyTaskIsStopped(taskEngine.StateChangeEvents(), task) + assert.Equal(t, + apierrors.NewNamedError(tc.expectedApplyingError), + tc.container.ApplyingError) + }) + } +} + +func TestImagePullRequired(t *testing.T) { + tcs := []struct { + name string + imagePullBehavior config.ImagePullBehaviorType + container *apicontainer.Container + setImageManagerExpectations func(i *mock_engine.MockImageManager) + setDockerClientExpectations func(d *mock_dockerapi.MockDockerClient) + expected bool + }{ + { + name: "always required if pull behavior is always", + imagePullBehavior: config.ImagePullAlwaysBehavior, + container: &apicontainer.Container{Image: "myimage"}, + expected: true, + }, + { + name: "always required if pull behavior is default", + imagePullBehavior: config.ImagePullDefaultBehavior, + container: &apicontainer.Container{Image: "myimage"}, + expected: true, + }, + { + name: "required if behavior is once and image is not in cache", + imagePullBehavior: config.ImagePullOnceBehavior, + container: &apicontainer.Container{Image: "myimage"}, + setImageManagerExpectations: func(i *mock_engine.MockImageManager) { + i.EXPECT(). + GetImageStateFromImageName("myimage"). + Return(nil, false) + }, + expected: true, + }, + { + name: "required if behavior is once, image is in cache but last pull was unsuccessful", + imagePullBehavior: config.ImagePullOnceBehavior, + container: &apicontainer.Container{Image: "myimage"}, + setImageManagerExpectations: func(i *mock_engine.MockImageManager) { + i.EXPECT(). + GetImageStateFromImageName("myimage"). + Return(&image.ImageState{PullSucceeded: false}, true) + }, + expected: true, + }, + { + name: "not required if behavior is once and image is in cache", + imagePullBehavior: config.ImagePullOnceBehavior, + container: &apicontainer.Container{Image: "myimage"}, + setImageManagerExpectations: func(i *mock_engine.MockImageManager) { + i.EXPECT(). + GetImageStateFromImageName("myimage"). + Return(&image.ImageState{PullSucceeded: true}, true) + }, + expected: false, + }, + { + name: "required if behavior is prefer-cached and image is not on host", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + container: &apicontainer.Container{Image: "myimage"}, + setDockerClientExpectations: func(d *mock_dockerapi.MockDockerClient) { + d.EXPECT().InspectImage("myimage").Return(nil, errors.New("not found")) + }, + expected: true, + }, + { + name: "not required if behavior is prefer-cached and image is on the host", + imagePullBehavior: config.ImagePullPreferCachedBehavior, + container: &apicontainer.Container{Image: "myimage"}, + setDockerClientExpectations: func(d *mock_dockerapi.MockDockerClient) { + d.EXPECT().InspectImage("myimage").Return(&types.ImageInspect{}, nil) + }, + expected: false, + }, + } + for _, tc := range tcs { + ctrl, dockerClient, _, taskEngine, _, imageManager, _, _ := + mocks(t, context.Background(), &defaultConfig) + defer ctrl.Finish() + + if tc.setImageManagerExpectations != nil { + tc.setImageManagerExpectations(imageManager) + } + if tc.setDockerClientExpectations != nil { + tc.setDockerClientExpectations(dockerClient) + } + + res := taskEngine.(*DockerTaskEngine).imagePullRequired(tc.imagePullBehavior, tc.container, "") + assert.Equal(t, tc.expected, res) + } +} + +func TestSetRegistryCredentials(t *testing.T) { + tcs := []struct { + name string + task *apitask.Task + setCredsManagerExpectations func(c *mock_credentials.MockManager) + expectedCreds *apicontainer.RegistryAuthenticationData + expectedCredsAfterCleanup *apicontainer.RegistryAuthenticationData + expectedError string + expectCleanup bool + }{ + { + name: "no creds needed", + task: &apitask.Task{Containers: []*apicontainer.Container{{}}}, + expectedCreds: nil, + expectedError: "", + expectedCredsAfterCleanup: nil, + expectCleanup: false, + }, + { + name: "execution role creds not found", + task: &apitask.Task{ + Containers: []*apicontainer.Container{ + { + RegistryAuthentication: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeECR, + ECRAuthData: &apicontainer.ECRAuthData{UseExecutionRole: true}, + }, + }, + }, + ExecutionCredentialsID: "exec-id", + }, + setCredsManagerExpectations: func(c *mock_credentials.MockManager) { + c.EXPECT(). + GetTaskCredentials("exec-id"). + Return(credentials.TaskIAMRoleCredentials{}, false) + }, + expectedCreds: nil, + expectedError: "engine ecr credentials: not found", + expectCleanup: false, + }, + { + name: "execution role creds success", + task: &apitask.Task{ + Containers: []*apicontainer.Container{ + { + RegistryAuthentication: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeECR, + ECRAuthData: &apicontainer.ECRAuthData{UseExecutionRole: true}, + }, + }, + }, + ExecutionCredentialsID: "exec-id", + }, + setCredsManagerExpectations: func(c *mock_credentials.MockManager) { + c.EXPECT(). + GetTaskCredentials("exec-id"). + Return( + credentials.TaskIAMRoleCredentials{ + IAMRoleCredentials: credentials.IAMRoleCredentials{ + AccessKeyID: "access-key-id", + }}, + true) + }, + expectedCreds: func() *apicontainer.RegistryAuthenticationData { + ecrAuthData := &apicontainer.ECRAuthData{UseExecutionRole: true} + ecrAuthData.SetPullCredentials(credentials.IAMRoleCredentials{AccessKeyID: "access-key-id"}) + creds := &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeECR, + ECRAuthData: ecrAuthData, + } + return creds + }(), + expectedError: "", + expectCleanup: true, + }, + { + name: "execution role creds not needed", + task: &apitask.Task{ + Containers: []*apicontainer.Container{ + { + RegistryAuthentication: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeECR, + ECRAuthData: &apicontainer.ECRAuthData{UseExecutionRole: false}, + }, + }, + }, + }, + expectedCreds: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeECR, + ECRAuthData: &apicontainer.ECRAuthData{UseExecutionRole: false}, + }, + expectedError: "", + expectCleanup: false, + }, + { + name: "asm auth creds not found", + task: &apitask.Task{ + Containers: []*apicontainer.Container{ + { + RegistryAuthentication: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeASM, + ASMAuthData: &apicontainer.ASMAuthData{}, + }, + }, + }, + }, + expectedError: "engine docker private registry credentials: not found", + expectCleanup: false, + }, + { + name: "asm auth creds success", + task: func() *apitask.Task { + asmAuthRes := &asmauth.ASMAuthResource{} + authData := types.AuthConfig{Username: "user", Password: "pass"} + asmAuthRes.PutASMDockerAuthConfig("key", authData) + t := &apitask.Task{ + Containers: []*apicontainer.Container{ + { + RegistryAuthentication: &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeASM, + ASMAuthData: &apicontainer.ASMAuthData{CredentialsParameter: "key"}, + }, + }, + }, + ResourcesMapUnsafe: map[string][]taskresource.TaskResource{ + asmauth.ResourceName: {asmAuthRes}, + }, + } + return t + }(), + expectedCreds: func() *apicontainer.RegistryAuthenticationData { + authData := types.AuthConfig{Username: "user", Password: "pass"} + asmAuthData := &apicontainer.ASMAuthData{CredentialsParameter: "key"} + asmAuthData.SetDockerAuthConfig(authData) + creds := &apicontainer.RegistryAuthenticationData{ + Type: apicontainer.AuthTypeASM, + ASMAuthData: asmAuthData, + } + return creds + }(), + expectedError: "", + expectCleanup: true, + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + // Prepare task engine and its dependencies + ctrl, _, _, taskEngine, credsManager, _, _, _ := mocks(t, context.Background(), &defaultConfig) + defer ctrl.Finish() + + // Set expectations on credentials manager if needed + if tc.setCredsManagerExpectations != nil { + tc.setCredsManagerExpectations(credsManager) + } + + // Backup current state of registry auth data to use later + container := tc.task.Containers[0] + var regCredsBefore apicontainer.RegistryAuthenticationData + if tc.expectCleanup { + regCredsBefore = *container.RegistryAuthentication + } + + // Test + cleanup, err := taskEngine.(*DockerTaskEngine).setRegistryCredentials(container, tc.task) + if tc.expectedError != "" { + assert.EqualError(t, err, tc.expectedError) + assert.Nil(t, cleanup) + } else { + require.Nil(t, err) + assert.Equal(t, tc.expectedCreds, container.RegistryAuthentication) + if tc.expectCleanup { + // Registry auth data should be reset to original state + require.NotNil(t, cleanup) + cleanup() + assert.Equal(t, ®CredsBefore, container.RegistryAuthentication) + } else { + require.Nil(t, cleanup) + } + } + }) + } +} diff --git a/agent/engine/docker_task_engine_windows_test.go b/agent/engine/docker_task_engine_windows_test.go index 6df3c606583..85f826b9cb6 100644 --- a/agent/engine/docker_task_engine_windows_test.go +++ b/agent/engine/docker_task_engine_windows_test.go @@ -25,10 +25,12 @@ import ( "time" mock_asm_factory "github.com/aws/amazon-ecs-agent/agent/asm/factory/mocks" + "github.com/aws/amazon-ecs-agent/agent/dockerclient" apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container" apitask "github.com/aws/amazon-ecs-agent/agent/api/task" "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi" + mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks" "github.com/aws/amazon-ecs-agent/agent/ecscni" mock_ecscni "github.com/aws/amazon-ecs-agent/agent/ecscni/mocks" mock_dockerstate "github.com/aws/amazon-ecs-agent/agent/engine/dockerstate/mocks" @@ -43,7 +45,9 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/docker/docker/api/types" dockercontainer "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/registry" "github.com/golang/mock/gomock" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -318,7 +322,21 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { // parallel. The dependency graph enforcement comes into effect for CREATED transitions. // Hence, do not enforce the order of invocation of these calls imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}) + imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + client.EXPECT().WithVersion(dockerclient.Version_1_35).Return(manifestPullClient, nil) + expectedCanonicalRef := sleepContainer.Image + "@" + testDigest.String() + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), sleepContainer.Image, nil). + Return(registry.DistributionInspect{ + Descriptor: ocispec.Descriptor{Digest: testDigest}, + }, nil) + client.EXPECT(). + PullImage(gomock.Any(), expectedCanonicalRef, nil, gomock.Any()). + Return(dockerapi.DockerContainerMetadata{}) + client.EXPECT(). + TagImage(gomock.Any(), expectedCanonicalRef, sleepContainer.Image). + Return(nil) imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil) imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false) @@ -515,6 +533,15 @@ func TestPauseContainerHappyPath(t *testing.T) { // For the other container imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) + dockerClient.EXPECT(). + WithVersion(dockerclient.Version_1_35). + Times(2). + Return(manifestPullClient, nil) + manifestPullClient.EXPECT(). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()). + Times(2). + Return(registry.DistributionInspect{}, nil) dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(2) diff --git a/agent/engine/engine_integ_test.go b/agent/engine/engine_integ_test.go index c43c9912e76..ae50b469f76 100644 --- a/agent/engine/engine_integ_test.go +++ b/agent/engine/engine_integ_test.go @@ -31,6 +31,7 @@ import ( apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container" apitask "github.com/aws/amazon-ecs-agent/agent/api/task" "github.com/aws/amazon-ecs-agent/agent/config" + "github.com/aws/amazon-ecs-agent/agent/dockerclient" "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi" "github.com/aws/amazon-ecs-agent/agent/dockerclient/sdkclientfactory" "github.com/aws/amazon-ecs-agent/agent/engine/dockerstate" @@ -62,6 +63,9 @@ const ( "StartPeriod":100000000, "Retries":3} }` + + // busybox image avaialble in a local registry set up by `make test-registry` + localRegistryBusyboxImage = "127.0.0.1:51670/busybox" ) func init() { @@ -77,13 +81,6 @@ func setupWithState(t *testing.T, state dockerstate.TaskEngineState) (TaskEngine return setup(defaultTestConfigIntegTest(), state, t) } -func verifyTaskRunningStateChange(t *testing.T, taskEngine TaskEngine) { - stateChangeEvents := taskEngine.StateChangeEvents() - event := <-stateChangeEvents - assert.Equal(t, event.(api.TaskStateChange).Status, apitaskstatus.TaskRunning, - "Expected task to be RUNNING") -} - func verifyTaskRunningStateChangeWithRuntimeID(t *testing.T, taskEngine TaskEngine) { stateChangeEvents := taskEngine.StateChangeEvents() event := <-stateChangeEvents @@ -203,6 +200,8 @@ func TestHostVolumeMount(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -230,6 +229,8 @@ func TestSweepContainer(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -272,6 +273,8 @@ func TestStartStopWithCredentials(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -291,6 +294,8 @@ func TestStartStopWithRuntimeID(t *testing.T) { testTask := createTestTask("testTaskWithContainerID") go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChangeWithRuntimeID(t, taskEngine) verifyTaskRunningStateChangeWithRuntimeID(t, taskEngine) verifyContainerStoppedStateChangeWithRuntimeID(t, taskEngine) @@ -325,6 +330,8 @@ func TestContainerHealthCheck(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) @@ -350,6 +357,8 @@ func TestEngineSynchronize(t *testing.T) { // Start a task go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) // Record the container information @@ -432,6 +441,8 @@ func TestLabels(t *testing.T) { "label1":"" }}`)} go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -474,6 +485,8 @@ func TestLogDriverOptions(t *testing.T) { } }}`)} go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -522,6 +535,8 @@ func testNetworkMode(t *testing.T, networkMode string) { HostConfig: aws.String(fmt.Sprintf(`{"NetworkMode":"%s"}`, networkMode))} go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -566,6 +581,8 @@ func TestTaskCleanup(t *testing.T) { testTask := createTestTask(testArn) go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -596,29 +613,345 @@ func TestTaskCleanup(t *testing.T) { // Tests that containers with ordering dependencies are able to reach MANIFEST_PULLED state // regardless of the dependencies. func TestManifestPulledDoesNotDependOnContainerOrdering(t *testing.T) { - taskEngine, done, _ := setupWithDefaultConfig(t) + imagePullBehaviors := []config.ImagePullBehaviorType{ + config.ImagePullDefaultBehavior, config.ImagePullAlwaysBehavior, + config.ImagePullPreferCachedBehavior, config.ImagePullOnceBehavior, + } + + for _, behavior := range imagePullBehaviors { + 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 = []string{"sh", "-c", "sleep 60"} + + second := createTestContainerWithImageAndName(testRegistryImage, "second") + second.SetDependsOn([]apicontainer.DependsOn{ + {ContainerName: first.Name, Condition: "COMPLETE"}, + }) + + task := &apitask.Task{ + Arn: "test-arn", + Family: "family", + Version: "1", + DesiredStatusUnsafe: apitaskstatus.TaskRunning, + Containers: []*apicontainer.Container{first, second}, + } + + // Start the task and wait for first container to start running + go taskEngine.AddTask(task) + + // Both containers and the task should reach MANIFEST_PULLED state and emit events for it + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + + // The first container should start running + verifyContainerRunningStateChange(t, taskEngine) + + // The first container should be in RUNNING state + assert.Equal(t, apicontainerstatus.ContainerRunning, first.GetKnownStatus()) + // The second container should be waiting in MANIFEST_PULLED state + assert.Equal(t, apicontainerstatus.ContainerManifestPulled, second.GetKnownStatus()) + + // Assert that both containers have digest populated + assert.NotEmpty(t, first.GetImageDigest()) + assert.NotEmpty(t, 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, testRegistryImage) + }) + } +} + +// Integration test for pullContainerManifest. +// The test depends on 127.0.0.1:51670/busybox image that is prepared by `make test-registry` +// command. +func TestPullContainerManifestInteg(t *testing.T) { + allPullBehaviors := []config.ImagePullBehaviorType{ + config.ImagePullDefaultBehavior, config.ImagePullAlwaysBehavior, + config.ImagePullOnceBehavior, config.ImagePullPreferCachedBehavior, + } + tcs := []struct { + name string + image string + setConfig func(c *config.Config) + imagePullBehaviors []config.ImagePullBehaviorType + digestResolutionNotNeeded bool + assertError func(t *testing.T, err error) + }{ + { + name: "digest available in image reference", + image: "ubuntu@sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966", + digestResolutionNotNeeded: true, + imagePullBehaviors: allPullBehaviors, + }, + { + name: "digest can be resolved from explicit tag", + image: localRegistryBusyboxImage, + imagePullBehaviors: allPullBehaviors, + }, + { + name: "digest can be resolved without an explicit tag", + image: localRegistryBusyboxImage, + imagePullBehaviors: allPullBehaviors, + }, + { + name: "manifest pull can timeout", + image: localRegistryBusyboxImage, + setConfig: func(c *config.Config) { c.ManifestPullTimeout = 0 }, + imagePullBehaviors: []config.ImagePullBehaviorType{config.ImagePullAlwaysBehavior}, + + assertError: func(t *testing.T, err error) { + assert.ErrorContains(t, err, "Could not transition to MANIFEST_PULLED; timed out") + }, + }, + { + name: "manifest pull can timeout - non-zero timeout", + image: localRegistryBusyboxImage, + setConfig: func(c *config.Config) { c.ManifestPullTimeout = 100 * time.Microsecond }, + imagePullBehaviors: []config.ImagePullBehaviorType{config.ImagePullAlwaysBehavior}, + assertError: func(t *testing.T, err error) { + assert.ErrorContains(t, err, "Could not transition to MANIFEST_PULLED; timed out") + }, + }, + } + for _, tc := range tcs { + for _, imagePullBehavior := range tc.imagePullBehaviors { + t.Run(fmt.Sprintf("%s - %v", tc.name, imagePullBehavior), func(t *testing.T) { + cfg := defaultTestConfigIntegTest() + cfg.ImagePullBehavior = imagePullBehavior + + if tc.setConfig != nil { + tc.setConfig(cfg) + } + + taskEngine, done, _ := setup(cfg, nil, t) + defer done() + + container := &apicontainer.Container{Image: tc.image} + task := &apitask.Task{Containers: []*apicontainer.Container{container}} + + res := taskEngine.(*DockerTaskEngine).pullContainerManifest(task, container) + if tc.assertError == nil { + require.NoError(t, res.Error) + if tc.digestResolutionNotNeeded { + assert.Empty(t, container.GetImageDigest()) + } else { + assert.NotEmpty(t, container.GetImageDigest()) + } + } else { + tc.assertError(t, res.Error) + } + }) + } + } +} + +// 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) +} + +// Tests that a task with invalid image fails as expected. +func TestInvalidImageInteg(t *testing.T) { + tcs := []struct { + name string + image string + expectedError string + }{ + { + name: "repo not found - fails during digest resolution", + image: "127.0.0.1:51670/invalid-image", + expectedError: "CannotPullImageManifestError: Error response from daemon: manifest" + + " unknown: manifest unknown", + }, + { + name: "invalid digest provided - fails during pull", + image: "127.0.0.1:51670/busybox" + + "@sha256:0000000000000000000000000000000000000000000000000000000000000000", + expectedError: "CannotPullContainerError: Error response from daemon: manifest for" + + " 127.0.0.1:51670/busybox" + + "@sha256:0000000000000000000000000000000000000000000000000000000000000000" + + " not found: manifest unknown: manifest unknown", + }, + } - first := createTestContainerWithImageAndName(testRegistryImage, "first") - first.Command = getLongRunningCommand() + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + // Prepare task engine + cfg := defaultTestConfigIntegTest() + cfg.ImagePullBehavior = config.ImagePullAlwaysBehavior + taskEngine, done, _ := setup(cfg, nil, t) + defer done() + + // Prepare a task + container := createTestContainerWithImageAndName(tc.image, "container") + task := &apitask.Task{ + Arn: "test-arn", + Family: "family", + Version: "1", + DesiredStatusUnsafe: apitaskstatus.TaskRunning, + Containers: []*apicontainer.Container{container}, + } + + // Start the task + go taskEngine.AddTask(task) + + // The container and the task both should stop + verifyContainerStoppedStateChangeWithReason(t, taskEngine, tc.expectedError) + verifyTaskStoppedStateChange(t, taskEngine) + }) + } +} - second := createTestContainerWithImageAndName(testRegistryImage, "second") - second.SetDependsOn([]apicontainer.DependsOn{{ContainerName: first.Name, Condition: "COMPLETE"}}) +// Tests that a task with an image that has a digest specified works normally. +func TestImageWithDigestInteg(t *testing.T) { + // Prepare task engine + cfg := defaultTestConfigIntegTest() + cfg.ImagePullBehavior = config.ImagePullAlwaysBehavior + taskEngine, done, _ := setup(cfg, nil, t) + defer done() + // Find image digest + versionedClient, err := taskEngine.(*DockerTaskEngine).client.WithVersion(dockerclient.Version_1_35) + manifest, manifestPullError := versionedClient.PullImageManifest( + context.Background(), localRegistryBusyboxImage, nil) + require.NoError(t, manifestPullError) + imageDigest := manifest.Descriptor.Digest.String() + + // Prepare a task with image digest + container := createTestContainerWithImageAndName( + localRegistryBusyboxImage+"@"+imageDigest, "container") + container.Command = []string{"sh", "-c", "sleep 1"} task := &apitask.Task{ Arn: "test-arn", Family: "family", Version: "1", DesiredStatusUnsafe: apitaskstatus.TaskRunning, - Containers: []*apicontainer.Container{first, second}, + Containers: []*apicontainer.Container{container}, } - // Start the task and wait for first container to start running + // Start the task go taskEngine.AddTask(task) + + // The task should run. No MANIFEST_PULLED events expected. verifyContainerRunningStateChange(t, taskEngine) + verifyTaskRunningStateChange(t, taskEngine) + assert.Equal(t, imageDigest, container.GetImageDigest()) - // The first container should be in RUNNING state - assert.Equal(t, apicontainerstatus.ContainerRunning, first.GetKnownStatus()) - // The second container should be waiting in MANIFEST_PULLED state - assert.Equal(t, apicontainerstatus.ContainerManifestPulled, second.GetKnownStatus()) + // Cleanup + container.SetDesiredStatus(apicontainerstatus.ContainerStopped) + verifyContainerStoppedStateChange(t, taskEngine) + verifyTaskStoppedStateChange(t, taskEngine) + err = taskEngine.(*DockerTaskEngine).removeContainer(task, container) + require.NoError(t, err, "failed to remove container during cleanup") + removeImage(t, localRegistryBusyboxImage) } diff --git a/agent/engine/engine_sudo_linux_integ_test.go b/agent/engine/engine_sudo_linux_integ_test.go index decf3334aa2..6a41960eafb 100644 --- a/agent/engine/engine_sudo_linux_integ_test.go +++ b/agent/engine/engine_sudo_linux_integ_test.go @@ -47,7 +47,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/aws/amazon-ecs-agent/agent/api" apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container" apitask "github.com/aws/amazon-ecs-agent/agent/api/task" "github.com/aws/amazon-ecs-agent/agent/config" @@ -130,6 +129,9 @@ func TestStartStopWithCgroup(t *testing.T) { } go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) @@ -167,6 +169,8 @@ func TestLocalHostVolumeMount(t *testing.T) { stateChangeEvents := taskEngine.StateChangeEvents() go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) verifyContainerStoppedStateChange(t, taskEngine) @@ -434,6 +438,8 @@ func TestExecCommandAgent(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -526,6 +532,8 @@ func TestManagedAgentEvent(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -794,13 +802,6 @@ func killMockExecCommandAgent(t *testing.T, client *sdkClient.Client, containerI require.NoError(t, err) } -func verifyTaskRunningStateChange(t *testing.T, taskEngine TaskEngine) { - stateChangeEvents := taskEngine.StateChangeEvents() - event := <-stateChangeEvents - assert.Equal(t, event.(api.TaskStateChange).Status, apitaskstatus.TaskRunning, - "Expected task to be RUNNING") -} - func TestGMSATaskFile(t *testing.T) { t.Setenv("ECS_GMSA_SUPPORTED", "True") t.Setenv("ZZZ_SKIP_DOMAIN_JOIN_CHECK_NOT_SUPPORTED_IN_PRODUCTION", "True") diff --git a/agent/engine/engine_unix_integ_test.go b/agent/engine/engine_unix_integ_test.go index 57d2a40eea0..e6f743051e7 100644 --- a/agent/engine/engine_unix_integ_test.go +++ b/agent/engine/engine_unix_integ_test.go @@ -269,6 +269,8 @@ func TestStartStopUnpulledImage(t *testing.T) { testTask := createTestTask("testStartUnpulled") go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -449,6 +451,10 @@ func TestDynamicPortForward(t *testing.T) { go taskEngine.AddTask(testTask) event := <-stateChangeEvents + require.Equal(t, apicontainerstatus.ContainerManifestPulled, event.(api.ContainerStateChange).Status, "Expected container to reach MANIFEST_PULLED state") + event = <-stateChangeEvents + require.Equal(t, apitaskstatus.TaskManifestPulled, event.(api.TaskStateChange).Status, "Expected task to reach MANIFEST_PULLED state") + event = <-stateChangeEvents require.Equal(t, event.(api.ContainerStateChange).Status, apicontainerstatus.ContainerRunning, "Expected container to be RUNNING") portBindings := event.(api.ContainerStateChange).PortBindings @@ -504,6 +510,10 @@ func TestMultipleDynamicPortForward(t *testing.T) { go taskEngine.AddTask(testTask) event := <-stateChangeEvents + require.Equal(t, apicontainerstatus.ContainerManifestPulled, event.(api.ContainerStateChange).Status, "Expected container to reach MANIFEST_PULLED state") + event = <-stateChangeEvents + require.Equal(t, apitaskstatus.TaskManifestPulled, event.(api.TaskStateChange).Status, "Expected task to reach MANIFEST_PULLED state") + event = <-stateChangeEvents require.Equal(t, event.(api.ContainerStateChange).Status, apicontainerstatus.ContainerRunning, "Expected container to be RUNNING") portBindings := event.(api.ContainerStateChange).PortBindings @@ -699,6 +709,8 @@ func TestInitOOMEvent(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -753,6 +765,8 @@ func TestSignalEvent(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -814,6 +828,8 @@ func TestDockerStopTimeout(t *testing.T) { go dockerTaskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -840,6 +856,8 @@ func TestStartStopWithSecurityOptionNoNewPrivileges(t *testing.T) { go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -883,6 +901,8 @@ func TestSwapConfigurationTask(t *testing.T) { testTask.Containers[0].DockerConfig = apicontainer.DockerConfig{HostConfig: aws.String(`{"MemorySwap":314572800, "MemorySwappiness":90}`)} go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -930,6 +950,8 @@ func TestPerContainerStopTimeout(t *testing.T) { go dockerTaskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -961,6 +983,8 @@ func TestMemoryOverCommit(t *testing.T) { "MemoryReservation": 52428800 }`)} go taskEngine.AddTask(testTask) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -1021,6 +1045,8 @@ func TestFluentdTag(t *testing.T) { SourceVolume: "logs"}} testTaskFleuntdDriver.Containers[0].Ports = []apicontainer.PortBinding{{ContainerPort: 24224, HostPort: 24224}} go taskEngine.AddTask(testTaskFleuntdDriver) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -1041,6 +1067,8 @@ func TestFluentdTag(t *testing.T) { }}`)} go taskEngine.AddTask(testTaskFluentdLogTag) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskRunningStateChange(t, taskEngine) @@ -1101,6 +1129,8 @@ func TestDockerExecAPI(t *testing.T) { finished := make(chan interface{}) go func() { // Both containers should start + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) @@ -1176,9 +1206,13 @@ func TestHostResourceManagerTrickleQueue(t *testing.T) { // goroutine to verify task running order go func() { // Tasks go RUNNING in order + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[0]) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[1]) @@ -1186,6 +1220,8 @@ func TestHostResourceManagerTrickleQueue(t *testing.T) { verifyContainerStoppedStateChange(t, taskEngine) verifyTaskIsStopped(stateChangeEvents, tasks[0]) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[2]) @@ -1264,9 +1300,13 @@ func TestHostResourceManagerResourceUtilization(t *testing.T) { go func() { // Tasks go RUNNING in order, 2nd task doesn't wait for 1st task // to transition to STOPPED as resources are available + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[0]) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[1]) @@ -1350,6 +1390,10 @@ func TestHostResourceManagerStopTaskNotBlockWaitingTasks(t *testing.T) { // goroutine to verify task running order and verify assertions go func() { + // First task goes to MANIFEST_PULLED + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // 1st task goes to RUNNING verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, tasks[0]) @@ -1455,6 +1499,8 @@ func TestHostResourceManagerLaunchTypeBehavior(t *testing.T) { // goroutine to verify task running order and verify assertions go func() { // Task goes to RUNNING + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) diff --git a/agent/engine/ordering_integ_test.go b/agent/engine/ordering_integ_test.go index 829dd6137ef..d4a2e008f56 100644 --- a/agent/engine/ordering_integ_test.go +++ b/agent/engine/ordering_integ_test.go @@ -75,6 +75,9 @@ func TestDependencyHealthCheck(t *testing.T) { finished := make(chan interface{}) go func() { // Both containers should start + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyTaskIsRunning(stateChangeEvents, testTask) @@ -131,6 +134,11 @@ func TestDependencyComplete(t *testing.T) { finished := make(chan interface{}) go func() { + // Both containers and the task should reach MANIFEST_PULLED regardless of ordering + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // First container should run to completion and then exit verifyContainerRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -186,6 +194,11 @@ func TestDependencyStart(t *testing.T) { finished := make(chan interface{}) go func() { + // Both containers and the task should go to MANIFEST_PULLED state + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // 'dependency' container should run first, followed by the 'parent' container verifySpecificContainerStateChange(t, taskEngine, "dependency", status.ContainerRunning) verifySpecificContainerStateChange(t, taskEngine, "parent", status.ContainerRunning) @@ -245,6 +258,11 @@ func TestDependencySuccess(t *testing.T) { finished := make(chan interface{}) go func() { + // All containers and the task should reach MANIFEST_PULLED + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // First container should run to completion verifyContainerRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -304,6 +322,11 @@ func TestDependencySuccessErrored(t *testing.T) { finished := make(chan interface{}) go func() { + // Both containers and the task should reach MANIFEST_PULLED state + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // First container should run to completion verifyContainerRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -360,6 +383,12 @@ func TestDependencySuccessTimeout(t *testing.T) { finished := make(chan interface{}) go func() { + // All containers and the task should reach MANIFEST_PULLED + for _ = range testTask.Containers { + verifyContainerManifestPulledStateChange(t, taskEngine) + } + verifyTaskManifestPulledStateChange(t, taskEngine) + // First container should run to completion verifyContainerRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -423,6 +452,11 @@ func TestDependencyHealthyTimeout(t *testing.T) { finished := make(chan interface{}) go func() { + // Both containers and the task should reach MANIFEST_PULLED + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + // First container should run to completion verifyContainerRunningStateChange(t, taskEngine) verifyContainerStoppedStateChange(t, taskEngine) @@ -500,6 +534,11 @@ func TestShutdownOrder(t *testing.T) { finished := make(chan interface{}) go func() { // Everything should first progress to running + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) @@ -589,6 +628,12 @@ func TestMultipleContainerDependency(t *testing.T) { finished := make(chan interface{}) go func() { + // All containers and the task should reach MANIFEST_PULLED regardless of dependency + for _ = range testTask.Containers { + verifyContainerManifestPulledStateChange(t, taskEngine) + } + verifyTaskManifestPulledStateChange(t, taskEngine) + // Only exit should first progress to running verifyContainerRunningStateChange(t, taskEngine) diff --git a/agent/engine/ordering_integ_unix_test.go b/agent/engine/ordering_integ_unix_test.go index 1878e4bdb54..6a45a37e45a 100644 --- a/agent/engine/ordering_integ_unix_test.go +++ b/agent/engine/ordering_integ_unix_test.go @@ -77,6 +77,11 @@ func TestGranularStopTimeout(t *testing.T) { finished := make(chan interface{}) go func() { + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyContainerManifestPulledStateChange(t, taskEngine) + verifyTaskManifestPulledStateChange(t, taskEngine) + verifyContainerRunningStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) verifyContainerRunningStateChange(t, taskEngine) diff --git a/agent/engine/task_manager.go b/agent/engine/task_manager.go index 9f4703c6b5a..661e1f90d2c 100644 --- a/agent/engine/task_manager.go +++ b/agent/engine/task_manager.go @@ -604,6 +604,7 @@ func getContainerEventLogFields(c api.ContainerStateChange) logger.Fields { if c.Container != nil { f["KnownSent"] = c.Container.GetSentStatus().String() } + f["KnownStatus"] = c.Container.GetKnownStatus() return f } @@ -618,7 +619,7 @@ func (mtask *managedTask) emitTaskEvent(task *apitask.Task, reason string) { logger.Critical("Failed to release resources after tast stopped", logger.Fields{field.TaskARN: mtask.Arn}) } } - if !taskKnownStatus.BackendRecognized() { + if taskKnownStatus != apitaskstatus.TaskManifestPulled && !taskKnownStatus.BackendRecognized() { logger.Debug("Skipping event emission for task", logger.Fields{ field.TaskID: mtask.GetID(), field.Error: "status not recognized by ECS", @@ -629,7 +630,7 @@ func (mtask *managedTask) emitTaskEvent(task *apitask.Task, reason string) { event, err := api.NewTaskStateChangeEvent(task, reason) if err != nil { if _, ok := err.(api.ErrShouldNotSendEvent); ok { - logger.Debug(err.Error()) + logger.Debug(err.Error(), logger.Fields{field.TaskID: mtask.GetID()}) } else { logger.Error("Skipping emitting event for task due to error", logger.Fields{ field.TaskID: mtask.GetID(), @@ -698,7 +699,10 @@ func (mtask *managedTask) emitContainerEvent(task *apitask.Task, cont *apicontai event, err := api.NewContainerStateChangeEvent(task, cont, reason) if err != nil { if _, ok := err.(api.ErrShouldNotSendEvent); ok { - logger.Debug(err.Error()) + logger.Debug(err.Error(), logger.Fields{ + field.TaskID: mtask.GetID(), + field.Container: cont.Name, + }) } else { logger.Error("Skipping emitting event for container due to error", logger.Fields{ field.TaskID: mtask.GetID(), diff --git a/agent/taskresource/asmauth/asmauth.go b/agent/taskresource/asmauth/asmauth.go index de109482c0a..788a042ff15 100644 --- a/agent/taskresource/asmauth/asmauth.go +++ b/agent/taskresource/asmauth/asmauth.go @@ -336,6 +336,16 @@ func (auth *ASMAuthResource) GetASMDockerAuthConfig(secretID string) (types.Auth return d, ok } +// Stores provided docker auth config against the provided secret ID. +func (auth *ASMAuthResource) PutASMDockerAuthConfig(secretID string, authCfg types.AuthConfig) { + auth.lock.Lock() + defer auth.lock.Unlock() + if auth.dockerAuthData == nil { + auth.dockerAuthData = make(map[string]types.AuthConfig) + } + auth.dockerAuthData[secretID] = authCfg +} + func (auth *ASMAuthResource) Initialize(resourceFields *taskresource.ResourceFields, taskKnownStatus status.TaskStatus, taskDesiredStatus status.TaskStatus) { diff --git a/agent/utils/reference/reference.go b/agent/utils/reference/reference.go index 27dcfa530ce..29998828f20 100644 --- a/agent/utils/reference/reference.go +++ b/agent/utils/reference/reference.go @@ -15,6 +15,10 @@ package reference import ( + "fmt" + + "github.com/aws/amazon-ecs-agent/ecs-agent/logger" + "github.com/aws/amazon-ecs-agent/ecs-agent/logger/field" "github.com/docker/distribution/reference" "github.com/opencontainers/go-digest" ) @@ -33,3 +37,75 @@ func GetDigestFromImageRef(imageRef string) digest.Digest { return "" } } + +// Finds a repo digest matching the provided image reference from a list of repo digests +// and returns the repo digest's digest. +func GetDigestFromRepoDigests(repoDigests []string, imageRef string) (digest.Digest, error) { + // Parse image reference + ref, err := reference.Parse(imageRef) + if err != nil { + return "", fmt.Errorf("failed to parse image reference '%s': %w", imageRef, err) + } + namedRef, ok := ref.(reference.Named) + if !ok { + return "", fmt.Errorf( + "failed to parse image reference '%s' as a named reference, it was parsed as '%v'", + imageRef, ref) + } + + // Find a repo digest matching imageRef and return its digest + for _, repoDigest := range repoDigests { + repoDigestRef, err := reference.Parse(repoDigest) + if err != nil { + logger.Error("Error in parsing repo digest. Skipping it.", logger.Fields{ + "repoDigest": repoDigest, + field.Error: err, + }) + continue + } + repoDigestCanonicalRef, ok := repoDigestRef.(reference.Canonical) + if !ok { + logger.Warn("Parsed repo digest is not in canonical form. Skipping it.", logger.Fields{ + "repoDigest": repoDigest, + "parsedRepoDigest": repoDigestRef.String(), + }) + continue + } + if repoDigestCanonicalRef.Name() == namedRef.Name() { + return repoDigestCanonicalRef.Digest(), nil + } + } + + 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 existing 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 23515a4a708..7910d575bfe 100644 --- a/agent/utils/reference/reference_test.go +++ b/agent/utils/reference/reference_test.go @@ -54,3 +54,163 @@ func TestImageFromDigest(t *testing.T) { }) } } + +func TestGetDigestFromRepoDigests(t *testing.T) { + testDigest1 := "sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b" + testDigest2 := "sha256:c3839dd800b9eb7603340509769c43e146a74c63dca3045a8e7dc8ee07e53966" + tcs := []struct { + name string + repoDigests []string + imageRef string + expectedDigest string + expectedError string + }{ + { + name: "repo digest matching imageRef is used - dockerhub", + repoDigests: []string{ + "public.ecr.aws/library/alpine@" + testDigest1, + "alpine@" + testDigest2, + }, + imageRef: "alpine", + expectedDigest: testDigest2, + }, + { + name: "repo digest matching imageRef is used - ecr", + repoDigests: []string{ + "public.ecr.aws/library/alpine@" + testDigest1, + "alpine@" + testDigest2, + }, + imageRef: "public.ecr.aws/library/alpine", + expectedDigest: testDigest1, + }, + { + name: "tags in image ref are ignored - dockerhub", + repoDigests: []string{ + "public.ecr.aws/library/alpine@" + testDigest1, + "alpine@" + testDigest2, + }, + imageRef: "alpine:edge", + expectedDigest: testDigest2, + }, + { + name: "tags in image ref are ignored - ecr", + repoDigests: []string{ + "public.ecr.aws/library/alpine@" + testDigest1, + "alpine@" + testDigest2, + }, + imageRef: "public.ecr.aws/library/alpine:edge", + expectedDigest: testDigest1, + }, + { + name: "invalid image ref", + imageRef: "invalid format", + expectedError: "failed to parse image reference 'invalid format': invalid reference format", + }, + { + name: "image ref not named", + imageRef: "", + expectedError: "failed to parse image reference '': repository name must have at least one component", + }, + { + name: "invalid repo digests are skipped", + imageRef: "alpine", + repoDigests: []string{"invalid format", "alpine@" + testDigest1}, + expectedDigest: testDigest1, + }, + { + name: "no repo digests match image ref", + imageRef: "public.ecr.aws/library/alpine", + repoDigests: []string{"alpine@" + testDigest1}, + expectedError: "found no repo digest matching 'public.ecr.aws/library/alpine'", + }, + { + name: "image reference can be canonical", + repoDigests: []string{"alpine@" + testDigest1}, + imageRef: "alpine@" + testDigest2, + expectedDigest: testDigest1, + }, + { + name: "non-canonical repo digests are skipped", + repoDigests: []string{"alpine"}, + imageRef: "alpine", + expectedError: "found no repo digest matching 'alpine'", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + dgst, err := GetDigestFromRepoDigests(tc.repoDigests, tc.imageRef) + if tc.expectedError == "" { + require.NoError(t, err) + expected, err := digest.Parse(tc.expectedDigest) + require.NoError(t, err) + assert.Equal(t, expected, dgst) + } else { + require.EqualError(t, err, tc.expectedError) + } + }) + } +} + +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/api/container/status/containerstatus.go b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status/containerstatus.go index 53686f01832..f579370ba5b 100644 --- a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status/containerstatus.go +++ b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status/containerstatus.go @@ -102,20 +102,6 @@ func (cs *ContainerStatus) ShouldReportToBackend(steadyStateStatus ContainerStat return *cs == steadyStateStatus || *cs == ContainerStopped } -// BackendStatus maps the internal container status in the agent to that in the -// backend -func (cs *ContainerStatus) BackendStatus(steadyStateStatus ContainerStatus) ContainerStatus { - if *cs == steadyStateStatus { - return ContainerRunning - } - - if *cs == ContainerStopped { - return ContainerStopped - } - - return ContainerStatusNone -} - // BackendStatusString maps the internal container status in Agent to a backend recognized // status string. func (cs ContainerStatus) BackendStatusString() string { diff --git a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/logger/field/constants.go b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/logger/field/constants.go index da5da4184f4..9a93f4d8c25 100644 --- a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/logger/field/constants.go +++ b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/logger/field/constants.go @@ -48,6 +48,7 @@ const ( ImagePulledAt = "imagePulledAt" ImageLastUsedAt = "imageLastUsedAt" ImagePullSucceeded = "imagePullSucceeded" + ImageDigest = "imageDigest" ContainerName = "containerName" ContainerImage = "containerImage" ContainerExitCode = "containerExitCode" 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 00000000000..0198c3dcfcb --- /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/api/container/status/containerstatus.go b/ecs-agent/api/container/status/containerstatus.go index 53686f01832..f579370ba5b 100644 --- a/ecs-agent/api/container/status/containerstatus.go +++ b/ecs-agent/api/container/status/containerstatus.go @@ -102,20 +102,6 @@ func (cs *ContainerStatus) ShouldReportToBackend(steadyStateStatus ContainerStat return *cs == steadyStateStatus || *cs == ContainerStopped } -// BackendStatus maps the internal container status in the agent to that in the -// backend -func (cs *ContainerStatus) BackendStatus(steadyStateStatus ContainerStatus) ContainerStatus { - if *cs == steadyStateStatus { - return ContainerRunning - } - - if *cs == ContainerStopped { - return ContainerStopped - } - - return ContainerStatusNone -} - // BackendStatusString maps the internal container status in Agent to a backend recognized // status string. func (cs ContainerStatus) BackendStatusString() string { diff --git a/ecs-agent/api/container/status/containerstatus_test.go b/ecs-agent/api/container/status/containerstatus_test.go index 2fd78e09a25..0ab7a6a62af 100644 --- a/ecs-agent/api/container/status/containerstatus_test.go +++ b/ecs-agent/api/container/status/containerstatus_test.go @@ -64,46 +64,6 @@ func TestShouldReportToBackend(t *testing.T) { } -func TestBackendStatus(t *testing.T) { - // BackendStatus is ContainerStatusNone when container status is ContainerStatusNone - var containerStatus ContainerStatus - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerStatusNone) - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStatusNone) - - // BackendStatus is still ContainerStatusNone when container status is ContainerManifestPulled - containerStatus = ContainerManifestPulled - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerStatusNone) - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStatusNone) - - // BackendStatus is still ContainerStatusNone when container status is ContainerPulled - containerStatus = ContainerPulled - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerStatusNone) - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStatusNone) - - // BackendStatus is still ContainerStatusNone when container status is ContainerCreated - containerStatus = ContainerCreated - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerStatusNone) - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStatusNone) - - containerStatus = ContainerRunning - // BackendStatus is ContainerRunning when container status is ContainerRunning - // and steady state is ContainerRunning - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerRunning) - // BackendStatus is still ContainerStatusNone when container status is ContainerRunning - // and steady state is ContainerResourcesProvisioned - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStatusNone) - - containerStatus = ContainerResourcesProvisioned - // BackendStatus is still ContainerRunning when container status is ContainerResourcesProvisioned - // and steady state is ContainerResourcesProvisioned - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerRunning) - - // BackendStatus is ContainerStopped when container status is ContainerStopped - containerStatus = ContainerStopped - assert.Equal(t, containerStatus.BackendStatus(ContainerRunning), ContainerStopped) - assert.Equal(t, containerStatus.BackendStatus(ContainerResourcesProvisioned), ContainerStopped) -} - type testContainerStatus struct { SomeStatus ContainerStatus `json:"status"` } diff --git a/ecs-agent/logger/field/constants.go b/ecs-agent/logger/field/constants.go index da5da4184f4..9a93f4d8c25 100644 --- a/ecs-agent/logger/field/constants.go +++ b/ecs-agent/logger/field/constants.go @@ -48,6 +48,7 @@ const ( ImagePulledAt = "imagePulledAt" ImageLastUsedAt = "imageLastUsedAt" ImagePullSucceeded = "imagePullSucceeded" + ImageDigest = "imageDigest" ContainerName = "containerName" ContainerImage = "containerImage" ContainerExitCode = "containerExitCode" diff --git a/ecs-agent/utils/retry/constant_backoff.go b/ecs-agent/utils/retry/constant_backoff.go new file mode 100644 index 00000000000..0198c3dcfcb --- /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 00000000000..ccabcdaff00 --- /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()) + } +}