From d481cabfed2a526ea861c7a0eacb6dad9346969e Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 4 Oct 2022 11:09:44 -0700 Subject: [PATCH 01/12] chore: allow cdn certificate import without http cert import --- internal/pkg/cli/deploy/svc.go | 23 +++++++++++--------- internal/pkg/cli/deploy/svc_test.go | 25 +++++++++++++++++++++- internal/pkg/manifest/validate_env.go | 7 ++---- internal/pkg/manifest/validate_env_test.go | 14 ++++++++++-- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index e7c72c1e572..55ea5df30d6 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1360,12 +1360,13 @@ func (d *backendSvcDeployer) validateALBRuntime() error { } func (d *lbWebSvcDeployer) validateALBRuntime() error { - hasImportedCerts := len(d.envConfig.HTTPConfig.Public.Certificates) != 0 - if d.lbMft.RoutingRule.RedirectToHTTPS != nil && d.app.Domain == "" && !hasImportedCerts { + hasALBCerts := len(d.envConfig.HTTPConfig.Public.Certificates) != 0 + hasCDNCerts := d.envConfig.CDNConfig.Config.Certificate != nil + if d.lbMft.RoutingRule.RedirectToHTTPS != nil && d.app.Domain == "" && !hasALBCerts && !hasCDNCerts { return fmt.Errorf("cannot configure http to https redirect without having a domain associated with the app %q or importing any certificates in env %q", d.app.Name, d.env.Name) } if d.lbMft.RoutingRule.Alias.IsEmpty() { - if hasImportedCerts { + if hasALBCerts || hasCDNCerts { return &errSvcWithNoALBAliasDeployingToEnvWithImportedCerts{ name: d.name, envName: d.env.Name, @@ -1375,7 +1376,7 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error { } importedHostedZones := d.lbMft.RoutingRule.Alias.HostedZones() if len(importedHostedZones) != 0 { - if !hasImportedCerts { + if !hasALBCerts && !hasCDNCerts { return fmt.Errorf("cannot specify alias hosted zones %v when no certificates are imported in environment %q", importedHostedZones, d.env.Name) } if d.envConfig.CDNEnabled() { @@ -1384,20 +1385,22 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error { } } } - if hasImportedCerts { + if hasALBCerts || hasCDNCerts { aliases, err := d.lbMft.RoutingRule.Alias.ToStringSlice() if err != nil { return fmt.Errorf("convert aliases to string slice: %w", err) } - cdnCert := d.envConfig.CDNConfig.Config.Certificate albCertValidator := d.newAliasCertValidator(nil) cfCertValidator := d.newAliasCertValidator(aws.String(cloudfront.CertRegion)) - if err := albCertValidator.ValidateCertAliases(aliases, d.envConfig.HTTPConfig.Public.Certificates); err != nil { - return fmt.Errorf("validate aliases against the imported public ALB certificate for env %s: %w", d.env.Name, err) + + if hasALBCerts { + if err := albCertValidator.ValidateCertAliases(aliases, d.envConfig.HTTPConfig.Public.Certificates); err != nil { + return fmt.Errorf("validate aliases against the imported public ALB certificate for env %s: %w", d.env.Name, err) + } } - if cdnCert != nil { - if err := cfCertValidator.ValidateCertAliases(aliases, []string{*cdnCert}); err != nil { + if hasCDNCerts { + if err := cfCertValidator.ValidateCertAliases(aliases, []string{*d.envConfig.CDNConfig.Config.Certificate}); err != nil { return fmt.Errorf("validate aliases against the imported CDN certificate for env %s: %w", d.env.Name, err) } } diff --git a/internal/pkg/cli/deploy/svc_test.go b/internal/pkg/cli/deploy/svc_test.go index 074a8da2804..ff1528dda25 100644 --- a/internal/pkg/cli/deploy/svc_test.go +++ b/internal/pkg/cli/deploy/svc_test.go @@ -993,7 +993,7 @@ func TestWorkloadDeployer_DeployWorkload(t *testing.T) { m.mockServiceDeployer.EXPECT().DeployService(gomock.Any(), "mockBucket", gomock.Any()).Return(nil) }, }, - "success with http redirect disabled and certs imported": { + "success with http redirect disabled and alb certs imported": { inRedirectToHTTPS: aws.Bool(false), inAliases: manifest.Alias{ AdvancedAliases: mockMultiAliases, @@ -1017,6 +1017,29 @@ func TestWorkloadDeployer_DeployWorkload(t *testing.T) { m.mockServiceDeployer.EXPECT().DeployService(gomock.Any(), "mockBucket", gomock.Any()).Return(nil) }, }, + "success with only cdn certs imported": { + inAliases: manifest.Alias{ + AdvancedAliases: mockMultiAliases, + }, + inEnvironment: &config.Environment{ + Name: mockEnvName, + Region: "us-west-2", + }, + inEnvironmentConfig: func() *manifest.Environment { + envConfig := &manifest.Environment{} + envConfig.CDNConfig.Config.Certificate = aws.String(mockCDNCertARN) + return envConfig + }, + inApp: &config.Application{ + Name: mockAppName, + }, + mock: func(m *deployMocks) { + m.mockEndpointGetter.EXPECT().ServiceDiscoveryEndpoint().Return("mockApp.local", nil) + m.mockEnvVersionGetter.EXPECT().Version().Return("v1.42.0", nil) + m.mockValidator.EXPECT().ValidateCertAliases([]string{"example.com", "foobar.com"}, []string{mockCDNCertARN}).Return(nil) + m.mockServiceDeployer.EXPECT().DeployService(gomock.Any(), "mockBucket", gomock.Any()).Return(nil) + }, + }, "success with http redirect disabled and domain imported": { inRedirectToHTTPS: aws.Bool(false), inAliases: manifest.Alias{ diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 3081fa9c22d..885c942433d 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -49,11 +49,8 @@ func (e EnvironmentConfig) validate() error { if e.CDNEnabled() { cdnCert := e.CDNConfig.Config.Certificate if e.HTTPConfig.Public.Certificates == nil { - if cdnCert != nil { - return &errFieldMustBeSpecified{ - missingField: "http.public.certificates", - conditionalFields: []string{"cdn.certificate"}, - } + if cdnCert != nil && !aws.BoolValue(e.CDNConfig.Config.TerminateTLS) { + return errors.New("cdn.terminate_tls must be true if cdn.certificate is set without http.public.certificates") } } else { if cdnCert == nil { diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index e89bc0efcc8..85930e0ce4b 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -214,7 +214,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - "error if cdn cert specified but public certs not specified": { + "error if cdn cert specified, cdn not terminating tls, and public certs not specified": { in: EnvironmentConfig{ CDNConfig: EnvironmentCDNConfig{ Config: AdvancedCDNConfig{ @@ -222,7 +222,17 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - wantedError: "\"http.public.certificates\" must be specified if \"cdn.certificate\" is specified", + wantedError: "cdn.terminate_tls must be true if cdn.certificate is set without http.public.certificates", + }, + "success if cdn cert specified, cdn terminating tls, and no public certs": { + in: EnvironmentConfig{ + CDNConfig: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ + Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), + TerminateTLS: aws.Bool(true), + }, + }, + }, }, "error if cdn cert not specified but public certs imported": { in: EnvironmentConfig{ From 52435d04878154a42052664f358e682033a79176 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 10:34:02 -0700 Subject: [PATCH 02/12] fix env manager role --- internal/pkg/cli/deploy/env.go | 5 ++- internal/pkg/cli/deploy/env_test.go | 45 +++++++++++++++---- internal/pkg/template/env.go | 8 ++++ .../partials/environment-manager-role.yml | 2 +- 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index d250d1aacbb..7c41955dd78 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -160,7 +160,7 @@ func (d *envDeployer) validateCDN(mft *manifest.Environment) error { } } - if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && (mft.HasImportedPublicALBCerts() || d.app.Domain != "") { + if mft.CDNEnabled() && mft.CDNDoesTLSTermination() { err := d.validateALBWorkloadsDontRedirect() var redirErr *errEnvHasPublicServicesWithRedirect switch { @@ -240,7 +240,8 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, } } if ruleARN == "" { - return false, fmt.Errorf("http listener not found on service %q", svc) + // this will happen if the service doesn't support https. + return false, nil } rule, err := d.lbDescriber.DescribeRule(ctx, ruleARN) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index b60fc81dd10..50690d55512 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -415,7 +415,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, }, } - mftCDNTerminateTLS := &manifest.Environment{ + mftCDNTerminateTLSAndHTTPCert := &manifest.Environment{ EnvironmentConfig: manifest.EnvironmentConfig{ HTTPConfig: manifest.EnvironmentHTTPConfig{ Public: manifest.PublicHTTPConfig{ @@ -429,6 +429,16 @@ func TestEnvDeployer_Validate(t *testing.T) { }, }, } + mftCDNTerminateTLS := &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + Certificate: aws.String("mockCDNCertARN"), + TerminateTLS: aws.Bool(true), + }, + }, + }, + } tests := map[string]struct { app *config.Application mft *manifest.Environment @@ -436,7 +446,7 @@ func TestEnvDeployer_Validate(t *testing.T) { expected string expectedStdErr string }{ - "cdn enabled, domain imported, no public http certs and validate aliases fails": { + "cdn enabled, domain imported, no public http certs, and validate aliases fails": { app: &config.Application{ Domain: "example.com", }, @@ -452,7 +462,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, expected: "some error", }, - "cdn enabled, domain imported, no public http certs and validate aliases succeeds": { + "cdn enabled, domain imported, no public http certs, and validate aliases succeeds": { app: &config.Application{ Domain: "example.com", }, @@ -469,7 +479,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to get env stack params": { app: &config.Application{}, - mft: mftCDNTerminateTLS, + mft: mftCDNTerminateTLSAndHTTPCert, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.envDescriber.EXPECT().Params().Return(nil, errors.New("some error")) }, @@ -477,7 +487,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to get service resources": { app: &config.Application{}, - mft: mftCDNTerminateTLS, + mft: mftCDNTerminateTLSAndHTTPCert, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -492,7 +502,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to check listener rule": { app: &config.Application{}, - mft: mftCDNTerminateTLS, + mft: mftCDNTerminateTLSAndHTTPCert, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -513,7 +523,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect": { app: &config.Application{}, - mft: mftCDNTerminateTLS, + mft: mftCDNTerminateTLSAndHTTPCert, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -617,7 +627,7 @@ If you'd like to use these services without a CDN, ensure each service's A recor }, "cdn tls termination enabled, success with three services that don't redirect": { app: &config.Application{}, - mft: mftCDNTerminateTLS, + mft: mftCDNTerminateTLSAndHTTPCert, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -651,6 +661,25 @@ If you'd like to use these services without a CDN, ensure each service's A recor m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleNoRedirect, nil) }, }, + "cdn tls termination enabled, one http only service deployed": { + app: &config.Application{}, + mft: mftCDNTerminateTLS, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + } + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleNoRedirect, nil) + }, + }, } for name, tc := range tests { diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 6da87be2fed..8d8f9d516ea 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -121,6 +121,14 @@ type EnvOpts struct { DelegateDNS bool } +// HasImportedCerts returns true if any https certificates have been +// imported to the environment. +func (e *EnvOpts) HasImportedCerts() bool { + return len(e.PublicHTTPConfig.ImportedCertARNs) > 0 || + len(e.PrivateHTTPConfig.ImportedCertARNs) > 0 || + (e.CDNConfig != nil && e.CDNConfig.ImportedCertificate != nil) +} + // HTTPConfig represents configuration for a Load Balancer. type HTTPConfig struct { CIDRPrefixListIDs []string diff --git a/internal/pkg/template/templates/environment/partials/environment-manager-role.yml b/internal/pkg/template/templates/environment/partials/environment-manager-role.yml index 757a3320668..06546f1d3c1 100644 --- a/internal/pkg/template/templates/environment/partials/environment-manager-role.yml +++ b/internal/pkg/template/templates/environment/partials/environment-manager-role.yml @@ -22,7 +22,7 @@ EnvironmentManagerRole: PolicyDocument: Version: '2012-10-17' Statement: -{{- if or .PublicHTTPConfig.ImportedCertARNs .PrivateHTTPConfig.ImportedCertARNs}} +{{- if .HasImportedCerts}} - Sid: ImportedCertificates Effect: Allow Action: [ From ed827d6bf88e985315f051fb08c8944e4723e4d6 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 10:38:06 -0700 Subject: [PATCH 03/12] move cert validator creation --- internal/pkg/cli/deploy/svc.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index 55ea5df30d6..316c7e64ae9 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1391,15 +1391,14 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error { return fmt.Errorf("convert aliases to string slice: %w", err) } - albCertValidator := d.newAliasCertValidator(nil) - cfCertValidator := d.newAliasCertValidator(aws.String(cloudfront.CertRegion)) - if hasALBCerts { + albCertValidator := d.newAliasCertValidator(nil) if err := albCertValidator.ValidateCertAliases(aliases, d.envConfig.HTTPConfig.Public.Certificates); err != nil { return fmt.Errorf("validate aliases against the imported public ALB certificate for env %s: %w", d.env.Name, err) } } if hasCDNCerts { + cfCertValidator := d.newAliasCertValidator(aws.String(cloudfront.CertRegion)) if err := cfCertValidator.ValidateCertAliases(aliases, []string{*d.envConfig.CDNConfig.Config.Certificate}); err != nil { return fmt.Errorf("validate aliases against the imported CDN certificate for env %s: %w", d.env.Name, err) } From 73e624996cd2428f1539d39d122c5b909dff11e9 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 11:10:08 -0700 Subject: [PATCH 04/12] fix plural word --- internal/pkg/cli/deploy/errors.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index b2ed47b31a0..0eb6b3c9b26 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -46,7 +46,10 @@ type errEnvHasPublicServicesWithRedirect struct { } func (e *errEnvHasPublicServicesWithRedirect) Error() string { - return fmt.Sprintf("%v services redirect HTTP to HTTPS", len(e.services)) + return fmt.Sprintf("%v %s HTTP to HTTPS", + len(e.services), + english.PluralWord(len(e.services), "service redirects", "services redirect"), + ) } // RecommendActions returns recommended actions to be taken after the error. From b23e28a0858d6ce36456a5e9788652f61e15787d Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 11:19:12 -0700 Subject: [PATCH 05/12] fix error message with correct suggestion --- internal/pkg/cli/deploy/env_test.go | 2 +- internal/pkg/cli/deploy/errors.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 50690d55512..599e2d0df9e 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -561,7 +561,7 @@ These services will not be reachable through the CDN. To fix this, set the following field in each manifest: %s http: - redirect_to_https: true + redirect_to_https: false %s and run %scopilot svc deploy%s. If you'd like to use these services without a CDN, ensure each service's A record is pointed to the ALB. diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index 0eb6b3c9b26..32d33f2bca1 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -71,7 +71,7 @@ and run %s.`, english.PluralWord(n, "redirects", "redirect"), color.Emphasize(english.PluralWord(n, "This service", "These services")+" will not be reachable through the CDN."), english.PluralWord(n, "its", "each"), - color.HighlightCodeBlock("http:\n redirect_to_https: true"), + color.HighlightCodeBlock("http:\n redirect_to_https: false"), color.HighlightCode("copilot svc deploy"), ) } From a44c12c97bbd852806d3f585890be3594deead54 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 13:59:00 -0700 Subject: [PATCH 06/12] fixes: - env package panic - only add alb domains to cf - correct lbServiceRedirects doc --- cf-custom-resources/lib/unique-json-values.js | 25 +++++- .../test/unique-json-values-test.js | 82 +++++++++++++++---- internal/pkg/cli/deploy/env.go | 4 +- internal/pkg/cli/env_package.go | 1 + .../environment/partials/cdn-resources.yml | 1 + 5 files changed, 90 insertions(+), 23 deletions(-) diff --git a/cf-custom-resources/lib/unique-json-values.js b/cf-custom-resources/lib/unique-json-values.js index fd9d2d19d50..75fc17f7fbe 100644 --- a/cf-custom-resources/lib/unique-json-values.js +++ b/cf-custom-resources/lib/unique-json-values.js @@ -81,10 +81,20 @@ const deadlineExpired = function () { * { * "svc1": ["svc1.com", "example.com"], * "svc2": ["example.com"] + * "svc3": ["svc3.com"] * } * - * This function returns a list of unique values found in that list. - * From the previous example, UniqueValues would be: + * The input event.ResourceProperties.FilterFor is a comma delimated list + * of keys in event.ResourceProperties.Aliases to filter for. + * Taking the above map, and event.ResourceProperties.FilterFor = "svc1,svc2" + * The services considered for uniqueness is: + * { + * "svc1": ["svc1.com", "example.com"], + * "svc2": ["example.com"] + * } + * + * This function returns a list of unique values found in this map. + * For this example, UniqueValues would be: * ["svc1.com", "example.com"] */ exports.handler = async function (event, context) { @@ -96,7 +106,13 @@ exports.handler = async function (event, context) { case "Create": case "Update": const aliasesForService = JSON.parse(event.ResourceProperties.Aliases || "{}"); - const unique = new Set(Object.values(aliasesForService).flat()); + const filterFor = new Set(event.ResourceProperties.FilterFor.split(",")); + const filteredAliasesForService = Object.fromEntries( + Object.entries(aliasesForService).filter( + ([key]) => filterFor.has(key) + ) + ); + const unique = new Set(Object.values(filteredAliasesForService).flat()); responseData.UniqueValues = Array.from(unique).sort(); break; case "Delete": @@ -104,9 +120,10 @@ exports.handler = async function (event, context) { break; default: throw new Error(`Unsupported request type ${event.RequestType}`); - } + }; }; + try { await Promise.race([deadlineExpired(), handler()]); await report(event, context, "SUCCESS", physicalResourceId, responseData); diff --git a/cf-custom-resources/test/unique-json-values-test.js b/cf-custom-resources/test/unique-json-values-test.js index 59df8cbe4b3..7499ecf7a89 100644 --- a/cf-custom-resources/test/unique-json-values-test.js +++ b/cf-custom-resources/test/unique-json-values-test.js @@ -75,8 +75,8 @@ describe("Unique Aliases", () => { }); }); - const aliasTest = (name, input, expectedOutput) => { - const tt = (name, reqType, input, expectedOutput) => { + const aliasTest = (name, props, expectedOutput) => { + const tt = (name, reqType, props, expectedOutput) => { test(name, () => { const request = nock(responseURL) .put("/", (body) => { @@ -95,9 +95,7 @@ describe("Unique Aliases", () => { ResponseURL: responseURL, RequestType: reqType, RequestId: testRequestId, - ResourceProperties: { - Aliases: JSON.stringify(input), // aliases get passed as a string - }, + ResourceProperties: props, LogicalResourceId: "mockID", }) .expectResolve(() => { @@ -106,28 +104,80 @@ describe("Unique Aliases", () => { }); }; - tt(`Create/${name}`, "Create", input, expectedOutput); - tt(`Update/${name}`, "Update", input, expectedOutput); + tt(`Create/${name}`, "Create", props, expectedOutput); + tt(`Update/${name}`, "Update", props, expectedOutput); }; - aliasTest("no aliases", {}, []); + aliasTest("no aliases", { + Aliases: "", + FilterFor: "" + }, []); aliasTest("one service", { - "svc1": ["svc1.com", "example.com"], + Aliases: JSON.stringify({ + "svc1": ["svc1.com", "example.com"] + }), + FilterFor: "svc1", }, ["example.com", "svc1.com"]); + aliasTest("one service excluded", { + Aliases: JSON.stringify({ + "svc1": ["svc1.com", "example.com"] + }), + FilterFor: "svc2", + }, []); + + aliasTest("one service empty filter for", { + Aliases: JSON.stringify({ + "svc1": ["svc1.com", "example.com"] + }), + FilterFor: "", + }, []); + aliasTest("two services no common aliases", { - "svc1": ["svc1.com"], - "svc2": ["svc2.com"] + Aliases: JSON.stringify({ + "svc1": ["svc1.com"], + "svc2": ["svc2.com"] + }), + FilterFor: "svc1,svc2", }, ["svc1.com", "svc2.com"]); aliasTest("two services, one with multiple common aliases", { - "svc1": ["svc1.com"], - "svc2": ["svc2.com", "example.com"] + Aliases: JSON.stringify({ + "svc1": ["svc1.com"], + "svc2": ["svc2.com", "example.com"] + }), + FilterFor: "svc1,svc2", }, ["example.com", "svc1.com", "svc2.com"]); aliasTest("two services with a common alias", { - "svc1": ["svc1.com", "example.com"], - "svc2": ["svc2.com", "example.com"] + Aliases: JSON.stringify({ + "svc1": ["svc1.com", "example.com"], + "svc2": ["svc2.com", "example.com"] + }), + FilterFor: "svc1,svc2", }, ["example.com", "svc1.com", "svc2.com"]); -}); \ No newline at end of file + + aliasTest("three services with a common alias one service filtered out", { + Aliases: JSON.stringify({ + "svc1": ["svc1.com", "example.com"], + "svc2": ["svc2.com", "example.com", "example2.com"], + "svc3": ["svc3.com", "example.com", "example2.com"] + }), + FilterFor: "svc2,svc1", + }, ["example.com", "example2.com", "svc1.com", "svc2.com"]); + + aliasTest("bunch of services with single alias, some filtered out, out of order", { + Aliases: JSON.stringify({ + "lbws3": ["three.lbws.com"], + "backend1": ["one.backend.internal"], + "lbws1": ["one.lbws.com"], + "lbws4": ["four.lbws.com"], + "backend2": ["two.backend.internal"], + "lbws2": ["two.lbws.com"], + "lbws6": ["lbws.com"], + "lbws5": ["lbws.com"], + }), + FilterFor: "lbws2,lbws3,lbws4,lbws1,lbws5,lbws6", + }, ["four.lbws.com", "lbws.com", "one.lbws.com", "three.lbws.com", "two.lbws.com"]); +}); diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 7c41955dd78..9c34706be7d 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -222,9 +222,7 @@ func (d *envDeployer) validateALBWorkloadsDontRedirect() error { } // lbServiceRedirects returns true if svc's HTTP listener rule redirects. We only check -// HTTPListenerRuleWithDomain because HTTPListenerRule: -// a) doesn't ever redirect -// b) won't work with cloudfront anyways (can't point ALB default DNS to CF) +// HTTPListenerRuleWithDomain because HTTPListenerRule doesn't ever redirect. func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, error) { stackDescriber := d.newServiceStackDescriber(svc) resources, err := stackDescriber.Resources() diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index 9c1ea684c41..108160a8ebd 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -113,6 +113,7 @@ func newPackageEnvOpts(vars packageEnvVars) (*packageEnvOpts, error) { App: appCfg, Env: envCfg, SessionProvider: sessProvider, + ConfigStore: opts.cfgStore, }) } return opts, nil diff --git a/internal/pkg/template/templates/environment/partials/cdn-resources.yml b/internal/pkg/template/templates/environment/partials/cdn-resources.yml index 33c3ae4e517..0bc477525fe 100644 --- a/internal/pkg/template/templates/environment/partials/cdn-resources.yml +++ b/internal/pkg/template/templates/environment/partials/cdn-resources.yml @@ -44,6 +44,7 @@ UniqueAliasesAction: Properties: ServiceToken: !GetAtt UniqueJSONValuesFunction.Arn Aliases: !Ref Aliases + FilterFor: !Ref ALBWorkloads {{- end}}{{/* endif or .CDNConfig.ImportedCertificate .DelegateDNS */}} CloudFrontDistribution: From 75a79d86dcc43d613d57a7737aeb8321dbaba640 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 15:39:52 -0700 Subject: [PATCH 07/12] add extra test --- cf-custom-resources/test/unique-json-values-test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cf-custom-resources/test/unique-json-values-test.js b/cf-custom-resources/test/unique-json-values-test.js index 7499ecf7a89..b3a53e31b9d 100644 --- a/cf-custom-resources/test/unique-json-values-test.js +++ b/cf-custom-resources/test/unique-json-values-test.js @@ -180,4 +180,14 @@ describe("Unique Aliases", () => { }), FilterFor: "lbws2,lbws3,lbws4,lbws1,lbws5,lbws6", }, ["four.lbws.com", "lbws.com", "one.lbws.com", "three.lbws.com", "two.lbws.com"]); + + aliasTest("bunch of services with single alias, some FilterFor services don't exist", { + Aliases: JSON.stringify({ + "lbws1": ["lbws1.com"], + "lbws2": ["lbws2.com"], + "lbws3": ["lbws3.com"], + "lbws4": ["lbws4.com"], + }), + FilterFor: "lbws1,lbws5,lbws6,lbws3", + }, ["lbws1.com", "lbws3.com"]); }); From c3a7153b9b27e4a2e4348d7d4d59d4ba18e9ac4b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 15:49:52 -0700 Subject: [PATCH 08/12] fix test, add back `hasImportedCerts` --- internal/pkg/cli/deploy/svc.go | 9 +++++---- .../template-with-imported-certs-observability.yml | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/pkg/cli/deploy/svc.go b/internal/pkg/cli/deploy/svc.go index 316c7e64ae9..7473ab49ba3 100644 --- a/internal/pkg/cli/deploy/svc.go +++ b/internal/pkg/cli/deploy/svc.go @@ -1362,11 +1362,12 @@ func (d *backendSvcDeployer) validateALBRuntime() error { func (d *lbWebSvcDeployer) validateALBRuntime() error { hasALBCerts := len(d.envConfig.HTTPConfig.Public.Certificates) != 0 hasCDNCerts := d.envConfig.CDNConfig.Config.Certificate != nil - if d.lbMft.RoutingRule.RedirectToHTTPS != nil && d.app.Domain == "" && !hasALBCerts && !hasCDNCerts { + hasImportedCerts := hasALBCerts || hasCDNCerts + if d.lbMft.RoutingRule.RedirectToHTTPS != nil && d.app.Domain == "" && !hasImportedCerts { return fmt.Errorf("cannot configure http to https redirect without having a domain associated with the app %q or importing any certificates in env %q", d.app.Name, d.env.Name) } if d.lbMft.RoutingRule.Alias.IsEmpty() { - if hasALBCerts || hasCDNCerts { + if hasImportedCerts { return &errSvcWithNoALBAliasDeployingToEnvWithImportedCerts{ name: d.name, envName: d.env.Name, @@ -1376,7 +1377,7 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error { } importedHostedZones := d.lbMft.RoutingRule.Alias.HostedZones() if len(importedHostedZones) != 0 { - if !hasALBCerts && !hasCDNCerts { + if !hasImportedCerts { return fmt.Errorf("cannot specify alias hosted zones %v when no certificates are imported in environment %q", importedHostedZones, d.env.Name) } if d.envConfig.CDNEnabled() { @@ -1385,7 +1386,7 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error { } } } - if hasALBCerts || hasCDNCerts { + if hasImportedCerts { aliases, err := d.lbMft.RoutingRule.Alias.ToStringSlice() if err != nil { return fmt.Errorf("convert aliases to string slice: %w", err) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 665ea595512..3541cd4ed09 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -310,6 +310,7 @@ Resources: Properties: ServiceToken: !GetAtt UniqueJSONValuesFunction.Arn Aliases: !Ref Aliases + FilterFor: !Ref ALBWorkloads CloudFrontDistribution: Metadata: 'aws:copilot:description': 'A CloudFront distribution for global content delivery' From 8289f5e46a0a593224d846a5d50bf3e65da5f76d Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 17:02:00 -0700 Subject: [PATCH 09/12] generate escaped params on svc/env package --- .../cloudformation/stack/lb_web_svc_test.go | 103 ++++++++---------- .../cloudformation/stack/rd_web_svc_test.go | 89 ++++++--------- .../stack/scheduled_job_test.go | 96 +++++++--------- .../deploy/cloudformation/stack/workload.go | 35 +++--- .../templates/workloads/params.json.tmpl | 8 -- 5 files changed, 143 insertions(+), 188 deletions(-) delete mode 100644 internal/pkg/template/templates/workloads/params.json.tmpl diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go index cabce3e5f45..066207281dc 100644 --- a/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go +++ b/internal/pkg/deploy/cloudformation/stack/lb_web_svc_test.go @@ -905,7 +905,7 @@ func TestLoadBalancedWebService_Parameters(t *testing.T) { } func TestLoadBalancedWebService_SerializedParameters(t *testing.T) { - var testLBWebServiceManifest = manifest.NewLoadBalancedWebService(&manifest.LoadBalancedWebServiceProps{ + testLBWebServiceManifest := manifest.NewLoadBalancedWebService(&manifest.LoadBalancedWebServiceProps{ WorkloadProps: &manifest.WorkloadProps{ Name: "frontend", Dockerfile: "frontend/Dockerfile", @@ -913,66 +913,57 @@ func TestLoadBalancedWebService_SerializedParameters(t *testing.T) { Path: "frontend", Port: 80, }) - testCases := map[string]struct { - mockDependencies func(ctrl *gomock.Controller, c *LoadBalancedWebService) - wantedParams string - wantedError error - }{ - "unavailable template": { - mockDependencies: func(ctrl *gomock.Controller, c *LoadBalancedWebService) { - m := mocks.NewMockloadBalancedWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) - c.wkld.parser = m - }, - wantedParams: "", - wantedError: errors.New("some error"), - }, - "render params template": { - mockDependencies: func(ctrl *gomock.Controller, c *LoadBalancedWebService) { - m := mocks.NewMockloadBalancedWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(&template.Content{Buffer: bytes.NewBufferString("params")}, nil) - c.wkld.parser = m + c := &LoadBalancedWebService{ + ecsWkld: &ecsWkld{ + wkld: &wkld{ + name: "frontend", + env: testEnvName, + app: testAppName, + rc: RuntimeConfig{ + Image: &ECRImage{ + RepoURL: testImageRepoURL, + ImageTag: testImageTag, + }, + AdditionalTags: map[string]string{ + "owner": "boss", + }, + }, }, - wantedParams: "params", + tc: testLBWebServiceManifest.TaskConfig, }, + manifest: testLBWebServiceManifest, } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - // GIVEN - ctrl := gomock.NewController(t) - defer ctrl.Finish() - c := &LoadBalancedWebService{ - ecsWkld: &ecsWkld{ - wkld: &wkld{ - name: aws.StringValue(testLBWebServiceManifest.Name), - env: testEnvName, - app: testAppName, - rc: RuntimeConfig{ - Image: &ECRImage{ - RepoURL: testImageRepoURL, - ImageTag: testImageTag, - }, - AdditionalTags: map[string]string{ - "owner": "boss", - }, - }, - }, - tc: testLBWebServiceManifest.TaskConfig, - }, - manifest: testLBWebServiceManifest, - } - tc.mockDependencies(ctrl, c) - - // WHEN - params, err := c.SerializedParameters() - - // THEN - require.Equal(t, tc.wantedError, err) - require.Equal(t, tc.wantedParams, params) - }) - } + params, err := c.SerializedParameters() + require.NoError(t, err) + require.Equal(t, params, `{ + "Parameters": { + "AddonsTemplateURL": "", + "AppName": "phonetool", + "ContainerImage": "111111111111.dkr.ecr.us-west-2.amazonaws.com/phonetool/frontend:manual-bf3678c", + "ContainerPort": "80", + "DNSDelegated": "false", + "EnvFileARN": "", + "EnvName": "test", + "HTTPSEnabled": "false", + "LogRetention": "30", + "RulePath": "frontend", + "Stickiness": "false", + "TargetContainer": "frontend", + "TargetPort": "80", + "TaskCPU": "256", + "TaskCount": "1", + "TaskMemory": "512", + "WorkloadName": "frontend" + }, + "Tags": { + "copilot-application": "phonetool", + "copilot-environment": "test", + "copilot-service": "frontend", + "owner": "boss" + } +}`) } func TestLoadBalancedWebService_Tags(t *testing.T) { diff --git a/internal/pkg/deploy/cloudformation/stack/rd_web_svc_test.go b/internal/pkg/deploy/cloudformation/stack/rd_web_svc_test.go index edd7b45b65d..ae77d1f53ce 100644 --- a/internal/pkg/deploy/cloudformation/stack/rd_web_svc_test.go +++ b/internal/pkg/deploy/cloudformation/stack/rd_web_svc_test.go @@ -444,62 +444,43 @@ func TestRequestDrivenWebService_Parameters(t *testing.T) { } func TestRequestDrivenWebService_SerializedParameters(t *testing.T) { - testCases := map[string]struct { - mockDependencies func(ctrl *gomock.Controller, c *RequestDrivenWebService) - - wantedParams string - wantedError error - }{ - "unavailable template": { - mockDependencies: func(ctrl *gomock.Controller, c *RequestDrivenWebService) { - m := mocks.NewMockrequestDrivenWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(nil, errors.New("serialization error")) - c.wkld.parser = m - }, - wantedParams: "", - wantedError: errors.New("serialization error"), - }, - "render params template": { - mockDependencies: func(ctrl *gomock.Controller, c *RequestDrivenWebService) { - m := mocks.NewMockrequestDrivenWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(&template.Content{Buffer: bytes.NewBufferString("params")}, nil) - c.wkld.parser = m + c := &RequestDrivenWebService{ + appRunnerWkld: &appRunnerWkld{ + wkld: &wkld{ + name: aws.StringValue(testRDWebServiceManifest.Name), + env: testEnvName, + app: testAppName, + rc: RuntimeConfig{ + Image: &ECRImage{ + RepoURL: testImageRepoURL, + ImageTag: testImageTag, + }, + }, }, - wantedParams: "params", + instanceConfig: testRDWebServiceManifest.InstanceConfig, + imageConfig: testRDWebServiceManifest.ImageConfig, }, + manifest: testRDWebServiceManifest, } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - // GIVEN - ctrl := gomock.NewController(t) - defer ctrl.Finish() - c := &RequestDrivenWebService{ - appRunnerWkld: &appRunnerWkld{ - wkld: &wkld{ - name: aws.StringValue(testRDWebServiceManifest.Name), - env: testEnvName, - app: testAppName, - rc: RuntimeConfig{ - Image: &ECRImage{ - RepoURL: testImageRepoURL, - ImageTag: testImageTag, - }, - }, - }, - instanceConfig: testRDWebServiceManifest.InstanceConfig, - imageConfig: testRDWebServiceManifest.ImageConfig, - }, - manifest: testRDWebServiceManifest, - } - tc.mockDependencies(ctrl, c) - - // WHEN - params, err := c.SerializedParameters() - - // THEN - require.Equal(t, tc.wantedError, err) - require.Equal(t, tc.wantedParams, params) - }) - } + params, err := c.SerializedParameters() + require.NoError(t, err) + require.Equal(t, params, `{ + "Parameters": { + "AddonsTemplateURL": "", + "AppName": "phonetool", + "ContainerImage": "111111111111.dkr.ecr.us-west-2.amazonaws.com/phonetool/frontend:manual-bf3678c", + "ContainerPort": "80", + "EnvName": "test", + "ImageRepositoryType": "ECR", + "InstanceCPU": "256", + "InstanceMemory": "512", + "WorkloadName": "frontend" + }, + "Tags": { + "copilot-application": "phonetool", + "copilot-environment": "test", + "copilot-service": "frontend" + } +}`) } diff --git a/internal/pkg/deploy/cloudformation/stack/scheduled_job_test.go b/internal/pkg/deploy/cloudformation/stack/scheduled_job_test.go index b917c0fc465..a4295bc4c5d 100644 --- a/internal/pkg/deploy/cloudformation/stack/scheduled_job_test.go +++ b/internal/pkg/deploy/cloudformation/stack/scheduled_job_test.go @@ -554,64 +554,48 @@ func TestScheduledJob_SerializedParameters(t *testing.T) { Timeout: "1h30m", Retries: 3, }) - testCases := map[string]struct { - mockDependencies func(ctrl *gomock.Controller, c *ScheduledJob) - wantedParams string - wantedError error - }{ - "unavailable template": { - mockDependencies: func(ctrl *gomock.Controller, c *ScheduledJob) { - m := mocks.NewMockloadBalancedWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) - c.wkld.parser = m - }, - wantedParams: "", - wantedError: errors.New("some error"), - }, - "render params template": { - mockDependencies: func(ctrl *gomock.Controller, c *ScheduledJob) { - m := mocks.NewMockloadBalancedWebSvcReadParser(ctrl) - m.EXPECT().Parse(wkldParamsTemplatePath, gomock.Any(), gomock.Any()).Return(&template.Content{Buffer: bytes.NewBufferString("params")}, nil) - c.wkld.parser = m - }, - wantedParams: "params", - }, - } - - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - // GIVEN - ctrl := gomock.NewController(t) - defer ctrl.Finish() - c := &ScheduledJob{ - ecsWkld: &ecsWkld{ - wkld: &wkld{ - name: aws.StringValue(testScheduledJobManifest.Name), - env: testEnvName, - app: testAppName, - rc: RuntimeConfig{ - Image: &ECRImage{ - RepoURL: testImageRepoURL, - ImageTag: testImageTag, - }, - AdditionalTags: map[string]string{ - "owner": "boss", - }, - }, + c := &ScheduledJob{ + ecsWkld: &ecsWkld{ + wkld: &wkld{ + name: aws.StringValue(testScheduledJobManifest.Name), + env: testEnvName, + app: testAppName, + rc: RuntimeConfig{ + Image: &ECRImage{ + RepoURL: testImageRepoURL, + ImageTag: testImageTag, + }, + AdditionalTags: map[string]string{ + "owner": "boss", }, - tc: testScheduledJobManifest.TaskConfig, }, - manifest: testScheduledJobManifest, - } - tc.mockDependencies(ctrl, c) - - // WHEN - params, err := c.SerializedParameters() - - // THEN - require.Equal(t, tc.wantedError, err) - require.Equal(t, tc.wantedParams, params) - }) + }, + tc: testScheduledJobManifest.TaskConfig, + }, + manifest: testScheduledJobManifest, } + params, err := c.SerializedParameters() + require.NoError(t, err) + require.Equal(t, params, `{ + "Parameters": { + "AddonsTemplateURL": "", + "AppName": "phonetool", + "ContainerImage": "111111111111.dkr.ecr.us-west-2.amazonaws.com/phonetool/frontend:manual-bf3678c", + "EnvFileARN": "", + "EnvName": "test", + "LogRetention": "30", + "Schedule": "cron(0 0 * * ? *)", + "TaskCPU": "256", + "TaskCount": "1", + "TaskMemory": "512", + "WorkloadName": "mailer" + }, + "Tags": { + "copilot-application": "phonetool", + "copilot-environment": "test", + "copilot-service": "mailer", + "owner": "boss" + } +}`) } diff --git a/internal/pkg/deploy/cloudformation/stack/workload.go b/internal/pkg/deploy/cloudformation/stack/workload.go index b8932e46de6..9a23aeb7a5c 100644 --- a/internal/pkg/deploy/cloudformation/stack/workload.go +++ b/internal/pkg/deploy/cloudformation/stack/workload.go @@ -4,6 +4,7 @@ package stack import ( + "encoding/json" "fmt" "strconv" @@ -17,11 +18,6 @@ import ( "github.com/aws/copilot-cli/internal/pkg/template/override" ) -// Template rendering configuration common across workloads. -const ( - wkldParamsTemplatePath = "workloads/params.json.tmpl" -) - // Parameter logical IDs common across workloads. const ( WorkloadAppNameParamKey = "AppName" @@ -186,19 +182,30 @@ func serializeTemplateConfig(parser template.Parser, stack templateConfigurer) ( if err != nil { return "", err } - doc, err := parser.Parse(wkldParamsTemplatePath, struct { - Parameters []*cloudformation.Parameter - Tags []*cloudformation.Tag + + tags := stack.Tags() + + config := struct { + Parameters map[string]*string `json:"Parameters"` + Tags map[string]*string `json:"Tags,omitempty"` }{ - Parameters: params, - Tags: stack.Tags(), - }, template.WithFuncs(map[string]interface{}{ - "inc": template.IncFunc, - })) + Parameters: make(map[string]*string, len(params)), + Tags: make(map[string]*string, len(tags)), + } + + for _, param := range params { + config.Parameters[aws.StringValue(param.ParameterKey)] = param.ParameterValue + } + for _, tag := range tags { + config.Tags[aws.StringValue(tag.Key)] = tag.Value + } + + str, err := json.MarshalIndent(config, "", " ") if err != nil { return "", err } - return doc.String(), nil + + return string(str), nil } func (w *wkld) addonsOutputs() (*template.WorkloadNestedStackOpts, error) { diff --git a/internal/pkg/template/templates/workloads/params.json.tmpl b/internal/pkg/template/templates/workloads/params.json.tmpl deleted file mode 100644 index 6125a5285ee..00000000000 --- a/internal/pkg/template/templates/workloads/params.json.tmpl +++ /dev/null @@ -1,8 +0,0 @@ -{ - "Parameters" : { {{$paramsSize := len .Parameters}}{{range $i, $p := .Parameters}} - {{if eq (inc $i) $paramsSize}}"{{$p.ParameterKey}}": "{{$p.ParameterValue}}"{{else}}"{{$p.ParameterKey}}": "{{$p.ParameterValue}}",{{end}}{{end}} - },{{$n := len .Tags}}{{if gt $n 0}} - "Tags": { {{range $i, $t := .Tags}} - {{if eq (inc $i) $n}}"{{$t.Key}}": "{{$t.Value}}"{{else}}"{{$t.Key}}": "{{$t.Value}}",{{end}}{{end}} - }{{end}} -} From efe29759b4e286f1582f7ed9ab845b4b0f344d3b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 5 Oct 2022 17:09:36 -0700 Subject: [PATCH 10/12] o ya --- internal/pkg/deploy/cloudformation/stack/workload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/deploy/cloudformation/stack/workload.go b/internal/pkg/deploy/cloudformation/stack/workload.go index 9a23aeb7a5c..03d3ed79220 100644 --- a/internal/pkg/deploy/cloudformation/stack/workload.go +++ b/internal/pkg/deploy/cloudformation/stack/workload.go @@ -202,7 +202,7 @@ func serializeTemplateConfig(parser template.Parser, stack templateConfigurer) ( str, err := json.MarshalIndent(config, "", " ") if err != nil { - return "", err + return "", fmt.Errorf("marshal stack parameters to JSON: %v", err) } return string(str), nil From 82b34c7e959d4c70833b5495e4f4a6f63a9a40e6 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 6 Oct 2022 09:36:38 -0700 Subject: [PATCH 11/12] alphabetize integ test params to match new serializer --- .../stack/testdata/stacklocal/cf.params.json | 28 ++++++++--------- .../testdata/workloads/job-test.params.json | 20 ++++++------- .../workloads/svc-grpc-test.params.json | 28 ++++++++--------- .../workloads/svc-nlb-dev.params.json | 30 +++++++++---------- .../workloads/svc-nlb-prod.params.json | 26 ++++++++-------- .../workloads/svc-nlb-test.params.json | 26 ++++++++-------- .../testdata/workloads/svc-prod.params.json | 28 ++++++++--------- .../workloads/svc-staging.params.json | 28 ++++++++--------- .../testdata/workloads/svc-test.params.json | 28 ++++++++--------- .../workloads/windows-svc-test.params.json | 28 ++++++++--------- .../workloads/worker-test.params.json | 18 +++++------ 11 files changed, 144 insertions(+), 144 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/cf.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/cf.params.json index 7e78d1d04b6..975bb99caee 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/cf.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/stacklocal/cf.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "my-svc", "ContainerImage": "mockImageURL:latest", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "1", - "LogRetention": "30", "ContainerPort": "5000", "DNSDelegated": "false", - "TargetContainer": "my-svc", - "TargetPort": "5000", "EnvFileARN": "", - "RulePath": "my-svc", + "EnvName": "test", "HTTPSEnabled": "true", - "Stickiness": "false" + "LogRetention": "30", + "RulePath": "my-svc", + "Stickiness": "false", + "TargetContainer": "my-svc", + "TargetPort": "5000", + "TaskCPU": "256", + "TaskCount": "1", + "TaskMemory": "512", + "WorkloadName": "my-svc" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "my-svc" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/job-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/job-test.params.json index b75d1e4c58f..7fe737d404c 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/job-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/job-test.params.json @@ -1,20 +1,20 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "job", "ContainerImage": "alpine", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "1", + "EnvFileARN": "", + "EnvName": "test", "LogRetention": "30", "Schedule": "cron(0 12 ? * MON *)", - "EnvFileARN": "" + "TaskCPU": "256", + "TaskCount": "1", + "TaskMemory": "512", + "WorkloadName": "job" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "job" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-grpc-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-grpc-test.params.json index d129f428da4..a3361798d96 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-grpc-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-grpc-test.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "gRPC", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "2", - "LogRetention": "30", "ContainerPort": "50051", "DNSDelegated": "false", - "TargetContainer": "gRPC", - "TargetPort": "50051", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "test", "HTTPSEnabled": "true", - "Stickiness": "false" + "LogRetention": "30", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "gRPC", + "TargetPort": "50051", + "TaskCPU": "256", + "TaskCount": "2", + "TaskMemory": "512", + "WorkloadName": "gRPC" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "gRPC" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-dev.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-dev.params.json index 73b29504270..20e585c6ed3 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-dev.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-dev.params.json @@ -1,28 +1,28 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "dev", - "WorkloadName": "fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "5", - "LogRetention": "30", "ContainerPort": "80", "DNSDelegated": "true", - "TargetContainer": "fe", - "TargetPort": "80", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "dev", "HTTPSEnabled": "true", - "Stickiness": "false", + "LogRetention": "30", "NLBAliases": "", - "NLBPort": "81" + "NLBPort": "81", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "fe", + "TargetPort": "80", + "TaskCPU": "256", + "TaskCount": "5", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "dev", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-prod.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-prod.params.json index 395d21503fd..85fead9b5e6 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-prod.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-prod.params.json @@ -1,25 +1,25 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "prod", - "WorkloadName": "fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "5", - "LogRetention": "30", "ContainerPort": "80", "DNSDelegated": "true", - "TargetContainer": "fe", - "TargetPort": "80", "EnvFileARN": "", + "EnvName": "prod", + "LogRetention": "30", "NLBAliases": "nlb.example.com", - "NLBPort": "443" + "NLBPort": "443", + "TargetContainer": "fe", + "TargetPort": "80", + "TaskCPU": "256", + "TaskCount": "5", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "prod", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-test.params.json index 4ae9e0702ae..59c43adb725 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-test.params.json @@ -1,25 +1,25 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "5", - "LogRetention": "30", "ContainerPort": "80", "DNSDelegated": "true", - "TargetContainer": "fe", - "TargetPort": "80", "EnvFileARN": "", + "EnvName": "test", + "LogRetention": "30", "NLBAliases": "", - "NLBPort": "443" + "NLBPort": "443", + "TargetContainer": "fe", + "TargetPort": "80", + "TaskCPU": "256", + "TaskCount": "5", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-prod.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-prod.params.json index 024d53b78ff..48bae789f8e 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-prod.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-prod.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "prod", - "WorkloadName": "fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "3", - "LogRetention": "1", "ContainerPort": "4000", "DNSDelegated": "false", - "TargetContainer": "fe", - "TargetPort": "4000", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "prod", "HTTPSEnabled": "true", - "Stickiness": "false" + "LogRetention": "1", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "fe", + "TargetPort": "4000", + "TaskCPU": "256", + "TaskCount": "3", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "prod", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-staging.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-staging.params.json index 3d70556b075..d702419ca17 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-staging.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-staging.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "staging", - "WorkloadName": "fe", "ContainerImage": "123456789000.dkr.ecr.us-east-1.amazonaws.com/vault/e2e:cicdtest", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "5", - "LogRetention": "30", "ContainerPort": "4000", "DNSDelegated": "false", - "TargetContainer": "fe", - "TargetPort": "4000", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "staging", "HTTPSEnabled": "true", - "Stickiness": "false" + "LogRetention": "30", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "fe", + "TargetPort": "4000", + "TaskCPU": "256", + "TaskCount": "5", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "staging", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-test.params.json index 84798649319..4d6e8ec6530 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-test.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "256", - "TaskMemory": "512", - "TaskCount": "2", - "LogRetention": "30", "ContainerPort": "4000", "DNSDelegated": "false", - "TargetContainer": "fe", - "TargetPort": "4000", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "test", "HTTPSEnabled": "true", - "Stickiness": "false" + "LogRetention": "30", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "fe", + "TargetPort": "4000", + "TaskCPU": "256", + "TaskCount": "2", + "TaskMemory": "512", + "WorkloadName": "fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/windows-svc-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/windows-svc-test.params.json index d5deb4a71f9..99edaf0ed1a 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/windows-svc-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/windows-svc-test.params.json @@ -1,26 +1,26 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "windows-fe", "ContainerImage": "", - "AddonsTemplateURL": "", - "TaskCPU": "1024", - "TaskMemory": "2048", - "TaskCount": "1", - "LogRetention": "30", "ContainerPort": "80", "DNSDelegated": "false", - "TargetContainer": "windows-fe", - "TargetPort": "80", "EnvFileARN": "", - "RulePath": "/", + "EnvName": "test", "HTTPSEnabled": "false", - "Stickiness": "false" + "LogRetention": "30", + "RulePath": "/", + "Stickiness": "false", + "TargetContainer": "windows-fe", + "TargetPort": "80", + "TaskCPU": "1024", + "TaskCount": "1", + "TaskMemory": "2048", + "WorkloadName": "windows-fe" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "windows-fe" } -} +} \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.params.json b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.params.json index 0ed82e13df7..752c4dd48eb 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.params.json +++ b/internal/pkg/deploy/cloudformation/stack/testdata/workloads/worker-test.params.json @@ -1,19 +1,19 @@ { - "Parameters" : { + "Parameters": { + "AddonsTemplateURL": "", "AppName": "my-app", - "EnvName": "test", - "WorkloadName": "dogworker", "ContainerImage": "amazon/ecs-example", - "AddonsTemplateURL": "", + "EnvFileARN": "", + "EnvName": "test", + "LogRetention": "30", "TaskCPU": "256", - "TaskMemory": "512", "TaskCount": "1", - "LogRetention": "30", - "EnvFileARN": "" + "TaskMemory": "512", + "WorkloadName": "dogworker" }, - "Tags": { + "Tags": { "copilot-application": "my-app", "copilot-environment": "test", "copilot-service": "dogworker" } -} +} \ No newline at end of file From 91fb5abc8a88168a38e1a56d1a93a602f49bc81a Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 6 Oct 2022 11:17:16 -0700 Subject: [PATCH 12/12] add quotes around manifest fields --- internal/pkg/manifest/validate_env.go | 2 +- internal/pkg/manifest/validate_env_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 885c942433d..465d258dbfd 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -50,7 +50,7 @@ func (e EnvironmentConfig) validate() error { cdnCert := e.CDNConfig.Config.Certificate if e.HTTPConfig.Public.Certificates == nil { if cdnCert != nil && !aws.BoolValue(e.CDNConfig.Config.TerminateTLS) { - return errors.New("cdn.terminate_tls must be true if cdn.certificate is set without http.public.certificates") + return errors.New(`"cdn.terminate_tls" must be true if "cdn.certificate" is set without "http.public.certificates"`) } } else { if cdnCert == nil { diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 85930e0ce4b..8f24b8e67a7 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -222,7 +222,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - wantedError: "cdn.terminate_tls must be true if cdn.certificate is set without http.public.certificates", + wantedError: `"cdn.terminate_tls" must be true if "cdn.certificate" is set without "http.public.certificates"`, }, "success if cdn cert specified, cdn terminating tls, and no public certs": { in: EnvironmentConfig{