Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nixos/modules/module-list.nix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without an accompanying section in the NixOS manual, you're kind of leaving me guessing here, so I hope my comments make sense.

Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,8 @@
./system/boot/timesyncd.nix
./system/boot/tmp.nix
./system/boot/uvesafb.nix
./system/vars/options.nix
./system/vars/on-machine-backend.nix
./system/etc/etc-activation.nix
./tasks/auto-upgrade.nix
./tasks/bcache.nix
Expand Down
18 changes: 18 additions & 0 deletions nixos/modules/services/networking/syncthing.nix
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,24 @@ in {

config = mkIf cfg.enable {

vars.generators.syncthing = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding the glue between this generator and the syncthing service. How do the files created by this generator actually get wired up into the running service?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope I am not misreading your question, but the pem files are seemingly simply used to set conventional nixos options such as on line 639-641 in this example.

The device id in syncthing.pub does not seem to be used in the example, but only stored. Presumably for outside use just as scripted setups of other devices to sync with.

files."cert.pem" = {};
files."key.pem" = {};
files."syncthing.pub".secret = false;
runtimeInputs = [
pkgs.coreutils
pkgs.gnugrep
pkgs.syncthing
];
script = ''
syncthing generate --config "$out"
< "$out"/config.xml grep -oP '(?<=<device id=")[^"]+' | uniq > "$out"/syncthing.pub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd feel better about this if we somehow asserted that the result of this is exactly 1 ID. I imagine things would get weird if this started returning multiple IDs or no ID at all.

Or, perhaps we could ask upstream syncthing if they'd be open to a change to syncthing generate to persist the id somewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
< "$out"/config.xml grep -oP '(?<=<device id=")[^"]+' | uniq > "$out"/syncthing.pub
< "$out"/config.xml grep -m 1 -oP '(?<=<device id=")[^"]+' > "$out"/syncthing.pub

should guarantee that it can yield no more than 1 id (uniq could still yield multiple, different ids). Due to the regex match not applying to empty strings, grep will already return rc 1 if no id is found, so the set -e in the reference backend should fetch it.

Might be a good idea to explicitly define that scripts are supposed to be run with set -e. :)

'';
};

config.syncthing.cert = lib.mkIf (config.vars.generators.syncthing.files."cert.pem".path != null) config.vars.generators.syncthing.files."cert.pem".path;
config.syncthing.key = lib.mkIf (config.vars.generators.syncthing.files."key.pem".path != null) config.vars.generators.syncthing.files."key.pem".path;

