The type for options.services.unbound.settings is too narrow

lierdakil commented 5 months ago

Describe the bug

The type for options.services.unbound.settings is too narrow, it precludes valid configs. Case in point,

{
config.services.unbound.settings.server.view = [{
  name = "foo";
  local-data = [
    "\"example.com 100500 IN A 1.2.3.4\""
  ];
}];
}

breaks with

A definition for option services.unbound.settings.server.view."[definition 1-entry 2]" is not of type signed integer or string or boolean or floating point number.

Overriding by something like this fixes it:

{
options.services.unbound.settings.server.view = lib.mkOption {
  type = lib.types.anything;
};
}

I'm sure there are other examples. The problem is the type definition doesn't allow nested attrsets beyond the top level, despite the rest of the code handling that just fine:

https://github.com/NixOS/nixpkgs/blob/3030f185ba6a4bf4f18b87f345f104e6a6961f34/nixos/modules/services/networking/unbound.nix#L125-L135

Unfortunately, I'm not proficient enough with nix modules to fix this properly (and by "properly" I mean use a more sensible type than anything)

Steps To Reproduce

Steps to reproduce the behavior:

Try to add views config to unbound (see silly example above)
Attempt to build the configuration, or simply deep-evaluate (e.g. with :p) config.services.unbound.settings in REPL.
Observe the failure

Expected behavior

I expect my config to work without nix getting in my way.

Screenshots

N/A

Additional context

Reproduced on d946372f5aec57986015fd001d4958a3cac74a0f, but master has the same narrow option type definition.

Notify maintainers

I couldn't find who's supposed to be maintaining the module, so here goes...

@rissson, as you've introduced the type in question
@SuperSandro2000, as you're the last one to touch the module
@ajs124, @das_j since you're listed as unbound package maintainers, thus this may (or may not) be of interest

Metadata

$ nix run nixpkgs#nix-info -- -m
 - system: `"x86_64-linux"`
 - host os: `Linux 6.1.77, NixOS, 23.11 (Tapir), 23.11.20240213.d946372`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.1`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
---

Note: channel nixpkgs are irrelevant, I'm on flakes.

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

SuperSandro2000 commented 5 months ago

We probably need to add listOf (attrsOf validSettingsPrimitiveTypes) to https://github.com/NixOS/nixpkgs/blob/3030f185ba6a4bf4f18b87f345f104e6a6961f34/nixos/modules/services/networking/unbound.nix#L131

lierdakil commented 5 months ago

I think that approach will inevitably have us playing whack-a-mole with edge cases.

In theory, I would suggest something to the tune of

  options.services.unbound.settings = lib.mkOption {
    type = with lib.types;
      let validTypes = oneOf [ int str bool float (listOf validTypes) (lazyAttrsOf validTypes)];
      in attrsOf validTypes;
  };

But IIUC that has potential to break stuff because lazyAttrsOf is weird.

There's a version of this approach that limits recursion to some arbitrary depth, then we could use attrsOf, but that's very ad-hoc...

lierdakil commented 5 months ago

Perhaps really something similar to anything is the right type, considering the config generator will bail if something's fatally not right https://github.com/NixOS/nixpkgs/blob/3030f185ba6a4bf4f18b87f345f104e6a6961f34/nixos/modules/services/networking/unbound.nix#L22 and, furthermore, there's now an additional safety net with unbound-checkconf.

That said, anything's merge behaviour isn't quite what we'd want, so a custom type would be preferable. I can't suggest one, though.

lierdakil commented 5 months ago

To be clear, just musing out loud, feel free to choose whatever approach seems right. As I said, I'm not too proficient with nix modules.

NixOS / nixpkgs