-
Notifications
You must be signed in to change notification settings - Fork 654
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
Support uppercase *_PROXY env vars. #1292
Comments
zamnuts
changed the title
Support uppercase GRPC_PROXY, HTTPS_PROXY, and HTTP_PROXY env vars.
Support uppercase *_PROXY env vars.
Mar 8, 2020
I'd like to work on this and open a PR if there's interest in merging. Just checking if your team is not already working on this before I actually spend time. Thanks! |
2 tasks
sharat87
added a commit
to appsmithorg/appsmith
that referenced
this issue
Nov 8, 2024
The Temporal connection SDK used in RTS, uses GRPC to talk to the Temporal server. The GRPC library being used only respects the lowercase proxy env variables (`http_proxy`, `https_proxy` and `no_proxy`), and not the uppercase ones. They have an [open issue asking for this since 2020](grpc/grpc-node#1292). However, in our `entrypoint.sh`, we normalize `http_proxy` and `HTTP_PROXY`, similarly for `https_proxy` and `HTTPS_PROXY`. But we're not doing this normalization for `no_proxy` and `NO_PROXY`. This PR fixes this by doing the same normalization to `no_proxy`. /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11737259912> > Commit: 310fa2c > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11737259912&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 08 Nov 2024 07:00:17 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved handling of proxy environment variables for enhanced configuration consistency. - **Bug Fixes** - Ensured `localhost` and `127.0.0.1` are always included in the `NO_PROXY` variable for better connectivity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There is no ongoing work on this. Feel free to pick it up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
PR #1243 introduced proxy support, but only supports lowercase environment variables. This is a non-issue on windows given nodejs/node#9157, but not on *nix. It doesn't account for the common use case of uppercase environment variables.
View the relevant source at https://github.com/grpc/grpc-node/blob/%40grpc/grpc-js%400.7.0/packages/grpc-js/src/http_proxy.ts#L45-L56
Solution:
Support both uppercase and lowercase environment variables, i.e. cross-platform case-insensitivity for
grpc_proxy
,https_proxy
,http_proxy
,no_grpc_proxy
, andno_proxy
.Alternative Solution:
Documenting that the grpc client only accepts lowercase env vars, and that their case sensitivity is governed by nodejs itself.
Additional references:
The popular
get-proxy
package (e.g. used bygot
, via the recommendedcaw
), explicitly supports both uppercase and lowercase env vars. As an aside, it also supports equivalentnpm-config
keys ofhttps-proxy
,http-proxy
, andproxy
, despite this being a different concern altogether (npm vs application). See https://github.com/kevva/get-proxy/blob/v2.1.0/index.js#L5-L8GNU Emacs mentions case-insensitivity in their manual: https://www.gnu.org/software/emacs/manual/html_node/url/Proxies.html
The text was updated successfully, but these errors were encountered: