From a2d98de3460f32807f8f47b69c20ec9dc9444285 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 9 Sep 2022 16:42:54 -0700 Subject: [PATCH 01/19] feat: add support for FIFO SQS Queue --- .../cloudformation/stack/transformers.go | 29 +++- internal/pkg/manifest/validate.go | 56 ++++++-- internal/pkg/manifest/validate_test.go | 133 +++++++++++++++++- internal/pkg/manifest/worker_svc.go | 24 +++- internal/pkg/manifest/worker_svc_test.go | 17 +++ internal/pkg/template/template_functions.go | 5 + internal/pkg/template/workload.go | 13 +- 7 files changed, 254 insertions(+), 23 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 975541df955..bbee34bcc1e 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -823,7 +823,7 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro } subscriptions.Topics = append(subscriptions.Topics, ts) } - subscriptions.Queue = convertQueue(s.Queue) + subscriptions.Queue = convertQueue(s.Queue, false) return &subscriptions, nil } @@ -841,10 +841,16 @@ func convertTopicSubscription(t manifest.TopicSubscription) ( FilterPolicy: filterPolicy, }, nil } + var queueAdvacnedConfig *template.SQSQueue + if template.IsFIFOFunc(aws.StringValue(t.Name)) { + queueAdvacnedConfig = convertQueue(t.Queue.Advanced, true) + } else { + queueAdvacnedConfig = convertQueue(t.Queue.Advanced, false) + } return &template.TopicSubscription{ Name: t.Name, Service: t.Service, - Queue: convertQueue(t.Queue.Advanced), + Queue: queueAdvacnedConfig, FilterPolicy: filterPolicy, }, nil } @@ -860,7 +866,24 @@ func convertFilterPolicy(filterPolicy map[string]interface{}) (*string, error) { return aws.String(string(bytes)), nil } -func convertQueue(q manifest.SQSQueue) *template.SQSQueue { +func convertQueue(q manifest.SQSQueue, isFIFO bool) *template.SQSQueue { + + if isFIFO { + if q.IsFIFOEmpty() { + return nil + } + return &template.SQSQueue{ + Retention: convertRetention(q.Retention), + Delay: convertDelay(q.Delay), + Timeout: convertTimeout(q.Timeout), + DeadLetter: convertDeadLetter(q.DeadLetter), + FifoThroughputLimit: aws.StringValue(q.FifoThroughputLimit), + HighThroughputFifo: aws.BoolValue(q.HighThroughputFifo), + ContentBasedDeduplication: aws.BoolValue(q.ContentBasedDeduplication), + DeduplicationScope: aws.StringValue(q.DeduplicationScope), + } + } + if q.IsEmpty() { return nil } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 71dbb90b17b..6b35efb86dc 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -6,6 +6,7 @@ package manifest import ( "errors" "fmt" + "github.com/aws/copilot-cli/internal/pkg/template" "net" "path/filepath" "regexp" @@ -44,10 +45,10 @@ const ( var ( intRangeBandRegexp = regexp.MustCompile(`^(\d+)-(\d+)$`) volumesPathRegexp = regexp.MustCompile(`^[a-zA-Z0-9\-\.\_/]+$`) - 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. - punctuationRegExp = regexp.MustCompile(`[\.\-]{2,}`) // Check for consecutive periods or dashes. - trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. + awsSNSTopicRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]*(\.fifo)?$`) // Validates that an expression contains only letters, numbers, underscores, hyphens, and .fifo suffix. + awsNameRegexp = regexp.MustCompile(`^[a-z][a-z0-9\-]+$`) // Validates that an expression starts with a letter and only contains letters, numbers, and hyphens. + punctuationRegExp = regexp.MustCompile(`[\.\-]{2,}`) // Check for consecutive periods or dashes. + trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. essentialContainerDependsOnValidStatuses = []string{dependsOnStart, dependsOnHealthy} dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} @@ -58,6 +59,8 @@ var ( httpProtocolVersions = []string{"GRPC", "HTTP1", "HTTP2"} invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} + validDeduplicationScopeValues = []string{"messageGroup", "queue"} + validFIFOThroughputLimitValues = []string{"perMessageGroupId", "perQueue"} ) // Validate returns nil if DynamicLoadBalancedWebService is configured correctly. @@ -1444,18 +1447,31 @@ func (t TopicSubscription) validate() error { if !isValidSubSvcName(svcName) { return fmt.Errorf("service name must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") } - if err := t.Queue.validate(); err != nil { + + if err := t.Queue.validate(template.IsFIFOFunc(aws.StringValue(t.Name))); err != nil { return fmt.Errorf(`validate "queue": %w`, err) } return nil } // validate returns nil if SQSQueue is configured correctly. -func (q SQSQueueOrBool) validate() error { - if q.IsEmpty() { - return nil +func (q SQSQueueOrBool) validate(isFIFO bool) error { + + if !isFIFO { + if q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || + q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil { + return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`) + } + if q.IsEmpty() { + return nil + } + return q.Advanced.validate() } - return q.Advanced.validate() + + if q.IsFIFOEmpty() { + return errors.New(`FIFO SQS Queue configuration is required, you can either set "queue: true" or complete configuration alternatively`) + } + return q.Advanced.validateFIFO() } // validate returns nil if SQSQueue is configured correctly. @@ -1469,6 +1485,24 @@ func (q SQSQueue) validate() error { return nil } +// validateFIFO returns nil if FIFO SQSQueue is configured correctly. +func (q SQSQueue) validateFIFO() error { + if q.IsFIFOEmpty() { + return nil + } + + if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { + return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) + } + if q.FifoThroughputLimit != nil && !contains(aws.StringValue(q.FifoThroughputLimit), validFIFOThroughputLimitValues) { + return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) + } + if err := q.DeadLetter.validate(); err != nil { + return fmt.Errorf(`validate "dead_letter": %w`, err) + } + return nil +} + // validate returns nil if DeadLetterQueue is configured correctly. func (d DeadLetterQueue) validate() error { if d.IsEmpty() { @@ -1634,9 +1668,9 @@ func validatePubSubName(name string) error { missingField: "name", } } - // Name must contain letters, numbers, and can't use special characters besides underscores and hyphens. + // Name must contain letters, numbers, and can't use special characters besides underscores, hyphens, and .fifo suffix. if !awsSNSTopicRegexp.MatchString(name) { - return fmt.Errorf(`"name" can only contain letters, numbers, underscores, and hypthens`) + return fmt.Errorf(`"name" can only contain letters, numbers, underscores, hyphens, and .fifo suffix`) } return nil } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index f98b3a841f8..d4478906f5e 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2665,7 +2665,7 @@ func TestTopic_validate(t *testing.T) { in: Topic{ Name: aws.String("!@#"), }, - wanted: errors.New(`"name" can only contain letters, numbers, underscores, and hypthens`), + wanted: errors.New(`"name" can only contain letters, numbers, underscores, hyphens, and .fifo suffix`), }, } for name, tc := range testCases { @@ -2697,6 +2697,17 @@ func TestSubscribeConfig_validate(t *testing.T) { }, wantedErrorPrefix: `validate "topics[0]": `, }, + /*"error if fail to validate topics1": { + config: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("mocktopic"), + Service: aws.String("mockservice"), + }, + }, + }, + wantedErrorPrefix: `validate "topics[0]": `, + },*/ } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -2712,6 +2723,7 @@ func TestSubscribeConfig_validate(t *testing.T) { } func TestTopicSubscription_validate(t *testing.T) { + duration111Seconds := 111 * time.Second testCases := map[string]struct { in TopicSubscription wanted error @@ -2733,6 +2745,125 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: errors.New("service name must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen"), }, + "should not return an error if service is in valid format": { + in: TopicSubscription{ + Name: aws.String("mockTopic"), + Service: aws.String("mockservice"), + }, + wanted: nil, + }, + "should return an error if fifo queue config is missing": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + }, + wanted: errors.New(`validate "queue": FIFO SQS Queue configuration is required, you can either set "queue: true" or complete configuration alternatively`), + }, + "should not return error if fifo queue enabled": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Enabled: aws.Bool(true), + }, + }, + wanted: nil, + }, + "should return error if invalid fifo throughput limit": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FifoThroughputLimit: aws.String("incorrectFIFOThoughoutLimit"), + }, + }, + }, + wanted: errors.New(`validate "queue": validate "fifo_throughput_limit": fifo throughput limit value must be one of perMessageGroupId or perQueue`), + }, + "should not return error if valid fifo throughput limit": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FifoThroughputLimit: aws.String("perMessageGroupId"), + }, + }, + }, + wanted: nil, + }, + "should return error if invalid deduplicate scope": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + DeduplicationScope: aws.String("incorrectDeduplicateScope"), + }, + }, + }, + wanted: errors.New(`validate "queue": validate "deduplication_scope": deduplication scope value must be one of messageGroup or queue`), + }, + "should not return error if valid deduplicate scope": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + DeduplicationScope: aws.String("messageGroup"), + }, + }, + }, + wanted: nil, + }, + "should return error if standard queue config has fifo queue parameters defined": { + in: TopicSubscription{ + Name: aws.String("mockTopic"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + DeduplicationScope: aws.String("messageGroup"), + }, + }, + }, + wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`), + }, + "should not return error if valid standard queue config defined": { + in: TopicSubscription{ + Name: aws.String("mockTopic"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + }, + }, + }, + wanted: nil, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 2e588faf9bb..5c1438424ed 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -86,6 +86,11 @@ func (q *SQSQueueOrBool) IsEmpty() bool { return q.Advanced.IsEmpty() && q.Enabled == nil } +// IsFIFOEmpty returns empty if the struct has all zero members. +func (q *SQSQueueOrBool) IsFIFOEmpty() bool { + return q.Advanced.IsFIFOEmpty() && q.Enabled == nil +} + // UnmarshalYAML implements the yaml(v3) interface. It allows SQSQueueOrBool to be specified as a // string or a struct alternately. func (q *SQSQueueOrBool) UnmarshalYAML(value *yaml.Node) error { @@ -110,10 +115,14 @@ func (q *SQSQueueOrBool) UnmarshalYAML(value *yaml.Node) error { // SQSQueue represents the configurable options for setting up a SQS Queue. type SQSQueue struct { - Retention *time.Duration `yaml:"retention"` - Delay *time.Duration `yaml:"delay"` - Timeout *time.Duration `yaml:"timeout"` - DeadLetter DeadLetterQueue `yaml:"dead_letter"` + Retention *time.Duration `yaml:"retention"` + Delay *time.Duration `yaml:"delay"` + Timeout *time.Duration `yaml:"timeout"` + DeadLetter DeadLetterQueue `yaml:"dead_letter"` + ContentBasedDeduplication *bool `yaml:"content_based_deduplication"` + DeduplicationScope *string `yaml:"deduplication_scope"` + FifoThroughputLimit *string `yaml:"fifo_throughput_limit"` + HighThroughputFifo *bool `yaml:"high_throughput_fifo"` } // IsEmpty returns empty if the struct has all zero members. @@ -122,6 +131,13 @@ func (q *SQSQueue) IsEmpty() bool { q.DeadLetter.IsEmpty() } +// IsFIFOEmpty returns empty if the struct has all zero members. +func (q *SQSQueue) IsFIFOEmpty() bool { + return q.Retention == nil && q.Delay == nil && q.Timeout == nil && + q.DeadLetter.IsEmpty() && q.FifoThroughputLimit == nil && q.HighThroughputFifo == nil && + q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil +} + // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { Tries *uint16 `yaml:"tries"` diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 65a52deda71..bd2e9fe8698 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -29,6 +29,23 @@ func newMockSQSQueue() SQSQueue { } } +func newMockSQSFIFOQueueOrBool() SQSQueueOrBool { + return SQSQueueOrBool{ + Advanced: newMockSQSFIFOQueue(), + } +} + +func newMockSQSFIFOQueue() SQSQueue { + duration111Seconds := 111 * time.Second + return SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FifoThroughputLimit: aws.String("asd"), + } +} + func TestNewWorkerSvc(t *testing.T) { testCases := map[string]struct { inProps WorkerServiceProps diff --git a/internal/pkg/template/template_functions.go b/internal/pkg/template/template_functions.go index 8b5e7622000..8dd93881b07 100644 --- a/internal/pkg/template/template_functions.go +++ b/internal/pkg/template/template_functions.go @@ -30,6 +30,11 @@ func ReplaceDashesFunc(logicalID string) string { return strings.ReplaceAll(logicalID, "-", dashReplacement) } +// IsFIFOFunc takes a string value and determines if has .fifo suffix. +func IsFIFOFunc(value string) bool { + return strings.Contains(value, ".fifo") +} + // IsArnFunc takes a string value and determines if it's an ARN or not. func IsARNFunc(value string) bool { return arn.IsARN(value) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 49b221409f3..fb0ee00c7c9 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -424,10 +424,14 @@ type TopicSubscription struct { // SQSQueue holds information needed to render a SQS Queue in a container definition. type SQSQueue struct { - Retention *int64 - Delay *int64 - Timeout *int64 - DeadLetter *DeadLetterQueue + Retention *int64 + Delay *int64 + Timeout *int64 + DeadLetter *DeadLetterQueue + FifoThroughputLimit string + HighThroughputFifo bool + ContentBasedDeduplication bool + DeduplicationScope string } // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. @@ -623,6 +627,7 @@ func withSvcParsingFuncs() ParseOption { "jsonQueueURIs": generateQueueURIJSON, "envControllerParams": envControllerParameters, "logicalIDSafe": StripNonAlphaNumFunc, + "isFIFO": IsFIFOFunc, "wordSeries": english.WordSeries, "pluralWord": english.PluralWord, "contains": contains, From 9880cc9062da0ab4cee9967c60cc5273a112253a Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 13 Sep 2022 15:12:56 -0700 Subject: [PATCH 02/19] cloudformation and validation changes --- .../cloudformation/stack/transformers.go | 22 ++- .../cloudformation/stack/transformers_test.go | 132 ++++++++++++++++++ .../deploy/cloudformation/stack/worker_svc.go | 1 + internal/pkg/manifest/validate.go | 6 + internal/pkg/manifest/validate_test.go | 34 +++++ .../workloads/partials/cf/subscribe.yml | 39 +++++- internal/pkg/template/workload.go | 22 ++- 7 files changed, 240 insertions(+), 16 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index bbee34bcc1e..d094a481c31 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -824,6 +824,7 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro subscriptions.Topics = append(subscriptions.Topics, ts) } subscriptions.Queue = convertQueue(s.Queue, false) + subscriptions.StandardQueue = subscriptions.StandardDefaultQueue() return &subscriptions, nil } @@ -872,15 +873,28 @@ func convertQueue(q manifest.SQSQueue, isFIFO bool) *template.SQSQueue { if q.IsFIFOEmpty() { return nil } + var fifoThroughputLimit, deduplicationScope *string + + if aws.BoolValue(q.HighThroughputFifo) != true { + if q.FifoThroughputLimit != nil { + fifoThroughputLimit = q.FifoThroughputLimit + } + if q.DeduplicationScope != nil { + deduplicationScope = q.DeduplicationScope + } + } else if aws.BoolValue(q.HighThroughputFifo) == true { + fifoThroughputLimit = aws.String("messageGroup") + deduplicationScope = aws.String("perMessageGroupId") + } + return &template.SQSQueue{ Retention: convertRetention(q.Retention), Delay: convertDelay(q.Delay), Timeout: convertTimeout(q.Timeout), DeadLetter: convertDeadLetter(q.DeadLetter), - FifoThroughputLimit: aws.StringValue(q.FifoThroughputLimit), - HighThroughputFifo: aws.BoolValue(q.HighThroughputFifo), - ContentBasedDeduplication: aws.BoolValue(q.ContentBasedDeduplication), - DeduplicationScope: aws.StringValue(q.DeduplicationScope), + FifoThroughputLimit: fifoThroughputLimit, + ContentBasedDeduplication: q.ContentBasedDeduplication, + DeduplicationScope: deduplicationScope, } } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index b4b56de89e3..dafdc0c0ec0 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1490,6 +1490,138 @@ func Test_convertSubscribe(t *testing.T) { Queue: nil, }, }, + "valid subscribe with high throughput fifo sqs": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + HighThroughputFifo: aws.Bool(true), + }, + }, + FilterPolicy: mockStruct, + }, + }, + Queue: manifest.SQSQueue{}, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("messageGroup"), + DeduplicationScope: aws.String("perMessageGroupId"), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + }, + Queue: nil, + }, + }, + "valid subscribe with default fifo sqs config values": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + }, + }, + FilterPolicy: mockStruct, + }, + }, + Queue: manifest.SQSQueue{}, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: nil, + DeduplicationScope: nil, + ContentBasedDeduplication: nil, + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + }, + Queue: nil, + }, + }, + "valid subscribe with custom fifo sqs config values": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, + FilterPolicy: mockStruct, + }, + }, + Queue: manifest.SQSQueue{}, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + }, + Queue: nil, + }, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index b06d20db95e..15c0a4b434a 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -153,6 +153,7 @@ func (s *WorkerService) Template() (string, error) { if err != nil { return "", fmt.Errorf("apply task definition overrides: %w", err) } + fmt.Println(string(overridenTpl)) return string(overridenTpl), nil } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 6b35efb86dc..9e6026efad5 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1491,12 +1491,18 @@ func (q SQSQueue) validateFIFO() error { return nil } + if q.HighThroughputFifo != nil && (q.FifoThroughputLimit != nil || q.DeduplicationScope != nil) { + return fmt.Errorf(`validate "high_throughput_fifo": cannot define fifo_throughput_limit and fifo_throughput_limit when high_throughput_fifo is defined`) + } if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) } if q.FifoThroughputLimit != nil && !contains(aws.StringValue(q.FifoThroughputLimit), validFIFOThroughputLimitValues) { return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) } + if q.FifoThroughputLimit != nil && strings.Compare(aws.StringValue(q.FifoThroughputLimit), "perMessageGroupId") == 0 && q.DeduplicationScope != nil && strings.Compare(aws.StringValue(q.DeduplicationScope), "queue") == 0 { + return fmt.Errorf(`when Deduplication scope is set to Queue, FIFO throughput limit must be set to Per Queue`) + } if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index d4478906f5e..78dbdf383d8 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2849,6 +2849,40 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`), }, + "should return error if high_throughput_fifo is defined along with deduplication_scope or fifo_throughput_limit": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + HighThroughputFifo: aws.Bool(true), + DeduplicationScope: aws.String("messageGroup"), + }, + }, + }, + wanted: errors.New(`validate "queue": validate "high_throughput_fifo": cannot define fifo_throughput_limit and fifo_throughput_limit when high_throughput_fifo is defined`), + }, + "should return error if invalid combination of fifo_throughput_limit and fifo_throughput_limit is defined": { + in: TopicSubscription{ + Name: aws.String("mockTopic.fifo"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FifoThroughputLimit: aws.String("perMessageGroupId"), + DeduplicationScope: aws.String("queue"), + }, + }, + }, + wanted: errors.New(`validate "queue": when Deduplication scope is set to Queue, FIFO throughput limit must be set to Per Queue`), + }, "should not return error if valid standard queue config defined": { in: TopicSubscription{ Name: aws.String("mockTopic"), diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index 1d5bb3ec79a..d013e67fb78 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -58,6 +58,7 @@ EventsKMSKey: - "kms:Decrypt" Resource: '*' +{{- if .Subscribe.StandardQueue}} EventsQueue: Metadata: 'aws:copilot:description': 'An events SQS queue to buffer messages' @@ -108,8 +109,11 @@ DeadLetterPolicy: Resource: !GetAtt DeadLetterQueue.Arn {{- end}}{{- end}} -{{- end }}{{/* endif .Subscribe */}} +{{- end}}{{/* endif .Subscribe */}} +{{- end}}{{/* endif .Subscribe.StandardQueue */}} + +{{- if .Subscribe.StandardQueue}} QueuePolicy: Type: AWS::SQS::QueuePolicy Properties: @@ -119,19 +123,20 @@ QueuePolicy: Statement: - Effect: Allow Principal: - AWS: + AWS: - !GetAtt TaskRole.Arn - Action: + Action: - sqs:ReceiveMessage - sqs:DeleteMessage Resource: !GetAtt EventsQueue.Arn {{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} {{- if not $topic.Queue}} + {{- if not (isFIFO $topic.Name)}} - Effect: Allow Principal: Service: sns.amazonaws.com - Action: + Action: - sqs:SendMessage Resource: !GetAtt EventsQueue.Arn Condition: @@ -139,7 +144,11 @@ QueuePolicy: aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{$topic.Name}}']] {{- end}} {{- end}} + {{- end}} +{{- end}}{{/* if .Subscribe */}} +{{- end}}{{/* endif .Subscribe.StandardQueue */}} +{{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}SNSTopicSubscription: Metadata: @@ -178,6 +187,18 @@ QueuePolicy: deadLetterTargetArn: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue.Arn maxReceiveCount: {{$topic.Queue.DeadLetter.Tries}} {{- end}} + {{- if isFIFO $topic.Name }} + FifoQueue: true + {{- end}} + {{- if $topic.Queue.FifoThroughputLimit }} + FifoThroughputLimit: {{$topic.Queue.FifoThroughputLimit}} + {{- end}} + {{- if $topic.Queue.DeduplicationScope }} + DeduplicationScope: {{$topic.Queue.DeduplicationScope}} + {{- end }} + {{- if $topic.Queue.ContentBasedDeduplication }} + ContentBasedDeduplication: {{$topic.Queue.ContentBasedDeduplication}} + {{- end}} {{- if $topic.Queue.DeadLetter}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue: @@ -187,6 +208,10 @@ QueuePolicy: Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days + {{- if isFIFO $topic.Name }} + FifoQueue: true + {{- end}} + {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterPolicy: Type: AWS::SQS::QueuePolicy @@ -203,7 +228,7 @@ QueuePolicy: - sqs:ReceiveMessage - sqs:DeleteMessage Resource: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue.Arn -{{- end}} +{{- end}} {{/* endif $topic.Queue.DeadLetter */}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}QueuePolicy: Type: AWS::SQS::QueuePolicy @@ -229,4 +254,6 @@ QueuePolicy: Condition: ArnEquals: aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] -{{- end}}{{- end}}{{- end}} \ No newline at end of file +{{- end}}{{/* endif $topic.Queue =*/}} +{{- end}}{{/* endrange $topic := .Subscribe.Topics */}} +{{- end}}{{/* if .Subscribe */}} \ No newline at end of file diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index fb0ee00c7c9..a47ae50493b 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -400,8 +400,9 @@ type Topic struct { // SubscribeOpts holds configuration needed if the service has subscriptions. type SubscribeOpts struct { - Topics []*TopicSubscription - Queue *SQSQueue + Topics []*TopicSubscription + Queue *SQSQueue + StandardQueue bool } // HasTopicQueues returns true if any individual subscription has a dedicated queue. @@ -414,6 +415,16 @@ func (s *SubscribeOpts) HasTopicQueues() bool { return false } +// StandardDefaultQueue returns true if at least one standard topic exist and does not have its own queue configs. +func (s *SubscribeOpts) StandardDefaultQueue() bool { + for _, t := range s.Topics { + if !IsFIFOFunc(aws.StringValue(t.Name)) { + return true + } + } + return false +} + // TopicSubscription holds information needed to render a SNS Topic Subscription in a container definition. type TopicSubscription struct { Name *string @@ -428,10 +439,9 @@ type SQSQueue struct { Delay *int64 Timeout *int64 DeadLetter *DeadLetterQueue - FifoThroughputLimit string - HighThroughputFifo bool - ContentBasedDeduplication bool - DeduplicationScope string + FifoThroughputLimit *string + ContentBasedDeduplication *bool + DeduplicationScope *string } // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. From bd7d33125ce463ad41a2ac8b8b7f077cebc6642b Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 14 Sep 2022 23:17:12 -0700 Subject: [PATCH 03/19] implement type field logic fir FIFO Queue --- internal/pkg/cli/deploy/svc.go | 1 + .../cloudformation/stack/transformers.go | 93 +++++++++++-------- .../cloudformation/stack/transformers_test.go | 54 ++++++++--- internal/pkg/manifest/validate.go | 41 ++++---- internal/pkg/manifest/validate_test.go | 17 ++-- internal/pkg/manifest/worker_svc.go | 24 ++++- internal/pkg/template/template_functions.go | 3 + .../workloads/partials/cf/subscribe.yml | 34 ++++--- internal/pkg/template/workload.go | 10 +- 9 files changed, 177 insertions(+), 100 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index 02e4fd12926..5a16e324d98 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1174,6 +1174,7 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* for _, topic := range topics { topicARNs = append(topicARNs, topic.ARN()) } + d.wsMft.AttachFIFOSuffixToFIFOQueues() subs := d.wsMft.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { return nil, err diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index d094a481c31..5b20c17cb99 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -25,8 +25,10 @@ import ( ) const ( - enabled = "ENABLED" - disabled = "DISABLED" + enabled = "ENABLED" + disabled = "DISABLED" + defaultQueueType = "standard" + fifoQueue = "fifo" ) // Default values for EFS options @@ -823,8 +825,12 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro } subscriptions.Topics = append(subscriptions.Topics, ts) } - subscriptions.Queue = convertQueue(s.Queue, false) - subscriptions.StandardQueue = subscriptions.StandardDefaultQueue() + subscriptions.Queue = convertQueue(s.Queue) + if subscriptions.Queue == nil && subscriptions.StandardDefaultQueue() { + subscriptions.QueueType = aws.String(defaultQueueType) + } else if strings.Compare(aws.StringValue(subscriptions.Queue.Type), "fifo") == 0 { + subscriptions.QueueType = aws.String(fifoQueue) + } return &subscriptions, nil } @@ -836,22 +842,20 @@ func convertTopicSubscription(t manifest.TopicSubscription) ( } if aws.BoolValue(t.Queue.Enabled) { return &template.TopicSubscription{ - Name: t.Name, - Service: t.Service, - Queue: &template.SQSQueue{}, + Name: t.Name, + Service: t.Service, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, FilterPolicy: filterPolicy, }, nil } - var queueAdvacnedConfig *template.SQSQueue - if template.IsFIFOFunc(aws.StringValue(t.Name)) { - queueAdvacnedConfig = convertQueue(t.Queue.Advanced, true) - } else { - queueAdvacnedConfig = convertQueue(t.Queue.Advanced, false) - } + queueAdvancedConfig := convertQueue(t.Queue.Advanced) + return &template.TopicSubscription{ Name: t.Name, Service: t.Service, - Queue: queueAdvacnedConfig, + Queue: queueAdvancedConfig, FilterPolicy: filterPolicy, }, nil } @@ -867,38 +871,46 @@ func convertFilterPolicy(filterPolicy map[string]interface{}) (*string, error) { return aws.String(string(bytes)), nil } -func convertQueue(q manifest.SQSQueue, isFIFO bool) *template.SQSQueue { +func convertQueue(q manifest.SQSQueue) *template.SQSQueue { + if q.IsEmpty() { + return nil + } - if isFIFO { - if q.IsFIFOEmpty() { - return nil + if strings.Compare(aws.StringValue(q.Type), defaultQueueType) == 0 { + return &template.SQSQueue{ + Retention: convertRetention(q.Retention), + Delay: convertDelay(q.Delay), + Timeout: convertTimeout(q.Timeout), + DeadLetter: convertDeadLetter(q.DeadLetter), + Type: aws.String(defaultQueueType), } - var fifoThroughputLimit, deduplicationScope *string + } - if aws.BoolValue(q.HighThroughputFifo) != true { - if q.FifoThroughputLimit != nil { - fifoThroughputLimit = q.FifoThroughputLimit - } - if q.DeduplicationScope != nil { - deduplicationScope = q.DeduplicationScope - } - } else if aws.BoolValue(q.HighThroughputFifo) == true { - fifoThroughputLimit = aws.String("messageGroup") - deduplicationScope = aws.String("perMessageGroupId") + var fifoThroughputLimit, deduplicationScope *string + if aws.BoolValue(q.HighThroughputFifo) != true { + if q.FifoThroughputLimit != nil { + fifoThroughputLimit = q.FifoThroughputLimit } - - return &template.SQSQueue{ - Retention: convertRetention(q.Retention), - Delay: convertDelay(q.Delay), - Timeout: convertTimeout(q.Timeout), - DeadLetter: convertDeadLetter(q.DeadLetter), - FifoThroughputLimit: fifoThroughputLimit, - ContentBasedDeduplication: q.ContentBasedDeduplication, - DeduplicationScope: deduplicationScope, + if q.DeduplicationScope != nil { + deduplicationScope = q.DeduplicationScope } + } else if aws.BoolValue(q.HighThroughputFifo) == true { + fifoThroughputLimit = aws.String("messageGroup") + deduplicationScope = aws.String("perMessageGroupId") } - if q.IsEmpty() { + return &template.SQSQueue{ + Retention: convertRetention(q.Retention), + Delay: convertDelay(q.Delay), + Timeout: convertTimeout(q.Timeout), + DeadLetter: convertDeadLetter(q.DeadLetter), + FifoThroughputLimit: fifoThroughputLimit, + ContentBasedDeduplication: q.ContentBasedDeduplication, + DeduplicationScope: deduplicationScope, + Type: q.Type, + } + + /*if q.IsEmpty() { return nil } return &template.SQSQueue{ @@ -906,7 +918,8 @@ func convertQueue(q manifest.SQSQueue, isFIFO bool) *template.SQSQueue { Delay: convertDelay(q.Delay), Timeout: convertTimeout(q.Timeout), DeadLetter: convertDeadLetter(q.DeadLetter), - } + Type: aws.String(defaultQueueType), + }*/ } func convertTime(t *time.Duration) *int64 { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index dafdc0c0ec0..a57f69af549 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1462,6 +1462,28 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, }, + StandardQueue: true, + }, + }, + "valid subscribe with no queue configs": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{}, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: nil, + StandardQueue: true, }, }, "valid subscribe with minimal queue": { @@ -1481,20 +1503,23 @@ func Test_convertSubscribe(t *testing.T) { wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: &template.SQSQueue{}, + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: nil, + StandardQueue: false, }, }, "valid subscribe with high throughput fifo sqs": { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: manifest.SQSQueueOrBool{ Advanced: manifest.SQSQueue{ @@ -1515,7 +1540,7 @@ func Test_convertSubscribe(t *testing.T) { wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: &template.SQSQueue{ Retention: aws.Int64(111), @@ -1530,14 +1555,15 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: nil, + StandardQueue: false, }, }, "valid subscribe with default fifo sqs config values": { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: manifest.SQSQueueOrBool{ Advanced: manifest.SQSQueue{ @@ -1547,6 +1573,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, + Type: aws.String("fifo"), }, }, FilterPolicy: mockStruct, @@ -1557,7 +1584,7 @@ func Test_convertSubscribe(t *testing.T) { wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: &template.SQSQueue{ Retention: aws.Int64(111), @@ -1569,11 +1596,13 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: nil, DeduplicationScope: nil, ContentBasedDeduplication: nil, + Type: aws.String("fifo"), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: nil, + StandardQueue: false, }, }, "valid subscribe with custom fifo sqs config values": { @@ -1593,6 +1622,7 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), + Type: aws.String("fifo"), }, }, FilterPolicy: mockStruct, @@ -1615,11 +1645,13 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), + Type: aws.String("fifo"), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: nil, + StandardQueue: false, }, }, } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 9e6026efad5..c6c81ba6b5a 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -6,7 +6,6 @@ package manifest import ( "errors" "fmt" - "github.com/aws/copilot-cli/internal/pkg/template" "net" "path/filepath" "regexp" @@ -45,10 +44,10 @@ const ( var ( intRangeBandRegexp = regexp.MustCompile(`^(\d+)-(\d+)$`) volumesPathRegexp = regexp.MustCompile(`^[a-zA-Z0-9\-\.\_/]+$`) - awsSNSTopicRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]*(\.fifo)?$`) // Validates that an expression contains only letters, numbers, underscores, hyphens, and .fifo suffix. - awsNameRegexp = regexp.MustCompile(`^[a-z][a-z0-9\-]+$`) // Validates that an expression starts with a letter and only contains letters, numbers, and hyphens. - punctuationRegExp = regexp.MustCompile(`[\.\-]{2,}`) // Check for consecutive periods or dashes. - trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. + awsSNSTopicRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`) // Validates that an expression contains only letters, numbers, underscores, hyphens, and .fifo suffix. + awsNameRegexp = regexp.MustCompile(`^[a-z][a-z0-9\-]+$`) // Validates that an expression starts with a letter and only contains letters, numbers, and hyphens. + punctuationRegExp = regexp.MustCompile(`[\.\-]{2,}`) // Check for consecutive periods or dashes. + trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. essentialContainerDependsOnValidStatuses = []string{dependsOnStart, dependsOnHealthy} dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} @@ -1448,30 +1447,32 @@ func (t TopicSubscription) validate() error { return fmt.Errorf("service name must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") } - if err := t.Queue.validate(template.IsFIFOFunc(aws.StringValue(t.Name))); err != nil { + if err := t.Queue.validate(); err != nil { return fmt.Errorf(`validate "queue": %w`, err) } return nil } // validate returns nil if SQSQueue is configured correctly. -func (q SQSQueueOrBool) validate(isFIFO bool) error { +func (q SQSQueueOrBool) validate() error { - if !isFIFO { - if q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || - q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil { - return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`) - } - if q.IsEmpty() { - return nil - } - return q.Advanced.validate() + if q.IsEmpty() { + return nil } - if q.IsFIFOEmpty() { - return errors.New(`FIFO SQS Queue configuration is required, you can either set "queue: true" or complete configuration alternatively`) + if q.Enabled != nil { + return nil + } else { + if q.Advanced.Type != nil && strings.Compare(aws.StringValue(q.Advanced.Type), "fifo") == 0 { + return q.Advanced.validateFIFO() + } else { + if q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || + q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil { + return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`) + } + return q.Advanced.validate() + } } - return q.Advanced.validateFIFO() } // validate returns nil if SQSQueue is configured correctly. @@ -1487,7 +1488,7 @@ func (q SQSQueue) validate() error { // validateFIFO returns nil if FIFO SQSQueue is configured correctly. func (q SQSQueue) validateFIFO() error { - if q.IsFIFOEmpty() { + if q.IsEmpty() { return nil } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 78dbdf383d8..bf05a8b99ca 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2752,16 +2752,9 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: nil, }, - "should return an error if fifo queue config is missing": { + "should not return error if standard queue is enabled": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), - Service: aws.String("mockservice"), - }, - wanted: errors.New(`validate "queue": FIFO SQS Queue configuration is required, you can either set "queue: true" or complete configuration alternatively`), - }, - "should not return error if fifo queue enabled": { - in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Enabled: aws.Bool(true), @@ -2780,6 +2773,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FifoThroughputLimit: aws.String("incorrectFIFOThoughoutLimit"), + Type: aws.String("fifo"), }, }, }, @@ -2796,6 +2790,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FifoThroughputLimit: aws.String("perMessageGroupId"), + Type: aws.String("fifo"), }, }, }, @@ -2812,6 +2807,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, DeduplicationScope: aws.String("incorrectDeduplicateScope"), + Type: aws.String("fifo"), }, }, }, @@ -2828,6 +2824,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, DeduplicationScope: aws.String("messageGroup"), + Type: aws.String("fifo"), }, }, }, @@ -2861,6 +2858,7 @@ func TestTopicSubscription_validate(t *testing.T) { DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, HighThroughputFifo: aws.Bool(true), DeduplicationScope: aws.String("messageGroup"), + Type: aws.String("fifo"), }, }, }, @@ -2878,6 +2876,7 @@ func TestTopicSubscription_validate(t *testing.T) { DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FifoThroughputLimit: aws.String("perMessageGroupId"), DeduplicationScope: aws.String("queue"), + Type: aws.String("fifo"), }, }, }, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 5c1438424ed..2f9be8cda3c 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -5,6 +5,7 @@ package manifest import ( "errors" + "strings" "time" "github.com/aws/aws-sdk-go/aws" @@ -86,10 +87,11 @@ func (q *SQSQueueOrBool) IsEmpty() bool { return q.Advanced.IsEmpty() && q.Enabled == nil } +/* // IsFIFOEmpty returns empty if the struct has all zero members. func (q *SQSQueueOrBool) IsFIFOEmpty() bool { return q.Advanced.IsFIFOEmpty() && q.Enabled == nil -} +}*/ // UnmarshalYAML implements the yaml(v3) interface. It allows SQSQueueOrBool to be specified as a // string or a struct alternately. @@ -123,20 +125,22 @@ type SQSQueue struct { DeduplicationScope *string `yaml:"deduplication_scope"` FifoThroughputLimit *string `yaml:"fifo_throughput_limit"` HighThroughputFifo *bool `yaml:"high_throughput_fifo"` + Type *string `yaml:"type"` } // IsEmpty returns empty if the struct has all zero members. func (q *SQSQueue) IsEmpty() bool { return q.Retention == nil && q.Delay == nil && q.Timeout == nil && - q.DeadLetter.IsEmpty() + q.DeadLetter.IsEmpty() && q.FifoThroughputLimit == nil && q.HighThroughputFifo == nil && + q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil && q.Type == nil } -// IsFIFOEmpty returns empty if the struct has all zero members. +/*// IsFIFOEmpty returns empty if the struct has all zero members. func (q *SQSQueue) IsFIFOEmpty() bool { return q.Retention == nil && q.Delay == nil && q.Timeout == nil && q.DeadLetter.IsEmpty() && q.FifoThroughputLimit == nil && q.HighThroughputFifo == nil && - q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil -} + q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil && q.Type == nil +}*/ // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { @@ -211,6 +215,16 @@ func (s *WorkerService) Subscriptions() []TopicSubscription { return s.Subscribe.Topics } +func (s *WorkerService) AttachFIFOSuffixToFIFOQueues() { + for idx, topic := range s.Subscribe.Topics { + if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type != nil && strings.Compare(aws.StringValue(s.Subscribe.Queue.Type), "fifo") == 0 { + s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") + } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type != nil && strings.Compare(aws.StringValue(topic.Queue.Advanced.Type), "fifo") == 0 { + s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") + } + } +} + func (s WorkerService) applyEnv(envName string) (workloadManifest, error) { overrideConfig, ok := s.Environments[envName] if !ok { diff --git a/internal/pkg/template/template_functions.go b/internal/pkg/template/template_functions.go index 8dd93881b07..08df902e866 100644 --- a/internal/pkg/template/template_functions.go +++ b/internal/pkg/template/template_functions.go @@ -70,6 +70,9 @@ func EnvVarSecretFunc(s string) string { return StripNonAlphaNumFunc(s) + "Secret" } +// Deref dereferences the pointer. +func Deref(i *string) string { return *i } + // Grabs word boundaries in default CamelCase. Matches lowercase letters & numbers // before the next capital as capturing group 1, and the first capital in the // next word as capturing group 2. Will match "yC" in "MyCamel" and "y2ndC" in"My2ndCamel" diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index d013e67fb78..2ba7e2f2899 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -58,10 +58,10 @@ EventsKMSKey: - "kms:Decrypt" Resource: '*' -{{- if .Subscribe.StandardQueue}} +{{- if and .Subscribe .Subscribe.QueueType}} EventsQueue: Metadata: - 'aws:copilot:description': 'An events SQS queue to buffer messages' + 'aws:copilot:description': {{- if eq (deref .Subscribe.QueueType) "fifo"}}'An events SQS FIFO queue to buffer messages'{{- else}}'An events SQS queue to buffer messages'{{- end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -81,16 +81,31 @@ EventsQueue: deadLetterTargetArn: !GetAtt DeadLetterQueue.Arn maxReceiveCount: {{.Subscribe.Queue.DeadLetter.Tries}} {{- end}} + {{- if eq (deref .Subscribe.QueueType) "fifo"}} + FifoQueue: true + {{- end}} + {{- if .Subscribe.Queue.FifoThroughputLimit }} + FifoThroughputLimit: {{.Subscribe.Queue.FifoThroughputLimit}} + {{- end}} + {{- if .Subscribe.Queue.DeduplicationScope }} + DeduplicationScope: {{.Subscribe.Queue.DeduplicationScope}} + {{- end }} + {{- if .Subscribe.Queue.ContentBasedDeduplication }} + ContentBasedDeduplication: {{.Subscribe.Queue.ContentBasedDeduplication}} + {{- end}} {{- end}} {{- if .Subscribe.Queue}}{{- if .Subscribe.Queue.DeadLetter}} DeadLetterQueue: Metadata: - 'aws:copilot:description': 'A dead letter SQS queue to buffer failed messages from the events queue' + 'aws:copilot:description': {{- if eq (deref .Subscribe.QueueType) "fifo"}}'A dead letter SQS FIFO queue to buffer failed messages from the events queue'{{- else}}'A dead letter SQS queue to buffer failed messages from the events queue'{{- end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days + {{- if eq (deref .Subscribe.QueueType) "fifo" }} + FifoQueue: true + {{- end }} DeadLetterPolicy: Type: AWS::SQS::QueuePolicy @@ -110,10 +125,10 @@ DeadLetterPolicy: {{- end}}{{- end}} {{- end}}{{/* endif .Subscribe */}} -{{- end}}{{/* endif .Subscribe.StandardQueue */}} +{{- end}}{{/* endif and .Subscribe .Subscribe.StandardQueu */}} -{{- if .Subscribe.StandardQueue}} +{{- if and .Subscribe .Subscribe.QueueType}} QueuePolicy: Type: AWS::SQS::QueuePolicy Properties: @@ -132,7 +147,6 @@ QueuePolicy: {{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} {{- if not $topic.Queue}} - {{- if not (isFIFO $topic.Name)}} - Effect: Allow Principal: Service: sns.amazonaws.com @@ -144,9 +158,8 @@ QueuePolicy: aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{$topic.Name}}']] {{- end}} {{- end}} - {{- end}} {{- end}}{{/* if .Subscribe */}} -{{- end}}{{/* endif .Subscribe.StandardQueue */}} +{{- end}}{{/* if and .Subscribe .Subscribe.QueueType */}} {{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} @@ -187,7 +200,7 @@ QueuePolicy: deadLetterTargetArn: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue.Arn maxReceiveCount: {{$topic.Queue.DeadLetter.Tries}} {{- end}} - {{- if isFIFO $topic.Name }} + {{- if eq (deref $topic.Queue.Type) "fifo"}} FifoQueue: true {{- end}} {{- if $topic.Queue.FifoThroughputLimit }} @@ -208,11 +221,10 @@ QueuePolicy: Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days - {{- if isFIFO $topic.Name }} + {{- if eq (deref $topic.Queue.Type) "fifo"}} FifoQueue: true {{- end}} - {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterPolicy: Type: AWS::SQS::QueuePolicy Properties: diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index a47ae50493b..a82399c82b7 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -400,9 +400,9 @@ type Topic struct { // SubscribeOpts holds configuration needed if the service has subscriptions. type SubscribeOpts struct { - Topics []*TopicSubscription - Queue *SQSQueue - StandardQueue bool + Topics []*TopicSubscription + Queue *SQSQueue + QueueType *string } // HasTopicQueues returns true if any individual subscription has a dedicated queue. @@ -418,7 +418,7 @@ func (s *SubscribeOpts) HasTopicQueues() bool { // StandardDefaultQueue returns true if at least one standard topic exist and does not have its own queue configs. func (s *SubscribeOpts) StandardDefaultQueue() bool { for _, t := range s.Topics { - if !IsFIFOFunc(aws.StringValue(t.Name)) { + if t.Queue == nil { return true } } @@ -442,6 +442,7 @@ type SQSQueue struct { FifoThroughputLimit *string ContentBasedDeduplication *bool DeduplicationScope *string + Type *string } // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. @@ -642,6 +643,7 @@ func withSvcParsingFuncs() ParseOption { "pluralWord": english.PluralWord, "contains": contains, "requiresVPCConnector": requiresVPCConnector, + "deref": Deref, }) } } From 5cd49ca61dc07aa4c8db8fa4f39f382cfe857937 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 16 Sep 2022 17:06:49 -0700 Subject: [PATCH 04/19] modify logic to add .fifo suffix and type field value to default and custom queues = --- internal/pkg/cli/deploy/svc.go | 4 +- .../cloudformation/stack/transformers.go | 35 +- .../cloudformation/stack/transformers_test.go | 329 ++++++++++++++++-- .../deploy/cloudformation/stack/worker_svc.go | 7 +- internal/pkg/manifest/validate.go | 6 +- internal/pkg/manifest/worker_svc.go | 39 ++- internal/pkg/manifest/worker_svc_test.go | 283 ++++++++++++++- internal/pkg/manifest/workload.go | 7 + .../workloads/partials/cf/envvars-common.yml | 2 +- .../workloads/partials/cf/subscribe.yml | 18 +- .../workloads/services/worker/cf.yml | 8 + internal/pkg/template/workload.go | 15 +- 12 files changed, 665 insertions(+), 88 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index 5a16e324d98..e2f789050bb 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1174,7 +1174,9 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* for _, topic := range topics { topicARNs = append(topicARNs, topic.ARN()) } - d.wsMft.AttachFIFOSuffixToFIFOQueues() + if !d.wsMft.Subscribe.IsEmpty() { + d.wsMft.AttachFIFOSuffixToFIFOQueues() + } subs := d.wsMft.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { return nil, err diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 5b20c17cb99..1fb4b9bf50c 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -25,10 +25,11 @@ import ( ) const ( - enabled = "ENABLED" - disabled = "DISABLED" - defaultQueueType = "standard" - fifoQueue = "fifo" + enabled = "ENABLED" + disabled = "DISABLED" + standardQueueType = "standard" + fifoQueueType = "fifo" + defaultQueueType = standardQueueType ) // Default values for EFS options @@ -826,11 +827,14 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro subscriptions.Topics = append(subscriptions.Topics, ts) } subscriptions.Queue = convertQueue(s.Queue) - if subscriptions.Queue == nil && subscriptions.StandardDefaultQueue() { + /*if subscriptions.Queue == nil && subscriptions.StandardDefaultQueue() { subscriptions.QueueType = aws.String(defaultQueueType) - } else if strings.Compare(aws.StringValue(subscriptions.Queue.Type), "fifo") == 0 { - subscriptions.QueueType = aws.String(fifoQueue) - } + //subscriptions.Queue = &template.SQSQueue{Type: aws.String(defaultQueueType)} + } else if subscriptions.Queue != nil && strings.Compare(aws.StringValue(subscriptions.Queue.Type), "fifo") == 0 { + subscriptions.QueueType = aws.String(fifoQueueType) + } else if subscriptions.Queue != nil && aws.StringValue(subscriptions.Queue.Type) != "fifo" { + subscriptions.QueueType = aws.String(defaultQueueType) + }*/ return &subscriptions, nil } @@ -876,13 +880,13 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { return nil } - if strings.Compare(aws.StringValue(q.Type), defaultQueueType) == 0 { + if strings.Compare(aws.StringValue(q.Type), standardQueueType) == 0 { return &template.SQSQueue{ Retention: convertRetention(q.Retention), Delay: convertDelay(q.Delay), Timeout: convertTimeout(q.Timeout), DeadLetter: convertDeadLetter(q.DeadLetter), - Type: aws.String(defaultQueueType), + Type: q.Type, } } @@ -909,17 +913,6 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { DeduplicationScope: deduplicationScope, Type: q.Type, } - - /*if q.IsEmpty() { - return nil - } - return &template.SQSQueue{ - Retention: convertRetention(q.Retention), - Delay: convertDelay(q.Delay), - Timeout: convertTimeout(q.Timeout), - DeadLetter: convertDeadLetter(q.DeadLetter), - Type: aws.String(defaultQueueType), - }*/ } func convertTime(t *time.Duration) *int64 { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index a57f69af549..da00db7f714 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1445,6 +1445,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, + Type: aws.String(defaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1461,8 +1462,8 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, + Type: aws.String(defaultQueueType), }, - StandardQueue: true, }, }, "valid subscribe with no queue configs": { @@ -1473,7 +1474,9 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: manifest.SQSQueue{}, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1482,11 +1485,12 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: nil, - StandardQueue: true, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, - "valid subscribe with minimal queue": { + "valid subscribe with queue enabled": { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1506,13 +1510,53 @@ func Test_convertSubscribe(t *testing.T) { Name: aws.String("name"), Service: aws.String("svc"), Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(standardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, - StandardQueue: false, + Queue: nil, + }, + }, + "valid subscribe with minimal queue": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + }, + FilterPolicy: mockStruct, + }, + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + }, + Queue: nil, }, }, "valid subscribe with high throughput fifo sqs": { @@ -1529,6 +1573,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, + Type: aws.String(fifoQueueType), HighThroughputFifo: aws.Bool(true), }, }, @@ -1549,17 +1594,17 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, + Type: aws.String(fifoQueueType), FifoThroughputLimit: aws.String("messageGroup"), DeduplicationScope: aws.String("perMessageGroupId"), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, - StandardQueue: false, + Queue: nil, }, }, - "valid subscribe with default fifo sqs config values": { + "valid subscribe with custom minimal fifo sqs config values": { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1573,13 +1618,12 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String("fifo"), + Type: aws.String(fifoQueueType), }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1596,16 +1640,15 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: nil, DeduplicationScope: nil, ContentBasedDeduplication: nil, - Type: aws.String("fifo"), + Type: aws.String(fifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, - StandardQueue: false, + Queue: nil, }, }, - "valid subscribe with custom fifo sqs config values": { + "valid subscribe with custom complete fifo sqs config values": { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1622,13 +1665,12 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String("fifo"), + Type: aws.String(fifoQueueType), }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1645,13 +1687,256 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String("fifo"), + Type: aws.String(fifoQueueType), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + }, + Queue: nil, + }, + }, + "valid subscribe with custom complete fifo sqs config and standard topic subscription to a default standard queue": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + Type: aws.String(fifoQueueType), + }, + }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + Type: aws.String(fifoQueueType), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, + }, + }, + "valid subscribe with custom complete fifo sqs config and multiple standard topic subscriptions to a default standard queue": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + Type: aws.String(fifoQueueType), + }, + }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FifoThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + Type: aws.String(fifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1"), + Service: aws.String("svc"), + }, + }, + Queue: &template.SQSQueue{ + Type: aws.String(standardQueueType), + }, + }, + }, + "valid subscribe with standard sqs config and fifo topic subscription to a default fifo queue": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{ + Type: aws.String(fifoQueueType), + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: &template.SQSQueue{ + Type: aws.String(fifoQueueType), + }, + }, + }, + "valid subscribe with standard sqs config and multiple fifo topic subscriptions to a default fifo queue": { + inSubscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{ + Type: aws.String(fifoQueueType), + }, + }, + wanted: &template.SubscribeOpts{ + Topics: []*template.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{ + Retention: aws.Int64(111), + Delay: aws.Int64(111), + Timeout: aws.Int64(111), + DeadLetter: &template.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + Type: aws.String(standardQueueType), + }, + FilterPolicy: aws.String(`{"store":["example_corp"]}`), + }, + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: &template.SQSQueue{ + Type: aws.String(fifoQueueType), }, - Queue: nil, - StandardQueue: false, }, }, } diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index 15c0a4b434a..76295a19e54 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -149,12 +149,13 @@ func (s *WorkerService) Template() (string, error) { if err != nil { return "", fmt.Errorf("parse worker service template: %w", err) } - overridenTpl, err := s.taskDefOverrideFunc(convertTaskDefOverrideRules(s.manifest.TaskDefOverrides), content.Bytes()) + fmt.Println("printing content", string(content.Bytes())) + overriddenTpl, err := s.taskDefOverrideFunc(convertTaskDefOverrideRules(s.manifest.TaskDefOverrides), content.Bytes()) if err != nil { return "", fmt.Errorf("apply task definition overrides: %w", err) } - fmt.Println(string(overridenTpl)) - return string(overridenTpl), nil + fmt.Println(string(overriddenTpl)) + return string(overriddenTpl), nil } // Parameters returns the list of CloudFormation parameters used by the template. diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index c6c81ba6b5a..086b8999005 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -58,8 +58,8 @@ var ( httpProtocolVersions = []string{"GRPC", "HTTP1", "HTTP2"} invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} - validDeduplicationScopeValues = []string{"messageGroup", "queue"} - validFIFOThroughputLimitValues = []string{"perMessageGroupId", "perQueue"} + validDeduplicationScopeValues = []string{messageGroup, queue} + validFIFOThroughputLimitValues = []string{perMessageGroupID, perQueue} ) // Validate returns nil if DynamicLoadBalancedWebService is configured correctly. @@ -1468,7 +1468,7 @@ func (q SQSQueueOrBool) validate() error { } else { if q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil { - return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`) + return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`) } return q.Advanced.validate() } diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 2f9be8cda3c..37af074cfea 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -87,12 +87,6 @@ func (q *SQSQueueOrBool) IsEmpty() bool { return q.Advanced.IsEmpty() && q.Enabled == nil } -/* -// IsFIFOEmpty returns empty if the struct has all zero members. -func (q *SQSQueueOrBool) IsFIFOEmpty() bool { - return q.Advanced.IsFIFOEmpty() && q.Enabled == nil -}*/ - // UnmarshalYAML implements the yaml(v3) interface. It allows SQSQueueOrBool to be specified as a // string or a struct alternately. func (q *SQSQueueOrBool) UnmarshalYAML(value *yaml.Node) error { @@ -135,13 +129,6 @@ func (q *SQSQueue) IsEmpty() bool { q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil && q.Type == nil } -/*// IsFIFOEmpty returns empty if the struct has all zero members. -func (q *SQSQueue) IsFIFOEmpty() bool { - return q.Retention == nil && q.Delay == nil && q.Timeout == nil && - q.DeadLetter.IsEmpty() && q.FifoThroughputLimit == nil && q.HighThroughputFifo == nil && - q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil && q.Type == nil -}*/ - // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. type DeadLetterQueue struct { Tries *uint16 `yaml:"tries"` @@ -215,14 +202,38 @@ func (s *WorkerService) Subscriptions() []TopicSubscription { return s.Subscribe.Topics } +// Rename is function func (s *WorkerService) AttachFIFOSuffixToFIFOQueues() { for idx, topic := range s.Subscribe.Topics { + // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type != nil && strings.Compare(aws.StringValue(s.Subscribe.Queue.Type), "fifo") == 0 { s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") - } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type != nil && strings.Compare(aws.StringValue(topic.Queue.Advanced.Type), "fifo") == 0 { + } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type != nil && strings.Compare(aws.StringValue(topic.Queue.Advanced.Type), "fifo") == 0 { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") } + // if condition sets topic specific type to defaultQueueType (i.e. standard) in case customer doesn't define it in their topic specific queue. + if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type == nil { + s.Subscribe.Topics[idx].Queue.Advanced.Type = aws.String(defaultQueueType) + } + } + if s.Subscribe.Queue.IsEmpty() && s.Subscribe.StandardDefaultQueue() { + s.Subscribe.Queue = SQSQueue{ + Type: aws.String(defaultQueueType), + } + //subscriptions.Queue = &template.SQSQueue{Type: aws.String(defaultQueueType)} + } else if !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type == nil { + s.Subscribe.Queue.Type = aws.String(defaultQueueType) + } +} + +// StandardDefaultQueue returns true if at least one standard topic exist and does not have its own queue configs. +func (s *SubscribeConfig) StandardDefaultQueue() bool { + for _, t := range s.Topics { + if t.Queue.IsEmpty() { + return true + } } + return false } func (s WorkerService) applyEnv(envName string) (workloadManifest, error) { diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index bd2e9fe8698..5c0d1f43229 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -42,7 +42,8 @@ func newMockSQSFIFOQueue() SQSQueue { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String("asd"), + FifoThroughputLimit: aws.String("perMessageID"), + Type: aws.String(fifoQueueType), } } @@ -1283,3 +1284,283 @@ func TestWorkerService_RequiredEnvironmentFeatures(t *testing.T) { }) } } + +func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { + duration111Seconds := 111 * time.Second + testCases := map[string]struct { + input *WorkerService + expected *WorkerService + }{ + "empty subscription": { + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{}, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{}, + }, + }, + }, + "valid subscribe with one topic specific standard queue and no default queue": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: newMockSQSQueueOrBool(), + }, + }, + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + Type: aws.String(defaultQueueType), + }, + }, + }, + }, + }, + }, + }, + }, + "valid subscribe with one topic specific fifo queue and no default queue": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: newMockSQSFIFOQueueOrBool(), + }, + }, + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + Queue: newMockSQSFIFOQueueOrBool(), + }, + }, + }, + }, + }, + }, + "valid subscribe with no topic specific standard queue but with default standard queue with empty config": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: SQSQueue{}, + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: SQSQueue{ + Type: aws.String(defaultQueueType), + }, + }, + }, + }, + }, + "valid subscribe with no topic specific standard queue but with default standard queue with minimal config": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: newMockSQSQueue(), + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + Type: aws.String(defaultQueueType), + }, + }, + }, + }, + }, + "valid subscribe with no topic specific fifo queue but with default fifo queue with minimal config": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: newMockSQSFIFOQueue(), + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: newMockSQSFIFOQueue(), + }, + }, + }, + }, + "valid subscribe with one topic specific standard queue and another subscription for the default fifo queue": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: newMockSQSQueueOrBool(), + }, + { + Name: aws.String("name2"), + Service: aws.String("svc"), + }, + }, + Queue: newMockSQSFIFOQueue(), + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + Type: aws.String(defaultQueueType), + }, + }, + }, + { + Name: aws.String("name2.fifo"), + Service: aws.String("svc"), + }, + }, + Queue: newMockSQSFIFOQueue(), + }, + }, + }, + }, + "valid subscribe with one topic specific fifo queue and another subscription for the default standard queue": { + + input: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name2"), + Service: aws.String("svc"), + Queue: newMockSQSFIFOQueueOrBool(), + }, + }, + }, + }, + }, + expected: &WorkerService{ + WorkerServiceConfig: WorkerServiceConfig{ + Subscribe: SubscribeConfig{ + Topics: []TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name2.fifo"), + Service: aws.String("svc"), + Queue: newMockSQSFIFOQueueOrBool(), + }, + }, + Queue: SQSQueue{ + Type: aws.String(defaultQueueType), + }, + }, + }, + }, + }, + } + for name, tc := range testCases { + svc := tc.input + + t.Run(name, func(t *testing.T) { + // WHEN + svc.AttachFIFOSuffixToFIFOQueues() + + // THEN + require.Equal(t, tc.expected, svc) + }) + } +} diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index 262bca57824..89e18461134 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -22,6 +22,13 @@ import ( const ( defaultDockerfileName = "Dockerfile" + perMessageGroupID = "perMessageGroupId" + perQueue = "perQueue" + messageGroup = "messageGroup" + queue = "queue" + standardQueueType = "standard" + fifoQueueType = "fifo" + defaultQueueType = standardQueueType ) const ( diff --git a/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml b/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml index 21f7fe2893c..06c46f0ea73 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml @@ -16,7 +16,7 @@ {{- end}}{{- end}} {{- if eq .WorkloadType "Worker Service"}} - Name: COPILOT_QUEUE_URI - Value: !Ref EventsQueue + Value: {{ if .Subscribe.Queue }}!Ref EventsQueue {{ else }} "" {{ end }} {{- end}} {{- if .Subscribe}}{{if .Subscribe.HasTopicQueues}} - Name: COPILOT_TOPIC_QUEUE_URIS diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index 2ba7e2f2899..106daa9638b 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -58,10 +58,10 @@ EventsKMSKey: - "kms:Decrypt" Resource: '*' -{{- if and .Subscribe .Subscribe.QueueType}} +{{- if and .Subscribe .Subscribe.Queue}} EventsQueue: Metadata: - 'aws:copilot:description': {{- if eq (deref .Subscribe.QueueType) "fifo"}}'An events SQS FIFO queue to buffer messages'{{- else}}'An events SQS queue to buffer messages'{{- end}} + 'aws:copilot:description': {{ if eq (deref .Subscribe.Queue.Type) "fifo"}}'An events SQS FIFO queue to buffer messages'{{ else}}'An events SQS queue to buffer messages'{{ end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -81,8 +81,8 @@ EventsQueue: deadLetterTargetArn: !GetAtt DeadLetterQueue.Arn maxReceiveCount: {{.Subscribe.Queue.DeadLetter.Tries}} {{- end}} - {{- if eq (deref .Subscribe.QueueType) "fifo"}} - FifoQueue: true + {{- if eq (deref .Subscribe.Queue.Type) "fifo"}} + FifoQueue: true {{- end}} {{- if .Subscribe.Queue.FifoThroughputLimit }} FifoThroughputLimit: {{.Subscribe.Queue.FifoThroughputLimit}} @@ -98,12 +98,12 @@ EventsQueue: {{- if .Subscribe.Queue}}{{- if .Subscribe.Queue.DeadLetter}} DeadLetterQueue: Metadata: - 'aws:copilot:description': {{- if eq (deref .Subscribe.QueueType) "fifo"}}'A dead letter SQS FIFO queue to buffer failed messages from the events queue'{{- else}}'A dead letter SQS queue to buffer failed messages from the events queue'{{- end}} + 'aws:copilot:description': {{ if eq (deref .Subscribe.Queue.Type) "fifo"}}'A dead letter SQS FIFO queue to buffer failed messages from the events queue'{{ else}} 'A dead letter SQS queue to buffer failed messages from the events queue'{{ end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days - {{- if eq (deref .Subscribe.QueueType) "fifo" }} + {{- if eq (deref .Subscribe.Queue.Type) "fifo" }} FifoQueue: true {{- end }} @@ -128,7 +128,7 @@ DeadLetterPolicy: {{- end}}{{/* endif and .Subscribe .Subscribe.StandardQueu */}} -{{- if and .Subscribe .Subscribe.QueueType}} +{{- if and .Subscribe .Subscribe.Queue}} QueuePolicy: Type: AWS::SQS::QueuePolicy Properties: @@ -159,7 +159,7 @@ QueuePolicy: {{- end}} {{- end}} {{- end}}{{/* if .Subscribe */}} -{{- end}}{{/* if and .Subscribe .Subscribe.QueueType */}} +{{- end}}{{/* if and .Subscribe .Subscribe.Queue */}} {{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} @@ -266,6 +266,6 @@ QueuePolicy: Condition: ArnEquals: aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] -{{- end}}{{/* endif $topic.Queue =*/}} +{{- end}}{{/* endif $topic.Queue */}} {{- end}}{{/* endrange $topic := .Subscribe.Topics */}} {{- end}}{{/* if .Subscribe */}} \ No newline at end of file diff --git a/internal/pkg/template/templates/workloads/services/worker/cf.yml b/internal/pkg/template/templates/workloads/services/worker/cf.yml index f8a632cfa3f..563a05bac95 100644 --- a/internal/pkg/template/templates/workloads/services/worker/cf.yml +++ b/internal/pkg/template/templates/workloads/services/worker/cf.yml @@ -38,6 +38,14 @@ Conditions: !Not [!Equals [!Ref AddonsTemplateURL, ""]] HasEnvFile: !Not [!Equals [!Ref EnvFileARN, ""]] + {{- if .Subscribe.Queue }} + HasEventsQueue: + true + {{- else }} + HasEventsQueue: + false + {{- end}} + Resources: {{include "loggroup" . | indent 2}} diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index a82399c82b7..b47d150daa3 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -400,9 +400,8 @@ type Topic struct { // SubscribeOpts holds configuration needed if the service has subscriptions. type SubscribeOpts struct { - Topics []*TopicSubscription - Queue *SQSQueue - QueueType *string + Topics []*TopicSubscription + Queue *SQSQueue } // HasTopicQueues returns true if any individual subscription has a dedicated queue. @@ -415,16 +414,6 @@ func (s *SubscribeOpts) HasTopicQueues() bool { return false } -// StandardDefaultQueue returns true if at least one standard topic exist and does not have its own queue configs. -func (s *SubscribeOpts) StandardDefaultQueue() bool { - for _, t := range s.Topics { - if t.Queue == nil { - return true - } - } - return false -} - // TopicSubscription holds information needed to render a SNS Topic Subscription in a container definition. type TopicSubscription struct { Name *string From 9addc4b7cb1312d9be22942bffe4043202411451 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 19 Sep 2022 17:27:49 -0700 Subject: [PATCH 05/19] modify cfn template and more tests --- internal/pkg/cli/deploy/svc.go | 2 +- .../cloudformation/stack/transformers.go | 6 +- .../cloudformation/stack/transformers_test.go | 67 +++++++++++++------ .../stack/worker_service_integration_test.go | 2 + internal/pkg/manifest/validate.go | 6 +- internal/pkg/manifest/validate_test.go | 16 ++--- internal/pkg/manifest/worker_svc.go | 7 +- internal/pkg/manifest/worker_svc_test.go | 20 +++--- .../workloads/partials/cf/envvars-common.yml | 2 +- .../workloads/partials/cf/subscribe.yml | 12 ++-- .../workloads/services/worker/cf.yml | 7 -- 11 files changed, 81 insertions(+), 66 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index e2f789050bb..2e1bed06294 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1175,7 +1175,7 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* topicARNs = append(topicARNs, topic.ARN()) } if !d.wsMft.Subscribe.IsEmpty() { - d.wsMft.AttachFIFOSuffixToFIFOQueues() + d.wsMft.RetrofitFIFOConfig() } subs := d.wsMft.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 1fb4b9bf50c..523f8e20f13 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -30,6 +30,8 @@ const ( standardQueueType = "standard" fifoQueueType = "fifo" defaultQueueType = standardQueueType + messageGroup = "messageGroup" + perMessageGroupId = "perMessageGroupId" ) // Default values for EFS options @@ -899,8 +901,8 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { deduplicationScope = q.DeduplicationScope } } else if aws.BoolValue(q.HighThroughputFifo) == true { - fifoThroughputLimit = aws.String("messageGroup") - deduplicationScope = aws.String("perMessageGroupId") + fifoThroughputLimit = aws.String(perMessageGroupId) + deduplicationScope = aws.String(messageGroup) } return &template.SQSQueue{ diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index da00db7f714..8bab22c0ba3 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1426,11 +1426,11 @@ func Test_convertSubscribe(t *testing.T) { wanted *template.SubscribeOpts }{ - "empty subscription": { + "empty subscription": { //1 inSubscribe: manifest.SubscribeConfig{}, wanted: nil, }, - "valid subscribe": { + "valid subscribe": { //2 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1445,7 +1445,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(defaultQueueType), + Type: aws.String(standardQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1462,11 +1462,11 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(defaultQueueType), + Type: aws.String(standardQueueType), }, }, }, - "valid subscribe with no queue configs": { + "valid subscribe with default queue configs": { //3 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1490,7 +1490,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, }, - "valid subscribe with queue enabled": { + "valid subscribe with queue enabled": { //4 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1502,7 +1502,9 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{}, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1515,10 +1517,12 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, - "valid subscribe with minimal queue": { + "valid subscribe with minimal queue": { //5 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1538,6 +1542,9 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1556,10 +1563,12 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, - "valid subscribe with high throughput fifo sqs": { + "valid subscribe with high throughput fifo sqs": { //6 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1580,7 +1589,9 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{}, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1595,13 +1606,15 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, Type: aws.String(fifoQueueType), - FifoThroughputLimit: aws.String("messageGroup"), - DeduplicationScope: aws.String("perMessageGroupId"), + FifoThroughputLimit: aws.String("perMessageGroupId"), + DeduplicationScope: aws.String("messageGroup"), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, "valid subscribe with custom minimal fifo sqs config values": { @@ -1624,6 +1637,9 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1645,10 +1661,12 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, - "valid subscribe with custom complete fifo sqs config values": { + "valid subscribe with custom complete fifo sqs config values": { //7 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1671,6 +1689,9 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, + Queue: manifest.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1692,10 +1713,12 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: nil, + Queue: &template.SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, - "valid subscribe with custom complete fifo sqs config and standard topic subscription to a default standard queue": { + "valid subscribe with custom complete fifo sqs config and standard topic subscription to a default standard queue": { //8 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1755,7 +1778,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, }, - "valid subscribe with custom complete fifo sqs config and multiple standard topic subscriptions to a default standard queue": { + "valid subscribe with custom complete fifo sqs config and multiple standard topic subscriptions to a default standard queue": { //9 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1823,7 +1846,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, }, - "valid subscribe with standard sqs config and fifo topic subscription to a default fifo queue": { + "valid subscribe with standard sqs config and fifo topic subscription to a default fifo queue": { //10 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { @@ -1877,7 +1900,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, }, - "valid subscribe with standard sqs config and multiple fifo topic subscriptions to a default fifo queue": { + "valid subscribe with standard sqs config and multiple fifo topic subscriptions to a default fifo queue": { //11 inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { diff --git a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go index 1d50902d203..e29654823a9 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go @@ -46,6 +46,8 @@ func TestWorkerService_Template(t *testing.T) { v, ok := content.(*manifest.WorkerService) require.True(t, ok) + v.RetrofitFIFOConfig() + ws, err := workspace.New() require.NoError(t, err) diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 086b8999005..d740e2c265d 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -44,7 +44,7 @@ const ( var ( intRangeBandRegexp = regexp.MustCompile(`^(\d+)-(\d+)$`) volumesPathRegexp = regexp.MustCompile(`^[a-zA-Z0-9\-\.\_/]+$`) - awsSNSTopicRegexp = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`) // Validates that an expression contains only letters, numbers, underscores, hyphens, and .fifo suffix. + 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. punctuationRegExp = regexp.MustCompile(`[\.\-]{2,}`) // Check for consecutive periods or dashes. trailingPunctRegExp = regexp.MustCompile(`[\-\.]$`) // Check for trailing dash or dot. @@ -1675,9 +1675,9 @@ func validatePubSubName(name string) error { missingField: "name", } } - // Name must contain letters, numbers, and can't use special characters besides underscores, hyphens, and .fifo suffix. + // Name must contain letters, numbers, and can't use special characters besides underscores, and hyphens. if !awsSNSTopicRegexp.MatchString(name) { - return fmt.Errorf(`"name" can only contain letters, numbers, underscores, hyphens, and .fifo suffix`) + return fmt.Errorf(`"name" can only contain letters, numbers, underscores, and hyphens`) } return nil } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index bf05a8b99ca..138573d7d19 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2665,7 +2665,7 @@ func TestTopic_validate(t *testing.T) { in: Topic{ Name: aws.String("!@#"), }, - wanted: errors.New(`"name" can only contain letters, numbers, underscores, hyphens, and .fifo suffix`), + wanted: errors.New(`"name" can only contain letters, numbers, underscores, and hyphens`), }, } for name, tc := range testCases { @@ -2764,7 +2764,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, "should return error if invalid fifo throughput limit": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ @@ -2781,7 +2781,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, "should not return error if valid fifo throughput limit": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ @@ -2798,7 +2798,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, "should return error if invalid deduplicate scope": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ @@ -2815,7 +2815,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, "should not return error if valid deduplicate scope": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ @@ -2844,11 +2844,11 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue`), + wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`), }, "should return error if high_throughput_fifo is defined along with deduplication_scope or fifo_throughput_limit": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ @@ -2866,7 +2866,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, "should return error if invalid combination of fifo_throughput_limit and fifo_throughput_limit is defined": { in: TopicSubscription{ - Name: aws.String("mockTopic.fifo"), + Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 37af074cfea..8297aae56af 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -202,8 +202,8 @@ func (s *WorkerService) Subscriptions() []TopicSubscription { return s.Subscribe.Topics } -// Rename is function -func (s *WorkerService) AttachFIFOSuffixToFIFOQueues() { +// RetrofitFIFOConfig appends ".fifo" suffix to fifo topics, also adds values to type field. +func (s *WorkerService) RetrofitFIFOConfig() { for idx, topic := range s.Subscribe.Topics { // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type != nil && strings.Compare(aws.StringValue(s.Subscribe.Queue.Type), "fifo") == 0 { @@ -216,11 +216,10 @@ func (s *WorkerService) AttachFIFOSuffixToFIFOQueues() { s.Subscribe.Topics[idx].Queue.Advanced.Type = aws.String(defaultQueueType) } } - if s.Subscribe.Queue.IsEmpty() && s.Subscribe.StandardDefaultQueue() { + if s.Subscribe.Queue.IsEmpty() && s.Subscribe.Topics != nil { s.Subscribe.Queue = SQSQueue{ Type: aws.String(defaultQueueType), } - //subscriptions.Queue = &template.SQSQueue{Type: aws.String(defaultQueueType)} } else if !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type == nil { s.Subscribe.Queue.Type = aws.String(defaultQueueType) } diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 5c0d1f43229..50e12c020d9 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -1285,7 +1285,7 @@ func TestWorkerService_RequiredEnvironmentFeatures(t *testing.T) { } } -func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { +func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { duration111Seconds := 111 * time.Second testCases := map[string]struct { input *WorkerService @@ -1303,8 +1303,7 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { }, }, }, - "valid subscribe with one topic specific standard queue and no default queue": { - + "valid subscribe with one topic specific standard queue and a default standard queue": { input: &WorkerService{ WorkerServiceConfig: WorkerServiceConfig{ Subscribe: SubscribeConfig{ @@ -1336,12 +1335,14 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { }, }, }, + Queue: SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, }, }, - "valid subscribe with one topic specific fifo queue and no default queue": { - + "valid subscribe with one topic specific fifo queue and a default standard queue": { input: &WorkerService{ WorkerServiceConfig: WorkerServiceConfig{ Subscribe: SubscribeConfig{ @@ -1365,12 +1366,14 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { Queue: newMockSQSFIFOQueueOrBool(), }, }, + Queue: SQSQueue{ + Type: aws.String(defaultQueueType), + }, }, }, }, }, "valid subscribe with no topic specific standard queue but with default standard queue with empty config": { - input: &WorkerService{ WorkerServiceConfig: WorkerServiceConfig{ Subscribe: SubscribeConfig{ @@ -1380,7 +1383,6 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: SQSQueue{}, }, }, }, @@ -1401,7 +1403,6 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { }, }, "valid subscribe with no topic specific standard queue but with default standard queue with minimal config": { - input: &WorkerService{ WorkerServiceConfig: WorkerServiceConfig{ Subscribe: SubscribeConfig{ @@ -1436,7 +1437,6 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { }, }, "valid subscribe with no topic specific fifo queue but with default fifo queue with minimal config": { - input: &WorkerService{ WorkerServiceConfig: WorkerServiceConfig{ Subscribe: SubscribeConfig{ @@ -1557,7 +1557,7 @@ func TestWorkerService_AttachFIFOSuffixToFIFOQueues(t *testing.T) { t.Run(name, func(t *testing.T) { // WHEN - svc.AttachFIFOSuffixToFIFOQueues() + svc.RetrofitFIFOConfig() // THEN require.Equal(t, tc.expected, svc) diff --git a/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml b/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml index 06c46f0ea73..21f7fe2893c 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/envvars-common.yml @@ -16,7 +16,7 @@ {{- end}}{{- end}} {{- if eq .WorkloadType "Worker Service"}} - Name: COPILOT_QUEUE_URI - Value: {{ if .Subscribe.Queue }}!Ref EventsQueue {{ else }} "" {{ end }} + Value: !Ref EventsQueue {{- end}} {{- if .Subscribe}}{{if .Subscribe.HasTopicQueues}} - Name: COPILOT_TOPIC_QUEUE_URIS diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index 106daa9638b..984f04b1c8e 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -58,10 +58,9 @@ EventsKMSKey: - "kms:Decrypt" Resource: '*' -{{- if and .Subscribe .Subscribe.Queue}} EventsQueue: Metadata: - 'aws:copilot:description': {{ if eq (deref .Subscribe.Queue.Type) "fifo"}}'An events SQS FIFO queue to buffer messages'{{ else}}'An events SQS queue to buffer messages'{{ end}} + 'aws:copilot:description': {{ if and .Subscribe .Subscribe.Queue (eq (deref .Subscribe.Queue.Type) "fifo")}}'An events SQS FIFO queue to buffer messages'{{ else}}'An events SQS queue to buffer messages'{{ end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -125,10 +124,8 @@ DeadLetterPolicy: {{- end}}{{- end}} {{- end}}{{/* endif .Subscribe */}} -{{- end}}{{/* endif and .Subscribe .Subscribe.StandardQueu */}} -{{- if and .Subscribe .Subscribe.Queue}} QueuePolicy: Type: AWS::SQS::QueuePolicy Properties: @@ -159,7 +156,6 @@ QueuePolicy: {{- end}} {{- end}} {{- end}}{{/* if .Subscribe */}} -{{- end}}{{/* if and .Subscribe .Subscribe.Queue */}} {{- if .Subscribe }} {{- range $topic := .Subscribe.Topics}} @@ -182,7 +178,7 @@ QueuePolicy: {{- if $topic.Queue}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}EventsQueue: Metadata: - 'aws:copilot:description': 'A SQS queue to buffer messages from the topic {{$topic.Name}}' + 'aws:copilot:description': {{ if eq (deref $topic.Queue.Type) "fifo"}}'A SQS FIFO queue to buffer messages from the topic {{$topic.Name}}' {{ else }} 'A SQS queue to buffer messages from the topic {{$topic.Name}}' {{ end }} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -216,7 +212,7 @@ QueuePolicy: {{- if $topic.Queue.DeadLetter}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue: Metadata: - 'aws:copilot:description': 'A dead letter SQS queue to buffer failed messages from the topic {{$topic.Name}}' + 'aws:copilot:description': {{ if eq (deref $topic.Queue.Type) "fifo"}} 'A dead letter SQS FIFO queue to buffer failed messages from the topic {{$topic.Name}}' {{ else }} 'A dead letter SQS queue to buffer failed messages from the topic {{$topic.Name}}' {{ end }} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -265,7 +261,7 @@ QueuePolicy: Resource: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}EventsQueue.Arn Condition: ArnEquals: - aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] + aws:SourceArn: {{ if eq (deref $topic.Queue.Type) "fifo"}} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{$topic.Name}}']] {{ else }} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] {{ end }} {{- end}}{{/* endif $topic.Queue */}} {{- end}}{{/* endrange $topic := .Subscribe.Topics */}} {{- end}}{{/* if .Subscribe */}} \ No newline at end of file diff --git a/internal/pkg/template/templates/workloads/services/worker/cf.yml b/internal/pkg/template/templates/workloads/services/worker/cf.yml index 563a05bac95..c195225af61 100644 --- a/internal/pkg/template/templates/workloads/services/worker/cf.yml +++ b/internal/pkg/template/templates/workloads/services/worker/cf.yml @@ -38,13 +38,6 @@ Conditions: !Not [!Equals [!Ref AddonsTemplateURL, ""]] HasEnvFile: !Not [!Equals [!Ref EnvFileARN, ""]] - {{- if .Subscribe.Queue }} - HasEventsQueue: - true - {{- else }} - HasEventsQueue: - false - {{- end}} Resources: {{include "loggroup" . | indent 2}} From ffea541bb2ec4d7ca9c3a83875de1f9105ef6312 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 19 Sep 2022 17:45:39 -0700 Subject: [PATCH 06/19] nits --- .../pkg/deploy/cloudformation/stack/transformers.go | 8 -------- .../pkg/deploy/cloudformation/stack/worker_svc.go | 2 -- internal/pkg/manifest/validate.go | 4 +--- internal/pkg/manifest/validate_test.go | 13 +------------ internal/pkg/manifest/worker_svc.go | 10 ---------- internal/pkg/template/template_functions.go | 5 ----- .../templates/workloads/services/worker/cf.yml | 1 - internal/pkg/template/workload.go | 1 - 8 files changed, 2 insertions(+), 42 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 523f8e20f13..c8e1feb2296 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -829,14 +829,6 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro subscriptions.Topics = append(subscriptions.Topics, ts) } subscriptions.Queue = convertQueue(s.Queue) - /*if subscriptions.Queue == nil && subscriptions.StandardDefaultQueue() { - subscriptions.QueueType = aws.String(defaultQueueType) - //subscriptions.Queue = &template.SQSQueue{Type: aws.String(defaultQueueType)} - } else if subscriptions.Queue != nil && strings.Compare(aws.StringValue(subscriptions.Queue.Type), "fifo") == 0 { - subscriptions.QueueType = aws.String(fifoQueueType) - } else if subscriptions.Queue != nil && aws.StringValue(subscriptions.Queue.Type) != "fifo" { - subscriptions.QueueType = aws.String(defaultQueueType) - }*/ return &subscriptions, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index 76295a19e54..73bb4cce9d4 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -149,12 +149,10 @@ func (s *WorkerService) Template() (string, error) { if err != nil { return "", fmt.Errorf("parse worker service template: %w", err) } - fmt.Println("printing content", string(content.Bytes())) overriddenTpl, err := s.taskDefOverrideFunc(convertTaskDefOverrideRules(s.manifest.TaskDefOverrides), content.Bytes()) if err != nil { return "", fmt.Errorf("apply task definition overrides: %w", err) } - fmt.Println(string(overriddenTpl)) return string(overriddenTpl), nil } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index d740e2c265d..60a7f464319 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1446,7 +1446,6 @@ func (t TopicSubscription) validate() error { if !isValidSubSvcName(svcName) { return fmt.Errorf("service name must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") } - if err := t.Queue.validate(); err != nil { return fmt.Errorf(`validate "queue": %w`, err) } @@ -1455,7 +1454,6 @@ func (t TopicSubscription) validate() error { // validate returns nil if SQSQueue is configured correctly. func (q SQSQueueOrBool) validate() error { - if q.IsEmpty() { return nil } @@ -1502,7 +1500,7 @@ func (q SQSQueue) validateFIFO() error { return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) } if q.FifoThroughputLimit != nil && strings.Compare(aws.StringValue(q.FifoThroughputLimit), "perMessageGroupId") == 0 && q.DeduplicationScope != nil && strings.Compare(aws.StringValue(q.DeduplicationScope), "queue") == 0 { - return fmt.Errorf(`when Deduplication scope is set to Queue, FIFO throughput limit must be set to Per Queue`) + return fmt.Errorf(`when deduplication_scope scope is set to queue, fifo_throughput_limit must be set to perQueue`) } if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 138573d7d19..00abb6dea80 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2697,17 +2697,6 @@ func TestSubscribeConfig_validate(t *testing.T) { }, wantedErrorPrefix: `validate "topics[0]": `, }, - /*"error if fail to validate topics1": { - config: SubscribeConfig{ - Topics: []TopicSubscription{ - { - Name: aws.String("mocktopic"), - Service: aws.String("mockservice"), - }, - }, - }, - wantedErrorPrefix: `validate "topics[0]": `, - },*/ } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -2880,7 +2869,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": when Deduplication scope is set to Queue, FIFO throughput limit must be set to Per Queue`), + wanted: errors.New(`validate "queue": when deduplication_scope scope is set to queue, fifo_throughput_limit must be set to perQueue`), }, "should not return error if valid standard queue config defined": { in: TopicSubscription{ diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 8297aae56af..b8f7abcc510 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -225,16 +225,6 @@ func (s *WorkerService) RetrofitFIFOConfig() { } } -// StandardDefaultQueue returns true if at least one standard topic exist and does not have its own queue configs. -func (s *SubscribeConfig) StandardDefaultQueue() bool { - for _, t := range s.Topics { - if t.Queue.IsEmpty() { - return true - } - } - return false -} - func (s WorkerService) applyEnv(envName string) (workloadManifest, error) { overrideConfig, ok := s.Environments[envName] if !ok { diff --git a/internal/pkg/template/template_functions.go b/internal/pkg/template/template_functions.go index 08df902e866..5a6ea23bad9 100644 --- a/internal/pkg/template/template_functions.go +++ b/internal/pkg/template/template_functions.go @@ -30,11 +30,6 @@ func ReplaceDashesFunc(logicalID string) string { return strings.ReplaceAll(logicalID, "-", dashReplacement) } -// IsFIFOFunc takes a string value and determines if has .fifo suffix. -func IsFIFOFunc(value string) bool { - return strings.Contains(value, ".fifo") -} - // IsArnFunc takes a string value and determines if it's an ARN or not. func IsARNFunc(value string) bool { return arn.IsARN(value) diff --git a/internal/pkg/template/templates/workloads/services/worker/cf.yml b/internal/pkg/template/templates/workloads/services/worker/cf.yml index c195225af61..f8a632cfa3f 100644 --- a/internal/pkg/template/templates/workloads/services/worker/cf.yml +++ b/internal/pkg/template/templates/workloads/services/worker/cf.yml @@ -38,7 +38,6 @@ Conditions: !Not [!Equals [!Ref AddonsTemplateURL, ""]] HasEnvFile: !Not [!Equals [!Ref EnvFileARN, ""]] - Resources: {{include "loggroup" . | indent 2}} diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index b47d150daa3..291e5a4dfa0 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -627,7 +627,6 @@ func withSvcParsingFuncs() ParseOption { "jsonQueueURIs": generateQueueURIJSON, "envControllerParams": envControllerParameters, "logicalIDSafe": StripNonAlphaNumFunc, - "isFIFO": IsFIFOFunc, "wordSeries": english.WordSeries, "pluralWord": english.PluralWord, "contains": contains, From 3d53bdcd6c3c57138d9fd59e6d963f7153334333 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 19 Sep 2022 18:00:03 -0700 Subject: [PATCH 07/19] nits --- internal/pkg/deploy/cloudformation/stack/transformers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index c8e1feb2296..40994ab7c13 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -885,14 +885,14 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { } var fifoThroughputLimit, deduplicationScope *string - if aws.BoolValue(q.HighThroughputFifo) != true { + if !aws.BoolValue(q.HighThroughputFifo) { if q.FifoThroughputLimit != nil { fifoThroughputLimit = q.FifoThroughputLimit } if q.DeduplicationScope != nil { deduplicationScope = q.DeduplicationScope } - } else if aws.BoolValue(q.HighThroughputFifo) == true { + } else if aws.BoolValue(q.HighThroughputFifo) { fifoThroughputLimit = aws.String(perMessageGroupId) deduplicationScope = aws.String(messageGroup) } From d050a263182f5c986fe38fb540aa1f41ae769df0 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 20 Sep 2022 14:51:04 -0700 Subject: [PATCH 08/19] addressed only all the nits --- .../cloudformation/stack/transformers.go | 29 +++--- .../cloudformation/stack/transformers_test.go | 78 ++++++++-------- internal/pkg/manifest/validate.go | 89 ++++++++++++++----- internal/pkg/manifest/validate_test.go | 62 +++++++++---- internal/pkg/manifest/worker_svc.go | 6 +- internal/pkg/manifest/worker_svc_test.go | 16 ++-- internal/pkg/manifest/workload.go | 20 +++-- 7 files changed, 191 insertions(+), 109 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 6df39a59aa8..6498efb736b 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -25,13 +25,16 @@ import ( ) const ( - enabled = "ENABLED" - disabled = "DISABLED" - standardQueueType = "standard" - fifoQueueType = "fifo" - defaultQueueType = standardQueueType - messageGroup = "messageGroup" - perMessageGroupId = "perMessageGroupId" + enabled = "ENABLED" + disabled = "DISABLED" +) + +const ( + sqsStandardQueueType = "standard" + sqsFifoQueueType = "fifo" + sqsDefaultQueueType = sqsStandardQueueType + sqsDedupeScopeMessageGroup = "messageGroup" + sqsFIFOThroughputLimitPerMessageGroupId = "perMessageGroupId" ) // Default values for EFS options @@ -844,17 +847,15 @@ func convertTopicSubscription(t manifest.TopicSubscription) ( Name: t.Name, Service: t.Service, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, FilterPolicy: filterPolicy, }, nil } - queueAdvancedConfig := convertQueue(t.Queue.Advanced) - return &template.TopicSubscription{ Name: t.Name, Service: t.Service, - Queue: queueAdvancedConfig, + Queue: convertQueue(t.Queue.Advanced), FilterPolicy: filterPolicy, }, nil } @@ -875,7 +876,7 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { return nil } - if strings.Compare(aws.StringValue(q.Type), standardQueueType) == 0 { + if aws.StringValue(q.Type) == sqsStandardQueueType { return &template.SQSQueue{ Retention: convertRetention(q.Retention), Delay: convertDelay(q.Delay), @@ -894,8 +895,8 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { deduplicationScope = q.DeduplicationScope } } else if aws.BoolValue(q.HighThroughputFifo) { - fifoThroughputLimit = aws.String(perMessageGroupId) - deduplicationScope = aws.String(messageGroup) + fifoThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) + deduplicationScope = aws.String(sqsDedupeScopeMessageGroup) } return &template.SQSQueue{ diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 689918e5204..48c4b793921 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1486,7 +1486,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1503,7 +1503,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, }, @@ -1516,7 +1516,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1527,7 +1527,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1544,7 +1544,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1553,13 +1553,13 @@ func Test_convertSubscribe(t *testing.T) { Name: aws.String("name"), Service: aws.String("svc"), Queue: &template.SQSQueue{ - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1577,14 +1577,14 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1599,13 +1599,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1623,7 +1623,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), HighThroughputFifo: aws.Bool(true), }, }, @@ -1631,7 +1631,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1646,7 +1646,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), FifoThroughputLimit: aws.String("perMessageGroupId"), DeduplicationScope: aws.String("messageGroup"), }, @@ -1654,7 +1654,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1672,14 +1672,14 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, FilterPolicy: mockStruct, }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1697,13 +1697,13 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: nil, DeduplicationScope: nil, ContentBasedDeduplication: nil, - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1724,14 +1724,14 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, FilterPolicy: mockStruct, }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1749,13 +1749,13 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1776,7 +1776,7 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, FilterPolicy: mockStruct, @@ -1787,7 +1787,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1805,7 +1805,7 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1815,7 +1815,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1836,7 +1836,7 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, FilterPolicy: mockStruct, @@ -1851,7 +1851,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1869,7 +1869,7 @@ func Test_convertSubscribe(t *testing.T) { FifoThroughputLimit: aws.String("queue"), DeduplicationScope: aws.String("perQueue"), ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1883,7 +1883,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, }, @@ -1901,7 +1901,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, @@ -1912,7 +1912,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1927,7 +1927,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1937,7 +1937,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, }, @@ -1955,7 +1955,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, @@ -1970,7 +1970,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1985,7 +1985,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(standardQueueType), + Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1999,7 +1999,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), }, }, }, diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 8db0bd6bdf9..9a266516bf6 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -58,10 +58,9 @@ var ( httpProtocolVersions = []string{"GRPC", "HTTP1", "HTTP2"} invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} - - validDeduplicationScopeValues = []string{messageGroup, queue} - validFIFOThroughputLimitValues = []string{perMessageGroupID, perQueue} validTopicsTypeValues = []string{standardTopicType, fifoTopicType} + validDeduplicationScopeValues = []string{sqsFifoMessageGroup, sqsFifoQueue} + validFIFOThroughputLimitValues = []string{sqsFifoPerMessageGroupID, sqsFifoPerQueue} ) // Validate returns nil if DynamicLoadBalancedWebService is configured correctly. @@ -1465,17 +1464,30 @@ func (q SQSQueueOrBool) validate() error { if q.Enabled != nil { return nil - } else { - if q.Advanced.Type != nil && strings.Compare(aws.StringValue(q.Advanced.Type), "fifo") == 0 { - return q.Advanced.validateFIFO() - } else { - if q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || - q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil { - return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`) - } - return q.Advanced.validate() - } } + if q.validateType() { + return fmt.Errorf(`validate "type": type value must be one of %s`, english.WordSeries(sqsValidQueueTypeValues, "or")) + } + if q.isFIFO() { + return q.Advanced.validateFIFO() + } + if q.hasFIFOFieldsEnabledOnStandardQueue() { + return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`) + } + return q.Advanced.validate() +} + +func (q SQSQueueOrBool) isFIFO() bool { + return q.Advanced.Type != nil && aws.StringValue(q.Advanced.Type) == sqsFifoQueueType +} + +func (q SQSQueueOrBool) validateType() bool { + return q.Advanced.Type != nil && !contains(aws.StringValue(q.Advanced.Type), sqsValidQueueTypeValues) +} + +func (q SQSQueueOrBool) hasFIFOFieldsEnabledOnStandardQueue() bool { + return q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || + q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil } // validate returns nil if SQSQueue is configured correctly. @@ -1495,17 +1507,17 @@ func (q SQSQueue) validateFIFO() error { return nil } - if q.HighThroughputFifo != nil && (q.FifoThroughputLimit != nil || q.DeduplicationScope != nil) { - return fmt.Errorf(`validate "high_throughput_fifo": cannot define fifo_throughput_limit and fifo_throughput_limit when high_throughput_fifo is defined`) + if err := q.validateHighThroughputFifo(); err != nil { + return err } - if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { - return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) + if err := q.validateDeduplicationScope(); err != nil { + return err } - if q.FifoThroughputLimit != nil && !contains(aws.StringValue(q.FifoThroughputLimit), validFIFOThroughputLimitValues) { - return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) + if err := q.validateFifoThroughputLimit(); err != nil { + return err } - if q.FifoThroughputLimit != nil && strings.Compare(aws.StringValue(q.FifoThroughputLimit), "perMessageGroupId") == 0 && q.DeduplicationScope != nil && strings.Compare(aws.StringValue(q.DeduplicationScope), "queue") == 0 { - return fmt.Errorf(`when deduplication_scope scope is set to queue, fifo_throughput_limit must be set to perQueue`) + if aws.StringValue(q.FifoThroughputLimit) == sqsFifoPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsFifoQueue { + return fmt.Errorf(`"fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`) } if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) @@ -1513,6 +1525,41 @@ func (q SQSQueue) validateFIFO() error { return nil } +func (q SQSQueue) validateHighThroughputFifo() error { + if q.HighThroughputFifo != nil { + if q.FifoThroughputLimit != nil { + return &errFieldMutualExclusive{ + firstField: "high_throughput_fifo", + secondField: "fifo_throughput_limit", + mustExist: false, + } + } + + if q.DeduplicationScope != nil { + return &errFieldMutualExclusive{ + firstField: "high_throughput_fifo", + secondField: "deduplication_scope", + mustExist: false, + } + } + } + return nil +} + +func (q SQSQueue) validateDeduplicationScope() error { + if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { + return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) + } + return nil +} + +func (q SQSQueue) validateFifoThroughputLimit() error { + if q.FifoThroughputLimit != nil && !contains(aws.StringValue(q.FifoThroughputLimit), validFIFOThroughputLimitValues) { + return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) + } + return nil +} + // validate returns nil if DeadLetterQueue is configured correctly. func (d DeadLetterQueue) validate() error { if d.IsEmpty() { diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index e456ef7de22..d2038d6db91 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2802,6 +2802,18 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: nil, }, + "should return error if invalid queue type is provided": { + in: TopicSubscription{ + Name: aws.String("mockTopic"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Type: aws.String("FIFO"), // it is case-sensitive, correct values are standard or fifo. + }, + }, + }, + wanted: errors.New(`validate "queue": validate "type": type value must be one of standard or fifo`), + }, "should return error if invalid fifo throughput limit": { in: TopicSubscription{ Name: aws.String("mockTopic"), @@ -2813,7 +2825,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FifoThroughputLimit: aws.String("incorrectFIFOThoughoutLimit"), - Type: aws.String("fifo"), + Type: aws.String(sqsFifoQueueType), }, }, }, @@ -2829,8 +2841,8 @@ func TestTopicSubscription_validate(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String("perMessageGroupId"), - Type: aws.String("fifo"), + FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + Type: aws.String(sqsFifoQueueType), }, }, }, @@ -2847,7 +2859,7 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, DeduplicationScope: aws.String("incorrectDeduplicateScope"), - Type: aws.String("fifo"), + Type: aws.String(sqsFifoQueueType), }, }, }, @@ -2863,8 +2875,8 @@ func TestTopicSubscription_validate(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - DeduplicationScope: aws.String("messageGroup"), - Type: aws.String("fifo"), + DeduplicationScope: aws.String(sqsFifoMessageGroup), + Type: aws.String(sqsFifoQueueType), }, }, }, @@ -2880,13 +2892,13 @@ func TestTopicSubscription_validate(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - DeduplicationScope: aws.String("messageGroup"), + DeduplicationScope: aws.String(sqsFifoMessageGroup), }, }, }, wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`), }, - "should return error if high_throughput_fifo is defined along with deduplication_scope or fifo_throughput_limit": { + "should return error if high_throughput_fifo is defined along with deduplication_scope": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), @@ -2897,14 +2909,32 @@ func TestTopicSubscription_validate(t *testing.T) { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, HighThroughputFifo: aws.Bool(true), - DeduplicationScope: aws.String("messageGroup"), - Type: aws.String("fifo"), + DeduplicationScope: aws.String(sqsFifoMessageGroup), + Type: aws.String(sqsFifoQueueType), + }, + }, + }, + wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput_fifo" and "deduplication_scope"`), + }, + "should return error if high_throughput_fifo is defined along with fifo_throughput_limit": { + in: TopicSubscription{ + Name: aws.String("mockTopic"), + Service: aws.String("mockservice"), + Queue: SQSQueueOrBool{ + Advanced: SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + HighThroughputFifo: aws.Bool(true), + FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + Type: aws.String(sqsFifoQueueType), }, }, }, - wanted: errors.New(`validate "queue": validate "high_throughput_fifo": cannot define fifo_throughput_limit and fifo_throughput_limit when high_throughput_fifo is defined`), + wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput_fifo" and "fifo_throughput_limit"`), }, - "should return error if invalid combination of fifo_throughput_limit and fifo_throughput_limit is defined": { + "should return error if invalid combination of deduplication_scope and fifo_throughput_limit is defined": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), @@ -2914,13 +2944,13 @@ func TestTopicSubscription_validate(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String("perMessageGroupId"), - DeduplicationScope: aws.String("queue"), - Type: aws.String("fifo"), + FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + DeduplicationScope: aws.String(sqsFifoQueue), + Type: aws.String(sqsFifoQueueType), }, }, }, - wanted: errors.New(`validate "queue": when deduplication_scope scope is set to queue, fifo_throughput_limit must be set to perQueue`), + wanted: errors.New(`validate "queue": "fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`), }, "should not return error if valid standard queue config defined": { in: TopicSubscription{ diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 9a8d3b74d09..c14e18fdf51 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -228,15 +228,15 @@ func (s *WorkerService) RetrofitFIFOConfig() { } // if condition sets topic specific type to defaultQueueType (i.e. standard) in case customer doesn't define it in their topic specific queue. if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type == nil { - s.Subscribe.Topics[idx].Queue.Advanced.Type = aws.String(defaultQueueType) + s.Subscribe.Topics[idx].Queue.Advanced.Type = aws.String(sqsDefaultQueueType) } } if s.Subscribe.Queue.IsEmpty() && s.Subscribe.Topics != nil { s.Subscribe.Queue = SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), } } else if !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type == nil { - s.Subscribe.Queue.Type = aws.String(defaultQueueType) + s.Subscribe.Queue.Type = aws.String(sqsDefaultQueueType) } } diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 50e12c020d9..bce21288104 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -43,7 +43,7 @@ func newMockSQSFIFOQueue() SQSQueue { Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FifoThroughputLimit: aws.String("perMessageID"), - Type: aws.String(fifoQueueType), + Type: aws.String(sqsFifoQueueType), } } @@ -1330,13 +1330,13 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, }, Queue: SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1367,7 +1367,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { }, }, Queue: SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1396,7 +1396,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { }, }, Queue: SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1430,7 +1430,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1497,7 +1497,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1545,7 +1545,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { }, }, Queue: SQSQueue{ - Type: aws.String(defaultQueueType), + Type: aws.String(sqsDefaultQueueType), }, }, }, diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index 73b4e7e69f1..f680b6278c4 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -22,13 +22,16 @@ import ( const ( defaultDockerfileName = "Dockerfile" - perMessageGroupID = "perMessageGroupId" - perQueue = "perQueue" - messageGroup = "messageGroup" - queue = "queue" - standardQueueType = "standard" - fifoQueueType = "fifo" - defaultQueueType = standardQueueType +) + +const ( + sqsFifoPerMessageGroupID = "perMessageGroupId" + sqsFifoPerQueue = "perQueue" + sqsFifoMessageGroup = "messageGroup" + sqsFifoQueue = "queue" + sqsStandardQueueType = "standard" + sqsFifoQueueType = "fifo" + sqsDefaultQueueType = sqsStandardQueueType ) const ( @@ -39,7 +42,8 @@ const ( // All placement options. var ( - subnetPlacements = []string{string(PublicSubnetPlacement), string(PrivateSubnetPlacement)} + subnetPlacements = []string{string(PublicSubnetPlacement), string(PrivateSubnetPlacement)} + sqsValidQueueTypeValues = []string{sqsStandardQueueType, sqsFifoQueueType} ) // Error definitions. From 76cf69fb587c025cfe8b49b11b1f37bdaf47ed93 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 20 Sep 2022 15:36:03 -0700 Subject: [PATCH 09/19] reduce nesting --- .../cloudformation/stack/transformers.go | 44 +++++++------------ internal/pkg/manifest/workload.go | 1 + 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 6498efb736b..4356f53ebfe 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -29,6 +29,7 @@ const ( disabled = "DISABLED" ) +// SQS Queue configs const ( sqsStandardQueueType = "standard" sqsFifoQueueType = "fifo" @@ -876,39 +877,26 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { return nil } - if aws.StringValue(q.Type) == sqsStandardQueueType { - return &template.SQSQueue{ - Retention: convertRetention(q.Retention), - Delay: convertDelay(q.Delay), - Timeout: convertTimeout(q.Timeout), - DeadLetter: convertDeadLetter(q.DeadLetter), - Type: q.Type, - } + queue := &template.SQSQueue{ + Retention: convertRetention(q.Retention), + Delay: convertDelay(q.Delay), + Timeout: convertTimeout(q.Timeout), + DeadLetter: convertDeadLetter(q.DeadLetter), + Type: q.Type, } - var fifoThroughputLimit, deduplicationScope *string - if !aws.BoolValue(q.HighThroughputFifo) { - if q.FifoThroughputLimit != nil { - fifoThroughputLimit = q.FifoThroughputLimit - } - if q.DeduplicationScope != nil { - deduplicationScope = q.DeduplicationScope - } - } else if aws.BoolValue(q.HighThroughputFifo) { - fifoThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) - deduplicationScope = aws.String(sqsDedupeScopeMessageGroup) + if aws.StringValue(q.Type) == sqsStandardQueueType { + return queue } - return &template.SQSQueue{ - Retention: convertRetention(q.Retention), - Delay: convertDelay(q.Delay), - Timeout: convertTimeout(q.Timeout), - DeadLetter: convertDeadLetter(q.DeadLetter), - FifoThroughputLimit: fifoThroughputLimit, - ContentBasedDeduplication: q.ContentBasedDeduplication, - DeduplicationScope: deduplicationScope, - Type: q.Type, + queue.ContentBasedDeduplication = q.ContentBasedDeduplication + queue.DeduplicationScope = q.DeduplicationScope + queue.FifoThroughputLimit = q.FifoThroughputLimit + if aws.BoolValue(q.HighThroughputFifo) { + queue.FifoThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) + queue.DeduplicationScope = aws.String(sqsDedupeScopeMessageGroup) } + return queue } func convertTime(t *time.Duration) *int64 { diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index f680b6278c4..d199f8a46dd 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -24,6 +24,7 @@ const ( defaultDockerfileName = "Dockerfile" ) +// SQS Queue configs const ( sqsFifoPerMessageGroupID = "perMessageGroupId" sqsFifoPerQueue = "perQueue" From 09792796d77767994fcf0ad05979f1b6db26b5e8 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 21 Sep 2022 15:05:26 -0700 Subject: [PATCH 10/19] add fifo field instead of type field --- .../cloudformation/stack/transformers.go | 48 +++-- .../cloudformation/stack/transformers_test.go | 184 ++++++++---------- .../deploy/cloudformation/stack/worker_svc.go | 1 + internal/pkg/manifest/validate.go | 50 ++--- internal/pkg/manifest/validate_test.go | 151 +++++++------- internal/pkg/manifest/worker_svc.go | 92 +++++---- internal/pkg/manifest/worker_svc_test.go | 34 ++-- internal/pkg/manifest/workload.go | 1 - internal/pkg/manifest/workload_test.go | 1 - internal/pkg/template/template_functions.go | 8 +- .../workloads/partials/cf/subscribe.yml | 50 ++--- internal/pkg/template/workload.go | 23 ++- 12 files changed, 324 insertions(+), 319 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 4356f53ebfe..af493f65ebe 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -808,7 +808,6 @@ func convertPublish(topics []manifest.Topic, accountID, region, app, env, svc st for _, topic := range topics { publishers.Topics = append(publishers.Topics, &template.Topic{ Name: topic.Name, - Type: aws.StringValue(topic.Type), AccountID: accountID, Partition: partition.ID(), Region: region, @@ -845,11 +844,9 @@ func convertTopicSubscription(t manifest.TopicSubscription) ( } if aws.BoolValue(t.Queue.Enabled) { return &template.TopicSubscription{ - Name: t.Name, - Service: t.Service, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Name: t.Name, + Service: t.Service, + Queue: &template.SQSQueue{}, FilterPolicy: filterPolicy, }, nil } @@ -882,20 +879,43 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { Delay: convertDelay(q.Delay), Timeout: convertTimeout(q.Timeout), DeadLetter: convertDeadLetter(q.DeadLetter), - Type: q.Type, } - if aws.StringValue(q.Type) == sqsStandardQueueType { + if q.FIFO.IsEmpty() { + return queue + } + + if aws.BoolValue(q.FIFO.Enable) { + queue.FIFOQueueConfig = &template.FIFOQueueConfig{ + Enable: nil, + } + queue.FIFOQueueConfig.Enable = aws.Bool(true) return queue } - queue.ContentBasedDeduplication = q.ContentBasedDeduplication - queue.DeduplicationScope = q.DeduplicationScope - queue.FifoThroughputLimit = q.FifoThroughputLimit - if aws.BoolValue(q.HighThroughputFifo) { - queue.FifoThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) - queue.DeduplicationScope = aws.String(sqsDedupeScopeMessageGroup) + if !q.FIFO.Advanced.IsEmpty() { + queue.FIFOQueueConfig = &template.FIFOQueueConfig{ + Advanced: &template.FIFOAdvanceConfig{ + ContentBasedDeduplication: nil, + DeduplicationScope: nil, + FIFOThroughputLimit: nil, + }, + } + if q.FIFO.Advanced.ContentBasedDeduplication != nil { + queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication = q.FIFO.Advanced.ContentBasedDeduplication + } + if q.FIFO.Advanced.DeduplicationScope != nil { + queue.FIFOQueueConfig.Advanced.DeduplicationScope = q.FIFO.Advanced.DeduplicationScope + } + if q.FIFO.Advanced.FIFOThroughputLimit != nil { + queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit = q.FIFO.Advanced.FIFOThroughputLimit + } + if aws.BoolValue(q.FIFO.Advanced.HighThroughputFifo) { + queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) + queue.FIFOQueueConfig.Advanced.DeduplicationScope = aws.String(sqsDedupeScopeMessageGroup) + } } + return queue } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 48c4b793921..8b909075c00 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1374,18 +1374,15 @@ func Test_convertPublish(t *testing.T) { inTopics: []manifest.Topic{ { Name: aws.String("topic1"), - Type: aws.String("standard"), }, { Name: aws.String("topic2"), - Type: aws.String("standard"), }, }, wanted: &template.PublishOpts{ Topics: []*template.Topic{ { Name: aws.String("topic1"), - Type: "standard", AccountID: accountId, Partition: partition, Region: region, @@ -1396,7 +1393,6 @@ func Test_convertPublish(t *testing.T) { { Name: aws.String("topic2"), - Type: "standard", AccountID: accountId, Partition: partition, Region: region, @@ -1411,18 +1407,15 @@ func Test_convertPublish(t *testing.T) { inTopics: []manifest.Topic{ { Name: aws.String("topic1"), - Type: aws.String("fifo"), }, { Name: aws.String("topic2"), - Type: aws.String("standard"), }, }, wanted: &template.PublishOpts{ Topics: []*template.Topic{ { Name: aws.String("topic1"), - Type: "fifo", AccountID: accountId, Partition: partition, Region: region, @@ -1433,7 +1426,6 @@ func Test_convertPublish(t *testing.T) { { Name: aws.String("topic2"), - Type: "standard", AccountID: accountId, Partition: partition, Region: region, @@ -1486,7 +1478,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, }, wanted: &template.SubscribeOpts{ @@ -1503,7 +1494,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, }, }, @@ -1515,9 +1505,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1526,9 +1514,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with queue enabled": { //4 @@ -1543,24 +1529,18 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: &template.SQSQueue{ - Type: aws.String(sqsStandardQueueType), - }, + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: &template.SQSQueue{}, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with minimal queue": { //5 @@ -1577,15 +1557,12 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1599,14 +1576,11 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with high throughput fifo sqs": { //6 @@ -1623,16 +1597,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsFifoQueueType), - HighThroughputFifo: aws.Bool(true), + FIFO: manifest.FIFOAdvanceConfigOrBool{Advanced: manifest.FIFOAdvanceConfig{HighThroughputFifo: aws.Bool(true)}}, }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1646,16 +1617,17 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsFifoQueueType), - FifoThroughputLimit: aws.String("perMessageGroupId"), - DeduplicationScope: aws.String("messageGroup"), + FIFOQueueConfig: &template.FIFOQueueConfig{ + Advanced: &template.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("perMessageGroupId"), + DeduplicationScope: aws.String("messageGroup"), + }, + }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with custom minimal fifo sqs config values": { @@ -1672,15 +1644,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1694,17 +1664,15 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: nil, - DeduplicationScope: nil, - ContentBasedDeduplication: nil, - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{ + Enable: aws.Bool(true), + Advanced: nil, + }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with custom complete fifo sqs config values": { //7 @@ -1721,18 +1689,19 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, FilterPolicy: mockStruct, }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1746,17 +1715,18 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{ + Advanced: &template.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with custom complete fifo sqs config and standard topic subscription to a default standard queue": { //8 @@ -1773,10 +1743,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, FilterPolicy: mockStruct, @@ -1786,9 +1759,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1802,10 +1773,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{ + Advanced: &template.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1814,9 +1788,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: nil, }, }, "valid subscribe with custom complete fifo sqs config and multiple standard topic subscriptions to a default standard queue": { //9 @@ -1833,10 +1805,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, FilterPolicy: mockStruct, @@ -1850,9 +1825,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: manifest.SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1866,10 +1839,13 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FifoThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{ + Advanced: &template.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1882,9 +1858,7 @@ func Test_convertSubscribe(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: &template.SQSQueue{ - Type: aws.String(sqsStandardQueueType), - }, + Queue: nil, }, }, "valid subscribe with standard sqs config and fifo topic subscription to a default fifo queue": { //10 @@ -1901,7 +1875,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, @@ -1912,7 +1885,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, }, }, wanted: &template.SubscribeOpts{ @@ -1927,7 +1900,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1937,7 +1909,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{Enable: aws.Bool(true)}, }, }, }, @@ -1955,7 +1927,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: manifest.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, }, FilterPolicy: mockStruct, @@ -1970,7 +1941,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: manifest.SQSQueue{ - Type: aws.String(sqsFifoQueueType), + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, }, }, wanted: &template.SubscribeOpts{ @@ -1985,7 +1956,6 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - Type: aws.String(sqsStandardQueueType), }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1999,7 +1969,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - Type: aws.String(sqsFifoQueueType), + FIFOQueueConfig: &template.FIFOQueueConfig{Enable: aws.Bool(true)}, }, }, }, diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index 73bb4cce9d4..684a281448c 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -153,6 +153,7 @@ func (s *WorkerService) Template() (string, error) { if err != nil { return "", fmt.Errorf("apply task definition overrides: %w", err) } + fmt.Println(string(overriddenTpl)) return string(overriddenTpl), nil } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 9a266516bf6..ddec008b3b1 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1414,9 +1414,6 @@ func (p PublishConfig) validate() error { // validate returns nil if Topic is configured correctly. func (t Topic) validate() error { - if t.Type != nil && !contains(aws.StringValue(t.Type), validTopicsTypeValues) { - return fmt.Errorf(`"type" value %q is not allowed; must be one of %s`, aws.StringValue(t.Type), english.WordSeries(validTopicsTypeValues, "or")) - } return validatePubSubName(aws.StringValue(t.Name)) } @@ -1461,33 +1458,30 @@ func (q SQSQueueOrBool) validate() error { if q.IsEmpty() { return nil } - if q.Enabled != nil { return nil } - if q.validateType() { - return fmt.Errorf(`validate "type": type value must be one of %s`, english.WordSeries(sqsValidQueueTypeValues, "or")) - } if q.isFIFO() { + if q.Advanced.FIFO.Enable != nil { + return nil + } return q.Advanced.validateFIFO() } - if q.hasFIFOFieldsEnabledOnStandardQueue() { - return fmt.Errorf(`parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`) - } return q.Advanced.validate() } func (q SQSQueueOrBool) isFIFO() bool { - return q.Advanced.Type != nil && aws.StringValue(q.Advanced.Type) == sqsFifoQueueType + return !q.Advanced.IsEmpty() && !q.Advanced.FIFO.IsEmpty() } -func (q SQSQueueOrBool) validateType() bool { - return q.Advanced.Type != nil && !contains(aws.StringValue(q.Advanced.Type), sqsValidQueueTypeValues) +// no-op for now. +func (q FIFOAdvanceConfigOrBool) validate() error { + return nil } -func (q SQSQueueOrBool) hasFIFOFieldsEnabledOnStandardQueue() bool { - return q.Advanced.FifoThroughputLimit != nil || q.Advanced.HighThroughputFifo != nil || - q.Advanced.DeduplicationScope != nil || q.Advanced.ContentBasedDeduplication != nil +func (q FIFOAdvanceConfig) hasFIFOFieldsEnabledOnStandardQueue() bool { + return q.FIFOThroughputLimit != nil || q.HighThroughputFifo != nil || + q.DeduplicationScope != nil || q.ContentBasedDeduplication != nil } // validate returns nil if SQSQueue is configured correctly. @@ -1501,8 +1495,8 @@ func (q SQSQueue) validate() error { return nil } -// validateFIFO returns nil if FIFO SQSQueue is configured correctly. -func (q SQSQueue) validateFIFO() error { +// validate returns nil if SQSQueue is configured correctly. +func (q FIFOAdvanceConfig) validate() error { if q.IsEmpty() { return nil } @@ -1516,18 +1510,26 @@ func (q SQSQueue) validateFIFO() error { if err := q.validateFifoThroughputLimit(); err != nil { return err } - if aws.StringValue(q.FifoThroughputLimit) == sqsFifoPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsFifoQueue { + if aws.StringValue(q.FIFOThroughputLimit) == sqsFifoPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsFifoQueue { return fmt.Errorf(`"fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`) } + return nil +} + +// validateFIFO returns nil if FIFO SQSQueue is configured correctly. +func (q SQSQueue) validateFIFO() error { + if err := q.FIFO.Advanced.validate(); err != nil { + return err + } if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) } return nil } -func (q SQSQueue) validateHighThroughputFifo() error { +func (q FIFOAdvanceConfig) validateHighThroughputFifo() error { if q.HighThroughputFifo != nil { - if q.FifoThroughputLimit != nil { + if q.FIFOThroughputLimit != nil { return &errFieldMutualExclusive{ firstField: "high_throughput_fifo", secondField: "fifo_throughput_limit", @@ -1546,15 +1548,15 @@ func (q SQSQueue) validateHighThroughputFifo() error { return nil } -func (q SQSQueue) validateDeduplicationScope() error { +func (q FIFOAdvanceConfig) validateDeduplicationScope() error { if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) } return nil } -func (q SQSQueue) validateFifoThroughputLimit() error { - if q.FifoThroughputLimit != nil && !contains(aws.StringValue(q.FifoThroughputLimit), validFIFOThroughputLimitValues) { +func (q FIFOAdvanceConfig) validateFifoThroughputLimit() error { + if q.FIFOThroughputLimit != nil && !contains(aws.StringValue(q.FIFOThroughputLimit), validFIFOThroughputLimitValues) { return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) } return nil diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index d2038d6db91..6a0e5a1d1a7 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -580,22 +580,6 @@ func TestBackendService_validate(t *testing.T) { }, wantedErrorMsgPrefix: `validate "publish": `, }, - "error if invalid type is defined": { - config: BackendService{ - BackendServiceConfig: BackendServiceConfig{ - ImageConfig: testImageConfig, - PublishConfig: PublishConfig{ - Topics: []Topic{ - { - Name: aws.String("mytopic"), - Type: aws.String("incorrectValue"), - }, - }, - }, - }, - }, - wantedErrorMsgPrefix: `validate "publish": validate "topics[0]": "type" value "incorrectValue" is not allowed`, - }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -2802,17 +2786,19 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: nil, }, - "should return error if invalid queue type is provided": { + "should not return error if fifo queue is enabled": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Type: aws.String("FIFO"), // it is case-sensitive, correct values are standard or fifo. + FIFO: FIFOAdvanceConfigOrBool{ + Enable: aws.Bool(true), + }, }, }, }, - wanted: errors.New(`validate "queue": validate "type": type value must be one of standard or fifo`), + wanted: nil, }, "should return error if invalid fifo throughput limit": { in: TopicSubscription{ @@ -2820,12 +2806,15 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String("incorrectFIFOThoughoutLimit"), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("incorrectFIFOThoughoutLimit"), + }, + }, }, }, }, @@ -2837,12 +2826,15 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + }, + }, }, }, }, @@ -2854,12 +2846,15 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - DeduplicationScope: aws.String("incorrectDeduplicateScope"), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + DeduplicationScope: aws.String("incorrectDeduplicateScope"), + }, + }, }, }, }, @@ -2871,46 +2866,36 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - DeduplicationScope: aws.String(sqsFifoMessageGroup), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + DeduplicationScope: aws.String(sqsFifoMessageGroup), + }, + }, }, }, }, wanted: nil, }, - "should return error if standard queue config has fifo queue parameters defined": { - in: TopicSubscription{ - Name: aws.String("mockTopic"), - Service: aws.String("mockservice"), - Queue: SQSQueueOrBool{ - Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - DeduplicationScope: aws.String(sqsFifoMessageGroup), - }, - }, - }, - wanted: errors.New(`validate "queue": parameters such as "content_based_deduplication", "deduplication_scope", "fifo_throughput_limit", and "high_throughput_fifo" are only used with FIFO SQS Queue; to enable fifo SQS add type: fifo in your topic specific queue`), - }, "should return error if high_throughput_fifo is defined along with deduplication_scope": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - HighThroughputFifo: aws.Bool(true), - DeduplicationScope: aws.String(sqsFifoMessageGroup), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + HighThroughputFifo: aws.Bool(true), + DeduplicationScope: aws.String(sqsFifoMessageGroup), + }, + }, }, }, }, @@ -2922,13 +2907,16 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - HighThroughputFifo: aws.Bool(true), - FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + HighThroughputFifo: aws.Bool(true), + FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + }, + }, }, }, }, @@ -2940,13 +2928,16 @@ func TestTopicSubscription_validate(t *testing.T) { Service: aws.String("mockservice"), Queue: SQSQueueOrBool{ Advanced: SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String(sqsFifoPerMessageGroupID), - DeduplicationScope: aws.String(sqsFifoQueue), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + DeduplicationScope: aws.String(sqsFifoQueue), + }, + }, }, }, }, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index c14e18fdf51..0891115517b 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -5,7 +5,6 @@ package manifest import ( "errors" - "strings" "time" "github.com/aws/aws-sdk-go/aws" @@ -22,7 +21,8 @@ const ( ) var ( - errUnmarshalQueueOpts = errors.New(`cannot unmarshal "queue" field into bool or map`) + errUnmarshalQueueOpts = errors.New(`cannot unmarshal "queue" field into bool or map`) + errUnmarshalFifoConfig = errors.New("unable to unmarshal fifo field into boolean or compose-style map") ) // WorkerService holds the configuration to create a worker service. @@ -90,18 +90,6 @@ func (q *SQSQueueOrBool) IsEmpty() bool { return q.Advanced.IsEmpty() && q.Enabled == nil } -// UnmarshalYAML implements the yaml(v3) interface. Here it sets default value -// of the type field if it is not set already. -func (t *Topic) UnmarshalYAML(value *yaml.Node) error { - t.Type = aws.String(defaultTopicType) - - type plain Topic - if err := value.Decode((*plain)(t)); err != nil { - return err - } - return nil -} - // UnmarshalYAML implements the yaml(v3) interface. It allows SQSQueueOrBool to be specified as a // string or a struct alternately. func (q *SQSQueueOrBool) UnmarshalYAML(value *yaml.Node) error { @@ -126,22 +114,61 @@ func (q *SQSQueueOrBool) UnmarshalYAML(value *yaml.Node) error { // SQSQueue represents the configurable options for setting up a SQS Queue. type SQSQueue struct { - Retention *time.Duration `yaml:"retention"` - Delay *time.Duration `yaml:"delay"` - Timeout *time.Duration `yaml:"timeout"` - DeadLetter DeadLetterQueue `yaml:"dead_letter"` - ContentBasedDeduplication *bool `yaml:"content_based_deduplication"` - DeduplicationScope *string `yaml:"deduplication_scope"` - FifoThroughputLimit *string `yaml:"fifo_throughput_limit"` - HighThroughputFifo *bool `yaml:"high_throughput_fifo"` - Type *string `yaml:"type"` + Retention *time.Duration `yaml:"retention"` + Delay *time.Duration `yaml:"delay"` + Timeout *time.Duration `yaml:"timeout"` + DeadLetter DeadLetterQueue `yaml:"dead_letter"` + FIFO FIFOAdvanceConfigOrBool `yaml:"fifo"` +} + +// FIFOAdvanceConfigOrBool represents the configurable options for fifo queues. +type FIFOAdvanceConfigOrBool struct { + Enable *bool + Advanced FIFOAdvanceConfig +} + +// IsEmpty returns true if the FifoAdvanceConfigOrBool struct has all nil values. +func (f *FIFOAdvanceConfigOrBool) IsEmpty() bool { + return f.Enable == nil && f.Advanced.IsEmpty() +} + +// FifoAdvanceConfig represents the advanced fifo queue config. +type FIFOAdvanceConfig struct { + ContentBasedDeduplication *bool `yaml:"content_based_deduplication"` + DeduplicationScope *string `yaml:"deduplication_scope"` + FIFOThroughputLimit *string `yaml:"fifo_throughput_limit"` + HighThroughputFifo *bool `yaml:"high_throughput_fifo"` +} + +// IsEmpty returns true if the FifoAdvanceConfig struct has all nil values. +func (f *FIFOAdvanceConfig) IsEmpty() bool { + return f.FIFOThroughputLimit == nil && f.HighThroughputFifo == nil && + f.DeduplicationScope == nil && f.ContentBasedDeduplication == nil +} + +// UnmarshalYAML overrides the default YAML unmarshaling logic for the FifoAdvanceConfigOrBool +// struct, allowing it to perform more complex unmarshaling behavior. +// This method implements the yaml.Unmarshaler (v3) interface. +func (t *FIFOAdvanceConfigOrBool) UnmarshalYAML(value *yaml.Node) error { + if err := value.Decode(&t.Advanced); err != nil { + var yamlTypeErr *yaml.TypeError + if !errors.As(err, &yamlTypeErr) { + return err + } + } + if !t.Advanced.IsEmpty() { + return nil + } + if err := value.Decode(&t.Enable); err != nil { + return errUnmarshalFifoConfig + } + return nil } // IsEmpty returns empty if the struct has all zero members. func (q *SQSQueue) IsEmpty() bool { return q.Retention == nil && q.Delay == nil && q.Timeout == nil && - q.DeadLetter.IsEmpty() && q.FifoThroughputLimit == nil && q.HighThroughputFifo == nil && - q.DeduplicationScope == nil && q.ContentBasedDeduplication == nil && q.Type == nil + q.DeadLetter.IsEmpty() && q.FIFO.IsEmpty() } // DeadLetterQueue represents the configurable options for setting up a Dead-Letter Queue. @@ -221,22 +248,11 @@ func (s *WorkerService) Subscriptions() []TopicSubscription { func (s *WorkerService) RetrofitFIFOConfig() { for idx, topic := range s.Subscribe.Topics { // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. - if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type != nil && strings.Compare(aws.StringValue(s.Subscribe.Queue.Type), "fifo") == 0 { + if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && !s.Subscribe.Queue.FIFO.IsEmpty() { s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") - } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type != nil && strings.Compare(aws.StringValue(topic.Queue.Advanced.Type), "fifo") == 0 { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. + } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && !topic.Queue.Advanced.FIFO.IsEmpty() { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") } - // if condition sets topic specific type to defaultQueueType (i.e. standard) in case customer doesn't define it in their topic specific queue. - if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.Type == nil { - s.Subscribe.Topics[idx].Queue.Advanced.Type = aws.String(sqsDefaultQueueType) - } - } - if s.Subscribe.Queue.IsEmpty() && s.Subscribe.Topics != nil { - s.Subscribe.Queue = SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - } - } else if !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.Type == nil { - s.Subscribe.Queue.Type = aws.String(sqsDefaultQueueType) } } diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index bce21288104..2193a1335a3 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -38,12 +38,15 @@ func newMockSQSFIFOQueueOrBool() SQSQueueOrBool { func newMockSQSFIFOQueue() SQSQueue { duration111Seconds := 111 * time.Second return SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - FifoThroughputLimit: aws.String("perMessageID"), - Type: aws.String(sqsFifoQueueType), + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, + FIFO: FIFOAdvanceConfigOrBool{ + Advanced: FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("perMessageID"), + }, + }, } } @@ -1330,14 +1333,11 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(sqsDefaultQueueType), }, }, }, }, - Queue: SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: SQSQueue{}, }, }, }, @@ -1366,9 +1366,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Queue: newMockSQSFIFOQueueOrBool(), }, }, - Queue: SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: SQSQueue{}, }, }, }, @@ -1395,9 +1393,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Service: aws.String("svc"), }, }, - Queue: SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: SQSQueue{}, }, }, }, @@ -1430,7 +1426,6 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1497,7 +1492,6 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Delay: &duration111Seconds, Timeout: &duration111Seconds, DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, - Type: aws.String(sqsDefaultQueueType), }, }, }, @@ -1544,9 +1538,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { Queue: newMockSQSFIFOQueueOrBool(), }, }, - Queue: SQSQueue{ - Type: aws.String(sqsDefaultQueueType), - }, + Queue: SQSQueue{}, }, }, }, diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index d199f8a46dd..f6ceb7e7bcd 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -412,7 +412,6 @@ type PublishConfig struct { // Topic represents the configurable options for setting up a SNS Topic. type Topic struct { Name *string `yaml:"name"` - Type *string `yaml:"type"` } // NetworkConfig represents options for network connection to AWS resources within a VPC. diff --git a/internal/pkg/manifest/workload_test.go b/internal/pkg/manifest/workload_test.go index c388e8208dc..764e02b3222 100644 --- a/internal/pkg/manifest/workload_test.go +++ b/internal/pkg/manifest/workload_test.go @@ -1069,7 +1069,6 @@ topics: Topics: []Topic{ { Name: aws.String("tests"), - Type: aws.String("standard"), }, }, }, diff --git a/internal/pkg/template/template_functions.go b/internal/pkg/template/template_functions.go index 5a6ea23bad9..471520ac0d6 100644 --- a/internal/pkg/template/template_functions.go +++ b/internal/pkg/template/template_functions.go @@ -59,15 +59,17 @@ func EnvVarNameFunc(s string) string { return StripNonAlphaNumFunc(s) + "Name" } +// IsFIFO checks if the given queue has FIFO config. +func (s SQSQueue) IsFIFO() bool { + return s.FIFOQueueConfig != nil +} + // EnvVarSecretFunc converts an input resource name to LogicalIDSafe, then appends // "Secret" to the end. func EnvVarSecretFunc(s string) string { return StripNonAlphaNumFunc(s) + "Secret" } -// Deref dereferences the pointer. -func Deref(i *string) string { return *i } - // Grabs word boundaries in default CamelCase. Matches lowercase letters & numbers // before the next capital as capturing group 1, and the first capital in the // next word as capturing group 2. Will match "yC" in "MyCamel" and "y2ndC" in"My2ndCamel" diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index 984f04b1c8e..24f3f1cf539 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -60,7 +60,7 @@ EventsKMSKey: EventsQueue: Metadata: - 'aws:copilot:description': {{ if and .Subscribe .Subscribe.Queue (eq (deref .Subscribe.Queue.Type) "fifo")}}'An events SQS FIFO queue to buffer messages'{{ else}}'An events SQS queue to buffer messages'{{ end}} + 'aws:copilot:description': {{ if and .Subscribe .Subscribe.Queue .Subscribe.Queue.IsFIFO }}'An events SQS FIFO queue to buffer messages'{{ else}}'An events SQS queue to buffer messages'{{ end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -80,29 +80,31 @@ EventsQueue: deadLetterTargetArn: !GetAtt DeadLetterQueue.Arn maxReceiveCount: {{.Subscribe.Queue.DeadLetter.Tries}} {{- end}} - {{- if eq (deref .Subscribe.Queue.Type) "fifo"}} + {{- if .Subscribe.Queue.IsFIFO }} FifoQueue: true + {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced }} + {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit }} + FifoThroughputLimit: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit}} {{- end}} - {{- if .Subscribe.Queue.FifoThroughputLimit }} - FifoThroughputLimit: {{.Subscribe.Queue.FifoThroughputLimit}} - {{- end}} - {{- if .Subscribe.Queue.DeduplicationScope }} - DeduplicationScope: {{.Subscribe.Queue.DeduplicationScope}} + {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.DeduplicationScope }} + DeduplicationScope: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.DeduplicationScope}} {{- end }} - {{- if .Subscribe.Queue.ContentBasedDeduplication }} - ContentBasedDeduplication: {{.Subscribe.Queue.ContentBasedDeduplication}} + {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication }} + ContentBasedDeduplication: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication}} + {{- end}} + {{- end}} {{- end}} {{- end}} {{- if .Subscribe.Queue}}{{- if .Subscribe.Queue.DeadLetter}} DeadLetterQueue: Metadata: - 'aws:copilot:description': {{ if eq (deref .Subscribe.Queue.Type) "fifo"}}'A dead letter SQS FIFO queue to buffer failed messages from the events queue'{{ else}} 'A dead letter SQS queue to buffer failed messages from the events queue'{{ end}} + 'aws:copilot:description': {{ if .Subscribe.Queue.IsFIFO }}'A dead letter SQS FIFO queue to buffer failed messages from the events queue'{{ else}} 'A dead letter SQS queue to buffer failed messages from the events queue'{{ end}} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days - {{- if eq (deref .Subscribe.Queue.Type) "fifo" }} + {{- if .Subscribe.Queue.IsFIFO}} FifoQueue: true {{- end }} @@ -178,7 +180,7 @@ QueuePolicy: {{- if $topic.Queue}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}EventsQueue: Metadata: - 'aws:copilot:description': {{ if eq (deref $topic.Queue.Type) "fifo"}}'A SQS FIFO queue to buffer messages from the topic {{$topic.Name}}' {{ else }} 'A SQS queue to buffer messages from the topic {{$topic.Name}}' {{ end }} + 'aws:copilot:description': {{ if $topic.Queue.IsFIFO }}'A SQS FIFO queue to buffer messages from the topic {{$topic.Name}}' {{ else }} 'A SQS queue to buffer messages from the topic {{$topic.Name}}' {{ end }} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey @@ -196,28 +198,30 @@ QueuePolicy: deadLetterTargetArn: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue.Arn maxReceiveCount: {{$topic.Queue.DeadLetter.Tries}} {{- end}} - {{- if eq (deref $topic.Queue.Type) "fifo"}} + {{- if $topic.Queue.IsFIFO }} FifoQueue: true + {{- if $topic.Queue.FIFOQueueConfig.Advanced}} + {{- if $topic.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit }} + FifoThroughputLimit: {{$topic.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit}} {{- end}} - {{- if $topic.Queue.FifoThroughputLimit }} - FifoThroughputLimit: {{$topic.Queue.FifoThroughputLimit}} - {{- end}} - {{- if $topic.Queue.DeduplicationScope }} - DeduplicationScope: {{$topic.Queue.DeduplicationScope}} + {{- if $topic.Queue.FIFOQueueConfig.Advanced.DeduplicationScope }} + DeduplicationScope: {{$topic.Queue.FIFOQueueConfig.Advanced.DeduplicationScope}} {{- end }} - {{- if $topic.Queue.ContentBasedDeduplication }} - ContentBasedDeduplication: {{$topic.Queue.ContentBasedDeduplication}} + {{- if $topic.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication }} + ContentBasedDeduplication: {{$topic.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication}} + {{- end}} + {{- end}} {{- end}} {{- if $topic.Queue.DeadLetter}} {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}DeadLetterQueue: Metadata: - 'aws:copilot:description': {{ if eq (deref $topic.Queue.Type) "fifo"}} 'A dead letter SQS FIFO queue to buffer failed messages from the topic {{$topic.Name}}' {{ else }} 'A dead letter SQS queue to buffer failed messages from the topic {{$topic.Name}}' {{ end }} + 'aws:copilot:description': {{ if $topic.Queue.IsFIFO }} 'A dead letter SQS FIFO queue to buffer failed messages from the topic {{$topic.Name}}' {{ else }} 'A dead letter SQS queue to buffer failed messages from the topic {{$topic.Name}}' {{ end }} Type: AWS::SQS::Queue Properties: KmsMasterKeyId: !Ref EventsKMSKey MessageRetentionPeriod: 1209600 # 14 days - {{- if eq (deref $topic.Queue.Type) "fifo"}} + {{- if $topic.Queue.IsFIFO}} FifoQueue: true {{- end}} @@ -261,7 +265,7 @@ QueuePolicy: Resource: !GetAtt {{logicalIDSafe $topic.Service}}{{logicalIDSafe $topic.Name}}EventsQueue.Arn Condition: ArnEquals: - aws:SourceArn: {{ if eq (deref $topic.Queue.Type) "fifo"}} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{$topic.Name}}']] {{ else }} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] {{ end }} + aws:SourceArn: {{ if $topic.Queue.IsFIFO }} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{$topic.Name}}']] {{ else }} !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-{{$topic.Service}}-{{logicalIDSafe $topic.Name}}']] {{ end }} {{- end}}{{/* endif $topic.Queue */}} {{- end}}{{/* endrange $topic := .Subscribe.Topics */}} {{- end}}{{/* if .Subscribe */}} \ No newline at end of file diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 86990d8a863..3936b7c2469 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -425,14 +425,24 @@ type TopicSubscription struct { // SQSQueue holds information needed to render a SQS Queue in a container definition. type SQSQueue struct { - Retention *int64 - Delay *int64 - Timeout *int64 - DeadLetter *DeadLetterQueue - FifoThroughputLimit *string + Retention *int64 + Delay *int64 + Timeout *int64 + DeadLetter *DeadLetterQueue + FIFOQueueConfig *FIFOQueueConfig +} + +// FifoAdvanceConfigOrBool holds information needed to render a FIFO SQS Queue in a container definition. +type FIFOQueueConfig struct { + Enable *bool + Advanced *FIFOAdvanceConfig +} + +// FifoAdvanceConfig holds information needed to render a advanced FIFO SQS Queue in a container definition. +type FIFOAdvanceConfig struct { + FIFOThroughputLimit *string ContentBasedDeduplication *bool DeduplicationScope *string - Type *string } // DeadLetterQueue holds information needed to render a dead-letter SQS Queue in a container definition. @@ -633,7 +643,6 @@ func withSvcParsingFuncs() ParseOption { "pluralWord": english.PluralWord, "contains": contains, "requiresVPCConnector": requiresVPCConnector, - "deref": Deref, }) } } From 31e69a279c69a5a56af96d37572804084b80c11d Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 21 Sep 2022 18:00:45 -0700 Subject: [PATCH 11/19] resolve static check --- .../pkg/deploy/cloudformation/stack/transformers.go | 3 --- internal/pkg/manifest/validate.go | 10 ++-------- internal/pkg/manifest/worker_svc.go | 3 --- internal/pkg/manifest/workload.go | 6 +----- 4 files changed, 3 insertions(+), 19 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index af493f65ebe..1fa7d35d7ec 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -31,9 +31,6 @@ const ( // SQS Queue configs const ( - sqsStandardQueueType = "standard" - sqsFifoQueueType = "fifo" - sqsDefaultQueueType = sqsStandardQueueType sqsDedupeScopeMessageGroup = "messageGroup" sqsFIFOThroughputLimitPerMessageGroupId = "perMessageGroupId" ) diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index ddec008b3b1..b080c32ba53 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -58,7 +58,6 @@ var ( httpProtocolVersions = []string{"GRPC", "HTTP1", "HTTP2"} invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} - validTopicsTypeValues = []string{standardTopicType, fifoTopicType} validDeduplicationScopeValues = []string{sqsFifoMessageGroup, sqsFifoQueue} validFIFOThroughputLimitValues = []string{sqsFifoPerMessageGroupID, sqsFifoPerQueue} ) @@ -1474,16 +1473,11 @@ func (q SQSQueueOrBool) isFIFO() bool { return !q.Advanced.IsEmpty() && !q.Advanced.FIFO.IsEmpty() } -// no-op for now. +// FIFOAdvanceConfigOrBool no-op for now. func (q FIFOAdvanceConfigOrBool) validate() error { return nil } -func (q FIFOAdvanceConfig) hasFIFOFieldsEnabledOnStandardQueue() bool { - return q.FIFOThroughputLimit != nil || q.HighThroughputFifo != nil || - q.DeduplicationScope != nil || q.ContentBasedDeduplication != nil -} - // validate returns nil if SQSQueue is configured correctly. func (q SQSQueue) validate() error { if q.IsEmpty() { @@ -1524,7 +1518,7 @@ func (q SQSQueue) validateFIFO() error { if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) } - return nil + return q.FIFO.validate() } func (q FIFOAdvanceConfig) validateHighThroughputFifo() error { diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 0891115517b..143b01ac119 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -15,9 +15,6 @@ import ( const ( workerSvcManifestPath = "workloads/services/worker/manifest.yml" - standardTopicType = "standard" - fifoTopicType = "fifo" - defaultTopicType = standardTopicType ) var ( diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index f6ceb7e7bcd..524b3c98aec 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -30,9 +30,6 @@ const ( sqsFifoPerQueue = "perQueue" sqsFifoMessageGroup = "messageGroup" sqsFifoQueue = "queue" - sqsStandardQueueType = "standard" - sqsFifoQueueType = "fifo" - sqsDefaultQueueType = sqsStandardQueueType ) const ( @@ -43,8 +40,7 @@ const ( // All placement options. var ( - subnetPlacements = []string{string(PublicSubnetPlacement), string(PrivateSubnetPlacement)} - sqsValidQueueTypeValues = []string{sqsStandardQueueType, sqsFifoQueueType} + subnetPlacements = []string{string(PublicSubnetPlacement), string(PrivateSubnetPlacement)} ) // Error definitions. From ed043e832ac2cd0c7e71a2dad7c937f0267e1e2a Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 21 Sep 2022 23:29:29 -0700 Subject: [PATCH 12/19] nits --- .../cloudformation/stack/transformers.go | 3 +- internal/pkg/manifest/validate.go | 30 +++++++------------ internal/pkg/manifest/worker_svc.go | 2 +- 3 files changed, 13 insertions(+), 22 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 1fa7d35d7ec..29ac1831cc3 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -884,9 +884,8 @@ func convertQueue(q manifest.SQSQueue) *template.SQSQueue { if aws.BoolValue(q.FIFO.Enable) { queue.FIFOQueueConfig = &template.FIFOQueueConfig{ - Enable: nil, + Enable: q.FIFO.Enable, } - queue.FIFOQueueConfig.Enable = aws.Bool(true) return queue } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index b080c32ba53..12cbe3da018 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1460,24 +1460,19 @@ func (q SQSQueueOrBool) validate() error { if q.Enabled != nil { return nil } + if err := q.Advanced.validate(); err != nil { + return err + } if q.isFIFO() { - if q.Advanced.FIFO.Enable != nil { - return nil - } - return q.Advanced.validateFIFO() + return q.Advanced.FIFO.validate() } - return q.Advanced.validate() + return nil } func (q SQSQueueOrBool) isFIFO() bool { return !q.Advanced.IsEmpty() && !q.Advanced.FIFO.IsEmpty() } -// FIFOAdvanceConfigOrBool no-op for now. -func (q FIFOAdvanceConfigOrBool) validate() error { - return nil -} - // validate returns nil if SQSQueue is configured correctly. func (q SQSQueue) validate() error { if q.IsEmpty() { @@ -1489,7 +1484,7 @@ func (q SQSQueue) validate() error { return nil } -// validate returns nil if SQSQueue is configured correctly. +// validate returns nil if FIFOAdvanceConfig is configured correctly. func (q FIFOAdvanceConfig) validate() error { if q.IsEmpty() { return nil @@ -1510,15 +1505,12 @@ func (q FIFOAdvanceConfig) validate() error { return nil } -// validateFIFO returns nil if FIFO SQSQueue is configured correctly. -func (q SQSQueue) validateFIFO() error { - if err := q.FIFO.Advanced.validate(); err != nil { - return err - } - if err := q.DeadLetter.validate(); err != nil { - return fmt.Errorf(`validate "dead_letter": %w`, err) +// validateFIFO returns nil if FIFOAdvanceConfigOrBool is configured correctly. +func (q FIFOAdvanceConfigOrBool) validate() error { + if q.Enable != nil { + return nil } - return q.FIFO.validate() + return q.Advanced.validate() } func (q FIFOAdvanceConfig) validateHighThroughputFifo() error { diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 143b01ac119..af274e7993a 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -241,7 +241,7 @@ func (s *WorkerService) Subscriptions() []TopicSubscription { return s.Subscribe.Topics } -// RetrofitFIFOConfig appends ".fifo" suffix to fifo topics, also adds values to type field. +// RetrofitFIFOConfig appends ".fifo" suffix to fifo topics. func (s *WorkerService) RetrofitFIFOConfig() { for idx, topic := range s.Subscribe.Topics { // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. From 56ca195292fa4bebba31f3a8e89ac3668cdc4347 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Thu, 22 Sep 2022 10:21:10 -0700 Subject: [PATCH 13/19] nits --- internal/pkg/cli/deploy/svc.go | 27 +++++++++---------- .../deploy/cloudformation/stack/worker_svc.go | 12 ++++----- internal/pkg/manifest/worker_svc.go | 1 + 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index 22138d499ae..e7c72c1e572 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1171,20 +1171,17 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* for _, topic := range topics { topicARNs = append(topicARNs, topic.ARN()) } - if !d.wsMft.Subscribe.IsEmpty() { - d.wsMft.RetrofitFIFOConfig() - } subs := d.wsMft.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { return nil, err } conf, err := stack.NewWorkerService(stack.WorkerServiceConfig{ - App: d.app, - Env: d.env.Name, - Manifest: d.wsMft, - RawManifest: d.rawMft, - RuntimeConfig: *rc, - Addons: d.addons, + App: d.app, + Env: d.env.Name, + Manifest: d.wsMft, + RawManifest: d.rawMft, + RuntimeConfig: *rc, + Addons: d.addons, }) if err != nil { return nil, fmt.Errorf("create stack configuration: %w", err) @@ -1210,12 +1207,12 @@ func (d *jobDeployer) stackConfiguration(in *StackRuntimeConfiguration) (*jobSta return nil, err } conf, err := stack.NewScheduledJob(stack.ScheduledJobConfig{ - App: d.app, - Env: d.env.Name, - Manifest: d.jobMft, - RawManifest: d.rawMft, - RuntimeConfig: *rc, - Addons: d.addons, + App: d.app, + Env: d.env.Name, + Manifest: d.jobMft, + RawManifest: d.rawMft, + RuntimeConfig: *rc, + Addons: d.addons, }) if err != nil { return nil, fmt.Errorf("create stack configuration: %w", err) diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index c39943ec328..9253c9b77ae 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -30,12 +30,12 @@ type WorkerService struct { // WorkerServiceConfig contains data required to initialize a scheduled job stack. type WorkerServiceConfig struct { - App *config.Application - Env string - Manifest *manifest.WorkerService - RawManifest []byte - RuntimeConfig RuntimeConfig - Addons addons + App *config.Application + Env string + Manifest *manifest.WorkerService + RawManifest []byte + RuntimeConfig RuntimeConfig + Addons addons } // NewWorkerService creates a new WorkerService stack from a manifest file. diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index af274e7993a..8bb6587dcbc 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -238,6 +238,7 @@ func (s *WorkerService) EnvFile() string { // Subscriptions returns a list of TopicSubscriotion objects which represent the SNS topics the service // receives messages from. func (s *WorkerService) Subscriptions() []TopicSubscription { + s.RetrofitFIFOConfig() return s.Subscribe.Topics } From fd1dcfd46318505237cb70eb4168d87665cef9fa Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Thu, 22 Sep 2022 11:50:37 -0700 Subject: [PATCH 14/19] integ tests --- .../testdata/workloads/worker-manifest.yml | 10 ++ .../testdata/workloads/worker-test.stack.yml | 139 +++++++++++++++++- 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml index 77fddeb93cd..8100661dd0e 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml @@ -49,6 +49,16 @@ subscribe: service: dogsvc queue: timeout: 1s + - name: mytopic + service: mytopic + queue: + fifo: true + - name: yourtopic + service: yourtopic + queue: + fifo: + content_based_deduplication: true + high_throughput_fifo: true # Optional fields for more advanced use-cases. # diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml index cacf5dc368f..46c4f538b19 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml @@ -55,6 +55,16 @@ Metadata: service: dogsvc queue: timeout: 1s + - name: mytopic + service: mytopic + queue: + fifo: true + - name: yourtopic + service: yourtopic + queue: + fifo: + content_based_deduplication: true + high_throughput_fifo: true # Optional fields for more advanced use-cases. # @@ -142,8 +152,10 @@ Resources: Value: !Ref EventsQueue - Name: COPILOT_TOPIC_QUEUE_URIS Value: !Sub - - '{"dogsvcGiveshuskiesEventsQueue":"${dogsvcgiveshuskiesURL}"}' + - '{"dogsvcGiveshuskiesEventsQueue":"${dogsvcgiveshuskiesURL}","mytopicMytopicfifoEventsQueue":"${mytopicmytopicfifoURL}","yourtopicYourtopicfifoEventsQueue":"${yourtopicyourtopicfifoURL}"}' - dogsvcgiveshuskiesURL: !Ref dogsvcgiveshuskiesEventsQueue + mytopicmytopicfifoURL: !Ref mytopicmytopicfifoEventsQueue + yourtopicyourtopicfifoURL: !Ref yourtopicyourtopicfifoEventsQueue EnvironmentFiles: - !If - HasEnvFile @@ -399,6 +411,8 @@ Resources: - ',' - - !GetAtt EventsQueue.QueueName - !GetAtt dogsvcgiveshuskiesEventsQueue.QueueName + - !GetAtt mytopicmytopicfifoEventsQueue.QueueName + - !GetAtt yourtopicyourtopicfifoEventsQueue.QueueName BacklogPerTaskCalculatorRole: Metadata: 'aws:copilot:description': 'An IAM role for BacklogPerTaskCalculatorFunction' @@ -439,6 +453,8 @@ Resources: Resource: - !GetAtt EventsQueue.Arn - !GetAtt dogsvcgiveshuskiesEventsQueue.Arn + - !GetAtt mytopicmytopicfifoEventsQueue.Arn + - !GetAtt yourtopicyourtopicfifoEventsQueue.Arn ManagedPolicyArns: - !Sub arn:${AWS::Partition}:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole BacklogPerTaskScheduledRule: @@ -500,6 +516,46 @@ Resources: Value: !GetAtt dogsvcgiveshuskiesEventsQueue.QueueName Unit: Count TargetValue: 900 + AutoScalingPolicymytopicmytopicfifoEventsQueue: + Metadata: + 'aws:copilot:description': "An autoscaling policy to maintain 900 messages/task for mytopicmytopicfifoEventsQueue" + Type: AWS::ApplicationAutoScaling::ScalingPolicy + Properties: + PolicyName: !Join [ '-', [ !Ref WorkloadName, BacklogPerTask, !GetAtt mytopicmytopicfifoEventsQueue.QueueName ] ] + PolicyType: TargetTrackingScaling + ScalingTargetId: !Ref AutoScalingTarget + TargetTrackingScalingPolicyConfiguration: + ScaleInCooldown: 120 + ScaleOutCooldown: 60 + CustomizedMetricSpecification: + Namespace: !Sub '${AppName}-${EnvName}-${WorkloadName}' + MetricName: BacklogPerTask + Statistic: Average + Dimensions: + - Name: QueueName + Value: !GetAtt mytopicmytopicfifoEventsQueue.QueueName + Unit: Count + TargetValue: 900 + AutoScalingPolicyyourtopicyourtopicfifoEventsQueue: + Metadata: + 'aws:copilot:description': "An autoscaling policy to maintain 900 messages/task for yourtopicyourtopicfifoEventsQueue" + Type: AWS::ApplicationAutoScaling::ScalingPolicy + Properties: + PolicyName: !Join [ '-', [ !Ref WorkloadName, BacklogPerTask, !GetAtt yourtopicyourtopicfifoEventsQueue.QueueName ] ] + PolicyType: TargetTrackingScaling + ScalingTargetId: !Ref AutoScalingTarget + TargetTrackingScalingPolicyConfiguration: + ScaleInCooldown: 120 + ScaleOutCooldown: 60 + CustomizedMetricSpecification: + Namespace: !Sub '${AppName}-${EnvName}-${WorkloadName}' + MetricName: BacklogPerTask + Statistic: Average + Dimensions: + - Name: QueueName + Value: !GetAtt yourtopicyourtopicfifoEventsQueue.QueueName + Unit: Count + TargetValue: 900 Service: DependsOn: - EnvControllerAction @@ -706,6 +762,87 @@ Resources: Properties: TopicName: !Sub '${AWS::StackName}-givesOtherdogs' KmsMasterKeyId: 'alias/aws/sns' + mytopicmytopicfifoSNSTopicSubscription: + Metadata: + 'aws:copilot:description': 'A SNS subscription to topic mytopic.fifo from service mytopic' + Type: AWS::SNS::Subscription + Properties: + TopicArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-mytopic-mytopic.fifo' ] ] + Protocol: 'sqs' + Endpoint: !GetAtt mytopicmytopicfifoEventsQueue.Arn + mytopicmytopicfifoEventsQueue: + Metadata: + 'aws:copilot:description': 'A SQS FIFO queue to buffer messages from the topic mytopic.fifo' + Type: AWS::SQS::Queue + Properties: + KmsMasterKeyId: !Ref EventsKMSKey + FifoQueue: true + mytopicmytopicfifoQueuePolicy: + Type: AWS::SQS::QueuePolicy + Properties: + Queues: [ !Ref 'mytopicmytopicfifoEventsQueue' ] + PolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + AWS: + - !GetAtt TaskRole.Arn + Action: + - sqs:ReceiveMessage + - sqs:DeleteMessage + Resource: !GetAtt mytopicmytopicfifoEventsQueue.Arn + - Effect: Allow + Principal: + Service: sns.amazonaws.com + Action: + - sqs:SendMessage + Resource: !GetAtt mytopicmytopicfifoEventsQueue.Arn + Condition: + ArnEquals: + aws:SourceArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-mytopic-mytopic.fifo' ] ] + yourtopicyourtopicfifoSNSTopicSubscription: + Metadata: + 'aws:copilot:description': 'A SNS subscription to topic yourtopic.fifo from service yourtopic' + Type: AWS::SNS::Subscription + Properties: + TopicArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-yourtopic-yourtopic.fifo' ] ] + Protocol: 'sqs' + Endpoint: !GetAtt yourtopicyourtopicfifoEventsQueue.Arn + yourtopicyourtopicfifoEventsQueue: + Metadata: + 'aws:copilot:description': 'A SQS FIFO queue to buffer messages from the topic yourtopic.fifo' + Type: AWS::SQS::Queue + Properties: + KmsMasterKeyId: !Ref EventsKMSKey + FifoQueue: true + FifoThroughputLimit: perMessageGroupId + DeduplicationScope: messageGroup + ContentBasedDeduplication: true + yourtopicyourtopicfifoQueuePolicy: + Type: AWS::SQS::QueuePolicy + Properties: + Queues: [ !Ref 'yourtopicyourtopicfifoEventsQueue' ] + PolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + AWS: + - !GetAtt TaskRole.Arn + Action: + - sqs:ReceiveMessage + - sqs:DeleteMessage + Resource: !GetAtt yourtopicyourtopicfifoEventsQueue.Arn + - Effect: Allow + Principal: + Service: sns.amazonaws.com + Action: + - sqs:SendMessage + Resource: !GetAtt yourtopicyourtopicfifoEventsQueue.Arn + Condition: + ArnEquals: + aws:SourceArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-yourtopic-yourtopic.fifo' ] ] givesOtherdogsSNSTopicPolicy: Type: AWS::SNS::TopicPolicy DependsOn: givesOtherdogsSNSTopic From 228c4d3228ea024ca008402e707cc8151b879aa0 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Thu, 22 Sep 2022 16:06:34 -0700 Subject: [PATCH 15/19] address feedback:1 --- internal/pkg/cli/deploy/svc.go | 2 +- .../testdata/workloads/worker-manifest.yml | 4 + .../testdata/workloads/worker-test.stack.yml | 81 +++++++++++++++++-- .../cloudformation/stack/transformers.go | 48 ++++------- .../cloudformation/stack/transformers_test.go | 39 ++++----- .../stack/worker_service_integration_test.go | 4 +- internal/pkg/manifest/validate.go | 59 +++++++------- internal/pkg/manifest/validate_test.go | 12 +-- internal/pkg/manifest/worker_svc.go | 31 +++---- internal/pkg/manifest/worker_svc_test.go | 4 +- internal/pkg/manifest/workload.go | 10 +-- .../workloads/partials/cf/subscribe.yml | 28 +++---- internal/pkg/template/workload.go | 6 -- 13 files changed, 182 insertions(+), 146 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index e7c72c1e572..e5ceab34e9d 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1171,7 +1171,7 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* for _, topic := range topics { topicARNs = append(topicARNs, topic.ARN()) } - subs := d.wsMft.Subscriptions() + subs := d.wsMft.Subscribe.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { return nil, err } diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml index 8100661dd0e..1ccf18d4aec 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml @@ -59,6 +59,10 @@ subscribe: fifo: content_based_deduplication: true high_throughput_fifo: true + - name: nonfifotopic + service: nonfifotopic + queue: + fifo: false # Optional fields for more advanced use-cases. # diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml index 46c4f538b19..0a91e614f3d 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml @@ -65,6 +65,10 @@ Metadata: fifo: content_based_deduplication: true high_throughput_fifo: true + - name: nonfifotopic + service: nonfifotopic + queue: + fifo: false # Optional fields for more advanced use-cases. # @@ -152,10 +156,11 @@ Resources: Value: !Ref EventsQueue - Name: COPILOT_TOPIC_QUEUE_URIS Value: !Sub - - '{"dogsvcGiveshuskiesEventsQueue":"${dogsvcgiveshuskiesURL}","mytopicMytopicfifoEventsQueue":"${mytopicmytopicfifoURL}","yourtopicYourtopicfifoEventsQueue":"${yourtopicyourtopicfifoURL}"}' + - '{"dogsvcGiveshuskiesEventsQueue":"${dogsvcgiveshuskiesURL}","mytopicMytopicfifoEventsQueue":"${mytopicmytopicfifoURL}","nonfifotopicNonfifotopicEventsQueue":"${nonfifotopicnonfifotopicURL}","yourtopicYourtopicfifoEventsQueue":"${yourtopicyourtopicfifoURL}"}' - dogsvcgiveshuskiesURL: !Ref dogsvcgiveshuskiesEventsQueue mytopicmytopicfifoURL: !Ref mytopicmytopicfifoEventsQueue yourtopicyourtopicfifoURL: !Ref yourtopicyourtopicfifoEventsQueue + nonfifotopicnonfifotopicURL: !Ref nonfifotopicnonfifotopicEventsQueue EnvironmentFiles: - !If - HasEnvFile @@ -413,6 +418,7 @@ Resources: - !GetAtt dogsvcgiveshuskiesEventsQueue.QueueName - !GetAtt mytopicmytopicfifoEventsQueue.QueueName - !GetAtt yourtopicyourtopicfifoEventsQueue.QueueName + - !GetAtt nonfifotopicnonfifotopicEventsQueue.QueueName BacklogPerTaskCalculatorRole: Metadata: 'aws:copilot:description': 'An IAM role for BacklogPerTaskCalculatorFunction' @@ -455,6 +461,7 @@ Resources: - !GetAtt dogsvcgiveshuskiesEventsQueue.Arn - !GetAtt mytopicmytopicfifoEventsQueue.Arn - !GetAtt yourtopicyourtopicfifoEventsQueue.Arn + - !GetAtt nonfifotopicnonfifotopicEventsQueue.Arn ManagedPolicyArns: - !Sub arn:${AWS::Partition}:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole BacklogPerTaskScheduledRule: @@ -556,6 +563,26 @@ Resources: Value: !GetAtt yourtopicyourtopicfifoEventsQueue.QueueName Unit: Count TargetValue: 900 + AutoScalingPolicynonfifotopicnonfifotopicEventsQueue: + Metadata: + 'aws:copilot:description': "An autoscaling policy to maintain 900 messages/task for nonfifotopicnonfifotopicEventsQueue" + Type: AWS::ApplicationAutoScaling::ScalingPolicy + Properties: + PolicyName: !Join [ '-', [ !Ref WorkloadName, BacklogPerTask, !GetAtt nonfifotopicnonfifotopicEventsQueue.QueueName ] ] + PolicyType: TargetTrackingScaling + ScalingTargetId: !Ref AutoScalingTarget + TargetTrackingScalingPolicyConfiguration: + ScaleInCooldown: 120 + ScaleOutCooldown: 60 + CustomizedMetricSpecification: + Namespace: !Sub '${AppName}-${EnvName}-${WorkloadName}' + MetricName: BacklogPerTask + Statistic: Average + Dimensions: + - Name: QueueName + Value: !GetAtt nonfifotopicnonfifotopicEventsQueue.QueueName + Unit: Count + TargetValue: 900 Service: DependsOn: - EnvControllerAction @@ -755,13 +782,6 @@ Resources: Condition: ArnEquals: aws:SourceArn: !Join ['', [!Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-dogsvc-giveshuskies']] - givesOtherdogsSNSTopic: - Metadata: - 'aws:copilot:description': 'A SNS topic to broadcast givesOtherdogs events' - Type: AWS::SNS::Topic - Properties: - TopicName: !Sub '${AWS::StackName}-givesOtherdogs' - KmsMasterKeyId: 'alias/aws/sns' mytopicmytopicfifoSNSTopicSubscription: Metadata: 'aws:copilot:description': 'A SNS subscription to topic mytopic.fifo from service mytopic' @@ -843,6 +863,51 @@ Resources: Condition: ArnEquals: aws:SourceArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-yourtopic-yourtopic.fifo' ] ] + nonfifotopicnonfifotopicSNSTopicSubscription: + Metadata: + 'aws:copilot:description': 'A SNS subscription to topic nonfifotopic from service nonfifotopic' + Type: AWS::SNS::Subscription + Properties: + TopicArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-nonfifotopic-nonfifotopic' ] ] + Protocol: 'sqs' + Endpoint: !GetAtt nonfifotopicnonfifotopicEventsQueue.Arn + nonfifotopicnonfifotopicEventsQueue: + Metadata: + 'aws:copilot:description': 'A SQS queue to buffer messages from the topic nonfifotopic' + Type: AWS::SQS::Queue + Properties: + KmsMasterKeyId: !Ref EventsKMSKey + nonfifotopicnonfifotopicQueuePolicy: + Type: AWS::SQS::QueuePolicy + Properties: + Queues: [ !Ref 'nonfifotopicnonfifotopicEventsQueue' ] + PolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + AWS: + - !GetAtt TaskRole.Arn + Action: + - sqs:ReceiveMessage + - sqs:DeleteMessage + Resource: !GetAtt nonfifotopicnonfifotopicEventsQueue.Arn + - Effect: Allow + Principal: + Service: sns.amazonaws.com + Action: + - sqs:SendMessage + Resource: !GetAtt nonfifotopicnonfifotopicEventsQueue.Arn + Condition: + ArnEquals: + aws:SourceArn: !Join [ '', [ !Sub 'arn:${AWS::Partition}:sns:${AWS::Region}:${AWS::AccountId}:', !Ref AppName, '-', !Ref EnvName, '-nonfifotopic-nonfifotopic' ] ] + givesOtherdogsSNSTopic: + Metadata: + 'aws:copilot:description': 'A SNS topic to broadcast givesOtherdogs events' + Type: AWS::SNS::Topic + Properties: + TopicName: !Sub '${AWS::StackName}-givesOtherdogs' + KmsMasterKeyId: 'alias/aws/sns' givesOtherdogsSNSTopicPolicy: Type: AWS::SNS::TopicPolicy DependsOn: givesOtherdogsSNSTopic diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 29ac1831cc3..ee8f1535db2 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -29,7 +29,7 @@ const ( disabled = "DISABLED" ) -// SQS Queue configs +// SQS Queue field values. const ( sqsDedupeScopeMessageGroup = "messageGroup" sqsFIFOThroughputLimitPerMessageGroupId = "perMessageGroupId" @@ -866,52 +866,38 @@ func convertFilterPolicy(filterPolicy map[string]interface{}) (*string, error) { return aws.String(string(bytes)), nil } -func convertQueue(q manifest.SQSQueue) *template.SQSQueue { - if q.IsEmpty() { +func convertQueue(in manifest.SQSQueue) *template.SQSQueue { + if in.IsEmpty() { return nil } queue := &template.SQSQueue{ - Retention: convertRetention(q.Retention), - Delay: convertDelay(q.Delay), - Timeout: convertTimeout(q.Timeout), - DeadLetter: convertDeadLetter(q.DeadLetter), + Retention: convertRetention(in.Retention), + Delay: convertDelay(in.Delay), + Timeout: convertTimeout(in.Timeout), + DeadLetter: convertDeadLetter(in.DeadLetter), } - if q.FIFO.IsEmpty() { + if !in.FIFO.IsEnabled() { return queue } - if aws.BoolValue(q.FIFO.Enable) { - queue.FIFOQueueConfig = &template.FIFOQueueConfig{ - Enable: q.FIFO.Enable, - } + if aws.BoolValue(in.FIFO.Enable) { + queue.FIFOQueueConfig = &template.FIFOQueueConfig{} return queue } - if !q.FIFO.Advanced.IsEmpty() { + if !in.FIFO.Advanced.IsEmpty() { queue.FIFOQueueConfig = &template.FIFOQueueConfig{ - Advanced: &template.FIFOAdvanceConfig{ - ContentBasedDeduplication: nil, - DeduplicationScope: nil, - FIFOThroughputLimit: nil, - }, + ContentBasedDeduplication: in.FIFO.Advanced.ContentBasedDeduplication, + DeduplicationScope: in.FIFO.Advanced.DeduplicationScope, + FIFOThroughputLimit: in.FIFO.Advanced.FIFOThroughputLimit, } - if q.FIFO.Advanced.ContentBasedDeduplication != nil { - queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication = q.FIFO.Advanced.ContentBasedDeduplication - } - if q.FIFO.Advanced.DeduplicationScope != nil { - queue.FIFOQueueConfig.Advanced.DeduplicationScope = q.FIFO.Advanced.DeduplicationScope - } - if q.FIFO.Advanced.FIFOThroughputLimit != nil { - queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit = q.FIFO.Advanced.FIFOThroughputLimit - } - if aws.BoolValue(q.FIFO.Advanced.HighThroughputFifo) { - queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) - queue.FIFOQueueConfig.Advanced.DeduplicationScope = aws.String(sqsDedupeScopeMessageGroup) + if aws.BoolValue(in.FIFO.Advanced.HighThroughputFifo) { + queue.FIFOQueueConfig.FIFOThroughputLimit = aws.String(sqsFIFOThroughputLimitPerMessageGroupId) + queue.FIFOQueueConfig.DeduplicationScope = aws.String(sqsDedupeScopeMessageGroup) } } - return queue } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 8b909075c00..46d51109f7c 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1618,10 +1618,8 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, FIFOQueueConfig: &template.FIFOQueueConfig{ - Advanced: &template.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("perMessageGroupId"), - DeduplicationScope: aws.String("messageGroup"), - }, + FIFOThroughputLimit: aws.String("perMessageGroupId"), + DeduplicationScope: aws.String("messageGroup"), }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), @@ -1664,10 +1662,7 @@ func Test_convertSubscribe(t *testing.T) { DeadLetter: &template.DeadLetterQueue{ Tries: aws.Uint16(35), }, - FIFOQueueConfig: &template.FIFOQueueConfig{ - Enable: aws.Bool(true), - Advanced: nil, - }, + FIFOQueueConfig: &template.FIFOQueueConfig{}, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), }, @@ -1716,11 +1711,9 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, FIFOQueueConfig: &template.FIFOQueueConfig{ - Advanced: &template.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - }, + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), @@ -1774,11 +1767,9 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, FIFOQueueConfig: &template.FIFOQueueConfig{ - Advanced: &template.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - }, + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), @@ -1840,11 +1831,9 @@ func Test_convertSubscribe(t *testing.T) { Tries: aws.Uint16(35), }, FIFOQueueConfig: &template.FIFOQueueConfig{ - Advanced: &template.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), - }, + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), }, }, FilterPolicy: aws.String(`{"store":["example_corp"]}`), @@ -1909,7 +1898,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - FIFOQueueConfig: &template.FIFOQueueConfig{Enable: aws.Bool(true)}, + FIFOQueueConfig: &template.FIFOQueueConfig{}, }, }, }, @@ -1969,7 +1958,7 @@ func Test_convertSubscribe(t *testing.T) { }, }, Queue: &template.SQSQueue{ - FIFOQueueConfig: &template.FIFOQueueConfig{Enable: aws.Bool(true)}, + FIFOQueueConfig: &template.FIFOQueueConfig{}, }, }, }, diff --git a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go index 179ece366f8..ff2719d7caf 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go @@ -47,7 +47,7 @@ func TestWorkerService_Template(t *testing.T) { v, ok := content.(*manifest.WorkerService) require.True(t, ok) - v.RetrofitFIFOConfig() + v.Subscribe.Topics = v.Subscribe.Subscriptions() ws, err := workspace.New() require.NoError(t, err) @@ -57,7 +57,7 @@ func TestWorkerService_Template(t *testing.T) { require.ErrorAs(t, err, ¬Found) serializer, err := stack.NewWorkerService(stack.WorkerServiceConfig{ - App: &config.Application{ + App: &config.Application{ Name: appName, }, Env: envName, diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 12cbe3da018..3bfed22c56d 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -57,9 +57,9 @@ var ( httpProtocolVersions = []string{"GRPC", "HTTP1", "HTTP2"} - invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} - validDeduplicationScopeValues = []string{sqsFifoMessageGroup, sqsFifoQueue} - validFIFOThroughputLimitValues = []string{sqsFifoPerMessageGroupID, sqsFifoPerQueue} + invalidTaskDefOverridePathRegexp = []string{`Family`, `ContainerDefinitions\[\d+\].Name`} + validSQSDeduplicationScopeValues = []string{sqsDeduplicationScopeMessageGroup, sqsDeduplicationScopeQueue} + validSQSFIFOThroughputLimitValues = []string{sqsFIFOThroughputLimitPerMessageGroupID, sqsFIFOThroughputLimitPerQueue} ) // Validate returns nil if DynamicLoadBalancedWebService is configured correctly. @@ -1457,16 +1457,10 @@ func (q SQSQueueOrBool) validate() error { if q.IsEmpty() { return nil } - if q.Enabled != nil { - return nil - } if err := q.Advanced.validate(); err != nil { return err } - if q.isFIFO() { - return q.Advanced.FIFO.validate() - } - return nil + return q.Advanced.FIFO.validate() } func (q SQSQueueOrBool) isFIFO() bool { @@ -1490,16 +1484,16 @@ func (q FIFOAdvanceConfig) validate() error { return nil } - if err := q.validateHighThroughputFifo(); err != nil { + if err := q.validateHighThroughputFIFO(); err != nil { return err } if err := q.validateDeduplicationScope(); err != nil { return err } - if err := q.validateFifoThroughputLimit(); err != nil { + if err := q.validateFIFOThroughputLimit(); err != nil { return err } - if aws.StringValue(q.FIFOThroughputLimit) == sqsFifoPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsFifoQueue { + if aws.StringValue(q.FIFOThroughputLimit) == sqsFIFOThroughputLimitPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsDeduplicationScopeQueue { return fmt.Errorf(`"fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`) } return nil @@ -1513,37 +1507,38 @@ func (q FIFOAdvanceConfigOrBool) validate() error { return q.Advanced.validate() } -func (q FIFOAdvanceConfig) validateHighThroughputFifo() error { - if q.HighThroughputFifo != nil { - if q.FIFOThroughputLimit != nil { - return &errFieldMutualExclusive{ - firstField: "high_throughput_fifo", - secondField: "fifo_throughput_limit", - mustExist: false, - } +func (q FIFOAdvanceConfig) validateHighThroughputFIFO() error { + if q.HighThroughputFifo == nil { + return nil + } + if q.FIFOThroughputLimit != nil { + return &errFieldMutualExclusive{ + firstField: "high_throughput_fifo", + secondField: "fifo_throughput_limit", + mustExist: false, } + } - if q.DeduplicationScope != nil { - return &errFieldMutualExclusive{ - firstField: "high_throughput_fifo", - secondField: "deduplication_scope", - mustExist: false, - } + if q.DeduplicationScope != nil { + return &errFieldMutualExclusive{ + firstField: "high_throughput_fifo", + secondField: "deduplication_scope", + mustExist: false, } } return nil } func (q FIFOAdvanceConfig) validateDeduplicationScope() error { - if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validDeduplicationScopeValues) { - return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validDeduplicationScopeValues, "or")) + if q.DeduplicationScope != nil && !contains(aws.StringValue(q.DeduplicationScope), validSQSDeduplicationScopeValues) { + return fmt.Errorf(`validate "deduplication_scope": deduplication scope value must be one of %s`, english.WordSeries(validSQSDeduplicationScopeValues, "or")) } return nil } -func (q FIFOAdvanceConfig) validateFifoThroughputLimit() error { - if q.FIFOThroughputLimit != nil && !contains(aws.StringValue(q.FIFOThroughputLimit), validFIFOThroughputLimitValues) { - return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validFIFOThroughputLimitValues, "or")) +func (q FIFOAdvanceConfig) validateFIFOThroughputLimit() error { + if q.FIFOThroughputLimit != nil && !contains(aws.StringValue(q.FIFOThroughputLimit), validSQSFIFOThroughputLimitValues) { + return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validSQSFIFOThroughputLimitValues, "or")) } return nil } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 6a0e5a1d1a7..4aa17b543ac 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2832,7 +2832,7 @@ func TestTopicSubscription_validate(t *testing.T) { DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: FIFOAdvanceConfigOrBool{ Advanced: FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + FIFOThroughputLimit: aws.String(sqsFIFOThroughputLimitPerMessageGroupID), }, }, }, @@ -2872,7 +2872,7 @@ func TestTopicSubscription_validate(t *testing.T) { DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: FIFOAdvanceConfigOrBool{ Advanced: FIFOAdvanceConfig{ - DeduplicationScope: aws.String(sqsFifoMessageGroup), + DeduplicationScope: aws.String(sqsDeduplicationScopeMessageGroup), }, }, }, @@ -2893,7 +2893,7 @@ func TestTopicSubscription_validate(t *testing.T) { FIFO: FIFOAdvanceConfigOrBool{ Advanced: FIFOAdvanceConfig{ HighThroughputFifo: aws.Bool(true), - DeduplicationScope: aws.String(sqsFifoMessageGroup), + DeduplicationScope: aws.String(sqsDeduplicationScopeMessageGroup), }, }, }, @@ -2914,7 +2914,7 @@ func TestTopicSubscription_validate(t *testing.T) { FIFO: FIFOAdvanceConfigOrBool{ Advanced: FIFOAdvanceConfig{ HighThroughputFifo: aws.Bool(true), - FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), + FIFOThroughputLimit: aws.String(sqsFIFOThroughputLimitPerMessageGroupID), }, }, }, @@ -2934,8 +2934,8 @@ func TestTopicSubscription_validate(t *testing.T) { DeadLetter: DeadLetterQueue{Tries: aws.Uint16(10)}, FIFO: FIFOAdvanceConfigOrBool{ Advanced: FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String(sqsFifoPerMessageGroupID), - DeduplicationScope: aws.String(sqsFifoQueue), + FIFOThroughputLimit: aws.String(sqsFIFOThroughputLimitPerMessageGroupID), + DeduplicationScope: aws.String(sqsDeduplicationScopeQueue), }, }, }, diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 8bb6587dcbc..b81630158fd 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -19,7 +19,7 @@ const ( var ( errUnmarshalQueueOpts = errors.New(`cannot unmarshal "queue" field into bool or map`) - errUnmarshalFifoConfig = errors.New("unable to unmarshal fifo field into boolean or compose-style map") + errUnmarshalFifoConfig = errors.New(`unable to unmarshal "fifo" field into boolean or compose-style map`) ) // WorkerService holds the configuration to create a worker service. @@ -129,6 +129,11 @@ func (f *FIFOAdvanceConfigOrBool) IsEmpty() bool { return f.Enable == nil && f.Advanced.IsEmpty() } +// IsEmpty returns true if the FifoAdvanceConfigOrBool struct has all nil values. +func (f *FIFOAdvanceConfigOrBool) IsEnabled() bool { + return aws.BoolValue(f.Enable) || !f.Advanced.IsEmpty() +} + // FifoAdvanceConfig represents the advanced fifo queue config. type FIFOAdvanceConfig struct { ContentBasedDeduplication *bool `yaml:"content_based_deduplication"` @@ -236,22 +241,20 @@ func (s *WorkerService) EnvFile() string { } // Subscriptions returns a list of TopicSubscriotion objects which represent the SNS topics the service -// receives messages from. -func (s *WorkerService) Subscriptions() []TopicSubscription { - s.RetrofitFIFOConfig() - return s.Subscribe.Topics -} - -// RetrofitFIFOConfig appends ".fifo" suffix to fifo topics. -func (s *WorkerService) RetrofitFIFOConfig() { - for idx, topic := range s.Subscribe.Topics { +// receives messages from. This method also appends ".fifo" to the topics and returns a new set of subs. +func (s *SubscribeConfig) Subscriptions() []TopicSubscription { + var subs []TopicSubscription + for _, topic := range s.Topics { + topicSubscription := topic // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. - if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && !s.Subscribe.Queue.FIFO.IsEmpty() { - s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") - } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && !topic.Queue.Advanced.FIFO.IsEmpty() { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. - s.Subscribe.Topics[idx].Name = aws.String(aws.StringValue(topic.Name) + ".fifo") + if topic.Queue.IsEmpty() && !s.Queue.IsEmpty() && s.Queue.FIFO.IsEnabled() { + topicSubscription.Name = aws.String(aws.StringValue(topic.Name) + ".fifo") + } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.FIFO.IsEnabled() { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. + topicSubscription.Name = aws.String(aws.StringValue(topic.Name) + ".fifo") } + subs = append(subs, topicSubscription) } + return subs } func (s WorkerService) applyEnv(envName string) (workloadManifest, error) { diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 2193a1335a3..3ee11a56fae 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -1288,7 +1288,7 @@ func TestWorkerService_RequiredEnvironmentFeatures(t *testing.T) { } } -func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { +func TestWorkerService_Subscriptions(t *testing.T) { duration111Seconds := 111 * time.Second testCases := map[string]struct { input *WorkerService @@ -1549,7 +1549,7 @@ func TestWorkerService_RetrofitFIFOConfig(t *testing.T) { t.Run(name, func(t *testing.T) { // WHEN - svc.RetrofitFIFOConfig() + svc.Subscribe.Topics = svc.Subscribe.Subscriptions() // THEN require.Equal(t, tc.expected, svc) diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index 524b3c98aec..a17c90b4631 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -24,12 +24,12 @@ const ( defaultDockerfileName = "Dockerfile" ) -// SQS Queue configs +// SQS Queue field options. const ( - sqsFifoPerMessageGroupID = "perMessageGroupId" - sqsFifoPerQueue = "perQueue" - sqsFifoMessageGroup = "messageGroup" - sqsFifoQueue = "queue" + sqsFIFOThroughputLimitPerMessageGroupID = "perMessageGroupId" + sqsFIFOThroughputLimitPerQueue = "perQueue" + sqsDeduplicationScopeMessageGroup = "messageGroup" + sqsDeduplicationScopeQueue = "queue" ) const ( diff --git a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml index 24f3f1cf539..dca02359da8 100644 --- a/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml +++ b/internal/pkg/template/templates/workloads/partials/cf/subscribe.yml @@ -82,15 +82,15 @@ EventsQueue: {{- end}} {{- if .Subscribe.Queue.IsFIFO }} FifoQueue: true - {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced }} - {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit }} - FifoThroughputLimit: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit}} + {{- if .Subscribe.Queue.FIFOQueueConfig }} + {{- if .Subscribe.Queue.FIFOQueueConfig.FIFOThroughputLimit }} + FifoThroughputLimit: {{.Subscribe.Queue.FIFOQueueConfig.FIFOThroughputLimit}} {{- end}} - {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.DeduplicationScope }} - DeduplicationScope: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.DeduplicationScope}} + {{- if .Subscribe.Queue.FIFOQueueConfig.DeduplicationScope }} + DeduplicationScope: {{.Subscribe.Queue.FIFOQueueConfig.DeduplicationScope}} {{- end }} - {{- if .Subscribe.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication }} - ContentBasedDeduplication: {{.Subscribe.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication}} + {{- if .Subscribe.Queue.FIFOQueueConfig.ContentBasedDeduplication }} + ContentBasedDeduplication: {{.Subscribe.Queue.FIFOQueueConfig.ContentBasedDeduplication}} {{- end}} {{- end}} {{- end}} @@ -200,15 +200,15 @@ QueuePolicy: {{- end}} {{- if $topic.Queue.IsFIFO }} FifoQueue: true - {{- if $topic.Queue.FIFOQueueConfig.Advanced}} - {{- if $topic.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit }} - FifoThroughputLimit: {{$topic.Queue.FIFOQueueConfig.Advanced.FIFOThroughputLimit}} + {{- if $topic.Queue.FIFOQueueConfig}} + {{- if $topic.Queue.FIFOQueueConfig.FIFOThroughputLimit }} + FifoThroughputLimit: {{$topic.Queue.FIFOQueueConfig.FIFOThroughputLimit}} {{- end}} - {{- if $topic.Queue.FIFOQueueConfig.Advanced.DeduplicationScope }} - DeduplicationScope: {{$topic.Queue.FIFOQueueConfig.Advanced.DeduplicationScope}} + {{- if $topic.Queue.FIFOQueueConfig.DeduplicationScope }} + DeduplicationScope: {{$topic.Queue.FIFOQueueConfig.DeduplicationScope}} {{- end }} - {{- if $topic.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication }} - ContentBasedDeduplication: {{$topic.Queue.FIFOQueueConfig.Advanced.ContentBasedDeduplication}} + {{- if $topic.Queue.FIFOQueueConfig.ContentBasedDeduplication }} + ContentBasedDeduplication: {{$topic.Queue.FIFOQueueConfig.ContentBasedDeduplication}} {{- end}} {{- end}} {{- end}} diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index f6257076962..de58c2e3172 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -445,12 +445,6 @@ type SQSQueue struct { // FifoAdvanceConfigOrBool holds information needed to render a FIFO SQS Queue in a container definition. type FIFOQueueConfig struct { - Enable *bool - Advanced *FIFOAdvanceConfig -} - -// FifoAdvanceConfig holds information needed to render a advanced FIFO SQS Queue in a container definition. -type FIFOAdvanceConfig struct { FIFOThroughputLimit *string ContentBasedDeduplication *bool DeduplicationScope *string From df6eb1642508fde285c2d0c7a6c885bd20988ed4 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Thu, 22 Sep 2022 16:16:10 -0700 Subject: [PATCH 16/19] fix static check --- internal/pkg/manifest/validate.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 3bfed22c56d..3cbb582129c 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1463,10 +1463,6 @@ func (q SQSQueueOrBool) validate() error { return q.Advanced.FIFO.validate() } -func (q SQSQueueOrBool) isFIFO() bool { - return !q.Advanced.IsEmpty() && !q.Advanced.FIFO.IsEmpty() -} - // validate returns nil if SQSQueue is configured correctly. func (q SQSQueue) validate() error { if q.IsEmpty() { From 52e4a0fe2762812ad4c5cd8254aa75f032f8889d Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Thu, 22 Sep 2022 17:20:08 -0700 Subject: [PATCH 17/19] modify integ test --- .../deploy/cloudformation/stack/transformers.go | 2 +- .../cloudformation/stack/transformers_test.go | 16 ++++++++-------- .../stack/worker_service_integration_test.go | 2 -- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index ee8f1535db2..484916c7183 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -822,7 +822,7 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro return nil, nil } var subscriptions template.SubscribeOpts - for _, sb := range s.Topics { + for _, sb := range s.Subscriptions() { ts, err := convertTopicSubscription(sb) if err != nil { return nil, err diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 46d51109f7c..063e61aeac4 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1608,7 +1608,7 @@ func Test_convertSubscribe(t *testing.T) { wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name"), + Name: aws.String("name.fifo"), Service: aws.String("svc"), Queue: &template.SQSQueue{ Retention: aws.Int64(111), @@ -1653,7 +1653,7 @@ func Test_convertSubscribe(t *testing.T) { wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ { - Name: aws.String("name"), + Name: aws.String("name.fifo"), Service: aws.String("svc"), Queue: &template.SQSQueue{ Retention: aws.Int64(111), @@ -1674,7 +1674,7 @@ func Test_convertSubscribe(t *testing.T) { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: manifest.SQSQueueOrBool{ Advanced: manifest.SQSQueue{ @@ -1726,7 +1726,7 @@ func Test_convertSubscribe(t *testing.T) { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: manifest.SQSQueueOrBool{ Advanced: manifest.SQSQueue{ @@ -1786,7 +1786,7 @@ func Test_convertSubscribe(t *testing.T) { inSubscribe: manifest.SubscribeConfig{ Topics: []manifest.TopicSubscription{ { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), Queue: manifest.SQSQueueOrBool{ Advanced: manifest.SQSQueue{ @@ -1869,7 +1869,7 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), }, }, @@ -1921,11 +1921,11 @@ func Test_convertSubscribe(t *testing.T) { FilterPolicy: mockStruct, }, { - Name: aws.String("name.fifo"), + Name: aws.String("name"), Service: aws.String("svc"), }, { - Name: aws.String("name1.fifo"), + Name: aws.String("name1"), Service: aws.String("svc"), }, }, diff --git a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go index ff2719d7caf..30b913de713 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_service_integration_test.go @@ -47,8 +47,6 @@ func TestWorkerService_Template(t *testing.T) { v, ok := content.(*manifest.WorkerService) require.True(t, ok) - v.Subscribe.Topics = v.Subscribe.Subscriptions() - ws, err := workspace.New() require.NoError(t, err) From 52ef8f567f3218acc0c263eaea6374b1ca86709c Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 23 Sep 2022 09:57:22 -0700 Subject: [PATCH 18/19] nits --- internal/pkg/cli/deploy/svc.go | 2 +- .../cloudformation/stack/transformers.go | 6 +- .../cloudformation/stack/transformers_test.go | 424 ++++++++++-------- .../deploy/cloudformation/stack/worker_svc.go | 2 +- internal/pkg/manifest/validate.go | 9 +- internal/pkg/manifest/worker_svc.go | 6 +- internal/pkg/manifest/worker_svc_test.go | 2 +- 7 files changed, 246 insertions(+), 205 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index e5ceab34e9d..e7c72c1e572 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1171,7 +1171,7 @@ func (d *workerSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (* for _, topic := range topics { topicARNs = append(topicARNs, topic.ARN()) } - subs := d.wsMft.Subscribe.Subscriptions() + subs := d.wsMft.Subscriptions() if err = validateTopicsExist(subs, topicARNs, d.app.Name, d.env.Name); err != nil { return nil, err } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 484916c7183..fec38ae8a87 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -817,8 +817,8 @@ func convertPublish(topics []manifest.Topic, accountID, region, app, env, svc st return &publishers, nil } -func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, error) { - if s.Topics == nil { +func convertSubscribe(s *manifest.WorkerService) (*template.SubscribeOpts, error) { + if s.Subscribe.Topics == nil { return nil, nil } var subscriptions template.SubscribeOpts @@ -829,7 +829,7 @@ func convertSubscribe(s manifest.SubscribeConfig) (*template.SubscribeOpts, erro } subscriptions.Topics = append(subscriptions.Topics, ts) } - subscriptions.Queue = convertQueue(s.Queue) + subscriptions.Queue = convertQueue(s.Subscribe.Queue) return &subscriptions, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 063e61aeac4..961ee95b0aa 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1455,28 +1455,32 @@ func Test_convertSubscribe(t *testing.T) { "store": []string{"example_corp"}, } testCases := map[string]struct { - inSubscribe manifest.SubscribeConfig + inSubscribe *manifest.WorkerService wanted *template.SubscribeOpts }{ "empty subscription": { //1 - inSubscribe: manifest.SubscribeConfig{}, + inSubscribe: &manifest.WorkerService{}, wanted: nil, }, "valid subscribe": { //2 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - }, - }, - Queue: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + }, }, }, }, @@ -1498,14 +1502,18 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with default queue configs": { //3 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + }, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1518,18 +1526,22 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with queue enabled": { //4 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Enabled: aws.Bool(true), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Enabled: aws.Bool(true), + }, + FilterPolicy: mockStruct, + }, }, - FilterPolicy: mockStruct, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1544,25 +1556,29 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with minimal queue": { //5 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + }, }, + FilterPolicy: mockStruct, }, }, - FilterPolicy: mockStruct, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1584,26 +1600,30 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with high throughput fifo sqs": { //6 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: manifest.FIFOAdvanceConfigOrBool{Advanced: manifest.FIFOAdvanceConfig{HighThroughputFifo: aws.Bool(true)}}, + }, }, - FIFO: manifest.FIFOAdvanceConfigOrBool{Advanced: manifest.FIFOAdvanceConfig{HighThroughputFifo: aws.Bool(true)}}, + FilterPolicy: mockStruct, }, }, - FilterPolicy: mockStruct, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1629,26 +1649,30 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with custom minimal fifo sqs config values": { - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, + }, }, - FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, + FilterPolicy: mockStruct, }, }, - FilterPolicy: mockStruct, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1671,32 +1695,36 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with custom complete fifo sqs config values": { //7 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), - }, - FIFO: manifest.FIFOAdvanceConfigOrBool{ - Advanced: manifest.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, + FilterPolicy: mockStruct, }, }, - FilterPolicy: mockStruct, + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1723,36 +1751,40 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with custom complete fifo sqs config and standard topic subscription to a default standard queue": { //8 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), - }, - FIFO: manifest.FIFOAdvanceConfigOrBool{ - Advanced: manifest.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), }, }, - FilterPolicy: mockStruct, - }, - { - Name: aws.String("name"), - Service: aws.String("svc"), + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1783,40 +1815,44 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with custom complete fifo sqs config and multiple standard topic subscriptions to a default standard queue": { //9 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), - }, - FIFO: manifest.FIFOAdvanceConfigOrBool{ - Advanced: manifest.FIFOAdvanceConfig{ - FIFOThroughputLimit: aws.String("queue"), - DeduplicationScope: aws.String("perQueue"), - ContentBasedDeduplication: aws.Bool(true), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + FIFO: manifest.FIFOAdvanceConfigOrBool{ + Advanced: manifest.FIFOAdvanceConfig{ + FIFOThroughputLimit: aws.String("queue"), + DeduplicationScope: aws.String("perQueue"), + ContentBasedDeduplication: aws.Bool(true), + }, + }, }, }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1"), + Service: aws.String("svc"), }, }, - FilterPolicy: mockStruct, - }, - { - Name: aws.String("name"), - Service: aws.String("svc"), - }, - { - Name: aws.String("name1"), - Service: aws.String("svc"), + Queue: manifest.SQSQueue{}, }, }, - Queue: manifest.SQSQueue{}, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1851,31 +1887,35 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with standard sqs config and fifo topic subscription to a default fifo queue": { //10 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + }, }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), }, }, - FilterPolicy: mockStruct, - }, - { - Name: aws.String("name"), - Service: aws.String("svc"), + Queue: manifest.SQSQueue{ + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, + }, }, }, - Queue: manifest.SQSQueue{ - FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, - }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ @@ -1903,35 +1943,39 @@ func Test_convertSubscribe(t *testing.T) { }, }, "valid subscribe with standard sqs config and multiple fifo topic subscriptions to a default fifo queue": { //11 - inSubscribe: manifest.SubscribeConfig{ - Topics: []manifest.TopicSubscription{ - { - Name: aws.String("name"), - Service: aws.String("svc"), - Queue: manifest.SQSQueueOrBool{ - Advanced: manifest.SQSQueue{ - Retention: &duration111Seconds, - Delay: &duration111Seconds, - Timeout: &duration111Seconds, - DeadLetter: manifest.DeadLetterQueue{ - Tries: aws.Uint16(35), + inSubscribe: &manifest.WorkerService{ + WorkerServiceConfig: manifest.WorkerServiceConfig{ + Subscribe: manifest.SubscribeConfig{ + Topics: []manifest.TopicSubscription{ + { + Name: aws.String("name"), + Service: aws.String("svc"), + Queue: manifest.SQSQueueOrBool{ + Advanced: manifest.SQSQueue{ + Retention: &duration111Seconds, + Delay: &duration111Seconds, + Timeout: &duration111Seconds, + DeadLetter: manifest.DeadLetterQueue{ + Tries: aws.Uint16(35), + }, + }, }, + FilterPolicy: mockStruct, + }, + { + Name: aws.String("name"), + Service: aws.String("svc"), + }, + { + Name: aws.String("name1"), + Service: aws.String("svc"), }, }, - FilterPolicy: mockStruct, - }, - { - Name: aws.String("name"), - Service: aws.String("svc"), - }, - { - Name: aws.String("name1"), - Service: aws.String("svc"), + Queue: manifest.SQSQueue{ + FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, + }, }, }, - Queue: manifest.SQSQueue{ - FIFO: manifest.FIFOAdvanceConfigOrBool{Enable: aws.Bool(true)}, - }, }, wanted: &template.SubscribeOpts{ Topics: []*template.TopicSubscription{ diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc.go b/internal/pkg/deploy/cloudformation/stack/worker_svc.go index 9253c9b77ae..73abd19eef4 100644 --- a/internal/pkg/deploy/cloudformation/stack/worker_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/worker_svc.go @@ -104,7 +104,7 @@ func (s *WorkerService) Template() (string, error) { if err != nil { return "", err } - subscribe, err := convertSubscribe(s.manifest.Subscribe) + subscribe, err := convertSubscribe(s.manifest) if err != nil { return "", err } diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 3cbb582129c..8bcf78171a1 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1457,10 +1457,7 @@ func (q SQSQueueOrBool) validate() error { if q.IsEmpty() { return nil } - if err := q.Advanced.validate(); err != nil { - return err - } - return q.Advanced.FIFO.validate() + return q.Advanced.validate() } // validate returns nil if SQSQueue is configured correctly. @@ -1471,7 +1468,7 @@ func (q SQSQueue) validate() error { if err := q.DeadLetter.validate(); err != nil { return fmt.Errorf(`validate "dead_letter": %w`, err) } - return nil + return q.FIFO.validate() } // validate returns nil if FIFOAdvanceConfig is configured correctly. @@ -1497,7 +1494,7 @@ func (q FIFOAdvanceConfig) validate() error { // validateFIFO returns nil if FIFOAdvanceConfigOrBool is configured correctly. func (q FIFOAdvanceConfigOrBool) validate() error { - if q.Enable != nil { + if q.IsEmpty() { return nil } return q.Advanced.validate() diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index b81630158fd..2c7b820e1a6 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -242,12 +242,12 @@ func (s *WorkerService) EnvFile() string { // Subscriptions returns a list of TopicSubscriotion objects which represent the SNS topics the service // receives messages from. This method also appends ".fifo" to the topics and returns a new set of subs. -func (s *SubscribeConfig) Subscriptions() []TopicSubscription { +func (s *WorkerService) Subscriptions() []TopicSubscription { var subs []TopicSubscription - for _, topic := range s.Topics { + for _, topic := range s.Subscribe.Topics { topicSubscription := topic // if condition appends .fifo suffix to the topic which doesn't have topic specific queue and subscribing to default FIFO queue. - if topic.Queue.IsEmpty() && !s.Queue.IsEmpty() && s.Queue.FIFO.IsEnabled() { + if topic.Queue.IsEmpty() && !s.Subscribe.Queue.IsEmpty() && s.Subscribe.Queue.FIFO.IsEnabled() { topicSubscription.Name = aws.String(aws.StringValue(topic.Name) + ".fifo") } else if !topic.Queue.IsEmpty() && !topic.Queue.Advanced.IsEmpty() && topic.Queue.Advanced.FIFO.IsEnabled() { // else if condition appends .fifo suffix to the topic which has topic specific FIFO queue configuration. topicSubscription.Name = aws.String(aws.StringValue(topic.Name) + ".fifo") diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go index 3ee11a56fae..9edbc38779c 100644 --- a/internal/pkg/manifest/worker_svc_test.go +++ b/internal/pkg/manifest/worker_svc_test.go @@ -1549,7 +1549,7 @@ func TestWorkerService_Subscriptions(t *testing.T) { t.Run(name, func(t *testing.T) { // WHEN - svc.Subscribe.Topics = svc.Subscribe.Subscriptions() + svc.Subscribe.Topics = svc.Subscriptions() // THEN require.Equal(t, tc.expected, svc) From 9b811c641ea92ffb5f3cf52d08332e6beb43c67e Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 23 Sep 2022 10:08:52 -0700 Subject: [PATCH 19/19] nits --- .../stack/testdata/workloads/worker-manifest.yml | 2 +- .../stack/testdata/workloads/worker-test.stack.yml | 2 +- internal/pkg/manifest/validate.go | 10 +++++----- internal/pkg/manifest/validate_test.go | 14 +++++++------- internal/pkg/manifest/worker_svc.go | 4 ++-- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml index 1ccf18d4aec..2ac94fec817 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-manifest.yml @@ -58,7 +58,7 @@ subscribe: queue: fifo: content_based_deduplication: true - high_throughput_fifo: true + high_throughput: true - name: nonfifotopic service: nonfifotopic queue: diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml index 0a91e614f3d..3d607b13120 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.stack.yml @@ -64,7 +64,7 @@ Metadata: queue: fifo: content_based_deduplication: true - high_throughput_fifo: true + high_throughput: true - name: nonfifotopic service: nonfifotopic queue: diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 8bcf78171a1..6b20361151a 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1487,7 +1487,7 @@ func (q FIFOAdvanceConfig) validate() error { return err } if aws.StringValue(q.FIFOThroughputLimit) == sqsFIFOThroughputLimitPerMessageGroupID && aws.StringValue(q.DeduplicationScope) == sqsDeduplicationScopeQueue { - return fmt.Errorf(`"fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`) + return fmt.Errorf(`"throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`) } return nil } @@ -1506,15 +1506,15 @@ func (q FIFOAdvanceConfig) validateHighThroughputFIFO() error { } if q.FIFOThroughputLimit != nil { return &errFieldMutualExclusive{ - firstField: "high_throughput_fifo", - secondField: "fifo_throughput_limit", + firstField: "high_throughput", + secondField: "throughput_limit", mustExist: false, } } if q.DeduplicationScope != nil { return &errFieldMutualExclusive{ - firstField: "high_throughput_fifo", + firstField: "high_throughput", secondField: "deduplication_scope", mustExist: false, } @@ -1531,7 +1531,7 @@ func (q FIFOAdvanceConfig) validateDeduplicationScope() error { func (q FIFOAdvanceConfig) validateFIFOThroughputLimit() error { if q.FIFOThroughputLimit != nil && !contains(aws.StringValue(q.FIFOThroughputLimit), validSQSFIFOThroughputLimitValues) { - return fmt.Errorf(`validate "fifo_throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validSQSFIFOThroughputLimitValues, "or")) + return fmt.Errorf(`validate "throughput_limit": fifo throughput limit value must be one of %s`, english.WordSeries(validSQSFIFOThroughputLimitValues, "or")) } return nil } diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 4aa17b543ac..99347969069 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2818,7 +2818,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": validate "fifo_throughput_limit": fifo throughput limit value must be one of perMessageGroupId or perQueue`), + wanted: errors.New(`validate "queue": validate "throughput_limit": fifo throughput limit value must be one of perMessageGroupId or perQueue`), }, "should not return error if valid fifo throughput limit": { in: TopicSubscription{ @@ -2880,7 +2880,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, wanted: nil, }, - "should return error if high_throughput_fifo is defined along with deduplication_scope": { + "should return error if high_throughput is defined along with deduplication_scope": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), @@ -2899,9 +2899,9 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput_fifo" and "deduplication_scope"`), + wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput" and "deduplication_scope"`), }, - "should return error if high_throughput_fifo is defined along with fifo_throughput_limit": { + "should return error if high_throughput is defined along with throughput_limit": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), @@ -2920,9 +2920,9 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput_fifo" and "fifo_throughput_limit"`), + wanted: errors.New(`validate "queue": must specify one, not both, of "high_throughput" and "throughput_limit"`), }, - "should return error if invalid combination of deduplication_scope and fifo_throughput_limit is defined": { + "should return error if invalid combination of deduplication_scope and throughput_limit is defined": { in: TopicSubscription{ Name: aws.String("mockTopic"), Service: aws.String("mockservice"), @@ -2941,7 +2941,7 @@ func TestTopicSubscription_validate(t *testing.T) { }, }, }, - wanted: errors.New(`validate "queue": "fifo_throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`), + wanted: errors.New(`validate "queue": "throughput_limit" must be set to "perQueue" when "deduplication_scope" is set to "queue"`), }, "should not return error if valid standard queue config defined": { in: TopicSubscription{ diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 2c7b820e1a6..5d68b425116 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -138,8 +138,8 @@ func (f *FIFOAdvanceConfigOrBool) IsEnabled() bool { type FIFOAdvanceConfig struct { ContentBasedDeduplication *bool `yaml:"content_based_deduplication"` DeduplicationScope *string `yaml:"deduplication_scope"` - FIFOThroughputLimit *string `yaml:"fifo_throughput_limit"` - HighThroughputFifo *bool `yaml:"high_throughput_fifo"` + FIFOThroughputLimit *string `yaml:"throughput_limit"` + HighThroughputFifo *bool `yaml:"high_throughput"` } // IsEmpty returns true if the FifoAdvanceConfig struct has all nil values.