Merge pull request 'vars: move ensure_consistent_state into health_check, move into store classes' (#2570) from vars-health_check into main

Reviewed-on: https://git.clan.lol/clan/clan-core/pulls/2570

pkgs/clan-cli/clan_cli/flash/flash.py

@@ -54,9 +54,7 @@ def flash_machine(
             extra_args = []
         system_config_nix: dict[str, Any] = {}
 
-        generate_vars_for_machine(
+        generate_vars_for_machine(machine, generator_name=None, regenerate=False)
-            machine, generator_name=None, regenerate=False, fix=False
-        )
         generate_facts([machine])
 
         if system_config.language:

pkgs/clan-cli/clan_cli/vars/_types.py

@@ -60,6 +60,20 @@ class StoreBase(ABC):
     def is_secret_store(self) -> bool:
         pass
 
+    def health_check(
+        self,
+        generator: "Generator | None" = None,
+        file_name: str | None = None,
+    ) -> str | None:
+        return None
+
+    def fix(
+        self,
+        generator: "Generator | None" = None,
+        file_name: str | None = None,
+    ) -> None:
+        return None
+
     def backend_collision_error(self, folder: Path) -> None:
         msg = (
             f"Var folder {folder} exists but doesn't look like a {self.store_name} secret."

pkgs/clan-cli/clan_cli/vars/check.py

@@ -66,8 +66,11 @@ def vars_status(machine: Machine, generator_name: None | str = None) -> VarStatu
                     )
                     missing_secret_vars.append(file)
                 else:
-                    needs_fix, msg = secret_vars_store.needs_fix(generator, file.name)
+                    msg = secret_vars_store.health_check(
-                    if needs_fix:
+                        generator=generator,
+                        file_name=file.name,
+                    )
+                    if msg:
                         log.info(
                             f"Secret var '{file.name}' for service '{generator.name}' in machine {machine.name} needs update: {msg}"
                         )

pkgs/clan-cli/clan_cli/vars/cli.py

@@ -4,6 +4,7 @@ import argparse
 from clan_cli.hyperlink import help_hyperlink
 
 from .check import register_check_parser
+from .fix import register_fix_parser
 from .generate import register_generate_parser
 from .get import register_get_parser
 from .keygen import register_keygen_parser
@@ -52,6 +53,23 @@ Examples:
     )
     register_check_parser(check_parser)
 
+    fix_parser = subparser.add_parser(
+        "fix",
+        help="fix inconsistencies in the vars store",
+        epilog=(
+            """
+This subcommand allows fixing of inconsistencies in the vars store.
+
+Examples:
+
+  $ clan vars fix [MACHINE]
+  Will fix vars for the specified machine.
+        """
+        ),
+        formatter_class=argparse.RawTextHelpFormatter,
+    )
+    register_fix_parser(fix_parser)
+
     list_parser = subparser.add_parser(
         "list",
         help="list all vars",
@@ -60,7 +78,7 @@ Examples:
 This subcommand allows listing all non-secret vars for a specific machine.
 
 The resulting list will be a json string with the name of the variable as its key
-and the fact itself as it's value.
+and the variable itself as it's value.
 
 This is how an example output might look like:
 ```

pkgs/clan-cli/clan_cli/vars/fix.py

@@ -0,0 +1,57 @@
+import argparse
+import importlib
+import logging
+
+from clan_cli.completions import add_dynamic_completer, complete_machines
+from clan_cli.errors import ClanError
+from clan_cli.machines.machines import Machine
+from clan_cli.vars.public_modules import FactStoreBase
+from clan_cli.vars.secret_modules import SecretStoreBase
+
+log = logging.getLogger(__name__)
+
+
+def fix_vars(machine: Machine, generator_name: None | str = None) -> None:
+    secret_vars_module = importlib.import_module(machine.secret_vars_module)
+    secret_vars_store: SecretStoreBase = secret_vars_module.SecretStore(machine=machine)
+    public_vars_module = importlib.import_module(machine.public_vars_module)
+    public_vars_store: FactStoreBase = public_vars_module.FactStore(machine=machine)
+
+    generators = machine.vars_generators
+    if generator_name:
+        for generator in generators:
+            if generator_name == generator.name:
+                generators = [generator]
+                break
+        else:
+            err_msg = (
+                f"Generator '{generator_name}' not found in machine {machine.name}"
+            )
+            raise ClanError(err_msg)
+
+    for generator in generators:
+        public_vars_store.fix(generator=generator)
+        secret_vars_store.fix(generator=generator)
+
+
+def fix_command(args: argparse.Namespace) -> None:
+    machine = Machine(
+        name=args.machine,
+        flake=args.flake,
+    )
+    fix_vars(machine, generator_name=args.generator)
+
+
+def register_fix_parser(parser: argparse.ArgumentParser) -> None:
+    machines_parser = parser.add_argument(
+        "machine",
+        help="The machine to fix vars for",
+    )
+    add_dynamic_completer(machines_parser, complete_machines)
+
+    parser.add_argument(
+        "--generator",
+        "-g",
+        help="the generator to check",
+    )
+    parser.set_defaults(func=fix_command)

pkgs/clan-cli/clan_cli/vars/generate.py

@@ -392,58 +392,29 @@ def _check_can_migrate(
     )
 
 
-def ensure_consistent_state(
-    machine: "Machine",
-    generator_name: str | None,
-    fix: bool,
-) -> None:
-    """
-    Apply local updates to secrets like re-encrypting with missing keys
-        when new users were added.
-    """
-
-    if generator_name is None:
-        generators = machine.vars_generators
-    else:
-        for generator in machine.vars_generators:
-            if generator_name == generator.name:
-                generators = [generator]
-                break
-        else:
-            err_msg = (
-                f"Could not find generator {generator_name} in machine {machine.name}"
-            )
-            raise ClanError(err_msg)
-    outdated = []
-    for generator in generators:
-        for file in generator.files:
-            if file.secret and machine.secret_vars_store.exists(generator, file.name):
-                if file.deploy:
-                    machine.secret_vars_store.ensure_machine_has_access(
-                        generator, file.name
-                    )
-                needs_update, msg = machine.secret_vars_store.needs_fix(
-                    generator, file.name
-                )
-                if needs_update:
-                    outdated.append((generator_name, file.name, msg))
-    if not fix and outdated:
-        msg = (
-            "The local state of some secret vars is inconsistent and needs to be updated.\n"
-            "Rerun 'clan vars generate' passing '--fix' to apply the necessary changes."
-            "Problems to fix:\n"
-            "\n".join(o[2] for o in outdated if o[2])
-        )
-        raise ClanError(msg)
-
-
 def generate_vars_for_machine(
     machine: "Machine",
     generator_name: str | None,
     regenerate: bool,
-    fix: bool,
 ) -> bool:
-    ensure_consistent_state(machine, generator_name, fix)
+    _generator = None
+    if generator_name:
+        for generator in machine.vars_generators:
+            if generator.name == generator_name:
+                _generator = generator
+                break
+
+    pub_healtcheck_msg = machine.public_vars_store.health_check(_generator)
+    sec_healtcheck_msg = machine.secret_vars_store.health_check(_generator)
+
+    if pub_healtcheck_msg or sec_healtcheck_msg:
+        msg = f"Health check failed for machine {machine.name}:\n"
+        if pub_healtcheck_msg:
+            msg += f"Public vars store: {pub_healtcheck_msg}\n"
+        if sec_healtcheck_msg:
+            msg += f"Secret vars store: {sec_healtcheck_msg}"
+        raise ClanError(msg)
+
     closure = get_closure(machine, generator_name, regenerate)
     if len(closure) == 0:
         return False
@@ -467,14 +438,13 @@ def generate_vars(
     machines: list["Machine"],
     generator_name: str | None = None,
     regenerate: bool = False,
-    fix: bool = False,
 ) -> bool:
     was_regenerated = False
     for machine in machines:
         errors = []
         try:
             was_regenerated |= generate_vars_for_machine(
-                machine, generator_name, regenerate, fix
+                machine, generator_name, regenerate
             )
             machine.flush_caches()
         except Exception as exc:
@@ -502,7 +472,7 @@ def generate_command(args: argparse.Namespace) -> None:
         machines = get_all_machines(args.flake, args.option)
     else:
         machines = get_selected_machines(args.flake, args.option, args.machines)
-    generate_vars(machines, args.generator, args.regenerate, args.fix)
+    generate_vars(machines, args.generator, args.regenerate)
 
 
 def register_generate_parser(parser: argparse.ArgumentParser) -> None:
@@ -532,11 +502,4 @@ def register_generate_parser(parser: argparse.ArgumentParser) -> None:
         default=None,
     )
 
-    parser.add_argument(
-        "--fix",
-        action=argparse.BooleanOptionalAction,
-        help="whether to fix local state inconsistencies, for example if a secret is not encrypted with the correct keys",
-        default=False,
-    )
-
     parser.set_defaults(func=generate_command)

pkgs/clan-cli/clan_cli/vars/secret_modules/__init__.py

@@ -5,7 +5,7 @@ from typing import TYPE_CHECKING
 from clan_cli.vars._types import StoreBase
 
 if TYPE_CHECKING:
-    from clan_cli.vars.generate import Generator
+    pass
 
 
 class SecretStoreBase(StoreBase):
@@ -13,28 +13,6 @@ class SecretStoreBase(StoreBase):
     def is_secret_store(self) -> bool:
         return True
 
-    def ensure_machine_has_access(self, generator: "Generator", name: str) -> None:
-        pass
-
-    def needs_fix(
-        self,
-        generator: "Generator",
-        name: str,
-    ) -> tuple[bool, str | None]:
-        """
-        Check if local state needs updating, eg. secret needs to be re-encrypted with new keys
-        """
-        return False, None
-
-    def fix(
-        self,
-        generator: "Generator",
-        name: str,
-    ) -> None:
-        """
-        Update local state, eg make sure secret is encrypted with correct keys
-        """
-
     @abstractmethod
     def populate_dir(self, output_dir: Path) -> None:
         pass

pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py

@@ -87,6 +87,48 @@ class SecretStore(SecretStoreBase):
     def secret_path(self, generator: Generator, secret_name: str) -> Path:
         return self.directory(generator, secret_name)
 
+    @override
+    def health_check(
+        self, generator: Generator | None = None, file_name: str | None = None
+    ) -> str | None:
+        """
+        Apply local updates to secrets like re-encrypting with missing keys
+            when new users were added.
+        """
+
+        if generator is None:
+            generators = self.machine.vars_generators
+        else:
+            generators = [generator]
+        file_found = False
+        outdated = []
+        for generator in generators:
+            for file in generator.files:
+                # if we check only a single file, continue on all the other ones
+                if file_name:
+                    if file.name == file_name:
+                        file_found = True
+                    else:
+                        continue
+                if file.secret and self.exists(generator, file.name):
+                    if file.deploy:
+                        self.ensure_machine_has_access(generator, file.name)
+                    needs_update, msg = self.needs_fix(generator, file.name)
+                    if needs_update:
+                        outdated.append((generator.name, file.name, msg))
+        if file_name and not file_found:
+            msg = f"file {file_name} was not found"
+            raise ClanError(msg)
+        if outdated:
+            msg = (
+                "The local state of some secret vars is inconsistent and needs to be updated.\n"
+                "Run 'clan vars fix' to apply the necessary changes."
+                "Problems to fix:\n"
+                "\n".join(o[2] for o in outdated if o[2])
+            )
+            return msg
+        return None
+
     def _set(
         self,
         generator: Generator,
@@ -183,11 +225,30 @@ class SecretStore(SecretStoreBase):
         return needs_update, msg
 
     @override
-    def fix(self, generator: Generator, name: str) -> None:
+    def fix(
+        self, generator: Generator | None = None, file_name: str | None = None
+    ) -> None:
         from clan_cli.secrets.secrets import update_keys
 
-        secret_path = self.secret_path(generator, name)
+        if generator is None:
-        update_keys(
+            generators = self.machine.vars_generators
-            secret_path,
+        else:
-            collect_keys_for_path(secret_path),
+            generators = [generator]
-        )
+        file_found = False
+        for generator in generators:
+            for file in generator.files:
+                # if we check only a single file, continue on all the other ones
+                if file_name:
+                    if file.name == file_name:
+                        file_found = True
+                    else:
+                        continue
+
+                secret_path = self.secret_path(generator, file.name)
+                update_keys(
+                    secret_path,
+                    collect_keys_for_path(secret_path),
+                )
+        if file_name and not file_found:
+            msg = f"file {file_name} was not found"
+            raise ClanError(msg)

pkgs/clan-cli/tests/test_vars.py

@@ -211,7 +211,7 @@ def test_generate_secret_var_sops_with_default_group(
     with pytest.raises(ClanError):
         cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine"])
     # apply fix
-    cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine", "--fix"])
+    cli.run(["vars", "fix", "--flake", str(flake.path), "my_machine"])
     # check if new user can access the secret
     monkeypatch.setenv("USER", "uschi")
     assert sops_store.user_has_access(
@@ -765,7 +765,6 @@ def test_stdout_of_generate(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             "my_generator",
             regenerate=False,
-            fix=False,
         )
 
     assert "Updated var my_generator/my_value" in caplog.text
@@ -779,7 +778,6 @@ def test_stdout_of_generate(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             "my_generator",
             regenerate=True,
-            fix=False,
         )
     assert "Updated var my_generator/my_value" in caplog.text
     assert "old: world" in caplog.text
@@ -791,7 +789,6 @@ def test_stdout_of_generate(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             "my_generator",
             regenerate=True,
-            fix=False,
         )
     assert "Updated" not in caplog.text
     assert "hello" in caplog.text
@@ -801,7 +798,6 @@ def test_stdout_of_generate(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             "my_secret_generator",
             regenerate=False,
-            fix=False,
         )
     assert "Updated secret var my_secret_generator/my_secret" in caplog.text
     assert "hello" not in caplog.text
@@ -817,7 +813,6 @@ def test_stdout_of_generate(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             "my_secret_generator",
             regenerate=True,
-            fix=False,
         )
     assert "Updated secret var my_secret_generator/my_secret" in caplog.text
     assert "world" not in caplog.text
@@ -919,7 +914,6 @@ def test_fails_when_files_are_left_from_other_backend(
             Machine(name="my_machine", flake=FlakeId(str(flake.path))),
             generator,
             regenerate=False,
-            fix=False,
         )
     my_secret_generator["files"]["my_secret"]["secret"] = False
     my_value_generator["files"]["my_value"]["secret"] = True
@@ -931,7 +925,6 @@ def test_fails_when_files_are_left_from_other_backend(
                 Machine(name="my_machine", flake=FlakeId(str(flake.path))),
                 generator,
                 regenerate=False,
-                fix=False,
             )