Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions cf-custom-resources/lib/unique-json-values.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -96,17 +106,24 @@ 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":
// Do nothing on delete, since this isn't a "real" resource.
break;
default:
throw new Error(`Unsupported request type ${event.RequestType}`);
}
};
};


try {
await Promise.race([deadlineExpired(), handler()]);
await report(event, context, "SUCCESS", physicalResourceId, responseData);
Expand Down
92 changes: 76 additions & 16 deletions cf-custom-resources/test/unique-json-values-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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(() => {
Expand All @@ -106,28 +104,90 @@ 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"]);
});

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",
Comment thread
dannyrandall marked this conversation as resolved.
}, ["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"]);
});
9 changes: 4 additions & 5 deletions internal/pkg/cli/deploy/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Comment thread
dannyrandall marked this conversation as resolved.
err := d.validateALBWorkloadsDontRedirect()
var redirErr *errEnvHasPublicServicesWithRedirect
switch {
Expand Down Expand Up @@ -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()
Expand All @@ -240,7 +238,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)
Expand Down
47 changes: 38 additions & 9 deletions internal/pkg/cli/deploy/env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -429,14 +429,24 @@ 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
setUpMocks func(*envDeployerMocks, *gomock.Controller)
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",
},
Expand All @@ -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",
},
Expand All @@ -469,15 +479,15 @@ 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"))
},
expected: "enable TLS termination on CDN: get env params: some error",
},
"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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -551,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.
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 5 additions & 2 deletions internal/pkg/cli/deploy/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Comment thread
dannyrandall marked this conversation as resolved.
len(e.services),
english.PluralWord(len(e.services), "service redirects", "services redirect"),
)
}

// RecommendActions returns recommended actions to be taken after the error.
Expand All @@ -68,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"),
)
}
Expand Down
19 changes: 11 additions & 8 deletions internal/pkg/cli/deploy/svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -1360,7 +1360,9 @@ func (d *backendSvcDeployer) validateALBRuntime() error {
}

func (d *lbWebSvcDeployer) validateALBRuntime() error {
hasImportedCerts := len(d.envConfig.HTTPConfig.Public.Certificates) != 0
hasALBCerts := len(d.envConfig.HTTPConfig.Public.Certificates) != 0
hasCDNCerts := d.envConfig.CDNConfig.Config.Certificate != nil
Comment thread
dannyrandall marked this conversation as resolved.
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)
}
Expand Down Expand Up @@ -1390,14 +1392,15 @@ func (d *lbWebSvcDeployer) validateALBRuntime() error {
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 {
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 cdnCert != nil {
if err := cfCertValidator.ValidateCertAliases(aliases, []string{*cdnCert}); err != nil {
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)
}
}
Expand Down
Loading