From 4c6786e8afe13ad703d80c6ddaf39630b3aacd70 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 3 Jan 2022 18:02:41 +0100 Subject: [PATCH] manifestv2, appresources: Implement a more generic 'apply' mecanism with possible rollback --- src/app.py | 34 ++++++---- src/backup.py | 10 +++ src/tests/test_app_resources.py | 115 ++++++++++++++++++++++++++++++++ src/utils/resources.py | 97 ++++++++++++++++++++------- 4 files changed, 218 insertions(+), 38 deletions(-) create mode 100644 src/tests/test_app_resources.py diff --git a/src/app.py b/src/app.py index 2e7c428d7..2f3f3e835 100644 --- a/src/app.py +++ b/src/app.py @@ -546,6 +546,7 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False if manifest["packaging_format"] >= 2: if no_safety_backup: + # FIXME: i18n logger.warning("Skipping the creation of a backup prior to the upgrade.") else: # FIXME: i18n @@ -570,8 +571,17 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False _assert_system_is_sane_for_app(manifest, "pre") + # We'll check that the app didn't brutally edit some system configuration + manually_modified_files_before_install = manually_modified_files() + app_setting_path = os.path.join(APPS_SETTING_PATH, app_instance_name) + # Attempt to patch legacy helpers ... + _patch_legacy_helpers(extracted_app_folder) + + # Apply dirty patch to make php5 apps compatible with php7 + _patch_legacy_php_versions(extracted_app_folder) + # Prepare env. var. to pass to script env_dict = _make_environment_for_app_script( app_instance_name, workdir=extracted_app_folder, action="upgrade" @@ -582,20 +592,19 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False if manifest["packaging_format"] < 2: env_dict["NO_BACKUP_UPGRADE"] = "1" if no_safety_backup else "0" - # We'll check that the app didn't brutally edit some system configuration - manually_modified_files_before_install = manually_modified_files() - - # Attempt to patch legacy helpers ... - _patch_legacy_helpers(extracted_app_folder) - - # Apply dirty patch to make php5 apps compatible with php7 - _patch_legacy_php_versions(extracted_app_folder) - # Start register change on system related_to = [("app", app_instance_name)] operation_logger = OperationLogger("app_upgrade", related_to, env=env_dict) operation_logger.start() + if manifest["packaging_format"] >= 2: + from yunohost.utils.resources import AppResourceManager + try: + AppResourceManager(app_instance_name, wanted=manifest, current=app_dict["manifest"]).apply(rollback_if_failure=True) + except Exception: + # FIXME : improve error handling .... + raise + # Execute the app upgrade script upgrade_failed = True try: @@ -880,10 +889,9 @@ def app_install( if packaging_format >= 2: from yunohost.utils.resources import AppResourceManager try: - AppResourceManager(app_instance_name, wanted=manifest, current={}).apply() + AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(rollback_if_failure=True) except Exception: # FIXME : improve error handling .... - AppResourceManager(app_instance_name, wanted={}, current=manifest).apply() raise else: # Initialize the main permission for the app @@ -997,7 +1005,7 @@ def app_install( if packaging_format >= 2: from yunohost.utils.resources import AppResourceManager try: - AppResourceManager(app_instance_name, wanted={}, current=manifest).apply() + AppResourceManager(app_instance_name, wanted={}, current=manifest).apply(rollback_if_failure=False) except Exception: # FIXME : improve error handling .... raise @@ -1109,7 +1117,7 @@ def app_remove(operation_logger, app, purge=False): if packaging_format >= 2: try: from yunohost.utils.resources import AppResourceManager - AppResourceManager(app, wanted={}, current=manifest).apply() + AppResourceManager(app, wanted={}, current=manifest).apply(rollback_if_failure=False) except Exception: # FIXME : improve error handling .... raise diff --git a/src/backup.py b/src/backup.py index 1e16435b4..c2d921548 100644 --- a/src/backup.py +++ b/src/backup.py @@ -49,6 +49,7 @@ from yunohost.app import ( _is_installed, _make_environment_for_app_script, _make_tmp_workdir_for_app, + _get_manifest_of_app, ) from yunohost.hook import ( hook_list, @@ -1512,6 +1513,15 @@ class RestoreManager: operation_logger.extra["env"] = env_dict operation_logger.flush() + manifest = _get_manifest_of_app(app_dir_in_archive) + if manifest["packaging_format"] >= 2: + from yunohost.utils.resources import AppResourceManager + try: + AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(rollback_if_failure=True) + except Exception: + # FIXME : improve error handling .... + raise + # Execute the app install script restore_failed = True try: diff --git a/src/tests/test_app_resources.py b/src/tests/test_app_resources.py new file mode 100644 index 000000000..d3abddc7f --- /dev/null +++ b/src/tests/test_app_resources.py @@ -0,0 +1,115 @@ +import os +import pytest + +from yunohost.utils.resources import AppResource, AppResourceManager, AppResourceClassesByType + +dummyfile = "/tmp/dummyappresource-testapp" + + +class DummyAppResource(AppResource): + + type = "dummy" + + default_properties = { + "file": "/tmp/dummyappresource-__APP__", + "content": "foo", + } + + def provision_or_update(self, context): + + open(self.file, "w").write(self.content) + + if self.content == "forbiddenvalue": + raise Exception("Emeged you used the forbidden value!1!£&") + + def deprovision(self, context): + + os.system(f"rm -f {self.file}") + + +AppResourceClassesByType["dummy"] = DummyAppResource + + +def setup_function(function): + + clean() + + +def teardown_function(function): + + clean() + + +def clean(): + + os.system(f"rm -f {dummyfile}") + + +def test_provision_dummy(): + + current = {"resources": {}} + wanted = {"resources": {"dummy": {}}} + + assert not os.path.exists(dummyfile) + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False) + assert open(dummyfile).read().strip() == "foo" + + +def test_deprovision_dummy(): + + current = {"resources": {"dummy": {}}} + wanted = {"resources": {}} + + open(dummyfile, "w").write("foo") + + assert open(dummyfile).read().strip() == "foo" + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False) + assert not os.path.exists(dummyfile) + + +def test_provision_dummy_nondefaultvalue(): + + current = {"resources": {}} + wanted = {"resources": {"dummy": {"content": "bar"}}} + + assert not os.path.exists(dummyfile) + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False) + assert open(dummyfile).read().strip() == "bar" + + +def test_update_dummy(): + + current = {"resources": {"dummy": {}}} + wanted = {"resources": {"dummy": {"content": "bar"}}} + + open(dummyfile, "w").write("foo") + + assert open(dummyfile).read().strip() == "foo" + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False) + assert open(dummyfile).read().strip() == "bar" + + +def test_update_dummy_fail(): + + current = {"resources": {"dummy": {}}} + wanted = {"resources": {"dummy": {"content": "forbiddenvalue"}}} + + open(dummyfile, "w").write("foo") + + assert open(dummyfile).read().strip() == "foo" + with pytest.raises(Exception): + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False) + assert open(dummyfile).read().strip() == "forbiddenvalue" + + +def test_update_dummy_failwithrollback(): + + current = {"resources": {"dummy": {}}} + wanted = {"resources": {"dummy": {"content": "forbiddenvalue"}}} + + open(dummyfile, "w").write("foo") + + assert open(dummyfile).read().strip() == "foo" + with pytest.raises(Exception): + AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=True) + assert open(dummyfile).read().strip() == "foo" diff --git a/src/utils/resources.py b/src/utils/resources.py index 23c1a7a5a..4e9876655 100644 --- a/src/utils/resources.py +++ b/src/utils/resources.py @@ -41,30 +41,77 @@ class AppResourceManager: def __init__(self, app: str, current: Dict, wanted: Dict): self.app = app - self.current = current.get("resources", {}) - self.wanted = wanted.get("resources", {}) + self.current = current + self.wanted = wanted - # c.f. the permission ressources where we need the app label >_> - self.wanted_manifest = wanted + def apply(self, rollback_if_failure, **context): - def apply(self, **context): + todos = list(self.compute_todos()) + completed = [] + rollback = False + exception = None - for name, infos in reversed(self.current.items()): - if name not in self.wanted.keys(): - resource = AppResourceClassesByType[name](infos, self.app, self) - # FIXME : i18n, better info strings - logger.info(f"Deprovisionning {name} ...") - resource.deprovision(context=context) - - for name, infos in self.wanted.items(): - resource = AppResourceClassesByType[name](infos, self.app, self) - if name not in self.current.keys(): - # FIXME : i18n, better info strings - logger.info(f"Provisionning {name} ...") + for todo, name, old, new in todos: + try: + if todo == "deprovision": + # FIXME : i18n, better info strings + logger.info(f"Deprovisionning {name} ...") + old.deprovision(context=context) + elif todo == "provision": + logger.info(f"Provisionning {name} ...") + new.provision_or_update(context=context) + elif todo == "update": + logger.info(f"Updating {name} ...") + new.provision_or_update(context=context) + except Exception as e: + exception = e + # FIXME: better error handling ? display stacktrace ? + logger.warning(f"Failed to {todo} for {name} : {e}") + if rollback_if_failure: + rollback = True + completed.append((todo, name, old, new)) + break + else: + pass else: - # FIXME : i18n, better info strings - logger.info(f"Updating {name} ...") - resource.provision_or_update(context=context) + completed.append((todo, name, old, new)) + + if rollback: + for todo, name, old, new in completed: + try: + # (NB. here we want to undo the todo) + if todo == "deprovision": + # FIXME : i18n, better info strings + logger.info(f"Reprovisionning {name} ...") + old.provision_or_update(context=context) + elif todo == "provision": + logger.info(f"Deprovisionning {name} ...") + new.deprovision(context=context) + elif todo == "update": + logger.info(f"Reverting {name} ...") + old.provision_or_update(context=context) + except Exception as e: + # FIXME: better error handling ? display stacktrace ? + logger.error(f"Failed to rollback {name} : {e}") + + if exception: + raise exception + + def compute_todos(self): + + for name, infos in reversed(self.current["resources"].items()): + if name not in self.wanted["resources"].keys(): + resource = AppResourceClassesByType[name](infos, self.app, self) + yield ("deprovision", name, resource, None) + + for name, infos in self.wanted["resources"].items(): + wanted_resource = AppResourceClassesByType[name](infos, self.app, self) + if name not in self.current["resources"].keys(): + yield ("provision", name, None, wanted_resource) + else: + infos_ = self.current["resources"][name] + current_resource = AppResourceClassesByType[name](infos_, self.app, self) + yield ("update", name, current_resource, wanted_resource) class AppResource: @@ -179,7 +226,7 @@ class PermissionsResource(AppResource): ) # Delete legacy is_public setting if not already done - self.delete_setting(f"is_public") + self.delete_setting("is_public") existing_perms = user_permission_list(short=True, apps=[self.app])["permissions"] for perm in existing_perms: @@ -195,8 +242,8 @@ class PermissionsResource(AppResource): permission_create( f"{self.app}.{perm}", allowed=init_allowed, - # This is why the ugly hack with self.manager and wanted_manifest exists >_> - label=self.manager.wanted_manifest["name"] if perm == "main" else perm, + # This is why the ugly hack with self.manager exists >_> + label=self.manager.wanted["name"] if perm == "main" else perm, url=infos["url"], additional_urls=infos["additional_urls"], auth_header=infos["auth_header"], @@ -499,7 +546,7 @@ class AptDependenciesAppResource(AppResource): "ynh_remove_app_dependencies") -class PortAppResource(AppResource): +class PortResource(AppResource): """ is_provisioned -> port setting exists and is not the port used by another app (ie not in another app setting) is_available -> true @@ -520,7 +567,7 @@ class PortAppResource(AppResource): default_properties = { "default": 1000, - "type": "internal", # FIXME : implement logic for exposed port (allow/disallow in firewall ?) + "expose": False, # FIXME : implement logic for exposed port (allow/disallow in firewall ?) } def _port_is_used(self, port):