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

Fix bound address/port for caddy's certmagic library #15758

Merged
merged 7 commits into from
May 12, 2021
Merged

Fix bound address/port for caddy's certmagic library #15758

merged 7 commits into from
May 12, 2021

Conversation

bminer
Copy link
Contributor

@bminer bminer commented May 7, 2021

This is my attempt to fix issue created by d2ea21d when switching to Caddy's certmagic library. I believe that the call to ManageSync causes Gitea to try and listen on port 80 even if PortToRedirect is set.

Please test this fix, but I think it works. Thoughts?

@codecov-commenter
Copy link

Codecov Report

Merging #15758 (9e1208e) into main (114c85e) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #15758      +/-   ##
==========================================
- Coverage   43.93%   43.93%   -0.01%     
==========================================
  Files         678      678              
  Lines       81945    81950       +5     
==========================================
- Hits        36005    36004       -1     
- Misses      40069    40075       +6     
  Partials     5871     5871              
Impacted Files Coverage Δ
cmd/web_letsencrypt.go 0.00% <0.00%> (ø)
modules/process/manager.go 72.83% <0.00%> (-2.47%) ⬇️
models/repo_list.go 77.04% <0.00%> (-0.78%) ⬇️
modules/util/timer.go 85.71% <0.00%> (+42.85%) ⬆️

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 114c85e...9e1208e. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 7, 2021
@techknowlogick techknowlogick added this to the 1.15.0 milestone May 7, 2021
@zeripath
Copy link
Contributor

zeripath commented May 7, 2021

the thing is certmagic shouldn't be opening any ports itself. We open and control these ports ourselves.

6543
6543 previously requested changes May 7, 2021
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

as per zeripath, we should find out why certmagic would (if ..) open a port itself and consider it a bug

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

the ListenHost and AltHTTPPort are used in the challenge solvers - that's why they're needed

@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 May 9, 2021
@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 May 12, 2021
@zeripath
Copy link
Contributor

make lgtm work

@zeripath zeripath merged commit 6a8e5f6 into go-gitea:main May 12, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request May 12, 2021
Backport go-gitea#15758

* Fix bound address/port for caddy's certmagic library

* Fix bug

Co-authored-by: zeripath <art27@cantab.net>
@zeripath zeripath added the backport/done All backports for this PR have been created label May 12, 2021
zeripath added a commit that referenced this pull request May 12, 2021
Backport #15758

* Fix bound address/port for caddy's certmagic library

* Fix bug

Co-authored-by: zeripath <art27@cantab.net>

Co-authored-by: Blake Miner <miner.blake@gmail.com>
@bminer bminer mentioned this pull request Jun 17, 2021
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
* Fix bound address/port for caddy's certmagic library

* Fix bug

Co-authored-by: zeripath <art27@cantab.net>
@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
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants