Added the changes to safely parse the SSM parameters for windows containers. (#3)

* First cut of the changes.

* Intermediate check-in before refactoring to hashing the paths.

* Refactored the code to use a concatenated hashed value for the CredSpec filename.

* Cleaning the cruft.

* Simplified the code a bit and incorporated the first round of PR review comments.

* Arranged the imports as per the language spec.

* Fixed a broken test.

* Fixed a few more broken tests.

* Refactored the code to only use the credspecContainer Map.

* Fixed more references and tests.

* Fixed more tests.

* Fixed more tests.

* Fixed more tests.

* Fixed more tests.

* Improved the documentation for the member variable.

* Incorporated some review comments.

* Removed the unnecessary method and cleaned up the code a bit.

* Some more clean-up of the code.

* Tidying the imports.

Author: saurabhc123

agent/api/task/task_windows.go

@@ -141,8 +141,9 @@ func (task *Task) requiresCredentialSpecResource() bool {
 // initializeCredentialSpecResource builds the resource dependency map for the credentialspec resource
 func (task *Task) initializeCredentialSpecResource(config *config.Config, credentialsManager credentials.Manager,
 	resourceFields *taskresource.ResourceFields) error {
-	credentialspecResource, err := credentialspec.NewCredentialSpecResource(task.Arn, config.AWSRegion, task.getAllCredentialSpecRequirements(),
-		task.ExecutionCredentialsID, credentialsManager, resourceFields.SSMClientCreator, resourceFields.S3ClientCreator)
+	credspecContainerMapping := task.getAllCredentialSpecRequirements()
+	credentialspecResource, err := credentialspec.NewCredentialSpecResource(task.Arn, config.AWSRegion, task.ExecutionCredentialsID,
+		credentialsManager, resourceFields.SSMClientCreator, resourceFields.S3ClientCreator, credspecContainerMapping)
 	if err != nil {
 		return err
 	}
@@ -162,17 +163,15 @@ func (task *Task) initializeCredentialSpecResource(config *config.Config, creden
 }
 
 // getAllCredentialSpecRequirements is used to build all the credential spec requirements for the task
-func (task *Task) getAllCredentialSpecRequirements() []string {
-	reqs := []string{}
-
+func (task *Task) getAllCredentialSpecRequirements() map[string]string {
+	reqsContainerMap := make(map[string]string)
 	for _, container := range task.Containers {
 		credentialSpec, err := container.GetCredentialSpec()
-		if err == nil && credentialSpec != "" && !utils.StrSliceContains(reqs, credentialSpec) {
-			reqs = append(reqs, credentialSpec)
+		if err == nil && credentialSpec != "" {
+			reqsContainerMap[credentialSpec] = container.Name
 		}
 	}
-
-	return reqs
+	return reqsContainerMap
 }
 
 // GetCredentialSpecResource retrieves credentialspec resource from resource map

agent/api/task/task_windows_test.go

@@ -19,6 +19,7 @@ package task
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"runtime"
 	"testing"
 
@@ -396,70 +397,70 @@ func TestRequiresCredentialSpecResource(t *testing.T) {
 
 func TestGetAllCredentialSpecRequirements(t *testing.T) {
 	hostConfig := "{\"SecurityOpt\": [\"credentialspec:file://gmsa_gmsa-acct.json\"]}"
-	container := &apicontainer.Container{}
+	container := &apicontainer.Container{Name: "webapp1"}
 	container.DockerConfig.HostConfig = &hostConfig
 
 	task := &Task{
 		Arn:        "test",
 		Containers: []*apicontainer.Container{container},
 	}
 
-	allCredSpecReq := task.getAllCredentialSpecRequirements()
+	credentialSpecContainerMap := task.getAllCredentialSpecRequirements()
 
-	credentialspec := "credentialspec:file://gmsa_gmsa-acct.json"
-	expectedCredSpecReq := []string{credentialspec}
+	credentialspecFileLocation := "credentialspec:file://gmsa_gmsa-acct.json"
+	expectedCredentialSpecContainerMap := map[string]string{credentialspecFileLocation: "webapp1"}
 
-	assert.EqualValues(t, expectedCredSpecReq, allCredSpecReq)
+	assert.True(t, reflect.DeepEqual(expectedCredentialSpecContainerMap, credentialSpecContainerMap))
 }
 
 func TestGetAllCredentialSpecRequirementsWithMultipleContainersUsingSameSpec(t *testing.T) {
 	hostConfig := "{\"SecurityOpt\": [\"credentialspec:file://gmsa_gmsa-acct.json\"]}"
-	c1 := &apicontainer.Container{}
+	c1 := &apicontainer.Container{Name: "webapp1"}
 	c1.DockerConfig.HostConfig = &hostConfig
 
-	c2 := &apicontainer.Container{}
+	c2 := &apicontainer.Container{Name: "webapp2"}
 	c2.DockerConfig.HostConfig = &hostConfig
 
 	task := &Task{
 		Arn:        "test",
 		Containers: []*apicontainer.Container{c1, c2},
 	}
 
-	allCredSpecReq := task.getAllCredentialSpecRequirements()
+	credentialSpecContainerMap := task.getAllCredentialSpecRequirements()
 
-	credentialspec := "credentialspec:file://gmsa_gmsa-acct.json"
-	expectedCredSpecReq := []string{credentialspec}
+	credentialspecFileLocation := "credentialspec:file://gmsa_gmsa-acct.json"
+	expectedCredentialSpecContainerMap := map[string]string{credentialspecFileLocation: "webapp2"}
 
-	assert.Equal(t, len(expectedCredSpecReq), len(allCredSpecReq))
-	assert.EqualValues(t, expectedCredSpecReq, allCredSpecReq)
+	assert.Equal(t, len(expectedCredentialSpecContainerMap), len(credentialSpecContainerMap))
+	assert.True(t, reflect.DeepEqual(expectedCredentialSpecContainerMap, credentialSpecContainerMap))
 }
 
 func TestGetAllCredentialSpecRequirementsWithMultipleContainers(t *testing.T) {
 	hostConfig1 := "{\"SecurityOpt\": [\"credentialspec:file://gmsa_gmsa-acct-1.json\"]}"
 	hostConfig2 := "{\"SecurityOpt\": [\"credentialspec:file://gmsa_gmsa-acct-2.json\"]}"
 
-	c1 := &apicontainer.Container{}
+	c1 := &apicontainer.Container{Name: "webapp1"}
 	c1.DockerConfig.HostConfig = &hostConfig1
 
-	c2 := &apicontainer.Container{}
+	c2 := &apicontainer.Container{Name: "webapp2"}
 	c2.DockerConfig.HostConfig = &hostConfig1
 
-	c3 := &apicontainer.Container{}
+	c3 := &apicontainer.Container{Name: "webapp3"}
 	c3.DockerConfig.HostConfig = &hostConfig2
 
 	task := &Task{
 		Arn:        "test",
 		Containers: []*apicontainer.Container{c1, c2, c3},
 	}
 
-	allCredSpecReq := task.getAllCredentialSpecRequirements()
+	credentialSpecContainerMap := task.getAllCredentialSpecRequirements()
 
 	credentialspec1 := "credentialspec:file://gmsa_gmsa-acct-1.json"
 	credentialspec2 := "credentialspec:file://gmsa_gmsa-acct-2.json"
 
-	expectedCredSpecReq := []string{credentialspec1, credentialspec2}
+	expectedCredentialSpecContainerMap := map[string]string{credentialspec1: "webapp2", credentialspec2: "webapp3"}
 
-	assert.EqualValues(t, expectedCredSpecReq, allCredSpecReq)
+	assert.True(t, reflect.DeepEqual(expectedCredentialSpecContainerMap, credentialSpecContainerMap))
 }
 
 func TestInitializeAndGetCredentialSpecResource(t *testing.T) {

agent/engine/docker_task_engine.go

@@ -25,11 +25,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/aws/aws-sdk-go/aws"
-
-	"github.com/aws/amazon-ecs-agent/agent/logger"
-	"github.com/aws/amazon-ecs-agent/agent/logger/field"
-
 	"github.com/aws/amazon-ecs-agent/agent/api"
 	apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
 	apicontainerstatus "github.com/aws/amazon-ecs-agent/agent/api/container/status"
@@ -47,6 +42,8 @@ import (
 	"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
 	"github.com/aws/amazon-ecs-agent/agent/engine/execcmd"
 	"github.com/aws/amazon-ecs-agent/agent/eventstream"
+	"github.com/aws/amazon-ecs-agent/agent/logger"
+	"github.com/aws/amazon-ecs-agent/agent/logger/field"
 	"github.com/aws/amazon-ecs-agent/agent/metrics"
 	"github.com/aws/amazon-ecs-agent/agent/statechange"
 	"github.com/aws/amazon-ecs-agent/agent/taskresource"
@@ -56,9 +53,9 @@ import (
 	"github.com/aws/amazon-ecs-agent/agent/utils/retry"
 	utilsync "github.com/aws/amazon-ecs-agent/agent/utils/sync"
 	"github.com/aws/amazon-ecs-agent/agent/utils/ttime"
-	dockercontainer "github.com/docker/docker/api/types/container"
-
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/docker/docker/api/types"
+	dockercontainer "github.com/docker/docker/api/types/container"
 	"github.com/pkg/errors"
 )
 

agent/engine/docker_task_engine_windows.go

@@ -19,11 +19,10 @@ package engine
 import (
 	"time"
 
-	"github.com/aws/amazon-ecs-agent/agent/logger"
-	"github.com/aws/amazon-ecs-agent/agent/logger/field"
-
 	apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
 	apitask "github.com/aws/amazon-ecs-agent/agent/api/task"
+	"github.com/aws/amazon-ecs-agent/agent/logger"
+	"github.com/aws/amazon-ecs-agent/agent/logger/field"
 	"github.com/pkg/errors"
 )
 

agent/engine/docker_task_engine_windows_test.go

@@ -38,7 +38,6 @@ import (
 	mock_ssm_factory "github.com/aws/amazon-ecs-agent/agent/ssm/factory/mocks"
 	"github.com/aws/amazon-ecs-agent/agent/taskresource"
 	"github.com/aws/amazon-ecs-agent/agent/taskresource/credentialspec"
-
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/docker/docker/api/types"
 	dockercontainer "github.com/docker/docker/api/types/container"
@@ -134,16 +133,14 @@ func TestCredentialSpecResourceTaskFile(t *testing.T) {
 	ssmClientCreator := mock_ssm_factory.NewMockSSMClientCreator(ctrl)
 	s3ClientCreator := mock_s3_factory.NewMockS3ClientCreator(ctrl)
 
-	credentialSpecReq := []string{credentialspecFile}
-
 	credentialSpecRes, cerr := credentialspec.NewCredentialSpecResource(
 		testTask.Arn,
 		defaultConfig.AWSRegion,
-		credentialSpecReq,
 		credentialsID,
 		credentialsManager,
 		ssmClientCreator,
-		s3ClientCreator)
+		s3ClientCreator,
+		nil)
 	assert.NoError(t, cerr)
 
 	credSpecdata := map[string]string{
@@ -215,16 +212,14 @@ func TestCredentialSpecResourceTaskFileErr(t *testing.T) {
 	ssmClientCreator := mock_ssm_factory.NewMockSSMClientCreator(ctrl)
 	s3ClientCreator := mock_s3_factory.NewMockS3ClientCreator(ctrl)
 
-	credentialSpecReq := []string{credentialspecFile}
-
 	credentialSpecRes, cerr := credentialspec.NewCredentialSpecResource(
 		testTask.Arn,
 		defaultConfig.AWSRegion,
-		credentialSpecReq,
 		credentialsID,
 		credentialsManager,
 		ssmClientCreator,
-		s3ClientCreator)
+		s3ClientCreator,
+		nil)
 	assert.NoError(t, cerr)
 
 	credSpecdata := map[string]string{

agent/engine/engine_windows_integ_test.go

@@ -29,12 +29,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/aws/amazon-ecs-agent/agent/ecs_client/model/ecs"
-
-	"github.com/cihub/seelog"
-
-	"github.com/docker/docker/api/types"
-
 	"github.com/aws/amazon-ecs-agent/agent/api"
 	apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
 	apicontainerstatus "github.com/aws/amazon-ecs-agent/agent/api/container/status"
@@ -47,6 +41,7 @@ import (
 	"github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi"
 	"github.com/aws/amazon-ecs-agent/agent/dockerclient/sdkclientfactory"
 	"github.com/aws/amazon-ecs-agent/agent/ec2"
+	"github.com/aws/amazon-ecs-agent/agent/ecs_client/model/ecs"
 	"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
 	"github.com/aws/amazon-ecs-agent/agent/engine/execcmd"
 	"github.com/aws/amazon-ecs-agent/agent/eventstream"
@@ -56,6 +51,8 @@ import (
 	taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
 	"github.com/aws/amazon-ecs-agent/agent/utils"
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/cihub/seelog"
+	"github.com/docker/docker/api/types"
 	sdkClient "github.com/docker/docker/client"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"