-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Add proxy settings and support for migration and webhook #16704
Conversation
The LFS client is missing. Otherwise lgtm. |
done. |
Codecov Report
@@ Coverage Diff @@
## main #16704 +/- ##
==========================================
- Coverage 45.37% 45.36% -0.02%
==========================================
Files 758 760 +2
Lines 85381 85482 +101
==========================================
+ Hits 38742 38779 +37
- Misses 40358 40420 +62
- Partials 6281 6283 +2
Continue to review full report at Codecov.
|
## Proxy (`proxy`) | ||
|
||
- `PROXY_ENABLED`: **false**: Enable the proxy if true, all requests to external via HTTP will be affected, if false, no proxy will be used even environment http_proxy/https_proxy | ||
- `PROXY_URL`: ****: Proxy server URL, support http:https://, https//, socks:https://, blank will follow environment http_proxy/https_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.
- `PROXY_URL`: ****: Proxy server URL, support http:https://, https//, socks:https://, blank will follow environment http_proxy/https_proxy | |
- `PROXY_URL`: **\<empty\>`**: Proxy server URL, support http:https://, https//, socks:https://, blank will follow environment http_proxy/https_proxy |
I think setting the proxy off by default is both breaking and the wrong thing to do. We should have it on by default so that we defer to the system proxy by default. |
Missing: gitea/modules/migrations/gitlab.go Line 317 in 274aeb3
gitea/modules/migrations/github.go Line 298 in 274aeb3
gitea/modules/migrations/gitea_downloader.go Line 285 in 274aeb3
|
I don't think it will break anything. |
Co-authored-by: zeripath <[email protected]>
if os.Getenv("http_proxy") != "" { | ||
return os.Getenv("http_proxy") | ||
} | ||
return os.Getenv("https_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.
httpproxy.FromEnvironment().ProxyFunc()(url)
should be used instead of custom logic as it also contains support for NO_PROXY
env varaible
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.
We cannot use that, because we will set it as envs of git clone.
u, err := url.Parse(from) | ||
if err == nil && (strings.EqualFold(u.Scheme, "http") || strings.EqualFold(u.Scheme, "https")) { | ||
if proxy.Match(u.Host) { | ||
envs = append(envs, fmt.Sprintf("https_proxy=%s", proxy.GetProxyURL())) |
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.
https_proxy
or http_proxy
should be used based on either https or http proxy protocol is used
@lafriks Could you send a PR to fix them? |
replace #15234, #16697 and fix #12018
This PR added a proxy supports which should be applied to every request to external http/https URL.
Follow requests will follow the proxy setting if no standalone setting.
And since webhook has supported standalone proxy before this PR, webhook's proxy settings will override the proxy settings.
This PR also added an option to ignore tls verify on migrations.
This PR means that by default Gitea will not obey the system proxy unless you set
[proxy]
ENABLED= true
but leave the rest of the proxy configuration blank.This is in contrast to previous behaviour where Gitea would partially obey system proxies.