Don't copy nginx/httpd vhost options to service modules using nginx

[Ma27]

Describe the bug

For instance services.jirafeau and services.dolibarr expose all of these options below services.X.nginx. There are a lot more cases that are trivial to find.

I think this is a bad practice and has the following downsides:

• every nixos module and every option increases the evaluation time. In this case for no good reason. I think this is a cheap way to save a bit of this.
• when searching for nginx options in configuration.nix(8), you have to skip numerous copies of the actual module until you're actually down at nginx. This is annoying. That said, you can't just use one of the other modules to check which virtualHost options. E.g. dolibarr has altered the defaults of forceSSL and enableACME, so you can't trust other options that look like nginx options in the man page.
• some of the options don't even make any sense: for instance no virtual host that reverse proxies a service should need globalRedirect.

Since I have an nginx bias, all of the above is written with focus on nginx. The same applies to modules exposing httpd's options, of course.

Expected "behavior"

IMHO this is a symptom of not understanding how merging inside the module system works (i.e. you can just define the relevant portions in your service.nix). Given the issues associated with this I'd suggest to

• rip all of the options out and give a concise eval-error describing what to do instead (i.e. services.nginx.virtualHosts.${config.services.foo.hostName}.X).
• provide examples in the option's descriptions and perhaps the manual on how to do it properly.
• mention somewhere in e.g. the contribution docs that this is considered an anti-pattern.

cc @infinisil @roberth @aanderse @Guekka @NetaliDev @vtuan10 @melvyn2 @RaitoBezarius for opinions.

---

Add a :+1: reaction to issues you find important.

[Guekka]

I agree with this proposal. The current approach of exposing numerous options under services.X.nginx can indeed be confusing, especially for new users. I have experienced this confusion myself when searching for options while discovering NixOS.

However, we would need strict rules to get a clear documentation. Otherwise, we could end up with users writing a hardcoded virtualHosts:

services.nextcloud.enable = true;
services.nginx.virtualHosts.nextcloud = {}; // ...

This would obviously break if upstream changed the name of the virtual host.

---

On a broader scale, this pattern can be found very often within nixpkgs. Be it with nginx, redis, postgres... Each module can enable them and wrap their options or not.

Modules have no strict rules about what they can do or not. Why do some of them enable nginx/postgres/... for the user, whereas others require the user to enable these services themself? Usually, this comes down to the module requiring a specific config, but this is not always the case.

Ultimately, I'm wondering if we should require (through automation?) the documentation of each module to mention the other modules it configures. Something like

The nextcloud module configures nginx and redis. To modify default settings, use services.nginx.virtualHosts.${config.services.nextcloud.hostName} and services.redis.servers.${config.services.nextcloud.hostName}.

---

PS: out of curiosity @Ma27, any reason for pinging me, or did you just pick a random maintainer?

[roberth]

• every option increases the evaluation time

Not wrong as a general rule of thumb, but it does not apply for options in un-evaluated submodules, and also not when _module.check = false. We might not want the latter, so here's a counterexample showing the prior.

