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

ecs-init support for old docker engines (pre docker 17.x) and future docker engines (when api 1.25 is deprecated). #4080

Merged
merged 1 commit into from
Mar 15, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 94 additions & 7 deletions ecs-init/docker/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,13 @@ import (
)

const (
// dockerClientAPIVersion specifies the minimum docker client API version
// required by ECS Init
// Version 1.25 is required for setting Init to true when constructing
// the HostConfig for creating the ECS Agent to enable the Task
// Networking with ENI capability
dockerClientAPIVersion = "1.25"
// enableTaskENIDockerClientAPIVersion is the docker version at which we enable
// the ECS_ENABLE_TASK_ENI env var and set agent's "Init" field to true.
enableTaskENIDockerClientAPIVersion = "1.25"
)

var dockerAPIVersion string

type dockerclient interface {
ListImages(opts godocker.ListImagesOptions) ([]godocker.APIImages, error)
LoadImage(opts godocker.LoadImageOptions) error
Expand Down Expand Up @@ -65,13 +64,98 @@ func (client godockerClientFactory) NewVersionedClient(endpoint string, apiVersi
return godocker.NewVersionedClient(endpoint, apiVersionString)
}

// backupAPIVersions are API versions that have been deprecated on docker engine 25,
// they are only checked if none of the preferredAPIVersions are available.
// This list can be updated with time as docker engine deprecates more API versions.
func backupAPIVersions() []string {
return []string{
"1.21",
"1.22",
"1.23",
"1.24",
}
}
Comment on lines +70 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC before this change ecs-init was not compatible with environments with Docker engines that do not support API versions >= 1.25. But this PR makes ecs-init compatible with such environments. So, does that mean the older API versions were artificially made incompatible with ecs-init?

I understand that Task ENI is not supported on API versions < 1.25, but ecs-init doesn't actually make any Docker API calls that are not supported on API versions < 1.25?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ecs-init doesn't actually make any Docker API calls that are not supported on API versions < 1.25?

Not by default, but I will check on the various flags that ecs-init can add during startup to see if there are any that are not supported before docker 1.25.


// preferredAPIVersions returns the preferred list of docker client API versions.
// When establishing the docker client for ecs-init to use, we will try these API version
// numbers first.
func preferredAPIVersions() []string {
return []string{
"1.25",
"1.26",
"1.27",
"1.28",
"1.29",
"1.30",
"1.31",
"1.32",
"1.33",
"1.34",
"1.35",
"1.36",
"1.37",
"1.38",
"1.39",
"1.40",
"1.41",
"1.42",
"1.43",
"1.44",
}
Comment on lines +82 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a plan to maintain this list and "backup versions" list going forwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No plan in particular, this more just matches what we're doing in ecs-agent:

func GetKnownAPIVersions() []DockerVersion {

Copy link
Contributor

Choose a reason for hiding this comment

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

So deprecation of older API versions would be the forcing function for us to update this list?

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 I guess deprecation of older versions, or potentially if a new ECS feature comes along that is only supported in a newer version of the docker API (though I think that's fairly unlikely at this point).

}

// knownAPIVersions returns all known docker API versions, in order from
// oldest to newest.
func knownAPIVersions() []string {
return append(backupAPIVersions(), preferredAPIVersions()...)
}

// dockerVersionCompare returns 0 if lhs and rhs are equal.
// returns 1 if lhs > rhs
// returns -1 if lhs < rhs
func dockerVersionCompare(lhs, rhs string) int {
if lhs == rhs {
return 0
}
var lhsI int = -1
var rhsI int = -1
for i, v := range knownAPIVersions() {
if lhs == v {
lhsI = i
}
if rhs == v {
rhsI = i
}
}
if lhsI < rhsI {
return -1
}
return 1
}

func newDockerClient(dockerClientFactory dockerClientFactory, pingBackoff backoff.Backoff) (dockerclient, error) {
var dockerClient dockerclient
var err error
allAPIVersions := append(preferredAPIVersions(), backupAPIVersions()...)
for _, apiVersion := range allAPIVersions {
dockerClient, err = newVersionedDockerClient(apiVersion, dockerClientFactory, pingBackoff)
if err == nil {
log.Infof("Successfully created docker client with API version %s", apiVersion)
dockerAPIVersion = apiVersion
break
}
log.Infof("Could not create docker client with API version %s: %s", apiVersion, err)
}
return dockerClient, err
}

func newVersionedDockerClient(apiVersion string, dockerClientFactory dockerClientFactory, pingBackoff backoff.Backoff) (dockerclient, error) {
dockerUnixSocketSourcePath, fromEnv := config.DockerUnixSocket()
if !fromEnv {
dockerUnixSocketSourcePath = "/var/run/docker.sock"
}
client, err := dockerClientFactory.NewVersionedClient(
config.UnixSocketPrefix+dockerUnixSocketSourcePath, dockerClientAPIVersion)
config.UnixSocketPrefix+dockerUnixSocketSourcePath, apiVersion)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -157,6 +241,9 @@ func isNetworkError(err error) bool {
func isRetryablePingError(err error) bool {
godockerError, ok := err.(*godocker.Error)
if ok {
if godockerError.Status == http.StatusBadRequest {
Copy link
Contributor Author

@sparrc sparrc Jan 29, 2024

Choose a reason for hiding this comment

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

when provided with an unsupported docker api version, docker returns "Bad Request" (status code 400), so don't get into a retry loop if the API version is not supported (client errors in general probably shouldn't be retried at all, but just sticking with 400 for now to avoid changing behavior more than necessary).

return false
}
return godockerError.Status != http.StatusOK
}
return false
Expand Down
152 changes: 127 additions & 25 deletions ecs-init/docker/dependencies_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
//go:build test
amogh09 marked this conversation as resolved.
Show resolved Hide resolved
// +build test

// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
Expand Down Expand Up @@ -35,6 +32,7 @@ const immediately = time.Duration(-1)

var netError = &url.Error{Err: &net.OpError{Op: "read", Net: "unix", Err: io.EOF}}
var httpError = &docker.Error{Status: http.StatusInternalServerError, Message: "error"}
var badRequestError = &docker.Error{Status: http.StatusBadRequest, Message: "Bad Request Error"}

func TestIsNetworkErrorReturnsTrue(t *testing.T) {
assert.True(t, isNetworkError(netError), "Expect isNetworkError to return true if network error passed in")
Expand All @@ -48,6 +46,10 @@ func TestIsRetryablePingErrorReturnsTrue(t *testing.T) {
assert.True(t, isRetryablePingError(httpError), "Expect RetryablePingError to be true if httpError passed in")
}

func TestIsRetryableBadRequestErrorReturnsFalse(t *testing.T) {
assert.False(t, isRetryablePingError(badRequestError), "Expect RetryablePingError to be false if 'bad request' client error passed in")
}

func TestIsRetryablePingErrorReturnsFalse(t *testing.T) {
assert.False(t, isRetryablePingError(fmt.Errorf("error")), "Expect RetryablePingError to be true if non-http error passed in")
}
Expand Down Expand Up @@ -100,10 +102,33 @@ func TestNewDockerClientDoesnotRetryOnPingNonNetworkError(t *testing.T) {
mockClientFactory := NewMockdockerClientFactory(ctrl)
mockBackoff := NewMockBackoff(ctrl)

gomock.InOrder(
mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(mockDockerClient, nil),
mockDockerClient.EXPECT().Ping().Return(fmt.Errorf("error")),
)
calls := []*gomock.Call{}
for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
calls = append(calls, mockDockerClient.EXPECT().Ping().Return(fmt.Errorf("error")))
}
gomock.InOrder(calls...)

_, err := newDockerClient(mockClientFactory, mockBackoff)
assert.Error(t, err, "Expect error when creating docker client with no retry")
}

func TestNewDockerClientDoesNotRetryOnHTTPBadRequestError(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockDockerClient := NewMockdockerclient(ctrl)
mockClientFactory := NewMockdockerClientFactory(ctrl)
mockBackoff := NewMockBackoff(ctrl)

calls := []*gomock.Call{}
for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
calls = append(calls, mockDockerClient.EXPECT().Ping().Return(badRequestError))
}
gomock.InOrder(calls...)

_, err := newDockerClient(mockClientFactory, mockBackoff)
assert.Error(t, err, "Expect error when creating docker client with no retry")
Expand All @@ -117,14 +142,17 @@ func TestNewDockerClientGivesUpRetryingOnPingNetworkError(t *testing.T) {
mockClientFactory := NewMockdockerClientFactory(ctrl)
mockBackoff := NewMockBackoff(ctrl)

gomock.InOrder(
mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(mockDockerClient, nil),
mockDockerClient.EXPECT().Ping().Return(netError),
mockBackoff.EXPECT().ShouldRetry().Return(true),
mockBackoff.EXPECT().Duration().Return(immediately),
mockDockerClient.EXPECT().Ping().Return(netError),
mockBackoff.EXPECT().ShouldRetry().Return(false),
)
calls := []*gomock.Call{}
for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(mockDockerClient, nil))
calls = append(calls, mockDockerClient.EXPECT().Ping().Return(netError))
calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(true))
calls = append(calls, mockBackoff.EXPECT().Duration().Return(immediately))
calls = append(calls, mockDockerClient.EXPECT().Ping().Return(netError))
calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(false))
}
gomock.InOrder(calls...)

_, err := newDockerClient(mockClientFactory, mockBackoff)
assert.Error(t, err, "Expect error when creating docker client with no retry")
Expand All @@ -137,20 +165,20 @@ func TestNewDockerClientGivesUpRetryingOnUnavailableSocket(t *testing.T) {
mockClientFactory := NewMockdockerClientFactory(ctrl)
mockBackoff := NewMockBackoff(ctrl)

realDockerClient, dockerClientErr := godockerClientFactory{}.NewVersionedClient("unix:///a/bad/docker.sock", dockerClientAPIVersion)
realDockerClient, dockerClientErr := godockerClientFactory{}.NewVersionedClient("unix:///a/bad/docker.sock", "1.25")

require.False(t, dockerClientErr != nil, "there should be no errors trying to set up a docker client (intentionally bad with nonexistent socket path")

gomock.InOrder(
// We use the real client to ensure we're classifying errors
// correctly as returned by the upstream's error handling.
mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), gomock.Any()).Return(realDockerClient, dockerClientErr),
calls := []*gomock.Call{}
for _, apiVersion := range append(preferredAPIVersions(), backupAPIVersions()...) {
t.Logf("Expecting NewVersionedClient call with docker api version %s", apiVersion)
calls = append(calls, mockClientFactory.EXPECT().NewVersionedClient(gomock.Any(), apiVersion).Return(realDockerClient, nil))
// "bad connection", retries
mockBackoff.EXPECT().ShouldRetry().Return(true),
mockBackoff.EXPECT().Duration().Return(immediately),
// Give up
mockBackoff.EXPECT().ShouldRetry().Return(false),
)
calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(true))
calls = append(calls, mockBackoff.EXPECT().Duration().Return(immediately))
calls = append(calls, mockBackoff.EXPECT().ShouldRetry().Return(false))
}
gomock.InOrder(calls...)

_, err := newDockerClient(mockClientFactory, mockBackoff)
require.Error(t, err, "expect an error when creating docker client")
Expand All @@ -160,3 +188,77 @@ func TestNewDockerClientGivesUpRetryingOnUnavailableSocket(t *testing.T) {
_, isExpectedError := err.(*url.Error)
assert.True(t, isExpectedError, "expect net.OpError wrapped by url.Error")
}

func TestDockerVersionCompare(t *testing.T) {
testCases := []struct {
lhs string
rhs string
expectedResult int
}{
{
lhs: "1.21",
rhs: "1.24",
expectedResult: -1,
},
{
lhs: "1.21",
rhs: "1.25",
expectedResult: -1,
},
{
lhs: "1.21",
rhs: "1.44",
expectedResult: -1,
},
{
lhs: "1.21",
rhs: "1.21",
expectedResult: 0,
},
{
lhs: "1.25",
rhs: "1.25",
expectedResult: 0,
},
{
lhs: "1.35",
rhs: "1.35",
expectedResult: 0,
},
{
lhs: "1.44",
rhs: "1.44",
expectedResult: 0,
},
{
lhs: "1.24",
rhs: "1.22",
expectedResult: 1,
},
{
lhs: "1.25",
rhs: "1.22",
expectedResult: 1,
},
{
lhs: "1.34",
rhs: "1.25",
expectedResult: 1,
},
{
lhs: "1.44",
rhs: "1.21",
expectedResult: 1,
},
{
lhs: "1.44",
rhs: "1.26",
expectedResult: 1,
},
}
for _, tc := range testCases {
actualResult := dockerVersionCompare(tc.lhs, tc.rhs)
testName := fmt.Sprintf("lhs=%s rhs=%s expectedResult=%d", tc.lhs, tc.rhs, tc.expectedResult)
assert.Equal(t, tc.expectedResult, actualResult, testName)
}
}
6 changes: 5 additions & 1 deletion ecs-init/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const (
minBackoffDuration = time.Second
// maxBackoffDuration specifies the maximum backoff duration for ping to
// return a success response from docker socket
maxBackoffDuration = 5 * time.Second
maxBackoffDuration = 3 * time.Second
// backoffJitterMultiple specifies the backoff jitter multiplier
// coefficient when pinging the docker socket
backoffJitterMultiple = 0.2
Expand Down Expand Up @@ -327,6 +327,10 @@ func (c *client) getContainerConfig(envVarsFromFiles map[string]string) *godocke
// Task networking is not supported when not running on EC2. Explicitly disable since it's enabled by default.
envVariables["ECS_ENABLE_TASK_ENI"] = "false"
}
if dockerVersionCompare(dockerAPIVersion, enableTaskENIDockerClientAPIVersion) < 0 {
// Task networking is not supported before docker API version 1.25
envVariables["ECS_ENABLE_TASK_ENI"] = "false"
}

if !isPathValid(config.MountDirectoryEBS(), true) {
// EBS Task Attach (EBSTA) is not supported for external instances
Expand Down
7 changes: 6 additions & 1 deletion ecs-init/docker/docker_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ func createHostConfig(binds []string) *godocker.HostConfig {
NetworkMode: networkMode,
UsernsMode: usernsMode,
CapAdd: caps,
Init: true,
}

// Task ENI feature requires agent to be "init" process, so set it if docker API
// version is new enough for this feature.
if dockerVersionCompare(dockerAPIVersion, enableTaskENIDockerClientAPIVersion) >= 0 {
hostConfig.Init = true
}

if ctrdapparmor.HostSupports() {
Expand Down
Loading
Loading