From f97ecbeb2919f14e5ef04f05aedffbf7beb11c0c Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 8 Jul 2021 15:38:38 -0400 Subject: [PATCH 01/17] chore: adds worker to template Adds subscribe data structures to template workload. Adds template function for generating subscription JSON that lists out subscribed topics, their services, and any related queue they may have. --- internal/pkg/template/workload.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 4510b86e9a6..e94791988c9 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -245,6 +245,17 @@ type TopicSubscription struct { Service *string } +// SubscribeOpts holds configuration needed if the service has subscriptions. +type SubscribeOpts struct { + Topics []*TopicSubscription +} + +// TopicSubscription holds information needed to render a SNS Topic Subscription in a container definition. +type TopicSubscription struct { + Name *string + Service *string +} + // NetworkOpts holds AWS networking configuration for the workloads. type NetworkOpts struct { AssignPublicIP string @@ -293,6 +304,7 @@ type WorkloadOpts struct { DesiredCountLambda string EnvControllerLambda string CredentialsParameter string + Subscribe SubscribeOpts // Additional options for job templates. ScheduleExpression string From 009b8f1d5a696bf1665c30e0111933e741037295 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 16 Jul 2021 13:01:46 -0700 Subject: [PATCH 02/17] adds validation/converters for template adds validations, excluding init/deploy checks for another pr --- .../cloudformation/stack/transformers.go | 30 ++++++++ .../cloudformation/stack/transformers_test.go | 74 ++++++++++++++++++- .../deploy/cloudformation/stack/validate.go | 26 +++++++ 3 files changed, 129 insertions(+), 1 deletion(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 56ba7a8e866..91e4d89c955 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -659,3 +659,33 @@ func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []str Service: aws.String(t.Service), }, nil } + +func convertSubscribe(s *manifest.SubscribeConfig) (*template.SubscribeOpts, error) { + if s == nil || s.Topics == nil { + return nil, nil + } + + subscriptions := template.SubscribeOpts{} + for _, sb := range *s.Topics { + ts, err := convertTopicSubscription(sb) + if err != nil { + return nil, err + } + + subscriptions.Topics = append(subscriptions.Topics, ts) + } + + return &subscriptions, nil +} + +func convertTopicSubscription(t manifest.TopicSubscription) (*template.TopicSubscription, error) { + err := validateTopicSubscription(t) + if err != nil { + return nil, fmt.Errorf(`invalid topic subscription %s: %w`, t.Name, err) + } + + return &template.TopicSubscription{ + Name: aws.String(t.Name), + Service: aws.String(t.Service), + }, nil +} diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 6e005855954..38959bab2ce 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1481,7 +1481,7 @@ func Test_convertPublish(t *testing.T) { {}, }, }, - wantedError: errMissingPublishTopicField, + wantedError: errMissingPubSubTopicField, }, "publish with no workers": { inPublish: &manifest.PublishConfig{ @@ -1647,3 +1647,75 @@ func Test_convertSubscribe(t *testing.T) { }) } } + +func Test_convertSubscribe(t *testing.T) { + testCases := map[string]struct { + inSubscribe *manifest.SubscribeConfig + + wanted *template.SubscribeOpts + wantedError error + }{ + "empty subscription": { + inSubscribe: &manifest.SubscribeConfig{}, + wanted: nil, + }, + "subscription with empty topic subscriptions": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: &[]manifest.TopicSubscription{ + {}, + }, + }, + wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "", errMissingPubSubTopicField), + }, + "valid publish": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: &[]manifest.TopicSubscription{ + { + Name: "topic1", + Service: "service1", + }, + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("topic1"), + Service: aws.String("service1"), + }, + }, + }, + }, + "invalid topic name": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: &[]manifest.TopicSubscription{ + { + Name: "t@p!c1~", + Service: "service1", + }, + }, + }, + wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "t@p!c1~", errInvalidPubSubTopicName), + }, + "invalid service name": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: &[]manifest.TopicSubscription{ + { + Name: "topic1", + Service: "s#rv!ce1~", + }, + }, + }, + wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "topic1", errSvcNameBadFormat), + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got, err := convertSubscribe(tc.inSubscribe) + if tc.wantedError != nil { + require.EqualError(t, err, tc.wantedError.Error()) + } else { + require.Equal(t, got, tc.wanted) + } + }) + } +} diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 2f9c8012a56..dcec217b7aa 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -21,13 +21,30 @@ const ( dependsOnHealthy = "HEALTHY" ) +const ( + // AppTagKey is tag key for Copilot app. + AppTagKey = "copilot-application" + // EnvTagKey is tag key for Copilot env. + EnvTagKey = "copilot-environment" + // ServiceTagKey is tag key for Copilot svc. + ServiceTagKey = "copilot-service" +) + // Empty field errors. var ( +<<<<<<< HEAD errNoFSID = errors.New("volume field `efs.id` cannot be empty") errNoContainerPath = errors.New("`path` cannot be empty") errNoSourceVolume = errors.New("`source_volume` cannot be empty") errEmptyEFSConfig = errors.New("bad EFS configuration: `efs` cannot be empty") errMissingPublishTopicField = errors.New("field `publish.topics[].name` cannot be empty") +======= + errNoFSID = errors.New("volume field `efs.id` cannot be empty") + errNoContainerPath = errors.New("`path` cannot be empty") + errNoSourceVolume = errors.New("`source_volume` cannot be empty") + errEmptyEFSConfig = errors.New("bad EFS configuration: `efs` cannot be empty") + errMissingPubSubTopicField = errors.New("topic `name` cannot be empty") +>>>>>>> 8cc1426c (adds validation/converters for template) ) // Conditional errors. @@ -47,7 +64,10 @@ var ( errInvalidSvcName = errors.New("service names cannot be empty") errSvcNameTooLong = errors.New("service names must not exceed 255 characters") errSvcNameBadFormat = errors.New("service names must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") +<<<<<<< HEAD errTopicSubscriptionNotAllowed = errors.New("topic not in list of topics available to subscribe to") +======= +>>>>>>> 8cc1426c (adds validation/converters for template) ) // Container dependency status options @@ -411,9 +431,15 @@ func validateContainerPath(input string) error { // ValidatePubSubName validates naming is correct for topics in publishing/subscribing cases, such as naming for a // SNS Topic intended for a publisher. +<<<<<<< HEAD func validatePubSubName(name string) error { if len(name) == 0 { return errMissingPublishTopicField +======= +func validatePubSubName(name *string) error { + if name == nil || len(aws.StringValue(name)) == 0 { + return errMissingPubSubTopicField +>>>>>>> 8cc1426c (adds validation/converters for template) } // Name must contain letters, numbers, and can't use special characters besides underscores and hyphens. From 2440e1e6ce5d1a0c2e3032486644e3a2cd3fda54 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Tue, 20 Jul 2021 15:07:34 -0400 Subject: [PATCH 03/17] adds queue to worker adds queue ds adds queue validators/converters --- .../cloudformation/stack/transformers.go | 101 ++++++++++--- .../cloudformation/stack/transformers_test.go | 133 ++++++++---------- .../deploy/cloudformation/stack/validate.go | 54 +++---- .../cloudformation/stack/validate_test.go | 72 +++++++++- internal/pkg/manifest/worker_svc.go | 73 +++++++++- internal/pkg/manifest/worker_svc_test.go | 61 +++++++- internal/pkg/template/workload.go | 53 +++++-- 7 files changed, 399 insertions(+), 148 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 91e4d89c955..af57505d027 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -630,62 +630,119 @@ func convertTopic(t manifest.Topic, accountID, partition, region, app, env, svc }, nil } -func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string) (*template.SubscribeOpts, error) { +func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, accountID, region, app, env, svc string) (*template.SubscribeOpts, error) { if s == nil || s.Topics == nil { return nil, nil } + partition, ok := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region) + if !ok { + return nil, fmt.Errorf("find the partition for region %s", region) + } var subscriptions template.SubscribeOpts - for _, sb := range *s.Topics { - ts, err := convertTopicSubscription(sb, validTopicARNs) + for _, sb := range s.Topics { + ts, err := convertTopicSubscription(sb, validTopicARNs, accountID, partition.ID(), region, app, env, svc) if err != nil { return nil, err } subscriptions.Topics = append(subscriptions.Topics, ts) } + queue, err := convertTopicQueue(s.Queue, accountID, partition.ID(), region, app, env, svc) + if err != nil { + return nil, err + } + subscriptions.Queue = queue return &subscriptions, nil } -func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []string) (*template.TopicSubscription, error) { +func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []string, accountID, partition, region, app, env, svc string) (*template.TopicSubscription, error) { err := validateTopicSubscription(t, validTopicARNs) if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } + queue, err := convertTopicQueue(t.Queue, accountID, partition, region, app, env, svc) + if err != nil { + return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) + } return &template.TopicSubscription{ Name: aws.String(t.Name), Service: aws.String(t.Service), + Queue: queue, }, nil } -func convertSubscribe(s *manifest.SubscribeConfig) (*template.SubscribeOpts, error) { - if s == nil || s.Topics == nil { +func convertTopicQueue(q *manifest.SQSQueue, accountID, partition, region, app, env, svc string) (*template.SQSQueue, error) { + if q == nil { return nil, nil } + retention, err := convertTime(q.Retention, 0, 1209600) + if err != nil { + return nil, fmt.Errorf("invalid `retention`: %w", err) + } + delay, err := convertTime(q.Delay, 0, 900) + if err != nil { + return nil, fmt.Errorf("invalid `delay`: %w", err) + } + timeout, err := convertTime(q.Timeout, 0, 43200) + if err != nil { + return nil, fmt.Errorf("invalid `timeout`: %w", err) + } + deadletter := convertDeadLetter(q.DeadLetter) + + return &template.SQSQueue{ + Name: q.Name, + Retention: retention, + Delay: delay, + Timeout: timeout, + KMS: q.KMS, + DeadLetter: deadletter, + FIFO: convertFIFO(q.FIFO), + AccountID: accountID, + Region: region, + Partition: partition, + App: app, + Env: env, + Svc: svc, + }, nil +} - subscriptions := template.SubscribeOpts{} - for _, sb := range *s.Topics { - ts, err := convertTopicSubscription(sb) - if err != nil { - return nil, err - } +func convertTime(t *string, floor, ceiling float64) (*int, error) { + if t == nil { + return nil, nil + } - subscriptions.Topics = append(subscriptions.Topics, ts) + tm, err := time.ParseDuration(aws.StringValue(t)) + if err != nil { + return nil, errDurationInvalid{reason: err} + } + if err := validateTime(tm, floor, ceiling); err != nil { + return nil, err } - return &subscriptions, nil + return aws.Int(int(tm.Seconds())), nil } -func convertTopicSubscription(t manifest.TopicSubscription) (*template.TopicSubscription, error) { - err := validateTopicSubscription(t) - if err != nil { - return nil, fmt.Errorf(`invalid topic subscription %s: %w`, t.Name, err) +func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { + if f == nil || (f.Enabled != nil && aws.BoolValue(f.Enabled) == false) { + return nil } - return &template.TopicSubscription{ - Name: aws.String(t.Name), - Service: aws.String(t.Service), - }, nil + return &template.FIFOQueue{ + HighThroughput: f.FIFO.HighThroughput, + } +} + +func convertDeadLetter(d *manifest.DeadLetterQueue) *template.DeadLetterQueue { + if d == nil { + return nil + } + validateDeadLetter(d) + + return &template.DeadLetterQueue{ + Id: d.ID, + Tries: &d.Tries, + } } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 38959bab2ce..286dde4de67 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1481,7 +1481,7 @@ func Test_convertPublish(t *testing.T) { {}, }, }, - wantedError: errMissingPubSubTopicField, + wantedError: errMissingPublishTopicField, }, "publish with no workers": { inPublish: &manifest.PublishConfig{ @@ -1565,7 +1565,13 @@ func Test_convertPublish(t *testing.T) { } func Test_convertSubscribe(t *testing.T) { - validTopics := []string{"arn:aws:us-east-1:123456789012:app-env-svc-name", "arn:aws:us-east-1:123456789012:app-env-svc-name2"} + validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:s-east-1:123456789012:app-env-svc-name2"} + accountId := "123456789123" + partition := "aws" + region := "us-west-2" + app := "testapp" + env := "testenv" + svc := "hello" testCases := map[string]struct { inSubscribe *manifest.SubscribeConfig @@ -1578,20 +1584,36 @@ func Test_convertSubscribe(t *testing.T) { }, "subscription with empty topic subscriptions": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ {}, }, }, wantedError: fmt.Errorf(`invalid topic subscription "": %w`, errMissingPublishTopicField), }, - "valid publish": { + "valid subscribe": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ { Name: "name", Service: "svc", }, }, + Queue: &manifest.SQSQueue{ + Name: aws.String("bestqueue"), + Retention: aws.String("111s"), + Delay: aws.String("111s"), + Timeout: aws.String("111s"), + KMS: aws.Bool(true), + DeadLetter: &manifest.DeadLetterQueue{ + Tries: 35, + }, + FIFO: &manifest.FIFOOrBool{ + Enabled: aws.Bool(true), + FIFO: manifest.FIFOQueue{ + HighThroughput: aws.Bool(false), + }, + }, + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1600,11 +1622,30 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, + Queue: &template.SQSQueue{ + Name: aws.String("bestqueue"), + Retention: aws.Int(111), + Delay: aws.Int(111), + Timeout: aws.Int(111), + KMS: aws.Bool(true), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: &template.FIFOQueue{ + HighThroughput: aws.Bool(false), + }, + AccountID: accountId, + Partition: partition, + Region: region, + App: app, + Env: env, + Svc: svc, + }, }, }, "invalid topic name": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ { Name: "t@p!c1~", Service: "service1", @@ -1615,7 +1656,7 @@ func Test_convertSubscribe(t *testing.T) { }, "invalid service name": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ { Name: "topic1", Service: "s#rv!ce1~", @@ -1626,7 +1667,7 @@ func Test_convertSubscribe(t *testing.T) { }, "topic not allowed": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ { Name: "topic1", Service: "svc", @@ -1635,86 +1676,28 @@ func Test_convertSubscribe(t *testing.T) { }, wantedError: fmt.Errorf(`invalid topic subscription "topic1": %w`, errTopicSubscriptionNotAllowed), }, - } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - got, err := convertSubscribe(tc.inSubscribe, validTopics) - if tc.wantedError != nil { - require.EqualError(t, err, tc.wantedError.Error()) - } else { - require.Equal(t, got, tc.wanted) - } - }) - } -} - -func Test_convertSubscribe(t *testing.T) { - testCases := map[string]struct { - inSubscribe *manifest.SubscribeConfig - - wanted *template.SubscribeOpts - wantedError error - }{ - "empty subscription": { - inSubscribe: &manifest.SubscribeConfig{}, - wanted: nil, - }, - "subscription with empty topic subscriptions": { - inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ - {}, - }, - }, - wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "", errMissingPubSubTopicField), - }, - "valid publish": { - inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ - { - Name: "topic1", - Service: "service1", - }, - }, - }, - wanted: &template.SubscribeOpts{ - Topics: []*template.TopicSubscription{ - { - Name: aws.String("topic1"), - Service: aws.String("service1"), - }, - }, - }, - }, - "invalid topic name": { + "subscribe queue delay invalid": { inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ + Topics: []manifest.TopicSubscription{ { - Name: "t@p!c1~", - Service: "service1", + Name: "name", + Service: "svc", }, }, - }, - wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "t@p!c1~", errInvalidPubSubTopicName), - }, - "invalid service name": { - inSubscribe: &manifest.SubscribeConfig{ - Topics: &[]manifest.TopicSubscription{ - { - Name: "topic1", - Service: "s#rv!ce1~", - }, + Queue: &manifest.SQSQueue{ + Delay: aws.String("999s"), }, }, - wantedError: fmt.Errorf(`invalid topic subscription %s: %w`, "topic1", errSvcNameBadFormat), + wantedError: fmt.Errorf("invalid `delay`: time must be between 0 and 900 seconds"), }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - got, err := convertSubscribe(tc.inSubscribe) + got, err := convertSubscribe(tc.inSubscribe, validTopics, accountId, region, app, env, svc) if tc.wantedError != nil { require.EqualError(t, err, tc.wantedError.Error()) } else { - require.Equal(t, got, tc.wanted) + require.Equal(t, tc.wanted, got) } }) } diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index dcec217b7aa..5d8fc3e7d63 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -8,8 +8,10 @@ import ( "fmt" "regexp" "strings" + "time" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/copilot-cli/internal/pkg/manifest" ) @@ -21,30 +23,14 @@ const ( dependsOnHealthy = "HEALTHY" ) -const ( - // AppTagKey is tag key for Copilot app. - AppTagKey = "copilot-application" - // EnvTagKey is tag key for Copilot env. - EnvTagKey = "copilot-environment" - // ServiceTagKey is tag key for Copilot svc. - ServiceTagKey = "copilot-service" -) - // Empty field errors. var ( -<<<<<<< HEAD errNoFSID = errors.New("volume field `efs.id` cannot be empty") errNoContainerPath = errors.New("`path` cannot be empty") errNoSourceVolume = errors.New("`source_volume` cannot be empty") errEmptyEFSConfig = errors.New("bad EFS configuration: `efs` cannot be empty") errMissingPublishTopicField = errors.New("field `publish.topics[].name` cannot be empty") -======= - errNoFSID = errors.New("volume field `efs.id` cannot be empty") - errNoContainerPath = errors.New("`path` cannot be empty") - errNoSourceVolume = errors.New("`source_volume` cannot be empty") - errEmptyEFSConfig = errors.New("bad EFS configuration: `efs` cannot be empty") - errMissingPubSubTopicField = errors.New("topic `name` cannot be empty") ->>>>>>> 8cc1426c (adds validation/converters for template) + errDeadLetterQueueTries = errors.New("DeadLetter `tries` field cannot exceed 1000") ) // Conditional errors. @@ -64,10 +50,7 @@ var ( errInvalidSvcName = errors.New("service names cannot be empty") errSvcNameTooLong = errors.New("service names must not exceed 255 characters") errSvcNameBadFormat = errors.New("service names must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") -<<<<<<< HEAD errTopicSubscriptionNotAllowed = errors.New("topic not in list of topics available to subscribe to") -======= ->>>>>>> 8cc1426c (adds validation/converters for template) ) // Container dependency status options @@ -431,15 +414,9 @@ func validateContainerPath(input string) error { // ValidatePubSubName validates naming is correct for topics in publishing/subscribing cases, such as naming for a // SNS Topic intended for a publisher. -<<<<<<< HEAD func validatePubSubName(name string) error { if len(name) == 0 { return errMissingPublishTopicField -======= -func validatePubSubName(name *string) error { - if name == nil || len(aws.StringValue(name)) == 0 { - return errMissingPubSubTopicField ->>>>>>> 8cc1426c (adds validation/converters for template) } // Name must contain letters, numbers, and can't use special characters besides underscores and hyphens. @@ -500,16 +477,31 @@ func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []s // Check that the topic is included in the list of available topics for _, topicARN := range validTopicARNs { - splitArn := strings.Split(topicARN, ":") - topicName := strings.Split(splitArn[len(splitArn)-1], "-") - if len(topicName) < 4 { - continue + arn, err := arn.Parse(topicARN) + if err != nil { + return err } + topicName := arn.Resource - if topicName[2] == ts.Service && topicName[3] == ts.Name { + if strings.Contains(topicName, ts.Service) && strings.Contains(topicName, ts.Name) { return nil } } return errTopicSubscriptionNotAllowed } + +func validateTime(t time.Duration, floor, ceiling float64) error { + if t.Seconds() < floor || t.Seconds() > ceiling { + return fmt.Errorf("time must be between %0.0f and %0.0f seconds", floor, ceiling) + } + + return nil +} + +func validateDeadLetter(dl *manifest.DeadLetterQueue) error { + if dl.Tries > 1000 { + return errDeadLetterQueueTries + } + return nil +} diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 92f4a12c264..2e371a5b00d 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -4,8 +4,10 @@ package stack import ( + "errors" "fmt" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/manifest" @@ -567,7 +569,7 @@ func TestValidateWorkerName(t *testing.T) { } func TestValidateTopicSubscription(t *testing.T) { - validTopics := []string{"arn:aws:us-east-1:123456789012:app-env-svc-name", "arn:aws:us-east-1:123456789012:app-env-svc-name2"} + validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"} testCases := map[string]struct { inTS manifest.TopicSubscription @@ -613,3 +615,71 @@ func TestValidateTopicSubscription(t *testing.T) { }) } } + +func TestValidateTime(t *testing.T) { + testCases := map[string]struct { + inTime time.Duration + inFloor float64 + inCeiling float64 + + wantErr error + }{ + "good case": { + inTime: 500, + inFloor: 0, + inCeiling: 600, + wantErr: nil, + }, + "bad time": { + inTime: 601000000000, + inFloor: 0, + inCeiling: 600, + wantErr: errors.New("time must be between 0 and 600 seconds"), + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := validateTime(tc.inTime, tc.inFloor, tc.inCeiling) + + if tc.wantErr == nil { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.wantErr.Error()) + } + }) + } +} + +func TestValidateDeadLetter(t *testing.T) { + testCases := map[string]struct { + inDL *manifest.DeadLetterQueue + + wantErr error + }{ + "good case": { + inDL: &manifest.DeadLetterQueue{ + Tries: 35, + }, + wantErr: nil, + }, + "wrong number of tries": { + inDL: &manifest.DeadLetterQueue{ + Tries: 9999, + }, + wantErr: errDeadLetterQueueTries, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := validateDeadLetter(tc.inDL) + + if tc.wantErr == nil { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.wantErr.Error()) + } + }) + } +} diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index e8b3369af52..fafa29be2c8 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -4,15 +4,22 @@ package manifest import ( + "errors" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/template" "github.com/imdario/mergo" + "gopkg.in/yaml.v3" ) const ( workerSvcManifestPath = "workloads/services/worker/manifest.yml" ) +var ( + errUnmarshalFIFO = errors.New("cannot unmarshal field `fifo` under `subscribe`") +) + // WorkerService holds the configuration to create a worker service. type WorkerService struct { Workload `yaml:",inline"` @@ -38,18 +45,76 @@ type WorkerServiceConfig struct { type WorkerServiceProps struct { WorkloadProps HealthCheck *ContainerHealthCheck // Optional healthcheck configuration. - Topics *[]TopicSubscription // Optional topics for subscriptions + Topics []TopicSubscription // Optional topics for subscriptions } // SubscribeConfig represents the configurable options for setting up subscriptions. type SubscribeConfig struct { - Topics *[]TopicSubscription `yaml:"topics"` + Topics []TopicSubscription `yaml:"topics"` + Queue *SQSQueue `yaml:"queue"` } // TopicSubscription represents the configurable options for setting up a SNS Topic Subscription. type TopicSubscription struct { - Name string `yaml:"name"` - Service string `yaml:"service"` + Name string `yaml:"name"` + Service string `yaml:"service"` + Queue *SQSQueue `yaml:"queue"` +} + +// SQSQueue represents the configurable options for setting up a SQS Queue. +type SQSQueue struct { + Name *string `yaml:"name"` + Retention *string `yaml:"retention"` + Delay *string `yaml:"delay"` + Timeout *string `yaml:"timeout"` + KMS *bool `yaml:"kms"` + DeadLetter *DeadLetterQueue `yaml:"dead_letter"` + FIFO *FIFOOrBool `yaml:"fifo"` +} + +// DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. +type DeadLetterQueue struct { + ID *string `yaml:"queue_id"` + Tries uint16 `yaml:"tries"` +} + +// FIFOOrBool contains custom unmarshaling logic for the `fifo` field in the manifest. +type FIFOOrBool struct { + FIFO FIFOQueue + Enabled *bool +} + +// FIFOQueue represents the configurable options for setting up a FIFO queue. +type FIFOQueue struct { + HighThroughput *bool `yaml:"high_throughput"` +} + +// IsEmpty returns empty if the struct has all zero members. +func (q *FIFOQueue) IsEmpty() bool { + return q.HighThroughput == nil +} + +// UnmarshalYAML implements the yaml(v2) interface. It allows FIFOQueue to be specified as a +// string or a struct alternately. +func (q *FIFOOrBool) UnmarshalYAML(unmarshal func(interface{}) error) error { + if err := unmarshal(&q.FIFO); err != nil { + switch err.(type) { + case *yaml.TypeError: + break + default: + return err + } + } + + if !q.FIFO.IsEmpty() { + q.Enabled = nil + return nil + } + + if err := unmarshal(&q.Enabled); err != nil { + return errUnmarshalFIFO + } + return nil } // NewWorkerService applies the props to a default Worker service configuration with diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index fb83228fcc0..83d4c9cb1e5 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -140,7 +140,7 @@ func TestWorkerSvc_MarshalBinary(t *testing.T) { Name: "testers", Dockerfile: "./testers/Dockerfile", }, - Topics: &[]TopicSubscription{ + Topics: []TopicSubscription{ { Name: "testTopic", Service: "service4TestTopic", @@ -259,7 +259,7 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: &[]TopicSubscription{ + Topics: []TopicSubscription{ { Name: "topicName", Service: "bestService", @@ -299,7 +299,7 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: &[]TopicSubscription{ + Topics: []TopicSubscription{ { Name: "topicName", Service: "bestService", @@ -484,7 +484,7 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: &[]TopicSubscription{ + Topics: []TopicSubscription{ { Name: "topicName", Service: "bestService", @@ -746,3 +746,56 @@ func TestWorkerSvc_ApplyEnv_CountOverrides(t *testing.T) { }) } } + +type testFIFO struct { + FIFO *FIFOOrBool `yaml:"fifo"` +} + +func Test_UnmarshalFifo(t *testing.T) { + testCases := map[string]struct { + manifest []byte + want testFIFO + wantErr string + }{ + "fifo specified": { + manifest: []byte(` +fifo: + high_throughput: true`), + want: testFIFO{ + FIFO: &FIFOOrBool{ + FIFO: FIFOQueue{ + HighThroughput: aws.Bool(true), + }, + }, + }, + }, + "enabled": { + manifest: []byte(` +fifo: true`), + want: testFIFO{ + FIFO: &FIFOOrBool{ + Enabled: aws.Bool(true), + }, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + // GIVEN + v := testFIFO{ + FIFO: &FIFOOrBool{}, + } + + // WHEN + err := yaml.Unmarshal(tc.manifest, &v) + // THEN + if tc.wantErr == "" { + require.NoError(t, err) + require.Equal(t, tc.want.FIFO.Enabled, v.FIFO.Enabled) + require.Equal(t, tc.want.FIFO.FIFO.HighThroughput, v.FIFO.FIFO.HighThroughput) + } else { + require.EqualError(t, err, tc.wantErr) + } + }) + } +} diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index e94791988c9..99e981195e9 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -43,9 +43,8 @@ const ( // Constants for ARN options. const ( - snsArnPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" - AWSPartition = "aws" - AWSChinaPartition = "aws-cn" + snsArnPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" + sqsArnPattern = "arn:%s:sqs:%s:%s:%s-%s-%s-%s" ) var ( @@ -237,23 +236,43 @@ type Topic struct { // SubscribeOpts holds configuration needed if the service has subscriptions. type SubscribeOpts struct { Topics []*TopicSubscription + Queue *SQSQueue } // TopicSubscription holds information needed to render a SNS Topic Subscription in a container definition. type TopicSubscription struct { Name *string Service *string + Queue *SQSQueue } -// SubscribeOpts holds configuration needed if the service has subscriptions. -type SubscribeOpts struct { - Topics []*TopicSubscription +// SQSQueue holds information needed to render a SQS Queue in a container definition. +type SQSQueue struct { + Name *string + Retention *int + Delay *int + Timeout *int + KMS *bool + DeadLetter *DeadLetterQueue + FIFO *FIFOQueue + + Region string + Partition string + AccountID string + App string + Env string + Svc string } -// TopicSubscription holds information needed to render a SNS Topic Subscription in a container definition. -type TopicSubscription struct { - Name *string - Service *string +// DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. +type DeadLetterQueue struct { + Id *string + Tries *uint16 +} + +// FIFOQueue holds information needed to specify a SQS Queue as FIFO in a container definition. +type FIFOQueue struct { + HighThroughput *bool } // NetworkOpts holds AWS networking configuration for the workloads. @@ -304,7 +323,6 @@ type WorkloadOpts struct { DesiredCountLambda string EnvControllerLambda string CredentialsParameter string - Subscribe SubscribeOpts // Additional options for job templates. ScheduleExpression string @@ -456,3 +474,16 @@ func envControllerParameters(o WorkloadOpts) []string { func (t Topic) ARN() string { return fmt.Sprintf(snsArnPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) } + +// ARN determines the arn for a queue using the SNSTopic arn start and the name of the topic +func (q SQSQueue) ARN(topicName *string) string { + var name string + if q.Name != nil { + name = aws.StringValue(q.Name) + } else if topicName != nil { + name = aws.StringValue(topicName) + } else { + name = "SQSQueue" + } + return fmt.Sprintf(snsArnPattern, q.Partition, q.Region, q.AccountID, q.App, q.Env, q.Svc, name) +} From bec476a84279f54eea709bf4598857523fca4210 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 12:17:06 -0400 Subject: [PATCH 04/17] sns->sqs pattern --- internal/pkg/template/workload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 99e981195e9..30591274828 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -485,5 +485,5 @@ func (q SQSQueue) ARN(topicName *string) string { } else { name = "SQSQueue" } - return fmt.Sprintf(snsArnPattern, q.Partition, q.Region, q.AccountID, q.App, q.Env, q.Svc, name) + return fmt.Sprintf(sqsArnPattern, q.Partition, q.Region, q.AccountID, q.App, q.Env, q.Svc, name) } From 1545d8f9f07998f486af35dcbfaf17c5b65942c7 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 12:33:31 -0400 Subject: [PATCH 05/17] adjusts formatting, bool --- .../deploy/cloudformation/stack/transformers.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index af57505d027..03d2eb6a8b9 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -690,7 +690,10 @@ func convertTopicQueue(q *manifest.SQSQueue, accountID, partition, region, app, if err != nil { return nil, fmt.Errorf("invalid `timeout`: %w", err) } - deadletter := convertDeadLetter(q.DeadLetter) + deadletter, err := convertDeadLetter(q.DeadLetter) + if err != nil { + return nil, err + } return &template.SQSQueue{ Name: q.Name, @@ -726,7 +729,7 @@ func convertTime(t *string, floor, ceiling float64) (*int, error) { } func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { - if f == nil || (f.Enabled != nil && aws.BoolValue(f.Enabled) == false) { + if f == nil || !aws.BoolValue(f.Enabled) { return nil } @@ -735,14 +738,16 @@ func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { } } -func convertDeadLetter(d *manifest.DeadLetterQueue) *template.DeadLetterQueue { +func convertDeadLetter(d *manifest.DeadLetterQueue) (*template.DeadLetterQueue, error) { if d == nil { - return nil + return nil, nil + } + if err := validateDeadLetter(d); err != nil { + return nil, err } - validateDeadLetter(d) return &template.DeadLetterQueue{ Id: d.ID, Tries: &d.Tries, - } + }, nil } From e5c5a43031b67b3848d38980c1f7d20c3a40f77a Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 12:42:15 -0400 Subject: [PATCH 06/17] adjusts arn to uri --- internal/pkg/template/workload.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 30591274828..5a3a0d535d9 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -44,7 +44,7 @@ const ( // Constants for ARN options. const ( snsArnPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" - sqsArnPattern = "arn:%s:sqs:%s:%s:%s-%s-%s-%s" + sqsURIPattern = "https://sqs.amazon%s.%s.com/%s/%s-%s-%s-%s" ) var ( @@ -470,13 +470,13 @@ func envControllerParameters(o WorkloadOpts) []string { return parameters } -// ARN determines the arn for a topic using the SNSTopic arn start and the name of the topic +// ARN determines the arn for a topic using the SNSTopic name and account information func (t Topic) ARN() string { return fmt.Sprintf(snsArnPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) } -// ARN determines the arn for a queue using the SNSTopic arn start and the name of the topic -func (q SQSQueue) ARN(topicName *string) string { +// URI determines the uri for a queue using the queue name and account information +func (q SQSQueue) URI(topicName *string) string { var name string if q.Name != nil { name = aws.StringValue(q.Name) @@ -485,5 +485,5 @@ func (q SQSQueue) ARN(topicName *string) string { } else { name = "SQSQueue" } - return fmt.Sprintf(sqsArnPattern, q.Partition, q.Region, q.AccountID, q.App, q.Env, q.Svc, name) + return fmt.Sprintf(sqsURIPattern, q.Region, q.Partition, q.AccountID, q.App, q.Env, q.Svc, name) } From 23bb8558f47fb54ef0b8b17b7d96246b1d4ba2ec Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 13:54:17 -0400 Subject: [PATCH 07/17] adjusts URI retrieval method --- .../cloudformation/stack/transformers.go | 20 +++++++++---------- .../cloudformation/stack/transformers_test.go | 4 +--- internal/pkg/template/workload.go | 7 +++---- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 03d2eb6a8b9..a2123ab11ae 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -634,21 +634,22 @@ func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, acco if s == nil || s.Topics == nil { return nil, nil } - partition, ok := endpoints.PartitionForRegion(endpoints.DefaultPartitions(), region) - if !ok { - return nil, fmt.Errorf("find the partition for region %s", region) + + sqsEndpoint, err := endpoints.DefaultResolver().EndpointFor(endpoints.SqsServiceID, region) + if err != nil { + return nil, err } var subscriptions template.SubscribeOpts for _, sb := range s.Topics { - ts, err := convertTopicSubscription(sb, validTopicARNs, accountID, partition.ID(), region, app, env, svc) + ts, err := convertTopicSubscription(sb, validTopicARNs, sqsEndpoint.URL, accountID, app, env, svc) if err != nil { return nil, err } subscriptions.Topics = append(subscriptions.Topics, ts) } - queue, err := convertTopicQueue(s.Queue, accountID, partition.ID(), region, app, env, svc) + queue, err := convertTopicQueue(s.Queue, sqsEndpoint.URL, accountID, app, env, svc) if err != nil { return nil, err } @@ -657,12 +658,12 @@ func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, acco return &subscriptions, nil } -func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []string, accountID, partition, region, app, env, svc string) (*template.TopicSubscription, error) { +func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []string, url, accountID, app, env, svc string) (*template.TopicSubscription, error) { err := validateTopicSubscription(t, validTopicARNs) if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } - queue, err := convertTopicQueue(t.Queue, accountID, partition, region, app, env, svc) + queue, err := convertTopicQueue(t.Queue, url, accountID, app, env, svc) if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } @@ -674,7 +675,7 @@ func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []str }, nil } -func convertTopicQueue(q *manifest.SQSQueue, accountID, partition, region, app, env, svc string) (*template.SQSQueue, error) { +func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (*template.SQSQueue, error) { if q == nil { return nil, nil } @@ -704,8 +705,7 @@ func convertTopicQueue(q *manifest.SQSQueue, accountID, partition, region, app, DeadLetter: deadletter, FIFO: convertFIFO(q.FIFO), AccountID: accountID, - Region: region, - Partition: partition, + URL: url, App: app, Env: env, Svc: svc, diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 286dde4de67..364846cb7b2 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1567,7 +1567,6 @@ func Test_convertPublish(t *testing.T) { func Test_convertSubscribe(t *testing.T) { validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:s-east-1:123456789012:app-env-svc-name2"} accountId := "123456789123" - partition := "aws" region := "us-west-2" app := "testapp" env := "testenv" @@ -1635,8 +1634,7 @@ func Test_convertSubscribe(t *testing.T) { HighThroughput: aws.Bool(false), }, AccountID: accountId, - Partition: partition, - Region: region, + URL: "https://sqs.us-west-2.amazonaws.com", App: app, Env: env, Svc: svc, diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 5a3a0d535d9..0559d955576 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -44,7 +44,7 @@ const ( // Constants for ARN options. const ( snsArnPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" - sqsURIPattern = "https://sqs.amazon%s.%s.com/%s/%s-%s-%s-%s" + sqsURIPattern = "%s/%s/%s-%s-%s-%s" ) var ( @@ -256,8 +256,7 @@ type SQSQueue struct { DeadLetter *DeadLetterQueue FIFO *FIFOQueue - Region string - Partition string + URL string AccountID string App string Env string @@ -485,5 +484,5 @@ func (q SQSQueue) URI(topicName *string) string { } else { name = "SQSQueue" } - return fmt.Sprintf(sqsURIPattern, q.Region, q.Partition, q.AccountID, q.App, q.Env, q.Svc, name) + return fmt.Sprintf(sqsURIPattern, q.URL, q.AccountID, q.App, q.Env, q.Svc, name) } From 17f578e8c5c45d9deafd30e6047ea0eef1e1e816 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 17:44:16 -0400 Subject: [PATCH 08/17] adjusts time to duration, adds constants --- .../cloudformation/stack/transformers.go | 40 ++++++++++++++----- .../cloudformation/stack/transformers_test.go | 31 +++++++++----- .../deploy/cloudformation/stack/validate.go | 22 ++++++---- .../cloudformation/stack/validate_test.go | 4 +- internal/pkg/manifest/worker_svc.go | 7 ++-- internal/pkg/template/workload.go | 6 +-- 6 files changed, 74 insertions(+), 36 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index a2123ab11ae..2e6a192feb5 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -41,6 +41,16 @@ const ( capacityProviderFargate = "FARGATE" ) +// Time interval options. +const ( + retentionMinValueSeconds = 0 + retentionMaxValueSeconds = 1209600 + delayMinValueSeconds = 0 + delayMaxValueSeconds = 900 + timeoutMinValueSeconds = 0 + timeoutMaxValueSeconds = 43200 +) + 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`) @@ -659,7 +669,7 @@ func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, acco } func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []string, url, accountID, app, env, svc string) (*template.TopicSubscription, error) { - err := validateTopicSubscription(t, validTopicARNs) + err := validateTopicSubscription(t, validTopicARNs, app, env) if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } @@ -679,15 +689,15 @@ func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc strin if q == nil { return nil, nil } - retention, err := convertTime(q.Retention, 0, 1209600) + retention, err := convertRetention(q.Retention) if err != nil { return nil, fmt.Errorf("invalid `retention`: %w", err) } - delay, err := convertTime(q.Delay, 0, 900) + delay, err := convertDelay(q.Delay) if err != nil { return nil, fmt.Errorf("invalid `delay`: %w", err) } - timeout, err := convertTime(q.Timeout, 0, 43200) + timeout, err := convertTimeout(q.Timeout) if err != nil { return nil, fmt.Errorf("invalid `timeout`: %w", err) } @@ -712,20 +722,28 @@ func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc strin }, nil } -func convertTime(t *string, floor, ceiling float64) (*int, error) { +func convertTime(t *time.Duration, floor, ceiling float64) (*int64, error) { if t == nil { return nil, nil } - tm, err := time.ParseDuration(aws.StringValue(t)) - if err != nil { - return nil, errDurationInvalid{reason: err} - } - if err := validateTime(tm, floor, ceiling); err != nil { + if err := validateTime(*t, floor, ceiling); err != nil { return nil, err } - return aws.Int(int(tm.Seconds())), nil + return aws.Int64(int64(t.Seconds())), nil +} + +func convertRetention(t *time.Duration) (*int64, error) { + return convertTime(t, retentionMinValueSeconds, retentionMaxValueSeconds) +} + +func convertDelay(t *time.Duration) (*int64, error) { + return convertTime(t, delayMinValueSeconds, delayMaxValueSeconds) +} + +func convertTimeout(t *time.Duration) (*int64, error) { + return convertTime(t, timeoutMinValueSeconds, timeoutMaxValueSeconds) } func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 364846cb7b2..8962ddb3ab4 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1568,9 +1568,9 @@ func Test_convertSubscribe(t *testing.T) { validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:s-east-1:123456789012:app-env-svc-name2"} accountId := "123456789123" region := "us-west-2" - app := "testapp" - env := "testenv" - svc := "hello" + app := "app" + env := "env" + svc := "svc" testCases := map[string]struct { inSubscribe *manifest.SubscribeConfig @@ -1599,9 +1599,9 @@ func Test_convertSubscribe(t *testing.T) { }, Queue: &manifest.SQSQueue{ Name: aws.String("bestqueue"), - Retention: aws.String("111s"), - Delay: aws.String("111s"), - Timeout: aws.String("111s"), + Retention: (*time.Duration)(aws.Int64(111000000000)), + Delay: (*time.Duration)(aws.Int64(111000000000)), + Timeout: (*time.Duration)(aws.Int64(111000000000)), KMS: aws.Bool(true), DeadLetter: &manifest.DeadLetterQueue{ Tries: 35, @@ -1623,9 +1623,9 @@ func Test_convertSubscribe(t *testing.T) { }, Queue: &template.SQSQueue{ Name: aws.String("bestqueue"), - Retention: aws.Int(111), - Delay: aws.Int(111), - Timeout: aws.Int(111), + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), KMS: aws.Bool(true), DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), @@ -1674,6 +1674,17 @@ func Test_convertSubscribe(t *testing.T) { }, wantedError: fmt.Errorf(`invalid topic subscription "topic1": %w`, errTopicSubscriptionNotAllowed), }, + "sneaky topic not allowed": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: "sneakytopic", + Service: "svc-name", + }, + }, + }, + wantedError: fmt.Errorf(`invalid topic subscription "sneakytopic": %w`, errTopicSubscriptionNotAllowed), + }, "subscribe queue delay invalid": { inSubscribe: &manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ @@ -1683,7 +1694,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &manifest.SQSQueue{ - Delay: aws.String("999s"), + Delay: (*time.Duration)(aws.Int64(99900000000000)), }, }, wantedError: fmt.Errorf("invalid `delay`: time must be between 0 and 900 seconds"), diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 5d8fc3e7d63..172d39d290f 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "regexp" - "strings" "time" "github.com/aws/aws-sdk-go/aws" @@ -30,7 +29,7 @@ var ( errNoSourceVolume = errors.New("`source_volume` cannot be empty") errEmptyEFSConfig = errors.New("bad EFS configuration: `efs` cannot be empty") errMissingPublishTopicField = errors.New("field `publish.topics[].name` cannot be empty") - errDeadLetterQueueTries = errors.New("DeadLetter `tries` field cannot exceed 1000") + errDeadLetterQueueTries = fmt.Errorf("DeadLetter `tries` field cannot exceed %d", deadLetterTriesMaxValue) ) // Conditional errors. @@ -53,14 +52,14 @@ var ( errTopicSubscriptionNotAllowed = errors.New("topic not in list of topics available to subscribe to") ) -// Container dependency status options +// Container dependency status options. var ( essentialContainerValidStatuses = []string{dependsOnStart, dependsOnHealthy} dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} sidecarDependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) -// Regex options +// Regex options. var ( awsSNSTopicRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`) // Validates that an expression contains only letters, numbers, underscores, and hyphens. awsNameRegexp = regexp.MustCompile(`^[a-z][a-z0-9\-]+$`) // Validates that an expression starts with a letter and only contains letters, numbers, and hyphens. @@ -68,6 +67,12 @@ var ( trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. ) +// Options for SQS Queues +var ( + resourceNameFormat = "%s-%s-%s-%s" // Format for copilot resource names of form app-env-svc-name + deadLetterTriesMaxValue = 1000 +) + // Validate that paths contain only an approved set of characters to guard against command injection. // We can accept 0-9A-Za-z-_. func validatePath(input string, maxLength int) error { @@ -466,7 +471,7 @@ func isCorrectSvcNameFormat(s string) bool { return len(trailingMatch) == 0 } -func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []string) error { +func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []string, app, env string) error { if err := validatePubSubName(ts.Name); err != nil { return err } @@ -476,14 +481,15 @@ func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []s } // Check that the topic is included in the list of available topics + topicName := fmt.Sprintf(resourceNameFormat, app, env, ts.Service, ts.Name) for _, topicARN := range validTopicARNs { arn, err := arn.Parse(topicARN) if err != nil { return err } - topicName := arn.Resource + validTopicName := arn.Resource - if strings.Contains(topicName, ts.Service) && strings.Contains(topicName, ts.Name) { + if validTopicName == topicName { return nil } } @@ -500,7 +506,7 @@ func validateTime(t time.Duration, floor, ceiling float64) error { } func validateDeadLetter(dl *manifest.DeadLetterQueue) error { - if dl.Tries > 1000 { + if dl.Tries > uint16(deadLetterTriesMaxValue) { return errDeadLetterQueueTries } return nil diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 2e371a5b00d..5ca7f104e10 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -570,6 +570,8 @@ func TestValidateWorkerName(t *testing.T) { func TestValidateTopicSubscription(t *testing.T) { validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"} + app := "app" + env := "env" testCases := map[string]struct { inTS manifest.TopicSubscription @@ -605,7 +607,7 @@ func TestValidateTopicSubscription(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { - err := validateTopicSubscription(tc.inTS, validTopics) + err := validateTopicSubscription(tc.inTS, validTopics, app, env) if tc.wantErr == nil { require.NoError(t, err) diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index fafa29be2c8..3474a22a501 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -5,6 +5,7 @@ package manifest import ( "errors" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/template" @@ -64,9 +65,9 @@ type TopicSubscription struct { // SQSQueue represents the configurable options for setting up a SQS Queue. type SQSQueue struct { Name *string `yaml:"name"` - Retention *string `yaml:"retention"` - Delay *string `yaml:"delay"` - Timeout *string `yaml:"timeout"` + Retention *time.Duration `yaml:"retention"` + Delay *time.Duration `yaml:"delay"` + Timeout *time.Duration `yaml:"timeout"` KMS *bool `yaml:"kms"` DeadLetter *DeadLetterQueue `yaml:"dead_letter"` FIFO *FIFOOrBool `yaml:"fifo"` diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 0559d955576..0a2c35ec005 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -249,9 +249,9 @@ type TopicSubscription struct { // SQSQueue holds information needed to render a SQS Queue in a container definition. type SQSQueue struct { Name *string - Retention *int - Delay *int - Timeout *int + Retention *int64 + Delay *int64 + Timeout *int64 KMS *bool DeadLetter *DeadLetterQueue FIFO *FIFOQueue From 740f24cc07399d4e731e383d52e1e893ff4dfb00 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 18:27:41 -0400 Subject: [PATCH 09/17] adjusts error message for delay, timeout, and retention --- internal/pkg/deploy/cloudformation/stack/transformers.go | 6 +++--- .../pkg/deploy/cloudformation/stack/transformers_test.go | 2 +- internal/pkg/deploy/cloudformation/stack/validate.go | 2 +- internal/pkg/deploy/cloudformation/stack/validate_test.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 2e6a192feb5..3dd19c1fa23 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -691,15 +691,15 @@ func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc strin } retention, err := convertRetention(q.Retention) if err != nil { - return nil, fmt.Errorf("invalid `retention`: %w", err) + return nil, fmt.Errorf(" `retention` %w", err) } delay, err := convertDelay(q.Delay) if err != nil { - return nil, fmt.Errorf("invalid `delay`: %w", err) + return nil, fmt.Errorf("`delay` %w", err) } timeout, err := convertTimeout(q.Timeout) if err != nil { - return nil, fmt.Errorf("invalid `timeout`: %w", err) + return nil, fmt.Errorf("`timeout` %w", err) } deadletter, err := convertDeadLetter(q.DeadLetter) if err != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 8962ddb3ab4..2dc2a114e6c 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1697,7 +1697,7 @@ func Test_convertSubscribe(t *testing.T) { Delay: (*time.Duration)(aws.Int64(99900000000000)), }, }, - wantedError: fmt.Errorf("invalid `delay`: time must be between 0 and 900 seconds"), + wantedError: fmt.Errorf("`delay` must be between 0 and 900 seconds"), }, } for name, tc := range testCases { diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 172d39d290f..b018ffe9e62 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -499,7 +499,7 @@ func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []s func validateTime(t time.Duration, floor, ceiling float64) error { if t.Seconds() < floor || t.Seconds() > ceiling { - return fmt.Errorf("time must be between %0.0f and %0.0f seconds", floor, ceiling) + return fmt.Errorf("must be between %0.0f and %0.0f seconds", floor, ceiling) } return nil diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 5ca7f104e10..7b706a74a3d 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -636,7 +636,7 @@ func TestValidateTime(t *testing.T) { inTime: 601000000000, inFloor: 0, inCeiling: 600, - wantErr: errors.New("time must be between 0 and 600 seconds"), + wantErr: errors.New("must be between 0 and 600 seconds"), }, } From 33c761722ba76d2c86e60bc2f45c1c24cd4bc60b Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 22 Jul 2021 20:27:01 -0400 Subject: [PATCH 10/17] adjusts duration usage --- .../cloudformation/stack/transformers.go | 14 +++++++------- .../cloudformation/stack/transformers_test.go | 16 +++++++++------- .../deploy/cloudformation/stack/validate.go | 6 +++--- .../cloudformation/stack/validate_test.go | 18 +++++++++--------- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 3dd19c1fa23..7717423b380 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -659,7 +659,7 @@ func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, acco subscriptions.Topics = append(subscriptions.Topics, ts) } - queue, err := convertTopicQueue(s.Queue, sqsEndpoint.URL, accountID, app, env, svc) + queue, err := convertQueue(s.Queue, sqsEndpoint.URL, accountID, app, env, svc) if err != nil { return nil, err } @@ -673,7 +673,7 @@ func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []str if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } - queue, err := convertTopicQueue(t.Queue, url, accountID, app, env, svc) + queue, err := convertQueue(t.Queue, url, accountID, app, env, svc) if err != nil { return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err) } @@ -685,7 +685,7 @@ func convertTopicSubscription(t manifest.TopicSubscription, validTopicARNs []str }, nil } -func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (*template.SQSQueue, error) { +func convertQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (*template.SQSQueue, error) { if q == nil { return nil, nil } @@ -722,7 +722,7 @@ func convertTopicQueue(q *manifest.SQSQueue, url, accountID, app, env, svc strin }, nil } -func convertTime(t *time.Duration, floor, ceiling float64) (*int64, error) { +func convertTime(t *time.Duration, floor, ceiling time.Duration) (*int64, error) { if t == nil { return nil, nil } @@ -735,15 +735,15 @@ func convertTime(t *time.Duration, floor, ceiling float64) (*int64, error) { } func convertRetention(t *time.Duration) (*int64, error) { - return convertTime(t, retentionMinValueSeconds, retentionMaxValueSeconds) + return convertTime(t, retentionMinValueSeconds*time.Second, retentionMaxValueSeconds*time.Second) } func convertDelay(t *time.Duration) (*int64, error) { - return convertTime(t, delayMinValueSeconds, delayMaxValueSeconds) + return convertTime(t, delayMinValueSeconds*time.Second, delayMaxValueSeconds*time.Second) } func convertTimeout(t *time.Duration) (*int64, error) { - return convertTime(t, timeoutMinValueSeconds, timeoutMaxValueSeconds) + return convertTime(t, timeoutMinValueSeconds*time.Second, timeoutMaxValueSeconds*time.Second) } func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 2dc2a114e6c..8463744f8bf 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -547,8 +547,8 @@ func Test_convertAutoscaling(t *testing.T) { func Test_convertHTTPHealthCheck(t *testing.T) { // These are used by reference to represent the output of the manifest.durationp function. - duration15Seconds := time.Duration(15 * time.Second) - duration60Seconds := time.Duration(60 * time.Second) + duration15Seconds := 15 * time.Second + duration60Seconds := 60 * time.Second testCases := map[string]struct { inputPath *string inputSuccessCodes *string @@ -1571,6 +1571,8 @@ func Test_convertSubscribe(t *testing.T) { app := "app" env := "env" svc := "svc" + duration111Seconds := 111 * time.Second + duration5Days := 120 * time.Hour testCases := map[string]struct { inSubscribe *manifest.SubscribeConfig @@ -1599,9 +1601,9 @@ func Test_convertSubscribe(t *testing.T) { }, Queue: &manifest.SQSQueue{ Name: aws.String("bestqueue"), - Retention: (*time.Duration)(aws.Int64(111000000000)), - Delay: (*time.Duration)(aws.Int64(111000000000)), - Timeout: (*time.Duration)(aws.Int64(111000000000)), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, KMS: aws.Bool(true), DeadLetter: &manifest.DeadLetterQueue{ Tries: 35, @@ -1694,10 +1696,10 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &manifest.SQSQueue{ - Delay: (*time.Duration)(aws.Int64(99900000000000)), + Delay: &duration5Days, }, }, - wantedError: fmt.Errorf("`delay` must be between 0 and 900 seconds"), + wantedError: fmt.Errorf("`delay` must be between 0s and 15m0s"), }, } for name, tc := range testCases { diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index b018ffe9e62..197fdd5468a 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -497,9 +497,9 @@ func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []s return errTopicSubscriptionNotAllowed } -func validateTime(t time.Duration, floor, ceiling float64) error { - if t.Seconds() < floor || t.Seconds() > ceiling { - return fmt.Errorf("must be between %0.0f and %0.0f seconds", floor, ceiling) +func validateTime(t, floor, ceiling time.Duration) error { + if t < floor || t > ceiling { + return fmt.Errorf("must be between %v and %v", floor, ceiling) } return nil diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 7b706a74a3d..7d7953d719b 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -621,22 +621,22 @@ func TestValidateTopicSubscription(t *testing.T) { func TestValidateTime(t *testing.T) { testCases := map[string]struct { inTime time.Duration - inFloor float64 - inCeiling float64 + inFloor time.Duration + inCeiling time.Duration wantErr error }{ "good case": { - inTime: 500, - inFloor: 0, - inCeiling: 600, + inTime: 500 * time.Second, + inFloor: 0 * time.Second, + inCeiling: 600 * time.Second, wantErr: nil, }, "bad time": { - inTime: 601000000000, - inFloor: 0, - inCeiling: 600, - wantErr: errors.New("must be between 0 and 600 seconds"), + inTime: 500 * time.Hour, + inFloor: 0 * time.Second, + inCeiling: 600 * time.Second, + wantErr: errors.New("must be between 0s and 10m0s"), }, } From ccf9edb9bb86da878ca243deee7ae3af334703ee Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 23 Jul 2021 12:12:50 -0400 Subject: [PATCH 11/17] adds subscribe tests to worker_svc --- .../cloudformation/stack/transformers.go | 4 +- .../cloudformation/stack/transformers_test.go | 16 +- .../deploy/cloudformation/stack/validate.go | 6 +- .../cloudformation/stack/validate_test.go | 30 +- internal/pkg/manifest/worker_svc.go | 8 +- internal/pkg/manifest/worker_svc_test.go | 275 +++++++++++++++++- internal/pkg/template/workload.go | 11 +- 7 files changed, 309 insertions(+), 41 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 7717423b380..63861c97f76 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -651,7 +651,7 @@ func convertSubscribe(s *manifest.SubscribeConfig, validTopicARNs []string, acco } var subscriptions template.SubscribeOpts - for _, sb := range s.Topics { + for _, sb := range *s.Topics { ts, err := convertTopicSubscription(sb, validTopicARNs, sqsEndpoint.URL, accountID, app, env, svc) if err != nil { return nil, err @@ -766,6 +766,6 @@ func convertDeadLetter(d *manifest.DeadLetterQueue) (*template.DeadLetterQueue, return &template.DeadLetterQueue{ Id: d.ID, - Tries: &d.Tries, + Tries: d.Tries, }, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 8463744f8bf..65173a414c8 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1585,7 +1585,7 @@ func Test_convertSubscribe(t *testing.T) { }, "subscription with empty topic subscriptions": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ {}, }, }, @@ -1593,7 +1593,7 @@ func Test_convertSubscribe(t *testing.T) { }, "valid subscribe": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "name", Service: "svc", @@ -1606,7 +1606,7 @@ func Test_convertSubscribe(t *testing.T) { Timeout: &duration111Seconds, KMS: aws.Bool(true), DeadLetter: &manifest.DeadLetterQueue{ - Tries: 35, + Tries: aws.Uint16(35), }, FIFO: &manifest.FIFOOrBool{ Enabled: aws.Bool(true), @@ -1645,7 +1645,7 @@ func Test_convertSubscribe(t *testing.T) { }, "invalid topic name": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "t@p!c1~", Service: "service1", @@ -1656,7 +1656,7 @@ func Test_convertSubscribe(t *testing.T) { }, "invalid service name": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "topic1", Service: "s#rv!ce1~", @@ -1667,7 +1667,7 @@ func Test_convertSubscribe(t *testing.T) { }, "topic not allowed": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "topic1", Service: "svc", @@ -1678,7 +1678,7 @@ func Test_convertSubscribe(t *testing.T) { }, "sneaky topic not allowed": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "sneakytopic", Service: "svc-name", @@ -1689,7 +1689,7 @@ func Test_convertSubscribe(t *testing.T) { }, "subscribe queue delay invalid": { inSubscribe: &manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ + Topics: &[]manifest.TopicSubscription{ { Name: "name", Service: "svc", diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 197fdd5468a..d68449a3eb0 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -67,7 +67,7 @@ var ( trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. ) -// Options for SQS Queues +// Options for SQS Queues. var ( resourceNameFormat = "%s-%s-%s-%s" // Format for copilot resource names of form app-env-svc-name deadLetterTriesMaxValue = 1000 @@ -485,7 +485,7 @@ func validateTopicSubscription(ts manifest.TopicSubscription, validTopicARNs []s for _, topicARN := range validTopicARNs { arn, err := arn.Parse(topicARN) if err != nil { - return err + continue } validTopicName := arn.Resource @@ -506,7 +506,7 @@ func validateTime(t, floor, ceiling time.Duration) error { } func validateDeadLetter(dl *manifest.DeadLetterQueue) error { - if dl.Tries > uint16(deadLetterTriesMaxValue) { + if aws.Uint16Value(dl.Tries) > uint16(deadLetterTriesMaxValue) { return errDeadLetterQueueTries } return nil diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 7d7953d719b..1ff7d845079 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -569,11 +569,11 @@ func TestValidateWorkerName(t *testing.T) { } func TestValidateTopicSubscription(t *testing.T) { - validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"} app := "app" env := "env" testCases := map[string]struct { - inTS manifest.TopicSubscription + inTS manifest.TopicSubscription + inValidTopics []string wantErr error }{ @@ -582,32 +582,44 @@ func TestValidateTopicSubscription(t *testing.T) { Name: "name2", Service: "svc", }, - wantErr: nil, + inValidTopics: []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"}, + wantErr: nil, }, "empty name": { inTS: manifest.TopicSubscription{ Service: "svc", }, - wantErr: errMissingPublishTopicField, + inValidTopics: []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"}, + wantErr: errMissingPublishTopicField, }, "empty svc name": { inTS: manifest.TopicSubscription{ Name: "theName", }, - wantErr: errInvalidSvcName, + inValidTopics: []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"}, + wantErr: errInvalidSvcName, }, "topic not in list of valid topics": { inTS: manifest.TopicSubscription{ Name: "badName", Service: "svc", }, - wantErr: errTopicSubscriptionNotAllowed, + inValidTopics: []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"}, + wantErr: errTopicSubscriptionNotAllowed, + }, + "topic in list of valid topics but one cannot be parsed": { + inTS: manifest.TopicSubscription{ + Name: "name2", + Service: "svc", + }, + inValidTopics: []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "", "arn:aws:sns:us-east-1:123456789012:app-env-svc-name2"}, + wantErr: nil, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - err := validateTopicSubscription(tc.inTS, validTopics, app, env) + err := validateTopicSubscription(tc.inTS, tc.inValidTopics, app, env) if tc.wantErr == nil { require.NoError(t, err) @@ -661,13 +673,13 @@ func TestValidateDeadLetter(t *testing.T) { }{ "good case": { inDL: &manifest.DeadLetterQueue{ - Tries: 35, + Tries: aws.Uint16(35), }, wantErr: nil, }, "wrong number of tries": { inDL: &manifest.DeadLetterQueue{ - Tries: 9999, + Tries: aws.Uint16(9999), }, wantErr: errDeadLetterQueueTries, }, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 3474a22a501..8e8b1a21ccb 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -46,13 +46,13 @@ type WorkerServiceConfig struct { type WorkerServiceProps struct { WorkloadProps HealthCheck *ContainerHealthCheck // Optional healthcheck configuration. - Topics []TopicSubscription // Optional topics for subscriptions + Topics *[]TopicSubscription // Optional topics for subscriptions } // SubscribeConfig represents the configurable options for setting up subscriptions. type SubscribeConfig struct { - Topics []TopicSubscription `yaml:"topics"` - Queue *SQSQueue `yaml:"queue"` + Topics *[]TopicSubscription `yaml:"topics"` + Queue *SQSQueue `yaml:"queue"` } // TopicSubscription represents the configurable options for setting up a SNS Topic Subscription. @@ -76,7 +76,7 @@ type SQSQueue struct { // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { ID *string `yaml:"queue_id"` - Tries uint16 `yaml:"tries"` + Tries *uint16 `yaml:"tries"` } // FIFOOrBool contains custom unmarshaling logic for the `fifo` field in the manifest. diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 83d4c9cb1e5..926aa5624b6 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -140,7 +140,7 @@ func TestWorkerSvc_MarshalBinary(t *testing.T) { Name: "testers", Dockerfile: "./testers/Dockerfile", }, - Topics: []TopicSubscription{ + Topics: &[]TopicSubscription{ { Name: "testTopic", Service: "service4TestTopic", @@ -259,7 +259,7 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: []TopicSubscription{ + Topics: &[]TopicSubscription{ { Name: "topicName", Service: "bestService", @@ -299,10 +299,10 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: []TopicSubscription{ + Topics: &[]TopicSubscription{ { - Name: "topicName", - Service: "bestService", + Name: "topicName2", + Service: "bestService2", }, }, }, @@ -410,6 +410,127 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, } + duration111Seconds := 111 * time.Second + mockWorkerServiceWithSubscribeNilOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: nil, + }, + }, + } + mockWorkerServiceWithNilSubscribeOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: nil, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + } + mockWorkerServiceWithEmptySubscribeOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{}, + }, + }, + } + mockWorkerServiceWithSubscribeEmptyOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{}, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + } testCases := map[string]struct { svc *WorkerService inEnvName string @@ -484,10 +605,10 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, Subscribe: &SubscribeConfig{ - Topics: []TopicSubscription{ + Topics: &[]TopicSubscription{ { - Name: "topicName", - Service: "bestService", + Name: "topicName2", + Service: "bestService2", }, }, }, @@ -573,6 +694,133 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, original: &mockWorkerServiceWithImageOverrideLocationByBuild, }, + "with nil subscribe overriden by full subscribe": { + svc: &mockWorkerServiceWithNilSubscribeOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithNilSubscribeOverride, + }, + "with full subscribe and nil subscribe env": { + svc: &mockWorkerServiceWithSubscribeNilOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithSubscribeNilOverride, + }, + "with empty subscribe overriden by full subscribe": { + svc: &mockWorkerServiceWithEmptySubscribeOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + ImageConfig: ImageWithHealthcheck{ + Image: Image{}, + }, + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithEmptySubscribeOverride, + }, + "with full subscribe and empty subscribe env": { + svc: &mockWorkerServiceWithSubscribeEmptyOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + KMS: aws.Bool(true), + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithSubscribeEmptyOverride, + }, } for name, tc := range testCases { @@ -755,7 +1003,7 @@ func Test_UnmarshalFifo(t *testing.T) { testCases := map[string]struct { manifest []byte want testFIFO - wantErr string + wantErr error }{ "fifo specified": { manifest: []byte(` @@ -778,6 +1026,11 @@ fifo: true`), }, }, }, + "invalid input": { + manifest: []byte(` +fifo: xyz`), + wantErr: errUnmarshalFIFO, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -789,12 +1042,12 @@ fifo: true`), // WHEN err := yaml.Unmarshal(tc.manifest, &v) // THEN - if tc.wantErr == "" { + if tc.wantErr == nil { require.NoError(t, err) require.Equal(t, tc.want.FIFO.Enabled, v.FIFO.Enabled) require.Equal(t, tc.want.FIFO.FIFO.HighThroughput, v.FIFO.FIFO.HighThroughput) } else { - require.EqualError(t, err, tc.wantErr) + require.EqualError(t, err, tc.wantErr.Error()) } }) } diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 0a2c35ec005..028f30efe5f 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -39,11 +39,14 @@ const ( DisablePublicIP = "DISABLED" PublicSubnetsPlacement = "PublicSubnets" PrivateSubnetsPlacement = "PrivateSubnets" + + // SQSQueue Configurations. + SQSQueueName = "SQSQueue" ) // Constants for ARN options. const ( - snsArnPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" + snsARNPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" sqsURIPattern = "%s/%s/%s-%s-%s-%s" ) @@ -471,10 +474,10 @@ func envControllerParameters(o WorkloadOpts) []string { // ARN determines the arn for a topic using the SNSTopic name and account information func (t Topic) ARN() string { - return fmt.Sprintf(snsArnPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) + return fmt.Sprintf(snsARNPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) } -// URI determines the uri for a queue using the queue name and account information +// URI determines the uri for a queue using the queue name, topic name, and account information func (q SQSQueue) URI(topicName *string) string { var name string if q.Name != nil { @@ -482,7 +485,7 @@ func (q SQSQueue) URI(topicName *string) string { } else if topicName != nil { name = aws.StringValue(topicName) } else { - name = "SQSQueue" + name = SQSQueueName } return fmt.Sprintf(sqsURIPattern, q.URL, q.AccountID, q.App, q.Env, q.Svc, name) } From 54811e23926c5e83efa5dcac3e92fb73baff363b Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 23 Jul 2021 14:27:44 -0400 Subject: [PATCH 12/17] adjusts template bools to non ptr adjusts bools in template to be non-pointer due to CF issues --- .../cloudformation/stack/transformers.go | 11 +++- .../cloudformation/stack/transformers_test.go | 64 ++++++++++++++++++- internal/pkg/template/workload.go | 6 +- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 63861c97f76..7dd4bfe5f11 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -56,6 +56,11 @@ var ( errInvalidSpotConfig = errors.New(`"count.spot" and "count.range" cannot be specified together`) ) +// ARN format options. +var ( + sqsARNFormat = "arn:%s:sqs:%s:%s:%s-%s-%s-%s" +) + type convertSidecarOpts struct { sidecarConfig map[string]*manifest.SidecarConfig imageConfig *manifest.Image @@ -711,7 +716,7 @@ func convertQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (* Retention: retention, Delay: delay, Timeout: timeout, - KMS: q.KMS, + KMS: aws.BoolValue(q.KMS), DeadLetter: deadletter, FIFO: convertFIFO(q.FIFO), AccountID: accountID, @@ -752,7 +757,7 @@ func convertFIFO(f *manifest.FIFOOrBool) *template.FIFOQueue { } return &template.FIFOQueue{ - HighThroughput: f.FIFO.HighThroughput, + HighThroughput: aws.BoolValue(f.FIFO.HighThroughput), } } @@ -765,7 +770,7 @@ func convertDeadLetter(d *manifest.DeadLetterQueue) (*template.DeadLetterQueue, } return &template.DeadLetterQueue{ - Id: d.ID, + ID: d.ID, Tries: d.Tries, }, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 65173a414c8..71c1673ab2d 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1565,7 +1565,7 @@ func Test_convertPublish(t *testing.T) { } func Test_convertSubscribe(t *testing.T) { - validTopics := []string{"arn:aws:sns:us-east-1:123456789012:app-env-svc-name", "arn:aws:sns:s-east-1:123456789012:app-env-svc-name2"} + validTopics := []string{"arn:aws:sns:us-west-2:123456789123:app-env-svc-name", "arn:aws:sns:us-west-2:123456789123:app-env-svc-name2"} accountId := "123456789123" region := "us-west-2" app := "app" @@ -1607,6 +1607,7 @@ func Test_convertSubscribe(t *testing.T) { KMS: aws.Bool(true), DeadLetter: &manifest.DeadLetterQueue{ Tries: aws.Uint16(35), + ID: aws.String("deadLetter"), }, FIFO: &manifest.FIFOOrBool{ Enabled: aws.Bool(true), @@ -1628,12 +1629,13 @@ func Test_convertSubscribe(t *testing.T) { Retention: aws.Int64(111), Delay: aws.Int64(111), Timeout: aws.Int64(111), - KMS: aws.Bool(true), + KMS: true, DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), + ID: aws.String("deadLetter"), }, FIFO: &template.FIFOQueue{ - HighThroughput: aws.Bool(false), + HighThroughput: false, }, AccountID: accountId, URL: "https://sqs.us-west-2.amazonaws.com", @@ -1713,3 +1715,59 @@ func Test_convertSubscribe(t *testing.T) { }) } } + +func Test_convertFIFO(t *testing.T) { + testCases := map[string]struct { + inFIFO *manifest.FIFOOrBool + + wanted *template.FIFOQueue + wantedError error + }{ + "empty FIFO": { + inFIFO: &manifest.FIFOOrBool{}, + wanted: nil, + }, + "FIFO with enabled false": { + inFIFO: &manifest.FIFOOrBool{ + Enabled: aws.Bool(false), + }, + wanted: nil, + }, + "FIFO with enabled true and no high throughput": { + inFIFO: &manifest.FIFOOrBool{ + Enabled: aws.Bool(true), + }, + wanted: &template.FIFOQueue{ + HighThroughput: false, + }, + }, + "FIFO with enabled true and high throughput false": { + inFIFO: &manifest.FIFOOrBool{ + Enabled: aws.Bool(true), + FIFO: manifest.FIFOQueue{ + HighThroughput: aws.Bool(false), + }, + }, + wanted: &template.FIFOQueue{ + HighThroughput: false, + }, + }, + "FIFO with enabled true and high throughput true": { + inFIFO: &manifest.FIFOOrBool{ + Enabled: aws.Bool(true), + FIFO: manifest.FIFOQueue{ + HighThroughput: aws.Bool(true), + }, + }, + wanted: &template.FIFOQueue{ + HighThroughput: true, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got := convertFIFO(tc.inFIFO) + require.Equal(t, tc.wanted, got) + }) + } +} diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 028f30efe5f..90e99de37ab 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -255,7 +255,7 @@ type SQSQueue struct { Retention *int64 Delay *int64 Timeout *int64 - KMS *bool + KMS bool DeadLetter *DeadLetterQueue FIFO *FIFOQueue @@ -268,13 +268,13 @@ type SQSQueue struct { // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. type DeadLetterQueue struct { - Id *string + ID *string Tries *uint16 } // FIFOQueue holds information needed to specify a SQS Queue as FIFO in a container definition. type FIFOQueue struct { - HighThroughput *bool + HighThroughput bool } // NetworkOpts holds AWS networking configuration for the workloads. From e05a5b592e6177cfddb5c1cb04855091535826fe Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 23 Jul 2021 14:43:48 -0400 Subject: [PATCH 13/17] removes unused var --- internal/pkg/deploy/cloudformation/stack/transformers.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 7dd4bfe5f11..8e1f2ba652b 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -56,11 +56,6 @@ var ( errInvalidSpotConfig = errors.New(`"count.spot" and "count.range" cannot be specified together`) ) -// ARN format options. -var ( - sqsARNFormat = "arn:%s:sqs:%s:%s:%s-%s-%s-%s" -) - type convertSidecarOpts struct { sidecarConfig map[string]*manifest.SidecarConfig imageConfig *manifest.Image From 573db1cf4bf72cf6723d69f339f114ffee893492 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Mon, 26 Jul 2021 13:33:22 -0400 Subject: [PATCH 14/17] removes kms, adjusts DL ID to name, adds topic tests for worker --- .../cloudformation/stack/transformers.go | 3 +- .../cloudformation/stack/transformers_test.go | 6 +- internal/pkg/manifest/worker_svc.go | 3 +- internal/pkg/manifest/worker_svc_test.go | 147 ++++++++++++++++-- internal/pkg/template/workload.go | 5 +- 5 files changed, 139 insertions(+), 25 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 8e1f2ba652b..ecc0001efa1 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -711,7 +711,6 @@ func convertQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (* Retention: retention, Delay: delay, Timeout: timeout, - KMS: aws.BoolValue(q.KMS), DeadLetter: deadletter, FIFO: convertFIFO(q.FIFO), AccountID: accountID, @@ -765,7 +764,7 @@ func convertDeadLetter(d *manifest.DeadLetterQueue) (*template.DeadLetterQueue, } return &template.DeadLetterQueue{ - ID: d.ID, + Name: d.Name, Tries: d.Tries, }, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 71c1673ab2d..ede2c5d8c8e 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1604,10 +1604,9 @@ func Test_convertSubscribe(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &manifest.DeadLetterQueue{ Tries: aws.Uint16(35), - ID: aws.String("deadLetter"), + Name: aws.String("deadLetter"), }, FIFO: &manifest.FIFOOrBool{ Enabled: aws.Bool(true), @@ -1629,10 +1628,9 @@ func Test_convertSubscribe(t *testing.T) { Retention: aws.Int64(111), Delay: aws.Int64(111), Timeout: aws.Int64(111), - KMS: true, DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), - ID: aws.String("deadLetter"), + Name: aws.String("deadLetter"), }, FIFO: &template.FIFOQueue{ HighThroughput: false, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 8e8b1a21ccb..4242f0374f3 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -68,14 +68,13 @@ type SQSQueue struct { Retention *time.Duration `yaml:"retention"` Delay *time.Duration `yaml:"delay"` Timeout *time.Duration `yaml:"timeout"` - KMS *bool `yaml:"kms"` DeadLetter *DeadLetterQueue `yaml:"dead_letter"` FIFO *FIFOOrBool `yaml:"fifo"` } // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { - ID *string `yaml:"queue_id"` + Name *string `yaml:"name"` Tries *uint16 `yaml:"tries"` } diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 926aa5624b6..ac44dcb7877 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -427,7 +427,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -461,7 +460,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -487,7 +485,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -501,13 +498,77 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, } - mockWorkerServiceWithSubscribeEmptyOverride := WorkerService{ + mockWorkerServiceWithSubscribeTopicNilOverride := WorkerService{ Workload: Workload{ Name: aws.String("phonetool"), Type: aws.String(WorkerServiceType), }, WorkerServiceConfig: WorkerServiceConfig{ - Subscribe: &SubscribeConfig{}, + Subscribe: &SubscribeConfig{ + Topics: nil, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + } + mockWorkerServiceWithNilSubscribeTopicOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Topics: nil, + }, + }, + }, + } + mockWorkerServiceWithSubscribeTopicEmptyOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{}, + }, }, Environments: map[string]*WorkerServiceConfig{ "test-sub": { @@ -521,7 +582,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -713,7 +773,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -744,7 +803,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -756,7 +814,7 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { original: &mockWorkerServiceWithSubscribeNilOverride, }, - "with empty subscribe overriden by full subscribe": { + "with full subscribe and empty subscribe env": { svc: &mockWorkerServiceWithEmptySubscribeOverride, inEnvName: "test-sub", wanted: &WorkerService{ @@ -778,7 +836,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -790,8 +847,38 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { original: &mockWorkerServiceWithEmptySubscribeOverride, }, - "with full subscribe and empty subscribe env": { - svc: &mockWorkerServiceWithSubscribeEmptyOverride, + "with nil subscribe topic overriden by full subscribe topic": { + svc: &mockWorkerServiceWithNilSubscribeTopicOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithNilSubscribeTopicOverride, + }, + "with full subscribe topic and nil subscribe topic env": { + svc: &mockWorkerServiceWithSubscribeTopicNilOverride, inEnvName: "test-sub", wanted: &WorkerService{ Workload: Workload{ @@ -809,7 +896,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, - KMS: aws.Bool(true), DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, }, @@ -819,7 +905,40 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, - original: &mockWorkerServiceWithSubscribeEmptyOverride, + original: &mockWorkerServiceWithSubscribeTopicNilOverride, + }, + "with empty subscribe topic overriden by full subscribe topic": { + svc: &mockWorkerServiceWithSubscribeTopicEmptyOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + ImageConfig: ImageWithHealthcheck{ + Image: Image{}, + }, + Subscribe: &SubscribeConfig{ + Topics: &[]TopicSubscription{ + { + Name: "name", + Service: "svc", + Queue: &SQSQueue{ + Name: aws.String("queue"), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + }, + }, + + original: &mockWorkerServiceWithSubscribeTopicEmptyOverride, }, } diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 90e99de37ab..ff4da763a74 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -255,7 +255,6 @@ type SQSQueue struct { Retention *int64 Delay *int64 Timeout *int64 - KMS bool DeadLetter *DeadLetterQueue FIFO *FIFOQueue @@ -268,7 +267,7 @@ type SQSQueue struct { // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. type DeadLetterQueue struct { - ID *string + Name *string Tries *uint16 } @@ -477,7 +476,7 @@ func (t Topic) ARN() string { return fmt.Sprintf(snsARNPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) } -// URI determines the uri for a queue using the queue name, topic name, and account information +// URI determines the uri for a queue using the queue name or topic name, and account information func (q SQSQueue) URI(topicName *string) string { var name string if q.Name != nil { From eb76c6890fcfb3a98559e4511aa70331ec627bce Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Mon, 26 Jul 2021 16:46:19 -0400 Subject: [PATCH 15/17] removes account info and names from queues removes act info from queues due to new json generation removes names from queues to prevent replacement errors --- .../cloudformation/stack/transformers.go | 7 ------- .../cloudformation/stack/transformers_test.go | 9 -------- internal/pkg/manifest/worker_svc.go | 2 -- internal/pkg/manifest/worker_svc_test.go | 12 ----------- internal/pkg/template/workload.go | 21 ------------------- 5 files changed, 51 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index ecc0001efa1..30e2c490d8e 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -707,17 +707,11 @@ func convertQueue(q *manifest.SQSQueue, url, accountID, app, env, svc string) (* } return &template.SQSQueue{ - Name: q.Name, Retention: retention, Delay: delay, Timeout: timeout, DeadLetter: deadletter, FIFO: convertFIFO(q.FIFO), - AccountID: accountID, - URL: url, - App: app, - Env: env, - Svc: svc, }, nil } @@ -764,7 +758,6 @@ func convertDeadLetter(d *manifest.DeadLetterQueue) (*template.DeadLetterQueue, } return &template.DeadLetterQueue{ - Name: d.Name, Tries: d.Tries, }, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index ede2c5d8c8e..fa29a5d87ff 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1600,13 +1600,11 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &manifest.SQSQueue{ - Name: aws.String("bestqueue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: &manifest.DeadLetterQueue{ Tries: aws.Uint16(35), - Name: aws.String("deadLetter"), }, FIFO: &manifest.FIFOOrBool{ Enabled: aws.Bool(true), @@ -1624,22 +1622,15 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Name: aws.String("bestqueue"), Retention: aws.Int64(111), Delay: aws.Int64(111), Timeout: aws.Int64(111), DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), - Name: aws.String("deadLetter"), }, FIFO: &template.FIFOQueue{ HighThroughput: false, }, - AccountID: accountId, - URL: "https://sqs.us-west-2.amazonaws.com", - App: app, - Env: env, - Svc: svc, }, }, }, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 4242f0374f3..9402a004466 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -64,7 +64,6 @@ type TopicSubscription struct { // SQSQueue represents the configurable options for setting up a SQS Queue. type SQSQueue struct { - Name *string `yaml:"name"` Retention *time.Duration `yaml:"retention"` Delay *time.Duration `yaml:"delay"` Timeout *time.Duration `yaml:"timeout"` @@ -74,7 +73,6 @@ type SQSQueue struct { // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { - Name *string `yaml:"name"` Tries *uint16 `yaml:"tries"` } diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index ac44dcb7877..f51ef6be4f2 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -423,7 +423,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -456,7 +455,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -481,7 +479,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -516,7 +513,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -541,7 +537,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -578,7 +573,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -769,7 +763,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -799,7 +792,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -832,7 +824,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -862,7 +853,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -892,7 +882,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, @@ -925,7 +914,6 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { Name: "name", Service: "svc", Queue: &SQSQueue{ - Name: aws.String("queue"), Retention: &duration111Seconds, Delay: &duration111Seconds, Timeout: &duration111Seconds, diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index ff4da763a74..a96e58e6424 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -251,23 +251,15 @@ type TopicSubscription struct { // SQSQueue holds information needed to render a SQS Queue in a container definition. type SQSQueue struct { - Name *string Retention *int64 Delay *int64 Timeout *int64 DeadLetter *DeadLetterQueue FIFO *FIFOQueue - - URL string - AccountID string - App string - Env string - Svc string } // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. type DeadLetterQueue struct { - Name *string Tries *uint16 } @@ -475,16 +467,3 @@ func envControllerParameters(o WorkloadOpts) []string { func (t Topic) ARN() string { return fmt.Sprintf(snsARNPattern, t.Partition, t.Region, t.AccountID, t.App, t.Env, t.Svc, aws.StringValue(t.Name)) } - -// URI determines the uri for a queue using the queue name or topic name, and account information -func (q SQSQueue) URI(topicName *string) string { - var name string - if q.Name != nil { - name = aws.StringValue(q.Name) - } else if topicName != nil { - name = aws.StringValue(topicName) - } else { - name = SQSQueueName - } - return fmt.Sprintf(sqsURIPattern, q.URL, q.AccountID, q.App, q.Env, q.Svc, name) -} From d4c9b9e1faf4c264fe331f098e11a4b2fa1b28e4 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Mon, 26 Jul 2021 16:59:11 -0400 Subject: [PATCH 16/17] removes unneeded uri pattern --- internal/pkg/template/workload.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index a96e58e6424..b3d339dc9b2 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -39,15 +39,11 @@ const ( DisablePublicIP = "DISABLED" PublicSubnetsPlacement = "PublicSubnets" PrivateSubnetsPlacement = "PrivateSubnets" - - // SQSQueue Configurations. - SQSQueueName = "SQSQueue" ) // Constants for ARN options. const ( snsARNPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" - sqsURIPattern = "%s/%s/%s-%s-%s-%s" ) var ( From 72c6d8c35b64a21a3a38a1eab404d943f85347e3 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Tue, 27 Jul 2021 10:26:11 -0400 Subject: [PATCH 17/17] adds tests for queues --- .../cloudformation/stack/transformers_test.go | 20 +++ internal/pkg/manifest/worker_svc_test.go | 144 ++++++++++++++++++ 2 files changed, 164 insertions(+) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index fa29a5d87ff..15b057971e5 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1634,6 +1634,26 @@ func Test_convertSubscribe(t *testing.T) { }, }, }, + "valid subscribe with minimal queue": { + inSubscribe: &manifest.SubscribeConfig{ + Topics: &[]manifest.TopicSubscription{ + { + Name: "name", + Service: "svc", + }, + }, + Queue: &manifest.SQSQueue{}, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: &template.SQSQueue{}, + }, + }, "invalid topic name": { inSubscribe: &manifest.SubscribeConfig{ Topics: &[]manifest.TopicSubscription{ diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index f51ef6be4f2..8959207f23f 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -585,6 +585,78 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { }, }, } + mockWorkerServiceWithSubscribeQueueNilOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Queue: nil, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + } + mockWorkerServiceWithNilSubscribeQueueOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Queue: nil, + }, + }, + }, + } + mockWorkerServiceWithSubscribeQueueEmptyOverride := WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{}, + }, + }, + Environments: map[string]*WorkerServiceConfig{ + "test-sub": { + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + } testCases := map[string]struct { svc *WorkerService inEnvName string @@ -928,6 +1000,78 @@ func TestWorkerSvc_ApplyEnv(t *testing.T) { original: &mockWorkerServiceWithSubscribeTopicEmptyOverride, }, + "with nil subscribe queue overriden by full subscribe queue": { + svc: &mockWorkerServiceWithNilSubscribeQueueOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + + original: &mockWorkerServiceWithNilSubscribeQueueOverride, + }, + "with full subscribe queue and nil subscribe queue env": { + svc: &mockWorkerServiceWithSubscribeQueueNilOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + + original: &mockWorkerServiceWithSubscribeQueueNilOverride, + }, + "with empty subscribe queue overriden by full subscribe queue": { + svc: &mockWorkerServiceWithSubscribeQueueEmptyOverride, + inEnvName: "test-sub", + wanted: &WorkerService{ + Workload: Workload{ + Name: aws.String("phonetool"), + Type: aws.String(WorkerServiceType), + }, + WorkerServiceConfig: WorkerServiceConfig{ + ImageConfig: ImageWithHealthcheck{ + Image: Image{}, + }, + Subscribe: &SubscribeConfig{ + Queue: &SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: &DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: &FIFOOrBool{Enabled: aws.Bool(true)}, + }, + }, + }, + }, + + original: &mockWorkerServiceWithSubscribeQueueEmptyOverride, + }, } for name, tc := range testCases {