Skip to content

Commit f1db8df

Browse files
authored
fix(integ-runner): region is not passed to CDK App (#949)
When using the `toolkit-lib` deployment engine, the selected region was not being passed to the CDK app. In the old engine, we could set the `AWS_REGION` environment variable and the CDK CLI would respect that, but the toolkit lib was passing that environment variable directly to the CDK app which was not respecting that. Fix that by threading the region through as an explicit parameter and adding an override parameter to the `AwsCliCompatible` base credentials: we don't necessarily want to read this only from the INI files. Additionally: add a validation to make sure that the CDK app respects the region we are requesting. CDK integ tests should either not pass an environment, or read `$CDK_DEFAULT_REGION` to determine the current region. It was too easy to just override the region with a hardcoded one, but that breaks the integ runner's attempts to isolate tests across regions. Guard against that by validating. Also in this PR: disable the update workflow. I've lost 2 hours debugging why my deployment was failing with nonsenical values and the answer was: the checked-in snapshot was nonsensical, and the update workflow will first deploy the checked-in snapshot and only then deploy the current snapshot. Good idea, bad execution: if existing snapshots are not region-agnostic they are guaranteed to fail. We should probably resynthesize them from the previous git commit instead. Whatever it is, what we do right now doesn't work so I'm disabling it. Fixes # --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
1 parent 29a6178 commit f1db8df

File tree

8 files changed

+120
-21
lines changed

8 files changed

+120
-21
lines changed

packages/@aws-cdk/integ-runner/lib/engines/toolkit-lib.ts

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import * as path from 'node:path';
22
import type { DeployOptions, ICdk, ListOptions, SynthFastOptions, SynthOptions, WatchEvents } from '@aws-cdk/cdk-cli-wrapper';
33
import type { DefaultCdkOptions, DestroyOptions } from '@aws-cdk/cloud-assembly-schema/lib/integ-tests';
4-
import type { DeploymentMethod, ICloudAssemblySource, IIoHost, IoMessage, IoRequest, NonInteractiveIoHostProps, StackSelector } from '@aws-cdk/toolkit-lib';
5-
import { ExpandStackSelection, MemoryContext, NonInteractiveIoHost, StackSelectionStrategy, Toolkit } from '@aws-cdk/toolkit-lib';
4+
import { UNKNOWN_REGION } from '@aws-cdk/cx-api';
5+
import type { DeploymentMethod, ICloudAssemblySource, IIoHost, IoMessage, IoRequest, IReadableCloudAssembly, NonInteractiveIoHostProps, StackSelector } from '@aws-cdk/toolkit-lib';
6+
import { BaseCredentials, ExpandStackSelection, MemoryContext, NonInteractiveIoHost, StackSelectionStrategy, Toolkit } from '@aws-cdk/toolkit-lib';
67
import * as chalk from 'chalk';
78
import * as fs from 'fs-extra';
89

@@ -27,6 +28,11 @@ export interface ToolkitLibEngineOptions {
2728
* @default false
2829
*/
2930
readonly showOutput?: boolean;
31+
32+
/**
33+
* The region the CDK app should synthesize itself for
34+
*/
35+
readonly region: string;
3036
}
3137

3238
/**
@@ -36,13 +42,23 @@ export class ToolkitLibRunnerEngine implements ICdk {
3642
private readonly toolkit: Toolkit;
3743
private readonly options: ToolkitLibEngineOptions;
3844
private readonly showOutput: boolean;
45+
private readonly ioHost: IntegRunnerIoHost;
3946

4047
public constructor(options: ToolkitLibEngineOptions) {
4148
this.options = options;
4249
this.showOutput = options.showOutput ?? false;
4350

51+
// We always create this for ourselves to emit warnings, but potentially
52+
// don't pass it to the toolkit.
53+
this.ioHost = new IntegRunnerIoHost();
54+
4455
this.toolkit = new Toolkit({
45-
ioHost: this.showOutput? new IntegRunnerIoHost() : new NoopIoHost(),
56+
ioHost: this.showOutput ? this.ioHost : new NoopIoHost(),
57+
sdkConfig: {
58+
baseCredentials: BaseCredentials.awsCliCompatible({
59+
defaultRegion: options.region,
60+
}),
61+
},
4662
// @TODO - these options are currently available on the action calls
4763
// but toolkit-lib needs them at the constructor level.
4864
// Need to decide what to do with them.
@@ -73,6 +89,7 @@ export class ToolkitLibRunnerEngine implements ICdk {
7389
stacks: this.stackSelector(options),
7490
validateStacks: options.validation,
7591
});
92+
await this.validateRegion(lock);
7693
await lock.dispose();
7794
}
7895

@@ -100,6 +117,7 @@ export class ToolkitLibRunnerEngine implements ICdk {
100117
try {
101118
// @TODO - use produce to mimic the current behavior more closely
102119
const lock = await cx.produce();
120+
await this.validateRegion(lock);
103121
await lock.dispose();
104122
// We should fix this once we have stabilized toolkit-lib as engine.
105123
// What we really should do is this:
@@ -217,7 +235,6 @@ export class ToolkitLibRunnerEngine implements ICdk {
217235
workingDirectory: this.options.workingDirectory,
218236
outdir,
219237
lookups: options.lookups,
220-
resolveDefaultEnvironment: false, // not part of the integ-runner contract
221238
contextStore: new MemoryContext(options.context),
222239
env: this.options.env,
223240
synthOptions: {
@@ -256,6 +273,53 @@ export class ToolkitLibRunnerEngine implements ICdk {
256273
method: options.deploymentMethod ?? 'change-set',
257274
};
258275
}
276+
277+
/**
278+
* Check that the regions for the stacks in the CloudAssembly match the regions requested on the engine
279+
*
280+
* This prevents misconfiguration of the integ test app. People tend to put:
281+
*
282+
* ```ts
283+
* new Stack(app, 'Stack', {
284+
* env: {
285+
* region: 'some-region-that-suits-me',
286+
* }
287+
* });
288+
* ```
289+
*
290+
* Into their integ tests, instead of:
291+
*
292+
* ```ts
293+
* {
294+
* region: process.env.CDK_DEFAULT_REGION,
295+
* }
296+
* ```
297+
*
298+
* This catches that misconfiguration.
299+
*/
300+
private async validateRegion(asm: IReadableCloudAssembly) {
301+
for (const stack of asm.cloudAssembly.stacksRecursively) {
302+
if (stack.environment.region !== this.options.region && stack.environment.region !== UNKNOWN_REGION) {
303+
this.ioHost.notify({
304+
action: 'deploy',
305+
code: 'CDK_RUNNER_W0000',
306+
time: new Date(),
307+
level: 'warn',
308+
message: `Stack ${stack.displayName} synthesizes for region ${stack.environment.region}, even though ${this.options.region} was requested. Please configure \`{ env: { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT } }\`, or use no env at all. Do not hardcode a region or account.`,
309+
data: {
310+
stackName: stack.displayName,
311+
stackRegion: stack.environment.region,
312+
requestedRegion: this.options.region,
313+
},
314+
}).catch((e) => {
315+
if (e) {
316+
// eslint-disable-next-line no-console
317+
console.error(e);
318+
}
319+
});
320+
}
321+
}
322+
}
259323
}
260324

