Repository: aws/amazon-ecs-cli-v2
Pull request #284 (2019-11-06 00:47:53)

Fix project-resources template for apps with "-" in the name
CloudFormation logical IDs can't have dashes in the name. This
change "sanitizes" the logical IDs we generate in the project
resources stack.

We'll replace dashes with the word DASH (since caps aren't
allowed in app/project names - we won't have any collisions with
any real apps, which may have dash in their name).

<!-- Provide summary of changes -->

<!-- Issue number, if available. E.g. "Fixes #31", "Addresses #42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.



Patch:
diff --git a/internal/pkg/deploy/cloudformation/stack/project.go b/internal/pkg/deploy/cloudformation/stack/project.go
index e7c8ad06..b2df8bf1 100644
--- a/internal/pkg/deploy/cloudformation/stack/project.go
+++ b/internal/pkg/deploy/cloudformation/stack/project.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"html/template"
 	"sort"
+	"strings"
 
 	"github.com/aws/amazon-ecs-cli-v2/internal/pkg/archer"
 	"github.com/aws/aws-sdk-go/aws"
@@ -79,7 +80,13 @@ func (e *ProjectStackConfig) ResourceTemplate(config *ProjectResourcesConfig) (s
 		return "", &ErrTemplateNotFound{templateLocation: projectResourcesTemplatePath, parentErr: err}
 	}
 
-	template, err := template.New("resourcetemplate").Parse(stackSetTemplate)
+	template, err := template.New("resourcetemplate").
+		Funcs(template.FuncMap{
+			"logicalIDSafe": func(logicalID string) string {
+				return strings.ReplaceAll(logicalID, "-", "DASH")
+			},
+		}).
+		Parse(stackSetTemplate)
 	if err != nil {
 		return "", err
 	}
diff --git a/templates/project/cf.yml b/templates/project/cf.yml
index 86a3e31e..fb91d4bf 100644
--- a/templates/project/cf.yml
+++ b/templates/project/cf.yml
@@ -1,6 +1,6 @@
 # Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 # SPDX-License-Identifier: Apache-2.0
-AWSTemplateFormatVersion: '2010-09-09' {{$accounts := .Accounts}} {{$project := .Project}} {{$apps := .Apps}}
+AWSTemplateFormatVersion: '2010-09-09'{{$accounts := .Accounts}}{{$project := .Project}}{{$apps := .Apps}}
 # Cross-regional resources deployed via a stackset in the tools account
 # to support the CodePipeline for a workspace
 Description: Cross-regional resources to support the CodePipeline for a workspace
@@ -82,7 +82,7 @@ Resources:
     DeletionPolicy: Retain
 
 {{range $app := $apps}}
-  ECRRepo{{$app}}:
+  ECRRepo{{logicalIDSafe $app}}:
     Type: AWS::ECR::Repository
     Properties:
       RepositoryName: {{$project}}/{{$app}}
@@ -112,7 +112,7 @@ Outputs:
   PipelineBucket:
     Description: Bucket used for CodePipeline to stage resources in.
     Value: !Ref PipelineBuiltArtifactBucket
-{{range $app := $apps}}  ECRRepo{{$app}}:
+{{range $app := $apps}}  ECRRepo{{logicalIDSafe $app}}:
     Description: ECR Repo used to store images of the {{$app}} app.
-    Value: !GetAtt ECRRepo{{$app}}.Arn
+    Value: !GetAtt ECRRepo{{logicalIDSafe $app}}.Arn
 {{end}}


Test patch:
diff --git a/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go b/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go
index 64ab835e..ff63831d 100644
--- a/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go
+++ b/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go
@@ -117,7 +117,7 @@ func Test_Project_Infrastructure(t *testing.T) {
 			// Clean up any StackInstances we may have created.
 			if stackInstances, err := cfClient.ListStackInstances(&awsCF.ListStackInstancesInput{
 				StackSetName: aws.String(projectStackSetName),
-			}); err == nil && stackInstances.Summaries[0] != nil {
+			}); err == nil && stackInstances.Summaries != nil && stackInstances.Summaries[0] != nil {
 				projectStackInstance := stackInstances.Summaries[0]
 				cfClient.DeleteStackInstances(&awsCF.DeleteStackInstancesInput{
 					Accounts:     []*string{projectStackInstance.Account},
@@ -160,6 +160,16 @@ func Test_Project_Infrastructure(t *testing.T) {
 
 		require.NoError(t, err)
 
+		// Add an application with dash
+		err = deployer.AddAppToProject(
+			&project,
+			&archer.Application{
+				Name: "myapp-frontend",
+			},
+		)
+
+		require.NoError(t, err)
+
 		// No new substacks should be created
 		stackInstances, err := cfClient.ListStackInstances(&awsCF.ListStackInstancesInput{
 			StackSetName: aws.String(projectStackSetName),
@@ -205,6 +215,12 @@ func Test_Project_Infrastructure(t *testing.T) {
 					strings.HasSuffix(*output.OutputValue, fmt.Sprintf("repository/%s/myapp", project.Name)),
 					fmt.Sprintf("ECRRepomyapp should be suffixed with repository/{project}/myapp but was %s", *output.OutputValue))
 			},
+			// We replace dashes with the word DASH for logical IDss
+			"ECRRepomyappDASHfrontend": func(output *awsCF.Output) {
+				require.True(t,
+					strings.HasSuffix(*output.OutputValue, fmt.Sprintf("repository/%s/myapp-frontend", project.Name)),
+					fmt.Sprintf("ECRRepomyappDASHfrontend should be suffixed with repository/{project}/myapp but was %s", *output.OutputValue))
+			},
 		}
 		require.True(t, len(deployedStack.Outputs) == len(expectedResultsForKey),
 			"There should have been %d output values - instead there were %d. The value of the CF stack was %s",
diff --git a/internal/pkg/deploy/cloudformation/stack/project_test.go b/internal/pkg/deploy/cloudformation/stack/project_test.go
index 957bebf0..ae45db04 100644
--- a/internal/pkg/deploy/cloudformation/stack/project_test.go
+++ b/internal/pkg/deploy/cloudformation/stack/project_test.go
@@ -50,25 +50,50 @@ func TestProjTemplate(t *testing.T) {
 }
 
 func TestProjResourceTemplate(t *testing.T) {
+	properlyEscapedTemplate := `AWSTemplateFormatVersion: '2010-09-09'
+Outputs:
+  KMSKeyARN:
+    Description: KMS Key used by CodePipeline for encrypting artifacts.
+    Value: !GetAtt KMSKey.Arn
+  PipelineBucket:
+    Description: Bucket used for CodePipeline to stage resources in.
+    Value: !Ref PipelineBuiltArtifactBucket
+  ECRRepoappDASH1:
+    Description: ECR Repo used to store images of the app-1 app.
+	Value: !GetAtt ECRRepoappDASH1.Arn`
+
 	testCases := map[string]struct {
 		box            packd.Box
 		expectedOutput string
+		given          *ProjectResourcesConfig
 		want           error
 	}{
 		"should return error given template not found": {
-			box:  emptyProjectBox(),
-			want: fmt.Errorf("failed to find the cloudformation template at %s", projectResourcesTemplatePath),
+			box:   emptyProjectBox(),
+			given: &ProjectResourcesConfig{},
+			want:  fmt.Errorf("failed to find the cloudformation template at %s", projectResourcesTemplatePath),
 		},
 		"should return template body when present": {
 			box:            projectBoxWithTemplateFile(),
+			given:          &ProjectResourcesConfig{},
 			expectedOutput: mockTemplate,
 		},
+		"should replace dashes in logical IDs": {
+			box: projectBoxTemplateFileWithSafeLogicalIDs(),
+			given: &ProjectResourcesConfig{
+				Accounts: []string{"1234"},
+				Apps:     []string{"app-1"},
+				Version:  1,
+				Project:  "testproject"},
+			expectedOutput: properlyEscapedTemplate,
+		},
 	}
 
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
 			projStack := NewProjectStackConfig(&archer.Project{Name: "testproject", AccountID: "1234"}, tc.box)
-			got, err := projStack.ResourceTemplate(&ProjectResourcesConfig{})
+
+			got, err := projStack.ResourceTemplate(tc.given)
 
 			if tc.want != nil {
 				require.EqualError(t, tc.want, err.Error())
@@ -147,3 +172,22 @@ func projectBoxWithTemplateFile() packd.Box {
 	box.AddString(projectResourcesTemplatePath, mockTemplate)
 	return box
 }
+
+func projectBoxTemplateFileWithSafeLogicalIDs() packd.Box {
+	box := packd.NewMemoryBox()
+	templateWithFunction := `AWSTemplateFormatVersion: '2010-09-09'
+Outputs:
+  KMSKeyARN:
+    Description: KMS Key used by CodePipeline for encrypting artifacts.
+    Value: !GetAtt KMSKey.Arn
+  PipelineBucket:
+    Description: Bucket used for CodePipeline to stage resources in.
+    Value: !Ref PipelineBuiltArtifactBucket
+{{range $app := .Apps}}  ECRRepo{{logicalIDSafe $app}}:
+    Description: ECR Repo used to store images of the {{$app}} app.
+	Value: !GetAtt ECRRepo{{logicalIDSafe $app}}.Arn{{end}}`
+
+	box.AddString(projectTemplatePath, mockTemplate)
+	box.AddString(projectResourcesTemplatePath, templateWithFunction)
+	return box
+}