Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(route53): CrossAccountZoneDelegationRecord does not remove old NS… #21599

Closed
Closed
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
Original file line number Diff line number Diff line change
@@ -15,7 +15,24 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent

switch (event.RequestType) {
case 'Create':
return cfnEventHandler(resourceProps, false);
case 'Update':
// We handle updates in two parts, first attempt to delete the old NS record, second create
// the new resource.

// DELETE old Resources
try {
const oldResourceProps = event.OldResourceProperties as unknown as ResourceProperties;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only process the delete if the hosted zone or record name changes? I'm a little worried about deleting records when it is not absolutely necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only delete the old NS record when the DelegatedZoneName of the OldResourceProperties is different from the one from the (new) ResourceProperties. Otherwise we'll be removing and recreating the same record after every 'Update' event which is not necessary and might also produce outages.


await cfnEventHandler(oldResourceProps, true);
} catch (e) {
// it is possible that the user removed the old role manually before this call,
// additionally, it is possible that the old record set was manually deleted. We
// want to ignore if this errors and continue execution and attempt to create the
// new resource, and not block the user.
}

// UPSERT new Resources
return cfnEventHandler(resourceProps, false);
case 'Delete':
return cfnEventHandler(resourceProps, true);
Original file line number Diff line number Diff line change
@@ -147,6 +147,118 @@ test('calls listHostedZonesByName to get zoneId if ParentZoneId is not provided'
});
});

test('calls change resource record set with Delete and Upsert for Update event', async () => {
// GIVEN
mockStsClient.promise.mockResolvedValue({
Credentials: { AccessKeyId: 'K', SecretAccessKey: 'S', SessionToken: 'T' },
});
mockRoute53Client.promise.mockResolvedValueOnce({});

// WHEN
const event = getCfnEvent({
RequestType: 'Update',
OldResourceProperties: {
ServiceToken: 'Foo',
AssumeRoleArn: 'roleArn2',
ParentZoneId: '1',
DelegatedZoneName: 'recordName',
DelegatedZoneNameServers: ['oldone', 'oldtwo'],
TTL: 172800,
},
});
await invokeHandler(event);

// THEN
// The old and new records can be called with a different roleArn
expect(mockStsClient.assumeRole).toHaveBeenCalledTimes(2);

expect(mockRoute53Client.changeResourceRecordSets).toHaveBeenCalledTimes(2);

// check first call to delete old NS record
expect(mockRoute53Client.changeResourceRecordSets).toHaveBeenNthCalledWith(1, {
HostedZoneId: '1',
ChangeBatch: {
Changes: [
{
Action: 'DELETE',
ResourceRecordSet: {
Name: 'recordName',
Type: 'NS',
TTL: 172800,
ResourceRecords: [{ Value: 'oldone' }, { Value: 'oldtwo' }],
},
},
],
},
});

// check second call to create new NS record
expect(mockRoute53Client.changeResourceRecordSets).toHaveBeenNthCalledWith(2, {
HostedZoneId: '1',
ChangeBatch: {
Changes: [
{
Action: 'UPSERT',
ResourceRecordSet: {
Name: 'recordName',
Type: 'NS',
TTL: 172800,
ResourceRecords: [{ Value: 'one' }, { Value: 'two' }],
},
},
],
},
});
});

test('calls change resource record set with DELETE for Update event with bad old roleArn', async () => {
// GIVEN
mockStsClient.promise.mockResolvedValueOnce({
Credentials: undefined,
});
mockStsClient.promise.mockResolvedValue({
Credentials: { AccessKeyId: 'K', SecretAccessKey: 'S', SessionToken: 'T' },
});
mockRoute53Client.promise.mockResolvedValueOnce({});

// WHEN
const event = getCfnEvent({
RequestType: 'Update',
OldResourceProperties: {
ServiceToken: 'Foo',
AssumeRoleArn: 'badRoleArn',
ParentZoneId: '1',
DelegatedZoneName: 'recordName',
DelegatedZoneNameServers: ['oldone', 'oldtwo'],
TTL: 172800,
},
});
await invokeHandler(event);

// THEN
// The old and new records can be called with a different roleArn
expect(mockStsClient.assumeRole).toHaveBeenCalledTimes(2);

expect(mockRoute53Client.changeResourceRecordSets).toHaveBeenCalledTimes(1);

// check for second call when we UPSET the new NS record
expect(mockRoute53Client.changeResourceRecordSets).toHaveBeenCalledWith({
HostedZoneId: '1',
ChangeBatch: {
Changes: [{
Action: 'UPSERT',
ResourceRecordSet: {
Name: 'recordName',
Type: 'NS',
TTL: 172800,
ResourceRecords: [{ Value: 'one' }, { Value: 'two' }],
},
}],
},
});
});


test('throws if more than one HostedZones are returnd for the provided ParentHostedZone', async () => {
// GIVEN
const parentZoneName = 'some.zone';