From 41f919c8d5699944c8c5252793cb80e324e06a27 Mon Sep 17 00:00:00 2001 From: Efe Karakus Date: Tue, 24 Jan 2023 15:00:51 -0800 Subject: [PATCH 1/2] fix(custom-domain-action): ignore A-record deletion if not found Fixes #4401 If the record fails to insert, then the custom resource fails. On failure, it tries to delete the record that was *not* inserted. Hence the Delete workflow fails resulting in the custom resource to fail to delete. This change ensures that we ignore the error if the A-record that we're trying to delete doesn't exist in the first place. --- cf-custom-resources/lib/custom-domain.js | 41 ++++++++++++------ .../test/custom-domain-test.js | 42 +++++++++++++++++++ 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/cf-custom-resources/lib/custom-domain.js b/cf-custom-resources/lib/custom-domain.js index 5d456f64a41..237898e48b7 100644 --- a/cf-custom-resources/lib/custom-domain.js +++ b/cf-custom-resources/lib/custom-domain.js @@ -4,6 +4,11 @@ const aws = require("aws-sdk"); +const changeRecordAction = { + Upsert: "UPSERT", + Delete: "DELETE", +} + // These are used for test purposes only let defaultResponseURL; let defaultLogGroup; @@ -158,17 +163,27 @@ const writeARecord = async function ( hostedZoneCache.set(domain, hostedZoneId); } console.log(`${action} A record into Hosted Zone ${hostedZoneId}`); - const changeBatch = await updateRecords( - route53, - hostedZoneId, - action, - alias, - accessDNS, - accessHostedZone - ); - await waitForRecordChange(route53, changeBatch.ChangeInfo.Id); + try { + const changeBatch = await updateRecords( + route53, + hostedZoneId, + action, + alias, + accessDNS, + accessHostedZone + ); + await waitForRecordChange(route53, changeBatch.ChangeInfo.Id); + } catch (err) { + if (action === changeRecordAction.Delete && isRecordSetNotFoundErr(err)) { + console.log(`${err.message}; Copilot is ignoring this record.`); + return; + } + throw err; + } }; +const isRecordSetNotFoundErr = (err) => err.message.includes("Tried to delete resource record set") && err.message.includes("but it was not found") + /** * Custom domain handler, invoked by Lambda. */ @@ -214,7 +229,7 @@ exports.handler = async function (event, context) { props.PublicAccessDNS, props.PublicAccessHostedZone, aliasTypes, - "UPSERT" + changeRecordAction.Upsert, ); break; case "Update": @@ -225,7 +240,7 @@ exports.handler = async function (event, context) { props.PublicAccessDNS, props.PublicAccessHostedZone, aliasTypes, - "UPSERT" + changeRecordAction.Upsert, ); // After upserting new aliases, delete unused ones. For example: previously we have ["foo.com", "bar.com"], // and now the aliases param is updated to just ["foo.com"] then we'll delete "bar.com". @@ -242,7 +257,7 @@ exports.handler = async function (event, context) { props.PublicAccessDNS, props.PublicAccessHostedZone, aliasTypes, - "DELETE" + changeRecordAction.Delete, ); break; case "Delete": @@ -253,7 +268,7 @@ exports.handler = async function (event, context) { props.PublicAccessDNS, props.PublicAccessHostedZone, aliasTypes, - "DELETE" + changeRecordAction.Delete ); break; default: diff --git a/cf-custom-resources/test/custom-domain-test.js b/cf-custom-resources/test/custom-domain-test.js index a586c4ef988..341e40a57eb 100644 --- a/cf-custom-resources/test/custom-domain-test.js +++ b/cf-custom-resources/test/custom-domain-test.js @@ -406,4 +406,46 @@ describe("DNS Validated Certificate Handler", () => { expect(request.isDone()).toBe(true); }); }); + + test("Ignore error if trying to delete an A-record that does not exist", () => { + const changeResourceRecordSetsFake = sinon.fake.rejects(new Error("InvalidChangeBatch: [Tried to delete resource record set [name='v1.foobar.com.', type='A'] but it was not found]")); + + const listHostedZonesByNameFake = sinon.fake.resolves({ + HostedZones: [ + { + Id: `/hostedzone/${testHostedZoneId}`, + }, + ], + }); + + AWS.mock( + "Route53", + "changeResourceRecordSets", + changeResourceRecordSetsFake + ); + AWS.mock("Route53", "listHostedZonesByName", listHostedZonesByNameFake); + + const request = nock(ResponseURL) + .put("/", (body) => { + return body.Status === "SUCCESS"; + }) + .reply(200); + return LambdaTester(handler.handler) + .event({ + RequestType: "Delete", + ResourceProperties: { + AppName: testAppName, + EnvName: testEnvName, + DomainName: testDomainName, + Aliases: testAliases, + Region: "us-east-1", + PublicAccessDNS: testAccessDNS, + PublicAccessHostedZone: testLBHostedZone, + AppDNSRole: testRootDNSRole, + }, + }) + .expectResolve(() => { + expect(request.isDone()).toBe(true); + }); + }); }); From 7c9bbc205b3ca5035a84733845f7c791f12a4f80 Mon Sep 17 00:00:00 2001 From: Efe Karakus Date: Tue, 24 Jan 2023 20:37:25 -0800 Subject: [PATCH 2/2] chore: add sample error message in comments --- cf-custom-resources/lib/custom-domain.js | 1 + 1 file changed, 1 insertion(+) diff --git a/cf-custom-resources/lib/custom-domain.js b/cf-custom-resources/lib/custom-domain.js index 237898e48b7..539cbd07b9e 100644 --- a/cf-custom-resources/lib/custom-domain.js +++ b/cf-custom-resources/lib/custom-domain.js @@ -182,6 +182,7 @@ const writeARecord = async function ( } }; +// Example error message: "InvalidChangeBatch: [Tried to delete resource record set [name='a.domain.com.', type='A'] but it was not found]" const isRecordSetNotFoundErr = (err) => err.message.includes("Tried to delete resource record set") && err.message.includes("but it was not found") /**