fix(cli): cannot cdk import resources with multiple identifiers

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<string, string>;
 
-    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<string, string> = {};
+      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;
-}
+}

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<string, unknown>) {
+  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<string, unknown>) {
+  return testStack({
+    stackName: 'StackWithTable',
+    template: {
+      Resources: {
+        MyTable: {
+          Type: 'AWS::DynamoDB::GlobalTable',
+          Properties: props,
         },
       },
     },
-  },
-});
+  });
+}
+
+function stackWithKeySigningKey(props: Record<string, unknown>) {
+  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<typeof testStack>) {
+  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'],
+          },
         ],
       };
     },