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(app): configure http with proxy url if present in environment #762

Merged

Conversation

jontze
Copy link
Contributor

@jontze jontze commented Mar 6, 2024

This PR adjusts the HttpModule configuration to apply proxy configuration from the https-proxy-agent package if the HTTP_PROXY or HTTPS_PROXY environment variables are present.

Should Close #714
Related to #752 (similar approach)

Why

(taken from #752)

HTTPS for corporate proxies doesn't work after #651 because it assumed that @nestjs/axios took care of correctly extracting proxy configuration from the environment variables. In reality, axios proxy setup is broken as mentioned in https://stackoverflow.com/a/53399378 and axios/axios#2072 (comment).

@wing328
Copy link
Member

wing328 commented Apr 13, 2024

@taras I wonder if you can review this change when you've time. Thank you.

@jontze jontze force-pushed the fix/consider-proxy-environment-variables branch from 9079344 to a4ef67e Compare April 15, 2024 15:04
@jontze
Copy link
Contributor Author

jontze commented Apr 15, 2024

I rebased my branch to the latest master.

@wing328
Copy link
Member

wing328 commented Apr 15, 2024

@jontze thanks. Have you tested it locally to confirm the fix?

@jontze
Copy link
Contributor Author

jontze commented Apr 16, 2024

@jontze thanks. Have you tested it locally to confirm the fix?

@wing328 Yes, I've been using the patch on my fork since the PR was created to work around the issue locally and haven't encountered any problems so far.

@wing328
Copy link
Member

wing328 commented Apr 16, 2024

Thanks @jontze let's give it a try

@wing328 wing328 merged commit 7787f07 into OpenAPITools:master Apr 16, 2024
3 checks passed
Copy link

🎉 This PR is included in version 2.13.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wing328
Copy link
Member

wing328 commented Apr 16, 2024

did a test with the latest release and it's working for m

thanks again for the PR

@berkitamas
Copy link

Hi,

This PR pretty much broke our setup. We have a local mirror which is set by generator-cli.repository.downloadUrl. That endpoint lives in the intranet. We have HTTPS_PROXY, HTTP_PROXY and NO_PROXY values set up in our environment. By checking the change, it looks like the values of NO_PROXY are ignored, and that is the culprit in our case (the mirror is set in NO_PROXY).

@wing328
Copy link
Member

wing328 commented Apr 21, 2024

@berkitamas thanks for reporting the issue. Please rollback to the previous version for the time being.

@jontze can you please take a look at the issue when you've time?

@jontze
Copy link
Contributor Author

jontze commented Apr 22, 2024

I'll take a look and try to come up with a solution that respect NO_PROXY .

jontze added a commit to jontze/openapi-generator-cli that referenced this pull request Apr 24, 2024
Related to OpenAPITools#762

In the linked PR, the proxy config was restored, but it didn't respected
the `NO_PROXY` | `no_proxy` configuration. This commit moved to a
different approach that is working with all common proxy environment
variables.
@jontze
Copy link
Contributor Author

jontze commented Apr 24, 2024

@wing328 I opened #781 to address this.

@berkitamas It would be amazing if you could verify the bugfix, if you have the time.

jontze added a commit to jontze/openapi-generator-cli that referenced this pull request Oct 18, 2024
Related to OpenAPITools#762

In the linked PR, the proxy config was restored, but it didn't respected
the `NO_PROXY` | `no_proxy` configuration. This commit moved to a
different approach that is working with all common proxy environment
variables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] HTTP_PROXY Support No Longer Works
3 participants