fix(core): DefaultSynthesizer deployments are never skipped (#17099)

The `DefaultSynthesizer` always adds an SSM parameter with the bootstrap stack version, so that it can check that the bootstrap stack has a recent enough version. Unfortunately, the CDK CLI will refuse to short-circuit any deployment that uses SSM parameters, because it can't tell if the parameter has changed, and so it has to pessimize. Adding a magic marker to the description of the parameter will now exempt it from the check. Fixes #16959. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Oct 22, 2021 · c74b012 · c74b012
1 parent a0d4ee7
commit c74b012
Show file tree

Hide file tree

Showing 13 changed files with 61 additions and 19 deletions.
diff --git a/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts b/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts
@@ -600,7 +600,7 @@ function addBootstrapVersionRule(stack: Stack, requiredVersion: number, bootstra
 
   const param = new CfnParameter(stack, 'BootstrapVersion', {
     type: 'AWS::SSM::Parameter::Value<String>',
-    description: 'Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store.',
+    description: `Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. ${cxapi.SSMPARAM_NO_INVALIDATE}`,
     default: bootstrapStackVersionSsmParameter,
   });
 

diff --git a/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts b/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts
@@ -70,6 +70,7 @@ describe('new style synthesis', () => {
     const template = app.synth().getStackByName('Stack').template;
     expect(template?.Parameters?.BootstrapVersion?.Type).toEqual('AWS::SSM::Parameter::Value<String>');
     expect(template?.Parameters?.BootstrapVersion?.Default).toEqual('/cdk-bootstrap/hnb659fds/version');
+    expect(template?.Parameters?.BootstrapVersion?.Description).toContain(cxapi.SSMPARAM_NO_INVALIDATE);
 
     const assertions = template?.Rules?.CheckBootstrapVersion?.Assertions ?? [];
     expect(assertions.length).toEqual(1);

diff --git a/packages/@aws-cdk/cx-api/lib/cxapi.ts b/packages/@aws-cdk/cx-api/lib/cxapi.ts
@@ -31,4 +31,13 @@ export const CLI_VERSION_ENV = 'CDK_CLI_VERSION';
 /**
  * If a context value is an object with this key, it indicates an error
  */
-export const PROVIDER_ERROR_KEY = '$providerError';
+export const PROVIDER_ERROR_KEY = '$providerError';
+
+
+/**
+ * This SSM parameter does not invalidate the template
+ *
+ * If this string occurs in the description of an SSM parameter, the CLI
+ * will not assume that the stack must always be redeployed.
+ */
+export const SSMPARAM_NO_INVALIDATE = '[cdk:skip]';
diff --git a/packages/@aws-cdk/pipelines/test/blueprint/stack-deployment.test.ts b/packages/@aws-cdk/pipelines/test/blueprint/stack-deployment.test.ts
@@ -31,7 +31,7 @@ describe('templateUrl', () => {
     const sd = StageDeployment.fromStage(stage);
 
     // THEN
-    expect(sd.stacks[0].templateUrl).toBe('https://cdk-hnb659fds-assets-111-us-east-1.s3.us-east-1.amazonaws.com/4ef627170a212f66f5d1d9240d967ef306f4820ff9cb05b3a7ec703df6af6c3e.json');
+    expect(sd.stacks[0].templateUrl).toBe('https://cdk-hnb659fds-assets-111-us-east-1.s3.us-east-1.amazonaws.com/93ae4de94f81d0905c37db64b7304f5d65233ca4d9581d3a32215743c9bb92dd.json');
   });
 
   test('without region', () => {
@@ -43,7 +43,7 @@ describe('templateUrl', () => {
     const sd = StageDeployment.fromStage(stage);
 
     // THEN
-    expect(sd.stacks[0].templateUrl).toBe('https://cdk-hnb659fds-assets-111-.s3.amazonaws.com/$%7BAWS::Region%7D/4ef627170a212f66f5d1d9240d967ef306f4820ff9cb05b3a7ec703df6af6c3e.json');
+    expect(sd.stacks[0].templateUrl).toBe('https://cdk-hnb659fds-assets-111-.s3.amazonaws.com/$%7BAWS::Region%7D/93ae4de94f81d0905c37db64b7304f5d65233ca4d9581d3a32215743c9bb92dd.json');
   });
 
 });

diff --git a/packages/@aws-cdk/pipelines/test/integ.newpipeline-with-vpc.expected.json b/packages/@aws-cdk/pipelines/test/integ.newpipeline-with-vpc.expected.json
@@ -2368,7 +2368,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/@aws-cdk/pipelines/test/integ.newpipeline.expected.json b/packages/@aws-cdk/pipelines/test/integ.newpipeline.expected.json
@@ -2303,7 +2303,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/@aws-cdk/pipelines/test/integ.pipeline-security.expected.json b/packages/@aws-cdk/pipelines/test/integ.pipeline-security.expected.json
@@ -2366,7 +2366,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/@aws-cdk/pipelines/test/integ.pipeline-with-assets-single-upload.expected.json b/packages/@aws-cdk/pipelines/test/integ.pipeline-with-assets-single-upload.expected.json
@@ -1519,7 +1519,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/@aws-cdk/pipelines/test/integ.pipeline-with-assets.expected.json b/packages/@aws-cdk/pipelines/test/integ.pipeline-with-assets.expected.json
@@ -1576,7 +1576,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/@aws-cdk/pipelines/test/integ.pipeline.expected.json b/packages/@aws-cdk/pipelines/test/integ.pipeline.expected.json
@@ -1299,7 +1299,7 @@
     "BootstrapVersion": {
       "Type": "AWS::SSM::Parameter::Value<String>",
       "Default": "/cdk-bootstrap/hnb659fds/version",
-      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store."
+      "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
     }
   },
   "Rules": {

diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
@@ -14,7 +14,7 @@ import { CfnEvaluationException } from './hotswap/evaluate-cloudformation-templa
 import { ToolkitInfo } from './toolkit-info';
 import {
   changeSetHasNoChanges, CloudFormationStack, TemplateParameters, waitForChangeSet,
-  waitForStackDeploy, waitForStackDelete, ParameterValues,
+  waitForStackDeploy, waitForStackDelete, ParameterValues, ParameterChanges,
 } from './util/cloudformation';
 import { StackActivityMonitor, StackActivityProgress } from './util/cloudformation/stack-activity-monitor';
 
@@ -466,7 +466,7 @@ export async function destroyStack(options: DestroyStackOptions) {
 async function canSkipDeploy(
   deployStackOptions: DeployStackOptions,
   cloudFormationStack: CloudFormationStack,
-  parameterChanges: boolean): Promise<boolean> {
+  parameterChanges: ParameterChanges): Promise<boolean> {
 
   const deployName = deployStackOptions.deployName || deployStackOptions.stack.stackName;
   debug(`${deployName}: checking if we can skip deploy`);
@@ -509,7 +509,11 @@ async function canSkipDeploy(
 
   // Parameters have changed
   if (parameterChanges) {
-    debug(`${deployName}: parameters have changed`);
+    if (parameterChanges === 'ssm') {
+      debug(`${deployName}: some parameters come from SSM so we have to assume they may have changed`);
+    } else {
+      debug(`${deployName}: parameters have changed`);
+    }
     return false;
   }
 

diff --git a/packages/aws-cdk/lib/api/util/cloudformation.ts b/packages/aws-cdk/lib/api/util/cloudformation.ts
@@ -1,3 +1,4 @@
+import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cx-api';
 import { CloudFormation } from 'aws-sdk';
 import { debug } from '../../logging';
 import { deserializeStructure } from '../../serialize';
@@ -11,6 +12,7 @@ export type Template = {
 interface TemplateParameter {
   Type: string;
   Default?: any;
+  Description?: string;
   [key: string]: any;
 }
 
@@ -424,11 +426,12 @@ export class ParameterValues {
   /**
    * Whether this set of parameter updates will change the actual stack values
    */
-  public hasChanges(currentValues: Record<string, string>): boolean {
+  public hasChanges(currentValues: Record<string, string>): ParameterChanges {
     // If any of the parameters are SSM parameters, deploying must always happen
-    // because we can't predict what the values will be.
-    if (Object.values(this.formalParams).some(p => p.Type.startsWith('AWS::SSM::Parameter::'))) {
-      return true;
+    // because we can't predict what the values will be. We will allow some
+    // parameters to opt out of this check by having a magic string in their description.
+    if (Object.values(this.formalParams).some(p => p.Type.startsWith('AWS::SSM::Parameter::') && !p.Description?.includes(SSMPARAM_NO_INVALIDATE))) {
+      return 'ssm';
     }
 
     // Otherwise we're dirty if:
@@ -445,3 +448,5 @@ export class ParameterValues {
     return false;
   }
 }
+
+export type ParameterChanges = boolean | 'ssm';
diff --git a/packages/aws-cdk/test/util/cloudformation.test.ts b/packages/aws-cdk/test/util/cloudformation.test.ts
@@ -1,3 +1,4 @@
+import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cx-api';
 import { CloudFormationStack, TemplateParameters } from '../../lib/api/util/cloudformation';
 import { MockedObject, MockSdkProvider, SyncHandlerSubsetOf } from './mock-sdk';
 
@@ -81,10 +82,32 @@ test('if a parameter is retrieved from SSM, the parameters always count as chang
   const oldValues = { Foo: '/Some/Key' };
 
   // If we don't pass a new value
-  expect(params.updateExisting({}, oldValues).hasChanges(oldValues)).toEqual(true);
+  expect(params.updateExisting({}, oldValues).hasChanges(oldValues)).toEqual('ssm');
 
   // If we do pass a new value but it's the same as the old one
-  expect(params.updateExisting({ Foo: '/Some/Key' }, oldValues).hasChanges(oldValues)).toEqual(true);
+  expect(params.updateExisting({ Foo: '/Some/Key' }, oldValues).hasChanges(oldValues)).toEqual('ssm');
+});
+
+test('if a parameter is retrieved from SSM, the parameters doesnt count as changed if it has the magic marker', () => {
+  const params = TemplateParameters.fromTemplate({
+    Parameters: {
+      Foo: {
+        Type: 'AWS::SSM::Parameter::Name',
+        Default: '/Some/Key',
+        Description: `blabla ${SSMPARAM_NO_INVALIDATE}`,
+      },
+    },
+  });
+  const oldValues = { Foo: '/Some/Key' };
+
+  // If we don't pass a new value
+  expect(params.updateExisting({}, oldValues).hasChanges(oldValues)).toEqual(false);
+
+  // If we do pass a new value but it's the same as the old one
+  expect(params.updateExisting({ Foo: '/Some/Key' }, oldValues).hasChanges(oldValues)).toEqual(false);
+
+  // If we do pass a new value and it's different
+  expect(params.updateExisting({ Foo: '/OTHER/Key' }, oldValues).hasChanges(oldValues)).toEqual(true);
 });
 
 test('empty string is a valid update value', () => {