From 0c754bda608df02a740d533b5d6b3ffa4bd215d7 Mon Sep 17 00:00:00 2001 From: martino <32328813+f3rs3n@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:07:21 +0200 Subject: [PATCH] fix(oci): clarify removal and host firewall diagnostics --- .../test_oci_removal_firewall_wording.py | 241 ++++++++++++++++++ oci/remote/install_oci.sh | 2 +- oci/remote/oci_remove.py | 6 +- oci/src/proxmenux_oci/management.py | 14 +- 4 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 .github/scripts/tests/test_oci_removal_firewall_wording.py diff --git a/.github/scripts/tests/test_oci_removal_firewall_wording.py b/.github/scripts/tests/test_oci_removal_firewall_wording.py new file mode 100644 index 00000000..6c7b27bd --- /dev/null +++ b/.github/scripts/tests/test_oci_removal_firewall_wording.py @@ -0,0 +1,241 @@ +"""Exercise extracted removal/UI/guard consumers with inert dependencies only.""" +import ast +import argparse +from contextlib import nullcontext +import importlib.util +import json +from pathlib import Path +import re +import runpy +import subprocess +import sys +from types import ModuleType, SimpleNamespace +import unittest +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parents[3] +MENU = ROOT / 'oci/src/proxmenux_oci/management.py' +REMOVE = ROOT / 'oci/remote/oci_remove.py' +INSTALL = ROOT / 'oci/remote/install_oci.sh' +PREVIEW = 'Private network targeted for release if no other guest uses it:' +CONFIRM = ('Remove the application? Its container disks are targeted for deletion; ' + 'recovery from backups is not checked here.') +RESULT = 'Removal command finished; review any warnings above.' +BRIDGE = 'Private network release attempted:' +FIREWALL = 'Could not verify removal of the managed host firewall rule.' +PORT = 'The host-monitor firewall port does not match exactly one TCP port in the container contract' +OPTIONAL = 'A matching managed host firewall rule may also be removed.' +MEMBERS = 'All of them are targeted for removal.' +WHOLE = ('It cannot be removed on its own, because the application would stop ' + 'working: continuing targets the whole application for removal.') +TARGETS = 'Containers targeted for removal:' +DATA = 'Container data targeted for deletion:' +KEYS = (PREVIEW, CONFIRM, RESULT, BRIDGE, FIREWALL, PORT, OPTIONAL, MEMBERS, WHOLE, TARGETS, DATA) + + +def extract(path, name, scope): + node = next(n for n in ast.parse(path.read_text()).body if isinstance(n, ast.FunctionDef) and n.name == name) + exec(compile(ast.Module(body=[node], type_ignores=[]), str(path), 'exec'), scope) + return scope[name] + + +class RemovalWordings(unittest.TestCase): + def preview(self, bridge='vmbr9', translated=None): + instances = ModuleType('oci_instances') + instances.ROOT = Path('/inert') + instances.read = lambda root, vmid: {'installation_id': 'owned'} + remover = ModuleType('oci_remove') + remover.members_of = lambda root, vmid: (101, {'stack': {}}, [102, 101]) + remover.guest_config = lambda member: None + remover.host_directories = lambda root, members: [] + remover.private_bridge = lambda primary: bridge + state = ModuleType('oci_installation_state') + state.parse_config = lambda raw: {} + scope = {'sys': SimpleNamespace(path=[]), 'source_text': lambda value: value or '', + 'translate': translated or (lambda value: value), 're': re} + with patch.dict(sys.modules, {'oci_instances': instances, 'oci_remove': remover, + 'oci_installation_state': state}): + return extract(MENU, '_removal_summary', scope)(Path('/inert'), 101) + + def test_preview_conditions_network_and_discloses_optional_firewall(self): + preview = self.preview() + self.assertIn(PREVIEW + ' vmbr9', preview) + self.assertIn(OPTIONAL, preview) + self.assertIn(MEMBERS, preview) + self.assertIn(TARGETS, preview) + self.assertIn(DATA, preview) + self.assertNotIn('Private network of the application that is released:', preview) + self.assertNotIn(PREVIEW, self.preview(bridge=None)) + + def test_member_preview_does_not_promise_whole_stack_removed(self): + instances = ModuleType('oci_instances') + instances.ROOT = Path('/inert') + instances.read = lambda root, vmid: {'installation_id': 'owned'} + remover = ModuleType('oci_remove') + remover.members_of = lambda root, vmid: (101, {'stack': {}}, [102, 101]) + remover.guest_config = lambda member: None + remover.host_directories = lambda root, members: [] + remover.private_bridge = lambda primary: None + state = ModuleType('oci_installation_state') + state.parse_config = lambda raw: {} + with patch.dict(sys.modules, {'oci_instances': instances, 'oci_remove': remover, + 'oci_installation_state': state}): + scope = {'sys': SimpleNamespace(path=[]), 'source_text': lambda value: value or '', + 'translate': lambda value: value, 're': re} + preview = extract(MENU, '_removal_summary', scope)(Path('/inert'), 102) + self.assertIn(WHOLE, preview) + + def test_cancellation_preserves_lifecycle_boundary_and_warns_precisely(self): + calls = [] + ui = SimpleNamespace(review=lambda message, title, **kw: calls.append((message, kw)) or False, + message=lambda *args: self.fail('unexpected error')) + scope = {'_removal_summary': lambda project, vmid: 'Preview', + '_run_lifecycle': lambda *args: self.fail('must not run'), + 'translate': lambda value: value, 'sys': SimpleNamespace(executable='python3'), + 'Path': Path, 'OSError': OSError, 'ValueError': ValueError, 'KeyError': KeyError} + self.assertFalse(extract(MENU, '_remove', scope)(Path('/inert'), ui, 101)) + self.assertEqual(calls, [('Preview', {'question': CONFIRM, 'default': False})]) + + def test_shared_bridge_and_skipped_member_are_not_reported_removed(self): + events = [] + records = {102: {'installation_id': 'owned'}, 101: {'installation_id': 'owned'}} + scope = {'members_of': lambda root, vmid: (101, records[101], [102, 101]), + 'instances': SimpleNamespace(read=lambda root, vmid: records[vmid], + identity=lambda raw: 'reassigned' if raw == b'reassigned' else 'owned', + location=lambda root, vmid: Path('/inert/absent/record.json')), + 'guest_config': lambda vmid: b'reassigned' if vmid == 102 else b'owned', + 'host_directories': lambda root, members: [], 'private_bridge': lambda primary: 'vmbr9', + 'bridge_in_use': lambda bridge, removed: True, + 'release_bridge': lambda bridge: self.fail('shared bridge release'), + 'remove_owned_host_firewall': lambda record: None, + 'run': lambda *args: events.append(('run', args)), + 'subprocess': SimpleNamespace(run=lambda *args, **kwargs: None), + 'Path': Path, 'shutil': SimpleNamespace(rmtree=lambda path: None), + 'image_cache': SimpleNamespace(prune=lambda root, lock: []), + 'oci_console': SimpleNamespace(remove_log=lambda vmid: None), + 'translate': lambda text: text, + 'msg_info': lambda text: events.append(('info', text)), + 'msg_ok': lambda text: events.append(('ok', text)), + 'msg_warn': lambda text: events.append(('warn', text))} + remove = extract(REMOVE, 'remove', scope) + for skipped_config in (b'reassigned', None): + with self.subTest(skipped_config=skipped_config): + scope['guest_config'] = lambda vmid: skipped_config if vmid == 102 else b'owned' + events.clear() + remove(Path('/inert'), 101) + self.assertEqual([x for x in events if x[0] == 'run'], + [('run', ('pct', 'destroy', '101', '--purge', '1', '--destroy-unreferenced-disks', '1'))]) + self.assertFalse(any(x == ('ok', 'The application was removed') for x in events)) + self.assertTrue(any('still used' in x[1] for x in events if x[0] == 'warn')) + self.assertTrue(any('no longer exists' in x[1] if skipped_config is None + else 'belongs to another container' in x[1] + for x in events if x[0] == 'warn')) + # The actual main() success line is independently guarded by the expected neutral literal. + tree = ast.parse(REMOVE.read_text()) + main = next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == 'main') + self.assertIn(RESULT, [n.value for n in ast.walk(main) if isinstance(n, ast.Constant) and isinstance(n.value, str)]) + + def test_actual_main_success_is_completion_not_all_members_removed(self): + events = [] + scope = {'argparse': argparse, 'Path': Path, 'instances': SimpleNamespace(ROOT=Path('/inert'), + locked=lambda root: nullcontext()), + 'os': SimpleNamespace(geteuid=lambda: 0), + 'sys': SimpleNamespace(argv=['oci_remove.py', '101']), + 'remove': lambda root, vmid: events.append(('warn', 'Skipped CT 102')), + 'translate': lambda text: text, + 'msg_error': lambda text: events.append(('error', text)), + 'msg_ok': lambda text: events.append(('ok', text)), + 'subprocess': subprocess} + with patch.object(sys, 'argv', ['oci_remove.py', '101']): + self.assertEqual(extract(REMOVE, 'main', scope)(), 0) + self.assertEqual(events, [('warn', 'Skipped CT 102'), ('ok', RESULT)]) + + def test_ignored_bridge_command_failures_do_not_claim_release(self): + events = [] + scope = {'members_of': lambda root, vmid: (101, {}, [101]), + 'instances': SimpleNamespace(read=lambda root, vmid: {'installation_id': 'owned'}, + identity=lambda cfg: 'owned', location=lambda root, vmid: Path('/inert/absent/record.json')), + 'guest_config': lambda vmid: b'owned', 'host_directories': lambda *args: [], + 'private_bridge': lambda primary: 'vmbr9', 'bridge_in_use': lambda *args: False, + 'release_bridge': lambda bridge: events.append(('attempt', bridge)), + 'remove_owned_host_firewall': lambda record: None, 'run': lambda *args: None, + 'subprocess': SimpleNamespace(run=lambda *args, **kwargs: None), + 'Path': Path, 'shutil': SimpleNamespace(rmtree=lambda path: None), + 'image_cache': SimpleNamespace(prune=lambda root, lock: []), + 'oci_console': SimpleNamespace(remove_log=lambda vmid: None), + 'translate': lambda text: text, 'msg_info': lambda text: None, + 'msg_ok': lambda text: events.append(('ok', text)), 'msg_warn': lambda text: None} + extract(REMOVE, 'remove', scope)(Path('/inert'), 101) + self.assertIn(('attempt', 'vmbr9'), events) + self.assertIn(('ok', BRIDGE + ' vmbr9'), events) + self.assertNotIn(('ok', 'Private network of the application released: vmbr9'), events) + + def test_firewall_delete_exception_has_unknown_outcome_not_unchanged_rule(self): + events = [] + installation_id = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' + commands = [] + def fake_run(args, **kwargs): + commands.append(args) + if args[1] == 'get': + return SimpleNamespace(stdout=json.dumps([{'comment': 'ProxMenux OCI firewall ' + installation_id, + 'dport': '8080', 'source': '192.0.2.0/24', 'proto': 'tcp', 'type': 'in', + 'action': 'ACCEPT', 'pos': 2}])) + raise subprocess.CalledProcessError(1, args) + scope = {'re': re, 'socket': SimpleNamespace(gethostname=lambda: 'node'), 'json': json, + 'subprocess': SimpleNamespace(run=fake_run, CalledProcessError=subprocess.CalledProcessError), + 'translate': lambda text: text, 'msg_ok': lambda text: events.append(('ok', text)), + 'msg_warn': lambda text: events.append(('warn', text)), 'OSError': OSError, 'ValueError': ValueError} + extract(REMOVE, 'remove_owned_host_firewall', scope)({'installation_id': installation_id, + 'deployment': {'host_firewall': {'port': 8080, 'source': '192.0.2.0/24'}}}) + self.assertEqual(commands[-1][1], 'delete') + self.assertEqual(events, [('warn', FIREWALL)]) + + def test_extractor_and_runtime_lookup_fallback_and_synthetic_translation(self): + generator = runpy.run_path(str(ROOT / '.github/scripts/build_translation_cache.py')) + found = set(generator['extract_python_texts']([ROOT / 'oci/src', ROOT / 'oci/remote'])) + shell_keys = set(generator['extract_translate_texts'](ROOT / 'oci/remote')) + self.assertTrue(set(KEYS) - {PORT} <= found, (set(KEYS) - {PORT}) - found) + self.assertIn(PORT, shell_keys) + spec = importlib.util.spec_from_file_location('oci_i18n', ROOT / 'oci/src/proxmenux_oci/i18n.py') + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + for path in sorted((ROOT / 'lang').glob('*.json')): + cache = json.loads(path.read_text()) + module._language, module._cache = path.stem, cache + for key in KEYS: + self.assertEqual(module.translate(key), cache.get(key) or key) + module._cache = {} + for key in KEYS: + self.assertEqual(module.translate(key), key) + module._language, module._cache = 'it', {key: 'IT: ' + key for key in KEYS} + self.assertIn('IT: ' + PREVIEW, self.preview(translated=module.translate)) + self.assertIn('IT: ' + OPTIONAL, self.preview(translated=module.translate)) + + def test_actual_shell_guard_fails_for_missing_tcp_contract_not_web_role(self): + source = INSTALL.read_text() + function = source[source.index('validate_host_monitor_firewall() {'):source.index('\napply_host_monitor_firewall() {')] + script = '''set -eu +translate() { printf '%s' "$1"; } +die() { printf '%s' "$1" >&2; exit 7; } +jq() { + case "$*" in + *'.host_firewall == null'*) printf 'false\\n' ;; + *'.host_firewall | type'*) printf 'true\\n' ;; + *'.host_firewall.bridge'*) printf 'vmbr0\\n' ;; + *'.host_firewall.source'*) printf '192.0.2.0/24\\n' ;; + *'.host_firewall.port'*) printf '8080\\n' ;; + *'.proxmox.installer_profile.host_monitor_firewall.web_port'*) printf '8080\\n' ;; + *'.container_contract.ports'*) printf '0\\n' ;; + *) exit 99 ;; + esac +} +ip() { printf '2: vmbr0 inet 192.0.2.1/24 scope global vmbr0\\n'; } +BRIDGE=vmbr0 HOST_MONITOR=netdata DEPLOYMENT_FILE=/inert/deployment TEMPLATE_FILE=/inert/template +''' + function + '\nvalidate_host_monitor_firewall\n' + result = subprocess.run(['bash', '-c', script], text=True, capture_output=True, check=False) + self.assertEqual(result.returncode, 7, result.stderr) + self.assertEqual(result.stderr, PORT) + + +if __name__ == '__main__': + unittest.main() diff --git a/oci/remote/install_oci.sh b/oci/remote/install_oci.sh index da3d61d2..7eed9a11 100755 --- a/oci/remote/install_oci.sh +++ b/oci/remote/install_oci.sh @@ -244,7 +244,7 @@ PY contract_count=$(jq --argjson port "$HOST_FIREWALL_PORT" '[.container_contract.ports[]? | select( .protocol == "tcp" and .container_port == $port)] | length' "$TEMPLATE_FILE") [[ $contract_count == 1 ]] \ - || die "$(translate "The host-monitor firewall port is not declared as the web port")" + || die "$(translate "The host-monitor firewall port does not match exactly one TCP port in the container contract")" HOST_FIREWALL_ENABLED=1 } diff --git a/oci/remote/oci_remove.py b/oci/remote/oci_remove.py index 33fa9f30..2df8011a 100644 --- a/oci/remote/oci_remove.py +++ b/oci/remote/oci_remove.py @@ -138,7 +138,7 @@ def remove_owned_host_firewall(record): except (OSError, ValueError, subprocess.CalledProcessError, json.JSONDecodeError): # Removal already destroyed the CT. A firewall API failure must not # turn that successful lifecycle operation into a failed one. - msg_warn(translate('The managed host firewall rule could not be removed and was left unchanged.')) + msg_warn(translate('Could not verify removal of the managed host firewall rule.')) def remove(root, vmid): @@ -166,7 +166,7 @@ def remove(root, vmid): msg_ok(f"{translate('Container removed:')} CT {member}") if bridge and not bridge_in_use(bridge, set(members)): release_bridge(bridge) - msg_ok(f"{translate('Private network of the application released:')} {bridge}") + msg_ok(f"{translate('Private network release attempted:')} {bridge}") elif bridge: msg_warn(f"{translate('The private network is still used by another container and is kept:')} {bridge}") remove_owned_host_firewall(primary) @@ -200,7 +200,7 @@ def main(): except (OSError, ValueError, KeyError, RuntimeError, subprocess.CalledProcessError) as error: msg_error(str(error) or type(error).__name__) return 1 - msg_ok(translate('The application was removed')) + msg_ok(translate('Removal command finished; review any warnings above.')) return 0 diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index 23ad7498..e7bcb1c4 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/oci/src/proxmenux_oci/management.py @@ -308,16 +308,17 @@ def _removal_summary(project, vmid): text = [] if len(members) > 1 and vmid == primary_id: text += [f"{application} {translate('runs in')} {len(members)} {translate('containers')}. " - f"{translate('All of them are removed.')}", ''] + f"{translate('All of them are targeted for removal.')}", ''] elif len(members) > 1: alone = translate('It cannot be removed on its own, because the application would stop ' - 'working: continuing removes the whole application.') + 'working: continuing targets the whole application for removal.') text += [f"CT {vmid} {translate('is one of the')} {len(members)} " f"{translate('containers of')} {application}. {alone}", ''] - text += [translate('Containers that are removed:'), *lines, '', - translate('Data that is deleted with them:'), *volumes] + text += [translate('Containers targeted for removal:'), *lines, '', + translate('Container data targeted for deletion:'), *volumes] if bridge: - text += ['', f"{translate('Private network of the application that is released:')} {bridge}"] + text += ['', f"{translate('Private network targeted for release if no other guest uses it:')} {bridge}"] + text += ['', translate('A matching managed host firewall rule may also be removed.')] if kept: text += ['', translate('Host paths found in container configs or saved records (not targeted for removal):'), *[f' {path}' for path in kept]] @@ -333,7 +334,8 @@ def _remove(project, ui, vmid): ui.message(f"{translate('The removal could not be prepared:')} {error}", translate('Remove OCI')) return False if not ui.review(summary, translate('Remove OCI'), - question=translate('Remove it? The data of its containers cannot be recovered afterwards.'), + question=translate('Remove the application? Its container disks are targeted for deletion; ' + 'recovery from backups is not checked here.'), default=False): return False return _run_lifecycle([sys.executable, str(project / 'remote/oci_remove.py'), str(vmid)],