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.

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

Conversation

RomainMuller
Copy link
Contributor

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
@RomainMuller RomainMuller requested a review from a team October 1, 2021 10:51
@RomainMuller RomainMuller self-assigned this Oct 1, 2021
@gitpod-io
Copy link

gitpod-io bot commented Oct 1, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 1, 2021
@RomainMuller RomainMuller added the pr-linter/exempt-test The PR linter will not require test changes label Oct 1, 2021
@RomainMuller RomainMuller enabled auto-merge (squash) October 1, 2021 12:06
@RomainMuller RomainMuller merged commit ceab036 into master Oct 1, 2021
@RomainMuller RomainMuller deleted the rmuller/proxy-agent branch October 1, 2021 12:54
@aws-cdk-automation
Copy link
Collaborator

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

// 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

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.

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

ryparker added a commit that referenced this pull request Oct 1, 2021
mergify bot pushed a commit that referenced this pull request Oct 1, 2021
…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*
xykkong added a commit to xykkong/aws-cdk that referenced this pull request Oct 6, 2021
* '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)
  ...
mergify bot pushed a commit that referenced this pull request Oct 7, 2021
## 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*
njlynch pushed a commit that referenced this pull request Oct 11, 2021
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
njlynch pushed a commit that referenced this pull request Oct 11, 2021
…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*
njlynch pushed a commit that referenced this pull request Oct 11, 2021
## 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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
## 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDK ignores no_proxy config
4 participants