Contracts rfc functions#485453
Conversation
492afb4 to
0f2e463
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
thanks for getting this out! |
|
That's a fair point although TBH I don't know how much we'll need to override in practice. Also, this implementation is less prone to infinite recursion and works with the documentation which IMO makes it already better. |
| type = submodule { | ||
| options = { | ||
| content = mkOption { | ||
| type = nullOr str; |
There was a problem hiding this comment.
just noticed this looks different from the original.
without having checked the content too much yet, could you comment on this difference maybe?
like, is there a reason this would work here yet not in the original?
There was a problem hiding this comment.
Ah I think that's because in the previous PR, I stripped down the module to avoid adding unnecessary details. I just forgot to do it here 😅
There was a problem hiding this comment.
ah, guess i got maybe the wrong one - i'd wondered about equivalents here of challenge sub-thread https://github.com/NixOS/nixpkgs/pull/432529/changes#r2328714550
|
I updated the description here to show what this PR does better than the first one. |
|
given the tasks listed are all marked as done, is there anything that would need to be done here before this PR can be switched from being listed as draft to ready for review? |
|
@KiaraGrouwstra I don’t see anything but I don’t know how it works for RFCs. I still set it as ready. |
ThinkChaos
left a comment
There was a problem hiding this comment.
Thanks for your work on this. I would really like a standard pattern for configuring secrets in NixOS!
My initial reaction was that it would make sense to use the module system more. I hacked up a quick demo then. Then today read the RFC again (last time was a long time ago). I realized independently landed on almost exactly the same thing as modules-interface! I've now also read the previous PR which seems closer to that.
I think doing that way has nice benefits because the module system takes care of some things for you, like propagating _file so errors point to the right place. It also seems more idiomatic to parametrize modules via config._module.args & config directly instead of using functors.
Basically my gut feeling is "I like the other version better". I'm not saying that as opposition to this implementation, just think multiple people landing on the same design is a good sign it. I might play with it more to get an idea of the issues you raise for myself.
Anyways, with this implementation, I think it's still possible to use modules in a more idiomatic way and left comments about that.
I also have opinions about the secrets contract. In general I think it makes sense to have the contracts promise the least possible, so providers have more flexibility in how they're implemented.
Are these APIs going to be frozen as soon as this PR is merged?
If not it could be nice to add a warning when accessing them to say they're still experimental.
Also high level, since contracts are for NixOS behavior, maye only the generic code should be in lib, and the contracts themselves all be defined in NixOS modules?
| importContract = | ||
| module: | ||
| let | ||
| importedModule = import module { inherit lib; }; | ||
| in | ||
| mkContractFunctions { | ||
| inherit (importedModule) mkConsumerOptions mkProviderOptions; | ||
| } | ||
| // { | ||
| inherit (importedModule) description behaviorTest; | ||
| }; |
There was a problem hiding this comment.
This loses the module's filepath, which'll lead to confusing messages for errors generated from within the contract's code.
Along with my other suggestion of changing mkConsumer & mkProvider to return modules, you could use lib.setDefaultModuleLocation to let the module system know where the code is from, which will then be used for error messages.
There was a problem hiding this comment.
I tried setting an option wrong, specifically setting this one to true instead of a path to a file and got:
… while evaluating the option `nodes.machine.testing.hardcoded-secret.mysecret.output.path':
(stack trace truncated; use '--show-trace' to show the full, detailed trace)
error: A definition for option `nodes.machine.testing.hardcoded-secret.mysecret.output.path' is not of type `string'. Definition values:
- In `nixpkgs/nixos/modules/testing/hardcoded-secret.nix': true
It doesn't seem too bad to me. TBH after checking in nixpkgs I still have no idea how to use setDefaultModuleLocation.
There was a problem hiding this comment.
I mean if the contract itself has a bug, say during development. Not the user code.
So for instance I added config.dummy = "plz-fail"; to the secret contract's mkConsumerOptions submodule type (here), and got this error:
error: The option `nodes.machine.testing.hardcoded-secret.mysecret.input.dummy' does not exist. Definition values:
- In `/nix/store/pcsb7z45hwi5y5yhl91slz1443p3i78f-source/nixos/modules/testing/hardcoded-secret.nix': "plz-fail"
It confusingly points to nixos/modules/testing/hardcoded-secret.nix since that's what the closest _file attr contains. When the error was actually in lib/contracts/secrets.nix.
| restartUnits = mkOption { | ||
| description = '' | ||
| Systemd units to restart after the secret is updated. | ||
| ''; | ||
| type = listOf str; | ||
| default = restartUnits; | ||
| }; |
There was a problem hiding this comment.
I have a custom secrets solution, and it only decrypts secrets when the unit using them is actually running, so "restart" doesn't really make sense in that context.
I'd propose a more generic name like "dependentUnits" or something along those lines.
| This path will exist after deploying to a target host, | ||
| it is not available through the nix store. |
There was a problem hiding this comment.
Same here, "This path will exist after deploying to a target host" sounds like to large of a guarantee. I think we should constrain it to something like "will exist when the dependent units run".
There was a problem hiding this comment.
Actually, I think we should be more generic. Maybe something that doesn't use systemd units depend on the secret. This should probably list a series of commands.
Or could this be a good use case for overriding contracts? There could be a plain contract without this option, another one for systemd units and another one with a list of commands to run?
|
@ThinkChaos thank you for the review.
I cannot agree more! I like the other implementation but the shortcomings feel to me like show stoppers. If they can be solved, that would be great. I tried at the time when creating the other PR but I got to the point where I needed to modify the module system itself. But I didn't know its inner workings enough to even know what I was looking for. To be bluntly honest, I don't particularly care about a specific implementation. I care much more about getting the idea through with some properties I find important and also getting momentum going in the community.
Fully agreed too. From my experience writing Golang, that was one of the best properties of interfaces and the reason why I didn't want to introduce too many options in contracts either.
I would discourage freezing them. At least not until we have a handful of contracts with a handful of consumer and provider modules each.
Sure, that makes sense. |
|
on the secrets contract, i'm also sort of wondering still if that might go well with say |
0f2e463 to
3fb7107
Compare
ThinkChaos
left a comment
There was a problem hiding this comment.
Just a basic example of what's needed to set the generated module's _file to get errors to point to the implementation's file.
lib.setDefaultModuleLocation is the main part: it basically sets _file in the module, which is a special attr the module system uses to know where the module was originally defined.
This isn't perfect though:
_file = modulePathshould only apply to the result ofmkConsumerOptions&mkProviderOptions, whereas here it also applies to user code viainputDefaults- the module defined in this file itself, i.e. the return value of
mkConsumer, is reported being part ofmodulePath, notlib/contracts/default.nix. In this case the module is trivial, but over time that might no longer be true and lead to confusion again. mkProviderneeds a similar adjustment
(Using deferredModule gives us this for free by propagating _file from where the option was set in the original config)
| importedModule = import module { inherit lib; }; | ||
| in | ||
| mkContractFunctions { | ||
| inherit (importedModule) mkConsumerOptions mkProviderOptions; |
There was a problem hiding this comment.
| inherit (importedModule) mkConsumerOptions mkProviderOptions; | |
| modulePath = module; | |
| inherit (importedModule) mkConsumerOptions mkProviderOptions; |
|
|
||
| mkContractFunctions = | ||
| { | ||
| mkConsumerOptions, |
There was a problem hiding this comment.
| mkConsumerOptions, | |
| modulePath, | |
| mkConsumerOptions, |
| mkConsumer = inputDefaults: { | ||
| options = { | ||
| input = mkConsumerOptions inputDefaults; | ||
|
|
||
| output = mkProviderOptions { } // { | ||
| visible = "shallow"; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
mkConsumer = inputDefaults:
lib.setDefaultModuleLocation modulePath {
input = mkConsumerOptions inputDefaults;
output = mkProviderOptions { } // {
visible = "shallow";
};
};|
having compared the further with the original PR, i no longer so much feel so reluctant on this approach anymore. for documentation purposes though, it would be good to get an example of the type of cases where this approach is more robust to user errors than the original, and what kind of error messages one might get here for those issues compared to in the original. |
| inherit (lib.types) listOf submodule str; | ||
| in | ||
| { | ||
| mkConsumerOptions = |
There was a problem hiding this comment.
these mk*Options functions seem like a clunky way of merging option types.
as an additional data point in this space, i tried an approach to this that abstracts over (potentially nested) attributes as well as over types of info to put in (default, defaultText, ...).
i suppose ideally, since using and perhaps declaring options for secrets are fairly common operations, it might make sense to try and look for a somewhat terse approach to this.
i dunno if this is the right way, but maybe it'll help give some ideas at least.
There was a problem hiding this comment.
I really like this approach. It even works when building the docs! Seems like we’re in to something now.
Second draft PR for showing implementation of the NixOS/rfcs#189 as well as 2 contracts example. I did not implement here the streaming backup service because it felt redundant and I rather had this PR published ASAP.
This PR is a sibling to the earlier #432529 PR. It is also incompatible with that PR. I personally prefer this implementation which is much closer to what I have in my SelfHostBlocks project. It is simpler to understand, does not lead to infinite recursions at the slightest mistake and the documentation just fine. I actually followed how modular services did it.
I purposely split the PR into similar commits as the #432529 one to allow easier apples to apples comparison.
What it does better than the first PR is:
Documentation can be built just fine. I copied screenshots here under so you can have a look.
When the user messes up, the error message is not an infinite recursion. It’s not to say the error message is great but it’s better than infinite recursion.
The default values set from a consumer can be accessed from within the global config. This was not the case is the first PR.
For example take this commit in the first PR 80ec05c where we are required to define the default values from within the config of the submodule. The reason is that otherwise the values of the input attrset are not accessible. It is not at all the style used in nixpkgs so it feels awkward.
Vs the same commit in this PR c6776a6?w=1 which is in the normal style of nixpkgs.
Commands that work in the PR are:
Here are screenshots from the manual
contracts









services.restic.fileBackups
services.nextcloud.fileBackup
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.