From d4a132bff91ab8e78ed38dc5ee41842554347ecf Mon Sep 17 00:00:00 2001
From: Elad Ben-Israel <benisrae@amazon.com>
Date: Wed, 19 Feb 2020 13:38:55 +0200
Subject: [PATCH] feat(cli): faster "no-op" deployments (#6346)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* feat(cli): skip deployment if template did not change

Bail-out before executing and creating change sets when the currently deployed template is identical to the one we are about to deploy.

This resolves #6046, where a stack that contains nested stack(s) will always try to update since CloudFormation assumes the nested template might have changed. In the CDK, since nested template URLs are immutable, we can trust that the URL will be changed, and therefore invalidate the *parent* template.

This also fixes the bug described in #2553, where a stack that includes a `Transform` always fail to deploy with `No updates are to be performed` when there are no changes to it.

The switch `--force` can be used to override this behavior.

Added unit test and performed a few manual tests to verify both bugs are resolved.

Resolves #6216

* Add alias -f for —force

* Add integration test

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
---
 packages/aws-cdk/README.md                    |   5 +
 packages/aws-cdk/bin/cdk.ts                   |   6 +-
 packages/aws-cdk/lib/api/deploy-stack.ts      |  76 +++++++-
 packages/aws-cdk/lib/api/deployment-target.ts |  24 ++-
 packages/aws-cdk/lib/cdk-toolkit.ts           |   9 +-
 .../aws-cdk/test/api/deploy-stack.test.ts     | 168 +++++++++++++++++-
 packages/aws-cdk/test/integ/cli/app/app.js    |   3 +
 .../test/integ/cli/app/nested-stack.js        |  20 +++
 packages/aws-cdk/test/integ/cli/common.bash   |   2 +
 .../test/integ/cli/test-cdk-fast-deploy.sh    |  51 ++++++
 10 files changed, 342 insertions(+), 22 deletions(-)
 create mode 100644 packages/aws-cdk/test/integ/cli/app/nested-stack.js
 create mode 100755 packages/aws-cdk/test/integ/cli/test-cdk-fast-deploy.sh

diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md
index 53f4745256fa5..9145aa5639e42 100644
--- a/packages/aws-cdk/README.md
+++ b/packages/aws-cdk/README.md
@@ -128,6 +128,11 @@ bootstrapped (using `cdk bootstrap`), only stacks that are not using assets and
 $ cdk deploy --app='node bin/main.js' MyStackName
 ```
 
+Before creating a change set, `cdk deploy` will compare the template of the
+currently deployed stack to the template that is about to be deployed and will
+skip deployment if they are identical. Use `--force` to override this behavior
+and always deploy the stack.
+
 #### `cdk destroy`
 Deletes a stack from it's environment. This will cause the resources in the stack to be destroyed (unless they were
 configured with a `DeletionPolicy` of `Retain`). During the stack destruction, the command will output progress
diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts
index 9739c234e98f7..b16b4179fed20 100644
--- a/packages/aws-cdk/bin/cdk.ts
+++ b/packages/aws-cdk/bin/cdk.ts
@@ -67,7 +67,8 @@ async function parseCommandLineArguments() {
       .option('ci', { type: 'boolean', desc: 'Force CI detection (deprecated)', default: process.env.CI !== undefined })
       .option('notification-arns', {type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true})
       .option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE)', nargs: 1, requiresArg: true })
-      .option('execute', {type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true})
+      .option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
+      .option('force', { alias: 'f', type: 'boolean', desc: 'Always deploy stack even if templates are identical', default: false })
     )
     .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs
       .option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only destroy requested stacks, don\'t include dependees' })
@@ -224,7 +225,8 @@ async function initCommandLine() {
           reuseAssets: args['build-exclude'],
           tags: configuration.settings.get(['tags']),
           sdk: aws,
-          execute: args.execute
+          execute: args.execute,
+          force: args.force,
         });
 
       case 'destroy':
diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
index d730ea84eb725..0a2e817d734c3 100644
--- a/packages/aws-cdk/lib/api/deploy-stack.ts
+++ b/packages/aws-cdk/lib/api/deploy-stack.ts
@@ -5,7 +5,7 @@ import * as uuid from 'uuid';
 import { Tag } from "../api/cxapp/stacks";
 import { prepareAssets } from '../assets';
 import { debug, error, print } from '../logging';
-import { toYAML } from '../serialize';
+import { deserializeStructure, toYAML } from '../serialize';
 import { Mode } from './aws-auth/credentials';
 import { ToolkitInfo } from './toolkit-info';
 import { changeSetHasNoChanges, describeStack, stackExists, stackFailedCreating, waitForChangeSet, waitForStack  } from './util/cloudformation';
@@ -54,6 +54,12 @@ export interface DeployStackOptions {
    * @default - no additional parameters will be passed to the template
    */
   parameters?: { [name: string]: string | undefined };
+
+  /**
+   * Deploy even if the deployed template is identical to the one we are about to deploy.
+   * @default false
+   */
+  force?: boolean;
 }
 
 const LARGE_TEMPLATE_SIZE_KB = 50;
@@ -64,6 +70,28 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
     throw new Error(`The stack ${options.stack.displayName} does not have an environment`);
   }
 
+  const cfn = await options.sdk.cloudFormation(options.stack.environment.account, options.stack.environment.region, Mode.ForWriting);
+  const deployName = options.deployName || options.stack.stackName;
+
+  if (!options.force) {
+    debug(`checking if we can skip this stack based on the currently deployed template (use --force to override)`);
+    const deployed = await getDeployedTemplate(cfn, deployName);
+    if (deployed && JSON.stringify(options.stack.template) === JSON.stringify(deployed.template)) {
+      debug(`${deployName}: no change in template, skipping (use --force to override)`);
+      return {
+        noOp: true,
+        outputs: await getStackOutputs(cfn, deployName),
+        stackArn: deployed.stackId,
+        stackArtifact: options.stack
+      };
+    } else {
+      debug(`${deployName}: template changed, deploying...`);
+    }
+  }
+
+  // bail out if the current template is exactly the same as the one we are about to deploy
+  // in cdk-land, this means nothing changed because assets (and therefore nested stacks) are immutable.
+
   const params = await prepareAssets(options.stack, options.toolkitInfo, options.reuseAssets);
 
   // add passed CloudFormation parameters
@@ -76,11 +104,8 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
     }
   }
 
-  const deployName = options.deployName || options.stack.stackName;
-
   const executionId = uuid.v4();
 
-  const cfn = await options.sdk.cloudFormation(options.stack.environment.account, options.stack.environment.region, Mode.ForWriting);
   const bodyParameter = await makeBodyParameter(options.stack, options.toolkitInfo);
 
   if (await stackFailedCreating(cfn, deployName)) {
@@ -212,3 +237,46 @@ export async function destroyStack(options: DestroyStackOptions) {
   }
   return;
 }
+
+async function getDeployedTemplate(cfn: aws.CloudFormation, stackName: string): Promise<{ template: any, stackId: string } | undefined> {
+  const stackId = await getStackId(cfn, stackName);
+  if (!stackId) {
+    return undefined;
+  }
+
+  const template = await readCurrentTemplate(cfn, stackName);
+  return { stackId, template };
+}
+
+export async function readCurrentTemplate(cfn: aws.CloudFormation, stackName: string) {
+  try {
+    const response = await cfn.getTemplate({ StackName: stackName, TemplateStage: 'Original' }).promise();
+    return (response.TemplateBody && deserializeStructure(response.TemplateBody)) || {};
+  } catch (e) {
+    if (e.code === 'ValidationError' && e.message === `Stack with id ${stackName} does not exist`) {
+      return {};
+    } else {
+      throw e;
+    }
+  }
+}
+
+async function getStackId(cfn: aws.CloudFormation, stackName: string): Promise<string | undefined> {
+  try {
+    const stacks = await cfn.describeStacks({ StackName: stackName }).promise();
+    if (!stacks.Stacks) {
+      return undefined;
+    }
+    if (stacks.Stacks.length !== 1) {
+      return undefined;
+    }
+
+    return stacks.Stacks[0].StackId!;
+
+  } catch (e) {
+    if (e.message.includes('does not exist')) {
+      return undefined;
+    }
+    throw e;
+  }
+}
\ No newline at end of file
diff --git a/packages/aws-cdk/lib/api/deployment-target.ts b/packages/aws-cdk/lib/api/deployment-target.ts
index 1c4cd453a4bce..f5c707b63ea6f 100644
--- a/packages/aws-cdk/lib/api/deployment-target.ts
+++ b/packages/aws-cdk/lib/api/deployment-target.ts
@@ -1,9 +1,8 @@
 import { CloudFormationStackArtifact } from '@aws-cdk/cx-api';
 import { Tag } from "../api/cxapp/stacks";
 import { debug } from '../logging';
-import { deserializeStructure } from '../serialize';
 import { Mode } from './aws-auth/credentials';
-import { deployStack, DeployStackResult } from './deploy-stack';
+import { deployStack, DeployStackResult, readCurrentTemplate } from './deploy-stack';
 import { loadToolkitInfo } from './toolkit-info';
 import { ISDK } from './util/sdk';
 
@@ -31,6 +30,12 @@ export interface DeployStackOptions {
   reuseAssets?: string[];
   tags?: Tag[];
   execute?: boolean;
+
+  /**
+   * Force deployment, even if the deployed template is identical to the one we are about to deploy.
+   * @default false deployment will be skipped if the template is identical
+   */
+  force?: boolean;
 }
 
 export interface ProvisionerProps {
@@ -49,18 +54,8 @@ export class CloudFormationDeploymentTarget implements IDeploymentTarget {
 
   public async readCurrentTemplate(stack: CloudFormationStackArtifact): Promise<Template> {
     debug(`Reading existing template for stack ${stack.displayName}.`);
-
     const cfn = await this.aws.cloudFormation(stack.environment.account, stack.environment.region, Mode.ForReading);
-    try {
-      const response = await cfn.getTemplate({ StackName: stack.stackName }).promise();
-      return (response.TemplateBody && deserializeStructure(response.TemplateBody)) || {};
-    } catch (e) {
-      if (e.code === 'ValidationError' && e.message === `Stack with id ${stack.stackName} does not exist`) {
-        return {};
-      } else {
-        throw e;
-      }
-    }
+    return readCurrentTemplate(cfn, stack.stackName);
   }
 
   public async deployStack(options: DeployStackOptions): Promise<DeployStackResult> {
@@ -75,7 +70,8 @@ export class CloudFormationDeploymentTarget implements IDeploymentTarget {
       reuseAssets: options.reuseAssets,
       toolkitInfo,
       tags: options.tags,
-      execute: options.execute
+      execute: options.execute,
+      force: options.force
     });
   }
 }
diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts
index f9a3717b30071..998be5133460a 100644
--- a/packages/aws-cdk/lib/cdk-toolkit.ts
+++ b/packages/aws-cdk/lib/cdk-toolkit.ts
@@ -142,7 +142,8 @@ export class CdkToolkit {
           reuseAssets: options.reuseAssets,
           notificationArns: options.notificationArns,
           tags,
-          execute: options.execute
+          execute: options.execute,
+          force: options.force
         });
 
         const message = result.noOp
@@ -308,6 +309,12 @@ export interface DeployOptions {
    * @default true
    */
   execute?: boolean;
+
+  /**
+   * Always deploy, even if templates are identical.
+   * @default false
+   */
+  force?: boolean;
 }
 
 export interface DestroyOptions {
diff --git a/packages/aws-cdk/test/api/deploy-stack.test.ts b/packages/aws-cdk/test/api/deploy-stack.test.ts
index ab3233358e229..95b12fc384d8e 100644
--- a/packages/aws-cdk/test/api/deploy-stack.test.ts
+++ b/packages/aws-cdk/test/api/deploy-stack.test.ts
@@ -2,9 +2,11 @@ import { deployStack } from '../../lib';
 import { testStack } from '../util';
 import { MockSDK } from '../util/mock-sdk';
 
+const FAKE_TEMPLATE = { resource: 'noerrorresource' };
+
 const FAKE_STACK = testStack({
   stackName: 'withouterrors',
-  template: { resource: 'noerrorresource' },
+  template: FAKE_TEMPLATE,
 });
 
 test('do deploy executable change set with 0 changes', async () => {
@@ -94,3 +96,167 @@ test('correctly passes CFN parameters, ignoring ones with empty values', async (
     { ParameterKey: 'A', ParameterValue: 'A-value' },
   ]);
 });
+
+test('deploy is skipped if template did not change', async () => {
+  const sdk = new MockSDK();
+  let describeStacksInput: AWS.CloudFormation.DescribeStacksInput;
+  let getTemplateInput: AWS.CloudFormation.GetTemplateInput;
+  let createChangeSetCalled = false;
+  let executeChangeSetCalled = false;
+
+  sdk.stubCloudFormation({
+    getTemplate(input) {
+      getTemplateInput = input;
+      return {
+        TemplateBody: JSON.stringify(FAKE_TEMPLATE)
+      };
+    },
+    describeStacks(input) {
+      describeStacksInput = input;
+      return {
+        Stacks: [
+          {
+            StackName: 'mock-stack-name',
+            StackId: 'mock-stack-id',
+            CreationTime: new Date(),
+            StackStatus: 'CREATE_COMPLETE'
+          }
+        ]
+      };
+    },
+    createChangeSet() {
+      createChangeSetCalled = true;
+      return { };
+    },
+    executeChangeSet() {
+      executeChangeSetCalled = true;
+      return { };
+    }
+  });
+
+  await deployStack({
+    stack: FAKE_STACK,
+    sdk
+  });
+
+  expect(createChangeSetCalled).toBeFalsy();
+  expect(executeChangeSetCalled).toBeFalsy();
+  expect(describeStacksInput!).toStrictEqual({ StackName: 'withouterrors' });
+  expect(getTemplateInput!).toStrictEqual({ StackName: 'withouterrors', TemplateStage: 'Original' });
+});
+
+test('deploy not skipped if template did not change and --force is applied', async () => {
+  const sdk = new MockSDK();
+  let describeStacksInput: AWS.CloudFormation.DescribeStacksInput;
+  let getTemplateInput: AWS.CloudFormation.GetTemplateInput;
+  let createChangeSetCalled = false;
+  let executeChangeSetCalled = false;
+  let describeChangeSetCalled = false;
+
+  sdk.stubCloudFormation({
+    getTemplate(input) {
+      getTemplateInput = input;
+      return {
+        TemplateBody: JSON.stringify(FAKE_TEMPLATE)
+      };
+    },
+    describeStacks(input) {
+      describeStacksInput = input;
+      return {
+        Stacks: [
+          {
+            StackName: 'mock-stack-name',
+            StackId: 'mock-stack-id',
+            CreationTime: new Date(),
+            StackStatus: 'CREATE_COMPLETE'
+          }
+        ]
+      };
+    },
+    createChangeSet() {
+      createChangeSetCalled = true;
+      return { };
+    },
+    executeChangeSet() {
+      executeChangeSetCalled = true;
+      return { };
+    },
+    describeChangeSet() {
+      describeChangeSetCalled = true;
+      return {
+        Status: 'CREATE_COMPLETE',
+        Changes: [],
+      };
+    }
+  });
+
+  await deployStack({
+    stack: FAKE_STACK,
+    sdk,
+    force: true
+  });
+
+  expect(createChangeSetCalled).toBeTruthy();
+  expect(executeChangeSetCalled).toBeTruthy();
+  expect(describeChangeSetCalled).toBeTruthy();
+  expect(getTemplateInput!).toBeUndefined();
+  expect(describeStacksInput!).toStrictEqual({ StackName: "withouterrors" });
+});
+
+test('deploy not skipped if template changed', async () => {
+  const sdk = new MockSDK();
+  let describeStacksInput: AWS.CloudFormation.DescribeStacksInput;
+  let getTemplateInput: AWS.CloudFormation.GetTemplateInput;
+  let createChangeSetCalled = false;
+  let executeChangeSetCalled = false;
+  let describeChangeSetCalled = false;
+
+  sdk.stubCloudFormation({
+    getTemplate(input) {
+      getTemplateInput = input;
+      return {
+        TemplateBody: JSON.stringify({ changed: 123 })
+      };
+    },
+    describeStacks(input) {
+      describeStacksInput = input;
+      return {
+        Stacks: [
+          {
+            StackName: 'mock-stack-name',
+            StackId: 'mock-stack-id',
+            CreationTime: new Date(),
+            StackStatus: 'CREATE_COMPLETE'
+          }
+        ]
+      };
+    },
+    createChangeSet() {
+      createChangeSetCalled = true;
+      return { };
+    },
+    executeChangeSet() {
+      executeChangeSetCalled = true;
+      return { };
+    },
+    describeChangeSet() {
+      describeChangeSetCalled = true;
+      return {
+        Status: 'CREATE_COMPLETE',
+        Changes: [],
+      };
+    }
+  });
+
+  await deployStack({
+    stack: FAKE_STACK,
+    sdk,
+    force: true
+  });
+
+  expect(createChangeSetCalled).toBeTruthy();
+  expect(executeChangeSetCalled).toBeTruthy();
+  expect(describeChangeSetCalled).toBeTruthy();
+  expect(getTemplateInput!).toBeUndefined();
+  expect(describeStacksInput!).toStrictEqual({ StackName: "withouterrors" });
+});
\ No newline at end of file
diff --git a/packages/aws-cdk/test/integ/cli/app/app.js b/packages/aws-cdk/test/integ/cli/app/app.js
index f94cbe6717e88..cf11fb0043026 100644
--- a/packages/aws-cdk/test/integ/cli/app/app.js
+++ b/packages/aws-cdk/test/integ/cli/app/app.js
@@ -6,6 +6,7 @@ const iam = require('@aws-cdk/aws-iam');
 const sns = require('@aws-cdk/aws-sns');
 const lambda = require('@aws-cdk/aws-lambda');
 const docker = require('@aws-cdk/aws-ecr-assets');
+const { StackWithNestedStack } = require('./nested-stack');
 
 const stackPrefix = process.env.STACK_NAME_PREFIX || 'cdk-toolkit-integration';
 
@@ -179,4 +180,6 @@ if (process.env.ENABLE_VPC_TESTING) { // Gating so we don't do context fetching
 
 new ConditionalResourceStack(app, `${stackPrefix}-conditional-resource`)
 
+new StackWithNestedStack(app, `${stackPrefix}-with-nested-stack`);
+
 app.synth();
diff --git a/packages/aws-cdk/test/integ/cli/app/nested-stack.js b/packages/aws-cdk/test/integ/cli/app/nested-stack.js
new file mode 100644
index 0000000000000..824818357c51c
--- /dev/null
+++ b/packages/aws-cdk/test/integ/cli/app/nested-stack.js
@@ -0,0 +1,20 @@
+const cfn = require('@aws-cdk/aws-cloudformation');
+const sns = require('@aws-cdk/aws-sns');
+const { Stack } = require('@aws-cdk/core');
+
+class StackWithNestedStack extends Stack {
+  constructor(scope, id) {
+    super(scope, id);
+    new MyNestedStack(this, 'MyNested');
+  }
+}
+
+class MyNestedStack extends cfn.NestedStack {
+  constructor(scope, id) {
+    super(scope, id);
+
+    new sns.Topic(this, 'MyTopic');
+  }
+}
+
+exports.StackWithNestedStack = StackWithNestedStack;
\ No newline at end of file
diff --git a/packages/aws-cdk/test/integ/cli/common.bash b/packages/aws-cdk/test/integ/cli/common.bash
index 60d08d2e187d3..4d7941962cff3 100644
--- a/packages/aws-cdk/test/integ/cli/common.bash
+++ b/packages/aws-cdk/test/integ/cli/common.bash
@@ -78,6 +78,7 @@ function prepare_fixture() {
         @aws-cdk/aws-lambda \
         @aws-cdk/aws-ssm \
         @aws-cdk/aws-ecr-assets \
+        @aws-cdk/aws-cloudformation \
         @aws-cdk/aws-ec2
 
     echo "| setup complete at: $PWD"
@@ -91,6 +92,7 @@ function cleanup() {
   cleanup_stack ${STACK_NAME_PREFIX}-test-1
   cleanup_stack ${STACK_NAME_PREFIX}-test-2
   cleanup_stack ${STACK_NAME_PREFIX}-iam-test
+  cleanup_stack ${STACK_NAME_PREFIX}-with-nested-stack
 }
 
 function setup() {
diff --git a/packages/aws-cdk/test/integ/cli/test-cdk-fast-deploy.sh b/packages/aws-cdk/test/integ/cli/test-cdk-fast-deploy.sh
new file mode 100755
index 0000000000000..940c4ab8cc198
--- /dev/null
+++ b/packages/aws-cdk/test/integ/cli/test-cdk-fast-deploy.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+set -euo pipefail
+scriptdir=$(cd $(dirname $0) && pwd)
+source ${scriptdir}/common.bash
+# ----------------------------------------------------------
+# this test verifies that deployment is skipped when the template did not
+# change, and that `--force` can be used to override this behavior.
+
+setup
+
+# we are using a stack with a nested stack because CFN will always attempt to
+# update a nested stack, which will allow us to verify that updates are actually
+# skipped unless --force is specified.
+stack_name="${STACK_NAME_PREFIX}-with-nested-stack"
+
+get_last_changeset() {
+    aws cloudformation describe-stacks --stack-name ${stack_name} --query 'Stacks[0].ChangeSetId'
+}
+
+# deploy once
+echo "============================================================"
+echo " deploying stack"
+echo "============================================================"
+cdk deploy -v ${stack_name}
+changeset1=$(get_last_changeset)
+echo "changeset1=${changeset1}"
+
+echo "============================================================"
+echo " deploying the same stack again (no change)"
+echo "============================================================"
+cdk deploy -v ${stack_name}
+changeset2=$(get_last_changeset)
+if [ "${changeset2}" != "${changeset1}" ]; then
+    echo "TEST FAILED: expected the 'cdk deploy' will skip deployment because the app did not change"
+    exit 1
+fi
+
+echo "============================================================"
+echo " deploying the same stack again (no change, --force)"
+echo "============================================================"
+cdk deploy --force -v ${stack_name}
+changeset3=$(get_last_changeset)
+if [ "${changeset3}" == "${changeset1}" ]; then
+    echo "TEST FAILED: expected --force to create a new changeset"
+    exit 1
+fi
+
+# destroy
+cdk destroy -f ${stack_name}
+
+echo "✅  success"