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

Gitea --port rewrites app.ini on it's own, breaks SSH access, API not working & no error logged. #13277

Closed
1 of 4 tasks
gsantner opened this issue Oct 23, 2020 · 8 comments · Fixed by #13288
Closed
1 of 4 tasks
Labels

Comments

@gsantner
Copy link

gsantner commented Oct 23, 2020

  • Gitea version (or commit ref): g26d6c1530
  • Git version: 2.17.1
  • Operating system: Ubuntu 18.04
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite

Description

I have updated to latest nightly build today and since that I cannot pull over SSH anymore suddenly. Has been working for years however.

I removed all old logs and checked log dir, appareantly NO relevant error is logged at all. Not even a authentication attempt, seemingly that point is not reached in code. Also not shows any error in shell. I tried like 5 times to pull in the meantime log-tail is running, the only thing that comes up is about notifications cron.

The git pull says:

parse "://:9003/api/internal/serv/command/3/ORGNAME/REPONAME?mode=1&verb=git-upload-pack": missing protocol scheme

So gitea somehow tries to connect to http interface, even though I cleary pull over SSH, wut? The port for http listener is 9003.

Screenshot attached.
Build with issue is:

Powered by Gitea Version: 1.14.0+dev-63-g26d6c1530
gitea-master-linux-amd64  10/22/2020 08:47:08 PM +00:00

My old version, nightly build from 19. October has been (no issue / bug not present there):

./gitea-2020-10-19 --version
Gitea version 1.14.0+dev-25-g9fe4b7b69

Screenshots

Screenshot_20201023-055106

@gsantner
Copy link
Author

gsantner commented Oct 23, 2020

The issue is that gitea now does !!write!! the app.ini config when restarting gitea.

I saw

grafik

Wrote the default config value again (checked diff, it was there also before updating gitea)

grafik

Pressed save, then restart gitea with systemctl:

grafik

Suddenly vim says file has changed since last write...and wonder..the :9003 value is there again (why gitea rewrites the config file and saves it to disk??)

grafik

systemctl file, pretty standard, always used it in this way:
grafik

I checked if there would be anything that also would write the app.ini file - there is none. Everything is gitea start command.

When I comment the setting completely I get:

Screenshot_20201024-001239

No errors logged to gitea log files.

@gsantner gsantner changed the title Cannot pull/clone anymore over SSH & no error logged. Gitea rewrites app.ini on it's own, breaks SSH access, clone not works & no error logged. Oct 23, 2020
@gsantner gsantner changed the title Gitea rewrites app.ini on it's own, breaks SSH access, clone not works & no error logged. Gitea rewrites app.ini on it's own, breaks SSH access, API not working & no error logged. Oct 23, 2020
@gsantner
Copy link
Author

gsantner commented Oct 23, 2020

Also broken on master release amd64 from 10/23/2020 06:53:55 PM +00:00.

UPDATE:: Found the issue: Passing gitea web -p PORT .. the port parameter breaks gitea now. But has not been breaking in the past. When passing i.e. 9003 "://:9003/" is written to config. This behaviour previously was not present and is the reason for SSH access not working.

@zeripath
Copy link
Contributor

Ok so this must be related to my recent pr on the install page. I'm not sure how this was ever working for you though because this code should not have significantly changed things

@zeripath
Copy link
Contributor

Oh crap.

@zeripath
Copy link
Contributor

gitea/cmd/web.go

Lines 119 to 124 in f40a2a4

// Flag for port number in case first time run conflict.
if ctx.IsSet("port") {
if err := setPort(ctx.String("port")); err != nil {
return err
}
}

Needs to be after

gitea/cmd/web.go

Lines 126 to 127 in f40a2a4

// Perform pre-initialization
needsInstall := routers.PreInstallInit(graceful.GetManager().HammerContext())

@lunny lunny added the type/bug label Oct 24, 2020
zeripath added a commit to zeripath/gitea that referenced this issue Oct 24, 2020
Unfortunately there was an error in go-gitea#13195 which set the --port
option before the settings were read. This PR fixes this by
moving applying this option to after the the settings are read

However, on looking further into this code I believe that the setPort
code was slightly odd.

Firstly, it may make sense to run the install page on a different
temporary port to the full system and this should be possible with
a --install-port option.

Secondy, if the --port option is provided we should apply it to both
otherwise there will be unusual behaviour on graceful restart

Thirdly, the documentation for --port says that the setting is
temporary - it should therefore not save its result to the configuration

(This however, does mean that authorized_keys and internal links may
not be correct. - I think we need to discuss this option further.)

Fix go-gitea#13277

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor

I think this option is somewhat broken to be honest.

The command line documentation states that it should be temporary - however, the previous code (and current code) writes this value to the configuration - so it's not temporary!

The reasoning of course is that gitea serv, gitea hook, gitea admin, gitea manager, etc. all have to communicate with the gitea process over its internal port and if this value is not saved then there will be an issue.

If we therefore want this --port to be temporary as it states we will need to add this --port option to all of those commands too...

but the question comes when gitea web writes its authorized_keys and hooks files - should it also write the --port ? It's supposed to be temporary so really it shouldn't be making permanent changes like that - but then unless you run with --port = PORT in the conf file then its all going to be broken.

I can see a point for the install-page to have an option to override the port settings and I've added an --install-port option for that but the question remains whether this value should be written to the file or not...

@gsantner
Copy link
Author

gsantner commented Oct 24, 2020

To be honest, I don't care whether or not something is written/cached. It's only a problem when gitea makes wrong configuration based upon the parameters, while it would be correct from config file as is.

I would recommend for future work: Not touch the config file at all! It's there for service configuration, not state management or caching. On some devices the file might even be marked as read-only so you can't overwrite it simply.
Instead if really required to save the state, why not read config file, make changes in memory, and save the updated file to some cache/tmp folder? I would go even further and say, the whole config (+args) should be taken constant from the time of starting gitea and running the cmdline/init code. No read/write from any config file afterwards.

@lunny
Copy link
Member

lunny commented Oct 24, 2020

But the installation will have to create/update the configuration file at once.

@zeripath zeripath changed the title Gitea rewrites app.ini on it's own, breaks SSH access, API not working & no error logged. Gitea --port rewrites app.ini on it's own, breaks SSH access, API not working & no error logged. Oct 28, 2020
techknowlogick added a commit that referenced this issue Oct 30, 2020
* Fix --port setting

Unfortunately there was an error in #13195 which set the --port
option before the settings were read. This PR fixes this by
moving applying this option to after the the settings are read

However, on looking further into this code I believe that the setPort
code was slightly odd.

Firstly, it may make sense to run the install page on a different
temporary port to the full system and this should be possible with
a --install-port option.

Secondy, if the --port option is provided we should apply it to both
otherwise there will be unusual behaviour on graceful restart

Thirdly, the documentation for --port says that the setting is
temporary - it should therefore not save its result to the configuration

(This however, does mean that authorized_keys and internal links may
not be correct. - I think we need to discuss this option further.)

Fix #13277

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update cmd/web.go

* Apply suggestions from code review

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants