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

Add proxy settings and support for migration and webhook #16704

Merged
merged 10 commits into from
Aug 18, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 16, 2021

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.

  • Git clone from external http/https URLs
  • Migrate LFS datas from external git http/https URLs
  • Migrate other datas via API from other git services http/https URLs
  • Send webhooks to external URLs

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.

⚠️ BREAKING ⚠️

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.

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 16, 2021
@lunny lunny added this to the 1.16.0 milestone Aug 16, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 16, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 16, 2021
@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 16, 2021

The LFS client is missing. Otherwise lgtm.

modules/git/command.go Outdated Show resolved Hide resolved
modules/git/repo.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member Author

lunny commented Aug 17, 2021

The LFS client is missing. Otherwise lgtm.

done.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #16704 (6eca452) into main (422c30d) will decrease coverage by 0.01%.
The diff coverage is 50.36%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
modules/migrations/gitea_downloader.go 0.90% <0.00%> (-0.03%) ⬇️
modules/migrations/gitlab.go 0.97% <0.00%> (-0.03%) ⬇️
modules/migrations/gogs.go 2.35% <0.00%> (-0.02%) ⬇️
services/mirror/mirror_pull.go 32.74% <0.00%> (ø)
modules/proxy/proxy.go 18.75% <18.75%> (ø)
modules/setting/proxy.go 45.45% <45.45%> (ø)
modules/repository/repo.go 47.32% <66.66%> (ø)
services/mirror/mirror_push.go 39.23% <66.66%> (ø)
modules/migrations/github.go 68.56% <71.42%> (+0.30%) ⬆️
modules/git/repo.go 47.71% <87.50%> (+2.31%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 422c30d...6eca452. Read the comment docs.

## 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//, socks://, blank will follow environment http_proxy/https_proxy
Copy link
Contributor

@zeripath zeripath Aug 17, 2021

Choose a reason for hiding this comment

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

Suggested change
- `PROXY_URL`: ****: Proxy server URL, support http://, https//, socks://, blank will follow environment http_proxy/https_proxy
- `PROXY_URL`: **\<empty\>`**: Proxy server URL, support http://, https//, socks://, blank will follow environment http_proxy/https_proxy

@zeripath
Copy link
Contributor

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.

@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 17, 2021

Missing:

resp, err := http.DefaultClient.Do(req)

resp, err := http.DefaultClient.Do(req)

resp, err := http.Get(asset.DownloadURL)

@lunny
Copy link
Member Author

lunny commented Aug 18, 2021

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.

I don't think it will break anything. off means follow operating system proxy setting.

@lunny
Copy link
Member Author

lunny commented Aug 18, 2021

@zeripath @KN4CK3R Both done.

Co-authored-by: zeripath <[email protected]>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 18, 2021
@lunny lunny merged commit f9acad8 into go-gitea:main Aug 18, 2021
@lunny lunny deleted the lunny/proxy branch August 18, 2021 13:11
if os.Getenv("http_proxy") != "" {
return os.Getenv("http_proxy")
}
return os.Getenv("https_proxy")
Copy link
Member

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

Copy link
Member Author

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()))
Copy link
Member

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

@lunny
Copy link
Member Author

lunny commented Aug 18, 2021

@lafriks Could you send a PR to fix them?

@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 20, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration does not clone repository from the proxy
7 participants