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

Adding HTTP(S) over HTTP(S) proxy support to add accounts. #26137

Merged
merged 14 commits into from
Jan 23, 2025

Conversation

lewis-sanchez
Copy link
Contributor

@lewis-sanchez lewis-sanchez commented Jan 8, 2025

This PR fixes #25990,

The PR configures Axios, an HTTP client that Azure Data Studio has a dependency on, to successfully make requests from a proxied environment. In cases where an HTTP proxy is used, the request to get tenants is unable to go through when a proxy only supports HTTP traffic. This can result in the request to hang and prevent completion. To avoid hanging an HTTPS request from a HTTP proxy, an HTTPS over HTTP tunnel is needed, which is what this PR adds.

I validated my changes, by having Azure Data Studio connect to the proxy endpoint set up by ProxyMan, and reviewed the following resources to identify that when Axios makes requests in proxy environments, it can result in errors that stop the request from completing.
https://janmolak.com/node-js-axios-behind-corporate-proxies-8b17a6f31f9d
axios/axios#925
axios/axios#3384
https://stackoverflow.com/questions/43433380/socket-hang-up-when-using-axios-get-but-not-when-using-https-get

@Benjin
Copy link
Contributor

Benjin commented Jan 8, 2025

This'll be huge if this fixes the proxy authentication issues! Can you update the description with information about how you've validated that this fixes the proxy issues?

@kburtram
Copy link
Member

kburtram commented Jan 9, 2025

@lewis-sanchez it looks like the CI build is failing. Do you mind taking a look and getting that clean? I doesn't look related to your change through.

@kburtram
Copy link
Member

@lewis-sanchez how are you testing this? Specifically, are we confident this won't introduce side effect in any other authentication use-cases?

@lewis-sanchez
Copy link
Contributor Author

@kburtram, I tested 3 scenarios.

Scenario 1 - Using a local proxy

The first scenario I tested was by setting up a local proxy with a tool named ProxyMan:
image

I configured Azure Data Studio to use the address provided by ProxyMan:
image

And that allowed me to see traffic going through the local proxy and to add an account:
image
image

Scenario 2 - Invalid proxy endpoint

The second scenario I tested, was setting the proxy address used by Azure Data Studio to an invalid one. Using an invalid proxy address resulted in the login page hanging and Azure Data Studio to show the following error message:
image

image

Scenario 3 - Not using a proxy

The third scenario I tested was to not provide Azure Data Studio with a proxy endpoint:
image

Going through the process of adding an account to make sure that there aren't any regressions being introduced and successfully doing that:
image

@lewis-sanchez lewis-sanchez merged commit a5d41aa into main Jan 23, 2025
12 checks passed
@lewis-sanchez lewis-sanchez deleted the lewissanchez/bug/fix-connectivity branch January 23, 2025 18:29
lewis-sanchez added a commit that referenced this pull request Jan 23, 2025
* Adjust axios proxy configuration

* Add logging around axios get request logic

* Minor clean up

* Fix conditional check

* Load environment proxy value

* Fix compilation error

* Review changes

* Add additional logging to tunnel logic with axios

* Remove unused import

* Set proxy false when proxy endpoint found

* Log when proxy endpoint is not found

* Improve log wording

* Prioritize workspace HTTP config before environment variables
kburtram pushed a commit that referenced this pull request Jan 23, 2025
…26162)

* Adjust axios proxy configuration

* Add logging around axios get request logic

* Minor clean up

* Fix conditional check

* Load environment proxy value

* Fix compilation error

* Review changes

* Add additional logging to tunnel logic with axios

* Remove unused import

* Set proxy false when proxy endpoint found

* Log when proxy endpoint is not found

* Improve log wording

* Prioritize workspace HTTP config before environment variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to add Account
4 participants