Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pull the image with the <image>@<digest> format #4256

Merged
merged 1 commit into from
Aug 12, 2024
Merged

pull the image with the <image>@<digest> format #4256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions agent/api/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,16 +1564,9 @@ func (c *Container) GetImageName() string {

// 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.
// Always returns false when container's image reference contains digest as no digest resolution is needed in that case.
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) == ""
return !c.IsInternal() && c.GetImageDigest() != "" && !referenceutil.DigestExists(c.Image)
prateekchaudhry marked this conversation as resolved.
Show resolved Hide resolved
}

// GetRestartAggregationDataForStats gets the restart aggregation data for stats of a container.
Expand Down
14 changes: 3 additions & 11 deletions agent/api/container/container_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,17 +1468,9 @@ func TestDigestResolved(t *testing.T) {
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) {
t.Run("never resolved for container if digest found in image reference", func(t *testing.T) {
image := "ubuntu@sha256:ed6d2c43c8fbcd3eaa44c9dab6d94cb346234476230dc1681227aa72d07181ee"
assert.False(t, (&Container{Image: image}).DigestResolutionRequired())
imageDigest := "sha256:ed6d2c43c8fbcd3eaa44c9dab6d94cb346234476230dc1681227aa72d07181ee"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing to the previous comment, can there be case where we have image but not imageDigest? What should be the expected behavior in that case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non blocking) nit: we may consider adding unit test case for the above

assert.False(t, (&Container{Image: image, ImageDigest: imageDigest}).DigestResolved())
})
}
226 changes: 115 additions & 111 deletions agent/engine/docker_task_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ 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"
)

Expand Down Expand Up @@ -1244,143 +1243,148 @@ func (engine *DockerTaskEngine) GetDaemonManagers() map[string]dm.DaemonManager
func (engine *DockerTaskEngine) pullContainerManifest(
task *apitask.Task, container *apicontainer.Container,
) dockerapi.DockerContainerMetadata {
if !container.DigestResolutionRequired() {
// Digest resolution not required
// (internal container or already has digest in image reference)
if container.IsInternal() {
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},
imageManifestDigest := referenceutil.GetDigestFromImageRef(container.Image)
// 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.
if imageManifestDigest == "" {
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{
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,
"repoDigests": imgInspect.RepoDigests,
})
return dockerapi.DockerContainerMetadata{
Error: dockerapi.CannotPullImageManifestError{
FromError: fmt.Errorf("failed to find a repo digest matching '%s'", 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{}
}
}

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,
// 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{}
}

// 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()
}
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}
}
imageManifestMediaType := distInspect.Descriptor.MediaType
logger.Info("Fetched image manifest MediaType for container from registry", logger.Fields{
field.TaskARN: task.Arn,
field.ContainerName: container.Name,
field.ImageMediaType: imageManifestMediaType,
field.Image: container.Image,
})
if imageManifestMediaType == mediaTypeManifestV1 || imageManifestMediaType == mediaTypeSignedManifestV1 {
logger.Info("skipping digest resolution for manifest v2 schema 1", logger.Fields{
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}
}
imageManifestMediaType := distInspect.Descriptor.MediaType
logger.Info("Fetched image manifest MediaType for container from registry", logger.Fields{
field.TaskARN: task.Arn,
field.ContainerName: container.Name,
field.ImageMediaType: imageManifestMediaType,
field.Image: container.Image,
})
return dockerapi.DockerContainerMetadata{}
if imageManifestMediaType == mediaTypeManifestV1 || imageManifestMediaType == mediaTypeSignedManifestV1 {
logger.Info("skipping digest resolution for manifest v2 schema 1", logger.Fields{
field.TaskARN: task.Arn,
field.ContainerName: container.Name,
field.ImageMediaType: imageManifestMediaType,
field.Image: container.Image,
})
return dockerapi.DockerContainerMetadata{}
}
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,
})
}
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{
Expand Down Expand Up @@ -1591,7 +1595,7 @@ func (engine *DockerTaskEngine) pullAndUpdateContainerReference(task *apitask.Ta
}
pullSucceeded := metadata.Error == nil

if pullSucceeded && imageRef != container.Image {
if pullSucceeded && imageRef != container.Image && !referenceutil.DigestExists(container.Image) {
amogh09 marked this conversation as resolved.
Show resolved Hide resolved
// 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)
Expand Down
5 changes: 3 additions & 2 deletions agent/engine/docker_task_engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4160,8 +4160,9 @@ func TestPullContainerManifest(t *testing.T) {
containerName: "my-sc-container",
},
{
name: "digest is not resolved if already available in image reference",
image: "public.ecr.aws/library/alpine@" + testDigest.String(),
name: "digest is copied if already available in image reference",
image: "public.ecr.aws/library/alpine@" + testDigest.String(),
expectedDigest: testDigest.String(),
},
{
name: "image pull not required - image inspect fails",
Expand Down
Loading
Loading