From 588bde069f1c8da310e5eabdce678a5fe5db6aa1 Mon Sep 17 00:00:00 2001 From: DavHau Date: Tue, 24 Oct 2023 18:40:48 +0100 Subject: [PATCH] PUT api/machines/{name}/config: ensure only valid config is ever written - add CLAN_MACHINE_SETTINGS_FILE variable to temporarily override the machine settings file - do a dry-run evaluation first with the new config before persisting it. --- lib/build-clan/default.nix | 9 ++-- pkgs/clan-cli/clan_cli/config/machine.py | 52 ++++++++++++------- .../clan_cli/webui/routers/machines.py | 14 ++--- pkgs/clan-cli/tests/test_flake/flake.nix | 10 ++-- pkgs/clan-cli/tests/test_machines_api.py | 41 +++++++++++---- 5 files changed, 87 insertions(+), 39 deletions(-) diff --git a/lib/build-clan/default.nix b/lib/build-clan/default.nix index 3b2a5b679..770f7856a 100644 --- a/lib/build-clan/default.nix +++ b/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") - (builtins.fromJSON - (builtins.readFile (directory + /machines/${machineName}/settings.json))); + if builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE" != "" + then builtins.fromJSON (builtins.readFile (builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE")) + 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 { diff --git a/pkgs/clan-cli/clan_cli/config/machine.py b/pkgs/clan-cli/clan_cli/config/machine.py index ba4e7fb3c..edd4cf930 100644 --- a/pkgs/clan-cli/clan_cli/config/machine.py +++ b/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 -) -> tuple[bool, Optional[str]]: + machine_name: str, config: Optional[dict] = None, flake: Optional[Path] = None +) -> 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( - nix_eval( - flags=[ - "--impure", - "--show-trace", - f".#nixosConfigurations.{machine_name}.config.system.build.toplevel.outPath", - ], - ), - capture_output=True, - text=True, - cwd=flake, - ) - if proc.returncode != 0: - return False, proc.stderr - return True, None + with NamedTemporaryFile(mode="w") as clan_machine_settings_file: + json.dump(config, clan_machine_settings_file, indent=2) + clan_machine_settings_file.seek(0) + env = os.environ.copy() + env["CLAN_MACHINE_SETTINGS_FILE"] = clan_machine_settings_file.name + proc = subprocess.run( + nix_eval( + flags=[ + "--impure", + "--show-trace", + "--show-trace", + "--impure", # needed to access CLAN_MACHINE_SETTINGS_FILE + f".#nixosConfigurations.{machine_name}.config.system.build.toplevel.outPath", + ], + ), + 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: diff --git a/pkgs/clan-cli/clan_cli/webui/routers/machines.py b/pkgs/clan-cli/clan_cli/webui/routers/machines.py index 3eb815e77..753a71136 100644 --- a/pkgs/clan-cli/clan_cli/webui/routers/machines.py +++ b/pkgs/clan-cli/clan_cli/webui/routers/machines.py @@ -2,14 +2,13 @@ import logging from typing import Annotated -from fastapi import APIRouter, Body - -import clan_cli.config as config +from fastapi import APIRouter, Body, HTTPException 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: - success, error = config.machine.verify_machine_config(name) +async def put_verify_machine_config(name: str) -> VerifyMachineResponse: + error = verify_machine_config(name) + success = error is None return VerifyMachineResponse(success=success, error=error) diff --git a/pkgs/clan-cli/tests/test_flake/flake.nix b/pkgs/clan-cli/tests/test_flake/flake.nix index be233b98c..c82903659 100644 --- a/pkgs/clan-cli/tests/test_flake/flake.nix +++ b/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) - else { }) + ( + if builtins.getEnv "CLAN_MACHINE_SETTINGS_FILE" != "" + 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"; diff --git a/pkgs/clan-cli/tests/test_machines_api.py b/pkgs/clan-cli/tests/test_machines_api.py index fd944ee9c..1fe9e958f 100644 --- a/pkgs/clan-cli/tests/test_machines_api.py +++ b/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}