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

Invalid default configuration, undocumented configuration options, and no session error logging #6855

Open
2 of 7 tasks
cbednarski opened this issue May 5, 2019 · 5 comments
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@cbednarski
Copy link

cbednarski commented May 5, 2019

  • Gitea version (or commit ref): Gitea version 1.8.0 built with go1.12.4 : bindata, sqlite, sqlite_unlock_notify

  • Git version: dna

  • Operating system: Ubuntu 18.04

  • Database (use [x]):

    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:

    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

I am just installing gitea for the first time on Ubuntu as a linux service.

The data for my installation is located under /data/gitea.

Here is a snippet of my configuration

[repository]
ROOT = /data/gitea/repositories
[session]
PROVIDER = file
[log]
MODE      = file
LEVEL     = Trace
ROOT_PATH = /data/gitea/logs

Here's my service file

[Unit]
Description=gitea server
After=network-online.target

[Service]
Type=simple
User=gitea
ExecStart=/usr/local/bin/gitea web --config /etc/gitea/app.ini
Restart=always

[Install]
WantedBy=multi-user.target

When I try to login the username and password are validated but I am returned to the login screen with an empty form. There is no error on the web. There is no error output in the logs. The only messages that show up are these:

2019/05/05 22:16:22 [D] Session ID: <someid>
2019/05/05 22:16:22 [D] CSRF Token: <some token>

However, the session was not actually being written anywhere and instead failed silently. I had to guess and add the following configuration:

[session]
PROVIDER = file
PROVIDER_CONFIG = /data/gitea/sessions

There are several bugs and/or unfortunate design choices here:

  1. There is no logging when Gitea fails to create a session file. All other failures during the installation (and there were many because of various data path and permission issues) resulted in a crash or fatal exit. However, sessions failed silently.

  2. GITEA_WORK_DIR seems to be required but this is not explicit in the documentation, it (apparently) cannot be configured by the configuration file, and gitea will happily start without it -- i.e. it is not actually required, but gitea will just be horribly broken.

  3. The default data path /usr/local/bin (based on the application path) is completely surprising. I have never seen another service work this way. I would expect it to use service defaults (/etc, /var/lib, etc.) or the well-established conventions $HOME/.config/appname/ or $HOME/.appname/) or PWD.

  4. Required configuration should not be specified via a totally optional environment variable. Rather, it should be a command-line argument like gitea web /var/lib/gitea. See minio /data/dir as an example. This makes it 100% obvious that it is required for things to work and also prevents the application from starting up in a broken state.

  5. gitea should sanity check the configuration before attempting to start with a configuration that will never work. See nginx -t as an example.

  6. The web install UI happily creates an invalid configuration that will never work. I used the web installer and then gitea crashed as soon as it had written the config file. This is likely because of the above issues.

  7. APP_DATA_PATH is mentioned in the FAQ, but not mentioned or explained in the docs, and it's not clear what this does. It seems there are three different ways to configure the same thing. This is at least one too many.

The setup process for gitea has been extremely frustrating. I spent about 5 minutes setting up minio the other day. By contrast, Gitea took about 3 hours of configuration whack-a-mole because it will happily start up with a completely broken, invalid configuration that will never work and then crash or fatal exit with vague or no error messages. I have Gitea up and running now and it's very nice, but the setup process was frustrating.

@lunny lunny added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label May 6, 2019
@zeripath
Copy link
Contributor

I suspect that part of the issue is that log.Fatal messages are currently silently disappearing on 1.8 - they're fixed on 1.9.

@zeripath
Copy link
Contributor

zeripath commented May 10, 2019

OK now I've looked a bit more at your issue I'll try to reply some of these. I'm sorry you've had difficulty - Gitea isn't built like a normal debian/ubuntu package, (more is the shame), and I think you've tried to force it to work like one without completely copying our documentation for it.

First of all did you look at: https://docs.gitea.io/en-us/linux-service/ and https://github.com/go-gitea/gitea/blob/master/contrib/systemd/gitea.service? Your service file is missing several key components that are required and likely caused your trouble. As I said above Gitea isn't built like a debian/ubuntu package - and has only just since #6631 been able to built in such a way but we don't build it that way by default.

Gitea is effectively built to expect to be run from its own directory with the location of the gitea executable and the working directory the same. (If you just moved the gitea binary to /home/gitea and ran it from there as the gitea user you would never have experienced these problems.) There are several environment variables that will override the default places gitea uses and these have been added over time to eventually create a gitea that could work like an expected debian/ubuntu package but only if these are variables are set. This is very similar to how firefox and google-chrome work, and if you look at these executables on debian they are scripts that will set environments correctly. We currently don't have a script for gitea although I would almost certainly recommend that anyone packaging gitea for debian/ubuntu should shadow the gitea binary with such a script - if only to help prevent problems like this in future. PRs are more than welcome here.

Now since #6631, it is possible to build gitea in such a way that you will probably not need to use these environment variables - however, which configuration variables depend on working directory vs. app path needs to be clearly looked at. (Personally I don't understand the reasoning behind some of these choices - However its very difficult to change these without breaking existing users) Unfortunately, we won't be able to change the default provided gitea on gitea.io to have the correct #6631 settings set during build as it will break existing users - so either debian packagers will either have to use a shadowing script described above or should rebuild with the correct variables.

Now to address some specific points. Regarding sessions, unfortunately the macaron.session package doesn't provide a way to test to see whether a configuration is acceptable. And in fact it is only when you attempt to login that session files should be created. It is at that point that they should cause a panic but they certainly shouldn't fail silently. Did you get a panic? You almost certainly experienced this problem because your default location was not accessible to the gitea service as your service file was incorrect.

Your issue with the GITEA_WORK_DIR and default data path are because you didn't set the service file correctly - I think I've explained above what these are doing.

I think you're partially right about checking configuration - there are certainly places were Gitea can determine whether it can create files where it wants to. However, as you see above the validity of a configuration file depends on the environment variables configured. Your configuration file is a perfectly valid file - just not from where you put gitea and where you ran it. As I said above, if you moved the gitea binary to /home/gitea and ran it from there with the gitea user you'd almost certainly have no trouble - depending on your values. We can do better, but we can't do this perfectly.

Now, what can we actually do to help prevent anyone else from experiencing these problems?

  1. I think we need to go through our configuration and think seriously about from which directory we relativise our paths - and we need to think seriously about the appropriateness for these.
  2. I think we need to create a shadowing gitea script that we recommend debian/ubuntu users use instead of the gitea binary - either that or we build specifically for debian/ubuntu users with their paths set within a package.
  3. We should improve our documentation to be clearer that the Gitea binary is not like a debian/ubuntu program and where it expects things to be. We probably just need to provide information to debian/ubuntu users to explain that things are not the way they expect.
  4. I think it's almost certainly worth going through the modules/setting/setting.go file and checking if we can test whether certain file paths are ever going to work - however, there will be a few places where this will depend on the values of the configuration in downstream packages.

However, please don't suffer in silence for 3 hours - use our discord and look at the documentation!

@cbednarski
Copy link
Author

Thanks for the detailed response! I'll try to read through and follow-up on some of these things. I would also be happy to write some config validation code when I get some time, since this seems like low-hanging fruit.

log.Fatal fixes in 1.9

I suspect that part of the issue is that log.Fatal messages are currently silently disappearing on 1.8 - they're fixed on 1.9.

That's great! I'll pull a version from master and see if that does anything interesting for me.

Install Docs

First of all did you look at: https://docs.gitea.io/en-us/linux-service/ and https://github.com/go-gitea/gitea/blob/master/contrib/systemd/gitea.service? Your service file is missing several key components that are required and likely caused your trouble.

I did see the linux service page but since I am familiar with systemd I wrote the .service file by hand. Also I did not notice the example service file since it was linked, while most other examples in the docs were inline. Oops!

I did read this part of the binary installation guide page that says:

NOTE: If you plan on running Gitea as a Linux service, you can skip [setting the environment variable] as the service file allows you to set WorkingDirectory. Otherwise, consider setting this environment variable (semi-)permanently so that Gitea consistently uses the correct working directory.

That line seems to be flat-out wrong, or at least misleading.

Duplicate Configs

I just took a look at https://github.com/go-gitea/gitea/blob/master/contrib/systemd/gitea.service and I noticed it has the following (partial snippet):

User=git
Group=git
WorkingDirectory=/var/lib/gitea/
ExecStart=/usr/local/bin/gitea web -c /etc/gitea/app.ini
Restart=always
Environment=USER=git HOME=/home/git GITEA_WORK_DIR=/var/lib/gitea

In my (now working) configuration I have User set in my service file but I did not set USER also. This seems redundant -- is that actually required? If so, why is it required twice? Likewise for WorkingDirectory and GITEA_WORK_DIR it seems like the same thing is configured the same thing twice which is confusing.

I have now set GITEA_WORK_DIR but not WorkingDirectory. If I set WorkingDirectory only and not GITEA_WORK_DIR, the app crashes on startup. So GITEA_WORK_DIR still seems to be required either way. USER seems extraneous.

Default Settings

Unfortunately, we won't be able to change the default provided gitea on gitea.io to have the correct #6631 settings set during build as it will break existing users

I'm curious to know more about this. I have only skimmed a bit of 6631 so far, but on the surface I could see a simple approach:

Either --work-dir flag / argument / config option or GIT_WORK_DIR must be present to start the program. If neither is set, it exits.

I've done quite a bit of backwards-compatibility surgery in the past and it seems like there could be a reasonable upgrade path here. For example, the unspecified "magic" configuration could be marked as deprecated, and warn that the config is invalid and the option must be explicitly configured.

A future version of gitea will implement the correct behavior and refuse to start if the config is missing. The user simply has to specify the configuration to continue, but their current database and working directory will not need to be changed manually.

Sessions

Now to address some specific points. Regarding sessions, unfortunately the macaron.session package doesn't provide a way to test to see whether a configuration is acceptable. And in fact it is only when you attempt to login that session files should be created. It is at that point that they should cause a panic but they certainly shouldn't fail silently. Did you get a panic

As far as I could tell there was no panic. After I entered my username and password I would be immediately returned to the login page with a blank form. The logs were empty. I will double-check this.

(In)valid Configuration

Your configuration file is a perfectly valid file - just not from where you put gitea and where you ran it.

While the config may be syntactically valid it is not correct, and in that sense it is not going to work. But most importantly, this is opaque to me. I don't actually know what gitea is doing internally, so there's no way for me to know it's incorrect.

As I said above, if you moved the gitea binary to /home/gitea and ran it from there with the gitea user you'd almost certainly have no trouble - depending on your values. We can do better, but we can't do this perfectly.

This is so surprising to me! I can't think of another program that doesn't expect to be run from the path, and where the location of the binary rather than pwd controls its behavior. It's very unusual.

Gitea is pulling configuration from 5 places(!): the environment, it's current binary path, the command line flags, the configuration file, and baked-in constants. At a minimum it uses 4 of these config sources. This is very complicated. My gut tells me it could/should use a minimum of 2.

Distro conventions

  1. We should improve our documentation to be clearer that the Gitea binary is not like a debian/ubuntu program and where it expects things to be.

I am coming from regularly using consul, nomad, vault, nginx, git, postgres etc. which do not strictly conform to debian packaging conventions, but also are compatible with them. FWIW I have also used FreeBSD and CentOS / RHEL and generally things use /etc/ there too.

Regardless of any small differences between unixes, gitea's behavior breaks all the standard conventions I can think of. Is that intended, or just incidental? I can't discern a design goal that would preclude it so I think gitea would be improved by following the same convention as everything else. Path of least surprise.

Forward

I realize that there are some past decisions here that are not easy to unwind. I have sporadically followed Gitea's development since before the Gogs fork, so I do not begrudge that there were decisions made in the past that were beyond your control. I'm also willing to put in a patch or two, though right now I am still trying to understand how things work and the best path forward.

Based on what you've said above I think a config-checker is the most straight foward way to proceed. At the very least it will make visible whatever logic and hidden or magic constants gitea is using, and make the problem more visible.

For the time being I think it would also be a good idea to emphasize the important of GITEA_WORK_DIR as being the thing that makes everything work. If I ran gitea as root (i.e. in docker) I would not notice most of these problems because with root permissions it will happily clobber everything else. In a traditional linux environment this option is still required.

@zeripath
Copy link
Contributor

OK work-path CLI option PR has been made.

@zeripath
Copy link
Contributor

I've also added a Contrib script in #6923 which we could use to shadow the gitea binary to set reasonable defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

3 participants