Skip to content

Commit cd324d0

Browse files
authored
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*
1 parent 1ab6c4c commit cd324d0

File tree

3 files changed

+62
-42
lines changed

3 files changed

+62
-42
lines changed

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

+29-15
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { deployStack, DeployStackResult, destroyStack, DeploymentMethod } from '
1212
import { EnvironmentResources, EnvironmentResourcesRegistry } from './environment-resources';
1313
import { HotswapMode } from './hotswap/common';
1414
import { loadCurrentTemplateWithNestedStacks, loadCurrentTemplate, RootTemplateWithNestedStacks } from './nested-stack-helpers';
15-
import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries, stabilizeStack } from './util/cloudformation';
15+
import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries, stabilizeStack, uploadStackTemplateAssets } from './util/cloudformation';
1616
import { StackActivityMonitor, StackActivityProgress } from './util/cloudformation/stack-activity-monitor';
1717
import { StackEventPoller } from './util/cloudformation/stack-event-poller';
1818
import { RollbackChoice } from './util/cloudformation/stack-status';
@@ -292,13 +292,6 @@ interface AssetOptions {
292292
*/
293293
readonly stack: cxapi.CloudFormationStackArtifact;
294294

295-
/**
296-
* Name of the toolkit stack, if not the default name.
297-
*
298-
* @default 'CDKToolkit'
299-
*/
300-
readonly toolkitStackName?: string;
301-
302295
/**
303296
* Execution role for the building.
304297
*
@@ -426,14 +419,29 @@ export class Deployments {
426419
const { stackSdk, resolvedEnvironment, envResources } = await this.prepareSdkFor(stackArtifact, undefined, Mode.ForReading);
427420
const cfn = stackSdk.cloudFormation();
428421

422+
await uploadStackTemplateAssets(stackArtifact, this);
423+
429424
// Upload the template, if necessary, before passing it to CFN
425+
const builder = new AssetManifestBuilder();
430426
const cfnParam = await makeBodyParameter(
431427
stackArtifact,
432428
resolvedEnvironment,
433-
new AssetManifestBuilder(),
429+
builder,
434430
envResources,
435431
stackSdk);
436432

433+
// If the `makeBodyParameter` before this added assets, make sure to publish them before
434+
// calling the API.
435+
const addedAssets = builder.toManifest(stackArtifact.assembly.directory);
436+
for (const entry of addedAssets.entries) {
437+
await this.buildSingleAsset('no-version-validation', addedAssets, entry, {
438+
stack: stackArtifact,
439+
});
440+
await this.publishSingleAsset(addedAssets, entry, {
441+
stack: stackArtifact,
442+
});
443+
}
444+
437445
const response = await cfn.getTemplateSummary(cfnParam).promise();
438446
if (!response.ResourceIdentifierSummaries) {
439447
debug('GetTemplateSummary API call did not return "ResourceIdentifierSummaries"');
@@ -805,16 +813,22 @@ export class Deployments {
805813

806814
/**
807815
* Build a single asset from an asset manifest
816+
*
817+
* If an assert manifest artifact is given, the bootstrap stack version
818+
* will be validated according to the constraints in that manifest artifact.
819+
* If that is not necessary, `'no-version-validation'` can be passed.
808820
*/
809821
// eslint-disable-next-line max-len
810-
public async buildSingleAsset(assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry, options: BuildStackAssetsOptions) {
822+
public async buildSingleAsset(assetArtifact: cxapi.AssetManifestArtifact | 'no-version-validation', assetManifest: AssetManifest, asset: IManifestEntry, options: BuildStackAssetsOptions) {
811823
const { resolvedEnvironment, envResources } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);
812824

813-
await this.validateBootstrapStackVersion(
814-
options.stack.stackName,
815-
assetArtifact.requiresBootstrapStackVersion,
816-
assetArtifact.bootstrapStackVersionSsmParameter,
817-
envResources);
825+
if (assetArtifact !== 'no-version-validation') {
826+
await this.validateBootstrapStackVersion(
827+
options.stack.stackName,
828+
assetArtifact.requiresBootstrapStackVersion,
829+
assetArtifact.bootstrapStackVersionSsmParameter,
830+
envResources);
831+
}
818832

819833
const publisher = this.cachedPublisher(assetManifest, resolvedEnvironment, options.stackName);
820834
await publisher.buildEntry(asset);

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

+33-23
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,6 @@ export type PrepareChangeSetOptions = {
305305
sdkProvider: SdkProvider;
306306
stream: NodeJS.WritableStream;
307307
parameters: { [name: string]: string | undefined };
308-
toolkitStackName?: string;
309308
resourcesToImport?: ResourcesToImport;
310309
}
311310

@@ -342,12 +341,14 @@ export async function createDiffChangeSet(options: PrepareChangeSetOptions): Pro
342341
}
343342

344343
/**
345-
* Returns all file entries from an AssetManifestArtifact. This is used in the
346-
* `uploadBodyParameterAndCreateChangeSet` function to find all template asset files to build and publish.
344+
* Returns all file entries from an AssetManifestArtifact that look like templates.
345+
*
346+
* This is used in the `uploadBodyParameterAndCreateChangeSet` function to find
347+
* all template asset files to build and publish.
347348
*
348349
* Returns a tuple of [AssetManifest, FileManifestEntry[]]
349350
*/
350-
function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
351+
function templatesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
351352
const assets: (FileManifestEntry)[] = [];
352353
const fileName = artifact.file;
353354
const assetManifest = AssetManifest.fromFile(fileName);
@@ -365,25 +366,7 @@ function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtif
365366

366367
async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
367368
try {
368-
for (const artifact of options.stack.dependencies) {
369-
// Skip artifact if it is not an Asset Manifest Artifact
370-
if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
371-
continue;
372-
}
373-
374-
// Build and publish each file entry of the Asset Manifest Artifact:
375-
const [assetManifest, file_entries] = fileEntriesFromAssetManifestArtifact(artifact);
376-
for (const entry of file_entries) {
377-
await options.deployments.buildSingleAsset(artifact, assetManifest, entry, {
378-
stack: options.stack,
379-
toolkitStackName: options.toolkitStackName,
380-
});
381-
await options.deployments.publishSingleAsset(assetManifest, entry, {
382-
stack: options.stack,
383-
toolkitStackName: options.toolkitStackName,
384-
});
385-
}
386-
}
369+
await uploadStackTemplateAssets(options.stack, options.deployments);
387370
const preparedSdk = (await options.deployments.prepareSdkWithDeployRole(options.stack));
388371

389372
const bodyParameter = await makeBodyParameter(
@@ -419,6 +402,33 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
419402
}
420403
}
421404

405+
/**
406+
* Uploads the assets that look like templates for this CloudFormation stack
407+
*
408+
* This is necessary for any CloudFormation call that needs the template, it may need
409+
* to be uploaded to an S3 bucket first. We have to follow the instructions in the
410+
* asset manifest, because technically that is the only place that knows about
411+
* bucket and assumed roles and such.
412+
*/
413+
export async function uploadStackTemplateAssets(stack: cxapi.CloudFormationStackArtifact, deployments: Deployments) {
414+
for (const artifact of stack.dependencies) {
415+
// Skip artifact if it is not an Asset Manifest Artifact
416+
if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
417+
continue;
418+
}
419+
420+
const [assetManifest, file_entries] = templatesFromAssetManifestArtifact(artifact);
421+
for (const entry of file_entries) {
422+
await deployments.buildSingleAsset(artifact, assetManifest, entry, {
423+
stack,
424+
});
425+
await deployments.publishSingleAsset(assetManifest, entry, {
426+
stack,
427+
});
428+
}
429+
}
430+
}
431+
422432
async function createChangeSet(options: CreateChangeSetOptions): Promise<DescribeChangeSetOutput> {
423433
await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);
424434

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

-4
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,6 @@ export class CdkToolkit {
186186
parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]),
187187
resourcesToImport,
188188
stream,
189-
toolkitStackName: options.toolkitStackName,
190189
});
191190
} else {
192191
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 {
246245
await this.props.deployments.buildSingleAsset(assetNode.assetManifestArtifact, assetNode.assetManifest, assetNode.asset, {
247246
stack: assetNode.parentStack,
248247
roleArn: options.roleArn,
249-
toolkitStackName: options.toolkitStackName,
250248
stackName: assetNode.parentStack.stackName,
251249
});
252250
};
@@ -255,7 +253,6 @@ export class CdkToolkit {
255253
await this.props.deployments.publishSingleAsset(assetNode.assetManifest, assetNode.asset, {
256254
stack: assetNode.parentStack,
257255
roleArn: options.roleArn,
258-
toolkitStackName: options.toolkitStackName,
259256
stackName: assetNode.parentStack.stackName,
260257
});
261258
};
@@ -1030,7 +1027,6 @@ export class CdkToolkit {
10301027
await graph.removeUnnecessaryAssets(assetNode => this.props.deployments.isSingleAssetPublished(assetNode.assetManifest, assetNode.asset, {
10311028
stack: assetNode.parentStack,
10321029
roleArn: options.roleArn,
1033-
toolkitStackName: options.toolkitStackName,
10341030
stackName: assetNode.parentStack.stackName,
10351031
}));
10361032
}

0 commit comments

Comments
 (0)