chore(manifest): move more validation from template pkg (#2878)

iamhopaul123 · web-flow · commit 37b569fe0538 · 2021-09-28T20:22:15.000Z
Part of #2818. This PR adds validation for - task def override - sidecar mount points - storage config - publish/subscribe Also make `topic.queue` to able to accept either bool or map as input so as users can better specify a topic specific queue with default config.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
diff --git a/internal/pkg/cli/svc_deploy.go b/internal/pkg/cli/svc_deploy.go
@@ -899,11 +899,11 @@ func (o *deploySvcOpts) buildWorkerQueueNames() string {
 	sb := new(strings.Builder)
 	first := true
 	for _, subscription := range o.subscriptions {
-		if subscription.Queue == nil {
+		if subscription.Queue.IsEmpty() {
 			continue
 		}
-		topicSvc := template.StripNonAlphaNumFunc(subscription.Service)
-		topicName := template.StripNonAlphaNumFunc(subscription.Name)
+		topicSvc := template.StripNonAlphaNumFunc(aws.StringValue(subscription.Service))
+		topicName := template.StripNonAlphaNumFunc(aws.StringValue(subscription.Name))
 		subName := fmt.Sprintf("%s%sEventsQueue", topicSvc, strings.Title(topicName))
 		if first {
 			sb.WriteString(subName)
diff --git a/internal/pkg/cli/svc_init.go b/internal/pkg/cli/svc_init.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"strconv"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/copilot-cli/internal/pkg/docker/dockerfile"
 
 	"github.com/aws/copilot-cli/internal/pkg/docker/dockerengine"
@@ -484,8 +485,8 @@ func (o *initSvcOpts) askSvcPublishers() (err error) {
 	subscriptions := make([]manifest.TopicSubscription, 0, len(topics))
 	for _, t := range topics {
 		subscriptions = append(subscriptions, manifest.TopicSubscription{
-			Name:    t.Name(),
-			Service: t.Workload(),
+			Name:    aws.String(t.Name()),
+			Service: aws.String(t.Workload()),
 		})
 	}
 	o.topics = subscriptions
@@ -500,8 +501,8 @@ func parseSerializedSubscription(input string) (manifest.TopicSubscription, erro
 		return manifest.TopicSubscription{}, fmt.Errorf("parse subscription from key: %s", input)
 	}
 	return manifest.TopicSubscription{
-		Name:    attrs[2],
-		Service: attrs[1],
+		Name:    aws.String(attrs[2]),
+		Service: aws.String(attrs[1]),
 	}, nil
 }
 
diff --git a/internal/pkg/cli/svc_init_test.go b/internal/pkg/cli/svc_init_test.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/copilot-cli/internal/pkg/docker/dockerfile"
 
 	"github.com/aws/copilot-cli/internal/pkg/deploy"
@@ -695,8 +696,8 @@ func TestSvcInitOpts_Execute(t *testing.T) {
 					gomock.Any(),
 				).Return([]manifest.TopicSubscription{
 					{
-						Name:    "thetopic",
-						Service: "theservice",
+						Name:    aws.String("thetopic"),
+						Service: aws.String("theservice"),
 					},
 				}, nil)
 			},
diff --git a/internal/pkg/cli/validate.go b/internal/pkg/cli/validate.go
@@ -16,6 +16,7 @@ import (
 
 	"github.com/spf13/afero"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/copilot-cli/internal/pkg/addon"
 	"github.com/aws/copilot-cli/internal/pkg/aws/apprunner"
@@ -678,13 +679,11 @@ func validateSubscriptionKey(val interface{}) error {
 	if err != nil {
 		return errSubscribeBadFormat
 	}
-	err = validatePubSubName(sub.Name)
-	if err != nil {
-		return fmt.Errorf("invalid topic subscription topic name `%s`: %w", sub.Name, err)
+	if err := validatePubSubName(aws.StringValue(sub.Name)); err != nil {
+		return fmt.Errorf("invalid topic subscription topic name `%s`: %w", aws.StringValue(sub.Name), err)
 	}
-	err = basicNameValidation(sub.Service)
-	if err != nil {
-		return fmt.Errorf("invalid topic subscription service name `%s`: %w", sub.Service, err)
+	if err = basicNameValidation(aws.StringValue(sub.Service)); err != nil {
+		return fmt.Errorf("invalid topic subscription service name `%s`: %w", aws.StringValue(sub.Service), err)
 	}
 	return nil
 }
@@ -715,7 +714,7 @@ func validateTopicsExist(subscriptions []manifest.TopicSubscription, topicARNs [
 	}
 
 	for _, ts := range subscriptions {
-		topicName := fmt.Sprintf(resourceNameFormat, app, env, ts.Service, ts.Name)
+		topicName := fmt.Sprintf(resourceNameFormat, app, env, aws.StringValue(ts.Service), aws.StringValue(ts.Name))
 		if !contains(topicName, validTopicResources) {
 			return fmt.Errorf(fmtErrTopicSubscriptionNotAllowed, topicName, env)
 		}
diff --git a/internal/pkg/cli/validate_test.go b/internal/pkg/cli/validate_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/copilot-cli/internal/pkg/manifest"
 
 	"github.com/spf13/afero"
@@ -796,14 +797,16 @@ func Test_validateTopicsExist(t *testing.T) {
 	duration10Hours := 10 * time.Hour
 	testGoodTopics := []manifest.TopicSubscription{
 		{
-			Name:    "events",
-			Service: "database",
+			Name:    aws.String("events"),
+			Service: aws.String("database"),
 		},
 		{
-			Name:    "orders",
-			Service: "database",
-			Queue: &manifest.SQSQueue{
-				Retention: &duration10Hours,
+			Name:    aws.String("orders"),
+			Service: aws.String("database"),
+			Queue: manifest.SQSQueueOrBool{
+				Advanced: manifest.SQSQueue{
+					Retention: &duration10Hours,
+				},
 			},
 		},
 	}
diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go
@@ -723,22 +723,28 @@ func convertSubscribe(s manifest.SubscribeConfig, validTopicARNs []string, accou
 func convertTopicSubscription(t manifest.TopicSubscription, url, accountID, app, env, svc string) (*template.TopicSubscription, error) {
 	err := validateTopicSubscription(t)
 	if err != nil {
-		return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err)
+		return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, aws.StringValue(t.Name), err)
 	}
-	queue, err := convertQueue(t.Queue)
+	if aws.BoolValue(t.Queue.Enabled) {
+		return &template.TopicSubscription{
+			Name:    t.Name,
+			Service: t.Service,
+			Queue:   &template.SQSQueue{},
+		}, nil
+	}
+	queue, err := convertQueue(t.Queue.Advanced)
 	if err != nil {
-		return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, t.Name, err)
+		return nil, fmt.Errorf(`invalid topic subscription "%s": %w`, aws.StringValue(t.Name), err)
 	}
-
 	return &template.TopicSubscription{
-		Name:    aws.String(t.Name),
-		Service: aws.String(t.Service),
+		Name:    t.Name,
+		Service: t.Service,
 		Queue:   queue,
 	}, nil
 }
 
-func convertQueue(q *manifest.SQSQueue) (*template.SQSQueue, error) {
-	if q == nil {
+func convertQueue(q manifest.SQSQueue) (*template.SQSQueue, error) {
+	if q.IsEmpty() {
 		return nil, nil
 	}
 	retention, err := convertRetention(q.Retention)
diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go
@@ -1661,11 +1661,11 @@ func Test_convertSubscribe(t *testing.T) {
 			inSubscribe: manifest.SubscribeConfig{
 				Topics: []manifest.TopicSubscription{
 					{
-						Name:    "name",
-						Service: "svc",
+						Name:    aws.String("name"),
+						Service: aws.String("svc"),
 					},
 				},
-				Queue: &manifest.SQSQueue{
+				Queue: manifest.SQSQueue{
 					Retention: &duration111Seconds,
 					Delay:     &duration111Seconds,
 					Timeout:   &duration111Seconds,
@@ -1695,28 +1695,32 @@ func Test_convertSubscribe(t *testing.T) {
 			inSubscribe: manifest.SubscribeConfig{
 				Topics: []manifest.TopicSubscription{
 					{
-						Name:    "name",
-						Service: "svc",
+						Name:    aws.String("name"),
+						Service: aws.String("svc"),
+						Queue: manifest.SQSQueueOrBool{
+							Enabled: aws.Bool(true),
+						},
 					},
 				},
-				Queue: &manifest.SQSQueue{},
+				Queue: manifest.SQSQueue{},
 			},
 			wanted: &template.SubscribeOpts{
 				Topics: []*template.TopicSubscription{
 					{
 						Name:    aws.String("name"),
 						Service: aws.String("svc"),
+						Queue:   &template.SQSQueue{},
 					},
 				},
-				Queue: &template.SQSQueue{},
+				Queue: nil,
 			},
 		},
 		"invalid topic name": {
 			inSubscribe: manifest.SubscribeConfig{
 				Topics: []manifest.TopicSubscription{
 					{
-						Name:    "t@p!c1~",
-						Service: "service1",
+						Name:    aws.String("t@p!c1~"),
+						Service: aws.String("service1"),
 					},
 				},
 			},
@@ -1726,8 +1730,8 @@ func Test_convertSubscribe(t *testing.T) {
 			inSubscribe: manifest.SubscribeConfig{
 				Topics: []manifest.TopicSubscription{
 					{
-						Name:    "topic1",
-						Service: "s#rv!ce1~",
+						Name:    aws.String("topic1"),
+						Service: aws.String("s#rv!ce1~"),
 					},
 				},
 			},
@@ -1737,11 +1741,11 @@ func Test_convertSubscribe(t *testing.T) {
 			inSubscribe: manifest.SubscribeConfig{
 				Topics: []manifest.TopicSubscription{
 					{
-						Name:    "name",
-						Service: "svc",
+						Name:    aws.String("name"),
+						Service: aws.String("svc"),
 					},
 				},
-				Queue: &manifest.SQSQueue{
+				Queue: manifest.SQSQueue{
 					Delay: &duration5Days,
 				},
 			},
diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go
@@ -444,11 +444,11 @@ func isCorrectSvcNameFormat(s string) bool {
 }
 
 func validateTopicSubscription(ts manifest.TopicSubscription) error {
-	if err := validatePubSubName(ts.Name); err != nil {
+	if err := validatePubSubName(aws.StringValue(ts.Name)); err != nil {
 		return err
 	}
 
-	if err := validateSvcName(ts.Service); err != nil {
+	if err := validateSvcName(aws.StringValue(ts.Service)); err != nil {
 		return err
 	}
 
diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go
@@ -533,20 +533,20 @@ func TestValidateTopicSubscription(t *testing.T) {
 	}{
 		"good case": {
 			inTS: manifest.TopicSubscription{
-				Name:    "name2",
-				Service: "svc",
+				Name:    aws.String("name2"),
+				Service: aws.String("svc"),
 			},
 			wantErr: nil,
 		},
 		"empty name": {
 			inTS: manifest.TopicSubscription{
-				Service: "svc",
+				Service: aws.String("svc"),
 			},
 			wantErr: errMissingPublishTopicField,
 		},
 		"empty svc name": {
 			inTS: manifest.TopicSubscription{
-				Name: "theName",
+				Name: aws.String("theName"),
 			},
 			wantErr: errInvalidSvcName,
 		},
diff --git a/internal/pkg/deploy/cloudformation/stack/worker_svc_test.go b/internal/pkg/deploy/cloudformation/stack/worker_svc_test.go
@@ -108,8 +108,8 @@ Outputs:
 				testWorkerSvcManifestWithBadSubscribe.Subscribe = manifest.SubscribeConfig{
 					Topics: []manifest.TopicSubscription{
 						{
-							Name:    "name",
-							Service: "un@cept#ble",
+							Name:    aws.String("name"),
+							Service: aws.String("un@cept#ble"),
 						},
 					},
 				}
diff --git a/internal/pkg/initialize/workload_test.go b/internal/pkg/initialize/workload_test.go
@@ -722,8 +722,8 @@ func TestWorkloadInitializer_Service(t *testing.T) {
 			inSvcPort:        80,
 			inTopics: []manifest.TopicSubscription{
 				{
-					Name:    "theTopic",
-					Service: "publisher",
+					Name:    aws.String("theTopic"),
+					Service: aws.String("publisher"),
 				},
 			},
 
diff --git a/internal/pkg/manifest/storage_test.go b/internal/pkg/manifest/storage_test.go
@@ -15,7 +15,7 @@ type testVolume struct {
 	EFS EFSConfigOrBool `yaml:"efs"`
 }
 
-func Test_UnmarshalEFS(t *testing.T) {
+func TestEFSConfigOrBool_UnmarshalYAML(t *testing.T) {
 	testCases := map[string]struct {
 		manifest []byte
 		want     testVolume
@@ -47,7 +47,7 @@ efs:
 				},
 			},
 		},
-		"with just managed ": {
+		"with just managed": {
 			manifest: []byte(`
 efs: true`),
 			want: testVolume{
diff --git a/internal/pkg/manifest/svc_test.go b/internal/pkg/manifest/svc_test.go
@@ -337,18 +337,20 @@ subscribe:
 						Subscribe: SubscribeConfig{
 							Topics: []TopicSubscription{
 								{
-									Name:    "publisher1",
-									Service: "testpubsvc",
+									Name:    aws.String("publisher1"),
+									Service: aws.String("testpubsvc"),
 								},
 								{
-									Name:    "publisher2",
-									Service: "testpubjob",
-									Queue: &SQSQueue{
-										Timeout: &duration15Seconds,
+									Name:    aws.String("publisher2"),
+									Service: aws.String("testpubjob"),
+									Queue: SQSQueueOrBool{
+										Advanced: SQSQueue{
+											Timeout: &duration15Seconds,
+										},
 									},
 								},
 							},
-							Queue: &SQSQueue{
+							Queue: SQSQueue{
 								Delay: &duration15Seconds,
 								DeadLetter: DeadLetterQueue{
 									Tries: aws.Uint16(5),
diff --git a/internal/pkg/manifest/transform.go b/internal/pkg/manifest/transform.go
@@ -26,6 +26,7 @@ var defaultTransformers = []mergo.Transformers{
 	rangeTransformer{},
 	efsConfigOrBoolTransformer{},
 	efsVolumeConfigurationTransformer{},
+	sqsQueueOrBoolTransformer{},
 }
 
 // See a complete list of `reflect.Kind` here: https://pkg.go.dev/reflect#Kind.
@@ -300,6 +301,31 @@ func (t efsVolumeConfigurationTransformer) Transformer(typ reflect.Type) func(ds
 	}
 }
 
+type sqsQueueOrBoolTransformer struct{}
+
+// Transformer returns custom merge logic for SQSQueueOrBool's fields.
+func (q sqsQueueOrBoolTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error {
+	if typ != reflect.TypeOf(SQSQueueOrBool{}) {
+		return nil
+	}
+	return func(dst, src reflect.Value) error {
+		dstStruct, srcStruct := dst.Interface().(SQSQueueOrBool), src.Interface().(SQSQueueOrBool)
+
+		if !srcStruct.Advanced.IsEmpty() {
+			dstStruct.Enabled = nil
+		}
+
+		if srcStruct.Enabled != nil {
+			dstStruct.Advanced = SQSQueue{}
+		}
+
+		if dst.CanSet() { // For extra safety to prevent panicking.
+			dst.Set(reflect.ValueOf(dstStruct))
+		}
+		return nil
+	}
+}
+
 type basicTransformer struct{}
 
 // Transformer returns custom merge logic for volume's fields.
diff --git a/internal/pkg/manifest/transform_test.go b/internal/pkg/manifest/transform_test.go
diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go
diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go
diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go
diff --git a/internal/pkg/manifest/worker_svc_test.go b/internal/pkg/manifest/worker_svc_test.go
diff --git a/site/content/docs/manifest/worker-service.en.md b/site/content/docs/manifest/worker-service.en.md

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ import (`
`16`	`16`
`17`	`17`	`"github.com/spf13/afero"`
`18`	`18`
	`19`	`+ "github.com/aws/aws-sdk-go/aws"`
`19`	`20`	`"github.com/aws/aws-sdk-go/aws/arn"`
`20`	`21`	`"github.com/aws/copilot-cli/internal/pkg/addon"`
`21`	`22`	`"github.com/aws/copilot-cli/internal/pkg/aws/apprunner"`
`@@ -678,13 +679,11 @@ func validateSubscriptionKey(val interface{}) error {`
`678`	`679`	`if err != nil {`
`679`	`680`	`return errSubscribeBadFormat`
`680`	`681`	`}`
`681`		`- err = validatePubSubName(sub.Name)`
`682`		`- if err != nil {`
`683`		- return fmt.Errorf("invalid topic subscription topic name `%s`: %w", sub.Name, err)
	`682`	`+ if err := validatePubSubName(aws.StringValue(sub.Name)); err != nil {`
	`683`	+ return fmt.Errorf("invalid topic subscription topic name `%s`: %w", aws.StringValue(sub.Name), err)
`684`	`684`	`}`
`685`		`- err = basicNameValidation(sub.Service)`
`686`		`- if err != nil {`
`687`		- return fmt.Errorf("invalid topic subscription service name `%s`: %w", sub.Service, err)
	`685`	`+ if err = basicNameValidation(aws.StringValue(sub.Service)); err != nil {`
	`686`	+ return fmt.Errorf("invalid topic subscription service name `%s`: %w", aws.StringValue(sub.Service), err)
`688`	`687`	`}`
`689`	`688`	`return nil`
`690`	`689`	`}`
`@@ -715,7 +714,7 @@ func validateTopicsExist(subscriptions []manifest.TopicSubscription, topicARNs [`
`715`	`714`	`}`
`716`	`715`
`717`	`716`	`for _, ts := range subscriptions {`
`718`		`- topicName := fmt.Sprintf(resourceNameFormat, app, env, ts.Service, ts.Name)`
	`717`	`+ topicName := fmt.Sprintf(resourceNameFormat, app, env, aws.StringValue(ts.Service), aws.StringValue(ts.Name))`
`719`	`718`	`if !contains(topicName, validTopicResources) {`
`720`	`719`	`return fmt.Errorf(fmtErrTopicSubscriptionNotAllowed, topicName, env)`
`721`	`720`	`}`
Original file line number	Diff line number	Diff line change
`@@ -444,11 +444,11 @@ func isCorrectSvcNameFormat(s string) bool {`
`444`	`444`	`}`
`445`	`445`
`446`	`446`	`func validateTopicSubscription(ts manifest.TopicSubscription) error {`
`447`		`- if err := validatePubSubName(ts.Name); err != nil {`
	`447`	`+ if err := validatePubSubName(aws.StringValue(ts.Name)); err != nil {`
`448`	`448`	`return err`
`449`	`449`	`}`
`450`	`450`
`451`		`- if err := validateSvcName(ts.Service); err != nil {`
	`451`	`+ if err := validateSvcName(aws.StringValue(ts.Service)); err != nil {`
`452`	`452`	`return err`
`453`	`453`	`}`
`454`	`454`
Original file line number	Diff line number	Diff line change
`@@ -108,8 +108,8 @@ Outputs:`
`108`	`108`	`testWorkerSvcManifestWithBadSubscribe.Subscribe = manifest.SubscribeConfig{`
`109`	`109`	`Topics: []manifest.TopicSubscription{`
`110`	`110`	`{`
`111`		`- Name: "name",`
`112`		`- Service: "un@cept#ble",`
	`111`	`+ Name: aws.String("name"),`
	`112`	`+ Service: aws.String("un@cept#ble"),`
`113`	`113`	`},`
`114`	`114`	`},`
`115`	`115`	`}`