fix: CDK does not honor NO_PROXY settings by RomainMuller · Pull Request #16751 · aws/aws-cdk

RomainMuller · 2021-10-01T10:51:23Z

CDK was extracting the value of HTTPS?_PROXY and passing this to
proxy-agent explicitly, which resulted in not honoring the NO_PROXY
setting.

This removes that behavior and lets proxy-agent delegate to
proxy-from-env, which will leverage values in HTTPS?_PROXY and
NO_PROXY correctly.

Fixes #7121

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

CDK was extracting the value of `HTTPS?_PROXY` and passing this to `proxy-agent` explicitly, which resulted in not honoring the `NO_PROXY` setting. This removes that behavior and lets `proxy-agent` delegate to `proxy-from-env`, which will leverage values in `HTTPS?_PROXY` and NO_PROXY correctly. Fixes #7121

gitpod-io · 2021-10-01T10:51:25Z

aws-cdk-automation · 2021-10-01T12:55:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: baa1f51
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

ryparker · 2021-10-01T21:28:44Z

packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/common.ts

+    // request.
+    //
+    // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
+    const ProxyAgent: any = require('proxy-agent');


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.

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

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" }

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

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).

This reverts commit ceab036.

…6761) ## Summary This [commit](ceab036) broke EKS deployments. CloudFormation throws "Internal failure." when attempting to create an EKS cluster. Full details : https://github.com/aws/aws-cdk/pull/16751/files#r720549975 This reverts commit ceab036. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

* '15588' of https://github.com/xykkong/aws-cdk: (47 commits) chore: rollback `GenericSSMParameterImage` deprecation (backport aws#16798) (aws#16800) chore(deps): bump actions/setup-node from 2.4.0 to 2.4.1 (aws#16778) Update CHANGELOG.md chore(release): 1.126.0 feat(assertions): matcher support for `templateMatches()` API (aws#16789) feat(stepfunctions-tasks): add step concurrency level to EmrCreateCluster (aws#15242) docs(s3): correct heading levels Object Ownership / Bucket deletion (aws#16790) chore(individual-pkg-gen): fix bug in setting alpha package visibility (aws#16787) fix(s3): setting `autoDeleteObjects` to `false` empties the bucket (aws#16756) fix(iam): `User.fromUserArn` does not work for ARNs that include a path (aws#16269) fix(cli): progress bar overshoots count by 1 for stack updates (aws#16168) fix(config): add SourceAccount condition to Lambda permission (aws#16617) docs(events): add a note about not using `EventPattern` with `CfnRule` (aws#16715) docs(core): fix reference to nonexistant enum value (aws#16716) chore(s3-deployments): update python version on BucketDeployment handler (aws#16771) chore: set response-requested length to 2 and closing-soon to 5 (aws#16763) fix(revert): "fix: CDK does not honor NO_PROXY settings (aws#16751)" (aws#16761) docs(GitHub issue templates): Upgrade to GitHub Issues v2 (aws#16592) chore: reset jsii-rosetta worker count to default (aws#16755) fix: CDK does not honor NO_PROXY settings (aws#16751) ...

## Summary CDK was extracting the value of HTTPS?_PROXY and passing this to proxy-agent explicitly, which resulted in not honoring the NO_PROXY setting. This removes that behavior and lets proxy-agent delegate to proxy-from-env, which will leverage values in HTTPS?_PROXY and NO_PROXY correctly. Tested by deploying [this sample repo](https://github.com/ryparker/aws-cdk-sample-eks) and monitoring Squid proxy logs while triggering the "onEvent" Lambda. Fixes #7121 Related PRs: #16751, #16751 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

CDK was extracting the value of `HTTPS?_PROXY` and passing this to `proxy-agent` explicitly, which resulted in not honoring the `NO_PROXY` setting. This removes that behavior and lets `proxy-agent` delegate to `proxy-from-env`, which will leverage values in `HTTPS?_PROXY` and NO_PROXY correctly. Fixes #7121

…6761) ## Summary This [commit](ceab036) broke EKS deployments. CloudFormation throws "Internal failure." when attempting to create an EKS cluster. Full details : https://github.com/aws/aws-cdk/pull/16751/files#r720549975 This reverts commit ceab036. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

## Summary CDK was extracting the value of HTTPS?_PROXY and passing this to proxy-agent explicitly, which resulted in not honoring the NO_PROXY setting. This removes that behavior and lets proxy-agent delegate to proxy-from-env, which will leverage values in HTTPS?_PROXY and NO_PROXY correctly. Tested by deploying [this sample repo](https://github.com/ryparker/aws-cdk-sample-eks) and monitoring Squid proxy logs while triggering the "onEvent" Lambda. Fixes #7121 Related PRs: #16751, #16751 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

CDK was extracting the value of `HTTPS?_PROXY` and passing this to `proxy-agent` explicitly, which resulted in not honoring the `NO_PROXY` setting. This removes that behavior and lets `proxy-agent` delegate to `proxy-from-env`, which will leverage values in `HTTPS?_PROXY` and NO_PROXY correctly. Fixes aws#7121

…ws#16761) ## Summary This [commit](aws@ceab036) broke EKS deployments. CloudFormation throws "Internal failure." when attempting to create an EKS cluster. Full details : https://github.com/aws/aws-cdk/pull/16751/files#r720549975 This reverts commit ceab036. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

## Summary CDK was extracting the value of HTTPS?_PROXY and passing this to proxy-agent explicitly, which resulted in not honoring the NO_PROXY setting. This removes that behavior and lets proxy-agent delegate to proxy-from-env, which will leverage values in HTTPS?_PROXY and NO_PROXY correctly. Tested by deploying [this sample repo](https://github.com/ryparker/aws-cdk-sample-eks) and monitoring Squid proxy logs while triggering the "onEvent" Lambda. Fixes aws#7121 Related PRs: aws#16751, aws#16751 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

RomainMuller requested a review from a team October 1, 2021 10:51

RomainMuller self-assigned this Oct 1, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 1, 2021

RomainMuller added the pr-linter/exempt-test The PR linter will not require test changes label Oct 1, 2021

njlynch approved these changes Oct 1, 2021

View reviewed changes

RomainMuller enabled auto-merge (squash) October 1, 2021 12:06

Merge branch 'master' into rmuller/proxy-agent

baa1f51

RomainMuller merged commit ceab036 into master Oct 1, 2021

RomainMuller deleted the rmuller/proxy-agent branch October 1, 2021 12:54

ryparker reviewed Oct 1, 2021

View reviewed changes

ryparker added a commit that referenced this pull request Oct 1, 2021

Revert "fix: CDK does not honor NO_PROXY settings (#16751)"

262a0d8

This reverts commit ceab036.

ryparker mentioned this pull request Oct 7, 2021

fix(aws-eks,sdk-provider): full proxy support #16840

Merged

zradlo1984 mentioned this pull request Nov 30, 2021

fix(cli): support CA bundle and proxy at the same time #16704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: CDK does not honor NO_PROXY settings#16751

fix: CDK does not honor NO_PROXY settings#16751
RomainMuller merged 2 commits intomasterfrom
rmuller/proxy-agent

RomainMuller commented Oct 1, 2021

Uh oh!

gitpod-io bot commented Oct 1, 2021

Uh oh!

aws-cdk-automation commented Oct 1, 2021

Uh oh!

ryparker Oct 1, 2021

Uh oh!

ryparker Oct 1, 2021

Uh oh!

ryparker Oct 1, 2021 •

edited

Loading

Uh oh!

ryparker Oct 1, 2021

Uh oh!

RomainMuller Oct 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

RomainMuller commented Oct 1, 2021

Uh oh!

gitpod-io bot commented Oct 1, 2021

Uh oh!

aws-cdk-automation commented Oct 1, 2021

AWS CodeBuild CI Report

Uh oh!

ryparker Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

ryparker Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

ryparker Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryparker Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

RomainMuller Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryparker Oct 1, 2021 •

edited

Loading