(cfn-include): Handling parameters in a way that is less scary when migrating

plumdog commented 3 years ago

Introduce a way of setting parameters, but in such a way that the diff doesn't make me scared when migrating existing Cloudformation stacks into CDK.

Use Case

I have an existing production system deployed using Cloudformation with parameters, and want to bring it into CDK so I can write new code as CDK and throw away the old deployment tooling. And, where possible, manage changes to the old code through CDK (eg using cfnTemplate.getResource). And I think this will all work fine.

However, I pointed it at my yaml templates, set the parameters for each stack, ran cdk diff and was immediately terrified. There were lots and lots of changes because of clever CDK things setting parameters. I think all these changes are no-ops from what I have been able to manually review.

A small slice of the diff I see:

Parameters
[-] Parameter EnvironmentName: {"Type":"String","Description":"The name of the environment"}
...
Resources
[~] AWS::S3::Bucket mystack-include/BackupBucket BackupBucket replace
 └─ [~] BucketName (requires replacement)
     └─ [~] .Fn::Sub:
         ├─ [-] ${EnvironmentName}-backups
         └─ [+] myapp-backups

Where the existing Cloudformation parameter EnvironmentName is set to myapp, and I've passed parameters: {EnvironmentName: 'myapp'} to CfnInclude. Is it going to try to delete that bucket and replace it? I think... no? It will fail to do so even if it does try, because there's stuff in the bucket, but hopefully this illustrates the source of my fear.

Proposed Solution

Some options I can think of:

Bravery. Trust in the CDK's handling of these parameters to do the right thing.
Diligence. Manually verify all of the parameters will be change from: (a reference to a parameter that is set to X) to: (just X)
cfn-include.CfnInclude grow the ability to set parameters "the old fashioned way", which would match with what I am migrating into CDK using this module. I would expect this to then show nothing in the diff (with the exception of the CDK metadata stuff) which would no longer terrify me. I could then maybe switch the parameters over one-by-one to reduce my fear and to make the diff easier to understand, if I felt the need.
A hack. I have hunted for a way of forcing the parameters to my stacks to be the values I want them to be but not found anything. If I don't set them, then they don't complain and I guess use the values currently deployed in Cloudformation, which is ok, but means their values are not in my source control. And I can maybe set them with the command line, but I can't see a way of doing this when running diff, so am unsure. I was able to update the defaults for the parameters in the template, but I hate this because the parameter values actually set in Cloudformation will take precedence.
Trust Cloudformation changesets rather than cdk's diffs. Run synth, update the stacks with the outputs from that, review the changesets, verify they are no-ops (but for adding CDKMetadata) and execute. This is annoyingly manual, but I think is quite safe.

I would be fairly happy with any of those options except 2. And there might well be another way of handling this to change the synth behaviour around parameters that I don't know.

[ ] :wave: I may be able to implement this feature request
[ ] :warning: This feature might incur a breaking change

This is a :rocket: Feature Request

skinny85 commented 3 years ago

Hey @plumdog ,

thanks for opening the issue. You've ran into a very interesting edge case in CloudFormation. The crux of the issue is:

There is a difference in the unresolved template, but not in the resolved template

Where "resolve" means "the values of the parameters have been substituted into the template body, and all CloudFormation functions have been called".

Now, the most important thing: this substitution is actually safe. Meaning, if you run cdk deploy while having that result of cdk diff, CloudFormation will generate an empty change set, and your Bucket will not be replaced.

The bad part is that, because the change set was empty, no actual deployment happened - which means cdk diff after that will still see the old template, and report that same difference.

You can get rid of it by changing any property of the affected resource - something like the metadata is enough. So, in your case, that would be:

const backupBucket = cfnInclude.getResource('BackupBucket');
backupBucket.addMetadata('A', 'B'); // this can be anything

After running cdk deploy, the metadata will force the resource to be updated, and the template for it will be changed (again, this is safe; contrary to what cdk diff says, your Bucket will not be replaced!).

After that deployment succeeds, you can safely get rid of the metadata, and perform another deployment to have your resource be "pristine".

skinny85 commented 3 years ago

Having cleared all that up, the question remains on "how do we make that cdk diff message less scary". I think it's an interesting problem! We would need to check the current value of the parameters, and "resolve" CloudFormation functions like Fn::Sub and Fn::Join to actually determine whether the value in the template and the new value are in fact the same after parameter substitution (even though they differ in the template itself).

I'm leaving this as a feature request for the cloudformation-diff module, where this logic would have to live.

vito-laurenza-zocdoc commented 3 years ago

Having cleared all that up, the question remains on "how do we make that cdk diff message less scary". I think it's an interesting problem! We would need to check the current value of the parameters, and "resolve" CloudFormation functions like Fn::Sub and Fn::Join to actually determine whether the value in the template and the new value are in fact the same after parameter substitution (even though they differ in the template itself).

I'm leaving this as a feature request for the cloudformation-diff module, where this logic would have to live.

This feature would be hugely helpful for those of us migrating lots of legacy stacks. We've been resorting to creating and manually verifying changesets and it's tedious at best.