From 2ca970b638a3342e84b7228555570033c1392273 Mon Sep 17 00:00:00 2001 From: richardpen Date: Tue, 4 Apr 2017 21:48:50 +0000 Subject: [PATCH] Fix docker auth configuration overrides incorrectly DockerAuth returned as an empty object(which is not nil) when it is not set from environment variable, which won't be overrides from contents read from file. --- agent/config/config_unix_test.go | 86 +++++++++++++++++++++++++ agent/config/types.go | 7 +- agent/config/types_test.go | 15 +++-- agent/engine/docker_container_engine.go | 7 +- 4 files changed, 108 insertions(+), 7 deletions(-) diff --git a/agent/config/config_unix_test.go b/agent/config/config_unix_test.go index 7f9bcb4f79e..2140e298d06 100644 --- a/agent/config/config_unix_test.go +++ b/agent/config/config_unix_test.go @@ -15,6 +15,8 @@ package config import ( + "fmt" + "io/ioutil" "os" "testing" "time" @@ -22,6 +24,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/ec2" "github.com/aws/amazon-ecs-agent/agent/engine/dockerclient" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestConfigDefault(t *testing.T) { @@ -62,3 +65,86 @@ func TestConfigDefault(t *testing.T) { assert.Equal(t, DefaultImageCleanupTimeInterval, cfg.ImageCleanupInterval, "ImageCleanupInterval default is set incorrectly") assert.Equal(t, DefaultNumImagesToDeletePerCycle, cfg.NumImagesToDeletePerCycle, "NumImagesToDeletePerCycle default is set incorrectly") } + +// TestConfigFromFile tests the configuration can be read from file +func TestConfigFromFile(t *testing.T) { + cluster := "TestCluster" + dockerAuthType := "dockercfg" + dockerAuth := `{ + "https://index.docker.io/v1/":{ + "auth":"admin", + "email":"email" + } +}` + configContent := fmt.Sprintf(`{ + "Cluster": "%s", + "EngineAuthType": "%s", + "EngineAuthData": %s, + "DataDir": "/var/run/ecs_agent", + "TaskIAMRoleEnabled": true, + "InstanceAttributes": { + "attribute1": "value1" + } +}`, cluster, dockerAuthType, dockerAuth) + + configFile := setupDockerAuthConfiguration(t, configContent) + defer os.Remove(configFile) + + os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", configFile) + defer os.Unsetenv("ECS_AGENT_CONFIG_FILE_PATH") + + config, err := fileConfig() + assert.NoError(t, err, "reading configuration from file failed") + + assert.Equal(t, cluster, config.Cluster, "cluster name not as expected from file") + 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") +} + +// TestDockerAuthMergeFromFile tests docker auth read from file correctly after merge +func TestDockerAuthMergeFromFile(t *testing.T) { + cluster := "myCluster" + dockerAuthType := "dockercfg" + dockerAuth := `{ + "https://index.docker.io/v1/":{ + "auth":"admin", + "email":"email" + } +}` + configContent := fmt.Sprintf(`{ + "Cluster": "TestCluster", + "EngineAuthType": "%s", + "EngineAuthData": %s, + "DataDir": "/var/run/ecs_agent", + "TaskIAMRoleEnabled": true, + "InstanceAttributes": { + "attribute1": "value1" + } +}`, dockerAuthType, dockerAuth) + + configFile := setupDockerAuthConfiguration(t, configContent) + defer os.Remove(configFile) + + os.Setenv("ECS_CLUSTER", cluster) + os.Setenv("ECS_AGENT_CONFIG_FILE_PATH", configFile) + defer os.Unsetenv("ECS_CLUSTER") + defer os.Unsetenv("ECS_AGENT_CONFIG_FILE_PATH") + + config, 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") +} + +// setupDockerAuthConfiguration create a temp file store the configuration +func setupDockerAuthConfiguration(t *testing.T, configContent string) string { + configFile, err := ioutil.TempFile("", "ecs-test") + require.NoError(t, err, "creating temp file for configuration failed") + + _, err = configFile.Write([]byte(configContent)) + require.NoError(t, err, "writing configuration to file failed") + + return configFile.Name() +} diff --git a/agent/config/types.go b/agent/config/types.go index 146e048ac77..efa4fd85937 100644 --- a/agent/config/types.go +++ b/agent/config/types.go @@ -154,9 +154,12 @@ type SensitiveRawMessage struct { contents json.RawMessage } -// NewSensitiveRawMessage returns a new encapsulated json.RawMessage that -// cannot be accidentally logged via .String/.GoString/%v/%#v +// NewSensitiveRawMessage returns a new encapsulated json.RawMessage or nil if +// the data is empty. It cannot be accidentally logged via .String/.GoString/%v/%#v func NewSensitiveRawMessage(data json.RawMessage) *SensitiveRawMessage { + if len(data) == 0 { + return nil + } return &SensitiveRawMessage{contents: data} } diff --git a/agent/config/types_test.go b/agent/config/types_test.go index d68bb1889f3..80d5f13e8a4 100644 --- a/agent/config/types_test.go +++ b/agent/config/types_test.go @@ -17,6 +17,8 @@ import ( "encoding/json" "fmt" "testing" + + "github.com/stretchr/testify/assert" ) func TestSensitiveRawMessageImplements(t *testing.T) { @@ -29,15 +31,20 @@ func TestSensitiveRawMessageImplements(t *testing.T) { func TestSensitiveRawMessage(t *testing.T) { sensitive := NewSensitiveRawMessage(json.RawMessage("secret")) - for i, str := range []string{ + for _, str := range []string{ sensitive.String(), sensitive.GoString(), fmt.Sprintf("%v", sensitive), fmt.Sprintf("%#v", sensitive), fmt.Sprint(sensitive), } { - if str != "[redacted]" { - t.Errorf("#%v: expected redacted, got %s", i, str) - } + assert.Equal(t, "[redacted]", str, "expected redacted") } } + +// TestEmptySensitiveRawMessage tests the message content is empty +func TestEmptySensitiveRawMessage(t *testing.T) { + sensitive := NewSensitiveRawMessage(json.RawMessage("")) + + assert.Nil(t, sensitive, "empty message should return nil") +} diff --git a/agent/engine/docker_container_engine.go b/agent/engine/docker_container_engine.go index 58acf2d1a52..c3b32896794 100644 --- a/agent/engine/docker_container_engine.go +++ b/agent/engine/docker_container_engine.go @@ -16,6 +16,7 @@ package engine import ( "archive/tar" "bufio" + "encoding/json" "io" "strings" "sync" @@ -146,9 +147,13 @@ func NewDockerGoClient(clientFactory dockerclient.Factory, cfg *config.Config) ( return nil, err } + var dockerAuthData json.RawMessage + if cfg.EngineAuthData != nil { + dockerAuthData = cfg.EngineAuthData.Contents() + } return &dockerGoClient{ clientFactory: clientFactory, - auth: dockerauth.NewDockerAuthProvider(cfg.EngineAuthType, cfg.EngineAuthData.Contents()), + auth: dockerauth.NewDockerAuthProvider(cfg.EngineAuthType, dockerAuthData), ecrClientFactory: ecr.NewECRFactory(cfg.AcceptInsecureCert), config: cfg, }, nil