Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: CDK does not honor NO_PROXY settings #16751

Merged
merged 2 commits into from
Oct 1, 2021
Merged

fix: CDK does not honor NO_PROXY settings #16751

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d292648
fix: CDK does not honor NO_PROXY settings
RomainMuller Oct 1, 2021
baa1f51
Merge branch 'master' into rmuller/proxy-agent
Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 7 additions & 19 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,13 @@ export abstract class ResourceHandler {
RoleSessionName: `AWSCDK.EKSCluster.${this.requestType}.${this.requestId}`,
});

const proxyAddress = this.httpProxyFromEnvironment();
if (proxyAddress) {
this.log(`Using proxy server: ${proxyAddress}`);
// eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
const ProxyAgent: any = require('proxy-agent');
aws.config.update({
httpOptions: { agent: new ProxyAgent(proxyAddress) },
});
}
// Configure the proxy agent. By default, this will use HTTPS?_PROXY and
// NO_PROXY environment variables to determine which proxy to use for each
// request.
//
// eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
const ProxyAgent: any = require('proxy-agent');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change is causing EKS deployments to fail. CloudFormation now throws Internal Failure. when trying to create.

This handler is used in the "onComplete" Lambda function which does not have the proxy-agent available. We only pass an http_proxy env var into the "onEvent" Lambda since that is the only Lambda that makes requests with the aws-sdk-js.

I tested this by moving the proxy logic to the onEvent() method and was able to successfully deploy again. I'll make a PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that new ProxyAgent() does not automatically detect process.env.http_proxy or process.env.HTTP_PROXY.

Copy link
Contributor

@ryparker ryparker Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what ProxyAgent looks like when you log it in the Lambda:

Code:

const agent = new ProxyAgent()
console.log(`agent: ${JSON.stringify(agent, null, 2)}`);

Logs:

2021-10-01T22:32:09.812Z	03de5aeb-e937-4b3d-91a3-aee2c524cb47	INFO	agent: {}

Code:

const agent = new ProxyAgent(process.env.http_agent);
console.log(`agent: ${JSON.stringify(agent, null, 2)}`);

Logs:

2021-10-01T22:30:23.701Z	0ffc17d9-134f-498a-a8d9-dcfbe0754ae5	INFO	agent: {
  "proxy": {
    "protocol": "http:",
    "slashes": true,
    "auth": "user1:user1",
    "host": "184.73.16.134",
    "port": null,
    "hostname": "184.73.16.134",
    "hash": null,
    "search": null,
    "query": null,
    "pathname": "/",
    "path": "/",
    "href": "http://user1:user1@184.73.16.134/"
  },
  "proxyUri": "http://user1:user1@184.73.16.134"
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested using https://github.com/ryparker/aws-cdk-sample-eks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's normal and expected that the ProxyAgent instance does not look the same. If provided with a proxy, it will use ALWAYS and ONLY that proxy. When not specified, it will delegate to proxy-from-env, which will look at the correct proxy environment variable for the request' protocol & honor NO_PROXY.

I'd argue that the broken Lambda function should receive an HTTPS_PROXY and not HTTP_PROXY, as the AWS SDK typically makes only TSL connections... So it should be expected NOT to use HTTP_PROXY (using that value is incorrect there and should be considered a bug).

aws.config.update({ httpOptions: { agent: new ProxyAgent() } });
}

public onEvent() {
Expand Down Expand Up @@ -75,16 +73,6 @@ export abstract class ResourceHandler {
console.log(JSON.stringify(x, undefined, 2));
}

private httpProxyFromEnvironment(): string | undefined {
if (process.env.http_proxy) {
return process.env.http_proxy;
}
if (process.env.HTTP_PROXY) {
return process.env.HTTP_PROXY;
}
return undefined;
}

protected abstract async onCreate(): Promise<OnEventResponse>;
protected abstract async onDelete(): Promise<OnEventResponse | void>;
protected abstract async onUpdate(): Promise<(OnEventResponse & EksUpdateId) | void>;
Expand Down
33 changes: 10 additions & 23 deletions packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,48 +374,35 @@ function parseHttpOptions(options: SdkHttpOptions) {
}
config.customUserAgent = userAgent;

const proxyAddress = options.proxyAddress || httpsProxyFromEnvironment();
const caBundlePath = options.caBundlePath || caBundlePathFromEnvironment();

if (proxyAddress && caBundlePath) {
throw new Error(`At the moment, cannot specify Proxy (${proxyAddress}) and CA Bundle (${caBundlePath}) at the same time. See https://github.com/aws/aws-cdk/issues/5804`);
if (options.proxyAddress && caBundlePath) {
throw new Error(`At the moment, cannot specify Proxy (${options.proxyAddress}) and CA Bundle (${caBundlePath}) at the same time. See https://github.com/aws/aws-cdk/issues/5804`);
// Maybe it's possible after all, but I've been staring at
// https://github.com/TooTallNate/node-proxy-agent/blob/master/index.js#L79
// a while now trying to figure out what to pass in so that the underlying Agent
// object will get the 'ca' argument. It's not trivial and I don't want to risk it.
}

if (proxyAddress) { // Ignore empty string on purpose
// https://aws.amazon.com/blogs/developer/using-the-aws-sdk-for-javascript-from-behind-a-proxy/
debug('Using proxy server: %s', proxyAddress);
// eslint-disable-next-line @typescript-eslint/no-require-imports
const ProxyAgent: any = require('proxy-agent');
config.httpOptions.agent = new ProxyAgent(proxyAddress);
}
if (caBundlePath) {
debug('Using CA bundle path: %s', caBundlePath);
config.httpOptions.agent = new https.Agent({
ca: readIfPossible(caBundlePath),
keepAlive: true,
});
} else {
// Configure the proxy agent. By default, this will use HTTPS?_PROXY and
// NO_PROXY environment variables to determine which proxy to use for each
// request.
//
// eslint-disable-next-line @typescript-eslint/no-require-imports
const ProxyAgent: any = require('proxy-agent');
config.httpOptions.agent = new ProxyAgent();
}

return config;
}

/**
* Find and return the configured HTTPS proxy address
*/
function httpsProxyFromEnvironment(): string | undefined {
if (process.env.https_proxy) {
return process.env.https_proxy;
}
if (process.env.HTTPS_PROXY) {
return process.env.HTTPS_PROXY;
}
return undefined;
}

/**
* Find and return a CA certificate bundle path to be passed into the SDK.
*/
Expand Down