Skip to content

Commit 3377c3b

Browse files
authored
fix(cli): notices don't work behind a proxy (#32590)
### Issue Fixes #32304 ### Reason for this change The CLI doesn't respect the proxy configuration (either via command line or via environment variables) to fetch notices. As a result, customers who can only access the internet via a proxy will never see notices. ### Description of changes - Proxy agent construction refactored into a public method, to be reused. - `Settings` has two new keys: `proxy` and `caBundlePath`. - These new settings are passed to the `Notices` class, which internally uses them to make the GET request to the notices URL. - Proxy integ test now also asserts that the notices URL is intercepted by the proxy. ### Description of how you validated changes Proxy integ test, and manual tests with `mitmproxy`. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 034679a commit 3377c3b

File tree

5 files changed

+55
-15
lines changed

5 files changed

+55
-15
lines changed

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2828,7 +2828,7 @@ integTest('requests go through a proxy when configured',
28282828
});
28292829

28302830
// We don't need to modify any request, so the proxy
2831-
// passes through all requests to the host.
2831+
// passes through all requests to the target host.
28322832
const endpoint = await proxyServer
28332833
.forAnyRequest()
28342834
.thenPassThrough();
@@ -2845,13 +2845,21 @@ integTest('requests go through a proxy when configured',
28452845
'--proxy', proxyServer.url,
28462846
'--ca-bundle-path', certPath,
28472847
],
2848+
modEnv: {
2849+
CDK_HOME: fixture.integTestDir,
2850+
},
28482851
});
28492852
} finally {
28502853
await fs.rm(certDir, { recursive: true, force: true });
28512854
await proxyServer.stop();
28522855
}
28532856

2854-
const actionsUsed = actions(await endpoint.getSeenRequests());
2857+
const requests = await endpoint.getSeenRequests();
2858+
2859+
expect(requests.map(req => req.url))
2860+
.toContain('https://cli.cdk.dev-tools.aws.dev/notices.json');
2861+
2862+
const actionsUsed = actions(requests);
28552863
expect(actionsUsed).toContain('AssumeRole');
28562864
expect(actionsUsed).toContain('CreateChangeSet');
28572865
}),

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -109,23 +109,27 @@ export class AwsCliCompatible {
109109
}
110110

