From e5f3c52119d55186a3c75a63ef769f448f3f88d9 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 20:27:39 +0100 Subject: [PATCH 01/17] perf(web-app-xwiki): skip the auth-off bootstrap restart when core and add-ons already run on their pins Every redeploy rendered the bootstrap config with OIDC/LDAP off, restarted XWiki, waited ~80 s for REST, checked the add-ons and then switched auth back on with a second restart and wait. A probe now asks the running wiki as superadmin first: when its core version equals the pinned core and every add-on reports INSTALLED at its pin, the bootstrap already renders the final auth config, so the final phase finds nothing to change. Any other answer (fresh install, missing add-on, pin or core bump, unreachable, 401, distribution wizard, 503, any status) falls back to today's two-restart flow; the written xwiki.properties is identical in every case. Verified on echoserver under the live OIDC config: superadmin basic auth answers 200 on /rest/wikis/xwiki/spaces and on bin/view (no Keycloak redirect, trylocal), and /rest/ reports 18.8.0. Replay harness: redeploy on pin 2 -> 0 restarts, with another config change 2 -> 1, all fallback scenarios unchanged. About 190 s per unchanged redeploy. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-xwiki/tasks/00_core.yml | 7 +- roles/web-app-xwiki/tasks/04_extensions.yml | 5 -- .../tasks/utils/extensions_ready.yml | 67 +++++++++++++++++++ roles/web-app-xwiki/vars/main.yml | 1 + 4 files changed, 73 insertions(+), 7 deletions(-) create mode 100644 roles/web-app-xwiki/tasks/utils/extensions_ready.yml diff --git a/roles/web-app-xwiki/tasks/00_core.yml b/roles/web-app-xwiki/tasks/00_core.yml index e785599eaf..c31c986cc6 100644 --- a/roles/web-app-xwiki/tasks/00_core.yml +++ b/roles/web-app-xwiki/tasks/00_core.yml @@ -4,6 +4,9 @@ - name: โœ… Validate XWiki variables include_tasks: 01_validation.yml +- name: "๐Ÿ”Ž Probe whether the running XWiki already carries every add-on on its pin" + include_tasks: utils/extensions_ready.yml + - block: - name: "๐Ÿงฉ load docker, db and proxy for {{ application_id }}" include_role: @@ -25,8 +28,8 @@ include_tasks: utils/flush_config.yml when: IS_STACK_HOST | bool vars: - xwiki_oidc_enabled_switch: false - xwiki_ldap_enabled_switch: false + xwiki_oidc_enabled_switch: "{{ (XWIKI_OIDC_ENABLED | bool) and (xwiki_extensions_ready | bool) }}" + xwiki_ldap_enabled_switch: "{{ (XWIKI_LDAP_ENABLED | bool) and (xwiki_extensions_ready | bool) }}" - name: "๐Ÿ‘” XWiki manager-side operations (file deploys + container exec)" include_tasks: 02_manager_ops.yml diff --git a/roles/web-app-xwiki/tasks/04_extensions.yml b/roles/web-app-xwiki/tasks/04_extensions.yml index 75d9d8a3be..7b8822602d 100644 --- a/roles/web-app-xwiki/tasks/04_extensions.yml +++ b/roles/web-app-xwiki/tasks/04_extensions.yml @@ -3,11 +3,6 @@ # Extension Script Service (services.extension.install). # Avoids REST job API and any Namespace class import for portability. # -# Flow: -# - Bootstrap config renders with both auth backends OFF (already in your role). -# - This file installs required extensions on the current wiki. -# - Final config later turns auth ON (already in your role). -# # Notes: # - We print machine-readable markers so Ansible can assert deterministically. diff --git a/roles/web-app-xwiki/tasks/utils/extensions_ready.yml b/roles/web-app-xwiki/tasks/utils/extensions_ready.yml new file mode 100644 index 0000000000..fc8f82791d --- /dev/null +++ b/roles/web-app-xwiki/tasks/utils/extensions_ready.yml @@ -0,0 +1,67 @@ +--- +- name: "๐Ÿ”Ž XWIKI | Probe whether the running wiki answers the superadmin" + uri_retry: + url: "{{ [XWIKI_REST_XWIKI, 'spaces'] | url_join }}" + method: GET + url_username: "{{ XWIKI_SUPERADMIN_USERNAME }}" + url_password: "{{ XWIKI_SUPERADMIN_PASSWORD }}" + force_basic_auth: true + follow_redirects: none + return_content: false + status_code: "{{ [-1] + range(100, 600) | list }}" + register: _xwiki_running + changed_when: false + when: + - IS_STACK_HOST | bool + - XWIKI_SSO_ENABLED | bool + +- name: "๐Ÿ”Ž XWIKI | Read the core version of the running wiki" + uri_retry: + url: "{{ XWIKI_REST_BASE }}" + method: GET + url_username: "{{ XWIKI_SUPERADMIN_USERNAME }}" + url_password: "{{ XWIKI_SUPERADMIN_PASSWORD }}" + force_basic_auth: true + follow_redirects: none + headers: + Accept: "application/xml" + return_content: true + status_code: "{{ [-1] + range(100, 600) | list }}" + register: _xwiki_running_core + changed_when: false + when: + - _xwiki_running is not skipped + - _xwiki_running.status == 200 + +- name: "๐Ÿงญ XWIKI | Compare the running core with the pinned core {{ XWIKI_CORE_VERSION }}" + set_fact: + _xwiki_core_on_pin: >- + {{ _xwiki_running_core is not skipped + and _xwiki_running_core.status == 200 + and _xwiki_running_core.content is search('' ~ (XWIKI_CORE_VERSION | regex_escape) ~ '') }} + +- name: "โœ… XWIKI | Check the running wiki's add-ons against their pins" + include_tasks: _check_extension_via_groovy.yml + loop: "{{ XWIKI_PLUGINS }}" + loop_control: + loop_var: plugin + label: "{{ plugin.id }}" + vars: + ext_id: "{{ plugin.id }}" + ext_version: "{{ plugin.version }}" + result_var: "xwiki_running_{{ plugin.id | regex_replace('[^A-Za-z0-9_]', '_') }}" + when: _xwiki_core_on_pin | bool + +- name: "๐Ÿงญ XWIKI | Skip the bootstrap boot when core and add-ons run on their pins" + set_fact: + xwiki_extensions_ready: >- + {{ (_xwiki_core_on_pin | bool) + and (XWIKI_PLUGINS + | map(attribute='id') + | map('regex_replace', '[^A-Za-z0-9_]', '_') + | map('regex_replace', '^', 'xwiki_running_') + | map('extract', hostvars[inventory_hostname], 'status') + | map('int') + | reject('equalto', 200) + | list + | length == 0) }} diff --git a/roles/web-app-xwiki/vars/main.yml b/roles/web-app-xwiki/vars/main.yml index 108d09bd82..13471469f1 100644 --- a/roles/web-app-xwiki/vars/main.yml +++ b/roles/web-app-xwiki/vars/main.yml @@ -14,6 +14,7 @@ XWIKI_DOCK_DATA_DIR: "{{ lookup('volume', 'web-app-xwiki', 'data').target }}" XWIKI_IMAGE: "{{ lookup('config', application_id, 'services.xwiki.image') }}" XWIKI_VERSION: "{{ lookup('config', application_id, 'services.xwiki.version') }}" +XWIKI_CORE_VERSION: "{{ (XWIKI_VERSION | string).split('-') | first }}" XWIKI_CONTAINER: "{{ lookup('config', application_id, 'services.xwiki.name') }}" XWIKI_CONTAINER_ADDRESS: "{{ lookup('container_address', application_id, entity_name) }}" From d6ebec8488fa93737f80c5a548c3d730da8c6f84 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 20:52:11 +0100 Subject: [PATCH 02/17] perf(web-app-nextcloud): run the post-upgrade pass only after a version change, take app updates from the daily update check otherwise Every redeploy ran occ maintenance:repair --include-expensive (43.5 s, maintenance mode on) and listed the appstore updates through Tor (49.7 s) although the version had not changed. 03_upgrade.yml now reads occ status and files/mimetype_version after occ upgrade; nextcloud_needs_post_upgrade decides whether the pass is due (version changed in this deploy, or the expensive repair has not run for the running version). A due deploy includes 03_upgrade/01_post_upgrade.yml with today's app update and repair; an unchanged one includes 03_upgrade/02_cached_updates.yml, which updates only the unpinned enabled apps whose release stored by the updatenotification background job is newer (nextcloud_cached_app_updates, PHP version_compare semantics). NEXTCLOUD_PLUGINS_PINNED is the single source of the pinned check. Verified against echoserver's live occ output (status, marker, updatenotification cache, app:list, with debug log lines): unchanged redeploy is not due and finds no update. Scenario harness 24/24 incl. negative controls on the old file. About 86 s per unchanged redeploy. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-nextcloud/README.md | 13 ++ .../filter_plugins/nextcloud_upgrade.py | 158 ++++++++++++++ .../tasks/02_manager_ops/03_upgrade.yml | 58 ++---- .../03_upgrade/01_post_upgrade.yml | 41 ++++ .../03_upgrade/02_cached_updates.yml | 21 ++ roles/web-app-nextcloud/vars/main.yml | 1 + .../filter_plugins/test_nextcloud_upgrade.py | 193 ++++++++++++++++++ 7 files changed, 448 insertions(+), 37 deletions(-) create mode 100644 roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml create mode 100644 roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml diff --git a/roles/web-app-nextcloud/README.md b/roles/web-app-nextcloud/README.md index b396448fe6..456d925c78 100644 --- a/roles/web-app-nextcloud/README.md +++ b/roles/web-app-nextcloud/README.md @@ -215,6 +215,19 @@ After the compose handlers ran, [`tasks/02_manager_ops/03_upgrade.yml`](./tasks/ The steps run in compose mode only. A swarm deploy that finds an older major stops with the planned steps; pin one major above the installed one per deploy there. +### Post-Upgrade Pass + +[`tasks/02_manager_ops/03_upgrade.yml`](./tasks/02_manager_ops/03_upgrade.yml) includes [`03_upgrade/01_post_upgrade.yml`](./tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml), the appstore app update and `occ maintenance:repair --include-expensive`, only while the version `occ status` reports after `occ upgrade` differs from the one `version.php` declared before the deploy or from `files/mimetype_version`, the server version the expensive repair last ran for. +A fresh install, every upgrade path and a deploy that stopped before the repair run the pass, a redeploy of an unchanged version skips it. + +A redeploy of an unchanged version includes [`03_upgrade/02_cached_updates.yml`](./tasks/02_manager_ops/03_upgrade/02_cached_updates.yml) instead, which updates apps without querying the appstore: + +1. `occ app:list --enabled --output=json` reads the installed app versions. +2. `occ config:list updatenotification --output=json` reads the newest release per app that the daily `updatenotification` background job found. +3. `occ app:update ` runs for every enabled app whose stored release is newer than the installed one, never for an app pinned through `archive`. + +An app the background job has not checked yet is not updated until its next run. + ## MCP Server Nextcloud serves a Model Context Protocol endpoint through the AppAPI proxy of the `context_agent` ExApp. diff --git a/roles/web-app-nextcloud/filter_plugins/nextcloud_upgrade.py b/roles/web-app-nextcloud/filter_plugins/nextcloud_upgrade.py index 36c98ef783..37ff20d9e5 100644 --- a/roles/web-app-nextcloud/filter_plugins/nextcloud_upgrade.py +++ b/roles/web-app-nextcloud/filter_plugins/nextcloud_upgrade.py @@ -11,6 +11,19 @@ _OC_VERSION = re.compile( _TAG = re.compile(r"^(?P\d+)(?:\.\d+)*(?P.*)$") _INCOMPATIBLE = re.compile(r"Disabled incompatible app: (?P[A-Za-z0-9_.-]+)") _STATUS_KEYS = ("installed", "versionstring", "maintenance", "needsDbUpgrade") +_LEADING_DIGITS = re.compile(r"[0-9]+") +_PHP_SPECIAL_FORMS = ( + ("dev", 0), + ("alpha", 1), + ("a", 1), + ("beta", 2), + ("b", 2), + ("RC", 3), + ("rc", 3), + ("#", 4), + ("pl", 5), + ("p", 5), +) def nextcloud_version_php(content): @@ -130,6 +143,149 @@ def nextcloud_status(output): return _json_object(output, _STATUS_KEYS, "nextcloud_status") +def nextcloud_needs_post_upgrade(status_output, installed_version, repaired_output): + """Return whether the running Nextcloud still owes its post-upgrade pass. + + Args: + status_output: stdout of ``occ status --output=json`` after ``occ upgrade``. + installed_version: version ``version.php`` declared before this deploy + started the pinned image, ``""`` when no data was found. + repaired_output: stdout of ``occ config:app:get files mimetype_version + --default-value=``, the server version the expensive repair last + ran for, empty when it never ran. + + Returns: + True when this deploy changed the running version or the expensive + repair has not run for it yet. + """ + running = _json_object( + status_output, (*_STATUS_KEYS, "version"), "nextcloud_needs_post_upgrade" + )["version"] + installed = str(installed_version).strip() + lines = [line.strip() for line in str(repaired_output).splitlines() if line.strip()] + repaired = lines[-1] if lines else "" + return installed not in ("", running) or repaired != running + + +def _is_digit(char): + return char != "" and char in "0123456789" + + +def _is_non_digit(char): + return not _is_digit(char) and char != "." + + +def _php_canonical_version(version): + out = [version[0]] + last = version[0] + for char in version[1:]: + if char in "-_+": + if out[-1] != ".": + out.append(".") + elif (_is_non_digit(last) and _is_digit(char)) or ( + _is_digit(last) and _is_non_digit(char) + ): + if out[-1] != ".": + out.append(".") + out.append(char) + elif not (char.isascii() and char.isalnum()): + if out[-1] != ".": + out.append(".") + else: + out.append(char) + last = char + if out[-1] == ".": + out.pop() + return "".join(out) + + +def _php_special_form(form): + for name, order in _PHP_SPECIAL_FORMS: + if form.startswith(name): + return order + return -1 + + +def _sign(value): + return (value > 0) - (value < 0) + + +def _php_compare_parts(left, right): + if _is_digit(left[:1]) and _is_digit(right[:1]): + return _sign( + int(_LEADING_DIGITS.match(left).group()) + - int(_LEADING_DIGITS.match(right).group()) + ) + if not _is_digit(left[:1]) and not _is_digit(right[:1]): + return _sign(_php_special_form(left) - _php_special_form(right)) + if _is_digit(left[:1]): + return _sign(_php_special_form("#N#") - _php_special_form(right)) + return _sign(_php_special_form(left) - _php_special_form("#N#")) + + +def _php_version_compare(left, right): + if not left or not right: + if not left and not right: + return 0 + return 1 if left else -1 + rest1 = left if left[0] == "#" else _php_canonical_version(left) + rest2 = right if right[0] == "#" else _php_canonical_version(right) + more1 = more2 = True + compare = 0 + while rest1 and rest2 and more1 and more2: + part1, dot1, tail1 = rest1.partition(".") + part2, dot2, tail2 = rest2.partition(".") + more1, more2 = bool(dot1), bool(dot2) + compare = _php_compare_parts(part1, part2) + if compare != 0: + break + if more1: + rest1 = tail1 + if more2: + rest2 = tail2 + if compare == 0: + if more1: + compare = 1 if _is_digit(rest1[:1]) else _php_version_compare(rest1, "#N#") + elif more2: + compare = -1 if _is_digit(rest2[:1]) else _php_version_compare("#N#", rest2) + return compare + + +def nextcloud_cached_app_updates(update_check_output, app_list_output, pinned): + """Return the unpinned enabled apps the daily update check found a newer release for. + + Args: + update_check_output: stdout of ``occ config:list updatenotification + --output=json``; the update check stores the newest release it saw + under the app id. + app_list_output: stdout of ``occ app:list --enabled --output=json``. + pinned: app ids whose release the role pins and never updates. + + Returns: + Sorted app ids; empty when the update check stored nothing. + """ + name = "nextcloud_cached_app_updates" + enabled = _json_object(app_list_output, ("enabled",), name)["enabled"] + stored = _json_object(update_check_output, ("apps",), name)["apps"] + if not isinstance(enabled, dict) or not isinstance(stored, dict): + return [] + if "updatenotification" not in stored: + return [] + found = stored["updatenotification"] + if not isinstance(found, dict): + return [] + updates = [] + for app, installed in enabled.items(): + if app in pinned or app not in found: + continue + available = found[app] + if not isinstance(installed, str) or not isinstance(available, str): + continue + if _php_version_compare(available.strip(), installed.strip()) > 0: + updates.append(app) + return sorted(updates) + + class FilterModule: def filters(self): return { @@ -138,4 +294,6 @@ class FilterModule: "nextcloud_major_steps": nextcloud_major_steps, "nextcloud_disabled_apps": nextcloud_disabled_apps, "nextcloud_status": nextcloud_status, + "nextcloud_needs_post_upgrade": nextcloud_needs_post_upgrade, + "nextcloud_cached_app_updates": nextcloud_cached_app_updates, } diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml index 7b06db60e5..e9dbb3fd51 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml @@ -7,45 +7,29 @@ delay: 5 until: occ_upgrade.rc == 0 -- name: ๐Ÿ”ง Nextcloud | Maintenance repair - shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} maintenance:repair --include-expensive" - register: occ_repair - changed_when: "'No repairs needed' not in occ_repair.stdout" - retries: 3 - delay: 10 - until: occ_repair.rc == 0 - -- name: ๐Ÿ”„ Nextcloud | App update (retry once) - shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update --all" - register: occ_app_update - changed_when: "'No apps found for update' not in occ_app_update.stdout" - retries: 2 - delay: 3 - until: occ_app_update.rc == 0 - when: NEXTCLOUD_PINNED_PLUGINS | length == 0 - -- name: ๐Ÿ” Nextcloud | List the apps the appstore offers an update for - shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update --all --showonly" - register: occ_app_updatable +- name: ๐Ÿ” Nextcloud | Read the version the upgrade left running + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} status --output=json" + register: occ_upgraded_status changed_when: false - retries: 2 - delay: 3 - until: occ_app_updatable.rc == 0 - when: NEXTCLOUD_PINNED_PLUGINS | length > 0 -- name: ๐Ÿ”„ Nextcloud | App update, leaving the pinned releases alone - shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update {{ item | quote }}" - register: occ_app_update_unpinned - changed_when: "'No apps found for update' not in occ_app_update_unpinned.stdout" - retries: 2 - delay: 3 - until: occ_app_update_unpinned.rc == 0 - when: NEXTCLOUD_PINNED_PLUGINS | length > 0 - loop: >- - {{ occ_app_updatable.stdout_lines | default([]) - | select('search', ' new version available') - | map('regex_replace', ' new version available.*$', '') - | difference(NEXTCLOUD_PINNED_PLUGINS) }} +- name: ๐Ÿ” Nextcloud | Read the version the expensive repair last ran for + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} config:app:get files mimetype_version --default-value=" + register: occ_repaired_version + changed_when: false + +- name: ๐Ÿงฎ Nextcloud | Decide whether the post-upgrade pass is due + set_fact: + nextcloud_post_upgrade_due: >- + {{ occ_upgraded_status.stdout + | nextcloud_needs_post_upgrade(nextcloud_installed_version, occ_repaired_version.stdout) }} + +- name: ๐Ÿ”„ Nextcloud | Post-upgrade pass + ansible.builtin.include_tasks: 03_upgrade/01_post_upgrade.yml + when: nextcloud_post_upgrade_due | bool + +- name: ๐Ÿ”„ Nextcloud | Update the apps the daily update check found a release for + ansible.builtin.include_tasks: 03_upgrade/02_cached_updates.yml + when: not (nextcloud_post_upgrade_due | bool) - name: โž• Nextcloud | Add missing columns shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} db:add-missing-columns" diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml new file mode 100644 index 0000000000..cdf1321839 --- /dev/null +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml @@ -0,0 +1,41 @@ +--- +- name: ๐Ÿ”„ Nextcloud | App update (retry once) + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update --all" + register: occ_app_update + changed_when: "'No apps found for update' not in occ_app_update.stdout" + retries: 2 + delay: 3 + until: occ_app_update.rc == 0 + when: not (NEXTCLOUD_PLUGINS_PINNED | bool) + +- name: ๐Ÿ”„ Nextcloud | App update, leaving the pinned releases alone + when: NEXTCLOUD_PLUGINS_PINNED | bool + block: + - name: ๐Ÿ” Nextcloud | List the apps the appstore offers an update for + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update --all --showonly" + register: occ_app_updatable + changed_when: false + retries: 2 + delay: 3 + until: occ_app_updatable.rc == 0 + + - name: ๐Ÿ”„ Nextcloud | App update for each unpinned app the appstore offers + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update {{ item | quote }}" + register: occ_app_update_unpinned + changed_when: "'No apps found for update' not in occ_app_update_unpinned.stdout" + retries: 2 + delay: 3 + until: occ_app_update_unpinned.rc == 0 + loop: >- + {{ occ_app_updatable.stdout_lines + | select('search', ' new version available') + | map('regex_replace', ' new version available.*$', '') + | difference(NEXTCLOUD_PINNED_PLUGINS) }} + +- name: ๐Ÿ”ง Nextcloud | Maintenance repair + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} maintenance:repair --include-expensive" + register: occ_repair + changed_when: "'No repairs needed' not in occ_repair.stdout" + retries: 3 + delay: 10 + until: occ_repair.rc == 0 diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml new file mode 100644 index 0000000000..e894504413 --- /dev/null +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml @@ -0,0 +1,21 @@ +--- +- name: ๐Ÿ“‹ Nextcloud | Read the installed app versions + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:list --enabled --output=json" + register: occ_enabled_apps + changed_when: false + +- name: ๐Ÿ“‹ Nextcloud | Read the app releases the daily update check found + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} config:list updatenotification --output=json" + register: occ_update_check + changed_when: false + +- name: ๐Ÿ”„ Nextcloud | App update for the releases the daily update check found + shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update {{ item | quote }}" + register: occ_app_update_cached + changed_when: "'No apps found for update' not in occ_app_update_cached.stdout" + retries: 2 + delay: 3 + until: occ_app_update_cached.rc == 0 + loop: >- + {{ occ_update_check.stdout + | nextcloud_cached_app_updates(occ_enabled_apps.stdout, NEXTCLOUD_PINNED_PLUGINS) }} diff --git a/roles/web-app-nextcloud/vars/main.yml b/roles/web-app-nextcloud/vars/main.yml index 3abcb22786..75cc1812aa 100644 --- a/roles/web-app-nextcloud/vars/main.yml +++ b/roles/web-app-nextcloud/vars/main.yml @@ -23,6 +23,7 @@ NEXTCLOUD_PINNED_PLUGINS: >- | selectattr('value.config.archive', 'defined') | selectattr('value.enabled', 'true') | map(attribute='key') | list }} +NEXTCLOUD_PLUGINS_PINNED: "{{ NEXTCLOUD_PINNED_PLUGINS | length > 0 }}" NEXTCLOUD_PLUGINS_ENABLED: "{{ lookup('config', application_id, 'services.nextcloud.plugins_enabled') }}" NEXTCLOUD_USER_LDAP_ENABLED: "{{ lookup('config', application_id, 'addons.user_ldap.enabled') }}" NEXTCLOUD_LDAP_CONFIG_PREFIX: "s01" diff --git a/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_upgrade.py b/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_upgrade.py index ced0016ffe..90fff94447 100644 --- a/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_upgrade.py +++ b/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_upgrade.py @@ -210,5 +210,198 @@ class TestNextcloudStatus(unittest.TestCase): MODULE.nextcloud_status("Nextcloud is not installed") +class TestNextcloudNeedsPostUpgrade(unittest.TestCase): + def test_unchanged_redeploy_skips_the_pass(self): + self.assertFalse( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.1.2", "32.0.1.2") + ) + + def test_minor_upgrade_in_this_deploy_runs_the_pass(self): + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.0.13", "32.0.0.13") + ) + + def test_major_upgrade_in_this_deploy_runs_the_pass(self): + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "31.0.11.1", "31.0.11.1") + ) + + def test_fresh_install_runs_the_pass(self): + self.assertTrue(MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "", "")) + + def test_deploy_that_stopped_before_the_repair_runs_the_pass_again(self): + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.1.2", "32.0.0.13") + ) + + def test_version_change_runs_the_pass_even_when_the_marker_already_matches(self): + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.0.13", "32.0.1.2") + ) + + def test_missing_local_data_volume_falls_back_to_the_marker(self): + self.assertFalse( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "", "32.0.1.2") + ) + + def test_php_notice_before_the_marker_is_skipped(self): + output = "PHP Deprecated: something in /var/www/html/lib/x.php\n32.0.1.2\n" + self.assertFalse( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.1.2", output) + ) + + def test_php_notice_without_a_marker_runs_the_pass(self): + output = "PHP Deprecated: something in /var/www/html/lib/x.php\n\n" + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.1.2", output) + ) + + def test_null_marker_runs_the_pass(self): + self.assertTrue( + MODULE.nextcloud_needs_post_upgrade(STATUS_JSON, "32.0.1.2", "null") + ) + + def test_status_without_version_is_rejected(self): + status = ( + '{"installed":true,"versionstring":"32.0.1","maintenance":false,' + '"needsDbUpgrade":false}' + ) + with self.assertRaises(AnsibleFilterError): + MODULE.nextcloud_needs_post_upgrade(status, "32.0.1.2", "32.0.1.2") + + +class TestPhpVersionCompare(unittest.TestCase): + def test_newer_patch_is_greater(self): + self.assertEqual(MODULE._php_version_compare("1.17.0", "1.16.2"), 1) + + def test_identical_versions_are_equal(self): + self.assertEqual(MODULE._php_version_compare("10.2.1", "10.2.1"), 0) + + def test_numeric_parts_compare_as_numbers(self): + self.assertEqual(MODULE._php_version_compare("1.10.0", "1.9.9"), 1) + + def test_shorter_version_is_older(self): + self.assertEqual(MODULE._php_version_compare("5.2", "5.2.0"), -1) + + def test_release_is_newer_than_its_beta(self): + self.assertEqual(MODULE._php_version_compare("2.0.0", "2.0.0-beta.1"), 1) + + def test_release_candidate_is_older_than_the_release(self): + self.assertEqual(MODULE._php_version_compare("1.0rc1", "1.0"), -1) + + def test_alpha_is_older_than_beta(self): + self.assertEqual(MODULE._php_version_compare("1.0.0-alpha", "1.0.0-beta"), -1) + + def test_patch_level_is_newer_than_the_release(self): + self.assertEqual(MODULE._php_version_compare("1.0pl1", "1.0"), 1) + + def test_empty_version_is_older(self): + self.assertEqual(MODULE._php_version_compare("", "1.0"), -1) + + +def _update_check(found): + return '{"apps":{"updatenotification":' + found + "}}" + + +INSTALLED_APPS_JSON = ( + '{"enabled":{"deck":"1.16.2","onlyoffice":"10.2.1","files":"2.4.0",' + '"calendar":"6.0.1"}}' +) + + +class TestNextcloudCachedAppUpdates(unittest.TestCase): + def test_newer_release_of_an_unpinned_app_is_updated(self): + output = _update_check('{"deck":"1.17.0","enabled":"yes"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates( + output, INSTALLED_APPS_JSON, ["onlyoffice"] + ), + ["deck"], + ) + + def test_newer_release_of_a_pinned_app_is_left_alone(self): + output = _update_check('{"onlyoffice":"10.3.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates( + output, INSTALLED_APPS_JSON, ["onlyoffice"] + ), + [], + ) + + def test_release_already_installed_is_not_updated(self): + output = _update_check('{"deck":"1.16.2"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), [] + ) + + def test_release_older_than_the_installed_one_is_not_updated(self): + output = _update_check('{"calendar":"5.5.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), [] + ) + + def test_empty_update_check_means_no_update(self): + self.assertEqual( + MODULE.nextcloud_cached_app_updates( + _update_check("[]"), INSTALLED_APPS_JSON, [] + ), + [], + ) + + def test_update_check_without_its_app_entry_means_no_update(self): + self.assertEqual( + MODULE.nextcloud_cached_app_updates('{"apps":[]}', INSTALLED_APPS_JSON, []), + [], + ) + + def test_own_settings_of_the_update_check_are_not_apps(self): + output = _update_check( + '{"core":"34.0.6.1","enabled":"yes","installed_version":"1.22.0",' + '"update_check_errors":0}' + ) + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), [] + ) + + def test_release_of_an_app_that_is_not_enabled_is_ignored(self): + output = _update_check('{"news":"27.0.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), [] + ) + + def test_app_without_an_installed_version_is_skipped(self): + output = _update_check('{"deck":"1.17.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates( + output, '{"enabled":{"deck":true}}', [] + ), + [], + ) + + def test_empty_cached_release_means_no_update(self): + output = _update_check('{"deck":""}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), [] + ) + + def test_several_updates_are_sorted(self): + output = _update_check('{"deck":"1.17.0","calendar":"6.1.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), + ["calendar", "deck"], + ) + + def test_php_notice_before_the_json_is_skipped(self): + output = "PHP Deprecated: x in y.php\n" + _update_check('{"deck":"1.17.0"}') + self.assertEqual( + MODULE.nextcloud_cached_app_updates(output, INSTALLED_APPS_JSON, []), + ["deck"], + ) + + def test_update_check_output_without_json_is_rejected(self): + with self.assertRaises(AnsibleFilterError): + MODULE.nextcloud_cached_app_updates("broken", INSTALLED_APPS_JSON, []) + + if __name__ == "__main__": unittest.main() From e5e3845f5bc94012f5853ec1e6da0b2834b3ef44 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 20:53:52 +0100 Subject: [PATCH 03/17] fix(web-app-nextcloud): report changed from the output occ actually prints Eight changed_when checks matched strings Nextcloud v34 never prints, so the tasks reported changed on every redeploy: occ upgrade prints 'No upgrade required.' (Upgrade.php:214), app:update prints '... up-to-date or no updates could be found' (AppUpdate.php:124,126), and db:add-missing-columns/-indices/-primary-keys print ' table updated successfully.' only on real work (AddMissingColumns.php:68, AddMissingIndices.php:82,117, AddMissingPrimaryKeys.php:74). maintenance:repair has no no-op output and always clears the frontend caches (ClearFrontendCaches.php:31-36), so it now reports changed; it only runs when the post-upgrade pass is due. Scenario harness 24/24; the negative controls with the old strings report changed on no-op output. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml | 8 ++++---- .../tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml | 6 +++--- .../tasks/02_manager_ops/03_upgrade/02_cached_updates.yml | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml index e9dbb3fd51..d7b402c086 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade.yml @@ -2,7 +2,7 @@ - name: โฌ†๏ธ Nextcloud | Upgrade shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} upgrade" register: occ_upgrade - changed_when: "'Already up to date' not in occ_upgrade.stdout" + changed_when: "'No upgrade required' not in occ_upgrade.stdout" retries: 3 delay: 5 until: occ_upgrade.rc == 0 @@ -34,17 +34,17 @@ - name: โž• Nextcloud | Add missing columns shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} db:add-missing-columns" register: occ_columns - changed_when: "'No columns found' not in occ_columns.stdout" + changed_when: "'table updated successfully' in occ_columns.stdout" - name: โž• Nextcloud | Add missing indices shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} db:add-missing-indices" register: occ_indices - changed_when: "'No indices found' not in occ_indices.stdout" + changed_when: "'table updated successfully' in occ_indices.stdout" - name: โž• Nextcloud | Add missing primary keys shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} db:add-missing-primary-keys" register: occ_pks - changed_when: "'No primary keys found' not in occ_pks.stdout" + changed_when: "'table updated successfully' in occ_pks.stdout" - name: ๐Ÿ”ด Nextcloud | Disable maintenance mode shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} maintenance:mode --off" diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml index cdf1321839..e7aea6b01c 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/01_post_upgrade.yml @@ -2,7 +2,7 @@ - name: ๐Ÿ”„ Nextcloud | App update (retry once) shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update --all" register: occ_app_update - changed_when: "'No apps found for update' not in occ_app_update.stdout" + changed_when: "'up-to-date or no updates could be found' not in occ_app_update.stdout" retries: 2 delay: 3 until: occ_app_update.rc == 0 @@ -22,7 +22,7 @@ - name: ๐Ÿ”„ Nextcloud | App update for each unpinned app the appstore offers shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update {{ item | quote }}" register: occ_app_update_unpinned - changed_when: "'No apps found for update' not in occ_app_update_unpinned.stdout" + changed_when: "'up-to-date or no updates could be found' not in occ_app_update_unpinned.stdout" retries: 2 delay: 3 until: occ_app_update_unpinned.rc == 0 @@ -35,7 +35,7 @@ - name: ๐Ÿ”ง Nextcloud | Maintenance repair shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} maintenance:repair --include-expensive" register: occ_repair - changed_when: "'No repairs needed' not in occ_repair.stdout" + changed_when: true retries: 3 delay: 10 until: occ_repair.rc == 0 diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml index e894504413..452e626d0b 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/03_upgrade/02_cached_updates.yml @@ -12,7 +12,7 @@ - name: ๐Ÿ”„ Nextcloud | App update for the releases the daily update check found shell: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} app:update {{ item | quote }}" register: occ_app_update_cached - changed_when: "'No apps found for update' not in occ_app_update_cached.stdout" + changed_when: "'up-to-date or no updates could be found' not in occ_app_update_cached.stdout" retries: 2 delay: 3 until: occ_app_update_cached.rc == 0 From c10510a1d133fce540eeabb2d4b09db96d29fbdd Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 20:56:01 +0100 Subject: [PATCH 04/17] fix(svc-db-postgres): keep the superuser password off the container exec argv The TCP-auth wait passed PGPASSWORD= on the container exec command line, where it shows in the host's process list. It now names the variable only (-e PGPASSWORD) and sets the value through the task environment, the form store-i18n's merge c887b8e04e already uses for this file and its test_exec_env_passes_no_value lint enforces. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/svc-db-postgres/tasks/01_init/ready.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/roles/svc-db-postgres/tasks/01_init/ready.yml b/roles/svc-db-postgres/tasks/01_init/ready.yml index 9226b230e6..f45412a15c 100644 --- a/roles/svc-db-postgres/tasks/01_init/ready.yml +++ b/roles/svc-db-postgres/tasks/01_init/ready.yml @@ -12,9 +12,11 @@ - name: โณ Wait for Postgres TCP auth no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" ansible.builtin.shell: > # nocheck: container-exec-resolver POSTGRES_LOCAL_CID resolved at delegate_to runtime - container exec -e PGPASSWORD={{ POSTGRES_PASSWORD | quote }} {{ POSTGRES_LOCAL_CID | quote }} sh -lc + container exec -e PGPASSWORD {{ POSTGRES_LOCAL_CID | quote }} sh -lc 'psql -h "{{ POSTGRES_LOCAL_HOST }}" -p "{{ POSTGRES_PORT }}" -U postgres -d postgres -c "SELECT 1"' + environment: + PGPASSWORD: "{{ POSTGRES_PASSWORD }}" delegate_to: "{{ POSTGRES_HOST_NODE }}" register: pg_ready until: pg_ready.rc == 0 From acad754cdad17b5c443efc9ea0699a2cc8f81b3f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 20:58:06 +0100 Subject: [PATCH 05/17] fix(group_vars): evaluate the CAPTCHA flags only for a configured provider A host that defines CAPTCHA with one provider replaces the group_vars mapping (no hash_behaviour merge), so galaxyserver, which configures only hCaptcha, left CAPTCHA.RECAPTCHA undefined. RECAPTCHA_ENABLED then failed to render, the applications render fallback returned the expression as text, and Keycloak's '| bool' coerced it to False with a deprecation warning that ansible-core 2.23 turns into an error. Both flags now check that their provider mapping is defined first; the effective values stay the same for every existing inventory. test_captcha_enabled renders every role's services.recaptcha/hcaptcha.enabled through the real templar for defaults, both providers, only the other provider and only this provider, and fails on the raw text (negative control with the old expression: 4 subtests). Co-Authored-By: Claude Opus 5.5 (1M context) --- group_vars/all/00_general.yml | 4 +- .../python/group_vars/test_captcha_enabled.py | 105 ++++++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 tests/unit/python/group_vars/test_captcha_enabled.py diff --git a/group_vars/all/00_general.yml b/group_vars/all/00_general.yml index 782bee85c7..c4ad5d9d2c 100644 --- a/group_vars/all/00_general.yml +++ b/group_vars/all/00_general.yml @@ -73,8 +73,8 @@ CAPTCHA: KEY: "" SECRET: "" -RECAPTCHA_ENABLED: "{{ (CAPTCHA.RECAPTCHA.KEY | length > 0) and (CAPTCHA.RECAPTCHA.SECRET | length > 0) }}" +RECAPTCHA_ENABLED: "{{ (CAPTCHA.RECAPTCHA is defined) and (CAPTCHA.RECAPTCHA.KEY | length > 0) and (CAPTCHA.RECAPTCHA.SECRET | length > 0) }}" -HCAPTCHA_ENABLED: "{{ (CAPTCHA.HCAPTCHA.KEY | length > 0) and (CAPTCHA.HCAPTCHA.SECRET | length > 0) }}" +HCAPTCHA_ENABLED: "{{ (CAPTCHA.HCAPTCHA is defined) and (CAPTCHA.HCAPTCHA.KEY | length > 0) and (CAPTCHA.HCAPTCHA.SECRET | length > 0) }}" WORKSTATION_USER: "administrator" diff --git a/tests/unit/python/group_vars/test_captcha_enabled.py b/tests/unit/python/group_vars/test_captcha_enabled.py new file mode 100644 index 0000000000..1a1d8e5cc7 --- /dev/null +++ b/tests/unit/python/group_vars/test_captcha_enabled.py @@ -0,0 +1,105 @@ +import unittest +import unittest.mock as mock +from pathlib import Path +from typing import Any, ClassVar + +from ansible.parsing.dataloader import DataLoader +from ansible.template import Templar +from ansible.utils.display import Display + +from utils.cache.base import _render_with_templar +from utils.cache.files import PROJECT_ROOT +from utils.cache.yaml import load_yaml_any +from utils.roles.mapping import ROLE_FILE_META_SERVICES + +_GENERAL_VARS = Path(PROJECT_ROOT) / "group_vars" / "all" / "00_general.yml" +_ROLES_DIR = Path(PROJECT_ROOT) / "roles" +_PROVIDERS = {"recaptcha": "RECAPTCHA", "hcaptcha": "HCAPTCHA"} +_CREDENTIALS = {"KEY": "site-key", "SECRET": "site-secret"} + + +def _consumers() -> list[tuple[str, str, str]]: + found = [] + for role_dir in sorted(_ROLES_DIR.iterdir()): + services_file = role_dir / ROLE_FILE_META_SERVICES + if not services_file.is_file(): + continue + services = load_yaml_any(services_file) or {} + for service_key, provider in _PROVIDERS.items(): + entry = services.get(service_key) + enabled = entry.get("enabled") if isinstance(entry, dict) else None + if isinstance(enabled, str) and f"{provider}_ENABLED" in enabled: + found.append((role_dir.name, service_key, enabled)) + return found + + +class TestCaptchaEnabled(unittest.TestCase): + """``services..enabled`` reaches its consumers through the + applications render, whose failure path hands back template text instead + of raising. Inventory ``CAPTCHA`` replaces the group_vars mapping whole, + so a host that configures one provider drops the other's sub-mapping. + """ + + group_vars: ClassVar[dict[str, Any]] + consumers: ClassVar[list[tuple[str, str, str]]] + + @classmethod + def setUpClass(cls): + cls.group_vars = DataLoader().load_from_file( + str(_GENERAL_VARS), trusted_as_template=True + ) + cls.consumers = _consumers() + + def _render(self, value: str, captcha: dict[str, Any]) -> tuple[Any, list[str]]: + variables = {**self.group_vars, "CAPTCHA": captcha} + templar = Templar(loader=DataLoader(), variables=variables) + with mock.patch.object(Display, "deprecated") as deprecated: + rendered = _render_with_templar(value, templar=templar, variables=variables) + coercions = [ + str(call) for call in deprecated.call_args_list if "coerced" in str(call) + ] + return rendered, coercions + + def _assert_every_consumer(self, captcha_for, expected_for): + for role, service_key, value in self.consumers: + provider = _PROVIDERS[service_key] + with self.subTest(role=role, service=service_key): + rendered, coercions = self._render(value, captcha_for(provider)) + self.assertIsInstance(rendered, bool, f"rendered to {rendered!r}") + self.assertEqual(rendered, expected_for(provider)) + self.assertEqual(coercions, []) + + def test_every_provider_has_a_consumer(self): + self.assertEqual( + {service_key for _, service_key, _ in self.consumers}, set(_PROVIDERS) + ) + + def test_the_group_vars_defaults_disable_every_provider(self): + self._assert_every_consumer( + lambda provider: self.group_vars["CAPTCHA"], lambda provider: False + ) + + def test_both_credentials_enable_the_provider(self): + self._assert_every_consumer( + lambda provider: {name: dict(_CREDENTIALS) for name in _PROVIDERS.values()}, + lambda provider: True, + ) + + def test_a_host_that_configures_only_the_other_provider_disables_this_one(self): + def only_the_other(provider): + return { + name: dict(_CREDENTIALS) + for name in _PROVIDERS.values() + if name != provider + } + + self._assert_every_consumer(only_the_other, lambda provider: False) + + def test_a_host_that_configures_only_this_provider_enables_it(self): + self._assert_every_consumer( + lambda provider: {provider: dict(_CREDENTIALS)}, lambda provider: True + ) + + +if __name__ == "__main__": + unittest.main() From afb82f57a19b6ca51addb19731d3415493f72574 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:01:06 +0100 Subject: [PATCH 06/17] feat(web-app-dashboard): let a role name the canonical domain its card opens The dashboard card of every app opened its primary domain, so Matrix led to the Synapse status page and Stalwart to its admin UI. A role can now declare services.dashboard.card: ; docker_cards builds the card URL with lookup('canonical_url', app, key), the Apps menu reuses it, and roles_with_service('dashboard') hands the same domain to the tile e2e spec. Matrix sets card: element, Stalwart card: webmail. Without card nothing changes, and the prometheus/matomo target lists are untouched. A lint requires the key to resolve in the role's own domains.canonical. The Matrix ansible flavor also sets Synapse's web_client_location to Element through matrix_synapse_configuration_extension_yaml, as the compose flavor already does, so matrix./ redirects there. Merge note: feature/iam-profile-rbac defines the same extension variable inside the OIDC block; the next merge must combine both into one block (agreed with that session). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../contributing/design/role/services/base.md | 17 ++++ plugins/lookup/roles_with_service.py | 44 +++++++++- .../lookup_plugins/docker_cards.py | 40 +++++---- roles/web-app-matrix/meta/services.yml | 1 + .../templates/flavor/ansible/vars.yml.j2 | 2 + roles/web-app-stalwart/meta/services.yml | 1 + .../roles/meta/test_dashboard_card_key.py | 83 +++++++++++++++++++ .../plugins/lookup/test_docker_cards.py | 55 ++++++++++++ .../plugins/lookup/test_roles_with_service.py | 67 +++++++++++++-- .../roles/applications/test_dashboard.py | 46 ++++++++++ utils/roles/applications/dashboard.py | 42 ++++++++++ 11 files changed, 374 insertions(+), 24 deletions(-) create mode 100644 tests/lint/ansible/roles/meta/test_dashboard_card_key.py create mode 100644 tests/unit/python/utils/roles/applications/test_dashboard.py create mode 100644 utils/roles/applications/dashboard.py diff --git a/docs/contributing/design/role/services/base.md b/docs/contributing/design/role/services/base.md index c089f599da..1a0acaf229 100644 --- a/docs/contributing/design/role/services/base.md +++ b/docs/contributing/design/role/services/base.md @@ -57,6 +57,23 @@ Rules: - `canonical:` is only used on alias entries that resolve back to the primary service key. - `frontend` vs. `backend` is derived from the role name prefix, not stored in config. +## Dashboard Card Target ๐Ÿƒ + +A consumer's `dashboard` binding MAY name `card: ` to point its dashboard card, its Apps menu entry and its tile-reachability target at a domain other than the primary one: + +```yaml +dashboard: + bond: 1 + enabled: "{{ 'web-app-dashboard' in group_names }}" + shared: "{{ 'web-app-dashboard' in group_names }}" + card: element +``` + +Rules: + +- `card:` MUST be a key of the role's `canonical` mapping in `meta/domains.yml`; the card URL is `lookup('canonical_url', , )`. +- Without `card:` the card targets the primary domain via `lookup('tls', , 'url.base')`. + ## Service Discovery ๐Ÿ” Service discovery is built from role configs, not from a central registry file. diff --git a/plugins/lookup/roles_with_service.py b/plugins/lookup/roles_with_service.py index 7c1001640e..979c2877f0 100644 --- a/plugins/lookup/roles_with_service.py +++ b/plugins/lookup/roles_with_service.py @@ -6,6 +6,11 @@ role whose merged applications config declares ``services..iframe`` (defaulting to ``enabled``) so consumers can tell embeddable cards from those that must open in a new tab. +For the ``dashboard`` service a role MAY name ``services.dashboard.card: +``; its entry then carries ``domains.canonical.`` and +``lookup('canonical_url', , )`` instead of the primary domain, so +the tile target matches the card ``docker_cards`` renders. + A role keeps declaring the service for inventory completeness but can opt out of this consumer-target list by setting ``services..scrape: false`` or ``services..track: false``. @@ -45,6 +50,7 @@ from ansible.plugins.loader import lookup_loader from ansible.plugins.lookup import LookupBase from utils.domains.primary_domain import get_primary_domain +from utils.roles.applications.dashboard import DASHBOARD_SERVICE, card_target_key from utils.roles.applications.mcp import ( DEFAULT_MCP_TRANSPORT, derive_allowed_consumers, @@ -85,6 +91,30 @@ def _resolve_canonical_domain(role_id: str, app_config: dict[str, Any]) -> str: return get_primary_domain({role_id: domains["canonical"]}, role_id) +def _resolve_named_canonical_domain( + role_id: str, app_config: dict[str, Any], key: str +) -> str: + """Return the domain ``domains.canonical.`` of one role. + + Args: + role_id: the role whose domains are read. + app_config: the role's merged applications entry. + key: the named canonical, e.g. ``element``. + + Raises: + AnsibleError: the role declares no such named canonical. + """ + domains = app_config.get("domains") + canonical = domains.get("canonical") if isinstance(domains, dict) else None + domain = canonical.get(key) if isinstance(canonical, dict) else None + if not isinstance(domain, str) or not domain: + raise AnsibleError( + f"roles_with_service: '{role_id}' has no canonical '{key}' " + f"(available: {sorted(canonical) if isinstance(canonical, dict) else []})" + ) + return domain + + def _deployed_roles(scope: str, vars_: dict[str, Any]) -> set[str] | None: """Return the role ids the scope admits, or None for no restriction. @@ -144,6 +174,7 @@ class LookupModule(LookupBase): tls_lookup = lookup_loader.get( "tls", loader=self._loader, templar=self._templar ) + lists_dashboard_cards = not topic and service_name == DASHBOARD_SERVICE results: list[dict[str, Any]] = [] for role_id, app_config in applications.items(): @@ -174,8 +205,19 @@ class LookupModule(LookupBase): canonical = _resolve_canonical_domain(str(role_id), app_config) if not canonical and direction is None: continue + card_key = ( + card_target_key(str(role_id), block) if lists_dashboard_cards else "" + ) canonical_url = "" - if canonical: + if card_key: + canonical = _resolve_named_canonical_domain( + str(role_id), app_config, card_key + ) + resolved = lookup_loader.get( + "canonical_url", loader=self._loader, templar=self._templar + ).run([str(role_id), card_key], variables=variables) + canonical_url = str(resolved[0]).rstrip("/") + elif canonical: resolved = tls_lookup.run( [str(role_id), "url.base"], variables=variables ) diff --git a/roles/web-app-dashboard/lookup_plugins/docker_cards.py b/roles/web-app-dashboard/lookup_plugins/docker_cards.py index ff6a52294d..5d08d82b0c 100644 --- a/roles/web-app-dashboard/lookup_plugins/docker_cards.py +++ b/roles/web-app-dashboard/lookup_plugins/docker_cards.py @@ -9,6 +9,7 @@ from ansible.plugins.lookup import LookupBase from utils.cache.files import read_text from utils.cache.yaml import load_yaml_any from utils.roles.applications.config import get +from utils.roles.applications.dashboard import DASHBOARD_SERVICE, card_target_key from utils.roles.mapping import ( ROLE_FILE_META_INFO, ROLE_FILE_META_MAIN, @@ -28,7 +29,8 @@ class LookupModule(LookupBase): - Retrieves the icon class from meta/info.yml.logo.class - Retrieves the display flag from meta/info.yml.display - Retrieves the tags from galaxy_info.galaxy_tags - - Builds the URL using the 'domains' variable + - Builds the URL from the primary domain, or from the domains.canonical + key named by services.dashboard.card - Sets the iframe flag from applications Only cards whose application_id is included in the variable group_names are returned. @@ -132,28 +134,32 @@ class LookupModule(LookupBase): self._templar.template(domain_url).strip() if domain_url else "" ) - url = "" - if domain_url: - try: - tls_lookup = lookup_loader.get( - "tls", loader=self._loader, templar=self._templar - ) - base_url = tls_lookup.run( - [application_id, "url.base"], variables=variables - )[0] - url = str(base_url).strip().rstrip("/") - except Exception as e: - raise AnsibleError( - f"Error building URL via tls for '{application_id}': {e}" - ) from e - dashboard_cfg = get( applications, application_id, - "services.dashboard", + f"services.{DASHBOARD_SERVICE}", strict=False, default={}, ) + + url = "" + if domain_url: + card_key = card_target_key(application_id, dashboard_cfg) + try: + if card_key: + target_url = lookup_loader.get( + "canonical_url", loader=self._loader, templar=self._templar + ).run([application_id, card_key], variables=variables)[0] + else: + target_url = lookup_loader.get( + "tls", loader=self._loader, templar=self._templar + ).run([application_id, "url.base"], variables=variables)[0] + url = str(target_url).strip().rstrip("/") + except Exception as e: + raise AnsibleError( + f"Error building the card URL for '{application_id}': {e}" + ) from e + if isinstance(dashboard_cfg, dict) and "iframe" in dashboard_cfg: iframe = dashboard_cfg["iframe"] else: diff --git a/roles/web-app-matrix/meta/services.yml b/roles/web-app-matrix/meta/services.yml index 841de75aec..2749a6d025 100644 --- a/roles/web-app-matrix/meta/services.yml +++ b/roles/web-app-matrix/meta/services.yml @@ -22,6 +22,7 @@ dashboard: bond: 1 enabled: "{{ 'web-app-dashboard' in group_names }}" shared: "{{ 'web-app-dashboard' in group_names }}" + card: element matomo: bond: 1 enabled: "{{ 'web-app-matomo' in group_names }}" diff --git a/roles/web-app-matrix/templates/flavor/ansible/vars.yml.j2 b/roles/web-app-matrix/templates/flavor/ansible/vars.yml.j2 index 7efd5913a6..748578d94e 100644 --- a/roles/web-app-matrix/templates/flavor/ansible/vars.yml.j2 +++ b/roles/web-app-matrix/templates/flavor/ansible/vars.yml.j2 @@ -141,6 +141,8 @@ matrix_synapse_ext_password_provider_ldap_attributes_name: givenName matrix_client_element_enabled: true matrix_client_element_default_hs_url: "{{ MATRIX_SYNAPSE_URL }}" +matrix_synapse_configuration_extension_yaml: | + web_client_location: "{{ lookup('canonical_url', application_id, 'element') }}" matrix_bridge_appservice_irc_enabled: {{ MATRIX_BRIDGE.appservice_irc | string | lower }} matrix_bridge_heisenbridge_enabled: {{ MATRIX_BRIDGE.heisenbridge | string | lower }} diff --git a/roles/web-app-stalwart/meta/services.yml b/roles/web-app-stalwart/meta/services.yml index 86795ed9e5..29ecff9f22 100644 --- a/roles/web-app-stalwart/meta/services.yml +++ b/roles/web-app-stalwart/meta/services.yml @@ -19,6 +19,7 @@ dashboard: bond: 1 enabled: "{{ 'web-app-dashboard' in group_names }}" shared: "{{ 'web-app-dashboard' in group_names }}" + card: webmail matomo: bond: 1 enabled: "{{ 'web-app-matomo' in group_names }}" diff --git a/tests/lint/ansible/roles/meta/test_dashboard_card_key.py b/tests/lint/ansible/roles/meta/test_dashboard_card_key.py new file mode 100644 index 0000000000..6e393166b6 --- /dev/null +++ b/tests/lint/ansible/roles/meta/test_dashboard_card_key.py @@ -0,0 +1,83 @@ +"""Lint guard for the dashboard card target a role names in ``meta/services.yml``. + +A role MAY point its dashboard card at a non-primary domain by naming +``card: `` on its ``dashboard`` binding. The key MUST be a key of the +role's ``meta/domains.yml`` ``canonical`` mapping, the only keys +``lookup('canonical_url', , )`` resolves. +""" + +from __future__ import annotations + +import unittest +from typing import TYPE_CHECKING, Any + +import yaml +from ansible.errors import AnsibleError + +from utils.cache.files import read_text +from utils.cache.yaml import load_yaml_str +from utils.roles.applications.dashboard import ( + CARD_TARGET_KEY, + DASHBOARD_SERVICE, + card_target_key, +) +from utils.roles.mapping import ROLE_FILE_META_DOMAINS, ROLE_FILE_META_SERVICES + +from . import PROJECT_ROOT + +if TYPE_CHECKING: + from pathlib import Path + +ROLES_DIR = PROJECT_ROOT / "roles" + + +def _load_yaml(path: Path) -> Any: + if not path.is_file(): + return None + try: + return load_yaml_str(read_text(str(path))) + except (UnicodeDecodeError, yaml.YAMLError): + return None + + +def _card_key_violation(role_dir: Path) -> str | None: + role_name = role_dir.name + services = _load_yaml(role_dir / ROLE_FILE_META_SERVICES) + if not isinstance(services, dict): + return None + try: + key = card_target_key(role_name, services.get(DASHBOARD_SERVICE)) + except AnsibleError as exc: + return str(exc) + if not key: + return None + domains = _load_yaml(role_dir / ROLE_FILE_META_DOMAINS) + canonical = domains.get("canonical") if isinstance(domains, dict) else None + if not isinstance(canonical, dict) or key not in canonical: + available = sorted(canonical) if isinstance(canonical, dict) else [] + return ( + f"{role_name}: services.{DASHBOARD_SERVICE}.{CARD_TARGET_KEY} '{key}' " + f"is not a key of {ROLE_FILE_META_DOMAINS} canonical " + f"(available: {available})." + ) + return None + + +class TestDashboardCardKey(unittest.TestCase): + def test_card_key_is_a_domain_key_of_its_role(self) -> None: + violations = [ + violation + for role_dir in sorted(ROLES_DIR.iterdir()) + if role_dir.is_dir() + and (violation := _card_key_violation(role_dir)) is not None + ] + if violations: + self.fail( + f"A dashboard binding's `{CARD_TARGET_KEY}` MUST name a " + "domains.canonical key of its own role:\n" + + "\n".join(f" - {v}" for v in violations) + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/python/plugins/lookup/test_docker_cards.py b/tests/unit/python/plugins/lookup/test_docker_cards.py index a450892007..5a981df409 100644 --- a/tests/unit/python/plugins/lookup/test_docker_cards.py +++ b/tests/unit/python/plugins/lookup/test_docker_cards.py @@ -125,6 +125,26 @@ class DummyTlsResolveLookup: return [base_url] +class DummyCanonicalUrlLookup: + """ + Deterministic canonical_url stub. + Emulates: lookup('canonical_url', app_id, key) -> 'http(s)://' + """ + + def run(self, terms, variables=None, **kwargs): + variables = variables or {} + app_id, key = terms + app_domains = variables.get("domains", {}).get(app_id) + if not isinstance(app_domains, dict) or key not in app_domains: + raise AnsibleError( + f"dummy canonical_url: '{app_id}' has no canonical '{key}'" + ) + scheme = ( + "https" if _ansible_bool(variables.get("TLS_ENABLED", True)) else "http" + ) + return [f"{scheme}://{app_domains[key]}"] + + class TestDockerCardsLookup(unittest.TestCase): def setUp(self): self.test_roles_dir = tempfile.mkdtemp(prefix="test_roles_") @@ -174,6 +194,8 @@ logo: return _VarsLookup("domains") if name == "tls": return DummyTlsResolveLookup(templar) + if name == "canonical_url": + return DummyCanonicalUrlLookup() raise AnsibleError(f"Unexpected lookup requested: {name}") docker_cards_module.lookup_loader.get = _patched_get @@ -304,6 +326,39 @@ logo: self.assertEqual(result[0][0]["url"], "http://myportfolio.com") + def _card_target_variables(self, dashboard): + fake_variables = self._base_fake_variables() + fake_variables["domains"] = { + "portfolio": { + "www": "myportfolio.com", + "client": "client.myportfolio.com", + } + } + fake_variables["applications"]["portfolio"]["services"]["dashboard"] = dashboard + return fake_variables + + def test_card_url_targets_the_card_key(self): + fake_variables = self._card_target_variables( + {"enabled": True, "card": "client"} + ) + + result = self._run_lookup(LookupModule(), fake_variables) + + self.assertEqual(result[0][0]["url"], "https://client.myportfolio.com") + + def test_card_url_without_card_key_targets_the_primary_domain(self): + fake_variables = self._card_target_variables({"enabled": True}) + + result = self._run_lookup(LookupModule(), fake_variables) + + self.assertEqual(result[0][0]["url"], "https://myportfolio.com") + + def test_unknown_card_key_fails_loudly(self): + fake_variables = self._card_target_variables({"enabled": True, "card": "nope"}) + + with self.assertRaisesRegex(AnsibleError, "nope"): + self._run_lookup(LookupModule(), fake_variables) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/python/plugins/lookup/test_roles_with_service.py b/tests/unit/python/plugins/lookup/test_roles_with_service.py index 6fa2d754e4..d75c1fc3c0 100644 --- a/tests/unit/python/plugins/lookup/test_roles_with_service.py +++ b/tests/unit/python/plugins/lookup/test_roles_with_service.py @@ -44,23 +44,34 @@ class RolesWithServiceLookupTests(unittest.TestCase): return lm def _run(self, terms, applications: dict, vars_: dict | None = None, **kwargs): - """Run the lookup with the ``applications`` and ``tls`` sub-lookups - patched to stay hermetic. The stubbed tls resolves - ``[role_id, "url.base"]`` to a deterministic URL so the lookup - can finish building each result entry without the real domain - machinery.""" + """Run the lookup with the ``applications``, ``tls`` and + ``canonical_url`` sub-lookups patched to stay hermetic. The stubbed + tls resolves ``[role_id, "url.base"]`` and the stubbed canonical_url + ``[role_id, key]`` to deterministic URLs so the lookup can finish + building each result entry without the real domain machinery; any + other sub-lookup raises ``KeyError``.""" lookup = self._make_lookup(vars_ or {}) class _StubTls: def run(self, terms_, variables=None, **kwargs): return [f"https://{terms_[0]}.example.com"] + class _StubCanonicalUrl: + def run(self, terms_, variables=None, **kwargs): + return [f"https://{terms_[1]}.{terms_[0]}.example.com"] + class _StubApplications: def run(self, terms_, variables=None, **kwargs): return [applications] + stubs = { + "applications": _StubApplications(), + "tls": _StubTls(), + "canonical_url": _StubCanonicalUrl(), + } + def _dispatch(name, **_kwargs): - return _StubApplications() if name == "applications" else _StubTls() + return stubs[name] with patch.object(self.mod.lookup_loader, "get", side_effect=_dispatch): return lookup.run(terms, variables=vars_ or {}, **kwargs) @@ -558,6 +569,50 @@ class RolesWithServiceLookupTests(unittest.TestCase): sorted(entry), ["canonical_domain", "canonical_url", "id", "iframe"] ) + def _card_target_role(self, service: str, **block_extra) -> dict: + block = {"enabled": True, "shared": True} + block.update(block_extra) + return { + "web-app-chat": { + "services": {service: block}, + "domains": { + "canonical": { + "synapse": "matrix.example.com", + "element": "element.example.com", + } + }, + }, + } + + def test_dashboard_card_key_targets_that_domain(self): + applications = self._card_target_role("dashboard", card="element") + entry = self._run(["dashboard"], applications)[0][0] + self.assertEqual( + (entry["canonical_domain"], entry["canonical_url"]), + ("element.example.com", "https://element.web-app-chat.example.com"), + ) + + def test_dashboard_without_card_key_targets_the_primary_domain(self): + applications = self._card_target_role("dashboard") + entry = self._run(["dashboard"], applications)[0][0] + self.assertEqual( + (entry["canonical_domain"], entry["canonical_url"]), + ("matrix.example.com", "https://web-app-chat.example.com"), + ) + + def test_dashboard_unknown_card_key_fails_loudly(self): + applications = self._card_target_role("dashboard", card="nope") + with self.assertRaisesRegex(AnsibleError, "nope"): + self._run(["dashboard"], applications) + + def test_other_services_ignore_a_card_key(self): + applications = self._card_target_role("prometheus", card="element") + entry = self._run(["prometheus"], applications)[0][0] + self.assertEqual( + (entry["canonical_domain"], entry["canonical_url"]), + ("matrix.example.com", "https://web-app-chat.example.com"), + ) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/python/utils/roles/applications/test_dashboard.py b/tests/unit/python/utils/roles/applications/test_dashboard.py new file mode 100644 index 0000000000..77a6b601a0 --- /dev/null +++ b/tests/unit/python/utils/roles/applications/test_dashboard.py @@ -0,0 +1,46 @@ +"""Unit tests for ``utils/roles/applications/dashboard.py``.""" + +from __future__ import annotations + +import unittest + +from ansible.errors import AnsibleError + +from utils.roles.applications.dashboard import card_target_key + + +class TestCardTargetKey(unittest.TestCase): + def test_a_binding_without_the_key_targets_the_primary_domain(self): + self.assertEqual( + "", card_target_key("web-app-foo", {"enabled": True, "shared": True}) + ) + + def test_a_missing_binding_targets_the_primary_domain(self): + self.assertEqual("", card_target_key("web-app-foo", None)) + + def test_the_named_key_is_returned(self): + self.assertEqual( + "element", + card_target_key("web-app-matrix", {"card": "element"}), + ) + + def test_surrounding_whitespace_is_dropped(self): + self.assertEqual( + "webmail", + card_target_key("web-app-stalwart", {"card": " webmail "}), + ) + + def test_an_alias_canonical_key_is_not_a_card_target(self): + self.assertEqual("", card_target_key("web-svc-cdn", {"canonical": "cdn"})) + + def test_an_empty_key_fails_loudly(self): + with self.assertRaisesRegex(AnsibleError, "web-app-foo"): + card_target_key("web-app-foo", {"card": " "}) + + def test_a_non_string_key_fails_loudly(self): + with self.assertRaises(AnsibleError): + card_target_key("web-app-foo", {"card": ["element"]}) + + +if __name__ == "__main__": + unittest.main() diff --git a/utils/roles/applications/dashboard.py b/utils/roles/applications/dashboard.py new file mode 100644 index 0000000000..403df776e5 --- /dev/null +++ b/utils/roles/applications/dashboard.py @@ -0,0 +1,42 @@ +"""Target of an application's dashboard card. + +A role's ``services.dashboard`` binding MAY name ``card: ``, a key of its +``domains.canonical`` mapping. The card, the Apps menu entry and the dashboard +tile-reachability target then point at that domain instead of the role's +primary domain. +""" + +from __future__ import annotations + +from typing import Any + +from ansible.errors import AnsibleError + +DASHBOARD_SERVICE = "dashboard" +CARD_TARGET_KEY = "card" + + +def card_target_key(application_id: str, dashboard: Any) -> str: + """Return the ``domains.canonical`` key the dashboard card targets. + + Args: + application_id: the role whose binding is read, named in the error. + dashboard: the role's ``services.dashboard`` binding. + + Returns: + The named key, or ``""`` when the binding names none and the card + targets the primary domain. + + Raises: + AnsibleError: the binding names something other than a non-empty + string. + """ + if not isinstance(dashboard, dict) or CARD_TARGET_KEY not in dashboard: + return "" + key = dashboard[CARD_TARGET_KEY] + if not isinstance(key, str) or not key.strip(): + raise AnsibleError( + f"{application_id}: services.{DASHBOARD_SERVICE}.{CARD_TARGET_KEY} " + f"must name a domains.canonical key, got {key!r}" + ) + return key.strip() From a0da3b326ed2b793d1e9f9e2c7a7881e97d3361f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:05:47 +0100 Subject: [PATCH 07/17] fix(web-app-yourls): upgrade the database schema on deploy instead of waiting for a browser click After an image update the YOURLS code was ahead of its schema (db_version 506 vs 507) and every short URL redirected to admin/upgrade.php until someone clicked Upgrade. install.yml now includes upgrade.yml, which runs files/php/upgrade_database.php as www-data inside the container: it boots YOURLS with YOURLS_UPGRADING, runs yourls_upgrade(1/3) when yourls_upgrade_is_needed(), prints 'upgraded -> ' or 'current ', and turns YOURLS' die() exits into exit 1. A probe short URL must then answer 301/302 without pointing at admin/upgrade.php. E2E against YOURLS 1.10.6 code with MariaDB: 506 -> 507 upgraded, second run 'current 507', duplicate-key failure exits 1 with the message. PHPUnit: 8 tests, 23 assertions. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../files/php/upgrade_database.php | 72 +++++++ roles/web-app-yourls/tasks/install.yml | 6 + roles/web-app-yourls/tasks/main.yml | 2 +- roles/web-app-yourls/tasks/upgrade.yml | 23 ++ roles/web-app-yourls/vars/main.yml | 3 + .../files/UpgradeDatabaseTest.php | 199 ++++++++++++++++++ 6 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 roles/web-app-yourls/files/php/upgrade_database.php create mode 100644 roles/web-app-yourls/tasks/upgrade.yml create mode 100644 tests/unit/php/roles/web-app-yourls/files/UpgradeDatabaseTest.php diff --git a/roles/web-app-yourls/files/php/upgrade_database.php b/roles/web-app-yourls/files/php/upgrade_database.php new file mode 100644 index 0000000000..0a51a374a5 --- /dev/null +++ b/roles/web-app-yourls/files/php/upgrade_database.php @@ -0,0 +1,72 @@ + -> " or "current ", exit 0. + * Failure: diagnostics on stderr, exit 1. Usage error: exit 2. + */ + +if ($argc !== 2) { + fwrite(STDERR, "usage: php upgrade_database.php YOURLS_ROOT\n"); + exit(2); +} + +define('YOURLS_UPGRADING', true); + +$infinito_upgrade_done = false; + +// Exception: YOURLS reports bootstrap and upgrade errors with die(), which exits 0; only this handler turns them into a failure. +register_shutdown_function(static function (): void { + if ($GLOBALS['infinito_upgrade_done']) { + return; + } + $chatter = ''; + while (ob_get_level() > 0) { + $chatter = ob_get_clean() . $chatter; + } + fwrite(STDERR, "YOURLS stopped before the database upgrade completed\n" . trim(strip_tags($chatter)) . "\n"); + exit(1); +}); + +function infinito_upgrade_report(string $line): void +{ + $GLOBALS['infinito_upgrade_done'] = true; + while (ob_get_level() > 0) { + ob_end_clean(); + } + echo $line, "\n"; + exit(0); +} + +function infinito_upgrade_fail(string $reason): void +{ + $GLOBALS['infinito_upgrade_done'] = true; + $chatter = ''; + while (ob_get_level() > 0) { + $chatter = ob_get_clean() . $chatter; + } + fwrite(STDERR, $reason . "\n" . trim(strip_tags($chatter)) . "\n"); + exit(1); +} + +ob_start(); +require rtrim($argv[1], '/') . '/includes/load-yourls.php'; + +list($infinito_old_version, $infinito_old_sql) = yourls_get_current_version_from_sql(); + +if (!yourls_upgrade_is_needed()) { + infinito_upgrade_report('current ' . $infinito_old_sql); +} + +if ((int) $infinito_old_sql < 200) { + infinito_upgrade_fail('db_version ' . $infinito_old_sql . ' predates YOURLS 1.4; its multi-request row conversion needs admin/upgrade.php'); +} + +yourls_upgrade(1, $infinito_old_version, YOURLS_VERSION, $infinito_old_sql, YOURLS_DB_VERSION); +yourls_upgrade(3, $infinito_old_version, YOURLS_VERSION, $infinito_old_sql, YOURLS_DB_VERSION); + +if (yourls_upgrade_is_needed()) { + infinito_upgrade_fail('YOURLS still needs a database upgrade after running it'); +} + +infinito_upgrade_report('upgraded ' . $infinito_old_sql . ' -> ' . YOURLS_DB_VERSION); diff --git a/roles/web-app-yourls/tasks/install.yml b/roles/web-app-yourls/tasks/install.yml index 859cb79958..7b971426c3 100644 --- a/roles/web-app-yourls/tasks/install.yml +++ b/roles/web-app-yourls/tasks/install.yml @@ -27,3 +27,9 @@ changed_when: "'tables successfully created' in yourls_install.stdout" failed_when: yourls_install.rc != 0 delegate_to: "{{ YOURLS_HOST_NODE }}" + +- name: "โฌ†๏ธ Bring the YOURLS database schema up to the running code" + ansible.builtin.include_tasks: + file: upgrade.yml + apply: + delegate_to: "{{ YOURLS_HOST_NODE }}" diff --git a/roles/web-app-yourls/tasks/main.yml b/roles/web-app-yourls/tasks/main.yml index 19d7f76cf9..39a55c2e4f 100644 --- a/roles/web-app-yourls/tasks/main.yml +++ b/roles/web-app-yourls/tasks/main.yml @@ -19,6 +19,6 @@ - name: "๐Ÿšฟ Flush compose handlers for {{ application_id }}" ansible.builtin.meta: flush_handlers -- name: "๐Ÿ“ฆ Install the YOURLS database schema for {{ application_id }}" +- name: "๐Ÿ“ฆ Install and upgrade the YOURLS database schema for {{ application_id }}" ansible.builtin.include_tasks: install.yml when: IS_STACK_HOST | bool diff --git a/roles/web-app-yourls/tasks/upgrade.yml b/roles/web-app-yourls/tasks/upgrade.yml new file mode 100644 index 0000000000..902930cfc6 --- /dev/null +++ b/roles/web-app-yourls/tasks/upgrade.yml @@ -0,0 +1,23 @@ +--- +- name: "โฌ†๏ธ Upgrade the YOURLS database schema when the code is ahead of it" + ansible.builtin.shell: + cmd: >- # nocheck: container-exec-resolver runtime-resolved CID, delegated to the hosting node + container exec -i --user {{ YOURLS_WEB_USER | quote }} {{ YOURLS_LOCAL_CID | quote }} + php -- {{ YOURLS_DOCROOT | quote }} + stdin: "{{ lookup('file', 'php/upgrade_database.php') }}" + register: yourls_upgrade + changed_when: yourls_upgrade.stdout is match('upgraded ') + failed_when: yourls_upgrade.rc != 0 or yourls_upgrade.stdout is not match('(upgraded|current) ') + +- name: "๐Ÿ”Ž Verify a short URL no longer redirects to admin/upgrade.php" + ansible.builtin.shell: + cmd: >- # nocheck: container-exec-resolver runtime-resolved CID, delegated to the hosting node + container exec {{ YOURLS_LOCAL_CID | quote }} + {{ 30 | curl }} -o /dev/null -w '%{http_code} %{redirect_url}' + http://127.0.0.1:{{ container_port }}/{{ YOURLS_UPGRADE_PROBE_KEYWORD }} + register: yourls_short_url_probe + changed_when: false + failed_when: >- + yourls_short_url_probe.rc != 0 + or yourls_short_url_probe.stdout is not match('30[12] ') + or 'admin/upgrade.php' in yourls_short_url_probe.stdout diff --git a/roles/web-app-yourls/vars/main.yml b/roles/web-app-yourls/vars/main.yml index 7c480bd034..a2fc42f2b0 100644 --- a/roles/web-app-yourls/vars/main.yml +++ b/roles/web-app-yourls/vars/main.yml @@ -12,6 +12,9 @@ YOURLS_CONTAINER: "{{ lookup('config', application_id, 'services.yourls.name') } YOURLS_CONTAINER_ADDRESS: "{{ lookup('container_address', application_id, entity_name) }}" YOURLS_ADMIN_LOCATION: "{{ lookup('config', application_id, 'server.locations.admin') }}" YOURLS_LANDINGPAGE_STATUS_CODE: "{{ lookup('config', application_id, 'server.status_codes.default') }}" +YOURLS_DOCROOT: "/var/www/html" +YOURLS_WEB_USER: "www-data" +YOURLS_UPGRADE_PROBE_KEYWORD: "infinito-upgrade-probe" YOURLS_SSO_PROXY_GATED: "{{ lookup('sso', application_id, 'is_proxy_gated') }}" YOURLS_SSO_ADMIN_GROUP: "/{{ lookup('rbac_group_path', application_id=application_id, role='administrator') }}" diff --git a/tests/unit/php/roles/web-app-yourls/files/UpgradeDatabaseTest.php b/tests/unit/php/roles/web-app-yourls/files/UpgradeDatabaseTest.php new file mode 100644 index 0000000000..7b58b27a60 --- /dev/null +++ b/tests/unit/php/roles/web-app-yourls/files/UpgradeDatabaseTest.php @@ -0,0 +1,199 @@ +Incorrect DB config, or could not connect to DB

