From a2409032a2a63106b8c95bc729e6ba427daf3200 Mon Sep 17 00:00:00 2001 From: Hogan Bobertz Date: Thu, 1 Feb 2024 12:34:37 -0500 Subject: [PATCH] chore: fix cdk migrate not deleting templates and lack of deduplication (#228) * fix bugs * only create cfnTemplateGenerator if we need it * fix integ tests --------- Co-authored-by: Hogan Bobertz --- .../tests/cli-integ-tests/cli.integtest.ts | 14 +++-- packages/aws-cdk/lib/cdk-toolkit.ts | 18 +++++- packages/aws-cdk/lib/commands/migrate.ts | 55 +++++++++++++------ 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts index b29e12b69a60e..ac3281cc07dba 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts @@ -600,20 +600,19 @@ integTest('cdk migrate generates migrate.json', withCDKMigrateFixture('typescrip })); integTest('cdk migrate --from-scan with AND/OR filters correctly filters resources', withDefaultFixture(async (fixture) => { - const migrateStackJr = 'migrate-stack-jr'; + const stackName = `cdk-migrate-integ-${fixture.randomString}`; await fixture.cdkDeploy('migrate-stack', { modEnv: { SAMPLE_RESOURCES: '1' }, }); await fixture.cdk( - ['migrate', '--stack-name', migrateStackJr, '--from-scan', 'new', '--filter', 'type=AWS::SNS::Topic,tag-key=tag1', 'type=AWS::SQS::Queue,tag-key=tag3'], + ['migrate', '--stack-name', stackName, '--from-scan', 'new', '--filter', 'type=AWS::SNS::Topic,tag-key=tag1', 'type=AWS::SQS::Queue,tag-key=tag3'], { modEnv: { MIGRATE_INTEG_TEST: '1' }, neverRequireApproval: true, verbose: true, captureStderr: false }, ); try { - const response = await fixture.aws.cloudFormation('describeGeneratedTemplate', { - GeneratedTemplateName: migrateStackJr, + GeneratedTemplateName: stackName, }); const resourceNames = []; for (const resource of response.Resources || []) { @@ -624,9 +623,11 @@ integTest('cdk migrate --from-scan with AND/OR filters correctly filters resourc fixture.log(`Resources: ${resourceNames}`); expect(resourceNames.some(ele => ele && ele.includes('migratetopic1'))).toBeTruthy(); expect(resourceNames.some(ele => ele && ele.includes('migratequeue1'))).toBeTruthy(); - expect(response.Resources?.length).toEqual(2); } finally { await fixture.cdkDestroy('migrate-stack'); + await fixture.aws.cloudFormation('deleteGeneratedTemplate', { + GeneratedTemplateName: stackName, + }); } })); @@ -663,6 +664,9 @@ integTest('cdk migrate --from-scan for resources with Write Only Properties gene } } finally { await fixture.cdkDestroy('migrate-stack'); + await fixture.aws.cloudFormation('deleteGeneratedTemplate', { + GeneratedTemplateName: stackName, + }); } })); diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 7ab7c41020948..6e236e742bbb4 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -735,10 +735,12 @@ export class CdkToolkit { public async migrate(options: MigrateOptions): Promise { warning('This is an experimental feature and development on it is still in progress. We make no guarantees about the outcome or stability of the functionality.'); const language = options.language?.toLowerCase() ?? 'typescript'; + const environment = setEnvironment(options.account, options.region); + let generateTemplateOutput: GenerateTemplateOutput | undefined; + let cfn: CfnTemplateGeneratorProvider | undefined; + let templateToDelete: string | undefined; try { - const environment = setEnvironment(options.account, options.region); - let generateTemplateOutput: GenerateTemplateOutput | undefined; // if neither fromPath nor fromStack is provided, generate a template using cloudformation const scanType = parseSourceOptions(options.fromPath, options.fromStack, options.stackName).source; if (scanType == TemplateSourceOptions.SCAN) { @@ -749,6 +751,7 @@ export class CdkToolkit { sdkProvider: this.props.sdkProvider, environment: environment, }); + templateToDelete = generateTemplateOutput.templateId; } else if (scanType == TemplateSourceOptions.PATH) { const templateBody = readFromPath(options.fromPath!); @@ -757,7 +760,7 @@ export class CdkToolkit { if (templateId) { // if we have a template id, we can call describe generated template to get the resource identifiers // resource metadata, and template source to generate the template - const cfn = new CfnTemplateGeneratorProvider(await buildCfnClient(this.props.sdkProvider, environment)); + cfn = new CfnTemplateGeneratorProvider(await buildCfnClient(this.props.sdkProvider, environment)); const generatedTemplateSummary = await cfn.describeGeneratedTemplate(templateId); generateTemplateOutput = buildGenertedTemplateOutput(generatedTemplateSummary, templateBody, generatedTemplateSummary.GeneratedTemplateId!); } else { @@ -796,6 +799,15 @@ export class CdkToolkit { } catch (e) { error(' ❌ Migrate failed for `%s`: %s', options.stackName, (e as Error).message); throw e; + } finally { + if (templateToDelete) { + if (!cfn) { + cfn = new CfnTemplateGeneratorProvider(await buildCfnClient(this.props.sdkProvider, environment)); + } + if (!process.env.MIGRATE_INTEG_TEST) { + await cfn.deleteGeneratedTemplate(templateToDelete); + } + } } } diff --git a/packages/aws-cdk/lib/commands/migrate.ts b/packages/aws-cdk/lib/commands/migrate.ts index 04b44298781d6..fd34a6f6184f8 100644 --- a/packages/aws-cdk/lib/commands/migrate.ts +++ b/packages/aws-cdk/lib/commands/migrate.ts @@ -539,9 +539,11 @@ export function buildGenertedTemplateOutput(generatedTemplateSummary: CloudForma ResourceIdentifier: r.ResourceIdentifier!, })), }; + const templateId = generatedTemplateSummary.GeneratedTemplateId!; return { migrateJson: migrateJson, resources: resources, + templateId: templateId, }; } @@ -590,6 +592,27 @@ export function appendWarningsToReadme(filepath: string, resources: CloudFormati fs.writeFileSync(filepath, lines.join('\n')); } +/** + * takes a list of resources and returns a list of unique resources based on the resource type and logical resource id. + * + * @param resources A list of resources to deduplicate + * @returns A list of unique resources + */ +function deduplicateResources(resources: CloudFormation.ResourceDetails) { + let uniqueResources: {[key: string]: CloudFormation.ResourceDetail} = {}; + + for (const resource of resources) { + const key = Object.keys(resource.ResourceIdentifier!)[0]; + + // Creating our unique identifier using the resource type, the key, and the value of the resource identifier + // The resource identifier is a combination of a key value pair defined by a resource's schema, and the resource type of the resource. + const uniqueIdentifer = `${resource.ResourceType}:${key}:${resource.ResourceIdentifier![key]}`; + uniqueResources[uniqueIdentifer] = resource; + } + + return Object.values(uniqueResources); +}; + /** * Class for making CloudFormation template generator calls */ @@ -647,23 +670,7 @@ export class CfnTemplateGeneratorProvider { } } - let uniqueResources: {[key: string]: CloudFormation.ScannedResource} = {}; - - // de-duplicating the list of related resources based on the resource identifier value - // The resource identifier key is not known at compile time so we need to get it using Object.keys. There will only - // ever be a single key so [0] retrives the first and only key - for (const resource of relatedResourceList) { - if (!resource.ResourceIdentifier) continue; - const key = Object.keys(resource.ResourceIdentifier!)[0]; - - // Creating our unique identifier using the resource type, the key, and the value of the resource identifier - // The resource identifier is a combination of a key value pair defined by a resource's schema, and the resource type of the resource. - const uniqueIdentifer = `${resource.ResourceType}:${key}:${resource.ResourceIdentifier![key]}`; - uniqueResources[uniqueIdentifer] = resource; - } - - // convert the dictionary back into a list - relatedResourceList = Object.values(uniqueResources); + relatedResourceList = deduplicateResources(relatedResourceList); // prune the managedbystack flag off of them again. return process.env.MIGRATE_INTEG_TEST ? resourceIdentifiers(relatedResourceList) : resourceIdentifiers(excludeManaged(relatedResourceList)) ; @@ -750,6 +757,7 @@ export class CfnTemplateGeneratorProvider { if (resourceList.length === 0) { throw new Error(`No resources found with filters ${filters.join(' ')}. Please try again with different filters.`); } + resourceList = deduplicateResources(resourceList); return process.env.MIGRATE_INTEG_TEST ? resourceIdentifiers(resourceList) : resourceIdentifiers(excludeManaged(resourceList)); } @@ -822,6 +830,18 @@ export class CfnTemplateGeneratorProvider { } return createTemplateOutput; } + + /** + * Deletes a generated template from the template generator. + * + * @param templateArn The arn of the template to delete + * @returns A promise that resolves when the template has been deleted + */ + async deleteGeneratedTemplate(templateArn: string): Promise { + await this.cfn.deleteGeneratedTemplate({ + GeneratedTemplateName: templateArn, + }).promise(); + } } /** @@ -870,6 +890,7 @@ export interface GenerateTemplateOptions { export interface GenerateTemplateOutput { migrateJson: MigrateJsonFormat; resources?: CloudFormation.ResourceDetails; + templateId?: string; } /**