Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from xml intermediate Nix representation to JSON #1275

Merged
merged 13 commits into from Apr 23, 2020

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented Apr 2, 2020

This change is intended to make life easier for plugin authors.
We have removed the XML parameter to ResourceDefinition and you are
now only provided with a name & a dict containing the evaluated
values.

Note that this will break all current plugins.

An example of this in use in a plugin can be found here: https://github.com/adisbladis/nixops-terraform.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

One thing I'm noticing, though, is the old code had some type information which the new code doesn't, and maybe we should do some type hinting and type conversions?

Also, I think it'd be useful to write down some docs, like that the config passed to a MachineDefinition is the deployment attrset for the node, and also how to convert nixops1 plugins to this new mechanism for nixops2. Maybe next to the authoring doc, as a updating to NixOps 2 doc?

nixops/backends/__init__.py Outdated Show resolved Hide resolved
nixops/backends/__init__.py Show resolved Hide resolved
@adisbladis adisbladis force-pushed the xml-to-json branch 2 times, most recently from fbb2d63 to 34936a1 Compare April 15, 2020 19:31
nixops/util.py Outdated Show resolved Hide resolved
@adisbladis adisbladis force-pushed the xml-to-json branch 2 times, most recently from 874d179 to 9f03c29 Compare April 15, 2020 19:51
@grahamc
Copy link
Member

grahamc commented Apr 15, 2020

lgtm :)

@grahamc
Copy link
Member

grahamc commented Apr 15, 2020

This is great! I think the only thing it needs from here is a bit of docs for a migration guide

@adisbladis adisbladis force-pushed the xml-to-json branch 3 times, most recently from b5b37cb to 8914d44 Compare April 16, 2020 13:54
@adisbladis adisbladis force-pushed the xml-to-json branch 3 times, most recently from 87f71e2 to a74e555 Compare April 17, 2020 14:00
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

lookin' real fancy. Some questions, and I'm giving this a clone to try it out!

doc/plugins/authoring.rst Outdated Show resolved Hide resolved
doc/plugins/authoring.rst Outdated Show resolved Hide resolved
doc/plugins/authoring.rst Show resolved Hide resolved
nixops/backends/__init__.py Outdated Show resolved Hide resolved
@grahamc
Copy link
Member

grahamc commented Apr 17, 2020

Nice, when causing a developer-error in the type of a module option:

TypeError: type of hasFastConnection must be bool; got str instead

@grahamc
Copy link
Member

grahamc commented Apr 17, 2020

Not sure if this is from thi sbranch or on master already, but:

kif.........> uploading key ‘vault-login’...
kif.........> error: Traceback (most recent call last):
  File "/home/grahamc/projects/github.com/NixOS/nixops/nixops/deployment.py", line 910, in worker
    m.send_keys()
  File "/home/grahamc/projects/github.com/NixOS/nixops/nixops/backends/__init__.py", line 275, in send_keys
    f.write(opts["text"])
TypeError: write() argument must be str, not None

Will look!

@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

I'm not getting that write error on master. It seems to come down to this:

if "text" in opts:
with open(tmp, "w+") as f:
f.write(opts["text"])
elif "keyFile" in opts:
self._logged_exec(["cp", opts["keyFile"], tmp])
else:
raise Exception(
"Neither 'text' or 'keyFile' options were set for key '{0}'.".format(
k
)

I'm seeing this json come through with a null text field:

{
  "machines": {
    "kif": {
      "keys": {
        "xxx": {
          "_module": {
            "args": {
              "name": "xxx"
            },
            "check": true
          },
          "destDir": "/run/keys",
          "group": "root",
          "keyFile": "/home/grahamc/projects/github.com/xxx",
          "path": "/run/keys/xxx",
          "permissions": "0600",
          "text": null,
          "user": "root"
        },

the XML output:

<?xml version='1.0' encoding='utf-8'?>
<expr>
  <attrs>
    <attr column="5" line="118" name="machines" path="/home/grahamc/projects/github.com/NixOS/nixops/nix/eval-machine-info.nix">
      <attrs>
        <attr name="kif">
          <attrs>
            <attr column="40" line="121" name="keys" path="/home/grahamc/projects/github.com/NixOS/nixops/nix/eval-machine-info.nix">
              <attrs>
                <attr name="xxx">
                  <attrs>
                    <attr name="_module">
                      <attrs>
                        <attr name="args">
                          <attrs>
                            <attr name="name">
                              <string value="xxx" />
                            </attr>
                          </attrs>
                        </attr>
                        <attr name="check">
                          <bool value="true" />
                        </attr>
                      </attrs>
                    </attr>
                    <attr name="destDir">
                      <string value="/run/keys" />
                    </attr>
                    <attr name="group">
                      <string value="root" />
                    </attr>
                    <attr name="keyFile">
                      <path value="/home/grahamc/projects/github.com/xxx" />
                    </attr>
                    <attr name="path">
                      <string value="/run/keys/xxx" />
                    </attr>
                    <attr name="permissions">
                      <string value="0600" />
                    </attr>
                    <attr name="text">
                      <null />
                    </attr>
                    <attr name="user">
                      <string value="root" />
                    </attr>
                  </attrs>
                </attr>

so these seem to be the same, so I'm a bit confused.

@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

This is why:

https://github.com/NixOS/nixops/pull/1275/files#diff-6ea77fd00d5f092f8150c2398eaf53d8L48-L49

By default, Nones would not get assigned in the first place.

@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

I tried adding a type to the keys:

diff --git a/nixops/backends/__init__.py b/nixops/backends/__init__.py
index 4428de77..127107b1 100644
--- a/nixops/backends/__init__.py
+++ b/nixops/backends/__init__.py
@@ -8,6 +8,14 @@ import nixops.util
 import nixops.resources
 import nixops.ssh_util

+class KeyOptions(nixops.resources.ResourceOptions):
+    text: Optional[str]
+    keyFile: Optional[str]
+    destDir: str
+    user: str
+    group: str
+    permissions: str
+    fizz: str

 class MachineOptions(nixops.resources.ResourceOptions):
     storeKeysOnMachine: bool
@@ -15,7 +23,7 @@ class MachineOptions(nixops.resources.ResourceOptions):
     alwaysActivate: bool
     owners: Sequence[str]
     hasFastConnection: bool
-    keys: Mapping[str, Mapping]
+    keys: Mapping[str, KeyOptions]
     nixosRelease: str


@@ -29,7 +37,7 @@ class MachineDefinition(nixops.resources.ResourceDefinition):
     always_activate: bool
     owners: List[str]
     has_fast_connection: bool
-    keys: Mapping[str, Mapping]
+    keys: Mapping[str, KeyOptions]

     def __init__(self, name: str, config: nixops.resources.ResourceEval):
         super().__init__(name, config)
@@ -38,7 +46,8 @@ class MachineDefinition(nixops.resources.ResourceDefinition):
         self.always_activate = config["alwaysActivate"]
         self.owners = config["owners"]
         self.has_fast_connection = config["hasFastConnection"]
-        self.keys = config["keys"]
+        self.keys = {k: KeyOptions(**v) for k,v in config["keys"].items()}
+


 class MachineState(nixops.resources.ResourceState):
@@ -255,13 +264,14 @@ class MachineState(nixops.resources.ResourceState):
             return
         if self.store_keys_on_machine:
             return
+        opts: KeyOptions
         for k, opts in self.get_keys().items():
+            from pprint import pprint
+            pprint(opts)
             self.log("uploading key ‘{0}’...".format(k))
             tmp = self.depl.tempdir + "/key-" + self.name
-            if "destDir" not in opts:
-                raise Exception("Key '{}' has no 'destDir' specified.".format(k))

-            destDir = opts["destDir"].rstrip("/")
+            destDir = opts.destDir.rstrip("/")
             self.run_command(
                 (
                     "test -d '{0}' || ("
@@ -270,11 +280,11 @@ class MachineState(nixops.resources.ResourceState):
                 ).format(destDir)
             )

-            if "text" in opts:
+            if opts.text is not None:
                 with open(tmp, "w+") as f:
-                    f.write(opts["text"])
-            elif "keyFile" in opts:
-                self._logged_exec(["cp", opts["keyFile"], tmp])
+                    f.write(opts.text)
+            elif opts.keyFile is not None:
+                self._logged_exec(["cp", opts.keyFile, tmp])
             else:
                 raise Exception(
                     "Neither 'text' or 'keyFile' options were set for key '{0}'.".format(
@@ -306,7 +316,7 @@ class MachineState(nixops.resources.ResourceState):
                         "chmod '{3}' {0}",
                     ]
                 ).format(
-                    tmp_outfile_esc, opts["user"], opts["group"], opts["permissions"]
+                    tmp_outfile_esc, opts.user, opts.group, opts.permissions
                 )
             )
             self.run_command("mv " + tmp_outfile_esc + " " + outfile_esc)

I was surprised that mypy was totally happy with this, but it doesn't actually work or validate the data. I think this is what you were saying w.r.t. checking generics. Now I'm curious about if this is solved elsewhere, like by pydantic? I'd really like to be able to have typed mappings.

adisbladis and others added 12 commits April 23, 2020 16:42
This change is intended to make life easier for plugin authors.
We have removed the XML parameter to ResourceDefinition and you are
now only provided with a name & a dict containing the evaluated
values.
With nix-instantiate --xml, the output of evaluation of a path shows
the original path to the file. With --json, the output shows the path
if it were copied to the Nix store.

Applying toString in the expression forces Nix to skip copying it to
the store under any circumstance.
It's no longer required since moving to JSON representation.
Co-authored-by: Adam Höse <adam.hose@tweag.io>
Also fix type checking in ImmutableValidatedObject even when an
attribute is _not_ passed to the constructor.

Co-authored-by: Graham Christensen <graham.christensen@tweag.io>
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

lgtm, the ratchet is unhappy because the generic validation code operates on Any which ... yep.

@grahamc
Copy link
Member

grahamc commented Apr 23, 2020

woot:

Imprecision went up:
nixops.util:		17.62 -> 18.2
Imprecision went down:
nixops.backends:		38.92 -> 37.24
nixops.backends.none:		54.7 -> 50.0
nixops.deployment:		17.55 -> 16.32
nixops.resources:		39.53 -> 36.42
nixops.resources.commandOutput:		19.47 -> 17.5
nixops.resources.ssh_keypair:		48.33 -> 44.26
Total:		25.3 -> 24.6

@grahamc grahamc merged commit 9962fe4 into NixOS:master Apr 23, 2020
kanban automation moved this from In progress to Done Apr 23, 2020
@tewfik-ghariani
Copy link
Contributor

Can we create a branch 'pre-xml-to-json' to keep the plugins alive before working on the switch?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-3/7154/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants