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

Refactor path system #24818

Closed
wxiaoguang opened this issue May 20, 2023 · 11 comments · Fixed by #25330
Closed

Refactor path system #24818

wxiaoguang opened this issue May 20, 2023 · 11 comments · Fixed by #25330
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 20, 2023

Feature Description

Gitea's path system was not well designed, it just got patched and patched and accumulated a lot of problems:

The root problem is, Gitea tries to guess these paths, and the environment/command argument/config option affect each other.

This a a refactoring plan for improving this system and resolving the problem with minimal breaking.

  1. Deprecate the environment GITEA_XXX_DIR and command argument "--custom-dir / --work-dir"
  2. When "gitea web" runs, try to detect whether there is an absolute "WORK_DIR" in app.ini
    • If no, use "guessed" absolute WorkDir to update the config
  3. Other commands always use the "WORK_DIR" in app.ini

There could be more details, I think this plan is feasible and stable.

Screenshots

No response

@wxiaoguang wxiaoguang added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels May 20, 2023
@silverwind
Copy link
Member

Generally agree. If there is no WORK_DIR in config, default it to current working directory. I would not write back into config at all. Writing into config should be avoided wherever possible.

@wxiaoguang
Copy link
Contributor Author

default it to current working directory.

It doesn't work. The "writing back" is necessary.

eg: some people do this:

  • put gitea in /usr/loca/bin
  • the "working dir" is /data/gitea
  • use GITEA_WORK_DIR to run gitea web
  • when run other commands, other commands only have --config option.

@eeyrjmr
Copy link
Contributor

eeyrjmr commented May 20, 2023

thing is, an application shouldn't write to /etc so if gitea needs to store such persistent changes it really should go into $HOME/.config/gitea.ini (or /var/lib/gitea/*) but this just adds more confusion as to where a setting is actually being set, as well as a full backup and restore

@silverwind
Copy link
Member

silverwind commented May 20, 2023

  • GITEA_WORK_DIR

This variable is mislabeled. A working directory in the classic sense is the directory where the application runs in. Not necessarily the directory where the data resides. A better name would be GITEA_DATA_DIR where subdirectories like repos exist, but I guess we can only change so much while staying compatible.

@wxiaoguang
Copy link
Contributor Author

thing is, an application shouldn't write to /etc so if gitea needs to store such persistent changes it really should go into $HOME/.config/gitea.ini (or /var/lib/gitea/*) but this just adds more confusion as to where a setting is actually being set, as well as a full backup and restore

If the app.ini is read-only, Gitea will prompt users to manually set the WORK_DIR correctly in their config file.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented May 20, 2023

  • GITEA_WORK_DIR

This variable is mislabeled. A working directory in the classic sense is the directory where the application runs in. Not necessarily the directory where the data resides. A better name would be GITEA_DATA_DIR where subdirectories like repos exist, but I guess we can only change so much while staying compatible.

There was already a DataDir in code.

In history, the code only got patched and patched (as an open-source project, I can understand), no design, refactoring or improvement, so the situation becomes today's problems.

@eeyrjmr
Copy link
Contributor

eeyrjmr commented May 20, 2023

thing is, an application shouldn't write to /etc so if gitea needs to store such persistent changes it really should go into $HOME/.config/gitea.ini (or /var/lib/gitea/*) but this just adds more confusion as to where a setting is actually being set, as well as a full backup and restore

If the app.ini is read-only, Gitea will prompt users to manually set the WORK_DIR correctly in their config file.

I appreciate that but for a correctly configured system /etc shouldn't be writable by non-root and if it was writeable by non-root implies a system that has some serious issue if it is compromised.
with gitea not being launchable as root, it is extremely safe to assume that /etc would not be writable.

So if it can be assumed that /etc isn't writable by root and gitea would not run as root, where would such settings be storable by gitea?

  1. database
  2. $WORK_DIR
  3. $XDG_CONFIG_HOME

@wxiaoguang
Copy link
Contributor Author

where would such settings be storable by gitea?

Sorry I don't get the point. The whole picture in my mind is:

For a fresh installation:

  1. No config/data on disk
  2. Run gitea web
  3. Visit the "install" page, set "work dir"
  4. Gitea save the config into "work dir/custom/app.ini"
  5. Then gitea could run with "--config work-dir/custom/app.ini"

