diff --git a/CHANGELOG.md b/CHANGELOG.md index 120f6322734..9a6ee9927b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## UNRELEASED * Feature - Support for provisioning Tasks with ENIs +* Enhancement - Retry failed container image pull operations. * Bug - Fixed a memory leak issue when submitting the task state change [#967](https://github.com/aws/amazon-ecs-agent/pull/967) ## 1.14.4 diff --git a/README.md b/README.md index 97e22fbf11d..c993500b4d1 100644 --- a/README.md +++ b/README.md @@ -175,9 +175,10 @@ configure them as something other than the defaults. | `ECS_IMAGE_MINIMUM_CLEANUP_AGE` | 30m | The minimum time interval between when an image is pulled and when it can be considered for automated image cleanup. | 1h | 1h | | `ECS_NUM_IMAGES_DELETE_PER_CYCLE` | 5 | The maximum number of images to delete in a single automated image cleanup cycle. If set to less than 1, the value is ignored. | 5 | 5 | | `ECS_INSTANCE_ATTRIBUTES` | `{"stack": "prod"}` | These attributes take effect only during initial registration. After the agent has joined an ECS cluster, use the PutAttributes API action to add additional attributes. For more information, see [Amazon ECS Container Agent Configuration](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-agent-config.html) in the Amazon ECS Developer Guide.| `{}` | `{}` | -| `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | `false` | -| `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | `/amazon-ecs-cni-plugins` | -| `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to [Instance Metdata](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html) for Tasks started with `awsvpc` network mode | `false` | `false`| +| `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | Not applicable | +| `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | Not applicable | +| `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to [Instance Metdata](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html) for Tasks started with `awsvpc` network mode | `false` | Not applicable | +| `ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES` | `["10.0.15.0/24"]` | In `awsvpc` network mode, traffic to these prefixes will be routed via the host bridge instead of the task ENI | `[]` | Not applicable | ### Persistence diff --git a/agent/acs/model/ecsacs/api.go b/agent/acs/model/ecsacs/api.go index da7f2aaa17d..3c6d1159ebe 100644 --- a/agent/acs/model/ecsacs/api.go +++ b/agent/acs/model/ecsacs/api.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 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 diff --git a/agent/api/errors.go b/agent/api/errors.go index 061fb85132a..18fc12e658d 100644 --- a/agent/api/errors.go +++ b/agent/api/errors.go @@ -46,7 +46,8 @@ type NamedError interface { ErrorName() string } -// NamedError is a wrapper type for 'error' which adds an optional name and provides a symetric marshal/unmarshal +// DefaultNamedError is a wrapper type for 'error' which adds an optional name and provides a symmetric +// marshal/unmarshal type DefaultNamedError struct { Err string `json:"error"` Name string `json:"name"` diff --git a/agent/config/config.go b/agent/config/config.go index 35a601f5cce..bb33dfe8442 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -29,6 +29,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/engine/dockerclient" "github.com/aws/amazon-ecs-agent/agent/utils" "github.com/cihub/seelog" + cnitypes "github.com/containernetworking/cni/pkg/types" ) const ( @@ -218,7 +219,8 @@ func fileConfig() (Config, error) { err = json.Unmarshal(data, &cfg) if err != nil { - seelog.Errorf("Error reading cfg json data, err %v", err) + seelog.Criticalf("Error reading cfg json data, err %v", err) + return cfg, err } // Handle any deprecated keys correctly here @@ -293,11 +295,12 @@ func environmentConfig() (Config, error) { } taskCleanupWaitDuration := parseEnvVariableDuration("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION") + availableLoggingDriversEnv := os.Getenv("ECS_AVAILABLE_LOGGING_DRIVERS") loggingDriverDecoder := json.NewDecoder(strings.NewReader(availableLoggingDriversEnv)) var availableLoggingDrivers []dockerclient.LoggingDriver err = loggingDriverDecoder.Decode(&availableLoggingDrivers) - // EOF means the string was blank as opposed to UnexepctedEof which means an + // EOF means the string was blank as opposed to UnexpectedEof which means an // invalid parse // Blank is not a warning; we have sane defaults if err != io.EOF && err != nil { @@ -327,20 +330,30 @@ func environmentConfig() (Config, error) { cniPluginsPath := os.Getenv("ECS_CNI_PLUGINS_PATH") awsVPCBlockInstanceMetadata := utils.ParseBool(os.Getenv("ECS_AWSVPC_BLOCK_IMDS"), false) - instanceAttributesEnv := os.Getenv("ECS_INSTANCE_ATTRIBUTES") - attributeDecoder := json.NewDecoder(strings.NewReader(instanceAttributesEnv)) var instanceAttributes map[string]string - - err = attributeDecoder.Decode(&instanceAttributes) - if err != io.EOF && err != nil { - err := fmt.Errorf("Invalid format for ECS_INSTANCE_ATTRIBUTES. Expected a json hash") - seelog.Warn(err) - errs = append(errs, err) + instanceAttributesEnv := os.Getenv("ECS_INSTANCE_ATTRIBUTES") + err = json.Unmarshal([]byte(instanceAttributesEnv), &instanceAttributes) + if instanceAttributesEnv != "" { + if err != nil { + wrappedErr := fmt.Errorf("Invalid format for ECS_INSTANCE_ATTRIBUTES. Expected a json hash: %v", err) + seelog.Error(wrappedErr) + errs = append(errs, wrappedErr) + } } for attributeKey, attributeValue := range instanceAttributes { seelog.Debugf("Setting instance attribute %v: %v", attributeKey, attributeValue) } + var additionalLocalRoutes []cnitypes.IPNet + additionalLocalRoutesEnv := os.Getenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES") + if additionalLocalRoutesEnv != "" { + err := json.Unmarshal([]byte(additionalLocalRoutesEnv), &additionalLocalRoutes) + if err != nil { + seelog.Errorf("Invalid format for ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES, expected a json array of CIDRs: %v", err) + errs = append(errs, err) + } + } + if len(errs) > 0 { err = utils.NewMultiError(errs...) } else { @@ -379,6 +392,7 @@ func environmentConfig() (Config, error) { InstanceAttributes: instanceAttributes, CNIPluginsPath: cniPluginsPath, AWSVPCBlockInstanceMetdata: awsVPCBlockInstanceMetadata, + AWSVPCAdditionalLocalRoutes: additionalLocalRoutes, }, err } diff --git a/agent/config/config_test.go b/agent/config/config_test.go index 2e920449fbc..dd18dbe825d 100644 --- a/agent/config/config_test.go +++ b/agent/config/config_test.go @@ -14,6 +14,7 @@ package config import ( + "encoding/json" "errors" "os" "reflect" @@ -80,37 +81,56 @@ func TestBrokenEC2MetadataEndpoint(t *testing.T) { } func TestEnvironmentConfig(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_CLUSTER", "myCluster") + defer os.Unsetenv("ECS_CLUSTER") os.Setenv("ECS_RESERVED_PORTS_UDP", "[42,99]") + defer os.Unsetenv("ECS_RESERVED_PORTS_UDP") os.Setenv("ECS_RESERVED_MEMORY", "20") + defer os.Unsetenv("ECS_RESERVED_MEMORY") os.Setenv("ECS_CONTAINER_STOP_TIMEOUT", "60s") + defer os.Unsetenv("ECS_CONTAINER_STOP_TIMEOUT") os.Setenv("ECS_AVAILABLE_LOGGING_DRIVERS", "[\""+string(dockerclient.SyslogDriver)+"\"]") + defer os.Unsetenv("ECS_AVAILABLE_LOGGING_DRIVERS") os.Setenv("ECS_SELINUX_CAPABLE", "true") + defer os.Unsetenv("ECS_SELINUX_CAPABLE") os.Setenv("ECS_APPARMOR_CAPABLE", "true") + defer os.Unsetenv("ECS_APPARMOR_CAPABLE") os.Setenv("ECS_DISABLE_PRIVILEGED", "true") + defer os.Unsetenv("ECS_DISABLE_PRIVILEGED") os.Setenv("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION", "90s") + defer os.Unsetenv("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION") os.Setenv("ECS_ENABLE_TASK_IAM_ROLE", "true") + defer os.Unsetenv("ECS_ENABLE_TASK_IAM_ROLE") os.Setenv("ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST", "true") + defer os.Unsetenv("ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST") os.Setenv("ECS_DISABLE_IMAGE_CLEANUP", "true") + defer os.Unsetenv("ECS_DISABLE_IMAGE_CLEANUP") os.Setenv("ECS_IMAGE_CLEANUP_INTERVAL", "2h") + defer os.Unsetenv("ECS_IMAGE_CLEANUP_INTERVAL") os.Setenv("ECS_IMAGE_MINIMUM_CLEANUP_AGE", "30m") + defer os.Unsetenv("ECS_IMAGE_MINIMUM_CLEANUP_AGE") os.Setenv("ECS_NUM_IMAGES_DELETE_PER_CYCLE", "2") + defer os.Unsetenv("ECS_NUM_IMAGES_DELETE_PER_CYCLE") os.Setenv("ECS_INSTANCE_ATTRIBUTES", "{\"my_attribute\": \"testing\"}") + defer os.Unsetenv("ECS_INSTANCE_ATTRIBUTES") os.Setenv("ECS_ENABLE_TASK_ENI", "true") + defer os.Unsetenv("ECS_ENABLE_TASK_ENI") + additionalLocalRoutesJSON := `["1.2.3.4/22","5.6.7.8/32"]` + os.Setenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES", additionalLocalRoutesJSON) + defer os.Unsetenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES") conf, err := environmentConfig() - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, "myCluster", conf.Cluster) assert.Equal(t, 2, len(conf.ReservedPortsUDP)) assert.Contains(t, conf.ReservedPortsUDP, uint16(42)) assert.Contains(t, conf.ReservedPortsUDP, uint16(99)) assert.Equal(t, uint16(20), conf.ReservedMemory) - expectedDuration, _ := time.ParseDuration("60s") assert.Equal(t, expectedDuration, conf.DockerStopTimeout) - assert.Equal(t, []dockerclient.LoggingDriver{dockerclient.SyslogDriver}, conf.AvailableLoggingDrivers) - assert.True(t, conf.PrivilegedDisabled) assert.True(t, conf.SELinuxCapable, "Wrong value for SELinuxCapable") assert.True(t, conf.AppArmorCapable, "Wrong value for AppArmorCapable") @@ -118,15 +138,19 @@ func TestEnvironmentConfig(t *testing.T) { assert.True(t, conf.TaskIAMRoleEnabledForNetworkHost, "Wrong value for TaskIAMRoleEnabledForNetworkHost") assert.True(t, conf.ImageCleanupDisabled, "Wrong value for ImageCleanupDisabled") assert.True(t, conf.TaskENIEnabled, "Wrong value for TaskNetwork") - - assert.Equal(t, (30 * time.Minute), conf.MinimumImageDeletionAge) - assert.Equal(t, (2 * time.Hour), conf.ImageCleanupInterval) + assert.Equal(t, 30*time.Minute, conf.MinimumImageDeletionAge) + assert.Equal(t, 2*time.Hour, conf.ImageCleanupInterval) assert.Equal(t, 2, conf.NumImagesToDeletePerCycle) assert.Equal(t, "testing", conf.InstanceAttributes["my_attribute"]) - assert.Equal(t, (90 * time.Second), conf.TaskCleanupWaitDuration) + assert.Equal(t, 90*time.Second, conf.TaskCleanupWaitDuration) + serializedAdditionalLocalRoutesJSON, err := json.Marshal(conf.AWSVPCAdditionalLocalRoutes) + assert.NoError(t, err, "should marshal additional local routes") + assert.Equal(t, additionalLocalRoutesJSON, string(serializedAdditionalLocalRoutesJSON)) } func TestTrimWhitespace(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_CLUSTER", "default \r") os.Setenv("ECS_ENGINE_AUTH_TYPE", "dockercfg\r") @@ -153,6 +177,8 @@ func TestTrimWhitespace(t *testing.T) { } func TestConfigBoolean(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_DISABLE_METRICS", "true") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -253,6 +279,8 @@ func TestValidFormatParseEnvVariableDuration(t *testing.T) { } func TestInvalidTaskCleanupTimeout(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION", "1s") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -267,6 +295,8 @@ func TestInvalidTaskCleanupTimeout(t *testing.T) { } func TestTaskCleanupTimeout(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION", "10m") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -281,6 +311,8 @@ func TestTaskCleanupTimeout(t *testing.T) { } func TestInvalidReservedMemory(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_RESERVED_MEMORY", "-1") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -295,6 +327,8 @@ func TestInvalidReservedMemory(t *testing.T) { } func TestReservedMemory(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_RESERVED_MEMORY", "1") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -309,6 +343,8 @@ func TestReservedMemory(t *testing.T) { } func TestTaskIAMRoleEnabled(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_ENABLE_TASK_IAM_ROLE", "true") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -321,6 +357,8 @@ func TestTaskIAMRoleEnabled(t *testing.T) { } func TestTaskIAMRoleForHostNetworkEnabled(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST", "true") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -333,6 +371,8 @@ func TestTaskIAMRoleForHostNetworkEnabled(t *testing.T) { } func TestCredentialsAuditLogFile(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") dummyLocation := "/foo/bar.log" os.Setenv("ECS_AUDIT_LOGFILE", dummyLocation) cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) @@ -346,6 +386,8 @@ func TestCredentialsAuditLogFile(t *testing.T) { } func TestCredentialsAuditLogDisabled(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_AUDIT_LOGFILE_DISABLED", "true") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -358,6 +400,8 @@ func TestCredentialsAuditLogDisabled(t *testing.T) { } func TestImageCleanupMinimumInterval(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_IMAGE_CLEANUP_INTERVAL", "1m") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -370,6 +414,8 @@ func TestImageCleanupMinimumInterval(t *testing.T) { } func TestImageCleanupMinimumNumImagesToDeletePerCycle(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_NUM_IMAGES_DELETE_PER_CYCLE", "-1") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) if err != nil { @@ -382,9 +428,18 @@ func TestImageCleanupMinimumNumImagesToDeletePerCycle(t *testing.T) { } func TestAWSVPCBlockInstanceMetadata(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Setenv("ECS_AWSVPC_BLOCK_IMDS", "true") defer os.Unsetenv("ECS_AWSVPC_BLOCK_IMDS") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) assert.NoError(t, err) assert.True(t, cfg.AWSVPCBlockInstanceMetdata) } + +func TestInvalidAWSVPCAdditionalLocalRoutes(t *testing.T) { + os.Setenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES", `["300.300.300.300/64"]`) + defer os.Unsetenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES") + _, err := environmentConfig() + assert.Error(t, err) +} diff --git a/agent/config/config_unix_test.go b/agent/config/config_unix_test.go index 8fce0ee4e25..c5b21b7da4a 100644 --- a/agent/config/config_unix_test.go +++ b/agent/config/config_unix_test.go @@ -23,11 +23,14 @@ import ( "github.com/aws/amazon-ecs-agent/agent/ec2" "github.com/aws/amazon-ecs-agent/agent/engine/dockerclient" + cnitypes "github.com/containernetworking/cni/pkg/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestConfigDefault(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Unsetenv("ECS_DISABLE_METRICS") os.Unsetenv("ECS_RESERVED_PORTS") os.Unsetenv("ECS_RESERVED_MEMORY") @@ -48,7 +51,7 @@ func TestConfigDefault(t *testing.T) { os.Unsetenv("ECS_AWSVPC_BLOCK_IMDS") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, "unix:///var/run/docker.sock", cfg.DockerEndpoint, "Default docker endpoint set incorrectly") assert.Equal(t, "/data/", cfg.DataDir, "Default datadir set incorrectly") @@ -85,6 +88,7 @@ func TestConfigFromFile(t *testing.T) { testPauseImageName := "pause-image-name" testPauseTag := "pause-image-tag" content := fmt.Sprintf(`{ + "AWSRegion": "not-real-1", "Cluster": "%s", "EngineAuthType": "%s", "EngineAuthData": %s, @@ -94,13 +98,14 @@ func TestConfigFromFile(t *testing.T) { "attribute1": "value1" }, "PauseContainerImageName":"%s", - "PauseContainerTag":"%s" + "PauseContainerTag":"%s", + "AWSVPCAdditionalLocalRoutes":["169.254.172.1/32"] }`, cluster, dockerAuthType, dockerAuth, testPauseImageName, testPauseTag) - configFile := setupDockerAuthConfiguration(t, content) - defer os.Remove(configFile) + filePath := setupFileConfiguration(t, content) + defer os.Remove(filePath) - os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", configFile) + os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", filePath) defer os.Unsetenv("ECS_AGENT_CONFIG_FILE_PATH") cfg, err := fileConfig() @@ -112,6 +117,11 @@ func TestConfigFromFile(t *testing.T) { assert.Equal(t, map[string]string{"attribute1": "value1"}, cfg.InstanceAttributes) assert.Equal(t, testPauseImageName, cfg.PauseContainerImageName, "should read PauseContainerImageName") assert.Equal(t, testPauseTag, cfg.PauseContainerTag, "should read PauseContainerTag") + assert.Equal(t, 1, len(cfg.AWSVPCAdditionalLocalRoutes), "should have one additional local route") + expectedLocalRoute, err := cnitypes.ParseCIDR("169.254.172.1/32") + assert.NoError(t, err) + assert.Equal(t, expectedLocalRoute.IP, cfg.AWSVPCAdditionalLocalRoutes[0].IP, "should match expected route IP") + assert.Equal(t, expectedLocalRoute.Mask, cfg.AWSVPCAdditionalLocalRoutes[0].Mask, "should match expected route Mask") } // TestDockerAuthMergeFromFile tests docker auth read from file correctly after merge @@ -124,7 +134,8 @@ func TestDockerAuthMergeFromFile(t *testing.T) { "email":"email" } }` - configContent := fmt.Sprintf(`{ + content := fmt.Sprintf(`{ + "AWSRegion": "not-real-1", "Cluster": "TestCluster", "EngineAuthType": "%s", "EngineAuthData": %s, @@ -135,21 +146,37 @@ func TestDockerAuthMergeFromFile(t *testing.T) { } }`, dockerAuthType, dockerAuth) - configFile := setupDockerAuthConfiguration(t, configContent) - defer os.Remove(configFile) + filePath := setupFileConfiguration(t, content) + defer os.Remove(filePath) os.Setenv("ECS_CLUSTER", cluster) - os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", configFile) + os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", filePath) defer os.Unsetenv("ECS_CLUSTER") defer os.Unsetenv("ECS_AGENT_CONFIG_FILE_PATH") - config, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) assert.NoError(t, err, "create configuration failed") - assert.Equal(t, cluster, config.Cluster, "cluster name not as expected from environment variable") - assert.Equal(t, dockerAuthType, config.EngineAuthType, "docker auth type not as expected from file") - assert.Equal(t, dockerAuth, string(config.EngineAuthData.Contents()), "docker auth data not as expected from file") - assert.Equal(t, map[string]string{"attribute1": "value1"}, config.InstanceAttributes) + assert.Equal(t, cluster, cfg.Cluster, "cluster name not as expected from environment variable") + assert.Equal(t, dockerAuthType, cfg.EngineAuthType, "docker auth type not as expected from file") + assert.Equal(t, dockerAuth, string(cfg.EngineAuthData.Contents()), "docker auth data not as expected from file") + assert.Equal(t, map[string]string{"attribute1": "value1"}, cfg.InstanceAttributes) +} + +func TestBadFileContent(t *testing.T) { + content := `{ + "AWSRegion": "not-real-1", + "AWSVPCAdditionalLocalRoutes":["169.254.172.1/32", "300.300.300.300/32", "foo"] + }` + + filePath := setupFileConfiguration(t, content) + defer os.Remove(filePath) + + os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", filePath) + defer os.Unsetenv("ECS_AGENT_CONFIG_FILE_PATH") + + _, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + assert.Error(t, err, "create configuration should fail") } func TestShouldLoadPauseContainerTarball(t *testing.T) { @@ -162,13 +189,13 @@ func TestShouldLoadPauseContainerTarball(t *testing.T) { assert.False(t, cfg.ShouldLoadPauseContainerTarball(), "should not load tarball if image name differs") } -// setupDockerAuthConfiguration create a temp file store the configuration -func setupDockerAuthConfiguration(t *testing.T, configContent string) string { - configFile, err := ioutil.TempFile("", "ecs-test") +// setupFileConfiguration create a temp file store the configuration +func setupFileConfiguration(t *testing.T, configContent string) string { + file, err := ioutil.TempFile("", "ecs-test") require.NoError(t, err, "creating temp file for configuration failed") - _, err = configFile.Write([]byte(configContent)) + _, err = file.Write([]byte(configContent)) require.NoError(t, err, "writing configuration to file failed") - return configFile.Name() + return file.Name() } diff --git a/agent/config/config_windows_test.go b/agent/config/config_windows_test.go index 3da083ce8ee..e984f47e2c3 100644 --- a/agent/config/config_windows_test.go +++ b/agent/config/config_windows_test.go @@ -25,6 +25,8 @@ import ( ) func TestConfigDefault(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Unsetenv("ECS_DISABLE_METRICS") os.Unsetenv("ECS_RESERVED_PORTS") os.Unsetenv("ECS_RESERVED_MEMORY") @@ -42,7 +44,7 @@ func TestConfigDefault(t *testing.T) { os.Unsetenv("ECS_IMAGE_CLEANUP_INTERVAL") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, "npipe:////./pipe/docker_engine", cfg.DockerEndpoint, "Default docker endpoint set incorrectly") assert.Equal(t, `C:\ProgramData\Amazon\ECS\data`, cfg.DataDir, "Default datadir set incorrectly") @@ -64,10 +66,12 @@ func TestConfigDefault(t *testing.T) { } func TestConfigIAMTaskRolesReserves80(t *testing.T) { + os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1") + defer os.Unsetenv("AWS_DEFAULT_REGION") os.Unsetenv("ECS_RESERVED_PORTS") os.Setenv("ECS_ENABLE_TASK_IAM_ROLE", "true") cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, []uint16{ DockerReservedPort, DockerReservedSSLPort, diff --git a/agent/config/types.go b/agent/config/types.go index 7f64cbc0b29..0c87a92f695 100644 --- a/agent/config/types.go +++ b/agent/config/types.go @@ -18,6 +18,7 @@ import ( "time" "github.com/aws/amazon-ecs-agent/agent/engine/dockerclient" + cnitypes "github.com/containernetworking/cni/pkg/types" ) type Config struct { @@ -167,6 +168,18 @@ type Config struct { // AWSVPCBlockInstanceMetdata specifies if InstanceMetadata endpoint should be blocked // for tasks that are launched with network mode "awsvpc" when ECS_AWSVPC_BLOCK_IMDS=true AWSVPCBlockInstanceMetdata bool + + // OverrideAWSVPCLocalIPv4Address overrides the local IPv4 address chosen + // for a task using the `awsvpc` networking mode. Using this configuration + // will limit you to running one `awsvpc` task at a time. IPv4 addresses + // must be specified in decimal-octet form and also specify the subnet + // size (e.g., "169.254.172.42/22"). + OverrideAWSVPCLocalIPv4Address *cnitypes.IPNet + + // AWSVPCAdditionalLocalRoutes allows the specification of routing table + // entries that will be added in the task's network namespace via the + // instance bridge interface rather than via the ENI. + AWSVPCAdditionalLocalRoutes []cnitypes.IPNet } // SensitiveRawMessage is a struct to store some data that should not be logged diff --git a/agent/ecr/mocks/ecr_mocks.go b/agent/ecr/mocks/ecr_mocks.go index 25a0737ad20..a79a9c55788 100644 --- a/agent/ecr/mocks/ecr_mocks.go +++ b/agent/ecr/mocks/ecr_mocks.go @@ -17,8 +17,8 @@ package mock_ecr import ( - ecr "github.com/aws/amazon-ecs-agent/agent/ecr" - ecr0 "github.com/aws/amazon-ecs-agent/agent/ecr/model/ecr" + ecr0 "github.com/aws/amazon-ecs-agent/agent/ecr" + ecr "github.com/aws/amazon-ecs-agent/agent/ecr/model/ecr" gomock "github.com/golang/mock/gomock" ) @@ -43,9 +43,9 @@ func (_m *MockECRSDK) EXPECT() *_MockECRSDKRecorder { return _m.recorder } -func (_m *MockECRSDK) GetAuthorizationToken(_param0 *ecr0.GetAuthorizationTokenInput) (*ecr0.GetAuthorizationTokenOutput, error) { +func (_m *MockECRSDK) GetAuthorizationToken(_param0 *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) { ret := _m.ctrl.Call(_m, "GetAuthorizationToken", _param0) - ret0, _ := ret[0].(*ecr0.GetAuthorizationTokenOutput) + ret0, _ := ret[0].(*ecr.GetAuthorizationTokenOutput) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -75,9 +75,9 @@ func (_m *MockECRFactory) EXPECT() *_MockECRFactoryRecorder { return _m.recorder } -func (_m *MockECRFactory) GetClient(_param0 string, _param1 string) ecr.ECRClient { +func (_m *MockECRFactory) GetClient(_param0 string, _param1 string) ecr0.ECRClient { ret := _m.ctrl.Call(_m, "GetClient", _param0, _param1) - ret0, _ := ret[0].(ecr.ECRClient) + ret0, _ := ret[0].(ecr0.ECRClient) return ret0 } @@ -106,9 +106,9 @@ func (_m *MockECRClient) EXPECT() *_MockECRClientRecorder { return _m.recorder } -func (_m *MockECRClient) GetAuthorizationToken(_param0 string) (*ecr0.AuthorizationData, error) { +func (_m *MockECRClient) GetAuthorizationToken(_param0 string) (*ecr.AuthorizationData, error) { ret := _m.ctrl.Call(_m, "GetAuthorizationToken", _param0) - ret0, _ := ret[0].(*ecr0.AuthorizationData) + ret0, _ := ret[0].(*ecr.AuthorizationData) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -117,7 +117,7 @@ func (_mr *_MockECRClientRecorder) GetAuthorizationToken(arg0 interface{}) *gomo return _mr.mock.ctrl.RecordCall(_mr.mock, "GetAuthorizationToken", arg0) } -func (_m *MockECRClient) IsTokenValid(_param0 *ecr0.AuthorizationData) bool { +func (_m *MockECRClient) IsTokenValid(_param0 *ecr.AuthorizationData) bool { ret := _m.ctrl.Call(_m, "IsTokenValid", _param0) ret0, _ := ret[0].(bool) return ret0 diff --git a/agent/ecs_client/model/ecs/service.go b/agent/ecs_client/model/ecs/service.go index 5459c055cd1..53c626c906a 100644 --- a/agent/ecs_client/model/ecs/service.go +++ b/agent/ecs_client/model/ecs/service.go @@ -70,8 +70,8 @@ func New(p client.ConfigProvider, cfgs ...*aws.Config) *ECS { // newClient creates, initializes and returns a new service client instance. func newClient(cfg aws.Config, handlers request.Handlers, endpoint, signingRegion, signingName string) *ECS { - if signingName == "" { - signingName = ServiceName + if len(signingName) == 0 { + signingName = "ecs" } svc := &ECS{ Client: client.New( diff --git a/agent/ecscni/plugin.go b/agent/ecscni/plugin.go index 12816385bcd..fc118ff08d8 100644 --- a/agent/ecscni/plugin.go +++ b/agent/ecscni/plugin.go @@ -24,7 +24,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/logger" "github.com/cihub/seelog" "github.com/containernetworking/cni/libcni" - "github.com/containernetworking/cni/pkg/types" + cnitypes "github.com/containernetworking/cni/pkg/types" "github.com/pkg/errors" ) @@ -107,8 +107,17 @@ func (client *cniClient) CleanupNS(cfg *Config) error { // constructNetworkConfig creates configuration for eni, ipam and bridge plugin func (client *cniClient) constructNetworkConfig(cfg *Config) (*libcni.NetworkConfigList, error) { _, dst, err := net.ParseCIDR(TaskIAMRoleEndpoint) - if err != nil { - return nil, err + + routes := []*cnitypes.Route{ + { + Dst: *dst, + }, + } + + for _, route := range cfg.AdditionalLocalRoutes { + seelog.Debugf("Adding an additional route for %s", route) + ipNetRoute := (net.IPNet)(route) + routes = append(routes, &cnitypes.Route{Dst: ipNetRoute}) } ipamConf := IPAMConfig{ @@ -117,11 +126,7 @@ func (client *cniClient) constructNetworkConfig(cfg *Config) (*libcni.NetworkCon IPV4Subnet: client.subnet, IPV4Address: cfg.IPAMV4Address, ID: cfg.ID, - IPV4Routes: []*types.Route{ - { - Dst: *dst, - }, - }, + IPV4Routes: routes, } bridgeName := defaultBridgeName @@ -152,7 +157,7 @@ func (client *cniClient) constructNetworkConfig(cfg *Config) (*libcni.NetworkCon } plugins := []*libcni.NetworkConfig{ &libcni.NetworkConfig{ - Network: &types.NetConf{ + Network: &cnitypes.NetConf{ Type: ECSBridgePluginName, }, Bytes: bridgeConfBytes, @@ -165,7 +170,7 @@ func (client *cniClient) constructNetworkConfig(cfg *Config) (*libcni.NetworkCon return nil, err } plugins = append(plugins, &libcni.NetworkConfig{ - Network: &types.NetConf{ + Network: &cnitypes.NetConf{ Type: ECSENIPluginName, }, Bytes: eniConfBytes, diff --git a/agent/ecscni/plugin_test.go b/agent/ecscni/plugin_test.go index 184059bb989..91280b609ff 100644 --- a/agent/ecscni/plugin_test.go +++ b/agent/ecscni/plugin_test.go @@ -7,6 +7,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/ecscni/mocks_cnitypes" "github.com/aws/amazon-ecs-agent/agent/ecscni/mocks_libcni" + cnitypes "github.com/containernetworking/cni/pkg/types" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" ) @@ -26,7 +27,11 @@ func TestSetupNS(t *testing.T) { mockResult.EXPECT().String().Return(""), ) - err := ecscniClient.SetupNS(&Config{}) + additionalRoutesJson := `["169.254.172.1/32", "10.11.12.13/32"]` + var additionalRoutes []cnitypes.IPNet + err := json.Unmarshal([]byte(additionalRoutesJson), &additionalRoutes) + assert.NoError(t, err) + err = ecscniClient.SetupNS(&Config{AdditionalLocalRoutes: additionalRoutes}) assert.NoError(t, err) } @@ -40,7 +45,11 @@ func TestCleanupNS(t *testing.T) { libcniClient.EXPECT().DelNetworkList(gomock.Any(), gomock.Any()).Return(nil) - err := ecscniClient.CleanupNS(&Config{}) + additionalRoutesJson := `["169.254.172.1/32", "10.11.12.13/32"]` + var additionalRoutes []cnitypes.IPNet + err := json.Unmarshal([]byte(additionalRoutesJson), &additionalRoutes) + assert.NoError(t, err) + err = ecscniClient.CleanupNS(&Config{AdditionalLocalRoutes: additionalRoutes}) assert.NoError(t, err) } @@ -49,15 +58,21 @@ func TestCleanupNS(t *testing.T) { func TestConstructNetworkConfig(t *testing.T) { ecscniClient := NewClient(&Config{}) + additionalRoutesJson := `["169.254.172.1/32", "10.11.12.13/32"]` + var additionalRoutes []cnitypes.IPNet + err := json.Unmarshal([]byte(additionalRoutesJson), &additionalRoutes) + assert.NoError(t, err) + config := &Config{ - ENIID: "eni-12345678", - ContainerID: "containerid12", - ContainerPID: "pid", - ENIIPV4Address: "172.31.21.40", - ENIIPV6Address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334", - ENIMACAddress: "02:7b:64:49:b1:40", - BridgeName: "bridge-test1", - BlockInstanceMetdata: true, + ENIID: "eni-12345678", + ContainerID: "containerid12", + ContainerPID: "pid", + ENIIPV4Address: "172.31.21.40", + ENIIPV6Address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334", + ENIMACAddress: "02:7b:64:49:b1:40", + BridgeName: "bridge-test1", + BlockInstanceMetdata: true, + AdditionalLocalRoutes: additionalRoutes, } networkConfigList, err := ecscniClient.(*cniClient).constructNetworkConfig(config) @@ -67,12 +82,13 @@ func TestConstructNetworkConfig(t *testing.T) { eniConfig := &ENIConfig{} for _, plugin := range networkConfigList.Plugins { var err error - if plugin.Network.Type == ECSBridgePluginName { + switch plugin.Network.Type { + case ECSBridgePluginName: err = json.Unmarshal(plugin.Bytes, bridgeConfig) - } else if plugin.Network.Type == ECSENIPluginName { + case ECSENIPluginName: err = json.Unmarshal(plugin.Bytes, eniConfig) } - assert.NoError(t, err, "unmarshal config from bytes failed") + assert.NoError(t, err, "unmarshal config from bytes failed for plugin %s\n%s", plugin.Network.Type, string(plugin.Bytes)) } assert.Equal(t, config.BridgeName, bridgeConfig.BridgeName) diff --git a/agent/ecscni/types.go b/agent/ecscni/types.go index 854c1dd2617..b4e79c79352 100644 --- a/agent/ecscni/types.go +++ b/agent/ecscni/types.go @@ -14,7 +14,7 @@ package ecscni import ( - "github.com/containernetworking/cni/pkg/types" + cnitypes "github.com/containernetworking/cni/pkg/types" ) const ( @@ -59,11 +59,11 @@ type IPAMConfig struct { // IPV4Subnet is the ip address range managed by ipam IPV4Subnet string `json:"ipv4-subnet,omitempty"` // IPV4Address is the ip address to deal with(assign or release) in ipam - IPV4Address string `json:"ipv4-address,omitempty"` + IPV4Address *cnitypes.IPNet `json:"ipv4-address,omitempty"` // IPV4Gateway is the gateway returned by ipam, defalut the '.1' in the subnet IPV4Gateway string `json:"ipv4-gateway,omitempty"` // IPV4Routes is the route to added in the containerr namespace - IPV4Routes []*types.Route `json:"ipv4-routes,omitempty"` + IPV4Routes []*cnitypes.Route `json:"ipv4-routes,omitempty"` } // BridgeConfig contains all the information needed to invoke the bridge plugin @@ -134,10 +134,12 @@ type Config struct { // BridgeName is the name used to create the bridge BridgeName string // IPAMV4Address is the ipv4 used to assign from ipam - IPAMV4Address string + IPAMV4Address *cnitypes.IPNet // ID is the information associate with ip in ipam ID string // BlockInstanceMetdata specifies if InstanceMetadata endpoint should be // blocked BlockInstanceMetdata bool + // AdditionalLocalRoutes specifies additional routes to be added to the task namespace + AdditionalLocalRoutes []cnitypes.IPNet } diff --git a/agent/engine/docker_container_engine.go b/agent/engine/docker_container_engine.go index 88c63a6b072..0ac6a29a118 100644 --- a/agent/engine/docker_container_engine.go +++ b/agent/engine/docker_container_engine.go @@ -16,14 +16,13 @@ package engine import ( "archive/tar" "bufio" + "context" "encoding/json" "io" "strings" "sync" "time" - "golang.org/x/net/context" - "github.com/aws/amazon-ecs-agent/agent/api" "github.com/aws/amazon-ecs-agent/agent/config" "github.com/aws/amazon-ecs-agent/agent/ecr" @@ -31,9 +30,10 @@ import ( "github.com/aws/amazon-ecs-agent/agent/engine/dockerclient" "github.com/aws/amazon-ecs-agent/agent/engine/dockeriface" "github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume" + "github.com/aws/amazon-ecs-agent/agent/utils" "github.com/aws/amazon-ecs-agent/agent/utils/ttime" - "github.com/cihub/seelog" + "github.com/cihub/seelog" docker "github.com/fsouza/go-dockerclient" ) @@ -71,6 +71,13 @@ const ( // StatsInactivityTimeout controls the amount of time we hold open a // connection to the Docker daemon waiting for stats data StatsInactivityTimeout = 5 * time.Second + + // retry settings for pulling images + maximumPullRetries = 10 + minimumPullRetryDelay = 250 * time.Millisecond + maximumPullRetryDelay = 1 * time.Second + pullRetryDelayMultiplier = 1.5 + pullRetryJitterMultiplier = 0.2 ) // DockerClient interface to make testing it easier @@ -218,23 +225,48 @@ func (dg *dockerGoClient) time() ttime.Time { } func (dg *dockerGoClient) PullImage(image string, authData *api.RegistryAuthenticationData) DockerContainerMetadata { + // TODO Switch to just using context.WithDeadline and get rid of this funky code timeout := dg.time().After(pullImageTimeout) + ctx, cancel := context.WithCancel(context.TODO()) response := make(chan DockerContainerMetadata, 1) - go func() { response <- dg.pullImage(image, authData) }() + go func() { + imagePullBackoff := utils.NewSimpleBackoff(minimumPullRetryDelay, maximumPullRetryDelay, pullRetryJitterMultiplier, pullRetryDelayMultiplier) + err := utils.RetryNWithBackoffCtx(ctx, imagePullBackoff, maximumPullRetries, func() error { + err := dg.pullImage(image, authData) + if err != nil { + seelog.Warnf("Failed to pull image %s: %s", image, err.Error()) + } + return err + }) + response <- DockerContainerMetadata{Error: wrapPullErrorAsEngineError(err)} + }() select { case resp := <-response: return resp case <-timeout: + cancel() return DockerContainerMetadata{Error: &DockerTimeoutError{pullImageTimeout, "pulled"}} } } -func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenticationData) DockerContainerMetadata { +func wrapPullErrorAsEngineError(err error) engineError { + var retErr engineError + if err != nil { + engErr, ok := err.(engineError) + if !ok { + engErr = CannotPullContainerError{err} + } + retErr = engErr + } + return retErr +} + +func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenticationData) engineError { log.Debug("Pulling image", "image", image) client, err := dg.dockerClient() if err != nil { - return DockerContainerMetadata{Error: CannotGetDockerClientError{version: dg.version, err: err}} + return CannotGetDockerClientError{version: dg.version, err: err} } // Special case; this image is not one that should be pulled, but rather @@ -242,14 +274,14 @@ func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenti if image == emptyvolume.Image+":"+emptyvolume.Tag { scratchErr := dg.createScratchImageIfNotExists() if scratchErr != nil { - return DockerContainerMetadata{Error: &api.DefaultNamedError{Name: "CreateEmptyVolumeError", Err: "Could not create empty volume " + scratchErr.Error()}} + return CreateEmptyVolumeError{scratchErr} } - return DockerContainerMetadata{} + return nil } authConfig, err := dg.getAuthdata(image, authData) if err != nil { - return DockerContainerMetadata{Error: CannotPullContainerError{err}} + return wrapPullErrorAsEngineError(err) } pullDebugOut, pullWriter := io.Pipe() @@ -314,20 +346,20 @@ func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenti break case pullErr := <-pullFinished: if pullErr != nil { - return DockerContainerMetadata{Error: CannotPullContainerError{pullErr}} + return CannotPullContainerError{pullErr} } - return DockerContainerMetadata{} + return nil case <-timeout: - return DockerContainerMetadata{Error: &DockerTimeoutError{dockerPullBeginTimeout, "pullBegin"}} + return &DockerTimeoutError{dockerPullBeginTimeout, "pullBegin"} } log.Debug("Pull began for image", "image", image) defer log.Debug("Pull completed for image", "image", image) err = <-pullFinished if err != nil { - return DockerContainerMetadata{Error: CannotPullContainerError{err}} + return CannotPullContainerError{err} } - return DockerContainerMetadata{} + return nil } func (dg *dockerGoClient) createScratchImageIfNotExists() error { diff --git a/agent/engine/docker_container_engine_test.go b/agent/engine/docker_container_engine_test.go index 8baed777119..6d7f5722c40 100644 --- a/agent/engine/docker_container_engine_test.go +++ b/agent/engine/docker_container_engine_test.go @@ -40,6 +40,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/engine/dockeriface/mocks" "github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume" "github.com/aws/amazon-ecs-agent/agent/utils/ttime/mocks" + "github.com/stretchr/testify/require" ) // xContainerShortTimeout is a short duration intended to be used by the @@ -90,15 +91,16 @@ func TestPullImageOutputTimeout(t *testing.T) { defer done() pullBeginTimeout := make(chan time.Time) - testTime.EXPECT().After(dockerPullBeginTimeout).Return(pullBeginTimeout) - testTime.EXPECT().After(pullImageTimeout) + testTime.EXPECT().After(dockerPullBeginTimeout).Return(pullBeginTimeout).MinTimes(1) + testTime.EXPECT().After(pullImageTimeout).MinTimes(1) wait := sync.WaitGroup{} wait.Add(1) + // multiple invocations will happen due to retries, but all should timeout mockDocker.EXPECT().PullImage(&pullImageOptsMatcher{"image:latest"}, gomock.Any()).Do(func(x, y interface{}) { pullBeginTimeout <- time.Now() wait.Wait() // Don't return, verify timeout happens - }) + }).Times(maximumPullRetries) // expected number of retries metadata := client.PullImage("image", nil) if metadata.Error == nil { @@ -159,9 +161,7 @@ func TestPullImage(t *testing.T) { mockDocker.EXPECT().PullImage(&pullImageOptsMatcher{"image:latest"}, gomock.Any()).Return(nil) metadata := client.PullImage("image", nil) - if metadata.Error != nil { - t.Error("Expected pull to succeed") - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullImageTag(t *testing.T) { @@ -172,9 +172,7 @@ func TestPullImageTag(t *testing.T) { mockDocker.EXPECT().PullImage(&pullImageOptsMatcher{"image:mytag"}, gomock.Any()).Return(nil) metadata := client.PullImage("image:mytag", nil) - if metadata.Error != nil { - t.Error("Expected pull to succeed") - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullImageDigest(t *testing.T) { @@ -188,9 +186,7 @@ func TestPullImageDigest(t *testing.T) { ).Return(nil) metadata := client.PullImage("image@sha256:bc8813ea7b3603864987522f02a76101c17ad122e1c46d790efc0fca78ca7bfb", nil) - if metadata.Error != nil { - t.Error("Expected pull to succeed") - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullEmptyvolumeImage(t *testing.T) { @@ -203,19 +199,13 @@ func TestPullEmptyvolumeImage(t *testing.T) { mockDocker.EXPECT().InspectImage(emptyvolume.Image+":"+emptyvolume.Tag).Return(nil, errors.New("Does not exist")), mockDocker.EXPECT().ImportImage(gomock.Any()).Do(func(x interface{}) { req := x.(docker.ImportImageOptions) - if req.Repository != emptyvolume.Image { - t.Fatal("Expected empty volume repository") - } - if req.Tag != emptyvolume.Tag { - t.Fatal("Expected empty volume repository") - } + require.Equal(t, emptyvolume.Image, req.Repository, "expected empty volume repository") + require.Equal(t, emptyvolume.Tag, req.Tag, "expected empty volume tag") }), ) metadata := client.PullImage(emptyvolume.Image+":"+emptyvolume.Tag, nil) - if metadata.Error != nil { - t.Error(metadata.Error) - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullExistingEmptyvolumeImage(t *testing.T) { @@ -229,9 +219,7 @@ func TestPullExistingEmptyvolumeImage(t *testing.T) { ) metadata := client.PullImage(emptyvolume.Image+":"+emptyvolume.Tag, nil) - if metadata.Error != nil { - t.Error(metadata.Error) - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullImageECRSuccess(t *testing.T) { @@ -285,9 +273,7 @@ func TestPullImageECRSuccess(t *testing.T) { ).Return(nil) metadata := client.PullImage(image, authData) - if metadata.Error != nil { - t.Error("Expected pull to succeed") - } + assert.NoError(t, metadata.Error, "Expected pull to succeed") } func TestPullImageECRAuthFail(t *testing.T) { @@ -322,12 +308,11 @@ func TestPullImageECRAuthFail(t *testing.T) { image := imageEndpoint + "/myimage:tag" ecrClientFactory.EXPECT().GetClient(region, endpointOverride).Return(ecrClient) + // no retries for this error ecrClient.EXPECT().GetAuthorizationToken(gomock.Any()).Return(nil, errors.New("test error")) metadata := client.PullImage(image, authData) - if metadata.Error == nil { - t.Error("Expected pull to fail") - } + assert.Error(t, metadata.Error, "expected pull to fail") } func TestCreateContainerTimeout(t *testing.T) { @@ -344,12 +329,8 @@ func TestCreateContainerTimeout(t *testing.T) { // Don't return, verify timeout happens }) metadata := client.CreateContainer(config.Config, nil, config.Name, xContainerShortTimeout) - if metadata.Error == nil { - t.Error("Expected error for pull timeout") - } - if metadata.Error.(api.NamedError).ErrorName() != "DockerTimeoutError" { - t.Error("Wrong error type") - } + assert.Error(t, metadata.Error, "expected error for pull timeout") + assert.Equal(t, "DockerTimeoutError", metadata.Error.(api.NamedError).ErrorName()) wait.Done() } diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index 337c9f2c565..fda5b7316ad 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -695,6 +695,17 @@ func (engine *DockerTaskEngine) buildCNIConfigFromTaskContainer(task *api.Task, if err != nil { return nil, errors.Wrapf(err, "engine: build cni configuration from taskfailed") } + + if engine.cfg.OverrideAWSVPCLocalIPv4Address != nil && + len(engine.cfg.OverrideAWSVPCLocalIPv4Address.IP) != 0 && + len(engine.cfg.OverrideAWSVPCLocalIPv4Address.Mask) != 0 { + cfg.IPAMV4Address = engine.cfg.OverrideAWSVPCLocalIPv4Address + } + + if len(engine.cfg.AWSVPCAdditionalLocalRoutes) != 0 { + cfg.AdditionalLocalRoutes = engine.cfg.AWSVPCAdditionalLocalRoutes + } + // Get the pid of container containers, ok := engine.state.ContainerMapByArn(task.Arn) if !ok { diff --git a/agent/engine/engine_mocks.go b/agent/engine/engine_mocks.go index 87ba74a2825..615ecb1c49d 100644 --- a/agent/engine/engine_mocks.go +++ b/agent/engine/engine_mocks.go @@ -17,6 +17,7 @@ package engine import ( + context0 "context" io "io" time "time" @@ -180,7 +181,7 @@ func (_m *MockDockerClient) EXPECT() *_MockDockerClientRecorder { return _m.recorder } -func (_m *MockDockerClient) ContainerEvents(_param0 context.Context) (<-chan DockerContainerChangeEvent, error) { +func (_m *MockDockerClient) ContainerEvents(_param0 context0.Context) (<-chan DockerContainerChangeEvent, error) { ret := _m.ctrl.Call(_m, "ContainerEvents", _param0) ret0, _ := ret[0].(<-chan DockerContainerChangeEvent) ret1, _ := ret[1].(error) @@ -304,7 +305,7 @@ func (_mr *_MockDockerClientRecorder) StartContainer(arg0, arg1 interface{}) *go return _mr.mock.ctrl.RecordCall(_mr.mock, "StartContainer", arg0, arg1) } -func (_m *MockDockerClient) Stats(_param0 string, _param1 context.Context) (<-chan *go_dockerclient.Stats, error) { +func (_m *MockDockerClient) Stats(_param0 string, _param1 context0.Context) (<-chan *go_dockerclient.Stats, error) { ret := _m.ctrl.Call(_m, "Stats", _param0, _param1) ret0, _ := ret[0].(<-chan *go_dockerclient.Stats) ret1, _ := ret[1].(error) diff --git a/agent/engine/errors.go b/agent/engine/errors.go index 9a079bb888f..a9638d924ad 100644 --- a/agent/engine/errors.go +++ b/agent/engine/errors.go @@ -204,6 +204,28 @@ func (err CannotPullECRContainerError) ErrorName() string { return "CannotPullECRContainerError" } +// Retry fulfills the utils.Retrier interface and allows retries to be skipped by utils.Retry* functions +func (err CannotPullECRContainerError) Retry() bool { + return false +} + +type CreateEmptyVolumeError struct { + fromError error +} + +func (err CreateEmptyVolumeError) Error() string { + return err.fromError.Error() +} + +func (err CreateEmptyVolumeError) ErrorName() string { + return "CreateEmptyVolumeError" +} + +// Retry fulfills the utils.Retrier interface and allows retries to be skipped by utils.Retry* functions +func (err CreateEmptyVolumeError) Retry() bool { + return false +} + // CannotCreateContainerError indicates any error when trying to create a container type CannotCreateContainerError struct { fromError error diff --git a/agent/eni/netlinkwrapper/mocks/mock_netlinkwrapper_linux.go b/agent/eni/netlinkwrapper/mocks/mock_netlinkwrapper_linux.go index 7d771248ce2..ba7c578d551 100644 --- a/agent/eni/netlinkwrapper/mocks/mock_netlinkwrapper_linux.go +++ b/agent/eni/netlinkwrapper/mocks/mock_netlinkwrapper_linux.go @@ -18,7 +18,7 @@ package mock_netlinkwrapper import ( gomock "github.com/golang/mock/gomock" - "github.com/vishvananda/netlink" + netlink "github.com/vishvananda/netlink" ) // Mock of NetLink interface diff --git a/agent/eni/udevwrapper/mocks/mock_udevwrapper_linux.go b/agent/eni/udevwrapper/mocks/mock_udevwrapper_linux.go index 590c26998ae..d82440896cc 100644 --- a/agent/eni/udevwrapper/mocks/mock_udevwrapper_linux.go +++ b/agent/eni/udevwrapper/mocks/mock_udevwrapper_linux.go @@ -17,7 +17,7 @@ package mock_udevwrapper import ( - "github.com/deniswernert/udev" + udev "github.com/deniswernert/udev" gomock "github.com/golang/mock/gomock" ) diff --git a/agent/tcs/model/ecstcs/api.go b/agent/tcs/model/ecstcs/api.go index a9aa2186ae7..7e291b4a774 100644 --- a/agent/tcs/model/ecstcs/api.go +++ b/agent/tcs/model/ecstcs/api.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 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 diff --git a/agent/utils/utils.go b/agent/utils/utils.go index 2b8b33f8f00..f0143822dc7 100644 --- a/agent/utils/utils.go +++ b/agent/utils/utils.go @@ -14,6 +14,7 @@ package utils import ( + "context" "crypto/rand" "encoding/binary" "encoding/hex" @@ -109,11 +110,28 @@ var _time ttime.Time = &ttime.DefaultTime{} // If the error is Retriable then that will be used to determine if it should be // retried func RetryWithBackoff(backoff Backoff, fn func() error) error { + return RetryWithBackoffCtx(context.Background(), backoff, fn) +} + +// RetryWithBackoffCtx takes a context, a Backoff, and a function to call that returns an error +// If the context is done, nil will be returned +// If the error is nil then the function will no longer be called +// If the error is Retriable then that will be used to determine if it should be +// retried +func RetryWithBackoffCtx(ctx context.Context, backoff Backoff, fn func() error) error { var err error - for err = fn(); true; err = fn() { - retriable, isRetriable := err.(Retriable) + for { + select { + case <-ctx.Done(): + return nil + default: + } - if err == nil || isRetriable && !retriable.Retry() { + err = fn() + + retriableErr, isRetriableErr := err.(Retriable) + + if err == nil || (isRetriableErr && !retriableErr.Retry()) { return err } @@ -128,8 +146,17 @@ func RetryWithBackoff(backoff Backoff, fn func() error) error { // If the error returned is Retriable, the Retriability of it will be respected. // If the number of tries is exhausted, the last error will be returned. func RetryNWithBackoff(backoff Backoff, n int, fn func() error) error { + return RetryNWithBackoffCtx(context.Background(), backoff, n, fn) +} + +// RetryNWithBackoffCtx takes a context, a Backoff, a maximum number of tries 'n', and a function that returns an error. +// The function is called until it does not return an error, the context is done, or the maximum tries have been +// reached. +// If the error returned is Retriable, the Retriability of it will be respected. +// If the number of tries is exhausted, the last error will be returned. +func RetryNWithBackoffCtx(ctx context.Context, backoff Backoff, n int, fn func() error) error { var err error - RetryWithBackoff(backoff, func() error { + RetryWithBackoffCtx(ctx, backoff, func() error { err = fn() n-- if n == 0 { diff --git a/agent/utils/utils_test.go b/agent/utils/utils_test.go index 7e3affeef3d..dd3595605eb 100644 --- a/agent/utils/utils_test.go +++ b/agent/utils/utils_test.go @@ -14,6 +14,7 @@ package utils import ( + "context" "errors" "testing" "time" @@ -97,23 +98,70 @@ func TestRetryWithBackoff(t *testing.T) { _time = mocktime defer func() { _time = &ttime.DefaultTime{} }() - mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(3) - counter := 3 - RetryWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), func() error { - if counter == 0 { - return nil - } - counter-- - return errors.New("err") + t.Run("retries", func(t *testing.T) { + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(3) + counter := 3 + RetryWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), func() error { + if counter == 0 { + return nil + } + counter-- + return errors.New("err") + }) + assert.Equal(t, 0, counter, "Counter didn't go to 0; didn't get retried enough") }) - assert.Equal(t, 0, counter, "Counter didn't go to 0; didn't get retried enough") - // no sleeps - RetryWithBackoff(NewSimpleBackoff(10*time.Second, 20*time.Second, 0, 2), func() error { - return NewRetriableError(NewRetriable(false), errors.New("can't retry")) + t.Run("no retries", func(t *testing.T) { + // no sleeps + RetryWithBackoff(NewSimpleBackoff(10*time.Second, 20*time.Second, 0, 2), func() error { + return NewRetriableError(NewRetriable(false), errors.New("can't retry")) + }) }) } +func TestRetryWithBackoffCtx(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mocktime := mock_ttime.NewMockTime(ctrl) + _time = mocktime + defer func() { _time = &ttime.DefaultTime{} }() + + t.Run("retries", func(t *testing.T) { + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(3) + counter := 3 + RetryWithBackoffCtx(context.TODO(), NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), func() error { + if counter == 0 { + return nil + } + counter-- + return errors.New("err") + }) + assert.Equal(t, 0, counter, "Counter didn't go to 0; didn't get retried enough") + }) + + t.Run("no retries", func(t *testing.T) { + // no sleeps + RetryWithBackoffCtx(context.TODO(), NewSimpleBackoff(10*time.Second, 20*time.Second, 0, 2), func() error { + return NewRetriableError(NewRetriable(false), errors.New("can't retry")) + }) + }) + + t.Run("cancel context", func(t *testing.T) { + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(2) + counter := 2 + ctx, cancel := context.WithCancel(context.TODO()) + RetryWithBackoffCtx(ctx, NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), func() error { + counter-- + if counter == 0 { + cancel() + } + return errors.New("err") + }) + assert.Equal(t, 0, counter, "Counter not 0; went the wrong number of times") + }) + +} + func TestRetryNWithBackoff(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -121,29 +169,83 @@ func TestRetryNWithBackoff(t *testing.T) { _time = mocktime defer func() { _time = &ttime.DefaultTime{} }() - // 2 tries, 1 sleep - mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(1) - counter := 3 - err := RetryNWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 2, func() error { - counter-- - return errors.New("err") + t.Run("count exceeded", func(t *testing.T) { + // 2 tries, 1 sleep + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(1) + counter := 3 + err := RetryNWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 2, func() error { + counter-- + return errors.New("err") + }) + assert.Equal(t, 1, counter, "Should have stopped after two tries") + assert.Error(t, err) + }) + + t.Run("retry succeeded", func(t *testing.T) { + // 3 tries, 2 sleeps + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(2) + counter := 3 + err := RetryNWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 5, func() error { + counter-- + if counter == 0 { + return nil + } + return errors.New("err") + }) + assert.Equal(t, 0, counter) + assert.NoError(t, err) + }) +} + +func TestRetryNWithBackoffCtx(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mocktime := mock_ttime.NewMockTime(ctrl) + _time = mocktime + defer func() { _time = &ttime.DefaultTime{} }() + + t.Run("count exceeded", func(t *testing.T) { + // 2 tries, 1 sleep + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(1) + counter := 3 + err := RetryNWithBackoffCtx(context.TODO(), NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 2, func() error { + counter-- + return errors.New("err") + }) + assert.Equal(t, 1, counter, "Should have stopped after two tries") + assert.Error(t, err) }) - assert.Equal(t, 1, counter, "Should have stopped after two tries") - assert.Error(t, err) - - // 3 tries, 2 sleeps - mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(2) - counter = 3 - err = RetryNWithBackoff(NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 5, func() error { - counter-- - if counter == 0 { - return nil - } - return errors.New("err") + + t.Run("retry succeeded", func(t *testing.T) { + // 3 tries, 2 sleeps + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(2) + counter := 3 + err := RetryNWithBackoffCtx(context.TODO(), NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 5, func() error { + counter-- + if counter == 0 { + return nil + } + return errors.New("err") + }) + assert.Equal(t, 0, counter) + assert.NoError(t, err) }) - assert.Equal(t, 0, counter) - assert.Nil(t, err) + t.Run("cancel context", func(t *testing.T) { + // 2 tries, 2 sleeps + mocktime.EXPECT().Sleep(100 * time.Millisecond).Times(2) + counter := 3 + ctx, cancel := context.WithCancel(context.TODO()) + err := RetryNWithBackoffCtx(ctx, NewSimpleBackoff(100*time.Millisecond, 100*time.Millisecond, 0, 1), 5, func() error { + counter-- + if counter == 1 { + cancel() + } + return errors.New("err") + }) + assert.Equal(t, 1, counter, "Should have stopped after two tries") + assert.Error(t, err) + }) } func TestParseBool(t *testing.T) {