Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): warn the user of resource renaming #33257

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
8 changes: 8 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ export function withCDKMigrateFixture(language: string, block: (content: TestFix
return withAws(withTimeout(DEFAULT_TEST_TIMEOUT_S, withCdkMigrateApp(language, block)));
}

export function withCompositeFixture(blocks: ((context: TestContext & AwsContext & DisableBootstrapContext) => Promise<void>)[]) {
return withAws(withTimeout(DEFAULT_TEST_TIMEOUT_S, async (context: TestContext & AwsContext & DisableBootstrapContext) => {
for (let block of blocks) {
await block(context);
}
}));
}

export interface DisableBootstrapContext {
/**
* Whether to disable creating the default bootstrap
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"app": "node refactoring.js",
"versionReporting": false,
"context": {
"aws-cdk:enableDiffNoFail": "true"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const cdk = require('aws-cdk-lib');
const sqs = require('aws-cdk-lib/aws-sqs');

const stackPrefix = process.env.STACK_NAME_PREFIX;

const app = new cdk.App();

class NoticesStackRefactored extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);
new sqs.Queue(this, 'newQueueName');
}
}

new NoticesStackRefactored(app, `${stackPrefix}-notices`);
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ import {
retry,
shell,
sleep,
withCDKMigrateFixture,
withCDKMigrateFixture, withCompositeFixture,
withDefaultFixture,
withExtendedTimeoutFixture,
withoutBootstrap,
withSamIntegrationFixture,
withSamIntegrationFixture, withCdkApp, withSpecificCdkApp,
withSpecificFixture,
} from '../../lib';
import { awsActionsFromRequests, startProxyServer } from '../../lib/proxy';
Expand Down Expand Up @@ -2833,6 +2833,40 @@ integTest(
}),
);

integTest('renamed resources require user approval',
withCompositeFixture([
// First deploy a stack
withCdkApp(async (fixture) => {
process.env.INTEG_NO_CLEAN = 'true';

await fixture.cdkDeploy('notices');
}),

// Then deploy a stack with the same name,
// but with a resource name changed.
withSpecificCdkApp('refactoring', async (fixture) => {
delete process.env.INTEG_NO_CLEAN;

const stackName = 'notices';
const stdErr = await fixture.cdkDeploy(stackName, {
allowErrExit: true,
});

// ensure the user would be ask for confirmation
expect(stdErr).toContain('Some resources have been renamed, which may cause resource replacement');

const stackDescription = await fixture.aws.cloudFormation.send(
new DescribeStacksCommand({
StackName: fixture.fullStackName(stackName),
}),
);

// ensure stack was not updated
expect(stackDescription.Stacks?.[0].StackStatus === 'CREATE_COMPLETE');
}),
]),
);

integTest('cdk notices are displayed correctly', withDefaultFixture(async (fixture) => {
const cache = {
expiration: 4125963264000, // year 2100 so we never overwrite the cache
Expand Down
52 changes: 52 additions & 0 deletions packages/aws-cdk/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,58 @@ and might have breaking changes in the future.

> *: `Fn::GetAtt` is only partially supported. Refer to [this implementation](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts#L477-L492) for supported resources and attributes.

#### Renaming detection

If you change the ID of a construct, this change will be propagated all the
way down to the CloudFormation level, leading to the change of at least one
resource logical ID. If you then use this template to update a stack created
with the previous version of the template, all the renamed resources will be
replaced.

Since a renaming like this might have happened by accident, the CLI will detect
such cases just before performing a deployment, and ask for confirmation to go
ahead. For example, suppose you have the following construct tree:

```text
MyStack
└ MyBucket
└ Resource
```

In the template, the bucket resource will have a logical ID like
`MyBucketFD3F4958`. So you deploy this template for the first time, creating
the stack. Then you decide to rename the `MyBucket` construct to `NewName`
and try to deploy it again, to update the stack. Since the logical ID has
changed (to something like `NewNameEF569C1A`), the CLI will prompt you with
the following message:

```text
Some resources have been renamed:
- MyStack/MyBucket/Resource -> MyStack/NewName/Resource

Do you wish to deploy these changes (y/n)?
```

Notice that it displays the CDK metadata path for the resource. If this
field is not present, it will use the logical ID instead.

Resources are considered equal if they have the same content (up to reordering
of properties). So if you have different logical IDs referring to equal objects,
and you rename at least two of them, it's impossible to establish a 1:1
correspondence as above. So the CLI will display a correspondence between sets
of identifiers:

```text
Some resources have been renamed:
- {MyStack/MyBucket1/Resource, MyStack/MyBucket2/Resource} ->
{MyStack/NewName1/Resource, MyStack/NewName2/Resource}

Do you wish to deploy these changes (y/n)?
```

As usual, you can force the CLI to skip this approval with the
`--require-approval nevel` option.

### `cdk rollback`

If a deployment performed using `cdk deploy --no-rollback` fails, your
Expand Down
48 changes: 33 additions & 15 deletions packages/aws-cdk/lib/cli/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import * as uuid from 'uuid';
import { Configuration, PROJECT_CONFIG } from './user-configuration';
import { SdkProvider } from '../api/aws-auth';
import { Bootstrapper, BootstrapEnvironmentOptions } from '../api/bootstrap';
import { BootstrapEnvironmentOptions, Bootstrapper } from '../api/bootstrap';
import {
CloudAssembly,
DefaultSelection,
Expand All @@ -18,37 +18,38 @@
} from '../api/cxapp/cloud-assembly';
import { CloudExecutable } from '../api/cxapp/cloud-executable';
import { environmentsFromDescriptors, globEnvironmentsFromStacks, looksLikeGlob } from '../api/cxapp/environments';
import { Deployments, DeploymentMethod, SuccessfulDeployStackResult, createDiffChangeSet } from '../api/deployments';
import { createDiffChangeSet, DeploymentMethod, Deployments, SuccessfulDeployStackResult } from '../api/deployments';
import { GarbageCollector } from '../api/garbage-collection/garbage-collector';
import { HotswapMode, HotswapPropertyOverrides, EcsHotswapProperties } from '../api/hotswap/common';
import { EcsHotswapProperties, HotswapMode, HotswapPropertyOverrides } from '../api/hotswap/common';
import { findCloudWatchLogGroups } from '../api/logs/find-cloudwatch-logs';
import { CloudWatchLogEventMonitor } from '../api/logs/logs-monitor';
import { tagsForStack, type Tag } from '../api/tags';
import { type Tag, tagsForStack } from '../api/tags';
import { StackActivityProgress } from '../api/util/cloudformation/stack-activity-monitor';
import { formatTime } from '../api/util/string-manipulation';
import {
appendWarningsToReadme,
buildCfnClient,
buildGenertedTemplateOutput,
CfnTemplateGeneratorProvider,
FromScan,
generateCdkApp,
generateStack,
generateTemplate,
GenerateTemplateOutput,
isThereAWarning,
parseSourceOptions,
readFromPath,
readFromStack,
setEnvironment,
parseSourceOptions,
generateTemplate,
FromScan,
TemplateSourceOptions,
GenerateTemplateOutput,
CfnTemplateGeneratorProvider,
writeMigrateJsonFile,
buildGenertedTemplateOutput,
appendWarningsToReadme,
isThereAWarning,
buildCfnClient,
} from '../commands/migrate';
import { printSecurityDiff, printStackDiff, RequireApproval } from '../diff';
import { ResourceImporter, removeNonImportResources } from '../import';
import { removeNonImportResources, ResourceImporter } from '../import';
import { listStacks } from '../list-stacks';
import { result as logResult, debug, error, highlight, info, success, warning } from '../logging';
import { ResourceMigrator } from '../migrator';
import { findResourceCorrespondence } from '../refactoring';
import { deserializeStructure, obscureTemplate, serializeStructure } from '../serialize';
import { CliIoHost } from '../toolkit/cli-io-host';
import { ToolkitError } from '../toolkit/error';
Expand Down Expand Up @@ -359,6 +360,24 @@
);
}

const currentTemplate = await this.props.deployments.readCurrentTemplate(stack);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be added to the new Toolkit as well (yes I now it's annoying).

const deployedResources = currentTemplate?.Resources;
const correspondence = findResourceCorrespondence(
deployedResources ?? {},
stack.template.Resources ?? {},
);

if (!correspondence.isEmpty()) {
warning(`Some resources have been renamed:${correspondence}`);

Check warning on line 371 in packages/aws-cdk/lib/cli/cdk-toolkit.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/cli/cdk-toolkit.ts#L371

Added line #L371 was not covered by tests

await askUserConfirmation(

Check warning on line 373 in packages/aws-cdk/lib/cli/cdk-toolkit.ts

View check run for this annotation

Codecov / codecov/patch

packages/aws-cdk/lib/cli/cdk-toolkit.ts#L373

Added line #L373 was not covered by tests
this.ioHost,
concurrency,
'Some resources have been renamed, which may cause resource replacement',
'Do you wish to deploy these changes',
);
}

if (Object.keys(stack.template.Resources || {}).length === 0) {
// The generated stack has no resources
if (!(await this.props.deployments.stackExists({ stack }))) {
Expand All @@ -378,7 +397,6 @@
}

if (requireApproval !== RequireApproval.Never) {
const currentTemplate = await this.props.deployments.readCurrentTemplate(stack);
if (printSecurityDiff(currentTemplate, stack, requireApproval)) {
await askUserConfirmation(
this.ioHost,
Expand Down
136 changes: 136 additions & 0 deletions packages/aws-cdk/lib/refactoring.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move to api/refactoring.ts dvd the test file accordingly.

Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import { createHash } from 'crypto';

/**
* The Resources section of a CFN template
*/
export type ResourceMap = Map<string, any>

/**
* Map of resource hashes to sets of logical IDs
*/
export type ResourceIndex = Map<string, Set<string>>

/**
* A pair of sets of logical IDs that are equivalent,
* that is, that refer to the same resource.
*/
export type RenamingPair = [Set<string>, Set<string>]

export class ResourceCorrespondence {
constructor(
public readonly pairs: RenamingPair[],
private readonly oldResources: Record<string, any>,
private readonly newResources: Record<string, any>,
) {
}

toString(): string {
function toMetadataUsing(set: Set<string>, resources: Record<string, any>): Set<string> {
return new Set([...set].map(s => resources[s].Metadata?.['aws:cdk:path'] as string ?? s));
}

let text = this.pairs
.map(([before, after]) => ([
toMetadataUsing(before, this.oldResources),
toMetadataUsing(after, this.newResources),
]))
.map(([before, after]) => {
if (before.size === 1 && after.size === 1) {
return `${[...before][0]} -> ${[...after][0]}`;
} else {
return `{${[...before].join(', ')}} -> {${[...after].join(', ')}}`;
}
})
.map(x => ` - ${x}`)
.join('\n');

return `\n${text}\n`;
}

isEmpty(): boolean {
return this.pairs.length === 0;
}
}

/**
* Given two records of (logical ID -> object), finds a list of objects that
* are present in both records but have different logical IDs. For each of
* these objects, this function returns a pair of sets of logical IDs. The
* first set coming from the first record, and the second set, from the
* second object. For example:
*
* const corr = findResourceCorrespondence({a: {x: 0}}, {b: {x: 0}});
*
* `corr` should be `[new Set('a'), new Set('b')]`
*
*/
export function findResourceCorrespondence(m1: Record<string, any>, m2: Record<string, any>): ResourceCorrespondence {
const pairs = renamingPairs(
index(new Map(Object.entries(m1))),
index(new Map(Object.entries(m2))),
);

return new ResourceCorrespondence(pairs, m1, m2);
}

function index(resourceMap: ResourceMap): ResourceIndex {
const result: ResourceIndex = new Map();
resourceMap.forEach((resource, logicalId) => {
const h = hashObject(resource);
if (!result.has(h)) {
result.set(h, new Set());
}
result.get(h)?.add(logicalId);
});
return result;
}

function renamingPairs(index1: ResourceIndex, index2: ResourceIndex): RenamingPair[] {
const result: RenamingPair[] = [];

const keyUnion = new Set([...index1.keys(), ...index2.keys()]);
keyUnion.forEach((key) => {
if (index1.has(key) && index2.has(key) && index1.get(key) !== index2.get(key)) {
result.push(partitionedSymmetricDifference(index1.get(key)!, index2.get(key)!));
}
});

return result.filter(c => c[0].size > 0 && c[1].size > 0);
}

/**
* Returns a pair [s0 \ s1, s1 \ s0].
*/
function partitionedSymmetricDifference(s0: Set<string>, s1: Set<string>): [Set<string>, Set<string>] {
return [difference(s0, s1), difference(s1, s0)];
}

function hashObject(obj: any): string {
const hash = createHash('sha256');

function addToHash(value: any) {
if (typeof value === 'object') {
if (value == null) {
addToHash('null');
} else if (Array.isArray(value)) {
value.forEach(addToHash);
} else {
Object.keys(value).sort().forEach(key => {
hash.update(key);
addToHash(value[key]);
});
}
} else {
hash.update(typeof value + value.toString());
}
}

const { Metadata, ...rest } = obj;
addToHash(rest);

return hash.digest('hex');
}

function difference<A>(xs: Set<A>, ys: Set<A>): Set<A> {
return new Set([...xs].filter(x => !ys.has(x)));
}
Loading