nixos/gitea: make use of declarative features where applicable

[aanderse]

Motivation for this change

I threw this together quickly so haven't really given any thought into how this would impact upgrading existing installations. I just wanted to get the code out there so I could get some feedback sooner rather than later. Feedback (positive or negative) appreciated.

NOTE: Best to review this commit by commit. If you think this should be broken up into multiple PRs I probably agree with you and can do so.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

---

[Ma27]

The patches itself seem fine, I'll do some testing on one of the next weekends unless one of the other maintainers is faster :)

[flokli]

cc @kolaente

[aanderse]

The patches itself seem fine, I'll do some testing on one of the next weekends unless one of the other maintainers is faster :)

@Ma27 I'm planning on testing this on an existing instance running mysql. Do you happen to have an existing instance running postgresql to test on?

[aanderse]

Updated code to add more declarative goodness via systemd.tmpfiles.rules (symlink).

[Ma27]

Do you happen to have an existing instance running postgresql to test on?

It's on my todo list to move several self-hosted services (including gitea) from sqlite to postgresql.
I'll probably have sufficient time this weekend, then I'd test the setup with this patch :)

[aanderse]

@GrahamcOfBorg test gitea.mysql gitea.postgres gitea.sqlite

[aanderse]

It's on my todo list to move several self-hosted services (including gitea) from sqlite to postgresql.
I'll probably have sufficient time this weekend, then I'd test the setup with this patch :)

@Ma27 That would be great. I haven't completely convinced myself this won't break existing postgresql installs, so if you migrate from sqlite to postgresql and then upgrade to this module that would be awesome. Please note that depending on if you're running master or not you may have to modify the database.socket value as it could either be in /tmp or /run/postgresql.

[aanderse]

@artemist If you're available for review and/or testing I would appreciate.

[nixos-discourse]

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/3

[Ma27]

Deployed the patches onto a gitea instance with postgresql running and an existing data set and didn't encounter any issues, so 👍

[aanderse]

@GrahamcOfBorg test gitea.mysql gitea.postgres gitea.sqlite

[aanderse]

@Ma27 if you feel I've adequately addressed your most recent comments please feel free to merge as after you reported back on testing I feel this is ready.

Thanks everyone!

[Ma27]

Had a final look, with passing tests and having this tested with actual data I guess this should be fine now 👍

[Ma27]

@aanderse thanks!

[aanderse]

Thanks to all for feedback, review, and testing!

[pvgoran]

This broke my configuration: I have services.gitea.user = "git", but services.gitea.database.user is at its default value (gitea), and services.gitea.database.createDatabase = true.