261325
/**
@@ -269,9 +333,16 @@ class IntegRunnerIoHost extends NonInteractiveIoHost {
269333
});
270334
}
271335
public async notify(msg: IoMessage<unknown>): Promise<void> {
336+
let color;
337+
switch (msg.level) {
338+
case 'error': color = chalk.red; break;
339+
case 'warn': color = chalk.yellow; break;
340+
default: color = chalk.gray;
341+
}
342+
272343
return super.notify({
273344
...msg,
274-
message: chalk.gray(msg.message),
345+
message: color(msg.message),
275346
});
276347
}
277348
}

packages/@aws-cdk/integ-runner/lib/runner/engine.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,8 @@ export function makeEngine(options: IntegRunnerOptions): ICdk {
1818
return new ToolkitLibRunnerEngine({
1919
workingDirectory: options.test.directory,
2020
showOutput: options.showOutput,
21-
env: {
22-
...options.env,
23-
},
21+
env: options.env,
22+
region: options.region,
2423
});
2524
case 'cli-wrapper':
2625
default:
@@ -29,6 +28,8 @@ export function makeEngine(options: IntegRunnerOptions): ICdk {
2928
showOutput: options.showOutput,
3029
env: {
3130
...options.env,
31+
// The CDK CLI will interpret this and use it usefully
32+
AWS_REGION: options.region,
3233
},
3334
});
3435
}

packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ export interface IntegRunnerOptions extends EngineOptions {
2626
*/
2727
readonly test: IntegTest;
2828

29+
/**
30+
* The region where the test should be deployed
31+
*/
32+
readonly region: string;
33+
2934
/**
3035
* The AWS profile to use when invoking the CDK CLI
3136
*

packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,11 @@ interface SnapshotAssembly {
3434
* the validation of the integration test snapshots
3535
*/
3636
export class IntegSnapshotRunner extends IntegRunner {
37-
constructor(options: IntegRunnerOptions) {
38-
super(options);
37+
constructor(options: Omit<IntegRunnerOptions, 'region'>) {
38+
super({
39+
...options,
40+
region: 'unused',
41+
});
3942
}
4043

4144
/**

packages/@aws-cdk/integ-runner/lib/workers/extract/extract_worker.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ export async function integTestWorker(request: IntegTestBatchRequest): Promise<I
3131
engine: request.engine,
3232
test,
3333
profile: request.profile,
34+
region: request.region,
3435
env: {
35-
AWS_REGION: request.region,
3636
CDK_DOCKER: process.env.CDK_DOCKER ?? 'docker',
3737
},
3838
showOutput: verbosity >= 2,
@@ -99,8 +99,8 @@ export async function watchTestWorker(options: IntegWatchOptions): Promise<void>
9999
engine: options.engine,
100100
test,
101101
profile: options.profile,
102+
region: options.region,
102103
env: {
103-
AWS_REGION: options.region,
104104
CDK_DOCKER: process.env.CDK_DOCKER ?? 'docker',
105105
},
106106
showOutput: verbosity >= 2,

packages/@aws-cdk/integ-runner/test/engines/toolkit-lib-snapshot-path.test.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,15 @@ describe('ToolkitLibRunnerEngine - Snapshot Path Handling', () => {
2525

2626
engine = new ToolkitLibRunnerEngine({
2727
workingDirectory: '/test/dir',
28+
region: 'us-dummy-1',
2829
});
2930
});
3031

3132
it('should use fromAssemblyDirectory when app is a path to existing snapshot directory', async () => {
3233
const snapshotPath = 'test.snapshot';
3334
const fullSnapshotPath = path.join('/test/dir', snapshotPath);
3435
const mockCx = { produce: jest.fn() };
35-
const mockLock = { dispose: jest.fn() };
36+
const mockLock = { dispose: jest.fn(), cloudAssembly: { stacksRecursively: [] } };
3637

3738
// Mock fs to indicate the snapshot directory exists
3839
mockedFs.pathExistsSync.mockReturnValue(true);
@@ -55,7 +56,7 @@ describe('ToolkitLibRunnerEngine - Snapshot Path Handling', () => {
5556
it('should use fromCdkApp when app is not a path to existing directory', async () => {
5657
const appCommand = 'node bin/app.js';
5758
const mockCx = { produce: jest.fn() };
58-
const mockLock = { dispose: jest.fn() };
59+
const mockLock = { dispose: jest.fn(), cloudAssembly: { stacksRecursively: [] } };
5960

6061
// Mock fs to indicate the path doesn't exist
6162
mockedFs.pathExistsSync.mockReturnValue(false);
@@ -76,7 +77,7 @@ describe('ToolkitLibRunnerEngine - Snapshot Path Handling', () => {
7677
const appPath = 'app.js';
7778
const fullAppPath = path.join('/test/dir', appPath);
7879
const mockCx = { produce: jest.fn() };
79-
const mockLock = { dispose: jest.fn() };
80+
const mockLock = { dispose: jest.fn(), cloudAssembly: { stacksRecursively: [] } };
8081

8182
// Mock fs to indicate the path exists but is not a directory
8283
mockedFs.pathExistsSync.mockReturnValue(true);

packages/@aws-cdk/integ-runner/test/engines/toolkit-lib.test.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,14 @@ describe('ToolkitLibRunnerEngine', () => {
2525

2626
engine = new ToolkitLibRunnerEngine({
2727
workingDirectory: '/test/dir',
28+
region: 'us-dummy-1',
2829
});
2930
});
3031

3132
describe('synth', () => {
3233
it('should call toolkit.synth with correct parameters', async () => {
3334
const mockCx = { produce: jest.fn() };
34-
const mockLock = { dispose: jest.fn() };
35+
const mockLock = { dispose: jest.fn(), cloudAssembly: { stacksRecursively: [] } };
3536
mockToolkit.fromCdkApp.mockResolvedValue(mockCx as any);
3637
mockToolkit.synth.mockResolvedValue(mockLock as any);
3738

@@ -56,7 +57,7 @@ describe('ToolkitLibRunnerEngine', () => {
5657
describe('synthFast', () => {
5758
it('should use fromCdkApp and produce for fast synthesis', async () => {
5859
const mockCx = { produce: jest.fn() };
59-
const mockLock = { dispose: jest.fn() };
60+
const mockLock = { dispose: jest.fn(), cloudAssembly: { stacksRecursively: [] } };
6061
mockCx.produce.mockResolvedValue(mockLock);
6162
mockToolkit.fromCdkApp.mockResolvedValue(mockCx as any);
6263

@@ -221,11 +222,12 @@ describe('ToolkitLibRunnerEngine', () => {
221222
const engineWithOutput = new ToolkitLibRunnerEngine({
222223
workingDirectory: '/test',
223224
showOutput: true,
225+
region: 'us-dummy-1',
224226
});
225227

226-
expect(MockedToolkit).toHaveBeenCalledWith({
228+
expect(MockedToolkit).toHaveBeenCalledWith(expect.objectContaining({
227229
ioHost: expect.any(Object),
228-
});
230+
}));
229231
});
230232

231233
it('should throw error when no app is provided', async () => {

packages/@aws-cdk/toolkit-lib/lib/api/aws-auth/base-credentials.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,14 @@ export class BaseCredentials {
9292
*/
9393
public static awsCliCompatible(options: AwsCliCompatibleOptions = {}): IBaseCredentialsProvider {
9494
return new class implements IBaseCredentialsProvider {
95-
public sdkBaseConfig(ioHost: IActionAwareIoHost, clientConfig: SdkBaseClientConfig) {
95+
public async sdkBaseConfig(ioHost: IActionAwareIoHost, clientConfig: SdkBaseClientConfig) {
9696
const ioHelper = IoHelper.fromActionAwareIoHost(ioHost);
9797
const awsCli = new AwsCliCompatible(ioHelper, clientConfig.requestHandler ?? {}, new IoHostSdkLogger(ioHelper));
98-
return awsCli.baseConfig(options.profile);
98+
99+
const ret = await awsCli.baseConfig(options.profile);
100+
return options.defaultRegion
101+
? { ...ret, defaultRegion: options.defaultRegion }
102+
: ret;
99103
}
100104

101105
public toString() {
@@ -140,6 +144,18 @@ export interface AwsCliCompatibleOptions {
140144
* @default - Use environment variable if set.
141145
*/
142146
readonly profile?: string;
147+
148+
/**
149+
* Use a different default region than the one in the profile
150+
*
151+
* If not supplied the environment variable AWS_REGION will be used, or
152+
* whatever region is set in the indicated profile in `~/.aws/config`.
153+
* If no region is set in the profile the region in `[default]` will
154+
* be used.
155+
*
156+
* @default - Use region from `~/.aws/config`.
157+
*/
158+
readonly defaultRegion?: string;
143159
}
144160

145161
export interface CustomBaseCredentialsOption {

0 commit comments

Comments
 (0)