Skip to content

Commit

Permalink
fix(cli): deployment stops on AccessDenied looking up bootstrap stack (…
Browse files Browse the repository at this point in the history 
…#26925)

The CLI always looks up the default bootstrap stack, for backwards compatibility reasons: in case the attributes introduced by the V2 `DefaultStackSynthesizer` that tell it what SSM parameter to use and what bucket to write assets to are not present, it needs to fall back to the default bootstrap stack found in CloudFormation.

The code happily survives a `StackNotFound` error, but is not prepared to deal with an `AccessDenied` error, that a customer in #26588 had configured their AWS account for.

The essence of the fix here is to catch all errors when looking up the toolkit stack, because they only become relevant if any of the properties of the toolkit stack are ever accessed. 

The customer also made the point that the lookup didn't even need to happen in the first place, because all information was already there. This is fair, and the organization of the code in this area has been a thorn in my side for a while now. There is some code that doesn't need to be on `ToolkitInfo` (which is the ancient name for the Bootstrap Stack), but is there for legacy reasons.

This PR introduces a refactor, where we introduce a new class `EnvironmentResources`, that manages interacting with the bootstrap resources in a particular environment. We can now pass `EnvironmentResources` everywhere we used to pass `ToolkitInfo`, and the actual lookup of the Bootstrap Stack is only triggered if the need arises (which hopefully should be never).

Closes #26588.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@rix0rrr
rix0rrr authored and Mike Wrighton committed Sep 14, 2023
1 parent 3a3aab2 commit 4a1d31b
Show file tree
Hide file tree
Showing 16 changed files with 460 additions and 343 deletions.
3 changes: 2 additions & 1 deletion packages/aws-cdk/lib/api/bootstrap/deploy-bootstrap.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { BOOTSTRAP_VERSION_OUTPUT, BootstrapEnvironmentOptions, BOOTSTRAP_VERSIO
import * as logging from '../../logging';
import { Mode, SdkProvider, ISDK } from '../aws-auth';
import { deployStack, DeployStackResult } from '../deploy-stack';
import { NoBootstrapStackEnvironmentResources } from '../environment-resources';
import { DEFAULT_TOOLKIT_STACK_NAME, ToolkitInfo } from '../toolkit-info';

/**
Expand Down Expand Up @@ -121,7 +122,7 @@ export class BootstrapStack {
parameters,
usePreviousParameters: options.usePreviousParameters ?? true,
// Obviously we can't need a bootstrap stack to deploy a bootstrap stack
toolkitInfo: ToolkitInfo.bootstraplessDeploymentsOnly(this.sdk),
envResources: new NoBootstrapStackEnvironmentResources(this.resolvedEnvironment, this.sdk),
});
}
}
Expand Down
15 changes: 8 additions & 7 deletions packages/aws-cdk/lib/api/deploy-stack.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import * as chalk from 'chalk';
import * as fs from 'fs-extra';
import * as uuid from 'uuid';
import { ISDK, SdkProvider } from './aws-auth';
import { EnvironmentResources } from './environment-resources';
import { CfnEvaluationException } from './evaluate-cloudformation-template';
import { HotswapMode, ICON } from './hotswap/common';
import { tryHotswapDeployment } from './hotswap-deployments';
import { ToolkitInfo } from './toolkit-info';
import {
changeSetHasNoChanges, CloudFormationStack, TemplateParameters, waitForChangeSet,
waitForStackDeploy, waitForStackDelete, ParameterValues, ParameterChanges, ResourcesToImport,
Expand Down Expand Up @@ -71,7 +71,7 @@ export interface DeployStackOptions {
/**
* Information about the bootstrap stack found in the target environment
*/
readonly toolkitInfo: ToolkitInfo;
readonly envResources: EnvironmentResources;

/**
* Role to pass to CloudFormation to execute the change set
Expand Down Expand Up @@ -262,7 +262,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
// an ad-hoc asset manifest, while passing their locations via template
// parameters.
const legacyAssets = new AssetManifestBuilder();
const assetParams = await addMetadataAssetsToManifest(stackArtifact, legacyAssets, options.toolkitInfo, options.reuseAssets);
const assetParams = await addMetadataAssetsToManifest(stackArtifact, legacyAssets, options.envResources, options.reuseAssets);

const finalParameterValues = { ...options.parameters, ...assetParams };

Expand Down Expand Up @@ -291,7 +291,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
stackArtifact,
options.resolvedEnvironment,
legacyAssets,
options.toolkitInfo,
options.envResources,
options.sdk,
options.overrideTemplate);
await publishAssets(legacyAssets.toManifest(stackArtifact.assembly.directory), options.sdkProvider, stackEnv, {
Expand Down Expand Up @@ -565,7 +565,7 @@ async function makeBodyParameter(
stack: cxapi.CloudFormationStackArtifact,
resolvedEnvironment: cxapi.Environment,
assetManifest: AssetManifestBuilder,
toolkitInfo: ToolkitInfo,
resources: EnvironmentResources,
sdk: ISDK,
overrideTemplate?: any,
): Promise<TemplateBodyParameter> {
Expand All @@ -582,6 +582,7 @@ async function makeBodyParameter(
return { TemplateBody: templateJson };
}

const toolkitInfo = await resources.lookupToolkit();
if (!toolkitInfo.found) {
error(
`The template for stack "${stack.displayName}" is ${Math.round(templateJson.length / 1024)}KiB. ` +
Expand Down Expand Up @@ -623,7 +624,7 @@ async function makeBodyParameter(
export async function makeBodyParameterAndUpload(
stack: cxapi.CloudFormationStackArtifact,
resolvedEnvironment: cxapi.Environment,
toolkitInfo: ToolkitInfo,
resources: EnvironmentResources,
sdkProvider: SdkProvider,
sdk: ISDK,
overrideTemplate?: any): Promise<TemplateBodyParameter> {
Expand All @@ -635,7 +636,7 @@ export async function makeBodyParameterAndUpload(
});

const builder = new AssetManifestBuilder();
const bodyparam = await makeBodyParameter(forceUploadStack, resolvedEnvironment, builder, toolkitInfo, sdk, overrideTemplate);
const bodyparam = await makeBodyParameter(forceUploadStack, resolvedEnvironment, builder, resources, sdk, overrideTemplate);
const manifest = builder.toManifest(stack.assembly.directory);
await publishAssets(manifest, sdkProvider, resolvedEnvironment, { quiet: true });
return bodyparam;
Expand Down
82 changes: 41 additions & 41 deletions packages/aws-cdk/lib/api/deployments.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import { Mode } from './aws-auth/credentials';
import { ISDK } from './aws-auth/sdk';
import { CredentialsOptions, SdkForEnvironment, SdkProvider } from './aws-auth/sdk-provider';
import { deployStack, DeployStackResult, destroyStack, makeBodyParameterAndUpload, DeploymentMethod } from './deploy-stack';
import { EnvironmentResources, EnvironmentResourcesRegistry } from './environment-resources';
import { HotswapMode } from './hotswap/common';
import { loadCurrentTemplateWithNestedStacks, loadCurrentTemplate, flattenNestedStackNames, TemplateWithNestedStackCount } from './nested-stack-helpers';
import { ToolkitInfo } from './toolkit-info';
import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries } from './util/cloudformation';
import { StackActivityProgress } from './util/cloudformation/stack-activity-monitor';
import { replaceEnvPlaceholders } from './util/placeholders';
Expand Down Expand Up @@ -38,6 +38,11 @@ export interface PreparedSdkWithLookupRoleForEnvironment {
* the default credentials (not the assume role credentials)
*/
readonly didAssumeRole: boolean;

/**
* An object for accessing the bootstrap resources in this environment
*/
readonly envResources: EnvironmentResources;
}

export interface DeployStackOptions {
Expand Down Expand Up @@ -256,6 +261,7 @@ export interface StackExistsOptions {

export interface DeploymentsProps {
sdkProvider: SdkProvider;
readonly toolkitStackName?: string;
readonly quiet?: boolean;
}

Expand All @@ -280,6 +286,11 @@ export interface PreparedSdkForEnvironment {
* @default - no execution role is used
*/
readonly cloudFormationRoleArn?: string;

/**
* Access class for environmental resources to help the deployment
*/
readonly envResources: EnvironmentResources;
}

/**
Expand All @@ -289,12 +300,13 @@ export interface PreparedSdkForEnvironment {
*/
export class Deployments {
private readonly sdkProvider: SdkProvider;
private readonly toolkitInfoCache = new Map<string, ToolkitInfo>();
private readonly sdkCache = new Map<string, SdkForEnvironment>();
private readonly publisherCache = new Map<AssetManifest, cdk_assets.AssetPublishing>();
private readonly environmentResources: EnvironmentResourcesRegistry;

constructor(private readonly props: DeploymentsProps) {
this.sdkProvider = props.sdkProvider;
this.environmentResources = new EnvironmentResourcesRegistry(props.toolkitStackName);
}

public async readCurrentTemplateWithNestedStacks(
Expand All @@ -317,21 +329,18 @@ export class Deployments {

public async resourceIdentifierSummaries(
stackArtifact: cxapi.CloudFormationStackArtifact,
toolkitStackName?: string,
): Promise<ResourceIdentifierSummaries> {
debug(`Retrieving template summary for stack ${stackArtifact.displayName}.`);
// Currently, needs to use `deploy-role` since it may need to read templates in the staging
// bucket which have been encrypted with a KMS key (and lookup-role may not read encrypted things)
const { stackSdk, resolvedEnvironment } = await this.prepareSdkFor(stackArtifact, undefined, Mode.ForReading);
const { stackSdk, resolvedEnvironment, envResources } = await this.prepareSdkFor(stackArtifact, undefined, Mode.ForReading);
const cfn = stackSdk.cloudFormation();

const toolkitInfo = await this.lookupToolkit(resolvedEnvironment, stackSdk, toolkitStackName);

// Upload the template, if necessary, before passing it to CFN
const cfnParam = await makeBodyParameterAndUpload(
stackArtifact,
resolvedEnvironment,
toolkitInfo,
envResources,
this.sdkProvider,
stackSdk);

Expand All @@ -355,16 +364,19 @@ export class Deployments {
};
}

const { stackSdk, resolvedEnvironment, cloudFormationRoleArn } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);

const toolkitInfo = await this.lookupToolkit(resolvedEnvironment, stackSdk, options.toolkitStackName);
const {
stackSdk,
resolvedEnvironment,
cloudFormationRoleArn,
envResources,
} = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);

// Do a verification of the bootstrap stack version
await this.validateBootstrapStackVersion(
options.stack.stackName,
options.stack.requiresBootstrapStackVersion,
options.stack.bootstrapStackVersionSsmParameter,
toolkitInfo);
envResources);

return deployStack({
stack: options.stack,
Expand All @@ -376,7 +388,7 @@ export class Deployments {
sdkProvider: this.sdkProvider,
roleArn: cloudFormationRoleArn,
reuseAssets: options.reuseAssets,
toolkitInfo,
envResources,
tags: options.tags,
deploymentMethod,
force: options.force,
Expand Down Expand Up @@ -420,6 +432,7 @@ export class Deployments {
return {
resolvedEnvironment: result.resolvedEnvironment,
stackSdk: result.sdk,
envResources: result.envResources,
};
}
} catch { }
Expand Down Expand Up @@ -464,6 +477,7 @@ export class Deployments {
stackSdk: stackSdk.sdk,
resolvedEnvironment,
cloudFormationRoleArn: arns.cloudFormationRoleArn,
envResources: this.environmentResources.for(resolvedEnvironment, stackSdk.sdk),
};
}

Expand Down Expand Up @@ -504,9 +518,11 @@ export class Deployments {
assumeRoleExternalId: stack.lookupRole?.assumeRoleExternalId,
});

const envResources = this.environmentResources.for(resolvedEnvironment, stackSdk.sdk);

// if we succeed in assuming the lookup role, make sure we have the correct bootstrap stack version
if (stackSdk.didAssumeRole && stack.lookupRole?.bootstrapStackVersionSsmParameter && stack.lookupRole.requiresBootstrapStackVersion) {
const version = await ToolkitInfo.versionFromSsmParameter(stackSdk.sdk, stack.lookupRole.bootstrapStackVersionSsmParameter);
const version = await envResources.versionFromSsmParameter(stack.lookupRole.bootstrapStackVersionSsmParameter);
if (version < stack.lookupRole.requiresBootstrapStackVersion) {
throw new Error(`Bootstrap stack version '${stack.lookupRole.requiresBootstrapStackVersion}' is required, found version '${version}'.`);
}
Expand All @@ -515,7 +531,7 @@ export class Deployments {
} else if (!stackSdk.didAssumeRole && stack.lookupRole?.requiresBootstrapStackVersion) {
warning(upgradeMessage);
}
return { ...stackSdk, resolvedEnvironment };
return { ...stackSdk, resolvedEnvironment, envResources };
} catch (e: any) {
debug(e);
// only print out the warnings if the lookupRole exists AND there is a required
Expand All @@ -528,33 +544,18 @@ export class Deployments {
}
}

/**
* Look up the toolkit for a given environment, using a given SDK
*/
public async lookupToolkit(resolvedEnvironment: cxapi.Environment, sdk: ISDK, toolkitStackName?: string) {
const key = `${resolvedEnvironment.account}:${resolvedEnvironment.region}:${toolkitStackName}`;
const existing = this.toolkitInfoCache.get(key);
if (existing) {
return existing;
}
const ret = await ToolkitInfo.lookup(resolvedEnvironment, sdk, toolkitStackName);
this.toolkitInfoCache.set(key, ret);
return ret;
}

private async prepareAndValidateAssets(asset: cxapi.AssetManifestArtifact, options: AssetOptions) {
const { stackSdk, resolvedEnvironment } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);
const toolkitInfo = await this.lookupToolkit(resolvedEnvironment, stackSdk, options.toolkitStackName);
const stackEnv = await this.sdkProvider.resolveEnvironment(options.stack.environment);
const { envResources } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);

await this.validateBootstrapStackVersion(
options.stack.stackName,
asset.requiresBootstrapStackVersion,
asset.bootstrapStackVersionSsmParameter,
toolkitInfo);
envResources);

const manifest = AssetManifest.fromFile(asset.file);

return { manifest, stackEnv };
return { manifest, stackEnv: envResources.environment };
}

/**
Expand Down Expand Up @@ -582,16 +583,15 @@ export class Deployments {
*/
// eslint-disable-next-line max-len
public async buildSingleAsset(assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry, options: BuildStackAssetsOptions) {
const { stackSdk, resolvedEnvironment: stackEnv } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);
const toolkitInfo = await this.lookupToolkit(stackEnv, stackSdk, options.toolkitStackName);
const { resolvedEnvironment, envResources } = await this.prepareSdkFor(options.stack, options.roleArn, Mode.ForWriting);

await this.validateBootstrapStackVersion(
options.stack.stackName,
assetArtifact.requiresBootstrapStackVersion,
assetArtifact.bootstrapStackVersionSsmParameter,
toolkitInfo);
envResources);

const publisher = this.cachedPublisher(assetManifest, stackEnv, options.stackName);
const publisher = this.cachedPublisher(assetManifest, resolvedEnvironment, options.stackName);
await publisher.buildEntry(asset);
if (publisher.hasFailures) {
throw new Error(`Failed to build asset ${asset.id}`);
Expand Down Expand Up @@ -624,17 +624,17 @@ export class Deployments {

/**
* Validate that the bootstrap stack has the right version for this stack
*
* Call into envResources.validateVersion, but prepend the stack name in case of failure.
*/
private async validateBootstrapStackVersion(
stackName: string,
requiresBootstrapStackVersion: number | undefined,
bootstrapStackVersionSsmParameter: string | undefined,
toolkitInfo: ToolkitInfo) {

if (requiresBootstrapStackVersion === undefined) { return; }
envResources: EnvironmentResources) {

try {
await toolkitInfo.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);
await envResources.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);
} catch (e: any) {
throw new Error(`${stackName}: ${e.message}`);
}
Expand Down
Loading

0 comments on commit 4a1d31b

Please sign in to comment.