'; + die(); + } + define('YOURLS_VERSION', '1.10.6'); + define('YOURLS_DB_VERSION', '507'); + + function yourls_get_current_version_from_sql() + { + $state = json_decode((string) file_get_contents(getenv('STUB_STATE')), true); + return [$state['version'], $state['db_version']]; + } + + function yourls_upgrade_is_needed() + { + list(, $sql) = yourls_get_current_version_from_sql(); + return $sql < YOURLS_DB_VERSION; + } + + function yourls_upgrade($step, $oldver, $newver, $oldsql, $newsql) + { + file_put_contents(getenv('STUB_LOG'), "$step $oldver $newver $oldsql $newsql\n", FILE_APPEND); + echo '

Updating DB. Please wait...

'; + if (getenv('STUB_FAIL_STEP') === (string) $step) { + echo "

Unable to update the DB.

"; + die(); + } + if ($step === 3 && getenv('STUB_SKIP_SAVE') !== '1') { + file_put_contents(getenv('STUB_STATE'), json_encode(['version' => $newver, 'db_version' => $newsql])); + } + } + PHP; + + private string $root; + + private string $state; + + private string $log; + + protected function setUp(): void + { + $this->root = sys_get_temp_dir() . '/yourls-upgrade-' . bin2hex(random_bytes(6)); + mkdir($this->root . '/includes', 0700, true); + file_put_contents($this->root . '/includes/load-yourls.php', self::STUB); + $this->state = $this->root . '/state.json'; + $this->log = $this->root . '/upgrade.log'; + touch($this->log); + } + + protected function tearDown(): void + { + foreach ([$this->root . '/includes/load-yourls.php', $this->state, $this->log] as $file) { + if (is_file($file)) { + unlink($file); + } + } + rmdir($this->root . '/includes'); + rmdir($this->root); + } + + private function seed(string $version, string $dbVersion): void + { + file_put_contents($this->state, json_encode(['version' => $version, 'db_version' => $dbVersion])); + } + + /** + * Run the script against the stub YOURLS root. + * + * @param array $env extra environment for the child + * @param list|null $args script arguments, null for the stub root + * @return array{0: string, 1: string, 2: int} stdout, stderr and exit code + */ + private function runScript(array $env = [], ?array $args = null): array + { + $script = dirname(__DIR__, 6) . '/roles/web-app-yourls/files/php/upgrade_database.php'; + $process = proc_open( + array_merge([PHP_BINARY, $script], $args ?? [$this->root]), + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + null, + $env + [ + 'STUB_STATE' => $this->state, + 'STUB_LOG' => $this->log, + 'PATH' => (string) getenv('PATH'), + ] + ); + $stdout = (string) stream_get_contents($pipes[1]); + $stderr = (string) stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + + return [$stdout, $stderr, proc_close($process)]; + } + + private function upgradeCalls(): string + { + return (string) file_get_contents($this->log); + } + + public function testCurrentSchemaIsReportedWithoutUpgrading(): void + { + $this->seed('1.10.6', '507'); + + [$stdout, , $code] = $this->runScript(); + + $this->assertSame(0, $code); + $this->assertSame("current 507\n", $stdout); + $this->assertSame('', $this->upgradeCalls()); + } + + public function testOutdatedSchemaRunsTheSchemaStepThenTheVersionStep(): void + { + $this->seed('1.10.2', '506'); + + [$stdout, , $code] = $this->runScript(); + + $this->assertSame(0, $code); + $this->assertSame("upgraded 506 -> 507\n", $stdout); + $this->assertSame("1 1.10.2 1.10.6 506 507\n3 1.10.2 1.10.6 506 507\n", $this->upgradeCalls()); + } + + public function testSecondRunAfterAnUpgradeIsANoop(): void + { + $this->seed('1.10.2', '506'); + $this->runScript(); + + [$stdout, , $code] = $this->runScript(); + + $this->assertSame(0, $code); + $this->assertSame("current 507\n", $stdout); + $this->assertSame(2, substr_count($this->upgradeCalls(), "\n")); + } + + public function testDieInsideAnUpgradeStepFailsTheRun(): void + { + $this->seed('1.10.2', '506'); + + [$stdout, $stderr, $code] = $this->runScript(['STUB_FAIL_STEP' => '1']); + + $this->assertSame(1, $code); + $this->assertSame('', $stdout); + $this->assertStringContainsString('Unable to update the DB.', $stderr); + } + + public function testDieDuringBootstrapFailsTheRun(): void + { + $this->seed('1.10.2', '506'); + + [$stdout, $stderr, $code] = $this->runScript(['STUB_BOOT_DIE' => '1']); + + $this->assertSame(1, $code); + $this->assertSame('', $stdout); + $this->assertStringContainsString('could not connect to DB', $stderr); + } + + public function testUpgradeThatLeavesTheSchemaOutdatedFails(): void + { + $this->seed('1.10.2', '506'); + + [$stdout, $stderr, $code] = $this->runScript(['STUB_SKIP_SAVE' => '1']); + + $this->assertSame(1, $code); + $this->assertSame('', $stdout); + $this->assertStringContainsString('still needs a database upgrade', $stderr); + } + + public function testPreOneFourSchemaIsRefusedBeforeAnyStep(): void + { + $this->seed('1.3', '100'); + + [, $stderr, $code] = $this->runScript(); + + $this->assertSame(1, $code); + $this->assertStringContainsString('predates YOURLS 1.4', $stderr); + $this->assertSame('', $this->upgradeCalls()); + } + + public function testMissingRootArgumentIsAUsageError(): void + { + [, $stderr, $code] = $this->runScript([], []); + + $this->assertSame(2, $code); + $this->assertStringContainsString('usage:', $stderr); + } +} From d45d1f07af1b8aa1f416501a28d273d2e710776f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:12:53 +0100 Subject: [PATCH 08/17] fix(web-app-discourse): clone the launcher into services/discourse_docker and retire the URL-named checkout DISCOURSE_REPOSITORY_DIR appended the repository URL to the services directory, so the launcher lived under services/https:/github.com/discourse/discourse_docker.git/. It now lives under services/discourse_docker. Before the clone, launcher/legacy_checkout.yml removes the URL-named tree (computed with relpath against the services directory) and prunes its emptied parents; a guard skips the cleanup when that path would escape the services directory or equal the new checkout. The cleanup must precede the clone: git otherwise resolves the old relative path as a URL and aborts with 'protocol ... is not supported'. Discourse data stays in the launcher's named volume and /var/discourse/shared; the first deploy rebuilds once because the bootstrap digest lived in the old containers/ directory. relpath joins the builtin filters test_defined knows. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-discourse/tasks/03_launcher.yml | 3 ++ .../tasks/launcher/legacy_checkout.yml | 20 +++++++++++ roles/web-app-discourse/vars/main.yml | 4 ++- .../ansible/filters/test_defined.py | 1 + .../test_legacy_checkout_order.py | 35 +++++++++++++++++++ 5 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 roles/web-app-discourse/tasks/launcher/legacy_checkout.yml create mode 100644 tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py diff --git a/roles/web-app-discourse/tasks/03_launcher.yml b/roles/web-app-discourse/tasks/03_launcher.yml index 3faf064458..7832a73a5d 100644 --- a/roles/web-app-discourse/tasks/03_launcher.yml +++ b/roles/web-app-discourse/tasks/03_launcher.yml @@ -3,6 +3,9 @@ ansible.builtin.set_fact: DISCOURSE_REBUILT: false +- name: "๐Ÿงน Retire the launcher checkout of the URL-named layout" + include_tasks: launcher/legacy_checkout.yml + - name: โฌ‡๏ธ Pull Discourse docker repository git: repo: "{{ DISCOURSE_REPOSITORY_URL }}" diff --git a/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml new file mode 100644 index 0000000000..c9bbf9864d --- /dev/null +++ b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml @@ -0,0 +1,20 @@ +--- +- name: "๐Ÿ›ก๏ธ Touch only the URL-named tree beside the new checkout" + when: (DISCOURSE_REPOSITORY_LEGACY_RELPATH | split('/') | first) not in ['..', DISCOURSE_REPOSITORY_DIR | basename] + become: true + block: + - name: "๐Ÿ—‘๏ธ Remove the URL-named launcher checkout" + ansible.builtin.file: + path: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH ] | path_join }}" + state: absent + + - name: "๐Ÿ—‘๏ธ Prune the emptied parents of the URL-named checkout" + ansible.builtin.command: + argv: + - rmdir + - --ignore-fail-on-non-empty + - --parents + - "{{ DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname }}" + chdir: "{{ DISCOURSE_SERVICES_DIR }}" + removes: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname ] | path_join }}" + when: DISCOURSE_REPOSITORY_LEGACY_RELPATH is search('/') diff --git a/roles/web-app-discourse/vars/main.yml b/roles/web-app-discourse/vars/main.yml index 8e423d3f92..7a9fd64fe3 100644 --- a/roles/web-app-discourse/vars/main.yml +++ b/roles/web-app-discourse/vars/main.yml @@ -24,7 +24,9 @@ DISCOURSE_DATABASE_TYPE: "{{ DISCOURSE_DATABASE.type }}" DISCOURSE_DATABASE_CONTAINER: "{{ DISCOURSE_DATABASE.container }}" # nocheck: container-var-name-lookup references the shared database container, not this role's service DISCOURSE_DATABASE_IS_SHARED: "{{ DISCOURSE_DATABASE.shared }}" -DISCOURSE_REPOSITORY_DIR: "{{ lookup('container', application_id, 'directories.services') }}{{ lookup('config', application_id, 'services.discourse.repository') }}/" +DISCOURSE_SERVICES_DIR: "{{ lookup('container', application_id, 'directories.services') }}" +DISCOURSE_REPOSITORY_DIR: "{{ [ DISCOURSE_SERVICES_DIR, 'discourse_docker' ] | path_join }}" +DISCOURSE_REPOSITORY_LEGACY_RELPATH: "{{ (DISCOURSE_SERVICES_DIR ~ DISCOURSE_REPOSITORY_URL) | relpath(DISCOURSE_SERVICES_DIR) }}" DISCOURSE_CONTAINERS_DIR: "{{ [ DISCOURSE_REPOSITORY_DIR, 'containers' ] | path_join }}" DISCOURSE_APPLICATION_YML_DEST: "{{ [ DISCOURSE_CONTAINERS_DIR, DISCOURSE_CONTAINER ~ '.yml' ] | path_join }}" diff --git a/tests/integration/ansible/filters/test_defined.py b/tests/integration/ansible/filters/test_defined.py index 34761f14a7..c98d537137 100644 --- a/tests/integration/ansible/filters/test_defined.py +++ b/tests/integration/ansible/filters/test_defined.py @@ -68,6 +68,7 @@ BUILTIN_FILTERS: set[str] = { "b64encode", "basename", "dirname", + "relpath", "from_json", "to_json", "from_yaml", diff --git a/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py b/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py new file mode 100644 index 0000000000..05a8ebcaa1 --- /dev/null +++ b/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py @@ -0,0 +1,35 @@ +from __future__ import annotations + +import unittest + +from utils.cache.files import PROJECT_ROOT, read_text +from utils.cache.yaml import load_yaml_str + +LAUNCHER = PROJECT_ROOT / "roles/web-app-discourse/tasks/03_launcher.yml" +CLEANUP = "launcher/legacy_checkout.yml" +GIT_MODULES = {"git", "ansible.builtin.git"} + + +class TestDiscourseLegacyCheckoutOrder(unittest.TestCase): + def test_the_url_named_checkout_is_retired_before_the_clone(self) -> None: + tasks = load_yaml_str(read_text(str(LAUNCHER))) + cleanup = next( + index + for index, task in enumerate(tasks) + if task.get("include_tasks") == CLEANUP + ) + clone = next( + index for index, task in enumerate(tasks) if GIT_MODULES & task.keys() + ) + self.assertLess( + cleanup, + clone, + "the git module clones from the services directory, where git takes " + "https://github.com/... for the old checkout's relative path " + "https:/github.com/... and aborts with 'protocol ... is not supported' " + "while that checkout exists", + ) + + +if __name__ == "__main__": + unittest.main() From 34da8a36c6a6e515e2e35b4f6d82d1c1f07a0f6a Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:18:37 +0100 Subject: [PATCH 09/17] perf(sys-svc-compose): set the compose container id without a remote echo resolve_host_cid ran a shell task on the stack host for every call even in compose mode, where it only echoed resolve_compose_cid back. The container-id shell task now runs in swarm only, and _LOCAL_CID takes resolve_compose_cid directly in compose. The util is included from about 60 roles; a full production run spent ~41 s on the 63 compose echoes. Replay harness old vs new: compose and swarm output facts identical (incl. the resolve-container-id subshell form), compose remote shells 20 -> 0, swarm commands unchanged; the mutant control reports DIFFERENT. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tasks/utils/swarm/resolve_host_cid.yml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/roles/sys-svc-compose/tasks/utils/swarm/resolve_host_cid.yml b/roles/sys-svc-compose/tasks/utils/swarm/resolve_host_cid.yml index 268df29dd9..207e253a7b 100644 --- a/roles/sys-svc-compose/tasks/utils/swarm/resolve_host_cid.yml +++ b/roles/sys-svc-compose/tasks/utils/swarm/resolve_host_cid.yml @@ -43,21 +43,18 @@ - name: "๐Ÿ” resolve_host_cid | resolve local container id for '{{ resolve_service_key }}'" ansible.builtin.shell: | - {% if DEPLOYMENT_MODE == 'swarm' %} container ps \ --filter "label=com.docker.swarm.service.name={{ _resolve_service_name }}" \ --filter status=running {{ resolve_cid_filters | default('') }} \ --format '{{ "{{.ID}}" }}' | head -1 - {% else %} - echo {{ resolve_compose_cid | quote }} - {% endif %} register: _resolve_cid changed_when: false retries: 30 delay: 2 - until: DEPLOYMENT_MODE == 'compose' or (_resolve_cid.stdout | trim) | length > 0 + until: (_resolve_cid.stdout | trim) | length > 0 delegate_to: "{{ lookup('vars', resolve_output_prefix ~ '_HOST_NODE') }}" + when: DEPLOYMENT_MODE == 'swarm' - name: "๐Ÿ”ง resolve_host_cid | set {{ resolve_output_prefix }}_LOCAL_CID" ansible.builtin.set_fact: - "{{ resolve_output_prefix }}_LOCAL_CID": "{{ _resolve_cid.stdout | trim }}" + "{{ resolve_output_prefix }}_LOCAL_CID": "{{ (_resolve_cid.stdout | trim) if DEPLOYMENT_MODE == 'swarm' else (resolve_compose_cid | trim) }}" From 73a16bef091f274fe29f240f840d72f74644aec5 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:22:32 +0100 Subject: [PATCH 10/17] perf(sys-svc-compose): prepare the compose instance in one module call Every app's compose pass ran three remote tasks before compose up: create the directories, compose ps for running containers, and stat the pending marker (53 apps x ~2.7 s in a production run). compose_instance_prepare does all three in one call from 02_stack_host_prep.yml, sharing the directory creation with directory_batch through utils/directory_batch.py; compose ps runs only in compose mode. up.yml evaluates the registered result, named compose_instance_before_files because it predates 04_files: the running check is skipped whenever the compose, env or override file changed in this run (those changes notify compose-up themselves). Replay harness old vs new: rendered files and the compose-up, marker and swarm handlers identical in every scenario; the only difference is the skipped running check when only the override changed, where compose-up still runs. About 85 s per full production run. Merge note: store-i18n's a4c7fda974 hoists the removed marker stat into a block; resolve up.yml with this side. Co-Authored-By: Claude Opus 5.5 (1M context) --- library/directory_batch.py | 90 +------- .../library/compose_instance_prepare.py | 65 ++++++ .../tasks/02_stack_host_prep.yml | 10 +- roles/sys-svc-compose/tasks/utils/up.yml | 38 +-- .../python/library/test_directory_batch.py | 3 + .../roles/sys-svc-compose/library/__init__.py | 5 + .../library/test_compose_instance_prepare.py | 216 ++++++++++++++++++ .../unit/python/utils/test_directory_batch.py | 73 ++++++ utils/directory_batch.py | 101 ++++++++ 9 files changed, 483 insertions(+), 118 deletions(-) create mode 100644 roles/sys-svc-compose/library/compose_instance_prepare.py create mode 100644 tests/unit/python/roles/sys-svc-compose/library/__init__.py create mode 100644 tests/unit/python/roles/sys-svc-compose/library/test_compose_instance_prepare.py create mode 100644 tests/unit/python/utils/test_directory_batch.py create mode 100644 utils/directory_batch.py diff --git a/library/directory_batch.py b/library/directory_batch.py index 115026cabd..ea333ff8eb 100644 --- a/library/directory_batch.py +++ b/library/directory_batch.py @@ -19,18 +19,8 @@ Returns: diff: one ``before``/``after`` record per changed path, in diff mode. """ -import os -from pathlib import Path - from ansible.module_utils.basic import AnsibleModule - -ATTRIBUTES = ("mode", "owner", "group") - -ATTRIBUTE_SPEC = { - "mode": {"type": "raw"}, - "owner": {"type": "str"}, - "group": {"type": "str"}, -} +from ansible.module_utils.directory_batch import ATTRIBUTE_SPEC, ATTRIBUTES, ensure_all ARGUMENT_SPEC = { "entries": { @@ -47,10 +37,6 @@ MUTUALLY_EXCLUSIVE = [["entries", name] for name in ("paths", *ATTRIBUTES)] REQUIRED_ONE_OF = [["entries", "paths"]] -class DirectoryError(Exception): - pass - - def requested_entries(params): if params["entries"] is not None: return params["entries"] @@ -60,64 +46,6 @@ def requested_entries(params): ] -def path_state(path): - target = Path(path) - if target.is_symlink(): - return "link" - if target.is_dir(): - return "directory" - if target.exists(): - return "file" - return "absent" - - -def apply_attributes(module, path, entry, changed, diff): - file_args = module.load_file_common_arguments( - {"path": path, **{name: entry[name] for name in ATTRIBUTES}} - ) - return module.set_fs_attributes_if_different(file_args, changed, diff, expand=False) - - -def create(module, path, entry, diff): - changed = False - current = "" - for part in path.strip("/").split("/"): - current = f"{current}/{part}" - candidate = Path(current) - if candidate.exists(): - continue - try: - candidate.mkdir() - changed = True - except FileExistsError: - if not candidate.is_dir(): - raise - changed = apply_attributes(module, current, entry, changed, diff) - return changed - - -def ensure(module, entry): - path = entry["path"] - if not Path(path).is_absolute(): - raise DirectoryError(f"{path!r} is not an absolute path") - if Path(path).is_symlink(): - path = os.path.realpath(path) - state = path_state(path) - diff = {"before": {"path": path}, "after": {"path": path}} - if state == "directory": - return apply_attributes(module, path, entry, False, diff), diff - if state != "absent": - raise DirectoryError(f"{path} already exists as a {state}") - diff["before"]["state"] = state - diff["after"]["state"] = "directory" - if module.check_mode: - return True, diff - try: - return create(module, path, entry, diff), diff - except OSError as error: - raise DirectoryError(f"Could not create {path}: {error}") from error - - def main(): module = AnsibleModule( argument_spec=ARGUMENT_SPEC, @@ -125,21 +53,7 @@ def main(): required_one_of=REQUIRED_ONE_OF, supports_check_mode=True, ) - changed_paths = [] - diffs = [] - for entry in requested_entries(module.params): - try: - changed, diff = ensure(module, entry) - except DirectoryError as error: - module.fail_json( - msg=str(error), - path=entry["path"], - changed=bool(changed_paths), - changed_paths=changed_paths, - ) - if changed: - changed_paths.append(entry["path"]) - diffs.append(diff) + changed_paths, diffs = ensure_all(module, requested_entries(module.params)) result = {"changed": bool(changed_paths), "changed_paths": changed_paths} if module._diff: result["diff"] = diffs diff --git a/roles/sys-svc-compose/library/compose_instance_prepare.py b/roles/sys-svc-compose/library/compose_instance_prepare.py new file mode 100644 index 0000000000..feb0996a5a --- /dev/null +++ b/roles/sys-svc-compose/library/compose_instance_prepare.py @@ -0,0 +1,65 @@ +"""Ensure a compose instance's directories and probe its state in one call. + +Args: + directories: paths ensured like ``directory_batch`` ``paths``. + mode: mode of every path in ``directories``. + instance: compose project directory ``compose ps`` runs in. + pending_marker: path reported in ``pending``. + running_check: whether to run ``compose ps -q --filter status=running`` + in ``instance``. + +Returns: + changed_paths: the directories that were created or whose mode changed. + diff: one ``before``/``after`` record per changed directory, in diff mode. + pending: whether ``pending_marker`` exists, a symlink not followed. + running: ``rc``, ``stdout`` and ``stderr`` of ``compose ps``, each stream + stripped of trailing newlines; null when ``running_check`` is false or + in check mode. +""" + +import os + +from ansible.module_utils.basic import AnsibleModule +from ansible.module_utils.directory_batch import ensure_all + +ARGUMENT_SPEC = { + "directories": {"type": "list", "elements": "path", "required": True}, + "mode": {"type": "raw", "required": True}, + "instance": {"type": "path", "required": True}, + "pending_marker": {"type": "path", "required": True}, + "running_check": {"type": "bool", "required": True}, +} + +RUNNING_ARGS = ("ps", "-q", "--filter", "status=running") + + +def running(module): + if not module.params["running_check"] or module.check_mode: + return None + compose = module.get_bin_path("compose", required=True) + rc, stdout, stderr = module.run_command( + [compose, *RUNNING_ARGS], cwd=module.params["instance"] + ) + return {"rc": rc, "stdout": stdout.rstrip("\r\n"), "stderr": stderr.rstrip("\r\n")} + + +def main(): + module = AnsibleModule(argument_spec=ARGUMENT_SPEC, supports_check_mode=True) + entries = [ + {"path": path, "mode": module.params["mode"], "owner": None, "group": None} + for path in module.params["directories"] + ] + changed_paths, diffs = ensure_all(module, entries) + result = { + "changed": bool(changed_paths), + "changed_paths": changed_paths, + "pending": os.path.lexists(module.params["pending_marker"]), + "running": running(module), + } + if module._diff: + result["diff"] = diffs + module.exit_json(**result) + + +if __name__ == "__main__": + main() diff --git a/roles/sys-svc-compose/tasks/02_stack_host_prep.yml b/roles/sys-svc-compose/tasks/02_stack_host_prep.yml index 628b87a212..b88fb5ce42 100644 --- a/roles/sys-svc-compose/tasks/02_stack_host_prep.yml +++ b/roles/sys-svc-compose/tasks/02_stack_host_prep.yml @@ -1,8 +1,12 @@ --- -- name: "๐Ÿ“ Create sys-svc-compose dirs (with parents) for '{{ application_id }}'" - directory_batch: - paths: "{{ lookup('container', application_id, 'directories').values() | list }}" +- name: "๐Ÿ“ Create sys-svc-compose dirs and probe '{{ application_id }}'" + compose_instance_prepare: + directories: "{{ lookup('container', application_id, 'directories').values() | list }}" mode: "0755" + instance: "{{ lookup('container', application_id, 'directories.instance') }}" + pending_marker: "{{ COMPOSE_UP_PENDING_MARKER }}" + running_check: "{{ compose_mode == 'compose' }}" + register: compose_instance_before_files - name: "๐Ÿ›ก๏ธ Ensure PHP hardening assets are installed" ansible.builtin.include_role: diff --git a/roles/sys-svc-compose/tasks/utils/up.yml b/roles/sys-svc-compose/tasks/utils/up.yml index 414120dd62..687e869625 100644 --- a/roles/sys-svc-compose/tasks/utils/up.yml +++ b/roles/sys-svc-compose/tasks/utils/up.yml @@ -1,34 +1,18 @@ --- - name: "๐Ÿ” Check running containers for '{{ application_id }}'" - command: >- - compose ps -q --filter status=running - args: - chdir: "{{ lookup('container', application_id, 'directories.instance') }}" - register: docker_ps - changed_when: > - (docker_ps.stdout | trim) == "" - failed_when: > - docker_ps.rc != 0 - and ( - (docker_ps.stderr | default('')) - | regex_search('(no configuration file provided|no such file or directory|env file .* not found)') is none - ) + ansible.builtin.assert: + that: >- + compose_instance_before_files.running.rc == 0 + or compose_instance_before_files.running.stderr + is search('(no configuration file provided|no such file or directory|env file .* not found)') + fail_msg: "compose ps exited {{ compose_instance_before_files.running.rc }}: {{ compose_instance_before_files.running.stderr }}" + quiet: true + changed_when: (compose_instance_before_files.running.stdout | trim) == "" when: - - > - not ( - docker_compose_template.changed | default(false) - or - env_template.changed | default(false) - ) - - compose_mode == 'compose' + - compose_instance_before_files.running is not none + - not (docker_compose_template.changed or env_template.changed or compose_override_template.changed) notify: compose-up -- name: "๐Ÿ” Check for a compose up a failed run left pending" - ansible.builtin.stat: - path: "{{ COMPOSE_UP_PENDING_MARKER }}" - register: compose_up_pending - when: compose_mode == 'compose' - - name: "๐Ÿ”” Resume the compose up a failed run left pending" ansible.builtin.debug: msg: "{{ COMPOSE_UP_PENDING_MARKER }} is still present, so the last compose up never completed" @@ -38,7 +22,7 @@ - compose-up when: - compose_mode == 'compose' - - compose_up_pending.stat.exists + - compose_instance_before_files.pending - compose_up_marker is not changed - name: "๐Ÿ”” Notify swarm deploy (under DEPLOYMENT_MODE == swarm)" diff --git a/tests/unit/python/library/test_directory_batch.py b/tests/unit/python/library/test_directory_batch.py index 2ebc4fcbdc..882690acd7 100644 --- a/tests/unit/python/library/test_directory_batch.py +++ b/tests/unit/python/library/test_directory_batch.py @@ -21,10 +21,13 @@ if TYPE_CHECKING: sys.path.insert(0, str(PROJECT_ROOT)) +from utils import directory_batch + MODULE_PATH = PROJECT_ROOT / "library" / "directory_batch.py" def _load_directory_batch() -> ModuleType: + sys.modules.setdefault("ansible.module_utils.directory_batch", directory_batch) spec = importlib.util.spec_from_file_location( "library_directory_batch", str(MODULE_PATH) ) diff --git a/tests/unit/python/roles/sys-svc-compose/library/__init__.py b/tests/unit/python/roles/sys-svc-compose/library/__init__.py new file mode 100644 index 0000000000..087c384b57 --- /dev/null +++ b/tests/unit/python/roles/sys-svc-compose/library/__init__.py @@ -0,0 +1,5 @@ +from pathlib import Path + +PROJECT_ROOT: Path = Path(__file__).resolve().parents[6] + +__all__ = ["PROJECT_ROOT"] diff --git a/tests/unit/python/roles/sys-svc-compose/library/test_compose_instance_prepare.py b/tests/unit/python/roles/sys-svc-compose/library/test_compose_instance_prepare.py new file mode 100644 index 0000000000..bfcb79fde5 --- /dev/null +++ b/tests/unit/python/roles/sys-svc-compose/library/test_compose_instance_prepare.py @@ -0,0 +1,216 @@ +from __future__ import annotations + +import contextlib +import importlib.util +import io +import json +import os +import stat +import sys +import tempfile +import unittest +from pathlib import Path +from typing import TYPE_CHECKING +from unittest import mock + +from ansible.module_utils.testing import patch_module_args + +from . import PROJECT_ROOT + +if TYPE_CHECKING: + from types import ModuleType + +sys.path.insert(0, str(PROJECT_ROOT)) + +from utils import directory_batch + +MODULE_PATH = ( + PROJECT_ROOT + / "roles" + / "sys-svc-compose" + / "library" + / "compose_instance_prepare.py" +) + +FAKE_COMPOSE = """#!/bin/sh +pwd -P > "{record}" +printf '%s\\n' "$@" >> "{record}" +printf '{stdout}' +printf '{stderr}' >&2 +exit {rc} +""" + + +def _load_module() -> ModuleType: + sys.modules.setdefault("ansible.module_utils.directory_batch", directory_batch) + spec = importlib.util.spec_from_file_location( + "sys_svc_compose_compose_instance_prepare", str(MODULE_PATH) + ) + if spec is None or spec.loader is None: + raise ImportError(f"Could not load: {MODULE_PATH}") + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) # type: ignore[attr-defined] + return mod + + +_MOD = _load_module() + + +class ComposeInstancePrepareTests(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = Path(self._tmp.name).resolve() + self.bin = self.root / "bin" + self.bin.mkdir() + self.record = self.root / "compose.record" + self.instance = self.root / "compose" / "app" + self.marker = self.instance / ".compose-up-pending" + self._fake_compose(stdout="", stderr="", rc=0) + + def tearDown(self): + self._tmp.cleanup() + + def _fake_compose(self, *, stdout, stderr, rc): + script = self.bin / "compose" + script.write_text( + FAKE_COMPOSE.format( + record=self.record, stdout=stdout, stderr=stderr, rc=rc + ), + encoding="utf-8", + ) + script.chmod(0o755) + + def _args(self, **overrides): + args = { + "directories": [str(self.instance), str(self.instance / "volumes")], + "mode": "0755", + "instance": str(self.instance), + "pending_marker": str(self.marker), + "running_check": True, + } + args.update(overrides) + return args + + def _run(self, args, *, check_mode=False, diff=False, path=None): + buffer = io.StringIO() + module_args = {**args, "_ansible_check_mode": check_mode, "_ansible_diff": diff} + with ( + mock.patch.dict(os.environ, {"PATH": str(path or self.bin)}), + patch_module_args(module_args), + contextlib.redirect_stdout(buffer), + self.assertRaises(SystemExit), + ): + _MOD.main() + return json.loads(buffer.getvalue()) + + def _recorded(self): + return self.record.read_text( # nocheck: cache-read tempdir record the fake compose rewrites on every call + encoding="utf-8" + ).splitlines() + + def test_creates_missing_directories_with_the_mode(self): + result = self._run(self._args()) + self.assertTrue(result["changed"]) + self.assertEqual( + result["changed_paths"], + [str(self.instance), str(self.instance / "volumes")], + ) + self.assertEqual(stat.S_IMODE(self.instance.stat().st_mode), 0o755) + + def test_existing_directories_are_unchanged(self): + self._run(self._args()) + result = self._run(self._args()) + self.assertFalse(result["changed"]) + self.assertEqual(result["changed_paths"], []) + + def test_probe_never_reports_changed_on_its_own(self): + self._run(self._args()) + self.marker.write_text("", encoding="utf-8") + result = self._run(self._args()) + self.assertFalse(result["changed"]) + + def test_pending_is_true_when_the_marker_exists(self): + self.instance.mkdir(parents=True) + self.marker.write_text("", encoding="utf-8") + self.assertTrue(self._run(self._args())["pending"]) + + def test_pending_is_false_without_the_marker(self): + self.assertFalse(self._run(self._args())["pending"]) + + def test_pending_counts_a_dangling_symlink_like_stat(self): + self.instance.mkdir(parents=True) + self.marker.symlink_to(self.root / "gone") + self.assertTrue(self._run(self._args())["pending"]) + + def test_running_reports_compose_ps_output_without_trailing_newlines(self): + self._fake_compose(stdout="abc\\ndef\\n", stderr="warn\\n", rc=0) + running = self._run(self._args())["running"] + self.assertEqual(running, {"rc": 0, "stdout": "abc\ndef", "stderr": "warn"}) + + def test_running_runs_ps_with_the_running_filter_in_the_instance(self): + self._run(self._args()) + self.assertEqual( + self._recorded(), + [str(self.instance), "ps", "-q", "--filter", "status=running"], + ) + + def test_running_reports_a_failing_ps_without_failing_the_module(self): + self._fake_compose(stdout="", stderr="no configuration file provided", rc=1) + result = self._run(self._args()) + self.assertNotIn("failed", result) + self.assertEqual(result["running"]["rc"], 1) + self.assertEqual(result["running"]["stderr"], "no configuration file provided") + + def test_running_is_null_and_ps_skipped_without_running_check(self): + result = self._run(self._args(running_check=False)) + self.assertIsNone(result["running"]) + self.assertFalse(self.record.exists()) + + def test_pending_is_reported_without_running_check(self): + self.instance.mkdir(parents=True) + self.marker.write_text("", encoding="utf-8") + self.assertTrue(self._run(self._args(running_check=False))["pending"]) + + def test_check_mode_creates_nothing_and_skips_ps(self): + result = self._run(self._args(), check_mode=True) + self.assertTrue(result["changed"]) + self.assertIsNone(result["running"]) + self.assertFalse(self.instance.exists()) + self.assertFalse(self.record.exists()) + + def test_directory_failure_fails_before_ps(self): + self.instance.parent.mkdir(parents=True) + self.instance.write_text("", encoding="utf-8") + result = self._run(self._args()) + self.assertTrue(result["failed"]) + self.assertIn("already exists as a file", result["msg"]) + self.assertFalse(self.record.exists()) + + def test_missing_compose_fails_loudly(self): + empty = self.root / "empty" + empty.mkdir() + result = self._run(self._args(), path=empty) + self.assertTrue(result["failed"]) + self.assertIn("compose", result["msg"]) + + def test_missing_compose_is_irrelevant_without_running_check(self): + empty = self.root / "empty" + empty.mkdir() + result = self._run(self._args(running_check=False), path=empty) + self.assertNotIn("failed", result) + + def test_diff_mode_lists_each_created_directory(self): + result = self._run(self._args(), diff=True) + self.assertEqual(len(result["diff"]), 2) + self.assertEqual(result["diff"][0]["after"]["state"], "directory") + + def test_running_check_is_required(self): + args = self._args() + del args["running_check"] + result = self._run(args) + self.assertTrue(result["failed"]) + self.assertIn("running_check", result["msg"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/python/utils/test_directory_batch.py b/tests/unit/python/utils/test_directory_batch.py new file mode 100644 index 0000000000..82a924a203 --- /dev/null +++ b/tests/unit/python/utils/test_directory_batch.py @@ -0,0 +1,73 @@ +from __future__ import annotations + +import contextlib +import io +import json +import tempfile +import unittest +from pathlib import Path + +from ansible.module_utils.basic import AnsibleModule +from ansible.module_utils.testing import patch_module_args + +from utils.directory_batch import ensure_all + + +def _entry(path, mode="0755"): + return {"path": str(path), "mode": mode, "owner": None, "group": None} + + +class EnsureAllTests(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = Path(self._tmp.name) + + def tearDown(self): + self._tmp.cleanup() + + def _ensure(self, entries, *, check_mode=False): + with patch_module_args({"_ansible_check_mode": check_mode}): + module = AnsibleModule(argument_spec={}, supports_check_mode=True) + return ensure_all(module, entries) + + def test_returns_the_changed_paths_in_order_with_one_diff_each(self): + first = self.root / "a" + second = self.root / "b" + changed_paths, diffs = self._ensure([_entry(first), _entry(second)]) + self.assertEqual(changed_paths, [str(first), str(second)]) + self.assertEqual( + [diff["after"]["path"] for diff in diffs], [str(first), str(second)] + ) + + def test_unchanged_paths_are_left_out(self): + existing = self.root / "kept" + existing.mkdir() + existing.chmod(0o755) + self.assertEqual(self._ensure([_entry(existing)]), ([], [])) + + def test_empty_entries_change_nothing(self): + self.assertEqual(self._ensure([]), ([], [])) + + def test_check_mode_reports_without_creating(self): + target = self.root / "new" + changed_paths, _ = self._ensure([_entry(target)], check_mode=True) + self.assertEqual(changed_paths, [str(target)]) + self.assertFalse(target.exists()) + + def test_first_error_fails_with_the_paths_changed_before_it(self): + created = self.root / "created" + blocker = self.root / "blocker" + never = self.root / "never" + blocker.write_text("", encoding="utf-8") + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer), self.assertRaises(SystemExit): + self._ensure([_entry(created), _entry(blocker), _entry(never)]) + result = json.loads(buffer.getvalue()) + self.assertTrue(result["failed"]) + self.assertEqual(result["path"], str(blocker)) + self.assertEqual(result["changed_paths"], [str(created)]) + self.assertFalse(never.exists()) + + +if __name__ == "__main__": + unittest.main() diff --git a/utils/directory_batch.py b/utils/directory_batch.py new file mode 100644 index 0000000000..25506cdc1d --- /dev/null +++ b/utils/directory_batch.py @@ -0,0 +1,101 @@ +import os +from pathlib import Path + +ATTRIBUTES = ("mode", "owner", "group") + +ATTRIBUTE_SPEC = { + "mode": {"type": "raw"}, + "owner": {"type": "str"}, + "group": {"type": "str"}, +} + + +class DirectoryError(Exception): + pass + + +def path_state(path): + target = Path(path) + if target.is_symlink(): + return "link" + if target.is_dir(): + return "directory" + if target.exists(): + return "file" + return "absent" + + +def apply_attributes(module, path, entry, changed, diff): + file_args = module.load_file_common_arguments( + {"path": path, **{name: entry[name] for name in ATTRIBUTES}} + ) + return module.set_fs_attributes_if_different(file_args, changed, diff, expand=False) + + +def create(module, path, entry, diff): + changed = False + current = "" + for part in path.strip("/").split("/"): + current = f"{current}/{part}" + candidate = Path(current) + if candidate.exists(): + continue + try: + candidate.mkdir() + changed = True + except FileExistsError: + if not candidate.is_dir(): + raise + changed = apply_attributes(module, current, entry, changed, diff) + return changed + + +def ensure(module, entry): + path = entry["path"] + if not Path(path).is_absolute(): + raise DirectoryError(f"{path!r} is not an absolute path") + if Path(path).is_symlink(): + path = os.path.realpath(path) + state = path_state(path) + diff = {"before": {"path": path}, "after": {"path": path}} + if state == "directory": + return apply_attributes(module, path, entry, False, diff), diff + if state != "absent": + raise DirectoryError(f"{path} already exists as a {state}") + diff["before"]["state"] = state + diff["after"]["state"] = "directory" + if module.check_mode: + return True, diff + try: + return create(module, path, entry, diff), diff + except OSError as error: + raise DirectoryError(f"Could not create {path}: {error}") from error + + +def ensure_all(module, entries): + """Ensure every entry in order and fail the module on the first error. + + Args: + module: the running ``AnsibleModule``. + entries: list of ``{path, mode, owner, group}``. + + Returns: + ``(changed_paths, diffs)``: the paths that were created or whose + attributes changed, and one ``before``/``after`` record per such path. + """ + changed_paths = [] + diffs = [] + for entry in entries: + try: + changed, diff = ensure(module, entry) + except DirectoryError as error: + module.fail_json( + msg=str(error), + path=entry["path"], + changed=bool(changed_paths), + changed_paths=changed_paths, + ) + if changed: + changed_paths.append(entry["path"]) + diffs.append(diff) + return changed_paths, diffs From 333b09cdca78b804473ab03f9e67050b98d3e896 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:29:48 +0100 Subject: [PATCH 11/17] perf(web-app-nextcloud): configure all queued plugins in one exec, writing only the values Nextcloud lacks Each plugin ran its own occ config:app:set per configuration value right after its install or enable, and a redeploy rewrote every value. Install and enable now queue the plugin; the configure stage runs once for the queue: it loads every queued addon spec, reads occ config:list once, keeps only the values Nextcloud does not hold exactly (nextcloud_plugin_config_pending; typed, masked and structured values are always kept), and runs the hooks in queue order afterwards. Combined with store-i18n's 7f72e53a58: the pending occ config:app:set calls go on the stdin of one container exec -i ... sh -s, so no value (Talk, OnlyOffice, whiteboard, BBB secrets) reaches a host argv; a failing call names its appid/configkey. Replay harness old vs new: identical final state, updated keys and hook order in first-deploy, converged, drift and failure scenarios (failure stops at the same key); converged config:app:set calls 19 -> 3. Merge note: resolve 03_configure.yml with this side against 7f72e53a58. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../filter_plugins/nextcloud_plugin_config.py | 85 +++++++ .../tasks/02_manager_ops/06_plugin.yml | 22 +- .../02_manager_ops/06_plugin/01_install.yml | 2 - .../02_manager_ops/06_plugin/02_enable.yml | 5 +- .../02_manager_ops/06_plugin/03_configure.yml | 64 +++--- .../tasks/02_manager_ops/06_plugin/README.md | 21 +- .../test_nextcloud_plugin_config.py | 214 ++++++++++++++++++ 7 files changed, 365 insertions(+), 48 deletions(-) create mode 100644 roles/web-app-nextcloud/filter_plugins/nextcloud_plugin_config.py create mode 100644 tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_plugin_config.py diff --git a/roles/web-app-nextcloud/filter_plugins/nextcloud_plugin_config.py b/roles/web-app-nextcloud/filter_plugins/nextcloud_plugin_config.py new file mode 100644 index 0000000000..f612f33e46 --- /dev/null +++ b/roles/web-app-nextcloud/filter_plugins/nextcloud_plugin_config.py @@ -0,0 +1,85 @@ +from __future__ import annotations + +from collections.abc import Mapping + +from ansible.plugins.filter.core import to_json + +SENSITIVE_VALUE = "***REMOVED SENSITIVE VALUE***" + + +def nextcloud_occ_value(configvalue) -> str: + """The string ``occ config:app:set --value`` receives for one entry. + + Args: + configvalue: the entry's ``configvalue`` as Ansible rendered it. + + Returns: + JSON for a mapping or a non-string sequence, ``str()`` for the rest. + """ + sequence = hasattr(configvalue, "__len__") and hasattr(configvalue, "__getitem__") + if isinstance(configvalue, Mapping) or ( + sequence and not isinstance(configvalue, str) + ): + return to_json(configvalue) + return str(configvalue) + + +def _held(current, value: str) -> bool: + if isinstance(current, bool) or current == SENSITIVE_VALUE: + return False + if isinstance(current, str): + return current == value + if isinstance(current, int): + return str(current) == value + return False + + +def _mapping(value) -> Mapping: + return value if isinstance(value, Mapping) else {} + + +def nextcloud_plugin_config_pending(specs, app_config): + """The plugin configuration entries ``occ config:app:set`` still has to write. + + Args: + specs: the addon specs of the plugins to configure, in the order their + entries are written; ``config.plugin_configuration`` is optional. + app_config: parsed ``occ config:list --output=json``. + + Returns: + list of dicts in spec and entry order, one per entry unless Nextcloud + already holds exactly its ``--value`` as a string or integer; an entry + with a ``type``, or held masked, as a boolean, float or structure, is + always returned: + appid - the app the key belongs to + configkey - the key + type - the ``--type`` of the entry, None when it has none + value - the ``--value`` string + """ + apps = _mapping(app_config["apps"]) + pending = [] + for spec in specs: + for item in (spec.get("config") or {}).get("plugin_configuration", []): + appid = item["appid"] + configkey = item["configkey"] + value = nextcloud_occ_value(item["configvalue"]) + typed = "type" in item + held = _mapping(apps.get(appid)) + if not typed and configkey in held and _held(held[configkey], value): + continue + pending.append( + { + "appid": appid, + "configkey": configkey, + "type": str(item["type"]) if typed else None, + "value": value, + } + ) + return pending + + +class FilterModule: + def filters(self): + return { + "nextcloud_plugin_config_pending": nextcloud_plugin_config_pending, + } diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin.yml index ef38c302f8..8fcaff427d 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin.yml @@ -8,6 +8,7 @@ - name: "๐Ÿงฎ Plan which plugins need an install, an enable or only their config" ansible.builtin.set_fact: nextcloud_plugin_plan: "{{ NEXTCLOUD_PLUGIN_ITEMS | nextcloud_plugin_plan(nextcloud_app_list.stdout | from_json) }}" + nextcloud_plugin_configure_keys: [] - name: "๐Ÿ”ด Disable the plugins that end up disabled" ansible.builtin.command: @@ -15,7 +16,7 @@ when: nextcloud_plugin_plan.disable | length > 0 changed_when: true -- name: "๐Ÿ“ฆ Install, enable and configure the plugins Nextcloud lacks" +- name: "๐Ÿ“ฆ Install and enable the plugins Nextcloud lacks" ansible.builtin.include_tasks: 06_plugin/01_install.yml loop: "{{ NEXTCLOUD_PLUGIN_ITEMS | selectattr('key', 'in', nextcloud_plugin_plan.install) | list }}" loop_control: @@ -26,7 +27,7 @@ plugin_key: "{{ plugin_item.key }}" plugin_value: "{{ plugin_item.value }}" -- name: "๐ŸŸข Enable and configure the installed plugins Nextcloud keeps disabled" +- name: "๐ŸŸข Enable the installed plugins Nextcloud keeps disabled" ansible.builtin.include_tasks: 06_plugin/02_enable.yml loop: "{{ NEXTCLOUD_PLUGIN_ITEMS | selectattr('key', 'in', nextcloud_plugin_plan.enable) | list }}" loop_control: @@ -36,16 +37,11 @@ vars: plugin_key: "{{ plugin_item.key }}" plugin_value: "{{ plugin_item.value }}" - plugin_task_path: "{{ NEXTCLOUD_CNODE_PLUGIN_TASKS_PATH }}{{ plugin_item.key }}.yml" -- name: "๐Ÿ”ง Configure the plugins Nextcloud already runs" +- name: "๐Ÿงฎ Queue the plugins Nextcloud already runs after the enabled ones" + ansible.builtin.set_fact: + nextcloud_plugin_configure_keys: "{{ nextcloud_plugin_configure_keys + nextcloud_plugin_plan.configure }}" + +- name: "๐Ÿ”ง Configure every queued plugin in one batch" ansible.builtin.include_tasks: 06_plugin/03_configure.yml - loop: "{{ NEXTCLOUD_PLUGIN_ITEMS | selectattr('key', 'in', nextcloud_plugin_plan.configure) | list }}" - loop_control: - loop_var: plugin_item - label: "{{ plugin_item.key }}" - no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" - vars: - plugin_key: "{{ plugin_item.key }}" - plugin_value: "{{ plugin_item.value }}" - plugin_task_path: "{{ NEXTCLOUD_CNODE_PLUGIN_TASKS_PATH }}{{ plugin_item.key }}.yml" + when: nextcloud_plugin_configure_keys | length > 0 diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/01_install.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/01_install.yml index a9810511df..4702fc9be2 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/01_install.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/01_install.yml @@ -80,5 +80,3 @@ - include_tasks: 02_enable.yml when: (install_result | nextcloud_install_status).runnable - vars: - plugin_task_path: "{{ NEXTCLOUD_CNODE_PLUGIN_TASKS_PATH }}{{ plugin_key }}.yml" diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/02_enable.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/02_enable.yml index ed7911a55c..385adc71ca 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/02_enable.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/02_enable.yml @@ -20,6 +20,7 @@ currently pinned Nextcloud server version. Skipping configure for this run. when: plugin_enable_incompatible -- name: "๐Ÿ”ง Configure {{ plugin_key }} (skipped when incompatible)" - ansible.builtin.include_tasks: 03_configure.yml +- name: "๐Ÿงฎ Queue {{ plugin_key }} for the batched configuration" + ansible.builtin.set_fact: + nextcloud_plugin_configure_keys: "{{ nextcloud_plugin_configure_keys + [plugin_key] }}" when: not plugin_enable_incompatible diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/03_configure.yml b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/03_configure.yml index 46c5f74ec0..df567eb4ad 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/03_configure.yml +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/03_configure.yml @@ -1,34 +1,46 @@ --- # Loaded via include_vars (not config lookup or lookup('file')) so the role-scoped vars referenced inside the payload render lazily in this task's scope. -- name: "๐Ÿ“„ Load {{ plugin_key }} addon spec from meta/addons/{{ plugin_key }}.yml" +- name: "๐Ÿ“„ Load the addon specs of the plugins to configure from meta/addons/" ansible.builtin.include_vars: file: "{{ [role_path, 'meta/addons', plugin_key ~ '.yml'] | path_join }}" - name: addon_spec + name: "nextcloud_addon_spec_{{ plugin_key }}" + loop: "{{ nextcloud_plugin_configure_keys }}" + loop_control: + loop_var: plugin_key -- name: "๐Ÿ”ง Set plugin configuration (batched shell, no async)" - ansible.builtin.shell: | - set -euo pipefail - {% for item in (addon_spec.config.plugin_configuration | default([])) %} - {{ NEXTCLOUD_DOCKER_EXEC_OCC }} \ - config:app:set {{ item.appid }} {{ item.configkey }}{{ (' --no-interaction --type=' ~ item.type) if item.type is defined else '' }} \ - --value '{{ ( (item.configvalue | to_json) if (item.configvalue is mapping or (item.configvalue is sequence and item.configvalue is not string)) else (item.configvalue | string) ) - | regex_replace("'", "'" ~ '"' ~ "'" ~ '"' ~ "'") }}' - {% endfor %} - args: - executable: /bin/bash - when: (addon_spec.config.plugin_configuration | default([])) | length > 0 - register: config_set_shell - changed_when: config_set_shell.stdout is search(' set to ') - failed_when: config_set_shell.rc != 0 +- name: "๐Ÿ“‹ Read the app configuration Nextcloud holds" + ansible.builtin.command: + cmd: "{{ NEXTCLOUD_DOCKER_EXEC_OCC }} config:list --output=json" + register: nextcloud_app_config + changed_when: false no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" -- name: ๐Ÿ”Ž Check if {{ plugin_task_path }} exists - stat: - path: "{{ plugin_task_path }}" - delegate_to: localhost - become: false - register: plugin_tasks_file +- name: "๐Ÿงฎ Keep the plugin configuration values Nextcloud does not hold yet" + ansible.builtin.set_fact: + nextcloud_plugin_config_pending: >- + {{ query('vars', *(nextcloud_plugin_configure_keys | map('regex_replace', '^', 'nextcloud_addon_spec_'))) + | nextcloud_plugin_config_pending(nextcloud_app_config.stdout | from_json) }} + no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" -- name: "๐Ÿ“‚ include {{ plugin_task_path }}" - include_tasks: "{{ plugin_task_path }}" - when: plugin_tasks_file.stat.exists +- name: "๐Ÿ”ง Set the pending plugin configuration (one exec, occ calls on stdin)" + ansible.builtin.shell: + cmd: "{{ NEXTCLOUD_DOCKER_EXEC_INTERACTIVE }} sh -s" + stdin: | + set -eu + {% for item in nextcloud_plugin_config_pending %} + {{ NEXTCLOUD_INTERNAL_OCC_COMMAND | quote }} config:app:set {{ item.appid | quote }} {{ item.configkey | quote }}{{ (' --no-interaction --type=' ~ item.type) if item.type is not none else '' }} --value {{ item.value | quote }} || { echo {{ ('config:app:set failed for ' ~ item.appid ~ '/' ~ item.configkey) | quote }} >&2; exit 1; } + {% endfor %} + when: nextcloud_plugin_config_pending | length > 0 + register: nextcloud_plugin_config_set + changed_when: nextcloud_plugin_config_set.stdout is search(' set to ') + failed_when: nextcloud_plugin_config_set.rc != 0 + no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" + +- name: "๐Ÿ“‚ Run the hook of every configured plugin from tasks/addons/" + ansible.builtin.include_tasks: "{{ plugin_task_path }}" + loop: "{{ nextcloud_plugin_configure_keys }}" + loop_control: + loop_var: plugin_key + vars: + plugin_task_path: "{{ NEXTCLOUD_CNODE_PLUGIN_TASKS_PATH }}{{ plugin_key }}.yml" + when: plugin_task_path is file diff --git a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/README.md b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/README.md index fe5d9a1bae..ef90610062 100644 --- a/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/README.md +++ b/roles/web-app-nextcloud/tasks/02_manager_ops/06_plugin/README.md @@ -28,8 +28,19 @@ Each `tasks/02_manager_ops/06_plugin/.yml` file owns exactly one OCC stage | File | Stage | OCC command | Continues to next stage when | |---|---|---|---| | [01_install.yml](./01_install.yml) | install | `occ app:install` | install result is `runnable` (success or already installed) | -| [02_enable.yml](./02_enable.yml) | enable | `occ app:enable` | enable result is not flagged `incompatible` | -| [03_configure.yml](./03_configure.yml) | configure | `occ config:app:set` plus optional hook | (terminal stage) | +| [02_enable.yml](./02_enable.yml) | enable | `occ app:enable` | enable result is not flagged `incompatible`; the plugin is then queued for the configure stage | +| [03_configure.yml](./03_configure.yml) | configure | one `occ config:list`, then `occ config:app:set` for the values that differ, plus optional hook | (terminal stage) | + +## Configure stage + +[03_configure.yml](./03_configure.yml) runs once for every queued plugin: first the plugins the install loop enabled, then those the enable loop enabled, then those of the `configure` plan list. + +1. It loads the addon spec of every queued plugin. +2. It reads `occ config:list` once and keeps an entry only when Nextcloud does not already hold the exact value `occ config:app:set` would store. Entries with a `type`, sensitive values that `config:list` masks, booleans, floats and structured values are always kept. +3. It writes the kept entries in spec order through one shell; the first failing `config:app:set` stops the shell and names its `appid/configkey` on stderr. The task reports changed only when `occ` answers `set to`. +4. It runs the hook of every queued plugin in queue order. + +The keep or drop decision lives in the filter `nextcloud_plugin_config_pending` ([filter_plugins/nextcloud_plugin_config.py](../../../filter_plugins/nextcloud_plugin_config.py)). The classification of "runnable" and "incompatible" is computed by the Jinja filter `nextcloud_install_status` defined in [roles/web-app-nextcloud/filter_plugins/nextcloud_install.py](../../../filter_plugins/nextcloud_install.py). The filter inspects `rc`, `stdout`, and `stderr` from the OCC call and returns flags that drive `until`, `failed_when`, and `when`. @@ -60,10 +71,10 @@ A new plugin requires no changes here when its addon spec and optional hook file - **Configuration keys (optional).** Add a `config.plugin_configuration` list to `meta/addons/.yml`. Each list item MUST contain `appid`, `configkey`, and `configvalue`. - `03_configure.yml` loads the addon spec and invokes `occ config:app:set` for every entry. + `03_configure.yml` loads the addon spec and invokes `occ config:app:set` for every entry whose value Nextcloud does not hold yet. - **Custom logic (optional).** Create `tasks/addons/.yml`. - The hook runs after `config:app:set` and MAY perform plugin-specific provisioning such as LDAP wiring or REST calls against the live container. + The hook runs after the configuration of every queued plugin and MAY perform plugin-specific provisioning such as LDAP wiring or REST calls against the live container. The hook is looked up via the path constant `NEXTCLOUD_CNODE_PLUGIN_TASKS_PATH` defined in [vars/main.yml](../../../vars/main.yml). Missing files MUST be tolerated; the corresponding step skips cleanly. @@ -77,4 +88,4 @@ This guarantees that mutually exclusive integrations such as the OIDC entry poin ## Entry point [02_manager_ops.yml](../../02_manager_ops.yml) includes [06_plugin.yml](../06_plugin.yml) once. -It loops over the `NEXTCLOUD_PLUGIN_ITEMS` of each plan list and unpacks the loop variable `plugin_item` into `plugin_key` (the OCC app name) and `plugin_value` (its meta entry). +It loops over the `NEXTCLOUD_PLUGIN_ITEMS` of the `install` and `enable` plan lists and unpacks the loop variable `plugin_item` into `plugin_key` (the OCC app name) and `plugin_value` (its meta entry), then includes the configure stage once for the queue `nextcloud_plugin_configure_keys`. diff --git a/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_plugin_config.py b/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_plugin_config.py new file mode 100644 index 0000000000..e57e10259f --- /dev/null +++ b/tests/unit/python/roles/web-app-nextcloud/filter_plugins/test_nextcloud_plugin_config.py @@ -0,0 +1,214 @@ +from __future__ import annotations + +import importlib.util +import sys +import unittest + +from ansible.template import Templar, trust_as_template + +from . import PROJECT_ROOT + + +def _load_module(rel_path: str, name: str): + path = PROJECT_ROOT / rel_path + spec = importlib.util.spec_from_file_location(name, str(path)) + mod = importlib.util.module_from_spec(spec) + assert spec and spec.loader + sys.modules[name] = mod + spec.loader.exec_module(mod) + return mod + + +config_mod = _load_module( + "roles/web-app-nextcloud/filter_plugins/nextcloud_plugin_config.py", + "nextcloud_plugin_config", +) +nextcloud_plugin_config_pending = config_mod.nextcloud_plugin_config_pending +nextcloud_occ_value = config_mod.nextcloud_occ_value + +PREVIOUS_VALUE_EXPRESSION = ( + "{{ ( (item.configvalue | to_json) if (item.configvalue is mapping or " + "(item.configvalue is sequence and item.configvalue is not string)) " + "else (item.configvalue | string) ) }}" +) + + +def _spec(*entries): + return {"config": {"plugin_configuration": list(entries)}} + + +def _entry(appid, configkey, configvalue, **extra): + return {"appid": appid, "configkey": configkey, "configvalue": configvalue, **extra} + + +def _apps(**apps): + return {"system": {}, "apps": apps} + + +def _keys(pending): + return [(item["appid"], item["configkey"]) for item in pending] + + +class TestNextcloudOccValue(unittest.TestCase): + def test_matches_the_previous_jinja_rendering(self): + values = [ + 43200, + 0, + "yes", + "", + "groupOfUniqueNames\nposixGroup", + "it's", + None, + True, + 1.5, + [{"name": "XWiki", "url": "https://wiki.example/"}], + {"custom_oidc": [{"name": "kc", "groupMapping": {"/a": "admin"}}]}, + {"รผmlaut": "รฉ", "nested": [1, 2.0, None, False]}, + '{"servers": []}', + ] + for value in values: + with self.subTest(value=value): + rendered = Templar(variables={"item": {"configvalue": value}}).template( + trust_as_template("=" + PREVIOUS_VALUE_EXPRESSION) + ) + self.assertEqual("=" + nextcloud_occ_value(value), rendered) + + +class TestNextcloudPluginConfigPending(unittest.TestCase): + def test_key_nextcloud_lacks_is_pending(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("user_ldap", "background_sync_interval", 43200))], + _apps(user_ldap={"enabled": "yes"}), + ) + self.assertEqual( + pending, + [ + { + "appid": "user_ldap", + "configkey": "background_sync_interval", + "type": None, + "value": "43200", + } + ], + ) + + def test_held_string_value_is_dropped(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("user_ldap", "s01ldap_host", "ldap"))], + _apps(user_ldap={"s01ldap_host": "ldap"}), + ) + self.assertEqual(pending, []) + + def test_held_integer_value_is_dropped(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("user_ldap", "s01ldap_port", 389))], + _apps(user_ldap={"s01ldap_port": 389}), + ) + self.assertEqual(pending, []) + + def test_negative_control_near_miss_values_stay_pending(self): + pending = nextcloud_plugin_config_pending( + [ + _spec( + _entry("a", "case", "Value"), + _entry("a", "space", "v"), + _entry("a", "number", 389), + _entry("a", "padded", "7"), + ) + ], + _apps(a={"case": "value", "space": "v ", "number": 390, "padded": 7.0}), + ) + self.assertEqual( + _keys(pending), + [("a", "case"), ("a", "space"), ("a", "number"), ("a", "padded")], + ) + + def test_boolean_value_is_always_pending(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("a", "flag", "1"), _entry("a", "other", "True"))], + _apps(a={"flag": True, "other": True}), + ) + self.assertEqual(_keys(pending), [("a", "flag"), ("a", "other")]) + + def test_masked_sensitive_value_is_always_pending(self): + masked = config_mod.SENSITIVE_VALUE + pending = nextcloud_plugin_config_pending( + [_spec(_entry("onlyoffice", "jwt_secret", masked))], + _apps(onlyoffice={"jwt_secret": masked}), + ) + self.assertEqual(_keys(pending), [("onlyoffice", "jwt_secret")]) + + def test_typed_entry_is_always_pending_with_its_type(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("spreed", "stun_servers", "[]", type="array"))], + _apps(spreed={"stun_servers": "[]"}), + ) + self.assertEqual(pending[0]["type"], "array") + self.assertEqual(pending[0]["value"], "[]") + + def test_structured_value_held_as_its_json_string_is_dropped(self): + value = [{"name": "XWiki", "url": "https://wiki.example/"}] + pending = nextcloud_plugin_config_pending( + [_spec(_entry("xwiki", "instances", value))], + _apps(xwiki={"instances": nextcloud_occ_value(value)}), + ) + self.assertEqual(pending, []) + + def test_structured_value_held_decoded_is_pending(self): + value = [{"name": "XWiki"}] + pending = nextcloud_plugin_config_pending( + [_spec(_entry("xwiki", "instances", value))], + _apps(xwiki={"instances": value}), + ) + self.assertEqual(_keys(pending), [("xwiki", "instances")]) + + def test_order_follows_specs_and_entries(self): + pending = nextcloud_plugin_config_pending( + [ + _spec( + _entry("onlyoffice", "jwt_secret", "s"), + _entry("onlyoffice", "DocumentServerUrl", "u"), + ), + {"enabled": True}, + _spec(_entry("bbb", "api.url", "b")), + ], + _apps(), + ) + self.assertEqual( + _keys(pending), + [ + ("onlyoffice", "jwt_secret"), + ("onlyoffice", "DocumentServerUrl"), + ("bbb", "api.url"), + ], + ) + + def test_spec_without_configuration_adds_nothing(self): + pending = nextcloud_plugin_config_pending( + [{"enabled": True}, {"config": None}, {"config": {"archive": "x"}}], + _apps(), + ) + self.assertEqual(pending, []) + + def test_empty_php_arrays_count_as_no_configuration(self): + pending = nextcloud_plugin_config_pending( + [_spec(_entry("deck", "k", "v"), _entry("news", "k", "v"))], + {"system": [], "apps": {"deck": []}}, + ) + self.assertEqual(_keys(pending), [("deck", "k"), ("news", "k")]) + pending = nextcloud_plugin_config_pending( + [_spec(_entry("deck", "k", "v"))], {"system": [], "apps": []} + ) + self.assertEqual(_keys(pending), [("deck", "k")]) + + +class TestFilterModule(unittest.TestCase): + def test_filter_is_registered(self): + self.assertEqual( + config_mod.FilterModule().filters(), + {"nextcloud_plugin_config_pending": nextcloud_plugin_config_pending}, + ) + + +if __name__ == "__main__": + unittest.main() From 2244cc691bca29f314e6395e2361e4f6bbe2784f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 21:35:14 +0100 Subject: [PATCH 12/17] perf(svc-bkp-volume-2-local): skip baudolo-seed when databases.csv already holds the consumer's row Every database consumer called baudolo-seed through the seed task, 37 times and always 'changed' in a full production run (54.4 s), although databases.csv rarely changes. The first seed of a play now reads the file once (05_read-seeded-databases.yml) into svc_bkp_volume_2_local_seeded; baudolo_seed_current compares the consumer's row with it and baudolo_seed_apply advances the cached state the way baudolo-seed writes the file, so the command runs only for a missing or differing row. A file baudolo did not write byte for byte (hand-edited, CRLF, short rows) falls back to one call per consumer; 02_reset forgets the cache with the file. Replay harness old vs new: databases.csv byte-identical in fresh, empty, converged, drift and reset-mid-play scenarios; converged seeds 37 -> 0, drift 38 -> 2; the negative control without reset invalidation diverges. About 44 s per unchanged redeploy. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugins/filter/baudolo_seed.py | 113 ++++++ roles/svc-bkp-volume-2-local/README.md | 1 + .../svc-bkp-volume-2-local/tasks/02_reset.yml | 4 + .../tasks/03_seed-database-to-backup.yml | 23 +- .../tasks/05_read-seeded-databases.yml | 22 ++ .../plugins/filter/test_baudolo_seed.py | 324 ++++++++++++++++++ 6 files changed, 482 insertions(+), 5 deletions(-) create mode 100644 plugins/filter/baudolo_seed.py create mode 100644 roles/svc-bkp-volume-2-local/tasks/05_read-seeded-databases.yml create mode 100644 tests/unit/python/plugins/filter/test_baudolo_seed.py diff --git a/plugins/filter/baudolo_seed.py b/plugins/filter/baudolo_seed.py new file mode 100644 index 0000000000..2226116356 --- /dev/null +++ b/plugins/filter/baudolo_seed.py @@ -0,0 +1,113 @@ +from __future__ import annotations + +import base64 +import binascii +import csv +import io +import re + +COLUMNS = ("instance", "database", "username", "password") +CLUSTER_ROW = "*" +_NAME_RE = re.compile(r"^[a-zA-Z0-9_][a-zA-Z0-9_-]*$") + + +def _serialize(rows) -> str: + buffer = io.StringIO() + writer = csv.writer(buffer, delimiter=";", lineterminator="\n") + writer.writerows(rows) + return buffer.getvalue() + + +def _database(value: str) -> str | None: + text = value.strip() + if text == CLUSTER_ROW: + return CLUSTER_ROW + if not text or text.lower() == "nan" or not _NAME_RE.match(text): + return None + return text + + +def baudolo_seeded(content: str) -> dict: + """The rows of a ``databases.csv`` and whether ``baudolo-seed`` wrote it. + + Args: + content: the file as base64, as ``slurp`` returns it; ``""`` for a file + that is absent or empty. + + Returns: + dict: + canonical - True only when the text is byte for byte what + ``baudolo-seed`` writes for these rows; undecodable or + hand-formatted text is False + rows - [instance, database, username, password] lists in file + order, empty when not canonical + """ + try: + text = base64.b64decode(content, validate=True).decode("utf-8") + except (binascii.Error, UnicodeDecodeError): + return {"canonical": False, "rows": []} + if not text: + return {"canonical": True, "rows": []} + parsed = list(csv.reader(io.StringIO(text, newline=""), delimiter=";")) + if not parsed or tuple(parsed[0]) != COLUMNS: + return {"canonical": False, "rows": []} + rows = parsed[1:] + canonical = ( + all(len(row) == len(COLUMNS) for row in rows) and _serialize(parsed) == text + ) + return {"canonical": canonical, "rows": rows if canonical else []} + + +def baudolo_seed_current(seeded, instance, database, username, password) -> bool: + """Whether ``baudolo-seed`` with these arguments would leave the file as is. + + Args: + seeded: the state ``baudolo_seeded`` or ``baudolo_seed_apply`` returned. + instance, database, username, password: the ``baudolo-seed`` arguments. + + Returns: + True when the file is canonical and every row of this instance and + database already carries these credentials. + """ + name = _database(database) + if not seeded["canonical"] or name is None: + return False + matches = [row for row in seeded["rows"] if row[0] == instance and row[1] == name] + return bool(matches) and all( + row[2] == username and row[3] == password for row in matches + ) + + +def baudolo_seed_apply(seeded, instance, database, username, password) -> dict: + """The state after ``baudolo-seed`` ran with these arguments. + + Args: + seeded: the state ``baudolo_seeded`` or ``baudolo_seed_apply`` returned. + instance, database, username, password: the ``baudolo-seed`` arguments. + + Returns: + dict shaped like the ``baudolo_seeded`` result, with every row of this + instance and database carrying the credentials, or one row appended + when there was none; a non-canonical state stays non-canonical. + """ + name = _database(database) + if not seeded["canonical"] or name is None: + return {"canonical": False, "rows": []} + rows = [list(row) for row in seeded["rows"]] + matched = False + for row in rows: + if row[0] == instance and row[1] == name: + row[2:] = [username, password] + matched = True + if not matched: + rows.append([instance, name, username, password]) + return {"canonical": True, "rows": rows} + + +class FilterModule: + def filters(self): + return { + "baudolo_seeded": baudolo_seeded, + "baudolo_seed_current": baudolo_seed_current, + "baudolo_seed_apply": baudolo_seed_apply, + } diff --git a/roles/svc-bkp-volume-2-local/README.md b/roles/svc-bkp-volume-2-local/README.md index 196385d452..abde30a068 100644 --- a/roles/svc-bkp-volume-2-local/README.md +++ b/roles/svc-bkp-volume-2-local/README.md @@ -9,6 +9,7 @@ File payloads are captured with rsync hard-link snapshots; databases register th This role installs the `baudolo` CLI, lays out the on-host backup tree, deploys the systemd service that drives the periodic run, and wires the cleanup-of-failed-backups dependency so partial snapshots are not retained. Database seeding for individual apps is contributed by the consumer roles via `tasks/03_seed-database-to-backup.yml`, which they include conditionally once `svc-bkp-volume-2-local` is in `group_names`. +The first seed of a play reads `databases.csv` once through `tasks/05_read-seeded-databases.yml`; a consumer whose row already holds its credentials skips `baudolo-seed`, every other consumer still calls it at its own point in the play. ## Cosmos diff --git a/roles/svc-bkp-volume-2-local/tasks/02_reset.yml b/roles/svc-bkp-volume-2-local/tasks/02_reset.yml index 6b5a5fc7c0..e2853b525f 100644 --- a/roles/svc-bkp-volume-2-local/tasks/02_reset.yml +++ b/roles/svc-bkp-volume-2-local/tasks/02_reset.yml @@ -3,3 +3,7 @@ ansible.builtin.file: path: "{{ FILE_DATABASE_SECRETS }}" state: absent + +- name: "๐Ÿงฎ Forget the seeded database definitions along with the file" + ansible.builtin.set_fact: + svc_bkp_volume_2_local_seeded: "{{ '' | baudolo_seeded }}" diff --git a/roles/svc-bkp-volume-2-local/tasks/03_seed-database-to-backup.yml b/roles/svc-bkp-volume-2-local/tasks/03_seed-database-to-backup.yml index f7da52467e..7a665afad5 100644 --- a/roles/svc-bkp-volume-2-local/tasks/03_seed-database-to-backup.yml +++ b/roles/svc-bkp-volume-2-local/tasks/03_seed-database-to-backup.yml @@ -1,6 +1,7 @@ --- -- include_tasks: 01_install.yml - when: run_once_svc_bkp_volume_2_local_install is not defined +- name: "๐Ÿ“– Read the seeded database definitions once per host" + include_tasks: 05_read-seeded-databases.yml + when: svc_bkp_volume_2_local_seeded is not defined - name: "๐Ÿ› BKP | Debug seed context (sanitized)" ansible.builtin.debug: @@ -47,9 +48,15 @@ Note: database_name may be an empty string to represent "all databases". changed_when: false -- name: ๐ŸŒฑ Seed database definition via baudolo-seed (system binary) - ansible.builtin.command: - argv: +- name: "๐Ÿงฎ Compare the database definition with the seeded one" + ansible.builtin.set_fact: + svc_bkp_volume_2_local_seed_argv: "{{ svc_bkp_volume_2_local_seed_command }}" + svc_bkp_volume_2_local_seed_current: >- + {{ svc_bkp_volume_2_local_seeded | baudolo_seed_current(*svc_bkp_volume_2_local_seed_command[2:]) }} + svc_bkp_volume_2_local_seeded: >- + {{ svc_bkp_volume_2_local_seeded | baudolo_seed_apply(*svc_bkp_volume_2_local_seed_command[2:]) }} + vars: + svc_bkp_volume_2_local_seed_command: - baudolo-seed - "{{ FILE_DATABASE_SECRETS | string }}" - "{{ database_instance | string }}" @@ -57,5 +64,11 @@ - "{{ database_username | string }}" - "{{ database_password | string }}" no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" + +- name: ๐ŸŒฑ Seed database definition via baudolo-seed (system binary) + ansible.builtin.command: + argv: "{{ svc_bkp_volume_2_local_seed_argv }}" + when: not (svc_bkp_volume_2_local_seed_current | bool) + no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" # todo: add flock to baudolo-seed upstream, then async seeding becomes safe again. throttle: 1 diff --git a/roles/svc-bkp-volume-2-local/tasks/05_read-seeded-databases.yml b/roles/svc-bkp-volume-2-local/tasks/05_read-seeded-databases.yml new file mode 100644 index 0000000000..5be6655631 --- /dev/null +++ b/roles/svc-bkp-volume-2-local/tasks/05_read-seeded-databases.yml @@ -0,0 +1,22 @@ +--- +- include_tasks: 01_install.yml + when: run_once_svc_bkp_volume_2_local_install is not defined + +- name: "๐Ÿ”Ž Check whether {{ FILE_DATABASE_SECRETS }} exists" + ansible.builtin.stat: + path: "{{ FILE_DATABASE_SECRETS }}" + register: svc_bkp_volume_2_local_seeded_file + +- name: "๐Ÿ“– Read the database definitions seeded on this host" + ansible.builtin.slurp: + src: "{{ FILE_DATABASE_SECRETS }}" + register: svc_bkp_volume_2_local_seeded_raw + when: svc_bkp_volume_2_local_seeded_file.stat.exists + no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" + +- name: "๐Ÿงฎ Remember the seeded database definitions for this play" + ansible.builtin.set_fact: + svc_bkp_volume_2_local_seeded: >- + {{ (svc_bkp_volume_2_local_seeded_raw.content if svc_bkp_volume_2_local_seeded_file.stat.exists else '') + | baudolo_seeded }} + no_log: "{{ MASK_CREDENTIALS_IN_LOGS | bool }}" diff --git a/tests/unit/python/plugins/filter/test_baudolo_seed.py b/tests/unit/python/plugins/filter/test_baudolo_seed.py new file mode 100644 index 0000000000..05b8118c4b --- /dev/null +++ b/tests/unit/python/plugins/filter/test_baudolo_seed.py @@ -0,0 +1,324 @@ +from __future__ import annotations + +import base64 +import io +import tempfile +import unittest +from contextlib import redirect_stderr, redirect_stdout +from pathlib import Path + +from baudolo.databases import DatabasesCsvError, validate_database +from baudolo.seed.__main__ import check_and_add_entry + +from plugins.filter.baudolo_seed import ( + FilterModule, + _database, + baudolo_seed_apply, + baudolo_seed_current, + baudolo_seeded, +) + +HEADER = "instance;database;username;password\n" + + +def _b64(text: str) -> str: + return base64.b64encode(text.encode("utf-8")).decode("ascii") + + +def _seeded(text: str) -> dict: + return baudolo_seeded(_b64(text)) + + +STABLE = [ + ("central-postgres", "nextcloud", "nextcloud", "pw-1"), + ("central-mariadb", "wordpress", "wordpress", "pa;ss"), + ("central-postgres", "", "postgres", 'quo"te'), + ("central-mariadb", "matomo", "matomo", "multi\nline"), + ("central-postgres", " keycloak ", "keycloak", " spaced "), + ("central-postgres", "gitea", "gitea", ""), + ("central-postgres", "listmonk", "listmonk", "รผnรฏcode"), +] +ENTRIES = [ + *STABLE[:5], + ("central-postgres", "nextcloud", "nextcloud", "pw-2"), + ("central-mariadb", "wordpress", "wordpress", "pa;ss"), + *STABLE[5:], +] + + +def _argv(entry) -> tuple[str, str, str, str]: + instance, name, username, password = entry + return instance, name or "*", username, password + + +def _bytes(path: Path) -> bytes: + if not path.exists(): + return b"" + return path.read_bytes() # nocheck: cache-read baudolo-seed rewrites the tempfile + + +def _b64_of(path: Path) -> str: + return base64.b64encode(_bytes(path)).decode("ascii") + + +def _seed(path: Path, argv) -> None: + with redirect_stdout(io.StringIO()), redirect_stderr(io.StringIO()): + check_and_add_entry(str(path), *argv) + + +class TestBaudoloSeedEquivalence(unittest.TestCase): + def _per_entry(self, path: Path, entries) -> int: + for entry in entries: + _seed(path, _argv(entry)) + return len(entries) + + def _skipping(self, path: Path, entries) -> int: + state = baudolo_seeded(_b64_of(path)) + calls = 0 + for entry in entries: + argv = _argv(entry) + current = baudolo_seed_current(state, *argv) + state = baudolo_seed_apply(state, *argv) + if not current: + _seed(path, argv) + calls += 1 + return calls + + def _run_both(self, start: bytes | None, entries): + with tempfile.TemporaryDirectory() as tmp: + old = Path(tmp) / "old.csv" + new = Path(tmp) / "new.csv" + if start is not None: + old.write_bytes(start) + new.write_bytes(start) + old_calls = self._per_entry(old, entries) + new_calls = self._skipping(new, entries) + return _bytes(old), _bytes(new), old_calls, new_calls + + def test_fresh_file_matches_and_skips_only_repeats(self): + old, new, old_calls, new_calls = self._run_both(None, ENTRIES) + self.assertEqual(old, new) + self.assertEqual(old_calls, len(ENTRIES)) + self.assertEqual(new_calls, len(ENTRIES) - 1) + + def test_empty_file_from_touch_matches(self): + old, new, _, _ = self._run_both(b"", ENTRIES) + self.assertEqual(old, new) + + def _converged(self, entries) -> bytes: + with tempfile.TemporaryDirectory() as tmp: + first = Path(tmp) / "first.csv" + self._per_entry(first, entries) + return _bytes(first) + + def test_converged_file_needs_no_call_and_keeps_its_bytes(self): + converged = self._converged(STABLE) + old, new, _, new_calls = self._run_both(converged, STABLE) + self.assertEqual(old, new) + self.assertEqual(new, converged) + self.assertEqual(new_calls, 0) + + def test_entries_that_overwrite_each_other_are_seeded_every_time(self): + converged = self._converged(ENTRIES) + old, new, _, new_calls = self._run_both(converged, ENTRIES) + self.assertEqual(old, new) + self.assertEqual(new_calls, 2) + + def test_rotated_password_is_seeded_again(self): + converged = self._converged(STABLE) + rotated = [*STABLE[:-1], ("central-postgres", "listmonk", "listmonk", "rot")] + old, new, _, new_calls = self._run_both(converged, rotated) + self.assertEqual(old, new) + self.assertEqual(new_calls, 1) + + def test_new_consumer_is_appended_after_the_converged_rows(self): + converged = self._converged(STABLE) + grown = [*STABLE, ("central-mariadb", "pixelfed", "pixelfed", "x")] + old, new, _, new_calls = self._run_both(converged, grown) + self.assertEqual(old, new) + self.assertEqual(new_calls, 1) + + def test_invalid_database_name_still_reaches_baudolo(self): + entry = ("central-postgres", "bad name", "u", "p") + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "databases.csv" + self._per_entry(path, STABLE) + with self.assertRaises(DatabasesCsvError): + self._skipping(path, [entry]) + + def test_hand_edited_file_falls_back_to_one_call_per_entry(self): + hand = (HEADER + "central-postgres;nextcloud;nextcloud;pw-2\r\n").encode() + old, new, old_calls, new_calls = self._run_both(hand, ENTRIES) + self.assertEqual(old, new) + self.assertEqual(new_calls, old_calls) + + def test_short_row_falls_back_to_one_call_per_entry(self): + short = (HEADER + "central-postgres;nextcloud\n").encode() + old, new, old_calls, new_calls = self._run_both(short, ENTRIES) + self.assertEqual(old, new) + self.assertEqual(new_calls, old_calls) + + def test_negative_control_a_state_older_than_the_file_breaks_equivalence(self): + converged = self._converged(STABLE) + with tempfile.TemporaryDirectory() as tmp: + stale = Path(tmp) / "stale.csv" + stale.write_bytes(b"") + state = baudolo_seeded(base64.b64encode(converged).decode("ascii")) + for entry in STABLE: + if not baudolo_seed_current(state, *_argv(entry)): + _seed(stale, _argv(entry)) + self.assertNotEqual(_bytes(stale), converged) + + +class TestBaudoloSeeded(unittest.TestCase): + def test_absent_or_empty_file_is_canonical_without_rows(self): + self.assertEqual(baudolo_seeded(""), {"canonical": True, "rows": []}) + + def test_header_only_is_canonical_without_rows(self): + self.assertEqual(_seeded(HEADER), {"canonical": True, "rows": []}) + + def test_rows_are_read_in_file_order(self): + text = HEADER + "a;db1;u1;p1\nb;*;u2;p2\n" + self.assertEqual( + _seeded(text), + { + "canonical": True, + "rows": [["a", "db1", "u1", "p1"], ["b", "*", "u2", "p2"]], + }, + ) + + def test_quoted_fields_stay_canonical(self): + text = HEADER + 'a;db;u;"pa;ss"\nb;db;u;"q""t"\n' + state = _seeded(text) + self.assertTrue(state["canonical"]) + self.assertEqual(state["rows"][0][3], "pa;ss") + self.assertEqual(state["rows"][1][3], 'q"t') + + def test_crlf_is_not_canonical(self): + self.assertFalse(_seeded(HEADER.replace("\n", "\r\n"))["canonical"]) + + def test_missing_trailing_newline_is_not_canonical(self): + self.assertFalse(_seeded(HEADER + "a;db;u;p")["canonical"]) + + def test_blank_line_is_not_canonical(self): + self.assertFalse(_seeded(HEADER + "\na;db;u;p\n")["canonical"]) + + def test_needless_quotes_are_not_canonical(self): + self.assertFalse(_seeded(HEADER + 'a;db;u;"p"\n')["canonical"]) + + def test_foreign_header_is_not_canonical(self): + self.assertFalse(_seeded("instance;database;user;password\n")["canonical"]) + + def test_undecodable_content_is_not_canonical(self): + self.assertFalse(baudolo_seeded("not base64!")["canonical"]) + self.assertFalse( + baudolo_seeded(base64.b64encode(b"\xff\xfe").decode())["canonical"] + ) + + +class TestBaudoloSeedCurrent(unittest.TestCase): + def setUp(self): + self.state = _seeded(HEADER + "a;db;u;p\na;*;su;sp\nb;db;u;p\nb;db;u;q\n") + + def test_same_credentials_are_current(self): + self.assertTrue(baudolo_seed_current(self.state, "a", "db", "u", "p")) + + def test_cluster_row_is_current(self): + self.assertTrue(baudolo_seed_current(self.state, "a", "*", "su", "sp")) + + def test_padded_database_name_matches_the_stripped_row(self): + self.assertTrue(baudolo_seed_current(self.state, "a", " db ", "u", "p")) + + def test_negative_control_other_password_is_not_current(self): + self.assertFalse(baudolo_seed_current(self.state, "a", "db", "u", "p2")) + + def test_negative_control_other_username_is_not_current(self): + self.assertFalse(baudolo_seed_current(self.state, "a", "db", "u2", "p")) + + def test_missing_row_is_not_current(self): + self.assertFalse(baudolo_seed_current(self.state, "c", "db", "u", "p")) + + def test_duplicate_rows_with_other_credentials_are_not_current(self): + self.assertFalse(baudolo_seed_current(self.state, "b", "db", "u", "p")) + + def test_invalid_database_name_is_never_current(self): + state = _seeded(HEADER + "a;bad name;u;p\n") + self.assertFalse(baudolo_seed_current(state, "a", "bad name", "u", "p")) + + def test_non_canonical_state_is_never_current(self): + state = _seeded(HEADER + 'a;db;u;"p"\n') + self.assertFalse(baudolo_seed_current(state, "a", "db", "u", "p")) + + +class TestBaudoloSeedApply(unittest.TestCase): + def test_new_definition_is_appended(self): + state = baudolo_seed_apply(_seeded(HEADER + "a;db;u;p\n"), "b", "*", "u", "p") + self.assertEqual(state["rows"], [["a", "db", "u", "p"], ["b", "*", "u", "p"]]) + + def test_every_matching_row_takes_the_credentials(self): + state = baudolo_seed_apply( + _seeded(HEADER + "a;db;u;p\nx;db;u;p\na;db;v;q\n"), "a", "db", "n", "m" + ) + self.assertEqual( + state["rows"], + [["a", "db", "n", "m"], ["x", "db", "u", "p"], ["a", "db", "n", "m"]], + ) + + def test_database_name_is_stored_stripped(self): + state = baudolo_seed_apply(baudolo_seeded(""), "a", " db ", "u", "p") + self.assertEqual(state["rows"], [["a", "db", "u", "p"]]) + + def test_input_state_is_not_mutated(self): + state = _seeded(HEADER + "a;db;u;p\n") + baudolo_seed_apply(state, "a", "db", "u", "q") + self.assertEqual(state["rows"], [["a", "db", "u", "p"]]) + + def test_non_canonical_state_stays_non_canonical(self): + state = baudolo_seed_apply( + _seeded(HEADER + 'a;db;u;"p"\n'), "a", "db", "u", "p" + ) + self.assertEqual(state, {"canonical": False, "rows": []}) + + def test_invalid_database_name_makes_the_state_non_canonical(self): + state = baudolo_seed_apply(baudolo_seeded(""), "a", "nan", "u", "p") + self.assertFalse(state["canonical"]) + + +class TestDatabaseNameParity(unittest.TestCase): + def test_validation_matches_baudolo(self): + for value in ( + "db", + " db ", + "*", + " * ", + "", + " ", + "nan", + "NaN", + "a-b_c", + "-a", + "a b", + "รผ", + ): + with self.subTest(value=value): + try: + expected = validate_database(value, instance="i") + except DatabasesCsvError: + expected = None + self.assertEqual(_database(value), expected) + + +class TestFilterModule(unittest.TestCase): + def test_filters_are_registered(self): + self.assertEqual( + FilterModule().filters(), + { + "baudolo_seeded": baudolo_seeded, + "baudolo_seed_current": baudolo_seed_current, + "baudolo_seed_apply": baudolo_seed_apply, + }, + ) + + +if __name__ == "__main__": + unittest.main() From 7751f6660d485802a99fead7dfdfd982741d73bc Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 22:02:42 +0100 Subject: [PATCH 13/17] perf(tls): read the certificate inventory once per play and write http.conf with the vhost Every TLS vhost ran its own certificate lookup, an include of sys-front-tls/tasks/01_http.yml and a separate http.conf write before the vhost template. The letsencrypt role now reads the certificate inventory once per play (utils/certificates.yml), and sys-stk-front-proxy writes the http->https handler and the vhost in one stack_host_contents call, http.conf first so a path collision keeps the old outcome. 01_http.yml is gone; its cert-renew dependency include moved into sys-front-tls/tasks/main.yml under the same conditions, and web-opt-rdr-domains already writes its own http.conf. front_proxy_vhost_changed keeps the reload verification and fallback restart tied to the vhost file, as nginx_conf.changed was before. Replay harness over 8 apps (proxy fresh/rerun/http-only/vhost-only/404, collision, self-signed, TLS off, swarm worker and manager, check mode, redirect domains): rendered files byte-identical, same handler and probe paths; the negative control diverges. About 160-195 s per full production run (55 TLS vhosts). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../sys-front-tls-letsencrypt/tasks/issue.yml | 14 +++--------- .../sys-front-tls-letsencrypt/tasks/main.yml | 15 ++++--------- .../tasks/utils/certificates.yml | 22 +++++++++++++++++++ roles/sys-front-tls/tasks/01_http.yml | 13 ----------- roles/sys-front-tls/tasks/main.yml | 10 ++++++--- roles/sys-stk-front-proxy/tasks/main.yml | 18 ++++++++++----- roles/sys-stk-front-proxy/vars/main.yml | 1 + 7 files changed, 49 insertions(+), 44 deletions(-) create mode 100644 roles/sys-front-tls-letsencrypt/tasks/utils/certificates.yml delete mode 100644 roles/sys-front-tls/tasks/01_http.yml diff --git a/roles/sys-front-tls-letsencrypt/tasks/issue.yml b/roles/sys-front-tls-letsencrypt/tasks/issue.yml index 6203f7d4d8..508360f603 100644 --- a/roles/sys-front-tls-letsencrypt/tasks/issue.yml +++ b/roles/sys-front-tls-letsencrypt/tasks/issue.yml @@ -9,20 +9,12 @@ vars: domain: "{{ _le_cert_domain }}" -- name: "๐Ÿ”Ž TLS | (letsencrypt) re-check cert files exist" - ansible.builtin.stat: - path: "{{ item }}" - register: _le_stat2 - loop: - - "{{ _cert_file }}" - - "{{ _cert_key }}" - changed_when: false +- name: "๐Ÿ”Ž TLS | (letsencrypt) refresh the certificate inventory" + include_tasks: utils/certificates.yml - name: "๐Ÿ›‘ TLS | (letsencrypt) fail if still missing" ansible.builtin.fail: msg: >- Let's Encrypt certificate files not found after issuance: cert={{ _cert_file }} key={{ _cert_key }} - when: > - (not _le_stat2.results[0].stat.exists | default(false)) or - (not _le_stat2.results[1].stat.exists | default(false)) + when: not ([_cert_file, _cert_key] is subset(tls_letsencrypt_certificates)) diff --git a/roles/sys-front-tls-letsencrypt/tasks/main.yml b/roles/sys-front-tls-letsencrypt/tasks/main.yml index d16545ecfe..cec149c4c1 100644 --- a/roles/sys-front-tls-letsencrypt/tasks/main.yml +++ b/roles/sys-front-tls-letsencrypt/tasks/main.yml @@ -20,17 +20,10 @@ but resolver returned enabled={{ _tls_enabled }}, mode={{ _tls_mode }}. changed_when: false -- name: "๐Ÿ”Ž TLS | (letsencrypt) check cert files exist" - ansible.builtin.stat: - path: "{{ item }}" - register: _le_stat - loop: - - "{{ _cert_file }}" - - "{{ _cert_key }}" - changed_when: false +- name: "๐Ÿ”Ž TLS | (letsencrypt) take the certificate inventory once per play" + include_tasks: utils/certificates.yml + when: tls_letsencrypt_certificates is not defined - name: "๐Ÿ“œ TLS | (letsencrypt) issue cert if missing" include_tasks: issue.yml - when: > - (not _le_stat.results[0].stat.exists | default(false)) or - (not _le_stat.results[1].stat.exists | default(false)) + when: not ([_cert_file, _cert_key] is subset(tls_letsencrypt_certificates)) diff --git a/roles/sys-front-tls-letsencrypt/tasks/utils/certificates.yml b/roles/sys-front-tls-letsencrypt/tasks/utils/certificates.yml new file mode 100644 index 0000000000..e3aea6f4d6 --- /dev/null +++ b/roles/sys-front-tls-letsencrypt/tasks/utils/certificates.yml @@ -0,0 +1,22 @@ +--- +- name: "๐Ÿ”Ž TLS | (letsencrypt) check the live directory" + ansible.builtin.stat: + path: "{{ LETSENCRYPT_LIVE_PATH }}" + register: tls_letsencrypt_live + +- name: "๐Ÿ”Ž TLS | (letsencrypt) find every certificate file in one call" + ansible.builtin.find: + paths: "{{ LETSENCRYPT_LIVE_PATH }}" + patterns: "{{ [_cert_file, _cert_key] | map('basename') | unique }}" + file_type: any + follow: true + recurse: true + depth: 2 + register: tls_letsencrypt_found + when: tls_letsencrypt_live.stat.exists + +- name: "๐Ÿ“Œ TLS | (letsencrypt) remember the certificate files" + ansible.builtin.set_fact: + tls_letsencrypt_certificates: >- + {{ tls_letsencrypt_found.files | map(attribute='path') | list + if tls_letsencrypt_live.stat.exists else [] }} diff --git a/roles/sys-front-tls/tasks/01_http.yml b/roles/sys-front-tls/tasks/01_http.yml deleted file mode 100644 index eb3bce0a01..0000000000 --- a/roles/sys-front-tls/tasks/01_http.yml +++ /dev/null @@ -1,13 +0,0 @@ ---- -- name: ๐Ÿ“‚ Include dependency 'sys-ctl-mtn-cert-renew' - include_role: - name: sys-ctl-mtn-cert-renew - when: - - run_once_sys_ctl_mtn_cert_renew is not defined - - lookup('tls', application_id, 'mode') == 'letsencrypt' - -- name: ๐Ÿ“ Create HTTP->HTTPS handler per domain - stack_host_template: - src: "http.conf.j2" - dest: "{{ lookup('nginx', 'files.domain', domain, protocol='http') }}" - notify: reload-openresty diff --git a/roles/sys-front-tls/tasks/main.yml b/roles/sys-front-tls/tasks/main.yml index 30cd6a3900..3c3dcc2ff9 100644 --- a/roles/sys-front-tls/tasks/main.yml +++ b/roles/sys-front-tls/tasks/main.yml @@ -14,6 +14,10 @@ name: sys-front-tls-selfsigned when: tls_mode_effective == 'self_signed' -- name: "๐Ÿ“‚ Load http redirect routines" - include_tasks: 01_http.yml - when: lookup('tls', application_id, 'enabled') | bool +- name: ๐Ÿ“‚ Include dependency 'sys-ctl-mtn-cert-renew' + include_role: + name: sys-ctl-mtn-cert-renew + when: + - run_once_sys_ctl_mtn_cert_renew is not defined + - lookup('tls', application_id, 'enabled') | bool + - tls_mode_effective == 'letsencrypt' diff --git a/roles/sys-stk-front-proxy/tasks/main.yml b/roles/sys-stk-front-proxy/tasks/main.yml index eb06d9d2ab..c8c53b1b56 100644 --- a/roles/sys-stk-front-proxy/tasks/main.yml +++ b/roles/sys-stk-front-proxy/tasks/main.yml @@ -98,9 +98,15 @@ webserver_vhost_flavour: basic - name: "๐Ÿš€ Deploy NGINX config for '{{ front_proxy_domain }}'" - stack_host_template: - src: "{{ front_proxy_template_src }}" - dest: "{{ front_proxy_domain_conf_dst_resolved }}" + stack_host_contents: + files: >- + {{ ([{'path': lookup('nginx', 'files.domain', front_proxy_domain, protocol='http'), + 'content': lookup('template', lookup('path_absolute', 'roles/sys-front-tls/templates/http.conf.j2'), + template_vars={'domain': front_proxy_domain})}] + if lookup('tls', front_proxy_application_id, 'enabled') | bool else []) + + [{'path': front_proxy_domain_conf_dst_resolved, + 'content': lookup('template', front_proxy_template_src)}] }} + mode: "0644" vars: application_id: "{{ front_proxy_application_id }}" domain: "{{ front_proxy_domain }}" @@ -124,14 +130,14 @@ register: front_proxy_reload_check failed_when: false changed_when: false - when: nginx_conf.changed + when: front_proxy_vhost_changed - name: "๐Ÿ”„ Redeploy: reload left {{ front_proxy_reload_check.status | default('none') }}" ansible.builtin.command: cmd: "true" notify: restart-openresty when: - - nginx_conf.changed + - front_proxy_vhost_changed - front_proxy_reload_check.status | default(-1) not in front_proxy_status_codes - >- not openresty_reload.changed @@ -139,4 +145,4 @@ - name: "๐Ÿ”„ Flush the fallback redeploy for '{{ front_proxy_domain }}'" ansible.builtin.meta: flush_handlers - when: nginx_conf.changed + when: front_proxy_vhost_changed diff --git a/roles/sys-stk-front-proxy/vars/main.yml b/roles/sys-stk-front-proxy/vars/main.yml index 840960dd45..85a105bd42 100644 --- a/roles/sys-stk-front-proxy/vars/main.yml +++ b/roles/sys-stk-front-proxy/vars/main.yml @@ -1,5 +1,6 @@ --- front_proxy_probe_status_codes: "{{ front_proxy_status_codes + front_proxy_upstream_down_status_codes }}" +front_proxy_vhost_changed: "{{ nginx_conf.changed and front_proxy_domain_conf_dst_resolved in nginx_conf.changed_paths }}" front_proxy_deferred_urls: "{{ dict(front_proxy_deferred_probes.keys() | zip(front_proxy_deferred_probes.values() | map(attribute='url'))) }}" front_proxy_deferred_answers: >- {{ From 11c2d8dcd3ea57616f95eab2c6a7f76466376e83 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 22:03:06 +0100 Subject: [PATCH 14/17] fix(roles): satisfy the include existence and pure-guard lints of the Discourse and XWiki bundles tests/integration/roles flagged two of today's bundles: the Discourse legacy-checkout guard lived in a block inside the included file, so the file loaded even when the guard was false; it now sits on the include_tasks in 03_launcher.yml and the file holds the two tasks with become. XWiki's utils/extensions_ready.yml included _check_extension_via_groovy.yml by bare name from tasks/utils/; it now uses {{ role_path }}/tasks/..., which resolves under include_tasks and include_role tasks_from alike (a bare ../ breaks under tasks_from, as the XWiki replay harness showed). Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-discourse/tasks/03_launcher.yml | 1 + .../tasks/launcher/legacy_checkout.yml | 32 +++++++++---------- .../tasks/utils/extensions_ready.yml | 2 +- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/roles/web-app-discourse/tasks/03_launcher.yml b/roles/web-app-discourse/tasks/03_launcher.yml index 7832a73a5d..be5a6e685c 100644 --- a/roles/web-app-discourse/tasks/03_launcher.yml +++ b/roles/web-app-discourse/tasks/03_launcher.yml @@ -5,6 +5,7 @@ - name: "๐Ÿงน Retire the launcher checkout of the URL-named layout" include_tasks: launcher/legacy_checkout.yml + when: (DISCOURSE_REPOSITORY_LEGACY_RELPATH | split('/') | first) not in ['..', DISCOURSE_REPOSITORY_DIR | basename] - name: โฌ‡๏ธ Pull Discourse docker repository git: diff --git a/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml index c9bbf9864d..d95e7910da 100644 --- a/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml +++ b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml @@ -1,20 +1,18 @@ --- -- name: "๐Ÿ›ก๏ธ Touch only the URL-named tree beside the new checkout" - when: (DISCOURSE_REPOSITORY_LEGACY_RELPATH | split('/') | first) not in ['..', DISCOURSE_REPOSITORY_DIR | basename] +- name: "๐Ÿ—‘๏ธ Remove the URL-named launcher checkout" + ansible.builtin.file: + path: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH ] | path_join }}" + state: absent become: true - block: - - name: "๐Ÿ—‘๏ธ Remove the URL-named launcher checkout" - ansible.builtin.file: - path: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH ] | path_join }}" - state: absent - - name: "๐Ÿ—‘๏ธ Prune the emptied parents of the URL-named checkout" - ansible.builtin.command: - argv: - - rmdir - - --ignore-fail-on-non-empty - - --parents - - "{{ DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname }}" - chdir: "{{ DISCOURSE_SERVICES_DIR }}" - removes: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname ] | path_join }}" - when: DISCOURSE_REPOSITORY_LEGACY_RELPATH is search('/') +- name: "๐Ÿ—‘๏ธ Prune the emptied parents of the URL-named checkout" + ansible.builtin.command: + argv: + - rmdir + - --ignore-fail-on-non-empty + - --parents + - "{{ DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname }}" + chdir: "{{ DISCOURSE_SERVICES_DIR }}" + removes: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname ] | path_join }}" + become: true + when: DISCOURSE_REPOSITORY_LEGACY_RELPATH is search('/') diff --git a/roles/web-app-xwiki/tasks/utils/extensions_ready.yml b/roles/web-app-xwiki/tasks/utils/extensions_ready.yml index fc8f82791d..780bdb43bd 100644 --- a/roles/web-app-xwiki/tasks/utils/extensions_ready.yml +++ b/roles/web-app-xwiki/tasks/utils/extensions_ready.yml @@ -41,7 +41,7 @@ and _xwiki_running_core.content is search('' ~ (XWIKI_CORE_VERSION | regex_escape) ~ '') }} - name: "โœ… XWIKI | Check the running wiki's add-ons against their pins" - include_tasks: _check_extension_via_groovy.yml + include_tasks: "{{ role_path }}/tasks/_check_extension_via_groovy.yml" loop: "{{ XWIKI_PLUGINS }}" loop_control: loop_var: plugin From 38230de0e319a5c3887cf379e90b4aae322c5c38 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 22:03:15 +0100 Subject: [PATCH 15/17] docs(web-app-stalwart): regenerate the Cosmos section with the role's name make docs renders the Cosmos intro from the role title, which 5b8e33db84 changed to Stalwart; the committed README still said Stalwart Mail Server. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-stalwart/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roles/web-app-stalwart/README.md b/roles/web-app-stalwart/README.md index 56aea9f6cc..ffc3c1a529 100644 --- a/roles/web-app-stalwart/README.md +++ b/roles/web-app-stalwart/README.md @@ -63,7 +63,7 @@ flowchart LR ## Cosmos -The diagram places Stalwart Mail Server in the Infinito.Nexus cosmos: the components it deploys (capabilities), the central services it consumes (dependencies), and its outward reach (federation and bridged external networks). +The diagram places Stalwart in the Infinito.Nexus cosmos: the components it deploys (capabilities), the central services it consumes (dependencies), and its outward reach (federation and bridged external networks). ```mermaid flowchart LR From f3e9f3de17b9e378a374dd1c38f214e8edb52222 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 22:11:54 +0100 Subject: [PATCH 16/17] fix(web-app-discourse): set become once through apply on the legacy-checkout include test_include_uniform_apply_keyword flagged legacy_checkout.yml after 11c2d8dcd3 moved its guard onto the include: both tasks repeated become: true. The include in 03_launcher.yml now applies become to the file and keeps the guard; the order test reads include_tasks in its mapping form too. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/web-app-discourse/tasks/03_launcher.yml | 5 ++++- .../web-app-discourse/tasks/launcher/legacy_checkout.yml | 2 -- .../web-app-discourse/test_legacy_checkout_order.py | 9 ++++++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/roles/web-app-discourse/tasks/03_launcher.yml b/roles/web-app-discourse/tasks/03_launcher.yml index be5a6e685c..e13119beab 100644 --- a/roles/web-app-discourse/tasks/03_launcher.yml +++ b/roles/web-app-discourse/tasks/03_launcher.yml @@ -4,7 +4,10 @@ DISCOURSE_REBUILT: false - name: "๐Ÿงน Retire the launcher checkout of the URL-named layout" - include_tasks: launcher/legacy_checkout.yml + include_tasks: + file: launcher/legacy_checkout.yml + apply: + become: true when: (DISCOURSE_REPOSITORY_LEGACY_RELPATH | split('/') | first) not in ['..', DISCOURSE_REPOSITORY_DIR | basename] - name: โฌ‡๏ธ Pull Discourse docker repository diff --git a/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml index d95e7910da..a0820a7f93 100644 --- a/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml +++ b/roles/web-app-discourse/tasks/launcher/legacy_checkout.yml @@ -3,7 +3,6 @@ ansible.builtin.file: path: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH ] | path_join }}" state: absent - become: true - name: "๐Ÿ—‘๏ธ Prune the emptied parents of the URL-named checkout" ansible.builtin.command: @@ -14,5 +13,4 @@ - "{{ DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname }}" chdir: "{{ DISCOURSE_SERVICES_DIR }}" removes: "{{ [ DISCOURSE_SERVICES_DIR, DISCOURSE_REPOSITORY_LEGACY_RELPATH | dirname ] | path_join }}" - become: true when: DISCOURSE_REPOSITORY_LEGACY_RELPATH is search('/') diff --git a/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py b/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py index 05a8ebcaa1..b0937cd1d3 100644 --- a/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py +++ b/tests/unit/python/roles/web-app-discourse/test_legacy_checkout_order.py @@ -10,13 +10,16 @@ CLEANUP = "launcher/legacy_checkout.yml" GIT_MODULES = {"git", "ansible.builtin.git"} +def _included_file(task: dict) -> str | None: + include = task.get("include_tasks") + return include.get("file") if isinstance(include, dict) else include + + class TestDiscourseLegacyCheckoutOrder(unittest.TestCase): def test_the_url_named_checkout_is_retired_before_the_clone(self) -> None: tasks = load_yaml_str(read_text(str(LAUNCHER))) cleanup = next( - index - for index, task in enumerate(tasks) - if task.get("include_tasks") == CLEANUP + index for index, task in enumerate(tasks) if _included_file(task) == CLEANUP ) clone = next( index for index, task in enumerate(tasks) if GIT_MODULES & task.keys() From 0359d04c82a2d788c39edda2f5ba6c88013a8fb6 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 25 Oct 2026 22:21:12 +0100 Subject: [PATCH 17/17] perf(sys-ctl-hlth-csp): probe four vhosts in parallel with csp-checker 3.2.0 csp-checker 3.2.0 checks every URL in its own browser context and takes --concurrency for a worker pool. The role pins 3.2.0, sets HEALTH_CSP_CONCURRENCY to 4, and the systemd unit hands it through script.py to the container command ahead of the -- URL separator, for the clearnet and the onion batch alike. TimeoutStartSec keeps its sequential sizing. Co-Authored-By: Claude Opus 5.5 (1M context) --- roles/sys-ctl-hlth-csp/defaults/main.yml | 3 +++ roles/sys-ctl-hlth-csp/files/python/script.py | 16 +++++++++++ roles/sys-ctl-hlth-csp/meta/services.yml | 2 +- roles/sys-ctl-hlth-csp/tasks/00_core.yml | 1 + .../sys-ctl-hlth-csp/files/test_script.py | 27 +++++++++++++++++++ .../test_unit_flags_are_parsable.py | 2 ++ 6 files changed, 50 insertions(+), 1 deletion(-) diff --git a/roles/sys-ctl-hlth-csp/defaults/main.yml b/roles/sys-ctl-hlth-csp/defaults/main.yml index d357878bf7..858d71e04a 100644 --- a/roles/sys-ctl-hlth-csp/defaults/main.yml +++ b/roles/sys-ctl-hlth-csp/defaults/main.yml @@ -16,3 +16,6 @@ HEALTH_CSP_TIMEOUT_MS: 20000 # Seconds added per vhost on top of its navigation budget when sizing TimeoutStartSec HEALTH_CSP_DOMAIN_RESERVE_SECONDS: 5 + +# Number of vhosts the checker probes in parallel, each in its own browser context +HEALTH_CSP_CONCURRENCY: 4 diff --git a/roles/sys-ctl-hlth-csp/files/python/script.py b/roles/sys-ctl-hlth-csp/files/python/script.py index 7d085e23eb..fb5f8962e9 100755 --- a/roles/sys-ctl-hlth-csp/files/python/script.py +++ b/roles/sys-ctl-hlth-csp/files/python/script.py @@ -159,6 +159,7 @@ def build_docker_cmd( proxy: str = "", timeout_ms: int = 0, accept_status: list[str] | None = None, + concurrency: int = 0, ) -> list[str]: cmd = ["container", "run", "--rm"] @@ -180,6 +181,9 @@ def build_docker_cmd( if timeout_ms: cmd.extend(["--timeout", str(timeout_ms)]) + if concurrency: + cmd.extend(["--concurrency", str(concurrency)]) + if ignore_network_blocks_from: cmd.append("--ignore-network-blocks-from") cmd.extend(ignore_network_blocks_from) @@ -205,6 +209,7 @@ def run_checker( proxy: str = "", timeout_ms: int = 0, accept_status: list[str] | None = None, + concurrency: int = 0, ) -> int: """ Runs the CSP checker container and returns its exit code. @@ -222,6 +227,7 @@ def run_checker( proxy=proxy, timeout_ms=timeout_ms, accept_status=accept_status, + concurrency=concurrency, ) try: @@ -302,6 +308,15 @@ def main() -> None: "own default. 0 leaves that default in place." ), ) + parser.add_argument( + "--concurrency", + type=int, + default=0, + help=( + "Number of URLs the checker probes in parallel, each in its own " + "browser context. 0 leaves the checker's own default in place." + ), + ) parser.add_argument( "--tor-proxy", default="", @@ -372,6 +387,7 @@ def main() -> None: proxy=batch_proxy, timeout_ms=batch_timeout, accept_status=accept_status, + concurrency=args.concurrency, ) rc = rc or batch_rc sys.exit(rc) diff --git a/roles/sys-ctl-hlth-csp/meta/services.yml b/roles/sys-ctl-hlth-csp/meta/services.yml index ed1933667c..cdc83424a1 100644 --- a/roles/sys-ctl-hlth-csp/meta/services.yml +++ b/roles/sys-ctl-hlth-csp/meta/services.yml @@ -8,4 +8,4 @@ csp: categories: [web] csp-checker: image: ghcr.io/kevinveenbirkenbach/csp-checker - version: 3.1.3 + version: 3.2.0 diff --git a/roles/sys-ctl-hlth-csp/tasks/00_core.yml b/roles/sys-ctl-hlth-csp/tasks/00_core.yml index 2d965b1a5c..68fefda5af 100644 --- a/roles/sys-ctl-hlth-csp/tasks/00_core.yml +++ b/roles/sys-ctl-hlth-csp/tasks/00_core.yml @@ -51,6 +51,7 @@ ), '--image=' ~ HEALTH_CSP_IMAGE, '--short', + '--concurrency=' ~ HEALTH_CSP_CONCURRENCY, (HEALTH_CSP_ALWAYS_PULL | bool) | ternary('--always-pull', ''), (HEALTH_CSP_IGNORE_NETWORK_BLOCKS_FROM | length > 0) | ternary( '--ignore-network-blocks-from ' ~ (HEALTH_CSP_IGNORE_NETWORK_BLOCKS_FROM | join(' ')), diff --git a/tests/unit/python/roles/sys-ctl-hlth-csp/files/test_script.py b/tests/unit/python/roles/sys-ctl-hlth-csp/files/test_script.py index 8d7e98aac8..6a357b5338 100644 --- a/tests/unit/python/roles/sys-ctl-hlth-csp/files/test_script.py +++ b/tests/unit/python/roles/sys-ctl-hlth-csp/files/test_script.py @@ -207,6 +207,33 @@ class TestBuildDockerCmdTimeout(unittest.TestCase): ) +class TestBuildDockerCmdConcurrency(unittest.TestCase): + def test_concurrency_arg_precedes_the_url_separator(self) -> None: + cmd = script.build_docker_cmd( + image="img", + urls=["https://a.example/"], + short_mode=True, + ignore_network_blocks_from=["cdn.example"], + concurrency=4, + ) + idx = cmd.index("--concurrency") + self.assertEqual(cmd[idx + 1], "4") + self.assertLess( + idx, + cmd.index("--"), + "an argument after the separator reaches the checker as a URL", + ) + + def test_no_concurrency_arg_without_one(self) -> None: + cmd = script.build_docker_cmd( + image="img", + urls=["https://a.example/"], + short_mode=True, + ignore_network_blocks_from=[], + ) + self.assertNotIn("--concurrency", cmd) + + class TestMainTimesOutOnionsOnly(unittest.TestCase): @patch("script.run_checker") @patch("script.build_urls_from_nginx_confs") diff --git a/tests/unit/python/roles/sys-ctl-hlth-csp/test_unit_flags_are_parsable.py b/tests/unit/python/roles/sys-ctl-hlth-csp/test_unit_flags_are_parsable.py index 759832c95a..123b7c6b82 100644 --- a/tests/unit/python/roles/sys-ctl-hlth-csp/test_unit_flags_are_parsable.py +++ b/tests/unit/python/roles/sys-ctl-hlth-csp/test_unit_flags_are_parsable.py @@ -17,6 +17,7 @@ _VALUE = { "--image": "img", "--tor-proxy": "socks5://127.0.0.1:9050", "--onion-timeout": "1000", + "--concurrency": "4", } _LIST_VALUE = { "--ignore-network-blocks-from": "a.test", @@ -53,6 +54,7 @@ class TestUnitFlagsAreParsable(unittest.TestCase): self.assertIn("--accept-status", found) self.assertIn("--onion-timeout", found) + self.assertIn("--concurrency", found) def test_every_flag_the_unit_passes_is_accepted_by_the_script(self): for flag in sorted(_flags_the_unit_passes()):