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

[cli] Change sets that consist only of removalPolicy changes cannot be executed #11521

Open
ghost opened this issue Nov 17, 2020 · 18 comments
Open
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@ghost
Copy link

ghost commented Nov 17, 2020

If the only change to an s3.Bucket object that's already been deployed is to change its removalPolicy, that change appears in cdk diff, but cdk deploy says (no changes) for that stack, and the change is not made to the bucket.

If you deploy another change, such as to the versioned property, along with the removalPolicy change, the change to removalPolicy is made.

This broke the "Your First AWS CDK app" topic in the developer guide, which updated the removalPolicy to allow cdk destroy to delete the bucket. The user could still complete the tutorial, but the bucket would remain when it should have been deleted. (I have updated this topic to make an additional change with the removalPolicy one, which has the added benefit of showing an IAM policy in the diff, which has the side effect of deploying the removalPolicy change.)

Unmodified version of this topic can be seen here: http://jkindall.aka.corp.amazon.com/snapshot/1605908585/hello_world.html

Reproduction Steps

mkdir hello-cdk
cd hello-cdk
cdk init --language typescript
npm install @aws-cdk/aws-s3

Edit lib/hello-cdk-stack.ts to read:

import * as cdk from '@aws-cdk/core';
import * as s3 from '@aws-cdk/aws-s3';

export class HelloCdkStack extends cdk.Stack {
  constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    new s3.Bucket(this, 'TheBucket', {
      versioned: false,
      // removalPolicy: cdk.RemovalPolicy.DESTROY
    });
  }
}

Then:

npm run build
cdk synth
cdk deploy

Uncomment the removalPolicy definiiton.

Save, build, and cdk diff. Note that the change to the removal policy appears in the diff output.

Now cdk deploy and note that it says HelloCdkStack (no changes). Another cdk diff will reveal that there's still a difference between the new synthesized template and the deployed one, indicating the change to removalPolicy was not deployed.

Finally change versioned to true, then cdk deploy and watch both changes be deployed. cdk diff afterward to be sure.

Environment

  • CDK CLI Version : 1.74.0
  • Framework Version: 1.74.0
  • Node.js Version: 12.16.3
  • OS : Windows 10
  • Language (Version): TypeScript 3.9.7

This is 🐛 Bug Report

@ghost ghost added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 17, 2020
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Nov 17, 2020
@ghost ghost changed the title [s3] Change to removal policy won't be deployed if it's the only change [s3] Change to Bucket removal policy won't be deployed if it's the only change Nov 17, 2020
@ghost ghost changed the title [s3] Change to Bucket removal policy won't be deployed if it's the only change [s3] Change to Bucket removal policy isn't deployed when it's the only change Nov 17, 2020
@iliapolo
Copy link
Contributor

This seems to be caused by the fact CloudFormation treats change sets that consist only of changes to the removal policy as empty change sets, causing our CLI to execute them.

if (changeSetHasNoChanges(changeSetDescription)) {
debug('No changes are to be performed on %s.', deployName);
await cfn.deleteChangeSet({ StackName: deployName, ChangeSetName: changeSetName }).promise();
return { noOp: true, outputs: cloudFormationStack.outputs, stackArn: changeSet.StackId!, stackArtifact };

@iliapolo iliapolo changed the title [s3] Change to Bucket removal policy isn't deployed when it's the only change [cli] Change sets that consist only of removalPolicy changes cannot be executed Nov 23, 2020
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Nov 23, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

This is unfortunately unfixable on our end.

@jerry-aws assume this behavior cannot be changed. How bad are the implications of that?

@rix0rrr rix0rrr added the blocked Work is blocked on this issue for this codebase. Other labels or comments may indicate why. label Nov 23, 2020
@iliapolo
Copy link
Contributor

@rix0rrr - yeah, i think the least we can do though is make --force apply the change set anyway.

But honestly if there are various scenarios with these "empty" change sets that we are not applying, we might need to reconsider this optimization, and just execute the change set regardless.

@iliapolo iliapolo removed their assignment Nov 23, 2020
@iliapolo iliapolo removed the @aws-cdk/aws-s3 Related to Amazon S3 label Nov 23, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

We do this on purpose to avoid a CloudFormation ExecuteChangeSet failure.

I guess a potential way around it would be: record whether we THINK the change set is going to be empty, try the Execute anyway and swallow the failure that might occur if we thought the changeset was going to be empty.

Or something to that effect.

But this check is not merely a speed optimization like the "skip deploy" feature is: it's definitely to work around a kind of CFN behavior.

@rix0rrr rix0rrr added effort/medium Medium work item – several days of effort p1 and removed blocked Work is blocked on this issue for this codebase. Other labels or comments may indicate why. labels Nov 23, 2020
@iliapolo
Copy link
Contributor

iliapolo commented Nov 23, 2020

But this check is not merely a speed optimization like the "skip deploy" feature is: it's definitely to work around a kind of CFN behavior.

Is this because we have no way of detecting this scenario from the failure message of ExecuteChangeSet? the same way we do for CreateChangeSet?

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

I don't remember. It might just be us not wanting to issue a failing command.

@iliapolo
Copy link
Contributor

But we already issue a failing CreateChangeSet command :)

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

