Skip to content

Commit

Permalink
fix(cli): cannot cdk import resources with multiple identifiers (aw…
Browse files Browse the repository at this point in the history 
…s#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 aws#20895.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@rix0rrr @homakk
rix0rrr authored and homakk committed Mar 28, 2023
1 parent abbcbbb commit 17402b7
Show file tree
Hide file tree
Showing 2 changed files with 246 additions and 53 deletions.
117 changes: 82 additions & 35 deletions packages/aws-cdk/lib/import.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -364,4 +411,4 @@ function addDefaultDeletionPolicy(resource: any): any {
export interface DiscoverImportableResourcesResult {
readonly additions: ImportableResource[];
readonly hasNonAdditions: boolean;
}
}
182 changes: 164 additions & 18 deletions packages/aws-cdk/test/import.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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'],
},
],
};
},
Expand Down

0 comments on commit 17402b7

Please sign in to comment.