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(http): use proxy agent setup that supports proxy env variables with no_proxy #781

Merged

Conversation

jontze
Copy link
Contributor

@jontze jontze commented Apr 22, 2024

Fixes a bug introduced in #762, where the "no-proxy" environment configuration was ignored.

With this PR I moved from the usage of https-proxy-agent to proxy-agent. This was done as https-proxy-agent was configured to take the proxy url from the environment but the library itself doesn't handle NO_PROXY / no_proxy configuration.

proxy-agent on the other side, is sitting on top of the https-proxy-agent and http-proxy-agent library and decides if the proxy is used based on the request URL and the domains specified in the "no proxy" environment variables.


To test this

  1. Set the configuration for your proxy in the usual environment variables HTTP_PROXY , HTTPS_PROXY, http_proxy, https_proxy, NO_PROXY, no_proxy.
  2. You can set your custom repository download url in the generator-cli.repository.downloadUrl config
  3. Append your domain to the no_proxy and NO_PROXY environment variables OR if you don't have a mirror at hand, just add the default domain specified in config.schema.json and ensure that you have a way to verify that the traffic went not through the proxy.
  4. Execute the generator

⚠️ Define both variables! If you only set the upper case NO_PROXY variable, the lower case one will have presence!

ℹ️ The downloaded jar will be cached near the dist files of the generator cli (so usually in you node_modules), beside you patched the version like this in your package.json of your project, then it's not in your node_modules:

  "@openapitools/openapi-generator-cli": "file:../openapi-generator-cli/dist/apps/generator-cli"

@jontze jontze force-pushed the fix/respect_no_proxy_in_proxy_setup branch from 7e31c2e to 69c9dc5 Compare April 24, 2024 17:31
@jontze jontze marked this pull request as ready for review April 24, 2024 17:32
@gonzalad
Copy link

I am not sure all the explicit http_proxy and https_proxy handling is necessary in the current codebase.

Axios already handles those environment variables automatically.

From https://github.com/axios/axios#request-config:

{quote}
proxy defines the hostname, port, and protocol of the proxy server.
You can also define your proxy using the conventional http_proxy and
https_proxy environment variables. If you are using environment
variables for your proxy configuration, you can also define a no_proxy environment
variable as a comma-separated list of domains that should not be proxied.
{quote}

@gonzalad
Copy link

gonzalad commented Oct 17, 2024

I am not sure all the explicit http_proxy and https_proxy handling is necessary in the current codebase.

Axios already handles those environment variables automatically.

I just read issue axios/axios#5256 (400 Bad Request using axios proxy default handling) and now I understand that you handle the proxy explicitly to avoid this issue.

Sorry for the noise !

@gonzalad
Copy link

gonzalad commented Oct 18, 2024

@jontze : I just tested your PR in my corporate environment, it works great thanks !

@wing328 : could this PR be reviewed and merged to provide support for NO_PROXY environment variable ?

Thanks very much to both of you :)

@wing328
Copy link
Member

wing328 commented Oct 18, 2024

@jontze can you please resolve the merge conflicts when you've time?

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 jontze force-pushed the fix/respect_no_proxy_in_proxy_setup branch from 69c9dc5 to 57e947a Compare October 18, 2024 09:33
@jontze
Copy link
Contributor Author

jontze commented Oct 18, 2024

Thanks for testing and validating the fix in your corp environment! @gonzalad

@wing328 I rebased to the latest master branch and resolved the conflicts in the package.json and lock file.

@wing328
Copy link
Member

wing328 commented Oct 18, 2024

@jontze thank you

I'll merge it next monday as we try to avoid changes on Friday :)

When you've time, can you please also PM me via Slack?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

@wing328 wing328 merged commit f452cc8 into OpenAPITools:master Oct 21, 2024
3 checks passed
Copy link

🎉 This PR is included in version 2.14.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wing328
Copy link
Member

wing328 commented Oct 21, 2024

PR merged.

Please give it a try with the latest release.

@gonzalad
Copy link

@wing328 : I've just tested the version 2.14.1 in my corporate environment and it works fine.

Thanks to both of you !

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.

3 participants