From a62b127aca961d79188474aa416aff63e0139584 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 23 Apr 2020 04:18:23 +0200 Subject: [PATCH 1/9] Fix improper use of logger.exception in app.py --- src/yunohost/app.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 4e4878f9e..41c2f97a3 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -736,7 +736,7 @@ def app_upgrade(app=[], url=None, file=None): upgrade_failed = True if upgrade_retcode != 0 else False if upgrade_failed: error = m18n.n('app_upgrade_script_failed') - logger.exception(m18n.n("app_upgrade_failed", app=app_instance_name, error=error)) + logger.error(m18n.n("app_upgrade_failed", app=app_instance_name, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) if msettings.get('interface') != 'api': dump_app_log_extract_for_debugging(operation_logger) @@ -744,13 +744,13 @@ def app_upgrade(app=[], url=None, file=None): except (KeyboardInterrupt, EOFError): upgrade_retcode = -1 error = m18n.n('operation_interrupted') - logger.exception(m18n.n("app_upgrade_failed", app=app_instance_name, error=error)) + logger.error(m18n.n("app_upgrade_failed", app=app_instance_name, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) # Something wrong happened in Yunohost's code (most probably hook_exec) except Exception: import traceback error = m18n.n('unexpected_error', error=u"\n" + traceback.format_exc()) - logger.exception(m18n.n("app_install_failed", app=app_instance_name, error=error)) + logger.error(m18n.n("app_install_failed", app=app_instance_name, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) finally: # Whatever happened (install success or failure) we check if it broke the system @@ -760,7 +760,7 @@ def app_upgrade(app=[], url=None, file=None): _assert_system_is_sane_for_app(manifest, "post") except Exception as e: broke_the_system = True - logger.exception(m18n.n("app_upgrade_failed", app=app_instance_name, error=str(e))) + logger.error(m18n.n("app_upgrade_failed", app=app_instance_name, error=str(e))) failure_message_with_debug_instructions = operation_logger.error(str(e)) # If upgrade failed or broke the system, @@ -1002,20 +1002,20 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu install_failed = True if install_retcode != 0 else False if install_failed: error = m18n.n('app_install_script_failed') - logger.exception(m18n.n("app_install_failed", app=app_id, error=error)) + logger.error(m18n.n("app_install_failed", app=app_id, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) if msettings.get('interface') != 'api': dump_app_log_extract_for_debugging(operation_logger) # Script got manually interrupted ... N.B. : KeyboardInterrupt does not inherit from Exception except (KeyboardInterrupt, EOFError): error = m18n.n('operation_interrupted') - logger.exception(m18n.n("app_install_failed", app=app_id, error=error)) + logger.error(m18n.n("app_install_failed", app=app_id, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) # Something wrong happened in Yunohost's code (most probably hook_exec) except Exception as e: import traceback error = m18n.n('unexpected_error', error=u"\n" + traceback.format_exc()) - logger.exception(m18n.n("app_install_failed", app=app_id, error=error)) + logger.error(m18n.n("app_install_failed", app=app_id, error=error)) failure_message_with_debug_instructions = operation_logger.error(error) finally: # Whatever happened (install success or failure) we check if it broke the system @@ -1025,7 +1025,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu _assert_system_is_sane_for_app(manifest, "post") except Exception as e: broke_the_system = True - logger.exception(m18n.n("app_install_failed", app=app_id, error=str(e))) + logger.error(m18n.n("app_install_failed", app=app_id, error=str(e))) failure_message_with_debug_instructions = operation_logger.error(str(e)) # If the install failed or broke the system, we remove it @@ -1062,7 +1062,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu except (KeyboardInterrupt, EOFError, Exception): remove_retcode = -1 import traceback - logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) + logger.error(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) # Remove all permission in LDAP for permission_name in user_permission_list()["permissions"].keys(): @@ -1234,7 +1234,7 @@ def app_remove(operation_logger, app): except (KeyboardInterrupt, EOFError, Exception): ret = -1 import traceback - logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) + logger.error(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) if ret == 0: logger.success(m18n.n('app_removed', app=app)) @@ -2197,7 +2197,7 @@ def _get_app_settings(app_id): if app_id == settings['id']: return settings except (IOError, TypeError, KeyError): - logger.exception(m18n.n('app_not_correctly_installed', + logger.error(m18n.n('app_not_correctly_installed', app=app_id)) return {} From 428f0a61fc074a996c982c239d4c6a457076437d Mon Sep 17 00:00:00 2001 From: Maniack Crudelis Date: Sun, 19 Apr 2020 18:15:58 +0200 Subject: [PATCH 2/9] Wait for fail2ban to reload --- data/helpers.d/fail2ban | 2 +- data/helpers.d/systemd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/helpers.d/fail2ban b/data/helpers.d/fail2ban index 58af9ec0b..40f435ecd 100644 --- a/data/helpers.d/fail2ban +++ b/data/helpers.d/fail2ban @@ -130,7 +130,7 @@ EOF ynh_store_file_checksum "$finalfail2banjailconf" ynh_store_file_checksum "$finalfail2banfilterconf" - ynh_systemd_action --service_name=fail2ban --action=reload + ynh_systemd_action --service_name=fail2ban --action=reload --line_match="(Started|Reloaded) Fail2Ban Service" --log_path=systemd local fail2ban_error="$(journalctl -u fail2ban | tail -n50 | grep "WARNING.*$app.*")" if [[ -n "$fail2ban_error" ]]; then diff --git a/data/helpers.d/systemd b/data/helpers.d/systemd index 960382f8f..2c290ad64 100644 --- a/data/helpers.d/systemd +++ b/data/helpers.d/systemd @@ -133,7 +133,7 @@ ynh_systemd_action() { for i in $(seq 1 $timeout) do # Read the log until the sentence is found, that means the app finished to start. Or run until the timeout - if grep --quiet "$line_match" "$templog" + if grep --extended-regexp --quiet "$line_match" "$templog" then ynh_print_info --message="The service $service_name has correctly started." break From 1ba08be8fb3cc9d177c74bc8620b14678d97eae1 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 23 Apr 2020 17:02:58 +0200 Subject: [PATCH 3/9] Make sure to return / and not empty string for stuff on domain root --- src/yunohost/app.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 41c2f97a3..69ea10928 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -455,6 +455,8 @@ def app_map(app=None, raw=False, user=None): perm_domain, perm_path = perm_url.split("/", 1) perm_path = "/" + perm_path.rstrip("/") + perm_path = perm_path if perm_path != "" else "/" + return perm_domain, perm_path this_app_perms = {p: i for p, i in permissions.items() if p.startswith(app_id + ".") and i["url"]} @@ -490,7 +492,6 @@ def app_map(app=None, raw=False, user=None): continue perm_domain, perm_path = _sanitized_absolute_url(perm_info["url"]) - if perm_name.endswith(".main"): perm_label = label else: @@ -1362,11 +1363,12 @@ def app_makedefault(operation_logger, app, domain=None): elif domain not in domain_list()['domains']: raise YunohostError('domain_unknown') - operation_logger.start() if '/' in app_map(raw=True)[domain]: raise YunohostError('app_make_default_location_already_used', app=app, domain=app_domain, other_app=app_map(raw=True)[domain]["/"]["id"]) + operation_logger.start() + # TODO / FIXME : current trick is to add this to conf.json.persisten # This is really not robust and should be improved # e.g. have a flag in /etc/yunohost/apps/$app/ to say that this is the @@ -1636,6 +1638,8 @@ def app_ssowatconf(): perm_domain, perm_path = perm_url.split("/", 1) perm_path = "/" + perm_path.rstrip("/") + perm_path = perm_path if perm_path != "" else "/" + return perm_domain + perm_path # Skipped From 794640a6739981560a6221988b4dbf2b5cecd847 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 25 Apr 2020 23:52:55 +0200 Subject: [PATCH 4/9] Make sure to strip() the path just in case Co-Authored-By: Bram --- src/yunohost/app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 69ea10928..8c52f4928 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -455,7 +455,7 @@ def app_map(app=None, raw=False, user=None): perm_domain, perm_path = perm_url.split("/", 1) perm_path = "/" + perm_path.rstrip("/") - perm_path = perm_path if perm_path != "" else "/" + perm_path = perm_path if perm_path.strip() != "" else "/" return perm_domain, perm_path @@ -1638,7 +1638,7 @@ def app_ssowatconf(): perm_domain, perm_path = perm_url.split("/", 1) perm_path = "/" + perm_path.rstrip("/") - perm_path = perm_path if perm_path != "" else "/" + perm_path = perm_path if perm_path.strip() != "" else "/" return perm_domain + perm_path From 01a6aa13719bf99ed7cf6210b6fa669ee15725ce Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 21 Apr 2020 04:45:16 +0200 Subject: [PATCH 5/9] Force-flush the regen-conf for nginx domain conf when adding/removing a domain... --- src/yunohost/domain.py | 32 +++++++++++++++++++++++++++++++- src/yunohost/regenconf.py | 12 ++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/yunohost/domain.py b/src/yunohost/domain.py index 18c4bd8e2..5ef6ef650 100644 --- a/src/yunohost/domain.py +++ b/src/yunohost/domain.py @@ -33,7 +33,7 @@ from yunohost.utils.error import YunohostError from moulinette.utils.log import getActionLogger from yunohost.app import app_ssowatconf -from yunohost.regenconf import regen_conf +from yunohost.regenconf import regen_conf, _force_clear_hashes, _process_regen_conf from yunohost.utils.network import get_public_ip from yunohost.log import is_unit_operation from yunohost.hook import hook_callback @@ -119,6 +119,17 @@ def domain_add(operation_logger, domain, dyndns=False): # Don't regen these conf if we're still in postinstall if os.path.exists('/etc/yunohost/installed'): + # Sometime we have weird issues with the regenconf where some files + # appears as manually modified even though they weren't touched ... + # There are a few ideas why this happens (like backup/restore nginx + # conf ... which we shouldnt do ...). This in turns creates funky + # situation where the regenconf may refuse to re-create the conf + # (when re-creating a domain..) + # So here we force-clear the has out of the regenconf if it exists. + # This is a pretty ad hoc solution and only applied to nginx + # because it's one of the major service, but in the long term we + # should identify the root of this bug... + _force_clear_hashes(["/etc/nginx/conf.d/%s.conf" % domain]) regen_conf(names=['nginx', 'metronome', 'dnsmasq', 'postfix', 'rspamd']) app_ssowatconf() @@ -176,6 +187,25 @@ def domain_remove(operation_logger, domain, force=False): os.system('rm -rf /etc/yunohost/certs/%s' % domain) + # Sometime we have weird issues with the regenconf where some files + # appears as manually modified even though they weren't touched ... + # There are a few ideas why this happens (like backup/restore nginx + # conf ... which we shouldnt do ...). This in turns creates funky + # situation where the regenconf may refuse to re-create the conf + # (when re-creating a domain..) + # + # So here we force-clear the has out of the regenconf if it exists. + # This is a pretty ad hoc solution and only applied to nginx + # because it's one of the major service, but in the long term we + # should identify the root of this bug... + _force_clear_hashes(["/etc/nginx/conf.d/%s.conf" % domain]) + # And in addition we even force-delete the file Otherwise, if the file was + # manually modified, it may not get removed by the regenconf which leads to + # catastrophic consequences of nginx breaking because it can't load the + # cert file which disappeared etc.. + if os.path.exists("/etc/nginx/conf.d/%s.conf" % domain): + _process_regen_conf("/etc/nginx/conf.d/%s.conf" % domain, new_conf=None, save=True) + regen_conf(names=['nginx', 'metronome', 'dnsmasq', 'postfix']) app_ssowatconf() diff --git a/src/yunohost/regenconf.py b/src/yunohost/regenconf.py index b7a42dd9d..fea6dbea7 100644 --- a/src/yunohost/regenconf.py +++ b/src/yunohost/regenconf.py @@ -463,6 +463,18 @@ def _update_conf_hashes(category, hashes): _save_regenconf_infos(categories) +def _force_clear_hashes(paths): + + categories = _get_regenconf_infos() + for path in paths: + for category in categories.keys(): + if path in categories[category]['conffiles']: + logger.debug("force-clearing old conf hash for %s in category %s" % (path, category)) + del categories[category]['conffiles'][path] + + _save_regenconf_infos(categories) + + def _process_regen_conf(system_conf, new_conf=None, save=True): """Regenerate a given system configuration file From 34fd4e90bd3ffcdf5ac067159c16251896dcfc7c Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 21 Apr 2020 04:48:13 +0200 Subject: [PATCH 6/9] Be more robust against broken config or service failing to start, show info to help debugging --- data/hooks/conf_regen/15-nginx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/data/hooks/conf_regen/15-nginx b/data/hooks/conf_regen/15-nginx index 55a5494b2..87cc9b5b9 100755 --- a/data/hooks/conf_regen/15-nginx +++ b/data/hooks/conf_regen/15-nginx @@ -26,7 +26,8 @@ do_init_regen() { ynh_render_template "yunohost_admin.conf" "${nginx_conf_dir}/yunohost_admin.conf" # Restart nginx if conf looks good, otherwise display error and exit unhappy - nginx -t 2>/dev/null && service nginx restart || (nginx -t && exit 1) + nginx -t 2>/dev/null || { nginx -t; exit 1; } + systemctl restart nginx || { journalctl --no-pager --lines=10 -u nginx >&2; exit 1; } exit 0 } @@ -109,8 +110,9 @@ do_post_regen() { mkdir -p "/etc/nginx/conf.d/${domain}.d" done - # Reload nginx configuration - pgrep nginx && service nginx reload + # Reload nginx if conf looks good, otherwise display error and exit unhappy + nginx -t 2>/dev/null || { nginx -t; exit 1; } + pgrep nginx && systemctl reload nginx || { journalctl --no-pager --lines=10 -u nginx >&2; exit 1; } } FORCE=${2:-0} From 176d0176db3b3b90a1e86eaa9e0ed370fc1f1187 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 24 Apr 2020 03:08:31 +0200 Subject: [PATCH 7/9] Be more robust against some situation where archive is corrupted --- locales/en.json | 3 ++- src/yunohost/backup.py | 42 +++++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/locales/en.json b/locales/en.json index 6a2af5e41..9ffa35ab6 100644 --- a/locales/en.json +++ b/locales/en.json @@ -88,6 +88,8 @@ "backup_archive_name_exists": "A backup archive with this name already exists.", "backup_archive_name_unknown": "Unknown local backup archive named '{name:s}'", "backup_archive_open_failed": "Could not open the backup archive", + "backup_archive_cant_retrieve_info_json": "Could not load infos for archive '{archive}' ... The info.json cannot be retrieved (or is not a valid json).", + "backup_archive_corrupted": "It looks like the backup archive '{archive}' is corrupted : {error}", "backup_archive_system_part_not_available": "System part '{part:s}' unavailable in this backup", "backup_archive_writing_error": "Could not add the files '{source:s}' (named in the archive '{dest:s}') to be backed up into the compressed archive '{archive:s}'", "backup_ask_for_copying_if_needed": "Do you want to perform the backup using {size:s} MB temporarily? (This way is used since some files could not be prepared using a more efficient method.)", @@ -105,7 +107,6 @@ "backup_delete_error": "Could not delete '{path:s}'", "backup_deleted": "Backup deleted", "backup_hook_unknown": "The backup hook '{hook:s}' is unknown", - "backup_invalid_archive": "This is not a backup archive", "backup_method_borg_finished": "Backup into Borg finished", "backup_method_copy_finished": "Backup copy finalized", "backup_method_custom_finished": "Custom backup method '{method:s}' finished", diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index 3344d2807..5e90bce6c 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -871,7 +871,7 @@ class RestoreManager(): Read the info file from inside an archive Exceptions: - backup_invalid_archive -- Raised if we can't read the info + backup_archive_cant_retrieve_info_json -- Raised if we can't read the info """ # Retrieve backup info info_file = os.path.join(self.work_dir, "info.json") @@ -884,7 +884,7 @@ class RestoreManager(): self.info["system"] = self.info["hooks"] except IOError: logger.debug("unable to load '%s'", info_file, exc_info=1) - raise YunohostError('backup_invalid_archive') + raise YunohostError('backup_archive_cant_retrieve_info_json', archive=self.archive_path) else: logger.debug("restoring from backup '%s' created on %s", self.name, datetime.utcfromtimestamp(self.info['created_at'])) @@ -892,10 +892,6 @@ class RestoreManager(): def _postinstall_if_needed(self): """ Post install yunohost if needed - - Exceptions: - backup_invalid_archive -- Raised if the current_host isn't in the - archive """ # Check if YunoHost is installed if not os.path.isfile('/etc/yunohost/installed'): @@ -907,7 +903,7 @@ class RestoreManager(): logger.debug("unable to retrieve current_host from the backup", exc_info=1) # FIXME include the current_host by default ? - raise YunohostError('backup_invalid_archive') + raise YunohostError("The main domain name cannot be retrieved from inside the archive, and is needed to perform the postinstall", raw_msg=True) logger.debug("executing the post-install...") tools_postinstall(domain, 'Yunohost', True) @@ -1924,6 +1920,12 @@ class TarBackupMethod(BackupMethod): self._archive_file, exc_info=1) raise YunohostError('backup_archive_open_failed') + try: + files_in_archive = tar.getnames() + print(files_in_archive) + except IOError as e: + raise YunohostError("backup_archive_corrupted", archive=self._archive_file, error=str(e)) + # FIXME : Is this really useful to close the archive just to # reopen it right after this with the same options ...? tar.close() @@ -1932,21 +1934,21 @@ class TarBackupMethod(BackupMethod): logger.debug(m18n.n("restore_extracting")) tar = tarfile.open(self._archive_file, "r:gz") - if "info.json" in tar.getnames(): + if "info.json" in files_in_archive: leading_dot = "" tar.extract('info.json', path=self.work_dir) - elif "./info.json" in tar.getnames(): + elif "./info.json" in files_in_archive: leading_dot = "./" tar.extract('./info.json', path=self.work_dir) else: logger.debug("unable to retrieve 'info.json' inside the archive", exc_info=1) tar.close() - raise YunohostError('backup_invalid_archive') + raise YunohostError('backup_archive_cant_retrieve_info_json', archive=self._archive_file) - if "backup.csv" in tar.getnames(): + if "backup.csv" in files_in_archive: tar.extract('backup.csv', path=self.work_dir) - elif "./backup.csv" in tar.getnames(): + elif "./backup.csv" in files_in_archive: tar.extract('./backup.csv', path=self.work_dir) else: # Old backup archive have no backup.csv file @@ -2288,7 +2290,7 @@ def backup_list(with_info=False, human_readable=False): try: d[a] = backup_info(a, human_readable=human_readable) except YunohostError as e: - logger.warning('%s: %s' % (a, e.strerror)) + logger.warning(str(e)) result = d @@ -2325,17 +2327,23 @@ def backup_info(name, with_details=False, human_readable=False): if not os.path.exists(info_file): tar = tarfile.open(archive_file, "r:gz") info_dir = info_file + '.d' + try: - if "info.json" in tar.getnames(): + files_in_archive = tar.getnames() + except IOError as e: + raise YunohostError("backup_archive_corrupted", archive=archive_file, error=str(e)) + + try: + if "info.json" in files_in_archive: tar.extract('info.json', path=info_dir) - elif "./info.json" in tar.getnames(): + elif "./info.json" in files_in_archive: tar.extract('./info.json', path=info_dir) else: raise KeyError except KeyError: logger.debug("unable to retrieve '%s' inside the archive", info_file, exc_info=1) - raise YunohostError('backup_invalid_archive') + raise YunohostError('backup_archive_cant_retrieve_info_json', archive=archive_file) else: shutil.move(os.path.join(info_dir, 'info.json'), info_file) finally: @@ -2348,7 +2356,7 @@ def backup_info(name, with_details=False, human_readable=False): info = json.load(f) except: logger.debug("unable to load '%s'", info_file, exc_info=1) - raise YunohostError('backup_invalid_archive') + raise YunohostError('backup_archive_cant_retrieve_info_json', archive=archive_file) # Retrieve backup size size = info.get('size', 0) From aea8b97993084b61fb63f9e8d2a56772bd473de7 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 25 Apr 2020 01:27:20 +0200 Subject: [PATCH 8/9] Remove tmp debug print() Co-Authored-By: Kayou --- src/yunohost/backup.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index 5e90bce6c..452d87361 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -1922,7 +1922,6 @@ class TarBackupMethod(BackupMethod): try: files_in_archive = tar.getnames() - print(files_in_archive) except IOError as e: raise YunohostError("backup_archive_corrupted", archive=self._archive_file, error=str(e)) From d91966ca98a2b4829695560cb4cb99dd46ea61ca Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 27 Apr 2020 23:48:46 +0200 Subject: [PATCH 9/9] Update changelog for 3.7.1.2 --- debian/changelog | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/debian/changelog b/debian/changelog index 6245bb4b0..fcef69c4f 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,14 @@ +yunohost (3.7.1.2) stable; urgency=low + + - [fix] Be more robust against some situation where some archives are corrupted + - [fix] Make nginx regen-conf more robust against broken config or service failing to start, show info to help debugging + - [fix] Force-flush the regen-conf for nginx domain conf when adding/removing a domain... + - [fix] app_map : Make sure to return / and not empty string for stuff on domain root + - [fix] Improve ynh_systemd_action to wait for fail2ban to reload + - [fix] Improper use of logger.exception in app.py leading to infamous weird "KeyError: label" + + -- Alexandre Aubin Mon, 27 Apr 2020 23:50:00 +0000 + yunohost (3.7.1.1) stable; urgency=low - [fix] lxc uid number is limited to 65536 by default (0c9a4509)