feat: add upstream proxy configuration for outbound requests#1458
feat: add upstream proxy configuration for outbound requests#1458Andreybest wants to merge 15 commits into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1458 +/- ##
==========================================
- Coverage 90.25% 90.10% -0.16%
==========================================
Files 69 69
Lines 5561 5657 +96
Branches 958 990 +32
==========================================
+ Hits 5019 5097 +78
- Misses 523 541 +18
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jescalada
left a comment
There was a problem hiding this comment.
@Andreybest Thanks for the contribution! 🚀
I left some comments for code refactoring. It'd be very helpful if you could upload images or a video demonstrating that this feature works as intended.
@coopernetes Wondering if this implements your original requirements (#759) and works on your end 🤔
dcoric
left a comment
There was a problem hiding this comment.
Thanks for tackling #759. I've left a few comments focused on correctness and robustness, especially around NO_PROXY pattern handling which is important to get right for the target use case. The main items are the missing */leading-dot NO_PROXY support and proxy URL validation.
|
Thanks for the reviews @jescalada and @dcoric! |
|
@jescalada Screen.Recording.2026-03-25.at.21.35.35_no_audio.mov |
Co-authored-by: Thomas Cooper <57812123+coopernetes@users.noreply.github.com> Signed-off-by: Andrew <andrey255@live.com>
jescalada
left a comment
There was a problem hiding this comment.
LGTM after fixing Denis' comment about validating/error handling the HttpsProxyAgent creation.
@coopernetes Does everything work as you expected? 🤔
coopernetes
left a comment
There was a problem hiding this comment.
LGTM. The only rub is the lack of direct authentication support in the proxy agent. Many client and server libraries do not implement auth directly so it's fine to leave it out for now. Worth making a follow up issue to add HTTP Basic and NTLM (Windows) auth support which are the two most common methods (at least that I'm aware of).
I'd suggest making that HTTP Basic, NTLM and Negotiate (the negotiate protocol is used to detect support for NTLM or Kerberos auth and then kick off which ever is supported). |
|
@dcoric Thank you again for the review! Can you please check again? :) |
|
@Andreybest could you run teh schema doc generation - you did typescript types gen but not but not config schema docs generation. See https://github.com/finos/git-proxy/blob/main/CONTRIBUTING.md#configuration-schema Very keen to merge this as it will unblock some testing for us on windows machines. We'll report back with testing in a bit. |
@goldensyntax can probably PR into your branch for this... |
Done that with: |
Merged @goldensyntax PR Can we merge now? |
|
@jescalada will need to approve and merge due to previously requested change. |
re-vlad
left a comment
There was a problem hiding this comment.
While the code supports upstreamProxy config, the user-facing doc does not mention this new feature. New users (especially in corporate environments) will not know how to enable it. Please add a configuration example under the "configuration" section in readme.md:
"upstreamProxy": {
"enabled": true,
"url": "http://proxy.corp.local:8080",
"noProxy": [
"localhost",
"127.0.0.1",
"github.com"
]
}```
I'd say 'both configuration file in |
|
Thank you for the review @re-vlad!
|
kriswest
left a comment
There was a problem hiding this comment.
We should be able to complete a retest of this tomorrow, which we'd like to do before this merges.
There was a problem hiding this comment.
@Andreybest LGTM 👍🏼
Do you mind adding an issue for direct authentication support (HTTP Basic, NTLM and Negotiate) as mentioned earlier and referencing to this PR?
Resolves #759.
Adds upstream proxy support. Uses values from both configuration file in
upstreamProxyfields or environment variables (HTTPS_PROXY/HTTP_PROXY,NO_PROXY)