Skip to content

Commit a70ff1a

Browse files
rix0rrrrix0rrr
authored
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*
1 parent d36857e commit a70ff1a

File tree

2 files changed

+246
-53
lines changed

2 files changed

+246
-53
lines changed

‎packages/aws-cdk/lib/import.ts

Lines changed: 82 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,18 @@ import { ResourceIdentifierProperties, ResourcesToImport } from './api/util/clou
99
import { error, print, success, warning } from './logging';
1010

1111
/**
12-
* Parameters that uniquely identify a physical resource of a given type
12+
* Set of parameters that uniquely identify a physical resource of a given type
1313
* for the import operation, example:
1414
*
1515
* ```
1616
* {
17-
* "AWS::S3::Bucket": ["BucketName"],
18-
* "AWS::IAM::Role": ["RoleName"],
19-
* "AWS::EC2::VPC": ["VpcId"]
17+
* "AWS::S3::Bucket": [["BucketName"]],
18+
* "AWS::DynamoDB::GlobalTable": [["TableName"], ["TableArn"], ["TableStreamArn"]],
19+
* "AWS::Route53::KeySigningKey": [["HostedZoneId", "Name"]],
2020
* }
2121
* ```
2222
*/
23-
export type ResourceIdentifiers = { [resourceType: string]: string[] };
23+
export type ResourceIdentifiers = { [resourceType: string]: string[][] };
2424

2525
/**
2626
* Mapping of CDK resources (L1 constructs) to physical resources to be imported
@@ -225,12 +225,21 @@ export class ResourceImporter {
225225
const resourceIdentifierSummaries = await this.cfn.resourceIdentifierSummaries(this.stack, this.options.toolkitStackName);
226226
for (const summary of resourceIdentifierSummaries) {
227227
if ('ResourceType' in summary && summary.ResourceType && 'ResourceIdentifiers' in summary && summary.ResourceIdentifiers) {
228-
ret[summary.ResourceType] = summary.ResourceIdentifiers;
228+
ret[summary.ResourceType] = (summary.ResourceIdentifiers ?? [])?.map(x => x.split(','));
229229
}
230230
}
231231
return ret;
232232
}
233233

234+
/**
235+
* Ask for the importable identifier for the given resource
236+
*
237+
* There may be more than one identifier under which a resource can be imported. The `import`
238+
* operation needs exactly one of them.
239+
*
240+
* - If we can get one from the template, we will use one.
241+
* - Otherwise, we will ask the user for one of them.
242+
*/
234243
private async askForResourceIdentifier(
235244
resourceIdentifiers: ResourceIdentifiers,
236245
chg: ImportableResource,
@@ -244,45 +253,83 @@ export class ResourceImporter {
244253
return undefined;
245254
}
246255

247-
const idProps = resourceIdentifiers[resourceType];
248-
const resourceProps = chg.resourceDefinition.Properties ?? {};
256+
const idPropSets = resourceIdentifiers[resourceType];
249257

250-
const fixedIdProps = idProps.filter(p => resourceProps[p]);
251-
const fixedIdInput: ResourceIdentifierProperties = Object.fromEntries(fixedIdProps.map(p => [p, resourceProps[p]]));
258+
// Retain only literal strings: strip potential CFN intrinsics
259+
const resourceProps = Object.fromEntries(Object.entries(chg.resourceDefinition.Properties ?? {})
260+
.filter(([_, v]) => typeof v === 'string')) as Record<string, string>;
252261

253-
const missingIdProps = idProps.filter(p => !resourceProps[p]);
262+
// Find property sets that are fully satisfied in the template, ask the user to confirm them
263+
const satisfiedPropSets = idPropSets.filter(ps => ps.every(p => resourceProps[p]));
264+
for (const satisfiedPropSet of satisfiedPropSets) {
265+
const candidateProps = Object.fromEntries(satisfiedPropSet.map(p => [p, resourceProps[p]]));
266+
const displayCandidateProps = fmtdict(candidateProps);
254267

255-
if (missingIdProps.length === 0) {
256-
// We can auto-import this, but ask the user to confirm
257-
const props = fmtdict(fixedIdInput);
258-
259-
if (!await promptly.confirm(
260-
`${chalk.blue(resourceName)} (${resourceType}): import with ${chalk.yellow(props)} (yes/no) [default: yes]? `,
268+
if (await promptly.confirm(
269+
`${chalk.blue(resourceName)} (${resourceType}): import with ${chalk.yellow(displayCandidateProps)} (yes/no) [default: yes]? `,
261270
{ default: 'yes' },
262271
)) {
263-
print(chalk.grey(`Skipping import of ${resourceName}`));
264-
return undefined;
272+
return candidateProps;
265273
}
266274
}
267275

268-
// Ask the user to provide missing props
269-
const userInput: ResourceIdentifierProperties = {};
270-
for (const missingIdProp of missingIdProps) {
271-
const response = (await promptly.prompt(
272-
`${chalk.blue(resourceName)} (${resourceType}): enter ${chalk.blue(missingIdProp)} to import (empty to skip):`,
273-
{ default: '', trim: true },
274-
));
275-
if (!response) {
276-
print(chalk.grey(`Skipping import of ${resourceName}`));
277-
return undefined;
276+
// If we got here and the user rejected any available identifiers, then apparently they don't want the resource at all
277+
if (satisfiedPropSets.length > 0) {
278+
print(chalk.grey(`Skipping import of ${resourceName}`));
279+
return undefined;
280+
}
281+
282+
// We cannot auto-import this, ask the user for one of the props
283+
// The only difference between these cases is what we print: for multiple properties, we print a preamble
284+
const prefix = `${chalk.blue(resourceName)} (${resourceType})`;
285+
let preamble;
286+
let promptPattern;
287+
if (idPropSets.length > 1) {
288+
preamble = `${prefix}: enter one of ${idPropSets.map(x => chalk.blue(x.join('+'))).join(', ')} to import (all empty to skip)`;
289+
promptPattern = `${prefix}: enter %`;
290+
} else {
291+
promptPattern = `${prefix}: enter %`;
292+
}
293+
294+
// Do the input loop here
295+
if (preamble) {
296+
print(preamble);
297+
}
298+
for (const idProps of idPropSets) {
299+
const input: Record<string, string> = {};
300+
for (const idProp of idProps) {
301+
// If we have a value from the template, use it as default. This will only be a partial
302+
// identifier if present, otherwise we would have done the import already above.
303+
const defaultValue = typeof resourceProps[idProp] ?? '';
304+
305+
const prompt = [
306+
promptPattern.replace(/%/, chalk.blue(idProp)),
307+
defaultValue
308+
? `[${defaultValue}]`
309+
: '(empty to skip)',
310+
].join(' ') + ':';
311+
const response = await promptly.prompt(prompt,
312+
{ default: defaultValue, trim: true },
313+
);
314+
315+
if (!response) {
316+
break;
317+
}
318+
319+
input[idProp] = response;
320+
// Also stick this property into 'resourceProps', so that it may be reused by a subsequent question
321+
// (for a different compound identifier that involves the same property). Just a small UX enhancement.
322+
resourceProps[idProp] = response;
323+
}
324+
325+
// If the user gave inputs for all values, we are complete
326+
if (Object.keys(input).length === idProps.length) {
327+
return input;
278328
}
279-
userInput[missingIdProp] = response;
280329
}
281330

282-
return {
283-
...fixedIdInput,
284-
...userInput,
285-
};
331+
print(chalk.grey(`Skipping import of ${resourceName}`));
332+
return undefined;
286333
}
287334

288335
/**
@@ -364,4 +411,4 @@ function addDefaultDeletionPolicy(resource: any): any {
364411
export interface DiscoverImportableResourcesResult {
365412
readonly additions: ImportableResource[];
366413
readonly hasNonAdditions: boolean;
367-
}
414+
}

‎packages/aws-cdk/test/import.test.ts

Lines changed: 164 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,31 +17,53 @@ const promptlyPrompt = promptly.prompt as jest.Mock;
1717

1818
let createChangeSetInput: AWS.CloudFormation.CreateChangeSetInput | undefined;
1919

20-
const STACK_WITH_QUEUE = testStack({
21-
stackName: 'StackWithQueue',
22-
template: {
23-
Resources: {
24-
MyQueue: {
25-
Type: 'AWS::SQS::Queue',
26-
Properties: {},
20+
function stackWithQueue(props: Record<string, unknown>) {
21+
return testStack({
22+
stackName: 'StackWithQueue',
23+
template: {
24+
Resources: {
25+
MyQueue: {
26+
Type: 'AWS::SQS::Queue',
27+
Properties: props,
28+
},
2729
},
2830
},
29-
},
31+
});
32+
}
33+
34+
const STACK_WITH_QUEUE = stackWithQueue({});
35+
36+
const STACK_WITH_NAMED_QUEUE = stackWithQueue({
37+
QueueName: 'TheQueueName',
3038
});
3139

32-
const STACK_WITH_NAMED_QUEUE = testStack({
33-
stackName: 'StackWithQueue',
34-
template: {
35-
Resources: {
36-
MyQueue: {
37-
Type: 'AWS::SQS::Queue',
38-
Properties: {
39-
QueueName: 'TheQueueName',
40+
function stackWithGlobalTable(props: Record<string, unknown>) {
41+
return testStack({
42+
stackName: 'StackWithTable',
43+
template: {
44+
Resources: {
45+
MyTable: {
46+
Type: 'AWS::DynamoDB::GlobalTable',
47+
Properties: props,
4048
},
4149
},
4250
},
43-
},
44-
});
51+
});
52+
}
53+
54+
function stackWithKeySigningKey(props: Record<string, unknown>) {
55+
return testStack({
56+
stackName: 'StackWithKSK',
57+
template: {
58+
Resources: {
59+
MyKSK: {
60+
Type: 'AWS::Route53::KeySigningKey',
61+
Properties: props,
62+
},
63+
},
64+
},
65+
});
66+
}
4567

4668
let sdkProvider: MockSdkProvider;
4769
let deployments: CloudFormationDeployments;
@@ -154,6 +176,122 @@ test('asks human to confirm automic import if identifier is in template', async
154176
]);
155177
});
156178

179+
test('only use one identifier if multiple are in template', async () => {
180+
// GIVEN
181+
const stack = stackWithGlobalTable({
182+
TableName: 'TheTableName',
183+
TableArn: 'ThisFieldDoesntExistInReality',
184+
TableStreamArn: 'NorDoesThisOne',
185+
});
186+
187+
// WHEN
188+
promptlyConfirm.mockResolvedValue(true); // Confirm yes/no
189+
await importTemplateFromClean(stack);
190+
191+
// THEN
192+
expect(createChangeSetInput?.ResourcesToImport).toEqual([
193+
{
194+
LogicalResourceId: 'MyTable',
195+
ResourceIdentifier: { TableName: 'TheTableName' },
196+
ResourceType: 'AWS::DynamoDB::GlobalTable',
197+
},
198+
]);
199+
});
200+
201+
test('only ask user for one identifier if multiple possible ones are possible', async () => {
202+
// GIVEN -- no identifiers in template, so ask user
203+
const stack = stackWithGlobalTable({});
204+
205+
// WHEN
206+
promptlyPrompt.mockResolvedValue('Banana');
207+
const importable = await importTemplateFromClean(stack);
208+
209+
// THEN -- only asked once
210+
expect(promptlyPrompt).toHaveBeenCalledTimes(1);
211+
expect(importable.resourceMap).toEqual({
212+
MyTable: { TableName: 'Banana' },
213+
});
214+
});
215+
216+
test('ask identifier if the value in the template is a CFN intrinsic', async () => {
217+
// GIVEN -- identifier in template is a CFN intrinsic so it doesn't count
218+
const stack = stackWithQueue({
219+
QueueName: { Ref: 'SomeParam' },
220+
});
221+
222+
// WHEN
223+
promptlyPrompt.mockResolvedValue('Banana');
224+
const importable = await importTemplateFromClean(stack);
225+
226+
// THEN
227+
expect(importable.resourceMap).toEqual({
228+
MyQueue: { QueueName: 'Banana' },
229+
});
230+
});
231+
232+
test('take compound identifiers from the template if found', async () => {
233+
// GIVEN
234+
const stack = stackWithKeySigningKey({
235+
HostedZoneId: 'z-123',
236+
Name: 'KeyName',
237+
});
238+
239+
// WHEN
240+
promptlyConfirm.mockResolvedValue(true);
241+
await importTemplateFromClean(stack);
242+
243+
// THEN
244+
expect(createChangeSetInput?.ResourcesToImport).toEqual([
245+
{
246+
LogicalResourceId: 'MyKSK',
247+
ResourceIdentifier: { HostedZoneId: 'z-123', Name: 'KeyName' },
248+
ResourceType: 'AWS::Route53::KeySigningKey',
249+
},
250+
]);
251+
});
252+
253+
test('ask user for compound identifiers if not found', async () => {
254+
// GIVEN
255+
const stack = stackWithKeySigningKey({});
256+
257+
// WHEN
258+
promptlyPrompt.mockReturnValue('Banana');
259+
await importTemplateFromClean(stack);
260+
261+
// THEN
262+
expect(createChangeSetInput?.ResourcesToImport).toEqual([
263+
{
264+
LogicalResourceId: 'MyKSK',
265+
ResourceIdentifier: { HostedZoneId: 'Banana', Name: 'Banana' },
266+
ResourceType: 'AWS::Route53::KeySigningKey',
267+
},
268+
]);
269+
});
270+
271+
test('do not ask for second part of compound identifier if the user skips the first', async () => {
272+
// GIVEN
273+
const stack = stackWithKeySigningKey({});
274+
275+
// WHEN
276+
promptlyPrompt.mockReturnValue('');
277+
const importMap = await importTemplateFromClean(stack);
278+
279+
// THEN
280+
expect(importMap.resourceMap).toEqual({});
281+
});
282+
283+
/**
284+
* Do a full import cycle with the given stack template
285+
*/
286+
async function importTemplateFromClean(stack: ReturnType<typeof testStack>) {
287+
givenCurrentStack(stack.stackName, { Resources: {} });
288+
const importer = new ResourceImporter(stack, deployments);
289+
const { additions } = await importer.discoverImportableResources();
290+
const importable = await importer.askForResourceIdentifiers(additions);
291+
await importer.importResources(importable, { stack });
292+
return importable;
293+
}
294+
157295
function givenCurrentStack(stackName: string, template: any) {
158296
sdkProvider.stubCloudFormation({
159297
describeStacks() {
@@ -181,6 +319,14 @@ function givenCurrentStack(stackName: string, template: any) {
181319
ResourceType: 'AWS::SQS::Queue',
182320
ResourceIdentifiers: ['QueueName'],
183321
},
322+
{
323+
ResourceType: 'AWS::DynamoDB::GlobalTable',
324+
ResourceIdentifiers: ['TableName', 'TableArn', 'TableStreamArn'],
325+
},
326+
{
327+
ResourceType: 'AWS::Route53::KeySigningKey',
328+
ResourceIdentifiers: ['HostedZoneId,Name'],
329+
},
184330
],
185331
};
186332
},

0 commit comments

Comments
 (0)