Merge pull request 'PUT api/machines/{name}/config: ensure only valid config is ever written' (#433) from DavHau-dave into main

lib/build-clan/default.nix

@@ -7,9 +7,12 @@ let
   machinesDirs = lib.optionalAttrs (builtins.pathExists "${directory}/machines") (builtins.readDir (directory + /machines));
 
   machineSettings = machineName:
-    lib.optionalAttrs (builtins.pathExists "${directory}/machines/${machineName}/settings.json")
+    if builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE" != ""
-      (builtins.fromJSON
+    then builtins.fromJSON (builtins.readFile (builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE"))
-        (builtins.readFile (directory + /machines/${machineName}/settings.json)));
+    else
+      lib.optionalAttrs (builtins.pathExists "${directory}/machines/${machineName}/settings.json")
+        (builtins.fromJSON
+          (builtins.readFile (directory + /machines/${machineName}/settings.json)));
 
   # TODO: remove default system once we have a hardware-config mechanism
   nixosConfiguration = { system ? "x86_64-linux", name }: nixpkgs.lib.nixosSystem {

pkgs/clan-cli/clan_cli/config/machine.py

@@ -1,7 +1,9 @@
 import json
+import os
 import subprocess
 import sys
 from pathlib import Path
+from tempfile import NamedTemporaryFile
 from typing import Optional
 
 from fastapi import HTTPException
@@ -13,29 +15,39 @@ from clan_cli.nix import nix_eval
 
 
 def verify_machine_config(
-    machine_name: str, flake: Optional[Path] = None
+    machine_name: str, config: Optional[dict] = None, flake: Optional[Path] = None
-) -> tuple[bool, Optional[str]]:
+) -> Optional[str]:
     """
     Verify that the machine evaluates successfully
     Returns a tuple of (success, error_message)
     """
+    if config is None:
+        config = config_for_machine(machine_name)
     if flake is None:
         flake = get_clan_flake_toplevel()
-    proc = subprocess.run(
+    with NamedTemporaryFile(mode="w") as clan_machine_settings_file:
-        nix_eval(
+        json.dump(config, clan_machine_settings_file, indent=2)
-            flags=[
+        clan_machine_settings_file.seek(0)
-                "--impure",
+        env = os.environ.copy()
-                "--show-trace",
+        env["CLAN_MACHINE_SETTINGS_FILE"] = clan_machine_settings_file.name
-                f".#nixosConfigurations.{machine_name}.config.system.build.toplevel.outPath",
+        proc = subprocess.run(
-            ],
+            nix_eval(
-        ),
+                flags=[
-        capture_output=True,
+                    "--impure",
-        text=True,
+                    "--show-trace",
-        cwd=flake,
+                    "--show-trace",
-    )
+                    "--impure",  # needed to access CLAN_MACHINE_SETTINGS_FILE
-    if proc.returncode != 0:
+                    f".#nixosConfigurations.{machine_name}.config.system.build.toplevel.outPath",
-        return False, proc.stderr
+                ],
-    return True, None
+            ),
+            capture_output=True,
+            text=True,
+            cwd=flake,
+            env=env,
+        )
+        if proc.returncode != 0:
+            return proc.stderr
+        return None
 
 
 def config_for_machine(machine_name: str) -> dict:
@@ -52,13 +64,16 @@ def config_for_machine(machine_name: str) -> dict:
         return json.load(f)
 
 
-def set_config_for_machine(machine_name: str, config: dict) -> None:
+def set_config_for_machine(machine_name: str, config: dict) -> Optional[str]:
     # write the config to a json file located at {flake}/machines/{machine_name}/settings.json
     if not machine_folder(machine_name).exists():
         raise HTTPException(
             status_code=404,
             detail=f"Machine {machine_name} not found. Create the machine first`",
         )
+    error = verify_machine_config(machine_name, config)
+    if error is not None:
+        return error
     settings_path = machine_settings_file(machine_name)
     settings_path.parent.mkdir(parents=True, exist_ok=True)
     with open(settings_path, "w") as f:
@@ -67,6 +82,7 @@ def set_config_for_machine(machine_name: str, config: dict) -> None:
 
     if repo_dir is not None:
         commit_file(settings_path, repo_dir)
+    return None
 
 
 def schema_for_machine(machine_name: str, flake: Optional[Path] = None) -> dict:

pkgs/clan-cli/clan_cli/webui/routers/machines.py

@@ -2,14 +2,13 @@
 import logging
 from typing import Annotated
 
-from fastapi import APIRouter, Body
+from fastapi import APIRouter, Body, HTTPException
-
-import clan_cli.config as config
 
 from ...config.machine import (
     config_for_machine,
     schema_for_machine,
     set_config_for_machine,
+    verify_machine_config,
 )
 from ...machines.create import create_machine as _create_machine
 from ...machines.list import list_machines as _list_machines
@@ -58,7 +57,9 @@ async def get_machine_config(name: str) -> ConfigResponse:
 async def set_machine_config(
     name: str, config: Annotated[dict, Body()]
 ) -> ConfigResponse:
-    set_config_for_machine(name, config)
+    error = set_config_for_machine(name, config)
+    if error is not None:
+        raise HTTPException(status_code=400, detail=error)
     return ConfigResponse(config=config)
 
 
@@ -69,6 +70,7 @@ async def get_machine_schema(name: str) -> SchemaResponse:
 
 
 @router.get("/api/machines/{name}/verify")
-async def verify_machine_config(name: str) -> VerifyMachineResponse:
+async def put_verify_machine_config(name: str) -> VerifyMachineResponse:
-    success, error = config.machine.verify_machine_config(name)
+    error = verify_machine_config(name)
+    success = error is None
     return VerifyMachineResponse(success=success, error=error)

pkgs/clan-cli/tests/test_flake/flake.nix

@@ -6,9 +6,13 @@
     nixosConfigurations.machine1 = inputs.nixpkgs.lib.nixosSystem {
       modules = [
         ./nixosModules/machine1.nix
-        (if builtins.pathExists ./machines/machine1/settings.json
+        (
-        then builtins.fromJSON (builtins.readFile ./machines/machine1/settings.json)
+          if builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE" != ""
-        else { })
+          then builtins.fromJSON (builtins.readFile (builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE"))
+          else if builtins.pathExists ./machines/machine1/settings.json
+          then builtins.fromJSON (builtins.readFile ./machines/machine1/settings.json)
+          else { }
+        )
         ({ lib, options, pkgs, ... }: {
           config = {
             nixpkgs.hostPlatform = "x86_64-linux";

pkgs/clan-cli/tests/test_machines_api.py

@@ -45,13 +45,36 @@ def test_configure_machine(api: TestClient, test_flake: Path) -> None:
     json_response = response.json()
     assert "schema" in json_response and "properties" in json_response["schema"]
 
-    # set some config
+    # set come invalid config (fileSystems missing)
     config = dict(
         clan=dict(
             jitsi=dict(
                 enable=True,
             ),
         ),
+    )
+    response = api.put(
+        "/api/machines/machine1/config",
+        json=config,
+    )
+    assert response.status_code == 400
+    assert (
+        "The ‘fileSystems’ option does not specify your root"
+        in response.json()["detail"]
+    )
+
+    # ensure config is still empty after the invalid attempt
+    response = api.get("/api/machines/machine1/config")
+    assert response.status_code == 200
+    assert response.json() == {"config": {}}
+
+    # set some valid config
+    config2 = dict(
+        clan=dict(
+            jitsi=dict(
+                enable=True,
+            ),
+        ),
         fileSystems={
             "/": dict(
                 device="/dev/fake_disk",
@@ -69,17 +92,17 @@ def test_configure_machine(api: TestClient, test_flake: Path) -> None:
     )
     response = api.put(
         "/api/machines/machine1/config",
-        json=config,
+        json=config2,
     )
     assert response.status_code == 200
-    assert response.json() == {"config": config}
+    assert response.json() == {"config": config2}
 
-    # verify the machine config
+    # ensure that the config has actually been updated
+    response = api.get("/api/machines/machine1/config")
+    assert response.status_code == 200
+    assert response.json() == {"config": config2}
+
+    # verify the machine config evaluates
     response = api.get("/api/machines/machine1/verify")
     assert response.status_code == 200
     assert response.json() == {"success": True, "error": None}
-
-    # get the config again
-    response = api.get("/api/machines/machine1/config")
-    assert response.status_code == 200
-    assert response.json() == {"config": config}