search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.67k stars 3.92k forks source link

(cli): the CLI still validates the bootstrap stack's version even when the asset producer says "don't" #21324

Closed huyphan closed 2 years ago

huyphan commented 2 years ago

Describe the bug

We have a setup that we don't rely on a bootstrap stack at all (i.e. we don't use CDKToolkit or any custom toolkit stack). We specifically omit the property requiresBootstrapStackVersion in the asset manifest. But when running cdk deploy <stack-name>, we saw that CDK still tries to validate the bootstrap stack and subsequently fails the deployment.

This behaviour is caused by the AssetManifestArtifact class where requiresBootstrapStackVersion is a required property of type number. Even if the custom synthesizer returns undefined (or null) for this property, AssetManifestArtifact will override it to 1. This causes the bootstrap stack version validation to happen in other operations.

This is only an issue with asset manifest artifact, but not with cloudformation artifact.

Expected Behavior

cdk deploy <stackname> should work without the need to validate the bootstrap stack.

Current Behavior

cdk deploy <stackname> command fails with the following error:

 ❌  MyStack-alpha failed: Error: MyStack-alpha: This deployment requires a bootstrap stack with a known name; pass '--toolkit-stack-name' or switch to using the 'DefaultStackSynthesizer' (see https://docs.aws.amazon.com/cdk/latest/guide/bootstrapping.html)
    at CloudFormationDeployments.validateBootstrapStackVersion (/some/workspace/node_modules/aws-cdk/lib/index.js:887147:17)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at CloudFormationDeployments.publishStackAssets (/some/workspace/node_modules/aws-cdk/lib/api/cloudformation-deployments.ts:466:3)
    at CloudFormationDeployments.deployStack (/some/workspace/node_modules/aws-cdk/lib/api/cloudformation-deployments.ts:349:5)
    at CdkToolkit.deploy (/some/workspace/node_modules/aws-cdk/lib/cdk-toolkit.ts:218:11)
    at initCommandLine (/some/workspace/node_modules/aws-cdk/lib/cli.ts:351:10)

MyStack-alpha: This deployment requires a bootstrap stack with a known name; pass '--toolkit-stack-name' or switch to using the 'DefaultStackSynthesizer' (see https://docs.aws.amazon.com/cdk/latest/guide/bootstrapping.html)

Reproduction Steps

We have a custom synthesizer that's rather large to copy-paste here, but here's an example of the asset manifest that our synthesizer produces: 

# file: cdk.out/manifest.json

...
    "<stack-name>.assets": {
      "type": "cdk:asset-manifest",
      "properties": {
        "file": "<stack-name>.assets.json"
      }
    },
...

# file: cdk.out/<stack-name>.asset.json

{
  "version": "17.0.0",
  "files": {
    "20857a268eb30cdd4dce71c5558c8713b5697fe09b8593290d03b2836a8ae7af/artifact": {
      "source": {
        "executable": [
          "some", "command"
        ]
      },
      "destinations": {
        "development-foobar-us-east-1": {
          "bucketName": "some-bucket-name",
          "objectKey": "some-unique-hash/artifact",
          "region": "us-east-1"
        }
      }
    }
  },
  "dockerImages": {}
}

Notice that the manifests completely omit the requiresBootstrapStackVersion and bootstrapStackVersionSsmParameter properties.

Possible Solution

There are many ways to address this, two options that I can think of:

Option 1: Updating the constructor of AssetManifestArtifact to allow requireBootstrapStackVersion to be undefined:

This change makes the behaviour of AssetManifestArtifact consistent with CloudFormationStackArtifact: 

❯ git diff packages
diff --git a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
index 10ddea69b..67f12efc0 100644
--- a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
+++ b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
@@ -38,7 +38,7 @@ export class AssetManifestArtifact extends CloudArtifact {
   /**
    * Version of bootstrap stack required to deploy this stack
    */
-  public readonly requiresBootstrapStackVersion: number;
+  public readonly requiresBootstrapStackVersion?: number;

   /**
    * Name of SSM parameter with bootstrap stack version
@@ -55,7 +55,7 @@ export class AssetManifestArtifact extends CloudArtifact {
       throw new Error('Invalid AssetManifestArtifact. Missing "file" property');
     }
     this.file = path.resolve(this.assembly.directory, properties.file);
-    this.requiresBootstrapStackVersion = properties.requiresBootstrapStackVersion ?? 1;
+    this.requiresBootstrapStackVersion = properties.requiresBootstrapStackVersion;
     this.bootstrapStackVersionSsmParameter = properties.bootstrapStackVersionSsmParameter;
   }
 }

This change is technically backward incompatible. It will break those who write their own synthesizers, omit the requiresBootstrapStackVersion property but still require a bootstrap stack behind the scene. If that use case exists, it's probably a wrong usage in the first place.

Option 2: Extending requireBootstrapStackVersion field to accept boolean values and relax the check on that field when deploying:

❯ git diff packages
diff --git a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
index 10ddea69b..ee35fcba9 100644
--- a/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
+++ b/packages/@aws-cdk/cx-api/lib/artifacts/asset-manifest-artifact.ts
@@ -38,7 +38,7 @@ export class AssetManifestArtifact extends CloudArtifact {
   /**
    * Version of bootstrap stack required to deploy this stack
    */
-  public readonly requiresBootstrapStackVersion: number;
+  public readonly requiresBootstrapStackVersion: number | boolean;

   /**
    * Name of SSM parameter with bootstrap stack version
diff --git a/packages/aws-cdk/lib/api/cloudformation-deployments.ts b/packages/aws-cdk/lib/api/cloudformation-deployments.ts
index ce81ab8d7..d02f38684 100644
--- a/packages/aws-cdk/lib/api/cloudformation-deployments.ts
+++ b/packages/aws-cdk/lib/api/cloudformation-deployments.ts
@@ -475,11 +475,11 @@ export class CloudFormationDeployments {
    */
   private async validateBootstrapStackVersion(
     stackName: string,
-    requiresBootstrapStackVersion: number | undefined,
+    requiresBootstrapStackVersion: number | boolean | undefined,
     bootstrapStackVersionSsmParameter: string | undefined,
     toolkitInfo: ToolkitInfo) {

-    if (requiresBootstrapStackVersion === undefined) { return; }
+    if (requiresBootstrapStackVersion === undefined || requiresBootstrapStackVersion === false) { return; }

     try {
       await toolkitInfo.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);

Option 3: Using 0 as a special value of requireBootstrapStackVersion :

❯ git diff packages
diff --git a/packages/aws-cdk/lib/api/cloudformation-deployments.ts b/packages/aws-cdk/lib/api/cloudformation-deployments.ts
index ce81ab8d7..d02f38684 100644
--- a/packages/aws-cdk/lib/api/cloudformation-deployments.ts
+++ b/packages/aws-cdk/lib/api/cloudformation-deployments.ts
@@ -475,11 +475,11 @@ export class CloudFormationDeployments {
    */
   private async validateBootstrapStackVersion(
     stackName: string,
     requiresBootstrapStackVersion: number | undefined,
     bootstrapStackVersionSsmParameter: string | undefined,
     toolkitInfo: ToolkitInfo) {

-    if (requiresBootstrapStackVersion === undefined) { return; }
+    if (requiresBootstrapStackVersion === undefined || requiresBootstrapStackVersion === 0) { return; }

     try {
       await toolkitInfo.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);

This is also a backward incompatible change. Version 0 implicitly represents the legacy bootstrap stack. When requireBootstrapStackVersion is set to 0, the current behaviour is to still check for the existence of the CDKToolkit stack, but do not care what version it is. With this code change, we will remove the checks entirely.

Additional Information/Context

No response

CDK CLI Version

5.33.0 (build 859272d)

Framework Version

No response

Node.js Version

v16.14.2

OS

Darwin Kernel Version 21.6.0

Language

Typescript

Language Version

No response

Other information

No response

github-actions[bot] commented 2 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.