111111
public static requestHandlerBuilder(options: SdkHttpOptions = {}): NodeHttpHandlerOptions {
112+
const agent = this.proxyAgent(options);
113+
114+
return {
115+
connectionTimeout: DEFAULT_CONNECTION_TIMEOUT,
116+
requestTimeout: DEFAULT_TIMEOUT,
117+
httpsAgent: agent,
118+
httpAgent: agent,
119+
};
120+
}
121+
122+
public static proxyAgent(options: SdkHttpOptions) {
112123
// Force it to use the proxy provided through the command line.
113124
// Otherwise, let the ProxyAgent auto-detect the proxy using environment variables.
114125
const getProxyForUrl = options.proxyAddress != null
115126
? () => Promise.resolve(options.proxyAddress!)
116127
: undefined;
117128

118-
const agent = new ProxyAgent({
129+
return new ProxyAgent({
119130
ca: tryGetCACert(options.caBundlePath),
120-
getProxyForUrl: getProxyForUrl,
131+
getProxyForUrl,
121132
});
122-
123-
return {
124-
connectionTimeout: DEFAULT_CONNECTION_TIMEOUT,
125-
requestTimeout: DEFAULT_TIMEOUT,
126-
httpsAgent: agent,
127-
httpAgent: agent,
128-
};
129133
}
130134

131135
/**

packages/aws-cdk/lib/cli.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
8989
output: configuration.settings.get(['outdir']),
9090
shouldDisplay: configuration.settings.get(['notices']),
9191
includeAcknowledged: cmd === 'notices' ? !argv.unacknowledged : false,
92+
httpOptions: {
93+
proxyAddress: configuration.settings.get(['proxy']),
94+
caBundlePath: configuration.settings.get(['caBundlePath']),
95+
},
9296
});
9397
await notices.refresh();
9498

packages/aws-cdk/lib/notices.ts

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import { ClientRequest } from 'http';
2-
import * as https from 'https';
2+
import { RequestOptions } from 'https';
3+
import * as https from 'node:https';
34
import * as path from 'path';
45
import type { Environment } from '@aws-cdk/cx-api';
56
import * as fs from 'fs-extra';
67
import * as semver from 'semver';
7-
import { debug, print, warning, error } from './logging';
8+
import { SdkHttpOptions } from './api';
9+
import { AwsCliCompatible } from './api/aws-auth/awscli-compatible';
10+
import { debug, error, print, warning } from './logging';
811
import { Context } from './settings';
912
import { loadTreeFromDir, some } from './tree';
1013
import { flatMap } from './util';
@@ -39,6 +42,11 @@ export interface NoticesProps {
3942
* @default true
4043
*/
4144
readonly shouldDisplay?: boolean;
45+
46+
/**
47+
* Options for the HTTP request
48+
*/
49+
readonly httpOptions?: SdkHttpOptions;
4250
}
4351

4452
export interface NoticesPrintOptions {
@@ -189,7 +197,7 @@ export class NoticesFilter {
189197
}
190198

191199
/**
192-
* Infomration about a bootstrapped environment.
200+
* Information about a bootstrapped environment.
193201
*/
194202
export interface BootstrappedEnvironment {
195203
readonly bootstrapStackVersion: number;
@@ -222,6 +230,7 @@ export class Notices {
222230
private readonly shouldDisplay: boolean;
223231
private readonly acknowledgedIssueNumbers: Set<Number>;
224232
private readonly includeAcknowlegded: boolean;
233+
private readonly httpOptions: SdkHttpOptions;
225234

226235
private data: Set<Notice> = new Set();
227236

@@ -234,6 +243,7 @@ export class Notices {
234243
this.includeAcknowlegded = props.includeAcknowledged ?? false;
235244
this.output = props.output ?? 'cdk.out';
236245
this.shouldDisplay = props.shouldDisplay ?? true;
246+
this.httpOptions = props.httpOptions ?? {};
237247
}
238248

239249
/**
@@ -263,7 +273,8 @@ export class Notices {
263273
}
264274

265275
try {
266-
const dataSource = new CachedDataSource(CACHE_FILE_PATH, options.dataSource ?? new WebsiteNoticeDataSource(), options.force ?? false);
276+
const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.httpOptions);
277+
const dataSource = new CachedDataSource(CACHE_FILE_PATH, underlyingDataSource, options.force ?? false);
267278
const notices = await dataSource.fetch();
268279
this.data = new Set(this.includeAcknowlegded ? notices : notices.filter(n => !this.acknowledgedIssueNumbers.has(n.issueNumber)));
269280
} catch (e: any) {
@@ -375,6 +386,12 @@ export interface NoticeDataSource {
375386
}
376387

377388
export class WebsiteNoticeDataSource implements NoticeDataSource {
389+
private readonly options: SdkHttpOptions;
390+
391+
constructor(options: SdkHttpOptions = {}) {
392+
this.options = options;
393+
}
394+
378395
fetch(): Promise<Notice[]> {
379396
const timeout = 3000;
380397
return new Promise((resolve, reject) => {
@@ -388,8 +405,13 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {
388405

389406
timer.unref();
390407

408+
const options: RequestOptions = {
409+
agent: AwsCliCompatible.proxyAgent(this.options),
410+
};
411+
391412
try {
392413
req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json',
414+
options,
393415
res => {
394416
if (res.statusCode === 200) {
395417
res.setEncoding('utf8');

packages/aws-cdk/lib/settings.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,7 @@ export class Settings {
266266
app: argv.app,
267267
browser: argv.browser,
268268
build: argv.build,
269+
caBundlePath: argv.caBundlePath,
269270
context,
270271
debug: argv.debug,
271272
tags,
@@ -285,6 +286,7 @@ export class Settings {
285286
output: argv.output,
286287
outputsFile: argv.outputsFile,
287288
progress: argv.progress,
289+
proxy: argv.proxy,
288290
bundlingStacks,
289291
lookups: argv.lookups,
290292
rollback: argv.rollback,

0 commit comments

Comments
 (0)