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

Support uppercase *_PROXY env vars. #1292

Open
zamnuts opened this issue Mar 8, 2020 · 2 comments
Open

Support uppercase *_PROXY env vars. #1292

zamnuts opened this issue Mar 8, 2020 · 2 comments

Comments

@zamnuts
Copy link

zamnuts commented Mar 8, 2020

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, and no_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 by got, via the recommended caw), explicitly supports both uppercase and lowercase env vars. As an aside, it also supports equivalent npm-config keys of https-proxy, http-proxy, and proxy, despite this being a different concern altogether (npm vs application). See https://github.com/kevva/get-proxy/blob/v2.1.0/index.js#L5-L8

GNU Emacs mentions case-insensitivity in their manual: https://www.gnu.org/software/emacs/manual/html_node/url/Proxies.html

The library recognizes such variables in either upper or lower case.

@zamnuts zamnuts changed the title Support uppercase GRPC_PROXY, HTTPS_PROXY, and HTTP_PROXY env vars. Support uppercase *_PROXY env vars. Mar 8, 2020
@sharat87
Copy link

sharat87 commented Nov 8, 2024

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!

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 -->
@murgatroid99
Copy link
Member

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
Projects
None yet
Development

No branches or pull requests

3 participants