From a70ff1ad332af780c052e3117b73df060deee7ae Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Mon, 6 Mar 2023 13:00:11 +0100 Subject: [PATCH] fix(cli): cannot `cdk import` resources with multiple identifiers (#24439) When CloudFormation tells us about identifiers for resources it can import, it returns a `string[]`. Our CLI used to interpret this as a set of identifiers that all must be present. Instead, the contract is actually: each `string` is a comma-separated list of identifiers that must be present together, but from all `strings` exactly one key combination should be supplied (and not multiple). So: * `['BucketName']` -> Supply BucketName (easy) * `['TableName', 'TableArn']` -> supply exactly one of TableName or TableArn (but not both) * `['HostedZoneId,Name']` -> supply BOTH HostedZoneId and Name. Because of our misinterpretations, both the cases of resources with multiple POSSIBLE identifiers as well as multiple REQUIRED identifiers would fail to import. Make the code correctly model the expect types: identifiers are a `string[][]`, where the outer array indicates `OR` and the inner array indicates `AND`. * For any of the combinations of properties we can lift from the template, prompt the user to confirm (typically 0 or 1, might be more). If the user rejected any of them, we don't do the resource at all. * If we couldn't lift any full key from the template, ask the user for the properties of each compound key, lifting parts of it from the template if possible. Fixes #20895. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk/lib/import.ts | 117 +++++++++++------ packages/aws-cdk/test/import.test.ts | 182 ++++++++++++++++++++++++--- 2 files changed, 246 insertions(+), 53 deletions(-) diff --git a/packages/aws-cdk/lib/import.ts b/packages/aws-cdk/lib/import.ts index 0a5795313fbc0..f3caf736373be 100644 --- a/packages/aws-cdk/lib/import.ts +++ b/packages/aws-cdk/lib/import.ts @@ -9,18 +9,18 @@ import { ResourceIdentifierProperties, ResourcesToImport } from './api/util/clou import { error, print, success, warning } from './logging'; /** - * Parameters that uniquely identify a physical resource of a given type + * Set of parameters that uniquely identify a physical resource of a given type * for the import operation, example: * * ``` * { - * "AWS::S3::Bucket": ["BucketName"], - * "AWS::IAM::Role": ["RoleName"], - * "AWS::EC2::VPC": ["VpcId"] + * "AWS::S3::Bucket": [["BucketName"]], + * "AWS::DynamoDB::GlobalTable": [["TableName"], ["TableArn"], ["TableStreamArn"]], + * "AWS::Route53::KeySigningKey": [["HostedZoneId", "Name"]], * } * ``` */ -export type ResourceIdentifiers = { [resourceType: string]: string[] }; +export type ResourceIdentifiers = { [resourceType: string]: string[][] }; /** * Mapping of CDK resources (L1 constructs) to physical resources to be imported @@ -225,12 +225,21 @@ export class ResourceImporter { const resourceIdentifierSummaries = await this.cfn.resourceIdentifierSummaries(this.stack, this.options.toolkitStackName); for (const summary of resourceIdentifierSummaries) { if ('ResourceType' in summary && summary.ResourceType && 'ResourceIdentifiers' in summary && summary.ResourceIdentifiers) { - ret[summary.ResourceType] = summary.ResourceIdentifiers; + ret[summary.ResourceType] = (summary.ResourceIdentifiers ?? [])?.map(x => x.split(',')); } } return ret; } + /** + * Ask for the importable identifier for the given resource + * + * There may be more than one identifier under which a resource can be imported. The `import` + * operation needs exactly one of them. + * + * - If we can get one from the template, we will use one. + * - Otherwise, we will ask the user for one of them. + */ private async askForResourceIdentifier( resourceIdentifiers: ResourceIdentifiers, chg: ImportableResource, @@ -244,45 +253,83 @@ export class ResourceImporter { return undefined; } - const idProps = resourceIdentifiers[resourceType]; - const resourceProps = chg.resourceDefinition.Properties ?? {}; + const idPropSets = resourceIdentifiers[resourceType]; - const fixedIdProps = idProps.filter(p => resourceProps[p]); - const fixedIdInput: ResourceIdentifierProperties = Object.fromEntries(fixedIdProps.map(p => [p, resourceProps[p]])); + // Retain only literal strings: strip potential CFN intrinsics + const resourceProps = Object.fromEntries(Object.entries(chg.resourceDefinition.Properties ?? {}) + .filter(([_, v]) => typeof v === 'string')) as Record; - const missingIdProps = idProps.filter(p => !resourceProps[p]); + // Find property sets that are fully satisfied in the template, ask the user to confirm them + const satisfiedPropSets = idPropSets.filter(ps => ps.every(p => resourceProps[p])); + for (const satisfiedPropSet of satisfiedPropSets) { + const candidateProps = Object.fromEntries(satisfiedPropSet.map(p => [p, resourceProps[p]])); + const displayCandidateProps = fmtdict(candidateProps); - if (missingIdProps.length === 0) { - // We can auto-import this, but ask the user to confirm - const props = fmtdict(fixedIdInput); - - if (!await promptly.confirm( - `${chalk.blue(resourceName)} (${resourceType}): import with ${chalk.yellow(props)} (yes/no) [default: yes]? `, + if (await promptly.confirm( + `${chalk.blue(resourceName)} (${resourceType}): import with ${chalk.yellow(displayCandidateProps)} (yes/no) [default: yes]? `, { default: 'yes' }, )) { - print(chalk.grey(`Skipping import of ${resourceName}`)); - return undefined; + return candidateProps; } } - // Ask the user to provide missing props - const userInput: ResourceIdentifierProperties = {}; - for (const missingIdProp of missingIdProps) { - const response = (await promptly.prompt( - `${chalk.blue(resourceName)} (${resourceType}): enter ${chalk.blue(missingIdProp)} to import (empty to skip):`, - { default: '', trim: true }, - )); - if (!response) { - print(chalk.grey(`Skipping import of ${resourceName}`)); - return undefined; + // If we got here and the user rejected any available identifiers, then apparently they don't want the resource at all + if (satisfiedPropSets.length > 0) { + print(chalk.grey(`Skipping import of ${resourceName}`)); + return undefined; + } + + // We cannot auto-import this, ask the user for one of the props + // The only difference between these cases is what we print: for multiple properties, we print a preamble + const prefix = `${chalk.blue(resourceName)} (${resourceType})`; + let preamble; + let promptPattern; + if (idPropSets.length > 1) { + preamble = `${prefix}: enter one of ${idPropSets.map(x => chalk.blue(x.join('+'))).join(', ')} to import (all empty to skip)`; + promptPattern = `${prefix}: enter %`; + } else { + promptPattern = `${prefix}: enter %`; + } + + // Do the input loop here + if (preamble) { + print(preamble); + } + for (const idProps of idPropSets) { + const input: Record = {}; + for (const idProp of idProps) { + // If we have a value from the template, use it as default. This will only be a partial + // identifier if present, otherwise we would have done the import already above. + const defaultValue = typeof resourceProps[idProp] ?? ''; + + const prompt = [ + promptPattern.replace(/%/, chalk.blue(idProp)), + defaultValue + ? `[${defaultValue}]` + : '(empty to skip)', + ].join(' ') + ':'; + const response = await promptly.prompt(prompt, + { default: defaultValue, trim: true }, + ); + + if (!response) { + break; + } + + input[idProp] = response; + // Also stick this property into 'resourceProps', so that it may be reused by a subsequent question + // (for a different compound identifier that involves the same property). Just a small UX enhancement. + resourceProps[idProp] = response; + } + + // If the user gave inputs for all values, we are complete + if (Object.keys(input).length === idProps.length) { + return input; } - userInput[missingIdProp] = response; } - return { - ...fixedIdInput, - ...userInput, - }; + print(chalk.grey(`Skipping import of ${resourceName}`)); + return undefined; } /** @@ -364,4 +411,4 @@ function addDefaultDeletionPolicy(resource: any): any { export interface DiscoverImportableResourcesResult { readonly additions: ImportableResource[]; readonly hasNonAdditions: boolean; -} \ No newline at end of file +} diff --git a/packages/aws-cdk/test/import.test.ts b/packages/aws-cdk/test/import.test.ts index ec970df533daf..9bbbc7b98a24f 100644 --- a/packages/aws-cdk/test/import.test.ts +++ b/packages/aws-cdk/test/import.test.ts @@ -17,31 +17,53 @@ const promptlyPrompt = promptly.prompt as jest.Mock; let createChangeSetInput: AWS.CloudFormation.CreateChangeSetInput | undefined; -const STACK_WITH_QUEUE = testStack({ - stackName: 'StackWithQueue', - template: { - Resources: { - MyQueue: { - Type: 'AWS::SQS::Queue', - Properties: {}, +function stackWithQueue(props: Record) { + return testStack({ + stackName: 'StackWithQueue', + template: { + Resources: { + MyQueue: { + Type: 'AWS::SQS::Queue', + Properties: props, + }, }, }, - }, + }); +} + +const STACK_WITH_QUEUE = stackWithQueue({}); + +const STACK_WITH_NAMED_QUEUE = stackWithQueue({ + QueueName: 'TheQueueName', }); -const STACK_WITH_NAMED_QUEUE = testStack({ - stackName: 'StackWithQueue', - template: { - Resources: { - MyQueue: { - Type: 'AWS::SQS::Queue', - Properties: { - QueueName: 'TheQueueName', +function stackWithGlobalTable(props: Record) { + return testStack({ + stackName: 'StackWithTable', + template: { + Resources: { + MyTable: { + Type: 'AWS::DynamoDB::GlobalTable', + Properties: props, }, }, }, - }, -}); + }); +} + +function stackWithKeySigningKey(props: Record) { + return testStack({ + stackName: 'StackWithKSK', + template: { + Resources: { + MyKSK: { + Type: 'AWS::Route53::KeySigningKey', + Properties: props, + }, + }, + }, + }); +} let sdkProvider: MockSdkProvider; let deployments: CloudFormationDeployments; @@ -154,6 +176,122 @@ test('asks human to confirm automic import if identifier is in template', async ]); }); +test('only use one identifier if multiple are in template', async () => { + // GIVEN + const stack = stackWithGlobalTable({ + TableName: 'TheTableName', + TableArn: 'ThisFieldDoesntExistInReality', + TableStreamArn: 'NorDoesThisOne', + }); + + // WHEN + promptlyConfirm.mockResolvedValue(true); // Confirm yes/no + await importTemplateFromClean(stack); + + // THEN + expect(createChangeSetInput?.ResourcesToImport).toEqual([ + { + LogicalResourceId: 'MyTable', + ResourceIdentifier: { TableName: 'TheTableName' }, + ResourceType: 'AWS::DynamoDB::GlobalTable', + }, + ]); +}); + +test('only ask user for one identifier if multiple possible ones are possible', async () => { + // GIVEN -- no identifiers in template, so ask user + const stack = stackWithGlobalTable({}); + + // WHEN + promptlyPrompt.mockResolvedValue('Banana'); + const importable = await importTemplateFromClean(stack); + + // THEN -- only asked once + expect(promptlyPrompt).toHaveBeenCalledTimes(1); + expect(importable.resourceMap).toEqual({ + MyTable: { TableName: 'Banana' }, + }); +}); + +test('ask identifier if the value in the template is a CFN intrinsic', async () => { + // GIVEN -- identifier in template is a CFN intrinsic so it doesn't count + const stack = stackWithQueue({ + QueueName: { Ref: 'SomeParam' }, + }); + + // WHEN + promptlyPrompt.mockResolvedValue('Banana'); + const importable = await importTemplateFromClean(stack); + + // THEN + expect(importable.resourceMap).toEqual({ + MyQueue: { QueueName: 'Banana' }, + }); +}); + +test('take compound identifiers from the template if found', async () => { + // GIVEN + const stack = stackWithKeySigningKey({ + HostedZoneId: 'z-123', + Name: 'KeyName', + }); + + // WHEN + promptlyConfirm.mockResolvedValue(true); + await importTemplateFromClean(stack); + + // THEN + expect(createChangeSetInput?.ResourcesToImport).toEqual([ + { + LogicalResourceId: 'MyKSK', + ResourceIdentifier: { HostedZoneId: 'z-123', Name: 'KeyName' }, + ResourceType: 'AWS::Route53::KeySigningKey', + }, + ]); +}); + +test('ask user for compound identifiers if not found', async () => { + // GIVEN + const stack = stackWithKeySigningKey({}); + + // WHEN + promptlyPrompt.mockReturnValue('Banana'); + await importTemplateFromClean(stack); + + // THEN + expect(createChangeSetInput?.ResourcesToImport).toEqual([ + { + LogicalResourceId: 'MyKSK', + ResourceIdentifier: { HostedZoneId: 'Banana', Name: 'Banana' }, + ResourceType: 'AWS::Route53::KeySigningKey', + }, + ]); +}); + +test('do not ask for second part of compound identifier if the user skips the first', async () => { + // GIVEN + const stack = stackWithKeySigningKey({}); + + // WHEN + promptlyPrompt.mockReturnValue(''); + const importMap = await importTemplateFromClean(stack); + + // THEN + expect(importMap.resourceMap).toEqual({}); +}); + +/** + * Do a full import cycle with the given stack template + */ +async function importTemplateFromClean(stack: ReturnType) { + givenCurrentStack(stack.stackName, { Resources: {} }); + const importer = new ResourceImporter(stack, deployments); + const { additions } = await importer.discoverImportableResources(); + const importable = await importer.askForResourceIdentifiers(additions); + await importer.importResources(importable, { stack }); + return importable; +} + function givenCurrentStack(stackName: string, template: any) { sdkProvider.stubCloudFormation({ describeStacks() { @@ -181,6 +319,14 @@ function givenCurrentStack(stackName: string, template: any) { ResourceType: 'AWS::SQS::Queue', ResourceIdentifiers: ['QueueName'], }, + { + ResourceType: 'AWS::DynamoDB::GlobalTable', + ResourceIdentifiers: ['TableName', 'TableArn', 'TableStreamArn'], + }, + { + ResourceType: 'AWS::Route53::KeySigningKey', + ResourceIdentifiers: ['HostedZoneId,Name'], + }, ], }; },