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

Update the proxyPolicy and constants #7069

Merged
merged 4 commits into from
Sep 25, 2020
Merged

Update the proxyPolicy and constants #7069

merged 4 commits into from
Sep 25, 2020

Conversation

zzhxiaofeng
Copy link
Contributor

Add the NO_PROXY and ALL_PROXY to constants, and update the proxyPolicy to support env vars for NO_PROXY and ALL_PROXY Azure/azure-sdk-for-java#5967

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Mostly JS style nits, but some functional questions in here too.

sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/util/constants.ts Outdated Show resolved Hide resolved
@tzhanl tzhanl requested a review from jongio March 18, 2020 08:39
@ramya-rao-a
Copy link
Contributor

@xirzec, @jeremymeng, Can we take another look at this PR?

@jeremymeng
Copy link
Member

I would expect isBypassed() used in the proxy policy so that if it returns true for some url, we then don't set proxy settings for the request.

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Some question and suggestions

sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
@tzhanl
Copy link
Contributor

tzhanl commented Jul 13, 2020

@jongio do you want to evaluate this PR? NO_PROXY and ALL_PROXY Env vars.

@jongio
Copy link
Member

jongio commented Jul 14, 2020

@zzhxiaofeng - This PR needs to be re-evaluated. It looks like HTTP_PROXY has moved to here: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-http/src/policies/proxyPolicy.ts

@zzhxiaofeng
Copy link
Contributor Author

@jeremymeng I have used function isBypassed in function getDefaultProxySettings to achieve the feature.

sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
@jeremymeng
Copy link
Member

@jeremymeng I have used function isBypassed in function getDefaultProxySettings to achieve the feature.

The NO_PROXY environment variable lists urls that should be excluded from proxying. Requests to those urls should be sent to servers directly.

I think isBypassed() should be call on a request url to see whether
the request should be sent through a proxy, instead of calling it on the proxy url. The right place is probably here something like

  public sendRequest(request: WebResourceLike): Promise<HttpOperationResponse> {
    if (!isBypassed(request.url) && !request.proxySettings) {
      request.proxySettings = this.proxySettings;
    }
    return this._nextPolicy.sendRequest(request);
  }

@zzhxiaofeng
Copy link
Contributor Author

@jeremymeng Thank you. I have updated the code here.

@jongio jongio requested review from jeremymeng and xirzec July 23, 2020 07:16
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

This change looks good, there are just a few little style things I'd like to clean up before merging.

Good job on this! 👍

sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/src/policies/proxyPolicy.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Looks good!

@jeremymeng
Copy link
Member

Merging. Thanks @zzhxiaofeng!

@jeremymeng jeremymeng merged commit 803ea43 into Azure:master Sep 25, 2020
deyaaeldeen pushed a commit to deyaaeldeen/azure-sdk-for-js that referenced this pull request Sep 25, 2020
* update to latest version of azure-sdk-for-js

* udpate core-http.api.md

* update proxyPolicy.ts

* update proxyPolicy.ts according to comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants