fix(cli): cdk import errors with 'S3 error: Access Denied' (#31727)

In #31597 we changed `cdk diff` to always use the file asset publishing role, instead of direct CLI credentials. This included a refactor that impacted `cdk import`, which was now not uploading the stack template at all anymore. The operation that is now broken only happens in a case with interactive input, which is why this wasn't caught by integ tests.

In this change, put the requisite asset-handling code around `makeBodyParameter` to make the asset uploading happen properly.

In future PRs:

- Add an integration test for `cdk import` which would have exposed the same error.
- Refactor the contract of `makeBodyParameter`, and perhaps more around asset uploading, to make the expectations and promises of that function more clear; right now it was not obvious what the function would and wouldn't do for you, which led to this error.

I did some refactorings in this PR already (renames, removing an unused argument). I saw an opportunity for more but didn't want to add risk and delay to this patch. Hence, forthcoming 😄 .

Closes #31716.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/aws-cdk/lib/api/deployments.ts

@@ -12,7 +12,7 @@ import { deployStack, DeployStackResult, destroyStack, DeploymentMethod } from '
 import { EnvironmentResources, EnvironmentResourcesRegistry } from './environment-resources';
 import { HotswapMode } from './hotswap/common';
 import { loadCurrentTemplateWithNestedStacks, loadCurrentTemplate, RootTemplateWithNestedStacks } from './nested-stack-helpers';
-import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries, stabilizeStack } from './util/cloudformation';
+import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries, stabilizeStack, uploadStackTemplateAssets } from './util/cloudformation';
 import { StackActivityMonitor, StackActivityProgress } from './util/cloudformation/stack-activity-monitor';
 import { StackEventPoller } from './util/cloudformation/stack-event-poller';
 import { RollbackChoice } from './util/cloudformation/stack-status';
@@ -292,13 +292,6 @@ interface AssetOptions {
    */
   readonly stack: cxapi.CloudFormationStackArtifact;
 
-  /**
-   * Name of the toolkit stack, if not the default name.
-   *
-   * @default 'CDKToolkit'
-   */
-  readonly toolkitStackName?: string;
-
   /**
    * Execution role for the building.
    *
@@ -426,14 +419,29 @@ export class Deployments {
     const { stackSdk, resolvedEnvironment, envResources } = await this.prepareSdkFor(stackArtifact, undefined, Mode.ForReading);
     const cfn = stackSdk.cloudFormation();
 
+    await uploadStackTemplateAssets(stackArtifact, this);
+
     // Upload the template, if necessary, before passing it to CFN
+    const builder = new AssetManifestBuilder();
     const cfnParam = await makeBodyParameter(
       stackArtifact,
       resolvedEnvironment,
-      new AssetManifestBuilder(),
+      builder,
       envResources,
       stackSdk);
 
+    // If the `makeBodyParameter` before this added assets, make sure to publish them before
+    // calling the API.
+    const addedAssets = builder.toManifest(stackArtifact.assembly.directory);
+    for (const entry of addedAssets.entries) {
+      await this.buildSingleAsset('no-version-validation', addedAssets, entry, {
+        stack: stackArtifact,
+      });
+      await this.publishSingleAsset(addedAssets, entry, {
+        stack: stackArtifact,
+      });
+    }
+
     const response = await cfn.getTemplateSummary(cfnParam).promise();
     if (!response.ResourceIdentifierSummaries) {
       debug('GetTemplateSummary API call did not return "ResourceIdentifierSummaries"');
@@ -805,16 +813,22 @@ export class Deployments {
 
   /**
    * Build a single asset from an asset manifest
+   *
+   * If an assert manifest artifact is given, the bootstrap stack version
+   * will be validated according to the constraints in that manifest artifact.
+   * If that is not necessary, `'no-version-validation'` can be passed.
    */
   // eslint-disable-next-line max-len
-  public async buildSingleAsset(assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry, options: BuildStackAssetsOptions) {
+  public async buildSingleAsset(assetArtifact: cxapi.AssetManifestArtifact | 'no-version-validation', assetManifest: AssetManifest, asset: IManifestEntry, options: BuildStackAssetsOptions) {
     const { resolvedEnvironment, envResources } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);
 
-    await this.validateBootstrapStackVersion(
-      options.stack.stackName,
-      assetArtifact.requiresBootstrapStackVersion,
-      assetArtifact.bootstrapStackVersionSsmParameter,
-      envResources);
+    if (assetArtifact !== 'no-version-validation') {
+      await this.validateBootstrapStackVersion(
+        options.stack.stackName,
+        assetArtifact.requiresBootstrapStackVersion,
+        assetArtifact.bootstrapStackVersionSsmParameter,
+        envResources);
+    }
 
     const publisher = this.cachedPublisher(assetManifest, resolvedEnvironment, options.stackName);
     await publisher.buildEntry(asset);

packages/aws-cdk/lib/api/util/cloudformation.ts

@@ -305,7 +305,6 @@ export type PrepareChangeSetOptions = {
   sdkProvider: SdkProvider;
   stream: NodeJS.WritableStream;
   parameters: { [name: string]: string | undefined };
-  toolkitStackName?: string;
   resourcesToImport?: ResourcesToImport;
 }
 
@@ -342,12 +341,14 @@ export async function createDiffChangeSet(options: PrepareChangeSetOptions): Pro
 }
 
 /**
- * Returns all file entries from an AssetManifestArtifact. This is used in the
- * `uploadBodyParameterAndCreateChangeSet` function to find all template asset files to build and publish.
+ * Returns all file entries from an AssetManifestArtifact that look like templates.
+ *
+ * This is used in the `uploadBodyParameterAndCreateChangeSet` function to find
+ * all template asset files to build and publish.
  *
  * Returns a tuple of [AssetManifest, FileManifestEntry[]]
  */
-function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
+function templatesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
   const assets: (FileManifestEntry)[] = [];
   const fileName = artifact.file;
   const assetManifest = AssetManifest.fromFile(fileName);
@@ -365,25 +366,7 @@ function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtif
 
 async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
   try {
-    for (const artifact of options.stack.dependencies) {
-      // Skip artifact if it is not an Asset Manifest Artifact
-      if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
-        continue;
-      }
-
-      // Build and publish each file entry of the Asset Manifest Artifact:
-      const [assetManifest, file_entries] = fileEntriesFromAssetManifestArtifact(artifact);
-      for (const entry of file_entries) {
-        await options.deployments.buildSingleAsset(artifact, assetManifest, entry, {
-          stack: options.stack,
-          toolkitStackName: options.toolkitStackName,
-        });
-        await options.deployments.publishSingleAsset(assetManifest, entry, {
-          stack: options.stack,
-          toolkitStackName: options.toolkitStackName,
-        });
-      }
-    }
+    await uploadStackTemplateAssets(options.stack, options.deployments);
     const preparedSdk = (await options.deployments.prepareSdkWithDeployRole(options.stack));
 
     const bodyParameter = await makeBodyParameter(
@@ -419,6 +402,33 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
   }
 }
 
+/**
+ * Uploads the assets that look like templates for this CloudFormation stack
+ *
+ * This is necessary for any CloudFormation call that needs the template, it may need
+ * to be uploaded to an S3 bucket first. We have to follow the instructions in the
+ * asset manifest, because technically that is the only place that knows about
+ * bucket and assumed roles and such.
+ */
+export async function uploadStackTemplateAssets(stack: cxapi.CloudFormationStackArtifact, deployments: Deployments) {
+  for (const artifact of stack.dependencies) {
+    // Skip artifact if it is not an Asset Manifest Artifact
+    if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
+      continue;
+    }
+
+    const [assetManifest, file_entries] = templatesFromAssetManifestArtifact(artifact);
+    for (const entry of file_entries) {
+      await deployments.buildSingleAsset(artifact, assetManifest, entry, {
+        stack,
+      });
+      await deployments.publishSingleAsset(assetManifest, entry, {
+        stack,
+      });
+    }
+  }
+}
+
 async function createChangeSet(options: CreateChangeSetOptions): Promise<DescribeChangeSetOutput> {
   await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);
 

packages/aws-cdk/lib/cdk-toolkit.ts

@@ -186,7 +186,6 @@ export class CdkToolkit {
               parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]),
               resourcesToImport,
               stream,
-              toolkitStackName: options.toolkitStackName,
             });
           } else {
             debug(`the stack '${stack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`);
@@ -246,7 +245,6 @@ export class CdkToolkit {
       await this.props.deployments.buildSingleAsset(assetNode.assetManifestArtifact, assetNode.assetManifest, assetNode.asset, {
         stack: assetNode.parentStack,
         roleArn: options.roleArn,
-        toolkitStackName: options.toolkitStackName,
         stackName: assetNode.parentStack.stackName,
       });
     };
@@ -255,7 +253,6 @@ export class CdkToolkit {
       await this.props.deployments.publishSingleAsset(assetNode.assetManifest, assetNode.asset, {
         stack: assetNode.parentStack,
         roleArn: options.roleArn,
-        toolkitStackName: options.toolkitStackName,
         stackName: assetNode.parentStack.stackName,
       });
     };
@@ -1030,7 +1027,6 @@ export class CdkToolkit {
     await graph.removeUnnecessaryAssets(assetNode => this.props.deployments.isSingleAssetPublished(assetNode.assetManifest, assetNode.asset, {
       stack: assetNode.parentStack,
       roleArn: options.roleArn,
-      toolkitStackName: options.toolkitStackName,
       stackName: assetNode.parentStack.stackName,
     }));
   }