After upgrade to 19.09, it says "services.gitea.database.user must match services.gitea.user if the database is to be automatically provisioned". :(

I know I'm late to the party, but... For me, this is clearly a regression. And this change wasn't even mentioned in release notes.

Also, this change overloads the createDatabase option: now it means not only "create database", but "use peer authentication" as well. This is the root of my problem, and it looks like a bad idea overall.

[aanderse]

@pvgoran sorry for the trouble 😞 Did you end up resolving this? For what reason are you using a local database but not socket authentication? postgresql or mysql? Can you provide more details about the problem you are having and what your constraints are?

[pvgoran]

@aanderse For now, I resolved this by using an old version of the module. Which is hardly an acceptable long-term solution.

I'm using the local database (postgresql) because gitea's system user is different from its database user. The system user is git (to maintain the familiar format of Git URLs which start with git@), and the database user is gitea (because the database's purpose and structure is specific to gitea, it's not about git in general).

I don't know what other details I can provide. My problem is simple: the configuration that worked before is (as of release 19.09) not accepted by NixOS anymore.

[aanderse]

@pvgoran that makes sense. While the documentation wasn't enough to explain the changes required in your case, fortunately the "fix" is quite trivial so we can get you back to using the most recent version of the module.

I assume all you need to do is set createDatabase to false and everything else should remain the same. Your config should likely look like this to work, similar to what you currently have:

database = {
  type = "postgres";
  name = "gitea";
  user = "git";
  passwordFile = ...;
  createDatabase = false;
};

Since you have an existing install the createDatabase is no longer required as the database has already been created.

Please let me know if this works out or not.

[pvgoran]

@aanderse I know that I can set createDatabase to false, and my existing instance will continue to work as it is. But this is just a hack, not a solution to the problem. NixOS is about reproducibility: I should be able to take the same module configuration, deploy it to another machine, and still get a working instance; if I can't, I'm back to imperative style of configuration.

[aanderse]

@pvgoran yes, you are correct... createDatabase is not overly reproducible and introduces a fair bit of state. Calling it a hack is fair. If you want reproducibility manage your databases yourself outside of NixOS.

[pvgoran]

That's the opposite of what I meant.

createDatabase is a fine option, it allows me to get a working instance, regardless of whether it's an old machine (with an existing database) or a new machine. On the contrary, setting createDatabase = false would break this reproducibility; that's why your advice of setting createDatabase = false is not a solution.

[aanderse]

Please keep in mind createDatabase is an option that trades reproducibility for convenience. Since NixOS can't manage the state of a database server (users, tables, databases) you end up with idempotent statements which are merely best efforts to ensure a database is how you described it to be in your configuration.nix. If you're relying on this option for reproducibility, not convenience, you're understanding of its intention is incorrect. This is a common problem which demonstrates why options like this are so dangerous.

[pvgoran]

Well, I don't want to argue about the meaning of words, it's counterproductive.

The problem is, the configuration user = "git"; database.user = "gitea"; database.createDatabase = true; makes sense, it worked before (with all its flaws that I'm well aware of), and it doesn't work now.

The requirement for user to be the same as database.user is just an implementation detail, it isn't logically necessary. So I say that it shouldn't be forced on users (who have a previously working configuration) just because it allows to simplify the implementation.

[aanderse]

@pvgoran I don't mean to come off as troublesome, but I really can't stress enough how you have been mislead by this module to think it was reproducible. As soon as you set createDatabase to true all reproducibility went out the window. Please do not rely on options like createDatabase for anything more than mere convenience.

For example, take this config:

database = {
  passwordFile = pkgs.writeText "password" "first-password";
  createDatabase = true;
};

And replace it with this configuration:

database = {
  passwordFile = pkgs.writeText "password" "second-password";
  createDatabase = true;
};

You have lost all reproducibility. I really really really want to stress these types of options lead to systems which are not reproducible. Documenting this and trying to bring a better understanding of the issue to the attention of our users and developers has been something @grahamc, @flokli and myself have been discussing over the past few months.

See

nixpkgs/nixos/modules/services/misc/gitea.nix

Line 336 in f53bdf3

if ! test -e "${cfg.stateDir}/db-created"; then

for specifics if required.

[Ma27]

What's wrong with documenting this change in the 19.09 release-notes? Breaking changes do happen.

[pvgoran]

@aanderse If we are talking about reproducibility in the strict sense, then NixOS, being a stateful Linux installation, can't really be reproducible (well, except maybe if special care is taken to ensure that all state is destroyed on activation; I don't think this is easy to achieve). It's not a Docker container which is designed to work statelessly (at least in some common cases).

I'm fully aware of the consequences of the scenario that you described. It's not anything new, it exists on the system level when mutableUsers = true, and it's actually kind of expected.

On the other hand, there are properties of the instance that are normally preserved during activation, and reproduced when deploying the same configuration to a new machine (even if state is different). In many cases, and in this case too, these properties boil down to "the system is running the particular service, and this service is usable". I may be wrong, but having the ability to maintain such invariant properties is one of the strong points of NixOS's declarative configuration system. And with this change, this ability, for this particular case, was lost.

[pvgoran]

@Ma27 Even if breaking changes happen, they are hardly desirable. If there is no way around them (which is not the case here), it would be better to first deprecate a feature, and only remove it in the next release.

[flokli]

This derailed a bit from @pvgoran 's the initial report, so let me try to explain what happened:

Before 615f8b8982b26bbb1a3e202be020d27a9f205c62

The gitea module had a pre-startup script that was checking for a small text file db-created. If that didn't exist, it created a database user and database with how the nixos configuration was configured at that point in time.
Afterwards, it created the db-created file to not do that next time.
In case of mysql, it didn't set up a database at all, but crashed during startup

After 615f8b8982b26bbb1a3e202be020d27a9f205c62

The gitea module offloaded the task of creating a database and username in case they don't already exist to the services.{mysql,postgresql}.ensure{Databases,Users}

On every bootup, before and after above commit

Gitea assembles the config based from the current NixOS configuration and annotates it with the currently configured database parameters.

With the ensure* options, things like password changes won't get applied to the database, only to the gitea configuration, as the semantics of ensureUser don't update passwords (they only create if it doesn't exist, as they can't distinguished admin changes from configuration changes)
Likewise, the previous postgresql initialization code only ran once and never updated database parameters, as the db-created file existed.

Switching to 19.09

In your case, you only saw an evaluation error, meaning switching your configuration bailed out early, as the NixOS module system noticed a configuration problem in your configuration, as you had createDatabase set to true, but differing database and system users (and the automatic database module doesn't cover that):

"services.gitea.database.user must match services.gitea.user if the database is to be automatically provisioned"

In that case, the system prompted to do this manually, by setting createDatabase to false and doing it manually.
Showing these kind of errors during evaluation is actually preferrable over throwing you into a broken-configured gitea installation after switching to the new system.

Systems always get tricky when there's state involved. Database initialization is even more tricky, as we don't want to risk accidentially creating users' data.
#84146 brings up some of the points, and why database and database user creation needs to be more explicit, and better documented.

We should indeed have added a release note to 19.09, and could still backport it.
What do you think of the following release note entry?

17e780d

[pvgoran]

@flokli Thanks for writing a summary, this will probably help someone to understand the situation.

Of course, an assertion is better than silently producing a broken configuration (or, even worse, a configuration that is not broken immediately, but will fail on a fresh instance).

We should indeed have added a release note to 19.09, and could still backport it.
What do you think of the following release note entry?

17e780d

Instead of this, I would rather see the commit reverted altogether, so that the functionality I'm using was restored, instead of being properly documented as removed.

Or, perhaps even better, the createDatabase option could be split into options createDatabasePeerAccess and createDatabasePasswordAccess, whereas createDatabase could declared as a deprecated alias of createDatabasePasswordAccess.

[flokli]

@pvgoran it actually made sense to aggregate this logic into the database-specific nixos module by then. Looking at the diff from #84146, it'll be made more explicit in the gitea module: https://github.com/NixOS/nixpkgs/pull/84146/files#diff-ca28bfecf4338f2def6526c29a1ddc36R299.

Adding all the new options, and only adding them for gitea (and for every other module using some sort of databases) probably isn't desirable. If your initial concern was not having to amend config, adding these won't help. I propose to provide a "somewhat good default" for most users, and if you deviate from the default config, you're expected to do the setup by yourself. This descreases the possibilities for pitfalls considerably.

[pvgoran]

@pvgoran it actually made sense to aggregate this logic into the database-specific nixos module by then. Looking at the diff from #84146, it'll be made more explicit in the gitea module: https://github.com/NixOS/nixpkgs/pull/84146/files#diff-ca28bfecf4338f2def6526c29a1ddc36R299.

@flokli Of course it made sense. I see how it simplifies the module code. The only problem is that the functionality that I happen to use was sacrificed in the process.

Adding all the new options, and only adding them for gitea (and for every other module using some sort of databases) probably isn't desirable. If your initial concern was not having to amend config, adding these won't help.

It's not about having to change the configuration text, it's about having to change the configuration meaning. Now I either have to change the Git URLs to use gitea@ instead of git@, or change postgres' database user from gitea to git, or use the old module version, or write the user creation code myself. All of these choices have noticeable drawbacks; if I could just use a different option name while keeping the meaning of configuration, it would not be a problem.

I propose to provide a "somewhat good default" for most users, and if you deviate from the default config, you're expected to do the setup by yourself. This descreases the possibilities for pitfalls considerably.

Well, I'd argue that using the same name for Gitea's system user and for Gitea's database user is a poor default. I explained the reasons for using different user names in this comment, and I think that these reasons are quite sensible.

[flokli]

@pvgoran it has been done, and it doesn't make any sense to change it back now either.

The few users that used gitea and had nonstandard usernames would already have migrated when switching to 19.09. We can add a release note entry if there's still people late at the party migrating now to 19.09, but I see zero benefit adding error-prone migration code half a year after most people already migrated.

[pvgoran]

@flokli It does make sense to change it back: like I said, the current defaults are poor.

But I understand that I'm fighting a lost battle here: even if this change was a mistake, noone likes to admit mistakes (let alone correct them), if making this mistake was (and continues to be) convenient.

[aanderse]

@pvgoran I assure you that after multiple lengthy video calls, chats, and time spent contemplating the issue neither @flokli nor myself view this change as a mistake and are having a hard time admitting so. We simply have a different opinion than yours.

I'd also like you to know I have spent more than a non trivial amount of time discussing the discussion in this thread with various community members and I don't feel "good" about where we are at. While I hold my position I really am regretting how this thread and change make you feel. I don't intend to trivialize your opinion.

After spending some more time understanding your position and goals I wonder if this solution is adequate for you:

services.postgresql.initialScript = ''
# put this in a secret file instead of your configuration.nix
select 'create role gitea with encrypted password s3cr3t nocreatedb nocreaterole login' where not exists (select from pg_roles where rolname = 'gitea')\gexec
select 'create database gitea owner gitea' where not exists (select from pg_database where datname = 'gitea')\gexec
'';

It has the downside of being more verbose, but it does satisfy other criteria for a solution you mentioned so far. To be fair NixOS gives you the tool to create your own system user git but doesn't create it for you in this module because you are doing something "custom" - the same logic could be applied here.

[pvgoran]

@aanderse I appreciate you giving it some thought, and I feel uneasy about occupying attention of several people with this subject.

To be fair NixOS gives you the tool to create your own system user git but doesn't create it for you in this module ...

What do you mean here? Is there a way to create the user git in addition to gitea, and having it work with gitea's repositories and SSH keys?

After spending some more time understanding your position and goals I wonder if this solution is adequate for you: ...

It doesn't seem good. It's non-modular (it "appropriates" the services.postgresql.initialScript option and thus kind of violates encapsulation, and it's impossible to use several scripts like this), and I'll need to write code that would generate this file. Also, this solution is too "hacky" and would leave too much noise in the configuration files.

So I'll probably end up writing a generic module that would generate systemd service(s) to (1) generate a random password file or copy it from somewhere, (2) create a postgres user and assign the password to it, (3) make this file accessible to a specified user.