Skip to content

Commit

Permalink
fix(cli): notices don't work behind a proxy (#32590)
Browse files Browse the repository at this point in the history
### 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*
  • Loading branch information
otaviomacedo authored Dec 20, 2024
1 parent 034679a commit 3377c3b
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2828,7 +2828,7 @@ integTest('requests go through a proxy when configured',
});

// We don't need to modify any request, so the proxy
// passes through all requests to the host.
// passes through all requests to the target host.
const endpoint = await proxyServer
.forAnyRequest()
.thenPassThrough();
Expand All @@ -2845,13 +2845,21 @@ integTest('requests go through a proxy when configured',
'--proxy', proxyServer.url,
'--ca-bundle-path', certPath,
],
modEnv: {
CDK_HOME: fixture.integTestDir,
},
});
} finally {
await fs.rm(certDir, { recursive: true, force: true });
await proxyServer.stop();
}

const actionsUsed = actions(await endpoint.getSeenRequests());
const requests = await endpoint.getSeenRequests();

expect(requests.map(req => req.url))
.toContain('https://cli.cdk.dev-tools.aws.dev/notices.json');

const actionsUsed = actions(requests);
expect(actionsUsed).toContain('AssumeRole');
expect(actionsUsed).toContain('CreateChangeSet');
}),
Expand Down
22 changes: 13 additions & 9 deletions packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,23 +109,27 @@ export class AwsCliCompatible {
}

public static requestHandlerBuilder(options: SdkHttpOptions = {}): NodeHttpHandlerOptions {
const agent = this.proxyAgent(options);

return {
connectionTimeout: DEFAULT_CONNECTION_TIMEOUT,
requestTimeout: DEFAULT_TIMEOUT,
httpsAgent: agent,
httpAgent: agent,
};
}

public static proxyAgent(options: SdkHttpOptions) {
// Force it to use the proxy provided through the command line.
// Otherwise, let the ProxyAgent auto-detect the proxy using environment variables.
const getProxyForUrl = options.proxyAddress != null
? () => Promise.resolve(options.proxyAddress!)
: undefined;

const agent = new ProxyAgent({
return new ProxyAgent({
ca: tryGetCACert(options.caBundlePath),
getProxyForUrl: getProxyForUrl,
getProxyForUrl,
});

return {
connectionTimeout: DEFAULT_CONNECTION_TIMEOUT,
requestTimeout: DEFAULT_TIMEOUT,
httpsAgent: agent,
httpAgent: agent,
};
}

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
output: configuration.settings.get(['outdir']),
shouldDisplay: configuration.settings.get(['notices']),
includeAcknowledged: cmd === 'notices' ? !argv.unacknowledged : false,
httpOptions: {
proxyAddress: configuration.settings.get(['proxy']),
caBundlePath: configuration.settings.get(['caBundlePath']),
},
});
await notices.refresh();

Expand Down
30 changes: 26 additions & 4 deletions packages/aws-cdk/lib/notices.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { ClientRequest } from 'http';
import * as https from 'https';
import { RequestOptions } from 'https';
import * as https from 'node:https';
import * as path from 'path';
import type { Environment } from '@aws-cdk/cx-api';
import * as fs from 'fs-extra';
import * as semver from 'semver';
import { debug, print, warning, error } from './logging';
import { SdkHttpOptions } from './api';
import { AwsCliCompatible } from './api/aws-auth/awscli-compatible';
import { debug, error, print, warning } from './logging';
import { Context } from './settings';
import { loadTreeFromDir, some } from './tree';
import { flatMap } from './util';
Expand Down Expand Up @@ -39,6 +42,11 @@ export interface NoticesProps {
* @default true
*/
readonly shouldDisplay?: boolean;

/**
* Options for the HTTP request
*/
readonly httpOptions?: SdkHttpOptions;
}

export interface NoticesPrintOptions {
Expand Down Expand Up @@ -189,7 +197,7 @@ export class NoticesFilter {
}

/**
* Infomration about a bootstrapped environment.
* Information about a bootstrapped environment.
*/
export interface BootstrappedEnvironment {
readonly bootstrapStackVersion: number;
Expand Down Expand Up @@ -222,6 +230,7 @@ export class Notices {
private readonly shouldDisplay: boolean;
private readonly acknowledgedIssueNumbers: Set<Number>;
private readonly includeAcknowlegded: boolean;
private readonly httpOptions: SdkHttpOptions;

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

Expand All @@ -234,6 +243,7 @@ export class Notices {
this.includeAcknowlegded = props.includeAcknowledged ?? false;
this.output = props.output ?? 'cdk.out';
this.shouldDisplay = props.shouldDisplay ?? true;
this.httpOptions = props.httpOptions ?? {};
}

/**
Expand Down Expand Up @@ -263,7 +273,8 @@ export class Notices {
}

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

export class WebsiteNoticeDataSource implements NoticeDataSource {
private readonly options: SdkHttpOptions;

constructor(options: SdkHttpOptions = {}) {
this.options = options;
}

fetch(): Promise<Notice[]> {
const timeout = 3000;
return new Promise((resolve, reject) => {
Expand All @@ -388,8 +405,13 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {

timer.unref();

const options: RequestOptions = {
agent: AwsCliCompatible.proxyAgent(this.options),
};

try {
req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json',
options,
res => {
if (res.statusCode === 200) {
res.setEncoding('utf8');
Expand Down
2 changes: 2 additions & 0 deletions packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ export class Settings {
app: argv.app,
browser: argv.browser,
build: argv.build,
caBundlePath: argv.caBundlePath,
context,
debug: argv.debug,
tags,
Expand All @@ -285,6 +286,7 @@ export class Settings {
output: argv.output,
outputsFile: argv.outputsFile,
progress: argv.progress,
proxy: argv.proxy,
bundlingStacks,
lookups: argv.lookups,
rollback: argv.rollback,
Expand Down

0 comments on commit 3377c3b

Please sign in to comment.