Skip to content

Commit

Permalink
fix(cli): partition is not being resolved at missing value lookup (aw…
Browse files Browse the repository at this point in the history
…s#15146)

The partition component of the lookup role ARN, when present, was not being resolved. Normally, this resolution is done in the path that handles stacks and assets. The lookup, however, happens before that, so we have to replace the environment placeholders there as well.

Fixes aws#15119
  • Loading branch information
otaviomacedo authored Jun 16, 2021
1 parent 03fca09 commit cc7191e
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 23 deletions.
41 changes: 21 additions & 20 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,25 @@ import { ToolkitInfo } from './toolkit-info';
import { CloudFormationStack, Template } from './util/cloudformation';
import { StackActivityProgress } from './util/cloudformation/stack-activity-monitor';

/**
* Replace the {ACCOUNT} and {REGION} placeholders in all strings found in a complex object.
*/
export async function replaceEnvPlaceholders<A extends { }>(object: A, env: cxapi.Environment, sdkProvider: SdkProvider): Promise<A> {
return cxapi.EnvironmentPlaceholders.replaceAsync(object, {
accountId: () => Promise.resolve(env.account),
region: () => Promise.resolve(env.region),
partition: async () => {
// There's no good way to get the partition!
// We should have had it already, except we don't.
//
// Best we can do is ask the "base credentials" for this environment for their partition. Cross-partition
// AssumeRole'ing will never work anyway, so this answer won't be wrong (it will just be slow!)
return (await sdkProvider.baseCredentialsPartition(env, Mode.ForReading)) ?? 'aws';
},
});
}


export interface DeployStackOptions {
/**
* Stack to deploy
Expand Down Expand Up @@ -223,12 +242,12 @@ export class CloudFormationDeployments {
const resolvedEnvironment = await this.sdkProvider.resolveEnvironment(stack.environment);

// Substitute any placeholders with information about the current environment
const arns = await this.replaceEnvPlaceholders({
const arns = await replaceEnvPlaceholders({
assumeRoleArn: stack.assumeRoleArn,

// Use the override if given, otherwise use the field from the stack
cloudFormationRoleArn: roleArn ?? stack.cloudFormationExecutionRoleArn,
}, resolvedEnvironment);
}, resolvedEnvironment, this.sdkProvider);

const stackSdk = await this.sdkProvider.forEnvironment(resolvedEnvironment, mode, {
assumeRoleArn: arns.assumeRoleArn,
Expand All @@ -241,24 +260,6 @@ export class CloudFormationDeployments {
};
}

/**
* Replace the {ACCOUNT} and {REGION} placeholders in all strings found in a complex object.
*/
private async replaceEnvPlaceholders<A extends { }>(object: A, env: cxapi.Environment): Promise<A> {
return cxapi.EnvironmentPlaceholders.replaceAsync(object, {
accountId: () => Promise.resolve(env.account),
region: () => Promise.resolve(env.region),
partition: async () => {
// There's no good way to get the partition!
// We should have had it already, except we don't.
//
// Best we can do is ask the "base credentials" for this environment for their partition. Cross-partition
// AssumeRole'ing will never work anyway, so this answer won't be wrong (it will just be slow!)
return (await this.sdkProvider.baseCredentialsPartition(env, Mode.ForReading)) ?? 'aws';
},
});
}

/**
* Publish all asset manifests that are referenced by the given stack
*/
Expand Down
5 changes: 4 additions & 1 deletion packages/aws-cdk/lib/api/cxapp/cloud-executable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ export class CloudExecutable {
if (tryLookup) {
debug('Some context information is missing. Fetching...');

await contextproviders.provideContextValues(assembly.manifest.missing, this.props.configuration.context, this.props.sdkProvider);
await contextproviders.provideContextValues(
assembly.manifest.missing,
this.props.configuration.context,
this.props.sdkProvider);

// Cache the new context to disk
await this.props.configuration.saveContext();
Expand Down
12 changes: 10 additions & 2 deletions packages/aws-cdk/lib/context-providers/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { SdkProvider } from '../api';
import { replaceEnvPlaceholders } from '../api/cloudformation-deployments';
import { debug } from '../logging';
import { Context, TRANSIENT_CONTEXT_KEY } from '../settings';
import { AmiContextProviderPlugin } from './ami';
import { AZContextProviderPlugin } from './availability-zones';
import { EndpointServiceAZContextProviderPlugin } from './endpoint-service-availability-zones';
import { HostedZoneContextProviderPlugin } from './hosted-zones';
import { LoadBalancerListenerContextProviderPlugin, LoadBalancerContextProviderPlugin } from './load-balancers';
import { LoadBalancerContextProviderPlugin, LoadBalancerListenerContextProviderPlugin } from './load-balancers';
import { ContextProviderPlugin } from './provider';
import { SecurityGroupContextProviderPlugin } from './security-groups';
import { SSMContextProviderPlugin } from './ssm-parameters';
Expand Down Expand Up @@ -36,7 +37,14 @@ export async function provideContextValues(

let value;
try {
value = await provider.getValue(missingContext.props);
const environment = cxapi.EnvironmentUtils.make(missingContext.props.account, missingContext.props.region);
const resolvedEnvironment = await sdk.resolveEnvironment(environment);

const arns = await replaceEnvPlaceholders({
lookupRoleArn: missingContext.props.lookupRoleArn,
}, resolvedEnvironment, sdk);

value = await provider.getValue({ ...missingContext.props, lookupRoleArn: arns.lookupRoleArn });
} catch (e) {
// Set a specially formatted provider value which will be interpreted
// as a lookup failure in the toolkit.
Expand Down
34 changes: 34 additions & 0 deletions packages/aws-cdk/test/context-providers/generic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,40 @@ test('errors are reported into the context value', async () => {
expect(context.get('asdf').$providerError).toBe('Something went wrong');
});

test('lookup role ARN is resolved', async () => {
// GIVEN
contextproviders.registerContextProvider(TEST_PROVIDER, class {
public async getValue(args: {[key: string]: any}): Promise<any> {
if (args.lookupRoleArn == null) {
throw new Error('No lookupRoleArn');
}

if (args.lookupRoleArn.includes('${AWS::Partition}')) {
throw new Error('Partition not resolved');
}

return 'some resolved value';
}
});
const context = new Context();

// WHEN
await contextproviders.provideContextValues([
{
key: 'asdf',
props: {
account: '1234',
region: 'us-east-1',
lookupRoleArn: 'arn:${AWS::Partition}:iam::280619947791:role/cdk-hnb659fds-lookup-role-280619947791-us-east-1',
},
provider: TEST_PROVIDER,
},
], context, mockSDK);

// THEN - Value gets resolved
expect(context.get('asdf')).toEqual('some resolved value');
});

test('errors are marked transient', async () => {
// GIVEN
contextproviders.registerContextProvider(TEST_PROVIDER, class {
Expand Down
5 changes: 5 additions & 0 deletions packages/aws-cdk/test/util/mock-sdk.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as cxapi from '@aws-cdk/cx-api';
import * as AWS from 'aws-sdk';
import { Account, ISDK, SDK, SdkProvider } from '../../lib/api/aws-auth';
import { Mode } from '../../lib/api/aws-auth/credentials';
import { ToolkitInfo } from '../../lib/api/toolkit-info';
import { CloudFormationStack } from '../../lib/api/util/cloudformation';

Expand Down Expand Up @@ -43,6 +44,10 @@ export class MockSdkProvider extends SdkProvider {
}
}

async baseCredentialsPartition(_environment: cxapi.Environment, _mode: Mode): Promise<string | undefined> {
return undefined;
}

public defaultAccount(): Promise<Account | undefined> {
return Promise.resolve({ accountId: '123456789012', partition: 'aws' });
}
Expand Down

0 comments on commit cc7191e

Please sign in to comment.