Add function to generate a config file with out of band values #328472
Conversation
github-actions
bot
added
6.topic: nixos
|
@ofborg test misc genConfigOutOfBand-test jackett snipe-it monica keycloak gitlab |
ibizaman
force-pushed
the
official_replace_secrets
branch
from
f42c5f6 to
41815b9
Compare
|
@ofborg test misc genConfigOutOfBand-test jackett snipe-it monica keycloak gitlab |
|
Mmmh I don't know why I got this from ofborg: I don't see where in the code it says that platform is not supported for those tests. They succeed locally for me at least on that platform. |
ibizaman
force-pushed
the
official_replace_secrets
branch
from
41815b9 to
09a5b98
Compare
ibizaman
force-pushed
the
official_replace_secrets
branch
from
09a5b98 to
b9ee1ed
Compare
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
|
@ofborg test filebeat |
|
@ofborg test kavita |
| ${utils.genConfigOutOfBand { | ||
| config = cfg.settings; | ||
| configLocation = "/var/lib/filebeat/filebeat.yml"; | ||
| generator = utils.genConfigOutOfBandFormatAdapter json; |
There was a problem hiding this comment.
Shouldn't it be yaml? I assume the module used json due to genJqSecretsReplacementSnippet.
There was a problem hiding this comment.
You know what, I tried YAML at first but it failed. It's because of the private key, it includes newlines. So with YAML, the newlines was creating a syntax error because the indentation was not respected. With JSON, the syntax is valid even with newlines because it includes opening and closing quotes. And anyway, JSON is a subset of YAML afterall.
That being said, another solution would be to enhance the replaceSecret.py script to have it replace "real" newlines with \n. That would be fully backwards compatible. I didn't do it because I don't want the replaceSecret.py script to have one argument per use case but could if you'd think that's better.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/thoughts-about-uniform-secrets-integration-in-nixos-modules/49623/3 |
github-actions
bot
added
the
6.topic: testing
|
@ofborg test genConfigOutOfBand-test |
The goal is to provide one official function, that is tested, that supports the majority of use cases. Those use cases are `replace-secret`, `genJqSecretsReplacementSnippet` and any module using a `_secret` field. Currently, these use cases have all different implementations. They all work of course, but having a central function that works for all cases would be better for maintainability and re-usability. Indeed, other modules that currently do not support secrets will be easily able to. This function has one additional feature compared to existing ones, it is generic in the config generator function, allowing one to generate a JSON, YAML, INI or whatever configuration file.
ibizaman
force-pushed
the
official_replace_secrets
branch
from
30f989d to
6f9cbe8
Compare
ofborg
bot
removed
2.status: merge conflict
|
I added a comment about how I made the tests here https://discourse.nixos.org/t/thoughts-about-uniform-secrets-integration-in-nixos-modules/49623/9?u=ibizaman |
There was a problem hiding this comment.
This definitely must be split up. Please create one PR for the lib changes with good commits where only one function is added. When that is merged we can add the core functionality and maybe convert a service. Then we can do a PR per service so that the respective maintainers can properly review them.
Also please follow the contributing guide when creating commits.
nixos/lib/utils.nix
Outdated
There was a problem hiding this comment.
This commit must be split into the respective commits before to have a clean history.
There was a problem hiding this comment.
I think the lib functions are making good progress, but feel free to put a version of the functions you need in local let bindings with a reference to the PR URLs so that they don't need the level of scrutiny that we apply to lib changes.
There was a problem hiding this comment.
Instant facepalm. What a great idea! I should’ve done that from the start. 🎉
|
@SuperSandro2000 for sure. I wanted to see the final result first. To make sure all the pieces fall into place and gather feedback. Not sure how we do that here usually. Just to make sure, when you say one PR for the lib, do you mean a single one for misc.nix, utils.nix and all other “internal” files? |
|
^ I started splitting this one into multiple PRs. |
This new 'recursiveCollect' function is like 'collect' while additionally descending into lists. This function will be used with user-provided freeform variables - arbitrary combinations of attrset, lists and values - in the upcoming secret management feature. The PR NixOS#328472 shows how it will be used.
This new 'mapDataRecursiveCond' function is like 'mapAttrsRecursiveCond' and additionally recurses on lists. This function will be used with user-provided freeform variables - arbitrary combinations of attrset, lists and values - in the upcoming secret management feature. The PR NixOS#328472 shows how it will be used.
There was a problem hiding this comment.
Does this support structured secrets? genJqSecretsReplacementSnippet supports that in #325217. See that PR for more info about what is structured secrets.
There was a problem hiding this comment.
TIL this exists, what a great feature.
There was a problem hiding this comment.
That should work indeed! "Just" need to pass in the correct generator function.
I must say though that the reason I started hacking on this feature is actually because I did not want to let the secret files have any structure. Because that means the final user must get the structure right and there's no easy way to give a meaningful error message. The experience for getting the structure right can be frustrating as I experienced first hand.
So I started on this because I wanted the structure of the config file to be determined in Nix directly, and any secret files would then only contain the secret value itself.
There was a problem hiding this comment.
That should work indeed! "Just" need to pass in the correct generator function.
Hmm, I do not know any "correct" JSON generator function.
I do not mean this PR has to support structured secrets. I just want to add that supporting structured secrets does have its value. For example, you may consider an attr name (in Nix lang) a secret. For another example, the secret value may be a list of strings.
There was a problem hiding this comment.
I find this difficult to review, and not just because of the parts that ought to be separate PRs. I don't have a clear understanding of:
- Why we need this
- Why it's so complex
- Why it has to add options to systemd services
- How this is better than what we already do
I think this needs a design document, bordering on the scale of an RFC, to define its motivation, design, and consideration of alternatives.
|
@ElvishJerricco TBH I though about doing that before starting the PR but I didn’t think the PR would grow that big. It makes sense to have one now. But also I wanted to use the PR as a testing ground. Personally I like to write code to get a feel for a feature. But that’s why the PR is still draft also. I’ll explain in the RFC what’s the rationale between everything in the PR. |
wegank
added
the
2.status: merge conflict
|
Just wanted to post an update. I couldn’t progress on this this last month but should be able to pick this up again in two weeks. I’m still fully invested in getting this across the finish line! |
Description of changes
The goal is to provide one official function, that is tested, that supports the majority of use cases. Those use cases are
replace-secret,genJqSecretsReplacementSnippetand any module using a_secretfield.Currently, these use cases all have different implementations. They all work of course, but having a central function that is tested and works for all cases would be better for maintainability and re-usability. Indeed, other modules that currently do not support secrets will be easily able to.
This function has one additional feature compared to existing ones, it is generic in the config generator function, allowing one to generate a JSON, YAML, INI or whatever configuration file.
Concrete example
The following service definition:
With the content of the secret files being:
Produces the service:
(I removed the
Environment=to avoid cluttering the output)And
cat /nix/store/myfrvbz29xa0vz1q785ivp23k6z6r4ir-unit-script-outOfBand-outOfBandConfig/bin/outOfBand-outOfBandConfigis:With
cat /nix/store/14445vijz8cbcja8lmdsm7w208amfll3-generator-templatebeing:{"a":{"a":"%SECRET_A_A%"},"b":"%SECRET_B%","c":"not secret C","d":{"d":"not secret D"},"e":"%SECRET_E%"}The final configuration file is:
$ ls -l /var/lib/outOfBandConfig/myconfig.json -rw------- 1 outOfBand outOfBand 125 Jul 30 20:40 /var/lib/outOfBandConfig/myconfig.json $ cat /var/lib/outOfBandConfig/myconfig.json {"a":{"a":"Secret of A"},"b":"prefix-Secret of B-suffix","c":"not secret C","d":{"d":"not secret D"},"e":"prefix-abc-suffix"}Update existing code
I did a grep for
replace-secret,genJqSecretsReplacementSnippetand_secretand compiled a list of modules that should/could be updated to use this instead. I already made some replacements - only for modules including tests - to prove that this implementation works in those cases. Hereunder is the compiled list of modules I found:Modules using secrets
Btw I have no idea how to best structure such a PR. Should I split it? Should I squash all commits? Something else?
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.