networking.firewall = mkIf cfg.openDefaultPorts {
allowedTCPPorts = [ 22000 ];
allowedUDPPorts = [ 21027 22000 ];
Expand Down
140 changes: 140 additions & 0 deletions nixos/modules/system/vars/on-machine-backend.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# we use this vars backend as an example backend.
# this generates a script which creates the values at the expected path.
# this script has to be run manually (I guess after updating the system) to generate the required vars.
{
pkgs,
lib,
config,
...
}:
let
cfg = config.vars.settings.on-machine;
sortedGenerators =
(lib.toposort (a: b: builtins.elem a.name b.dependencies) (lib.attrValues config.vars.generators))
.result;

promptCmd = {
hidden = "read -sr prompt_value";
line = "read -r prompt_value";
multiline = ''
echo 'press control-d to finish'
prompt_value=$(cat)
'';
};
generate-vars = pkgs.writeShellApplication {
Copy link
Member

@roberth roberth Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this script may be run on the deployer host, which can be a different platform.

Suggested change
generate-vars = pkgs.writeShellApplication {
generate-vars = deployerPkgs: deployerPkgs.writeShellApplication {

It is then up to the caller to pass a suitable pkgs instance that works for the deployer host.

EDIT: this is of course not the complete solution because the script contents still refer to pkgs. Use a separate file and callPackage to avoid a footgun.

Suggested change
generate-vars = pkgs.writeShellApplication {
generate-vars = { deployerPkgs }: deployerPkgs.callPackage ./generate-vars.nix { inherit cfg; };

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a test case, you could add aarch64-darwin to this and run the checks on darwin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually we overwrite pkgs of the system where the vars are defined if the caller architecture is different from the target systems architecture, since this also supports string context references to pkgs and the vars definitons want to live next to the service definitions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend against that. It blurs the line too much.

  • You'll be evaluating a NixOS configuration that "runs on" macOS, which is unexpected. It may work out for you anyway, thanks to laziness, but if vars calls someone's code that reads something from the rest of the NixOS config (accidentally or otherwise), they'll get strange errors like "util-linux is not supported on darwin" or worse.
  • Overriding is a code smell.
  • Overriding is in general less efficient than evaluating the right thing in one go, especially if it's a NixOS config.

Explicit is good. By making this a proper parameter, readers and module authors have at least an idea of what's going on, and you don't rely on the overriding magic to never hit them in the face.

name = "generate-vars";
text = ''
set -efuo pipefail

PATH=${lib.makeBinPath [ pkgs.coreutils ]}

# make the output directory overridable
OUT_DIR=''${OUT_DIR:-${cfg.fileLocation}}

# check if all files are present or all files are missing
# if not, they are in an inconsistent state and we bail out
${lib.concatMapStringsSep "\n" (gen: ''
all_files_missing=true
all_files_present=true
${lib.concatMapStringsSep "\n" (file: ''
if test -e ${lib.escapeShellArg file.path} ; then
all_files_missing=false
else
all_files_present=false
fi
'') (lib.attrValues gen.files)}

if [ $all_files_missing = false ] && [ $all_files_present = false ] ; then
echo "Inconsistent state for generator: {gen.name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo "Inconsistent state for generator: {gen.name}"
echo "Inconsistent state for generator: ${gen.name}"

(unless this some ultra-obscure bash syntax i am not aware of ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sadly think that "Inconsistent state for generator: ${gen.name}" is a pretty bad error message here. I think it should at least try to communicate to the user what exactly is wrong, i.e. which file was found / not found. Ideally it would communicate something actionable as well, i.e:

You can try purging ''${OUT_DIR:-${cfg.fileLocation}}, after making sure it contains nothing you want to keep

I understand that it's annoying to implement in bash, but I am not sure that bash is the right choice of language here in the first place tbh.

exit 1
fi
if [ $all_files_present = true ] ; then
echo "All secrets for ${gen.name} are present"
elif [ $all_files_missing = true ] ; then

# prompts
prompts=$(mktemp -d)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should slap umask 077 in places...

trap 'rm -rf $prompts' EXIT
export prompts
mkdir -p "$prompts"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mktemp -d does the mkdiring.

${lib.concatMapStringsSep "\n" (prompt: ''
echo ${lib.escapeShellArg prompt.description}
${promptCmd.${prompt.type}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API smells a bit weird to me: if I'm reading this correctly, we expect promptCmd to be a snippet of shell code that writes to a $prompt_value variable. Why not make it some command that prints a result to stdout (and exits non zero if something went wrong)?

echo -n "$prompt_value" > "$prompts"/${prompt.name}
'') (lib.attrValues gen.prompts)}
echo "Generating vars for ${gen.name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need shell escaping? Could gen.name contain a double quote? I think so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not double quotes as currently all names have the type lib.types.strMatching "[a-zA-Z0-9:_\\.-]*", but ending a name with \ would break this line. Quoting using ' would alleviate this, but for robustness sake, escaping most injected values seems preferable to me.


# dependencies
in=$(mktemp -d)
trap 'rm -rf $in' EXIT
export in
mkdir -p "$in"
${lib.concatMapStringsSep "\n" (input: ''
mkdir -p "$in"/${input}
${lib.concatMapStringsSep "\n" (file: ''
cp "$OUT_DIR"/${
if file.secret then "secret" else "public"
}/${input}/${file.name} "$in"/${input}/${file.name}
'') (lib.attrValues config.vars.generators.${input}.files)}
'') gen.dependencies}

# outputs
out=$(mktemp -d)
trap 'rm -rf $out' EXIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traps override each other. Confer (trap 'echo a' EXIT; trap 'echo b' EXIT).

The way it stands, unless the script exits prematurely, only the very final 'rm -rf $out' gets executed, leaving all the other $outs, and all $ins and $promptss unremoved. This issue becomes apparent when declaring variables readonly.

One possible solution would be to spawn a subshell for each generator, e.g.:

...
elif [ $all_files_missing = true ] ; then
  (
    prompts=$(mktemp -d)
    in=$(mktemp -d)
    out=$(mktemp -d)
    readonly prompts in out # optional assertion
    trap 'rm -fR "$prompts" "$in" "$out"' EXIT
    export prompts in out
    ...
  )
fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export out
mkdir -p "$out"

(
# prepare PATH
unset PATH
${lib.optionalString (gen.runtimeInputs != [ ]) ''
PATH=${lib.makeBinPath gen.runtimeInputs}
export PATH
''}

# actually run the generator
${gen.script}
)

# check if all files got generated
${lib.concatMapStringsSep "\n" (file: ''
if ! test -e "$out"/${file.name} ; then
echo 'generator ${gen.name} failed to generate ${file.name}'
exit 1
fi
'') (lib.attrValues gen.files)}

# move the files to the correct location
${lib.concatMapStringsSep "\n" (file: ''
OUT_FILE="$OUT_DIR"/${if file.secret then "secret" else "public"}/${file.generator}/${file.name}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow reuse vars.settings.fileModule here? I see how we do want OUT_DIR to be overridable, but reconstructing this path it multiple times feels wrong to me.

mkdir -p "$(dirname "$OUT_FILE")"
mv "$out"/${file.name} "$OUT_FILE"
'') (lib.attrValues gen.files)}
rm -rf "$out"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is unnecessary because of the trap 'rm -rf $out' EXIT up above, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, the trap only runs on exit, it's better to clean up the files if they are no longer needed earlier. the trap is just in case the script exists unexpectedly

fi
'') sortedGenerators}
'';
};
in
{
options.vars.settings.on-machine = {
enable = lib.mkEnableOption "Enable on-machine vars backend";
fileLocation = lib.mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is a weird name for me. It sounds like it's for the location of a file, but it's actually for the location of a directory of files. Perhaps varsLocation or varFilesLocation?

type = lib.types.str;
default = "/etc/vars";
};
};
config = lib.mkIf cfg.enable {
vars.settings.fileModule = file: {
path =
if file.config.secret then
"${cfg.fileLocation}/secret/${file.config.generator}/${file.config.name}"
else
"${cfg.fileLocation}/public/${file.config.generator}/${file.config.name}";
};
environment.systemPackages = [
generate-vars
];
system.build.generate-vars = generate-vars;
};
}
185 changes: 185 additions & 0 deletions nixos/modules/system/vars/options.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
{
lib,
config,
pkgs,
...
}:
{
options.vars = {
settings = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not RFC 42. What's the thought behind this name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it's the provider-specific settings, similar in spirit to how SelfHostBlocks does it, except here it's (IMO correctly) in terms of modules.

fileModule = lib.mkOption {
Copy link
Member

@roberth roberth Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
fileModule = lib.mkOption {
allFiles = lib.mkOption {

type = lib.types.deferredModule;
internal = true;
description = ''
A module to be imported in every vars.files.<name> submodule.
Used by backends to define the `path` attribute.

Takes the file as an argument and returns maybe an attrset which should at least contain the `path` attribute.
Can be used to set other file attributes as well, like `value`.
'';
default = { };
};
};
generators = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contains both generators (executables), as well as their target files (calls of those executables).
Since those are in a 1:many relationship (potentially), this part of the option tree should be named after the "target files" instead (or whichever term those are described with).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you have an insofar unnamed domain entity for groups of files that are generated together, and perhaps its name should be put here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, the idea was, that one generator can create many files, so the files are below the generators in the hierarchy. Files belong to a generator in that they need to be generated by one, not sure where else in the option tree they could live

description = ''
A set of generators that can be used to generate files.
Generators are scripts that produce files based on the values of other generators and user input.
Each generator is expected to produce a set of files under a directory.
'';
default = { };
type = lib.types.attrsOf (
lib.types.submodule (generator: {
options = {
name = lib.mkOption {
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
description = ''
The name of the generator.
This name will be used to refer to the generator in other generators.
'';
readOnly = true;
default = generator.config._module.args.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default = generator.config._module.args.name;
default = generator.name;

defaultText = "Name of the generator";
};

dependencies = lib.mkOption {
description = ''
A list of other generators that this generator depends on.
The output values of these generators will be available to the generator script as files.
For example, the file 'file1' of a dependency named 'dep1' will be available via $in/dep1/file1.
'';
type = lib.types.listOf lib.types.str;
default = [ ];
};
files = lib.mkOption {
description = ''
A set of files to generate.
The generator 'script' is expected to produce exactly these files under $out.
'';
defaultText = "attrs of files";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultText = "attrs of files";
defaultText = literalMD "attrs of files";

Not sure.

type = lib.types.attrsOf (
lib.types.submodule (file: {
imports = [
config.vars.settings.fileModule
];
options = {
name = lib.mkOption {
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
description = ''
name of the generated file
'';
readOnly = true;
default = file.config._module.args.name;
Comment on lines +60 to +71
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lib.types.submodule (file: {
imports = [
config.vars.settings.fileModule
];
options = {
name = lib.mkOption {
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
description = ''
name of the generated file
'';
readOnly = true;
default = file.config._module.args.name;
lib.types.submodule ({ name, ... }: {
imports = [
config.vars.settings.fileModule
];
options = {
name = lib.mkOption {
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
description = ''
name of the generated file
'';
readOnly = true;
default = name;

This seems to be a much more idiomatic way of writing it. That piece of code had confused the hell out of me, but maybe I'm just used to different things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only changing it to default = file.name would work to the same effect, but I still find it unusual.

defaultText = "Name of the file";
};
generator = lib.mkOption {
description = ''
The generator that produces the file.
This is the name of another generator.
'';
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
readOnly = true;
internal = true;
default = generator.config.name;
defaultText = "Name of the generator";
};
deploy = lib.mkOption {
description = ''
Whether the file should be deployed to the target machine.

Disable this if the generated file is only used as an input to other generators.
'';
type = lib.types.bool;
default = true;
};
secret = lib.mkOption {
description = ''
Whether the file should be treated as a secret.
'';
type = lib.types.bool;
default = true;
};
path = lib.mkOption {
description = ''
The path to the file containing the content of the generated value.
This will be set automatically
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uber nit: missing period at the end of this sentence?

'';
type = lib.types.nullOr lib.types.str;
default = null;
};
};
})
);
};
prompts = lib.mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this API, is there any guarantee about the order the user is prompted? I guess I'd expect iteration over attrsets to be deterministic in nix, but would it be preferable to have an API that gives the developer control over the order of the prompts? Ideas:

  • change this to an array (perhaps not module option merge friendly)
  • add a z-index sort of number
  • allowing prompts to depend on other prompts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attrValues always generates the prompts in lexicographical order. so it should be the same every time (at least for implementation that take this from bash). Prompts just take an input and output a string. so there is no real need to have a fixed order?

description = ''
A set of prompts to ask the user for values.
Prompts are available to the generator script as files.
For example, a prompt named 'prompt1' will be available via $prompts/prompt1
'';
default = { };
Comment on lines +113 to +119
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a value option or something similar to each prompt? I would like to be able to pre-define the value of the prompt to ensure that a user can always opt-in to a purely declarative configuration.

Btw, I currently don't understand when an author of a nixos module would decide to utilize a prompt instead of a generated random value. If I am writing a generator for my personal configuration, I can understand that I might want to use prompt for certain things - but when would I want to use a prompt in an upstream module and force interactivity on every user?

I'm just a little worried right now that introducing prompts without a way to declaratively set their value will sooner or later result in some generators being added to nixpkgs which depend on some form of interactivity. Therefore I'd like to have a way to specify the value of a prompt ahead of time, so the generation script can then just copy the specified file to prompts/$name instead of asking the user a question.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we mainly use prompts for API keys, which we usually don't want to leak into the config. I guess we can think about pre declaring things, this would also make the testing code a bit easier which I worked on last time I touched that code. But this increases the surface every backend implementation has to take care of.

type = lib.types.attrsOf (
lib.types.submodule (prompt: {
options = {
name = lib.mkOption {
description = ''
The name of the prompt.
This name will be used to refer to the prompt in the generator script.
'';
type = lib.types.strMatching "[a-zA-Z0-9:_\\.-]*";
default = prompt.config._module.args.name;
defaultText = "Name of the prompt";
};
description = lib.mkOption {
description = ''
The description of the prompted value
'';
type = lib.types.str;
example = "SSH private key";
default = prompt.config._module.args.name;
defaultText = "Name of the prompt";
};
type = lib.mkOption {
description = ''
The input type of the prompt.
The following types are available:
- hidden: A hidden text (e.g. password)
- line: A single line of text
- multiline: A multiline text
'';
type = lib.types.enum [
"hidden"
"line"
"multiline"
];
default = "line";
};
};
})
);
};
runtimeInputs = lib.mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (feel free to not make any change here): This option smells a bit off to me: nix makes it so easy for people to build their own scripts/binaries with all their own dependencies: why are we doing this for them?

Is this just to make things a little more ergonomic for users? For example, this might save people from an extra layer or two of indentation that would be introduced by using a trivial builder (such as writerShellApplication).

description = ''
A list of packages that the generator script requires.
These packages will be available in the PATH when the script is run.
'';
type = lib.types.listOf lib.types.package;
default = [ pkgs.coreutils ];
};
script = lib.mkOption {
description = ''
The script to run to generate the files.
The script will be run with the following environment variables:
- $in: The directory containing the output values of all declared dependencies
- $out: The output directory to put the generated files
- $prompts: The directory containing the prompted values as files
The script should produce the files specified in the 'files' attribute under $out.
'';
type = lib.types.either lib.types.str lib.types.path;
default = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an odd default. Should we set it to null to help people remember to actually define a script?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scripts are optional in our outside implementation, because in the case that there is prompt but no script, the prompt automatically gets persisted as a secret file (which can be referenced again) I removed this shortcut for now, to have less complexity. But we could add it back at a later point maybe

};
};
})
);
};
};
}
1 change: 1 addition & 0 deletions nixos/tests/all-tests.nix
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,7 @@ in {
user-home-mode = handleTest ./user-home-mode.nix {};
ustreamer = handleTest ./ustreamer.nix {};
uwsgi = handleTest ./uwsgi.nix {};
vars = handleTest ./vars.nix {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This placed seemed pretty ordered until this line got added. Or is all-tests.nix also just kindasorted like module-list.nix :)

v2ray = handleTest ./v2ray.nix {};
varnish60 = handleTest ./varnish.nix { package = pkgs.varnish60; };
varnish75 = handleTest ./varnish.nix { package = pkgs.varnish75; };
Expand Down
Loading
Loading