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.

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

Add functionality to pull image manifests to DockerClient #4140

Merged
merged 1 commit into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ clean:
-rm -f .generic-rpm-integrated-done
-rm -f amazon-ecs-volume-plugin
-rm -rf $(EBS_CSI_DRIVER_DIR)/bin
-rm -rm /tmp/private-test-registry-htpasswd # private registry credentials cleanup

clean-all: clean
# for our dockerfree builds, we likely don't have docker
Expand Down
33 changes: 33 additions & 0 deletions agent/dockerclient/dockerapi/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import (
"github.com/docker/docker/api/types/events"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/network"
"github.com/docker/docker/api/types/registry"
"github.com/docker/docker/api/types/volume"
)

Expand Down Expand Up @@ -121,6 +122,10 @@ type DockerClient interface {
// be processed by the listener.
ContainerEvents(context.Context) (<-chan DockerContainerChangeEvent, error)

// Given an image reference and registry auth credentials, pulls the image manifest
// of the image from the registry.
PullImageManifest(context.Context, string, *apicontainer.RegistryAuthenticationData) (registry.DistributionInspect, error)

// PullImage pulls an image. authData should contain authentication data provided by the ECS backend.
PullImage(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) DockerContainerMetadata

Expand Down Expand Up @@ -329,6 +334,34 @@ func (dg *dockerGoClient) time() ttime.Time {
return dg._time
}

// Pulls image manifest from the registry
func (dg *dockerGoClient) PullImageManifest(
ctx context.Context, imageRef string, authData *apicontainer.RegistryAuthenticationData,
) (registry.DistributionInspect, error) {
// Get auth creds
sdkAuthConfig, err := dg.getAuthdata(imageRef, authData)
if err != nil {
return registry.DistributionInspect{}, wrapPullErrorAsNamedError(imageRef, err)
Copy link
Contributor

@mye956 mye956 Apr 18, 2024

Choose a reason for hiding this comment

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

Q: Just curious, any reasons why we can't use nil if there were errors?

Copy link
Contributor Author

@amogh09 amogh09 Apr 18, 2024

Choose a reason for hiding this comment

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

Ah that's because this method has a value return type and not a pointer so the compiler won't allow returning nil. I think it's standard practice to return zero value (that's what var d registry.DistributionInspect would initialize to as well). I just followed the method signature of Docker SDK which is

DistributionInspect(ctx context.Context, imageRef, encodedRegistryAuth string) (registry.DistributionInspect, error)

It is just like how functions that have a (string, error) return type would return "", nil for error cases and not a nil. The user must check the returned error before using the returned value.

}
encodedAuth, err := registry.EncodeAuthConfig(sdkAuthConfig)
if err != nil {
return registry.DistributionInspect{}, wrapPullErrorAsNamedError(imageRef, err)
}

// Get an SDK Docker Client and call Distribution API
client, err := dg.sdkDockerClient()
if err != nil {
return registry.DistributionInspect{}, CannotGetDockerClientError{version: dg.version, err: err}
}
distInspect, err := client.DistributionInspect(ctx, imageRef, encodedAuth)
if err != nil {
err = redactEcrUrls(imageRef, err)
return registry.DistributionInspect{}, CannotPullContainerError{err}
}

return distInspect, nil
}

func (dg *dockerGoClient) PullImage(ctx context.Context, image string,
authData *apicontainer.RegistryAuthenticationData, timeout time.Duration) DockerContainerMetadata {
ctx, cancel := context.WithTimeout(ctx, timeout)
Expand Down
128 changes: 128 additions & 0 deletions agent/dockerclient/dockerapi/docker_client_integ_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//go:build integration
// +build integration

// 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 dockerapi

import (
"context"
"testing"

"github.com/aws/amazon-ecs-agent/agent/api/container"
apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/dockerclient"
"github.com/aws/amazon-ecs-agent/agent/dockerclient/sdkclientfactory"
"github.com/docker/docker/api/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const dockerEndpoint = "unix:///var/run/docker.sock"

// This integration test checks that dockerGoClient can pull image manifest from registries.
//
// The test is skipped on environments with a Docker Engine version that does not support
// API version 1.35 as older engine versions do not have Distribution API needed for pulling
// image manifest. Technically, API version >1.30 have Distribution API but engine versions
// between API version 1.30 and 1.35 can be configured to allow image pulls from v1 registries
// but Distribution API does not work with v1 registries. v1 registry support was dropped
// with engine version 17.12 that was shipped with API version 1.35.
//
// The test depends on local test registries that are set up by `make test-registry` command.
func TestImageManifestPullInteg(t *testing.T) {
// Prepare a docker client that can pull image manifests
sdkClientFactory := sdkclientfactory.NewFactory(context.Background(), dockerEndpoint)
cfg := &config.Config{}
defaultClient, err := NewDockerGoClient(sdkClientFactory, cfg, context.Background())
require.NoError(t, err)
version := dockerclient.GetSupportedDockerAPIVersion(dockerclient.Version_1_35)
supportedClient, err := defaultClient.WithVersion(version)
if err != nil {
t.Skipf("Skipping test due to unsupported Docker version: %v", err)
}

tcs := []struct {
name string
dockerClient DockerClient
imageRef string
authData *container.RegistryAuthenticationData
expectedError string
}{
{
name: "private registry success",
dockerClient: supportedClient,
imageRef: "127.0.0.1:51671/busybox:latest",
authData: func() *container.RegistryAuthenticationData {
asmAuthData := &apicontainer.ASMAuthData{}
asmAuthData.SetDockerAuthConfig(types.AuthConfig{
Username: "username",
Password: "password",
})
return &container.RegistryAuthenticationData{Type: apicontainer.AuthTypeASM,
ASMAuthData: asmAuthData,
}
}(),
},
{
name: "private registry auth failure",
dockerClient: supportedClient,
imageRef: "127.0.0.1:51671/busybox:latest",
expectedError: "no basic auth credentials",
},
{
name: "public registry success",
mye956 marked this conversation as resolved.
Show resolved Hide resolved
dockerClient: supportedClient,
imageRef: "127.0.0.1:51670/busybox:latest",
},
{
name: "public registry success, no explicit tag",
dockerClient: supportedClient,
imageRef: "127.0.0.1:51670/busybox",
},
{
name: "public ECR success",
dockerClient: supportedClient,
imageRef: "public.ecr.aws/amazonlinux/amazonlinux:2",
},
{
name: "Docker client version too old",
dockerClient: func() DockerClient {
sdkClientFactory := sdkclientfactory.NewFactory(context.Background(), dockerEndpoint)
cfg := &config.Config{}
defaultClient, err := NewDockerGoClient(sdkClientFactory, cfg, context.Background())
require.NoError(t, err)
version := dockerclient.GetSupportedDockerAPIVersion(dockerclient.Version_1_29)
unsupportedClient, err := defaultClient.WithVersion(version)
require.NoError(t, err)
return unsupportedClient
}(),
imageRef: "public.ecr.aws/amazonlinux/amazonlinux:2",
expectedError: `"distribution inspect" requires API version 1.30`,
},
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
distInspect, err := tc.dockerClient.PullImageManifest(
context.Background(), tc.imageRef, tc.authData)
if tc.expectedError == "" {
require.NoError(t, err)
assert.NotEmpty(t, distInspect.Descriptor.Digest.Encoded())
} else {
assert.ErrorContains(t, err, tc.expectedError)
}
})
}
}
147 changes: 140 additions & 7 deletions agent/dockerclient/dockerapi/docker_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,16 @@ import (
"github.com/aws/amazon-ecs-agent/ecs-agent/ec2"
"github.com/aws/amazon-ecs-agent/ecs-agent/utils/retry"
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/docker/docker/api/types"
dockercontainer "github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/events"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/network"
"github.com/docker/docker/api/types/registry"
"github.com/docker/docker/api/types/volume"
"github.com/docker/go-connections/nat"
"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -295,6 +298,143 @@ func TestPullImageECRSuccess(t *testing.T) {
assert.NoError(t, metadata.Error, "Expected pull to succeed")
}

func TestPullImageManifest(t *testing.T) {
someErr := errors.New("some error")
testDigest, err := digest.Parse("sha256:98ea6e4f216f2fb4b69fff9b3a44842c38686ca685f3f55dc48c5d3fb1107be4")
require.NoError(t, err)
testDistributionInspect := registry.DistributionInspect{
Descriptor: ocispec.Descriptor{Digest: testDigest},
}
type testCase struct {
name string
ctx context.Context
imageRef string
authData *apicontainer.RegistryAuthenticationData
setSDKFactoryExpectations func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller)
setECRClientExpectations func(*mock_ecr.MockECRClient)
expectedError error
expectedDistributionInspect registry.DistributionInspect
}
tcs := []testCase{
{
name: "failure in getting ECR auth data",
ctx: context.Background(),
imageRef: "image",
authData: &apicontainer.RegistryAuthenticationData{
Type: apicontainer.AuthTypeECR,
ECRAuthData: &apicontainer.ECRAuthData{RegistryID: "registryId"},
},
setECRClientExpectations: func(me *mock_ecr.MockECRClient) {
me.EXPECT().GetAuthorizationToken("registryId").Return(nil, someErr)
},
expectedError: CannotPullECRContainerError{someErr},
},
{
name: "Failure in getting SDK client",
ctx: context.Background(),
imageRef: "image",
setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) {
f.EXPECT().GetDefaultClient().Return(nil, someErr)
},
expectedError: CannotGetDockerClientError{version: "", err: someErr},
},
{
name: "Failure in DistributionInspect API - image URL is redacted",
ctx: context.Background(),
imageRef: "image",
setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) {
client := mock_sdkclient.NewMockClient(ctrl)
client.EXPECT().
DistributionInspect(
gomock.Any(), "image", base64.URLEncoding.EncodeToString([]byte("{}"))).
Return(
registry.DistributionInspect{},
errors.New("Some error for https://prod-us-east-1-starport-layer-bucket.s3.us-east-1.amazonaws.com"))
f.EXPECT().GetDefaultClient().Return(client, nil)
},
expectedError: CannotPullContainerError{errors.New("Some error for REDACTED ECR URL related to image")},
},
{
name: "Manifest is returned if there are no errors - no auth data",
ctx: context.Background(),
imageRef: "image",
setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) {
client := mock_sdkclient.NewMockClient(ctrl)
client.EXPECT().
DistributionInspect(
gomock.Any(), "image", base64.URLEncoding.EncodeToString([]byte("{}"))).
Return(testDistributionInspect, nil)
f.EXPECT().GetDefaultClient().Return(client, nil)
},
expectedDistributionInspect: testDistributionInspect,
},
func() testCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: Didn't know we could do this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes how cool is that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows tighter scoping of variables.

authData := &apicontainer.RegistryAuthenticationData{
Type: apicontainer.AuthTypeASM,
ASMAuthData: &apicontainer.ASMAuthData{},
}
authConfig := types.AuthConfig{Username: "username", Password: "password"}
authData.ASMAuthData.SetDockerAuthConfig(authConfig)
encodedAuthConfig, err := registry.EncodeAuthConfig(authConfig)
require.NoError(t, err)
return testCase{
name: "Manifest is returned if there are no errors - auth data",
ctx: context.Background(),
imageRef: "image",
authData: authData,
setSDKFactoryExpectations: func(f *mock_sdkclientfactory.MockFactory, ctrl *gomock.Controller) {
client := mock_sdkclient.NewMockClient(ctrl)
client.EXPECT().
DistributionInspect(gomock.Any(), "image", encodedAuthConfig).
Return(testDistributionInspect, nil)
f.EXPECT().GetDefaultClient().Return(client, nil)
},
expectedDistributionInspect: testDistributionInspect,
}
}(),
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

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)

client, err := NewDockerGoClient(sdkFactory, defaultTestConfig(), context.Background())
require.NoError(t, err)

if tc.setSDKFactoryExpectations != nil {
tc.setSDKFactoryExpectations(sdkFactory, ctrl)
}

goClient, ok := client.(*dockerGoClient)
require.True(t, ok)
ecrClientFactory := mock_ecr.NewMockECRFactory(ctrl)
ecrClient := mock_ecr.NewMockECRClient(ctrl)
mockTime := mock_ttime.NewMockTime(ctrl)
goClient.ecrClientFactory = ecrClientFactory
goClient._time = mockTime

if tc.setECRClientExpectations != nil {
ecrClientFactory.EXPECT().GetClient(tc.authData.ECRAuthData).Return(ecrClient, nil)
tc.setECRClientExpectations(ecrClient)
}

res, err := goClient.PullImageManifest(tc.ctx, tc.imageRef, tc.authData)
if tc.expectedError != nil {
assert.Equal(t, tc.expectedError, err)
} else {
require.Nil(t, err)
assert.Equal(t, tc.expectedDistributionInspect, res)
}
})
}
}

func TestPullImageECRAuthFail(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -1243,16 +1383,13 @@ func TestPingSdkFailError(t *testing.T) {
func TestUsesVersionedClient(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

// Docker SDK tests
mockDockerSDK := mock_sdkclient.NewMockClient(ctrl)
mockDockerSDK.EXPECT().Ping(gomock.Any()).Return(types.Ping{}, nil)
sdkFactory := mock_sdkclientfactory.NewMockFactory(ctrl)
sdkFactory.EXPECT().GetDefaultClient().AnyTimes().Return(mockDockerSDK, nil)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

client, err := NewDockerGoClient(sdkFactory, defaultTestConfig(), ctx)
assert.NoError(t, err)

Expand All @@ -1272,16 +1409,13 @@ func TestUsesVersionedClient(t *testing.T) {
func TestUnavailableVersionError(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

// Docker SDK tests
mockDockerSDK := mock_sdkclient.NewMockClient(ctrl)
mockDockerSDK.EXPECT().Ping(gomock.Any()).Return(types.Ping{}, nil)
sdkFactory := mock_sdkclientfactory.NewMockFactory(ctrl)
sdkFactory.EXPECT().GetDefaultClient().AnyTimes().Return(mockDockerSDK, nil)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

client, err := NewDockerGoClient(sdkFactory, defaultTestConfig(), ctx)
assert.NoError(t, err)

Expand All @@ -1294,7 +1428,6 @@ func TestUnavailableVersionError(t *testing.T) {

sdkFactory.EXPECT().GetClient(dockerclient.DockerVersion("1.21")).Times(1).Return(nil, errors.New("Cannot get client"))
metadata := vclient.StartContainer(ctx, "foo", defaultTestConfig().ContainerStartTimeout)

assert.NotNil(t, metadata.Error, "Expected error, didn't get one")
if namederr, ok := metadata.Error.(apierrors.NamedError); ok {
if namederr.ErrorName() != "CannotGetDockerclientError" {
Expand Down