Skip to content

feat: add upstream proxy configuration for outbound requests#1458

Merged
kriswest merged 16 commits into
finos:mainfrom
Andreybest:759-upstream-proxy
May 20, 2026
Merged

feat: add upstream proxy configuration for outbound requests#1458
kriswest merged 16 commits into
finos:mainfrom
Andreybest:759-upstream-proxy

Conversation

@Andreybest
Copy link
Copy Markdown
Contributor

@Andreybest Andreybest commented Mar 16, 2026

Resolves #759.

Adds upstream proxy support. Uses values from both configuration file in upstreamProxy fields or environment variables (HTTPS_PROXY/HTTP_PROXY, NO_PROXY)

@Andreybest Andreybest requested a review from a team as a code owner March 16, 2026 03:17
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 16, 2026

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 9cc79cc
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6a0db66138ebbb00072a4959

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Mar 16, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

@Andreybest Andreybest requested a review from kriswest March 16, 2026 03:17
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 81.44330% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.10%. Comparing base (c1d48ef) to head (9cc79cc).

Files with missing lines Patch % Lines
src/proxy/routes/index.ts 81.11% 17 Missing ⚠️
src/config/index.ts 85.71% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 🤔

Comment thread config.schema.json
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Comment thread test/upstreamProxy.test.ts
Copy link
Copy Markdown
Contributor

@dcoric dcoric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/proxy/routes/index.ts
Comment thread src/proxy/routes/index.ts Outdated
Comment thread website/docs/architecture/architecture.md
Comment thread src/proxy/routes/index.ts
Comment thread src/config/index.ts Fixed
@Andreybest
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @jescalada and @dcoric!
Made changes and left comments for your comments. Please recheck :)

@Andreybest
Copy link
Copy Markdown
Contributor Author

@jescalada
Here is the video on how the proxy works:

Screen.Recording.2026-03-25.at.21.35.35_no_audio.mov

Comment thread proxy.config.json Outdated
Co-authored-by: Thomas Cooper <57812123+coopernetes@users.noreply.github.com>
Signed-off-by: Andrew <andrey255@live.com>
Copy link
Copy Markdown
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after fixing Denis' comment about validating/error handling the HttpsProxyAgent creation.

@coopernetes Does everything work as you expected? 🤔

Comment thread src/proxy/routes/index.ts
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts
Comment thread src/proxy/routes/index.ts Outdated
Comment thread src/proxy/routes/index.ts Outdated
Copy link
Copy Markdown
Contributor

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@kriswest
Copy link
Copy Markdown
Contributor

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).

@Andreybest
Copy link
Copy Markdown
Contributor Author

@dcoric Thank you again for the review! Can you please check again? :)

@Andreybest Andreybest requested a review from dcoric April 21, 2026 23:06
@kriswest
Copy link
Copy Markdown
Contributor

@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.

@kriswest
Copy link
Copy Markdown
Contributor

@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

@goldensyntax can probably PR into your branch for this...

@goldensyntax
Copy link
Copy Markdown
Contributor

@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

@goldensyntax can probably PR into your branch for this...

Done that with:
Andreybest#1

@Andreybest
Copy link
Copy Markdown
Contributor Author

@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.

Merged @goldensyntax PR
Would be happy to see it in main too :)

Can we merge now?

Copy link
Copy Markdown
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kriswest
Copy link
Copy Markdown
Contributor

@jescalada will need to approve and merge due to previously requested change.

Copy link
Copy Markdown

@re-vlad re-vlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
  ]
}```

@re-vlad
Copy link
Copy Markdown

re-vlad commented May 14, 2026

Adds upstream proxy support. Uses values from both configuration file in upstreamProxy fields and environment variables (HTTPS_PROXY/HTTP_PROXY, NO_PROXY)

I'd say 'both configuration file in upstreamProxy fields OR environment variables' (according to the src/proxy/routes/index.ts, lines 237–238 const proxyUrl = url || getEnvProxyUrl();)

@Andreybest
Copy link
Copy Markdown
Contributor Author

Thank you for the review @re-vlad!

  1. This is already described in website/docs/architecture/architecture.md
  2. Changed PR description as you suggested

@Andreybest Andreybest requested a review from re-vlad May 18, 2026 02:22
@re-vlad re-vlad requested a review from vzateychuk May 18, 2026 09:35
Copy link
Copy Markdown
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to complete a retest of this tomorrow, which we'd like to do before this merges.

Copy link
Copy Markdown
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retest completed here and all looks good to us too.

We may need to have a chat about the authentication spec - but we're able to auth with proxy credentials added to the URL currently.

@kriswest
Copy link
Copy Markdown
Contributor

One CLI test failed on windows - re-running that as I'm not convinced it was specific to this PR

@Andreybest
Copy link
Copy Markdown
Contributor Author

Retest completed here and all looks good to us too.

We may need to have a chat about the authentication spec - but we're able to auth with proxy credentials added to the URL currently.

Glad to hear that!

On auth issue - raised #1541. @kriswest @jescalada

@kriswest kriswest merged commit 82031e9 into finos:main May 20, 2026
31 of 33 checks passed
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.

Expose proxy support within GitProxy itself for air gapped environments

9 participants