--- a/nixos/modules/services/web-apps/jirafeau.nix
+++ b/nixos/modules/services/web-apps/jirafeau.nix
@@ -79,7 +79,7 @@ in
     };

     nginxConfig = mkOption {
-      type = types.submodule
+      type = throw "" types.submodule
         (import ../web-servers/nginx/vhost-options.nix { inherit config lib; });
       default = {};
       example = literalExpression ''

The above does break documentation, whose evaluation is cached anyway, so we can ignore that for the purpose of the example.

nix-repl> :b (nixos { fileSystems."/".device = "x"; boot.loader.grub.enable = false; documentation.enable = false; }).toplevel
...
This derivation produced the following outputs:
...

I could even build with throw "" mkOption there, but I don't want to give a false impression; don't have a good rule of thumb for when that works. Option tree handling is not optimal yet; see https://github.com/NixOS/nixpkgs/issues/291536.

---

Regardless, this pattern is wrong for a different reason. Option types are not guaranteed to be idempotent when their computed option values are reinterpreted as definitions (and then resolved and merged for the final nginx config). In fact it's easy to find an example where it goes wrong: all properties are lost, such as mkForce, mkBefore, etc. The way to preserve all that and only merge once is to use types.deferredModule (sorry for the bad fragment; we should stop using definition lists imo), or such types as attrsOf (deferredModule), if needed.

This solves the semantic issue, and gets rid of the duplicated option docs.

(OT: you could recover them with deferredModuleWith { staticModules } if you ever needed to, and if it's ok to import that set of modules)

---

• some of the options don't even make any sense

Impossible to prevent in general, but absolutely agreed for docs. deferredModule will only render e.g. nginxConfig, but no sub-options.

It does make sense to have little convenience options for common modifications, although that could become a bit too arbitrary and bloated too. Maintainers will have to strike a balance.

[aanderse]

sounds good @Ma27

i especially agree strongly with the comments about documentation!

thanks for making this issue

[Ma27]

PS: out of curiosity @Ma27, any reason for pinging me, or did you just pick a random maintainer?

I picked most of the people I could trace back with git blame who seemed to originally add this. I wanted to make sure that I'm not missing reasons to leave it that way.

However, we would need strict rules to get a clear documentation. Otherwise, we could end up with users writing a hardcoded virtualHosts:

Isn't the key for a virtualHost usually the domain it's served under? My suggestion is to instruct people to do something like services.nginx.virtualHosts.${config.services.nextcloud.hostName} = { ... };. That way you won't need to worry about that.

Be it with nginx, redis, postgres... Each module can enable them and wrap their options or not.

Sure, modules can touch other modules and to a certain degree it's a good thing. For instance, for PHP applications you need some kind of reverse proxy that serves assets and forwards the correct prefixes to FPM, so it makes sense to configure it[1]. It usually makes sense to wrap this behind some kind of services.foo.postgresql.enable option to allow people to build environments where reverse-proxy/database/whatever lives on a different node.

That's not my point though: some kind of wrapping tailored to the service is OK. It's not OK though to expose every single option of a different service module (as discussed above). Sooner or later you'll need to touch these modules directly.

The nextcloud module configures nginx and redis. To modify default settings, use services.nginx.virtualHosts.${config.services.nextcloud.hostName} and services.redis.servers.${config.services.nextcloud.hostName}.

I think we'll need way more metadata fo rthi sto work. Because at the end of the day, nginx & redis touch other modules (i.e. systemd.services), so it needs to be defined what's actually interesting. For now, I think it's OK to expose e.g. a hostName option and document in its description how the reverse-proxy can be modified.

Option types are not guaranteed to be idempotent when their computed option values are reinterpreted as definitions (and then resolved and merged for the final nginx config). In fact it's easy to find an example where it goes wrong: all properties are lost, such as mkForce, mkBefore, etc

Good point! I think I've seen an issue related to that already, but forgot to mention it originally. Thanks for adding this!

This solves the semantic issue, and gets rid of the duplicated option docs.

I'd argue that this approach is not desirable either:

• Tools like nil get very far with providing completion, however deferredModule won't work here since there are "de-facto" no options defined below a deferredModule.
• We'd still have combinations that don't make much sense (globalRedirect comes to mind) and I don't think that this should be exposed by a service module then. By using services.nginx for advanced configuration, it's clearer IMHO that you're now touching the reverse-proxy and not the application, so you need to make sure on your own that you do the right thing.

It does make sense to have little convenience options for common modifications, although that could become a bit too arbitrary and bloated too

Agreed. Some basics (in the case discussed here e.g. enable, hostName & max "upload" size may make sense) are good to have. For everything else I'd argue it's better to "forward" users to the relevant modules (i.e. nginx in this case).

[1] I should add that I have opinions about the nginx case in particular nonetheless: https://github.com/NixOS/nixpkgs/issues/277723