chore(stack): add rule prefix and override integ test (#2762)

Part of #2588. Work on the basis of #2756.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
aws · Aug 25, 2021 · e7d1feb · e7d1feb
1 parent 83ae7e1
commit e7d1feb
Show file tree

Hide file tree

Showing 15 changed files with 153 additions and 26 deletions.
diff --git a/internal/pkg/deploy/cloudformation/stack/backend_svc.go b/internal/pkg/deploy/cloudformation/stack/backend_svc.go
@@ -12,6 +12,7 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/addon"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 )
 
 // Parameter logical IDs for a backend service.
@@ -56,7 +57,7 @@ func NewBackendService(mft *manifest.BackendService, env, app string, rc Runtime
 				addons: addons,
 			},
 			tc:                  mft.TaskConfig,
-			taskDefOverrideFunc: mockCloudFormationOverrideFunc,
+			taskDefOverrideFunc: override.CloudFormationTemplate,
 		},
 		manifest: mft,
 

diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go
@@ -12,6 +12,7 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/addon"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 )
 
 // Template rendering configuration.
@@ -64,7 +65,7 @@ func NewLoadBalancedWebService(mft *manifest.LoadBalancedWebService, env, app st
 				addons: addons,
 			},
 			tc:                  mft.TaskConfig,
-			taskDefOverrideFunc: mockCloudFormationOverrideFunc,
+			taskDefOverrideFunc: override.CloudFormationTemplate,
 		},
 		manifest:     mft,
 		httpsEnabled: false,

diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack/mocks"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 )
@@ -40,6 +41,10 @@ func (m mockTemplater) Template() (string, error) {
 	return m.tpl, nil
 }
 
+var mockCloudFormationOverrideFunc = func(overrideRules []override.Rule, origTemp []byte) ([]byte, error) {
+	return origTemp, nil
+}
+
 func TestLoadBalancedWebService_StackName(t *testing.T) {
 	testCases := map[string]struct {
 		inSvcName string

diff --git a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go
@@ -16,6 +16,7 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/addon"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 	"github.com/robfig/cron/v3"
 )
 
@@ -107,7 +108,7 @@ func NewScheduledJob(mft *manifest.ScheduledJob, env, app string, rc RuntimeConf
 				addons: addons,
 			},
 			tc:                  mft.TaskConfig,
-			taskDefOverrideFunc: mockCloudFormationOverrideFunc,
+			taskDefOverrideFunc: override.CloudFormationTemplate,
 		},
 		manifest: mft,
 

diff --git a/internal/pkg/deploy/cloudformation/stack/stack_local_integration_test.go b/internal/pkg/deploy/cloudformation/stack/stack_local_integration_test.go
@@ -15,18 +15,21 @@ import (
 )
 
 const (
-	manifestPath           = "autoscaling-svc-manifest.yml"
-	wantedCFNTemplatePath  = "autoscaling-svc-cf.yml"
-	wantedCFNParameterPath = "autoscaling-svc-cf.params.json"
+	autoScalingManifestPath = "manifest.yml"
 
 	appName  = "my-app"
 	envName  = "test"
 	imageURL = "mockImageURL"
 	imageTag = "latest"
 )
 
-func Test_Autoscaling_Integration(t *testing.T) {
-	path := filepath.Join("testdata", "autoscaling", manifestPath)
+func Test_Stack_Local_Integration(t *testing.T) {
+	const (
+		wantedAutoScalingCFNTemplatePath  = "autoscaling-svc-cf.yml"
+		wantedAutoScalingCFNParameterPath = "cf.params.json"
+		wantedOverrideCFNTemplatePath     = "override-cf.yml"
+	)
+	path := filepath.Join("testdata", "stacklocal", autoScalingManifestPath)
 	wantedManifestBytes, err := ioutil.ReadFile(path)
 	require.NoError(t, err)
 	mft, err := manifest.UnmarshalWorkload(wantedManifestBytes)
@@ -44,7 +47,15 @@ func Test_Autoscaling_Integration(t *testing.T) {
 	require.NoError(t, err)
 
 	t.Run("CloudFormation template must contain autoscaling resources", func(t *testing.T) {
-		path := filepath.Join("testdata", "autoscaling", wantedCFNTemplatePath)
+		path := filepath.Join("testdata", "stacklocal", wantedAutoScalingCFNTemplatePath)
+		wantedCFNBytes, err := ioutil.ReadFile(path)
+		require.NoError(t, err)
+
+		require.Contains(t, tpl, string(wantedCFNBytes))
+	})
+
+	t.Run("CloudFormation template must be overridden correctly", func(t *testing.T) {
+		path := filepath.Join("testdata", "stacklocal", wantedOverrideCFNTemplatePath)
 		wantedCFNBytes, err := ioutil.ReadFile(path)
 		require.NoError(t, err)
 
@@ -55,7 +66,7 @@ func Test_Autoscaling_Integration(t *testing.T) {
 		params, err := serializer.SerializedParameters()
 		require.NoError(t, err)
 
-		path := filepath.Join("testdata", "autoscaling", wantedCFNParameterPath)
+		path := filepath.Join("testdata", "stacklocal", wantedAutoScalingCFNParameterPath)
 		wantedCFNParamsBytes, err := ioutil.ReadFile(path)
 		require.NoError(t, err)
 

diff --git a/...stdata/autoscaling/autoscaling-svc-cf.yml → ...estdata/stacklocal/autoscaling-svc-cf.yml b/...stdata/autoscaling/autoscaling-svc-cf.yml → ...estdata/stacklocal/autoscaling-svc-cf.yml
@@ -9,7 +9,6 @@
             Action: 'sts:AssumeRole'
       ManagedPolicyArns:
         - !Sub 'arn:${AWS::Partition}:iam::aws:policy/service-role/AmazonEC2ContainerServiceAutoscaleRole'
-
   AutoScalingTarget:
     Metadata:
       'aws:copilot:description': "An AutoScaling target to scale your service's desired count"
@@ -21,13 +20,11 @@
         Fn::Join:
           - '/'
           - - 'service'
-            - Fn::ImportValue:
-                !Sub '${AppName}-${EnvName}-ClusterId'
+            - Fn::ImportValue: !Sub '${AppName}-${EnvName}-ClusterId'
             - !GetAtt Service.Name
       ScalableDimension: ecs:service:DesiredCount
       ServiceNamespace: ecs
       RoleARN: !GetAtt AutoScalingRole.Arn
-
   AutoScalingPolicyECSServiceAverageCPUUtilization:
     Type: AWS::ApplicationAutoScaling::ScalingPolicy
     Properties:
@@ -40,7 +37,6 @@
         ScaleInCooldown: 120
         ScaleOutCooldown: 60
         TargetValue: 70
-
   AutoScalingPolicyECSServiceAverageMemoryUtilization:
     Type: AWS::ApplicationAutoScaling::ScalingPolicy
     Properties:

diff --git a/...utoscaling/autoscaling-svc-cf.params.json → .../stack/testdata/stacklocal/cf.params.json b/...utoscaling/autoscaling-svc-cf.params.json → .../stack/testdata/stacklocal/cf.params.json
diff --git a/.../autoscaling/autoscaling-svc-manifest.yml → ...on/stack/testdata/stacklocal/manifest.yml b/.../autoscaling/autoscaling-svc-manifest.yml → ...on/stack/testdata/stacklocal/manifest.yml
@@ -32,6 +32,18 @@ count:
 # Enable running commands in your container.
 exec: true
 
+taskdef_overrides:
+  - path: "ContainerDefinitions[0].Ulimits[-].Name"
+    value: "cpu"
+  - path: "ContainerDefinitions[0].Ulimits[0].SoftLimit"
+    value: 1024
+  - path: "ContainerDefinitions[0].Ulimits[0].HardLimit"
+    value: 2048
+  - path: "ContainerDefinitions[0].PortMappings[-].ContainerPort"
+    value: 2056
+  - path: "ContainerDefinitions[0].PortMappings[1].Protocol"
+    value: "udp"
+
 # Optional fields for more advanced use-cases.
 #
 #variables:                    # Pass environment variables as key value pairs.

diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/override-cf.yml b/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/override-cf.yml
@@ -0,0 +1,45 @@
+  TaskDefinition:
+    Metadata:
+      'aws:copilot:description': 'An ECS task definition to group your containers and run them on ECS'
+    Type: AWS::ECS::TaskDefinition
+    DependsOn: LogGroup
+    Properties:
+      Family: !Join ['', [!Ref AppName, '-', !Ref EnvName, '-', !Ref WorkloadName]]
+      NetworkMode: awsvpc
+      RequiresCompatibilities:
+        - FARGATE
+      Cpu: !Ref TaskCPU
+      Memory: !Ref TaskMemory
+      ExecutionRoleArn: !Ref ExecutionRole
+      TaskRoleArn: !Ref TaskRole
+      ContainerDefinitions:
+        - Name: !Ref WorkloadName
+          Image: !Ref ContainerImage
+          # We pipe certain environment variables directly into the task definition.
+          # This lets customers have access to, for example, their LB endpoint - which they'd
+          # have no way of otherwise determining.
+          Environment:
+            - Name: COPILOT_APPLICATION_NAME
+              Value: !Sub '${AppName}'
+            - Name: COPILOT_SERVICE_DISCOVERY_ENDPOINT
+              Value:
+            - Name: COPILOT_ENVIRONMENT_NAME
+              Value: !Sub '${EnvName}'
+            - Name: COPILOT_SERVICE_NAME
+              Value: !Sub '${WorkloadName}'
+            - Name: COPILOT_LB_DNS
+              Value: !GetAtt EnvControllerAction.PublicLoadBalancerDNSName
+          LogConfiguration:
+            LogDriver: awslogs
+            Options:
+              awslogs-region: !Ref AWS::Region
+              awslogs-group: !Ref LogGroup
+              awslogs-stream-prefix: copilot
+          PortMappings:
+            - ContainerPort: !Ref ContainerPort
+            - ContainerPort: 2056
+              Protocol: "udp"
+          Ulimits:
+            - Name: "cpu"
+              SoftLimit: 1024
+              HardLimit: 2048
diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"hash/crc32"
+	"regexp"
 	"strings"
 	"time"
 
@@ -59,6 +60,9 @@ const (
 var (
 	errEphemeralBadSize  = errors.New("ephemeral storage must be between 20 GiB and 200 GiB")
 	errInvalidSpotConfig = errors.New(`"count.spot" and "count.range" cannot be specified together`)
+
+	taskDefOverrideRulePrefixes      = []string{"Resources", "TaskDefinition", "Properties"}
+	invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`}
 )
 
 type convertSidecarOpts struct {
@@ -333,15 +337,29 @@ func logConfigOpts(lc *manifest.Logging) *template.LogConfigOpts {
 
 func convertTaskDefOverrideRules(inRules []manifest.OverrideRule) []override.Rule {
 	var res []override.Rule
+	suffixStr := strings.Join(taskDefOverrideRulePrefixes, override.PathSegmentSeparator)
 	for _, r := range inRules {
+		if !isValidTaskDefOverridePath(r.Path) {
+			continue
+		}
 		res = append(res, override.Rule{
-			Path:  r.Path,
+			Path:  strings.Join([]string{suffixStr, r.Path}, override.PathSegmentSeparator),
 			Value: r.Value,
 		})
 	}
 	return res
 }
 
+func isValidTaskDefOverridePath(path string) bool {
+	for _, s := range invalidTaskDefOverridePathRegexp {
+		re := regexp.MustCompile(fmt.Sprintf(`^%s$`, s))
+		if re.MatchString(path) {
+			return false
+		}
+	}
+	return true
+}
+
 // convertStorageOpts converts a manifest Storage field into template data structures which can be used
 // to execute CFN templates
 func convertStorageOpts(wlName *string, in *manifest.Storage) (*template.StorageOpts, error) {

diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go
@@ -11,7 +11,9 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v3"
 )
 
 func Test_convertSidecar(t *testing.T) {
@@ -545,6 +547,44 @@ func Test_convertAutoscaling(t *testing.T) {
 	}
 }
 
+func Test_convertTaskDefOverrideRules(t *testing.T) {
+	testCases := map[string]struct {
+		inRule []manifest.OverrideRule
+
+		wanted []override.Rule
+	}{
+		"should have proper prefix": {
+			inRule: []manifest.OverrideRule{
+				{
+					Path:  "ContainerDefinitions[0].Ulimits[-].HardLimit",
+					Value: yaml.Node{},
+				},
+				{
+					Path:  "ContainerDefinitions[0].Name",
+					Value: yaml.Node{},
+				},
+				{
+					Path:  "Family",
+					Value: yaml.Node{},
+				},
+			},
+			wanted: []override.Rule{
+				{
+					Path:  "Resources.TaskDefinition.Properties.ContainerDefinitions[0].Ulimits[-].HardLimit",
+					Value: yaml.Node{},
+				},
+			},
+		},
+	}
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			got := convertTaskDefOverrideRules(tc.inRule)
+
+			require.Equal(t, tc.wanted, got)
+		})
+	}
+}
+
 func Test_convertHTTPHealthCheck(t *testing.T) {
 	// These are used by reference to represent the output of the manifest.durationp function.
 	duration15Seconds := 15 * time.Second

diff --git a/internal/pkg/deploy/cloudformation/stack/validate_template_integration_test.go b/internal/pkg/deploy/cloudformation/stack/validate_template_integration_test.go
@@ -18,7 +18,7 @@ import (
 )
 
 func TestAutoscalingIntegration_Validate(t *testing.T) {
-	path := filepath.Join("testdata", "autoscaling", manifestPath)
+	path := filepath.Join("testdata", "stacklocal", autoScalingManifestPath)
 	wantedManifestBytes, err := ioutil.ReadFile(path)
 	require.NoError(t, err)
 	mft, err := manifest.UnmarshalWorkload(wantedManifestBytes)

diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go
@@ -11,6 +11,7 @@ import (
 	"github.com/aws/copilot-cli/internal/pkg/addon"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 	"github.com/aws/copilot-cli/internal/pkg/template"
+	"github.com/aws/copilot-cli/internal/pkg/template/override"
 )
 
 type workerSvcReadParser interface {
@@ -46,7 +47,7 @@ func NewWorkerService(mft *manifest.WorkerService, env, app string, rc RuntimeCo
 				addons: addons,
 			},
 			tc:                  mft.TaskConfig,
-			taskDefOverrideFunc: mockCloudFormationOverrideFunc,
+			taskDefOverrideFunc: override.CloudFormationTemplate,
 		},
 		manifest:      mft,
 		allowedTopics: allowedTopics,

diff --git a/internal/pkg/deploy/cloudformation/stack/workload.go b/internal/pkg/deploy/cloudformation/stack/workload.go
@@ -255,11 +255,6 @@ func envVarOutputNames(outputs []addon.Output) []string {
 	return envVars
 }
 
-// TODO: Remove this mock function and use override.CloudFormationTemplate when it is finished.
-var mockCloudFormationOverrideFunc = func(overrideRules []override.Rule, origTemp []byte) ([]byte, error) {
-	return origTemp, nil
-}
-
 type ecsWkld struct {
 	*wkld
 	tc manifest.TaskConfig

diff --git a/internal/pkg/template/override/rule.go b/internal/pkg/template/override/rule.go
@@ -13,9 +13,10 @@ import (
 )
 
 const (
+	// PathSegmentSeparator is the separator for path segments.
+	PathSegmentSeparator = "."
 	// seqAppendToLastSymbol is the symbol used to add a node to the tail of a list.
 	seqAppendToLastSymbol = "-"
-	pathSegmentSeparator  = "."
 )
 
 // Subset of YAML tag values: http://yaml.org/type/
@@ -51,7 +52,7 @@ func (r Rule) validate() error {
 	if r.Path == "" {
 		return fmt.Errorf("rule path is empty")
 	}
-	pathSegments := strings.Split(r.Path, pathSegmentSeparator)
+	pathSegments := strings.Split(r.Path, PathSegmentSeparator)
 	for _, pathSegment := range pathSegments {
 		if !pathSegmentRegexp.MatchString(pathSegment) {
 			return fmt.Errorf(`invalid override path segment "%s": segments must be of the form "array[0]", "array[%s]" or "key"`,
@@ -62,7 +63,7 @@ func (r Rule) validate() error {
 }
 
 func (r Rule) parse() (nodeUpserter, error) {
-	pathSegments := strings.SplitN(r.Path, pathSegmentSeparator, 2)
+	pathSegments := strings.SplitN(r.Path, PathSegmentSeparator, 2)
 	segment, err := parsePathSegment(pathSegments[0])
 	if err != nil {
 		return nil, err