Research:

It's not the addition of an Output. That leads to a ChangeSet like this:

{
    "Changes": [],
    "ChangeSetName": "CDK-e31b231e-3c25-49d3-b496-e25d1de4a2ec",
    "ChangeSetId": "arn:aws:cloudformation:eu-west-1:993655754359:changeSet/CDK-e31b231e-3c25-49d3-b496-e25d1de4a2ec/fe9d7b49-4cdc-4a4b-81ef-ace4f7b015cf",
    "StackId": "arn:aws:cloudformation:eu-west-1:993655754359:stack/TestcfnStack/cbed84d0-2d95-11eb-ac6f-0a7fc4da8782",
    "StackName": "TestcfnStack",
    "Description": "CDK Changeset for execution e31b231e-3c25-49d3-b496-e25d1de4a2ec",
    "Parameters": null,
    "CreationTime": "2020-11-23T14:13:12.065Z",
    "ExecutionStatus": "AVAILABLE",
    "Status": "CREATE_COMPLETE",
    "StatusReason": null,
    "NotificationARNs": [],
    "RollbackConfiguration": {},
    "Capabilities": [
        "CAPABILITY_IAM",
        "CAPABILITY_NAMED_IAM",
        "CAPABILITY_AUTO_EXPAND"
    ],
    "Tags": null
}

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

When changing just the removalpolicy, change set looks like this:

{
    "Changes": [],
    "ChangeSetName": "CDK-e9f3a791-8b62-4ce8-aca0-27f3428ed77b",
    "ChangeSetId": "arn:aws:cloudformation:eu-west-1:993655754359:changeSet/CDK-e9f3a791-8b62-4ce8-aca0-27f3428ed77b/d113381e-6936-4fed-88ba-bd2ae0194de2",
    "StackId": "arn:aws:cloudformation:eu-west-1:993655754359:stack/TestcfnStack/cbed84d0-2d95-11eb-ac6f-0a7fc4da8782",
    "StackName": "TestcfnStack",
    "Description": "CDK Changeset for execution e9f3a791-8b62-4ce8-aca0-27f3428ed77b",
    "Parameters": null,
    "CreationTime": "2020-11-23T14:17:14.609Z",
    "ExecutionStatus": "UNAVAILABLE",
    "Status": "FAILED",
    "StatusReason": "The submitted information didn't contain changes. Submit different information to create a change set.",
    "NotificationARNs": [],
    "RollbackConfiguration": {},
    "Capabilities": [
        "CAPABILITY_IAM",
        "CAPABILITY_NAMED_IAM",
        "CAPABILITY_AUTO_EXPAND"
    ],
    "Tags": null
}

Execute leads to:

An error occurred (InvalidChangeSetStatus) when calling the ExecuteChangeSet operation: ChangeSet [arn:aws:cloudformation:eu-west-1:993655754359:stack/TestcfnStack/cbed84d0-2d95-11eb-ac6f-0a7fc4da8782] cannot be executed in its current status of [FAILED]

So it definitely will not execute.

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 23, 2020

Behavior is the same if you add a Parameter.

@ghost
Copy link
Author

ghost commented Nov 23, 2020

Is it feasible to deploy some kind of no-op change (like setting a property to its current value) along with the problematic change in this situation, assuming the situation can be detected? Or are CloudFormation changes that don't actually change anything invalid?

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Nov 23, 2020
@jfly
Copy link

jfly commented Mar 2, 2022

I just ran into this myself. Is there any hope for fixing this, or at least a workaround with some sort of no-op change?

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 2, 2022

You need to make the no-op change yourself unfortunately

@jfly
Copy link

jfly commented Mar 2, 2022

Is there a clever no-op that I could blindly use on any resource type? Or do I have to figure out a no-op change for each type of resource I'm dealing with?

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 2, 2022

Add a WaitConditionHandle. Doesn’t do or cost anything, but does change the template

@blimmer
Copy link
Contributor

blimmer commented Apr 12, 2022

I was able to force a deployment by using:

cdk deploy --no-version-reporting '<STACKNAME>'

This removes the CDKMetadata property, forcing a change. I was able to reproduce this issue using the CloudFormation console. The changeset failed when just changing a retention policy. This seems like a pretty bad CloudFormation bug - is there any public issue tracking this upstream?

@blimmer
Copy link
Contributor

blimmer commented Apr 25, 2022

I reached out to support about this and they mentioned that there's an internal feature request ("Add support for policy change through ChangeSets") that has been opened and accepted by the CloudFormation team. Maybe the CDK team could add their +1 internally to help move that along (cc @rix0rrr)?

@corymhall corymhall added the needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. label Aug 12, 2022
@TheRealAmazonKendra
Copy link
Contributor

Related to #15065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

No branches or pull requests

7 participants