init: arbitrary output resources #1048
Conversation
There was a problem hiding this comment.
This is definitely an interesting feature and I am looking forward to use this.
I started to add type annotation in https://github.com/NixOS/nixops/pull/1042/files
It would be great if you also could add them in this module.
This should help us to migrate in python3 and is general a good idea when integration
tests are hard to realize.
nixops/resources/output.py
Outdated
| if self.value is None: | ||
| self.name = defn.name | ||
| self.state = self.UP | ||
| output_dir = tempfile.mkdtemp(prefix="nixops-output-tmp") |
There was a problem hiding this comment.
This leaks a temporary directory in case of an error (for example if subprocess.check_output fails). Unfortunately tempfile.TemporaryDirectory is only available in python3, leaving two alternatives here:
- surround everything with a try-finally block
- add a helper function using contextlib and call rmtree in the end
There was a problem hiding this comment.
revised with try-finally
There was a problem hiding this comment.
also working type annotations ...
nix/output.nix
Outdated
| type = types.nullOr (types.str); | ||
| #type = types.nullOr (types.either types.str types.path); | ||
| description = '' | ||
| Text of a shell script which will produce a JSON value. |
There was a problem hiding this comment.
I wonder if we should limit this to shell. I could imagine with a high-level programming languages it would be easier to generate json in a safe way. Also one could leverage libraries.
|
Thanks for the feedback, I’ll incorporate the thoughts over the weekend sometime. I do expect that having resources available in phase 1 is going to be more important going forward: #959 Any thoughts on the direction of the design? At the moment I normally accomplish a similar thing by just using the file system and putting deployments into their own folder and use relative paths. With key/value store we can continue to use the “resource” paradigm when we have more backends. Another consideration is that it is sometimes cumbersome to output proper json from the command line. The net effect of this design is that you can include any supporting scripts directly into nix and they are run for you automatically. |
|
Another thought, use nix-shell in some way to manage the local dependencies that may be in the script? Or just allow people to write whatever they want? |
|
First of all I am not in the NixOps team, so I won't be able to merge this pull request in the end. Rather then a shell script I would change it to accept the path to an executable program. This could be a script or whatever the user wants to use. resources.output.myKeys.command = pkgs.writeScript "foo.py" ''
#!${python}/bin/python
# do whatever
'';
resources.output.myKeys2.command = pkgs.writeScript "foo.sh" ''
#!${bash}/bin/bash
# do whatever
'';Then one could use whatever fits the best to generate the output. An alternative could be the following to allow additional dependencies resources.output.myKeys2.command = pkgs.writeScript "foo.sh" ''
#!/usr/bin/env nix-shell -p
#!nix-shell -p python3.pkgs.foobar -p python3 nix -i python3
''; |
tomberek
force-pushed
the
init_outputs
branch
9 times, most recently
from
f445246 to
b515685
Compare
|
Wireguard example: |
|
Also have tested using this to serialize (via tar and base 64) an output to re-serialize after garbage collection: This set's up a new set of SSL CA's and certificates. The result is serialized to a value stored in the statefile. The following in |
|
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-december/1711/5 |
There was a problem hiding this comment.
Thanks for the pull request, this is quite useful for quite a few things I do even in my own deployments.
However I don't like the name output, because it's not very descriptive of what this is doing. I'd rather prefer something like commandOutput, customOutput or even just custom.
nix/output.nix
Outdated
| Warning: This uses shell features and is potentially dangerous. | ||
| Environment variables: | ||
| $out is a temp directory available for use. | ||
| ''; |
There was a problem hiding this comment.
nitpick: Indentation , also please note that descriptions are written in Docbook, so maybe use <warning/> and probably <envar/> here.
nixops/resources/output.py
Outdated
| res = subprocess.check_output( | ||
| [defn.script], | ||
| env=env, | ||
| shell=True) |
There was a problem hiding this comment.
Please note that this will make the script dependent on the system's shell. For example if you're using NixOps on a Debian machine, you'll end up getting dash instead of bash. So I'd probably wrap the script with stdenv.shell or pkgs.bash so that if the script is using bashisms it will still work.
One possibility would be to add another option to the resource, like eg. executable, which defaults to using something like eg. pkgs.writeScript "somename" "#!${pkgs.stdenv.shell}\n${config.script}". That way you only need to run the executable here without the need to set shell=True.
There was a problem hiding this comment.
Incorporated all the other feedback. I'm not sure how to do this one. Whenever I try to do this, it seems the writeScript gets evaluated, but not instantiated.
There was a problem hiding this comment.
Can you post an output of that with --debug?
|
This still functions on top of master. Also works well with #1054 TODO: I'd like to create a similar thing, but where the resource is created and stored on the remote machines. Send-keys would then retrieve things like public keys (pub/priv created on remote machine) and send public side to other machines that need it. |
|
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/nixops-arbitrary-output/1406/2 |
nixops/resources/output.py
Outdated
| res = subprocess.check_output( | ||
| [defn.script], | ||
| env=env, | ||
| shell=True) |
There was a problem hiding this comment.
Can you post an output of that with --debug?
tomberek
force-pushed
the
init_outputs
branch
2 times, most recently
from
5f65041 to
70453b6
Compare
|
Added a test. Runs with |
| machine = {resources, ...} : { | ||
| imports = [ <nixpkgs/nixos/modules/profiles/minimal.nix> ]; | ||
| deployment = { | ||
| keys."secret.key".text = resources.output.thing.value; |
There was a problem hiding this comment.
This requires the 3 evaluations you added, so I suggest another example/test ?
Use content-hashed sha256 for resource_id Improve typing Expose resources to deployment.keys Re-evaluate info.machines in order to expose resources and publicIPv4 to deployment.keys. Add test for output resource generation Use SelfDeletingDir New test for modification of output resource Remove superfluous return
|
@tomberek Btw didn't you have a version where it used Also maybe an overkill, but I wish we could do such scripts in isolation (inside nix builds ? in chroots?), the idea of allowing random scripts is a bit scary to me :) But that's probably more complicated. |
nixops/deployment.py
Outdated
| @@ -309,13 +309,13 @@ def evaluate_args(self): | |||
| except subprocess.CalledProcessError: | |||
| raise NixEvalError | |||
|
|
|||
| def evaluate_config(self, attr): | |||
| def evaluate_config(self, attr,extra_exprs=[]): | |||
There was a problem hiding this comment.
Note that the 3 evals diff is still here even after the rebase
|
I never got the writeScriptBin approach to work. It had to do with the order of instantiation, so the drv got created, but never realized. I decided to leave that as a future improvement. Regarding the three-phase: yes, most use cases include using an output resource in deployment.keys such that it’s most useful with it. I can remove it if you think it’s cleaner that way. (I’d have to modify the tests, but that’s ok) Another approach is to redesign the whole thing in a better way, I’d appreciate any ideas. Especially if we include a method to generate the resources remotely. |
Can you share the nix/output.nix that had the issue ? I want to try it out as well.
I'm just not sure if adding yet another eval is a good idea, it will make things slower in my opinion. I'd like to get more opinions on that (perhaps in a separate PR): @edolstra @rbvermaa @grahamc @aszlig @domenkozar (randomly pinging people :-) ), any opinions ? |
|
@AmineChikhaoui : I've got some of the "executable" work here if you want to play around with it: Note: it's not in a working state. |
|
I gave it a try to use a generated executable (which uses Not sure yet why it would be very different from using a regular NixOS module. I know NixOps is a bit different in the evaluation but it still uses |
|
@AmineChikhaoui: This looks a lot like it's recursing into |
|
@aszlig Ok, I think this happens in with @grahamc's nix patch NixOS/nix#2761, I get the following output (not sure if relevant for our case though): |
|
Btw the first eval |
|
I agree with @aszlig, |
|
@edolstra I'm not committed to any particular name. Any suggestions?
@aszlig recommendations:
I'm thinking resources.custom would be good. It could later come in multiple types "local" and "remote", "vault", "s3", "url", etc. specifying where the resource is created and/or stored. I don't want to bikeshed on this, so if there's a consensus, strong feeling, no response for a day, or a first response for one of these I will refactor/test/push a new version. |
|
So @edolstra mentioned something about --xml vs --json when I asked about the issue but I thought it's not related then I tried that and it's actually the root cause of the infinite recursion issue I was seeing. I wish we can nuke the xml parsing we have there and only use json but that will require an update to all the resources that do that in the |
Using |
|
Forgot to post an update here: So I guess we will go with the current approach. |
Remove three-phase evaluation
|
@AmineChikhaoui : updated as requested. Thanks for working through this! I think you ran into the same issue I did on the script. |
| Text of a script which will produce a JSON value. | ||
| <warning>Warning: This uses shell features and is potentially dangerous.</warning> | ||
| Environment variables: | ||
| <envar>$out</envar> is a temp directory available for use. |
There was a problem hiding this comment.
out is available as an environment variable in the script as temp directory.
https://github.com/NixOS/nixops/pull/1048/files#diff-6d7517db2e330d537da7846c6ab7f4c5R69
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Using the ssh_keypair as a model, this allows one to create whatever JSON via a shell script in order to allow customized resources.
Example use: