-
Notifications
You must be signed in to change notification settings - Fork 210
[Do not merge] Add DeltaSharingClient Proxy Configuration #704
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
base: main
Are you sure you want to change the base?
Conversation
|
Nice thanks for the PR @zhu-tom ! |
moderakh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhu-tom The change looks good to me as a custom modification to support proxy usage when interacting with the server. However, please do not merge the PR.
Thank you
bob-hodgeman-kr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon testing this PR, I found that I was unable to make an https call via an http proxy. I've tested the proposed fix and basic testing shows that it is working.
| new HttpRoute(target) | ||
| } else { | ||
| // Route via proxy | ||
| new HttpRoute(target, proxy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor always constructs an insecure (http) route through the proxy. This does not work if one is using an http proxy to make an https call to the server. Instead, consider using an alternate constructor and setting the secure flag appropriately based on the target:
| new HttpRoute(target, proxy) | |
| val isSecure = target.getSchemeName == "https" | |
| new HttpRoute(target, null, proxy, isSecure) |
|
Hi team, |
|
Hi team 👋 Is there any update on why this PR can’t be merged? In many enterprise setups all egress must go through an HTTP(S) proxy, and the current Scala client builds an HttpClient without useSystemProperties() / a system route planner. As a result it ignores standard JVM proxy flags (-Dhttp.proxyHost, -Dhttps.proxyHost, http.nonProxyHosts, etc.), which makes the client unusable behind corporate proxies unless the network provides a fully transparent proxy. Would you consider a minimal, backward-compatible change to the client builder (e.g., calling useSystemProperties() or wiring a SystemDefaultRoutePlanner)? That would let the client honor existing JVM/env proxy configuration while keeping custom configs possible. If merging isn’t feasible, could you share the rationale (security/perf concerns, API stability, or platform compatibility)? I’m happy to help with tests and a small doc snippet. In the meantime, how can we use the library without this proxy configuration? |
No description provided.