For an existing installation (upgrading):

  1. Stop old gitea
  2. Run gitea web
  3. Gitea tries to write the GITEA_WORK_DIR into "app.ini"
    • If the "app.ini" is read-only, let site admin manually set this value
  4. Then gitea could run with "--config app.ini"

(there could be some slightly different cases, IMO all of them could be covered)


Does this answer your question? Or could you help to show some bad cases?

@eeyrjmr
Copy link
Contributor

eeyrjmr commented May 20, 2023

my bad, I interpreted aspects of this as advocating writing to /etc/gitea/app.ini

@lunny
Copy link
Member

lunny commented May 20, 2023

Maybe we should introduce another file to store the write-back information including tokens.

@silverwind
Copy link
Member

It's not completely unheard of that applications write back into config, Redis does it too. But I agree, a second file would be good where we could write stuff to, e.g. app.override.ini or something.

lunny pushed a commit that referenced this issue Jun 21, 2023
# The problem

There were many "path tricks":

* By default, Gitea uses its program directory as its work path
* Gitea tries to use the "work path" to guess its "custom path" and
"custom conf (app.ini)"
* Users might want to use other directories as work path
* The non-default work path should be passed to Gitea by GITEA_WORK_DIR
or "--work-path"
* But some Gitea processes are started without these values
    * The "serv" process started by OpenSSH server
    * The CLI sub-commands started by site admin
* The paths are guessed by SetCustomPathAndConf again and again
* The default values of "work path / custom path / custom conf" can be
changed when compiling

# The solution

* Use `InitWorkPathAndCommonConfig` to handle these path tricks, and use
test code to cover its behaviors.
* When Gitea's web server runs, write the WORK_PATH to "app.ini", this
value must be the most correct one, because if this value is not right,
users would find that the web UI doesn't work and then they should be
able to fix it.
* Then all other sub-commands can use the WORK_PATH in app.ini to
initialize their paths.
* By the way, when Gitea starts for git protocol, it shouldn't output
any log, otherwise the git protocol gets broken and client blocks
forever.

The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path
> env var GITEA_WORK_DIR > builtin default

The "app.ini" searching order is: cmd arg --config > cmd arg "work path
/ custom path" > env var "work path / custom path" > builtin default


## ⚠️ BREAKING

If your instance's "work path / custom path / custom conf" doesn't meet
the requirements (eg: work path must be absolute), Gitea will report a
fatal error and exit. You need to set these values according to the
error log.



----

Close #24818
Close #24222
Close #21606
Close #21498
Close #25107
Close #24981
Maybe close #24503

Replace #23301
Replace #22754

And maybe more
wxiaoguang added a commit to wxiaoguang/gitea that referenced this issue Jun 21, 2023
# The problem

There were many "path tricks":

* By default, Gitea uses its program directory as its work path
* Gitea tries to use the "work path" to guess its "custom path" and
"custom conf (app.ini)"
* Users might want to use other directories as work path
* The non-default work path should be passed to Gitea by GITEA_WORK_DIR
or "--work-path"
* But some Gitea processes are started without these values
    * The "serv" process started by OpenSSH server
    * The CLI sub-commands started by site admin
* The paths are guessed by SetCustomPathAndConf again and again
* The default values of "work path / custom path / custom conf" can be
changed when compiling

# The solution

* Use `InitWorkPathAndCommonConfig` to handle these path tricks, and use
test code to cover its behaviors.
* When Gitea's web server runs, write the WORK_PATH to "app.ini", this
value must be the most correct one, because if this value is not right,
users would find that the web UI doesn't work and then they should be
able to fix it.
* Then all other sub-commands can use the WORK_PATH in app.ini to
initialize their paths.
* By the way, when Gitea starts for git protocol, it shouldn't output
any log, otherwise the git protocol gets broken and client blocks
forever.

The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path
> env var GITEA_WORK_DIR > builtin default

The "app.ini" searching order is: cmd arg --config > cmd arg "work path
/ custom path" > env var "work path / custom path" > builtin default

## ⚠️ BREAKING

If your instance's "work path / custom path / custom conf" doesn't meet
the requirements (eg: work path must be absolute), Gitea will report a
fatal error and exit. You need to set these values according to the
error log.

----

Close go-gitea#24818
Close go-gitea#24222
Close go-gitea#21606
Close go-gitea#21498
Close go-gitea#25107
Close go-gitea#24981
Maybe close go-gitea#24503

Replace go-gitea#23301
Replace go-gitea#22754

And maybe more
# Conflicts:
#	cmd/web.go
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants