From 118a757ac74836854fe13d6478af72147264cf50 Mon Sep 17 00:00:00 2001 From: martino <32328813+f3rs3n@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:48:34 +0200 Subject: [PATCH] fix(oci): distinguish clean removal from incomplete cleanup --- .../test_oci_recovery_inventory_wording.py | 6 +- .../test_oci_removal_firewall_wording.py | 163 ++++++++++++++++-- oci/remote/oci_remove.py | 50 ++++-- oci/src/proxmenux_oci/management.py | 7 +- 4 files changed, 190 insertions(+), 36 deletions(-) diff --git a/.github/scripts/tests/test_oci_recovery_inventory_wording.py b/.github/scripts/tests/test_oci_recovery_inventory_wording.py index 122f5ead..b4cbce13 100644 --- a/.github/scripts/tests/test_oci_recovery_inventory_wording.py +++ b/.github/scripts/tests/test_oci_recovery_inventory_wording.py @@ -74,7 +74,7 @@ class InventoryMessages(unittest.TestCase): 'guest_config': lambda vmid: b'description: owned', 'host_directories': lambda root, members: ['/bind/saved'], 'private_bridge': lambda primary: None, - 'remove_owned_host_firewall': lambda primary: None, + 'remove_owned_host_firewall': lambda primary: True, 'run': lambda *args: events.append(('run', args)), 'subprocess': SimpleNamespace(run=lambda *args, **kwargs: None), 'Path': Path, 'shutil': SimpleNamespace(rmtree=lambda path: None), @@ -86,13 +86,13 @@ class InventoryMessages(unittest.TestCase): 'msg_warn': lambda text: events.append(('warn', text))} remove = extracted(REMOVE, 'remove', scope) remove(Path('/inert'), 101) - self.assertIn(('warn', POST_HOST + ' /bind/saved'), events) + self.assertIn(('info', POST_HOST + ' /bind/saved'), events) self.assertIn(('run', ('pct', 'destroy', '101', '--purge', '1', '--destroy-unreferenced-disks', '1')), events) self.assertIn(('log', 101), events) scope['translate'] = lambda text: 'Tradotto: ' + text if text == POST_HOST else text events.clear() remove(Path('/inert'), 101) - self.assertIn(('warn', 'Tradotto: ' + POST_HOST + ' /bind/saved'), events) + self.assertIn(('info', 'Tradotto: ' + POST_HOST + ' /bind/saved'), events) class RecoveryMessages(unittest.TestCase): diff --git a/.github/scripts/tests/test_oci_removal_firewall_wording.py b/.github/scripts/tests/test_oci_removal_firewall_wording.py index 6c7b27bd..2ab3e802 100644 --- a/.github/scripts/tests/test_oci_removal_firewall_wording.py +++ b/.github/scripts/tests/test_oci_removal_firewall_wording.py @@ -18,10 +18,11 @@ 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.') +CONFIRM = 'Remove the application? Its container disks are deleted, and only a backup can bring them back.' RESULT = 'Removal command finished; review any warnings above.' BRIDGE = 'Private network release attempted:' +BRIDGE_FAILURE = 'Could not complete private network release:' +SUCCESS = 'The application was removed' 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.' @@ -30,7 +31,7 @@ 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) +KEYS = (PREVIEW, CONFIRM, RESULT, BRIDGE, BRIDGE_FAILURE, SUCCESS, FIREWALL, PORT, OPTIONAL, MEMBERS, WHOLE, TARGETS, DATA) def extract(path, name, scope): @@ -40,12 +41,13 @@ def extract(path, name, scope): class RemovalWordings(unittest.TestCase): - def preview(self, bridge='vmbr9', translated=None): + def preview(self, bridge='vmbr9', translated=None, firewall=True): 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.members_of = lambda root, vmid: (101, {'stack': {}, 'deployment': + {'host_firewall': {'port': 8080}} if firewall else {}}, [102, 101]) remover.guest_config = lambda member: None remover.host_directories = lambda root, members: [] remover.private_bridge = lambda primary: bridge @@ -66,6 +68,7 @@ class RemovalWordings(unittest.TestCase): self.assertIn(DATA, preview) self.assertNotIn('Private network of the application that is released:', preview) self.assertNotIn(PREVIEW, self.preview(bridge=None)) + self.assertNotIn(OPTIONAL, self.preview(firewall=False)) def test_member_preview_does_not_promise_whole_stack_removed(self): instances = ModuleType('oci_instances') @@ -110,7 +113,8 @@ class RemovalWordings(unittest.TestCase): '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), + 'Path': lambda value: Path('/inert/no-lifecycle') if str(value).startswith('/etc/pve/') else Path(value), + '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, @@ -126,7 +130,7 @@ class RemovalWordings(unittest.TestCase): 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('still used' in x[1] for x in events if x[0] == 'info')) 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')) @@ -136,19 +140,113 @@ class RemovalWordings(unittest.TestCase): 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')), + events = self.lifecycle_events(missing=True) + self.assertIn(('warn', 'The container no longer exists: CT 102'), events) + self.assertEqual(events[-1], ('ok', RESULT)) + + def lifecycle_events(self, *, missing=False, reassigned=False, bridge='none', + ip_rc=0, pvesh_rc=0, ip_error=False, firewall_failure=False, + firewall_success=False, kept=False): + events, commands = [], [] + self.last_commands = commands + record = {'installation_id': 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'} + if firewall_failure or firewall_success: + record['deployment'] = {'host_firewall': {'source': '192.0.2.0/24', 'port': 8080}} + records = {101: record, 102: {'installation_id': 'owned'}} + + def fake_command(args, **kwargs): + commands.append(tuple(args)) + if ip_error and args[:2] == ['ip', 'link']: + raise OSError('ip unavailable') + if args[:2] == ['pvesh', 'get']: + return SimpleNamespace(stdout=json.dumps([{'comment': 'ProxMenux OCI firewall ' + record['installation_id'], + 'dport': '8080', 'source': '192.0.2.0/24', 'proto': 'tcp', 'type': 'in', + 'action': 'ACCEPT', 'pos': 2}]), returncode=0) + if args[:2] == ['pvesh', 'delete'] and '/firewall/' in args[2] and firewall_failure: + raise subprocess.CalledProcessError(1, args) + return SimpleNamespace(returncode=ip_rc if args[:2] == ['ip', 'link'] else + pvesh_rc if args[:2] == ['pvesh', 'delete'] else 0) + + fake_subprocess = SimpleNamespace(run=fake_command, CalledProcessError=subprocess.CalledProcessError) + scope = {'members_of': lambda root, vmid: (101, record, [102, 101]), + 'instances': SimpleNamespace(ROOT=Path('/inert'), locked=lambda root: nullcontext(), + read=lambda root, vmid: records[vmid], + identity=lambda cfg: record['installation_id'] if cfg == b'primary' else + 'other' if cfg == b'reassigned' else 'owned', + location=lambda root, vmid: Path('/inert/absent/record.json')), + 'guest_config': lambda vmid: (None if missing else b'reassigned' if reassigned else b'owned') + if vmid == 102 else b'primary', + 'host_directories': lambda *args: ['/retained'] if kept else [], + 'private_bridge': lambda primary: 'vmbr9' if bridge != 'none' else None, + 'bridge_in_use': lambda *args: bridge == 'shared', + 'run': lambda *args: commands.append(args), 'subprocess': fake_subprocess, + 'socket': SimpleNamespace(gethostname=lambda: 'node'), 'json': json, 're': re, + 'Path': lambda value: Path('/inert/no-lifecycle') if str(value).startswith('/etc/pve/') else Path(value), + '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_error': lambda text: events.append(('error', text)), + 'msg_info': lambda text: events.append(('info', text)), 'msg_ok': lambda text: events.append(('ok', text)), - 'subprocess': subprocess} + 'msg_warn': lambda text: events.append(('warn', text)), + 'msg_error': lambda text: events.append(('error', text)), + 'argparse': argparse, 'os': SimpleNamespace(geteuid=lambda: 0), + 'sys': SimpleNamespace(argv=['oci_remove.py', '101'])} + scope['release_bridge'] = extract(REMOVE, 'release_bridge', scope) + scope['remove_owned_host_firewall'] = extract(REMOVE, 'remove_owned_host_firewall', scope) + scope['remove'] = extract(REMOVE, 'remove', scope) 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)]) + return events + + def test_clean_removal_has_clear_success(self): + events = self.lifecycle_events() + self.assertEqual(events[-1], ('ok', 'The application was removed')) + self.assertFalse(any(kind == 'warn' for kind, _ in events)) + + def test_skipped_identity_and_kept_bridge_have_partial_result(self): + events = self.lifecycle_events(reassigned=True, bridge='shared') + self.assertIn(('warn', 'The VMID belongs to another container now and is not touched: CT 102'), events) + self.assertEqual(events[-1], ('ok', RESULT)) + + def test_shared_bridge_intentionally_kept_does_not_taint_clean_removal(self): + events = self.lifecycle_events(bridge='shared') + self.assertTrue(any('still used' in text for kind, text in events if kind == 'info')) + self.assertEqual(events[-1], ('ok', 'The application was removed')) + + def test_failed_bridge_commands_have_partial_result(self): + for ip_rc, pvesh_rc in ((1, 0), (0, 1), (1, 1)): + with self.subTest(ip_rc=ip_rc, pvesh_rc=pvesh_rc): + events = self.lifecycle_events(bridge='private', ip_rc=ip_rc, pvesh_rc=pvesh_rc) + self.assertTrue(any(kind == 'warn' and 'private network' in text.lower() + for kind, text in events), events) + self.assertEqual(events[-1], ('ok', RESULT)) + + def test_successful_bridge_release_and_retained_paths_are_clean(self): + events = self.lifecycle_events(bridge='private', kept=True) + self.assertIn(('ok', BRIDGE + ' vmbr9'), events) + self.assertTrue(any(kind == 'info' and '/retained' in text for kind, text in events)) + self.assertFalse(any(kind == 'warn' for kind, _ in events)) + self.assertEqual(events[-1], ('ok', SUCCESS)) + + def test_bridge_exception_is_reported_but_cleanup_continues(self): + events = self.lifecycle_events(bridge='private', ip_error=True) + self.assertEqual([cmd[:2] for cmd in self.last_commands], + [('pct', 'stop'), ('pct', 'destroy'), ('pct', 'stop'), + ('pct', 'destroy'), ('ip', 'link'), ('pvesh', 'delete')]) + self.assertTrue(any(kind == 'warn' and 'private network' in text.lower() + for kind, text in events), events) + self.assertEqual(events[-1], ('ok', RESULT)) + + def test_successful_firewall_delete_is_clean(self): + events = self.lifecycle_events(firewall_success=True) + self.assertTrue(any('Host firewall rule removed:' in text for kind, text in events if kind == 'ok')) + self.assertEqual(events[-1], ('ok', SUCCESS)) + + def test_firewall_delete_failure_has_partial_result(self): + events = self.lifecycle_events(firewall_failure=True) + self.assertIn(('warn', FIREWALL), events) + self.assertEqual(events[-1], ('ok', RESULT)) def test_ignored_bridge_command_failures_do_not_claim_release(self): events = [] @@ -160,7 +258,8 @@ class RemovalWordings(unittest.TestCase): '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), + 'Path': lambda value: Path('/inert/no-lifecycle') if str(value).startswith('/etc/pve/') else Path(value), + '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, @@ -170,6 +269,36 @@ class RemovalWordings(unittest.TestCase): self.assertIn(('ok', BRIDGE + ' vmbr9'), events) self.assertNotIn(('ok', 'Private network of the application released: vmbr9'), events) + def test_invalid_managed_firewall_metadata_warns_instead_of_silent_clean(self): + events = [] + scope = {'re': re, 'socket': SimpleNamespace(gethostname=lambda: 'node'), 'json': json, + 'subprocess': subprocess, 'translate': lambda text: text, + 'msg_ok': lambda text: events.append(('ok', text)), + 'msg_warn': lambda text: events.append(('warn', text))} + result = extract(REMOVE, 'remove_owned_host_firewall', scope)( + {'installation_id': 'invalid', 'deployment': {'host_firewall': {'source': '192.0.2.0/24', 'port': 8080}}}) + self.assertFalse(result) + self.assertEqual(events, [('warn', FIREWALL)]) + + def test_ambiguous_firewall_match_warns_without_deleting_other_rules(self): + events, commands = [], [] + installation_id = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' + rule = {'comment': 'ProxMenux OCI firewall ' + installation_id, 'dport': '8080', + 'source': '192.0.2.0/24', 'proto': 'tcp', 'type': 'in', 'action': 'ACCEPT', 'pos': 2} + def fake_run(args, **kwargs): + commands.append(args) + return SimpleNamespace(stdout=json.dumps([rule, {**rule, 'pos': 3}])) + 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))} + result = extract(REMOVE, 'remove_owned_host_firewall', scope)( + {'installation_id': installation_id, + 'deployment': {'host_firewall': {'source': '192.0.2.0/24', 'port': 8080}}}) + self.assertFalse(result) + self.assertEqual(events, [('warn', FIREWALL)]) + self.assertEqual([command[1] for command in commands], ['get']) + def test_firewall_delete_exception_has_unknown_outcome_not_unchanged_rule(self): events = [] installation_id = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' diff --git a/oci/remote/oci_remove.py b/oci/remote/oci_remove.py index 2df8011a..653fb597 100644 --- a/oci/remote/oci_remove.py +++ b/oci/remote/oci_remove.py @@ -100,8 +100,14 @@ def bridge_in_use(bridge, removed): def release_bridge(bridge): node = socket.gethostname().split('.', 1)[0] - subprocess.run(['ip', 'link', 'delete', bridge, 'type', 'bridge'], check=False, capture_output=True) - subprocess.run(['pvesh', 'delete', f'/nodes/{node}/network/{bridge}'], check=False, capture_output=True) + succeeded = [] + for command in (['ip', 'link', 'delete', bridge, 'type', 'bridge'], + ['pvesh', 'delete', f'/nodes/{node}/network/{bridge}']): + try: + succeeded.append(subprocess.run(command, check=False, capture_output=True).returncode == 0) + except (OSError, subprocess.CalledProcessError): + succeeded.append(False) + return all(succeeded) def remove_owned_host_firewall(record): @@ -112,12 +118,16 @@ def remove_owned_host_firewall(record): deliberately left alone as well. """ plan = record.get('deployment', {}).get('host_firewall') or {} + if not plan: + return True installation_id = record.get('installation_id', '') if not isinstance(plan, dict) or not re.fullmatch(r'[0-9a-f-]{36}', installation_id): - return + msg_warn(translate('Could not verify removal of the managed host firewall rule.')) + return False source, port = plan.get('source'), plan.get('port') if not isinstance(source, str) or not isinstance(port, int): - return + msg_warn(translate('Could not verify removal of the managed host firewall rule.')) + return False comment = f'ProxMenux OCI firewall {installation_id}' node = socket.gethostname().split('.', 1)[0] try: @@ -130,15 +140,20 @@ def remove_owned_host_firewall(record): and str(rule.get('proto', '')).lower() == 'tcp' and str(rule.get('type', '')).lower() == 'in' and str(rule.get('action', '')).upper() == 'ACCEPT'] + if not matches: + return True if len(matches) != 1 or not isinstance(matches[0].get('pos'), int): - return + msg_warn(translate('Could not verify removal of the managed host firewall rule.')) + return False subprocess.run(['pvesh', 'delete', f"/nodes/{node}/firewall/rules/{matches[0]['pos']}"], check=True, capture_output=True) msg_ok(f"{translate('Host firewall rule removed:')} TCP {port} {translate('from')} {source}") + return True 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. + # A firewall API failure must not abort remaining record cleanup; + # report the unverified outcome without claiming the rule survived. msg_warn(translate('Could not verify removal of the managed host firewall rule.')) + return False def remove(root, vmid): @@ -150,26 +165,33 @@ def remove(root, vmid): 'recover it from the management menu before removing it')) kept = host_directories(root, members) bridge = private_bridge(primary) + incomplete = False msg_info(translate('Removing the containers...')) for member in members: record = instances.read(root, member) config = guest_config(member) if config is None: msg_warn(f"{translate('The container no longer exists:')} CT {member}") + incomplete = True oci_console.remove_log(member) elif instances.identity(config) != record['installation_id']: msg_warn(f"{translate('The VMID belongs to another container now and is not touched:')} CT {member}") + incomplete = True else: subprocess.run(['pct', 'stop', str(member), '--skiplock', '1'], check=False, capture_output=True) run('pct', 'destroy', str(member), '--purge', '1', '--destroy-unreferenced-disks', '1') oci_console.remove_log(member) msg_ok(f"{translate('Container removed:')} CT {member}") if bridge and not bridge_in_use(bridge, set(members)): - release_bridge(bridge) + released = release_bridge(bridge) msg_ok(f"{translate('Private network release attempted:')} {bridge}") + if not released: + msg_warn(f"{translate('Could not complete private network release:')} {bridge}") + incomplete = True elif bridge: - msg_warn(f"{translate('The private network is still used by another container and is kept:')} {bridge}") - remove_owned_host_firewall(primary) + msg_info(f"{translate('The private network is still used by another container and is kept:')} {bridge}") + if not remove_owned_host_firewall(primary): + incomplete = True lifecycle = Path(f'/etc/pve/priv/proxmenux-stack-{primary_id}.json') if lifecycle.exists() and not lifecycle.is_symlink(): lifecycle.unlink() @@ -181,7 +203,8 @@ def remove(root, vmid): for path, size in image_cache.prune(root, lock=False): msg_ok(f"{translate('Unused image removed from the cache:')} {path.name}") for path in kept: - msg_warn(f"{translate('Host directory listed in saved records (not targeted for removal):')} {path}") + msg_info(f"{translate('Host directory listed in saved records (not targeted for removal):')} {path}") + return incomplete def main(): @@ -193,14 +216,15 @@ def main(): parser.error(translate('Root privileges are required')) try: with instances.locked(args.root): - remove(args.root, args.vmid) + incomplete = remove(args.root, args.vmid) except BlockingIOError: msg_error(translate('Another OCI operation is using the instance registry')) return 1 except (OSError, ValueError, KeyError, RuntimeError, subprocess.CalledProcessError) as error: msg_error(str(error) or type(error).__name__) return 1 - msg_ok(translate('Removal command finished; review any warnings above.')) + msg_ok(translate('Removal command finished; review any warnings above.') if incomplete + else translate('The application was removed')) return 0 diff --git a/oci/src/proxmenux_oci/management.py b/oci/src/proxmenux_oci/management.py index e7bcb1c4..879029fa 100644 --- a/oci/src/proxmenux_oci/management.py +++ b/oci/src/proxmenux_oci/management.py @@ -318,7 +318,8 @@ def _removal_summary(project, vmid): translate('Container data targeted for deletion:'), *volumes] if 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 (primary.get('deployment') or {}).get('host_firewall'): + 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]] @@ -334,8 +335,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 the application? Its container disks are targeted for deletion; ' - 'recovery from backups is not checked here.'), + question=translate('Remove the application? Its container disks are deleted, ' + 'and only a backup can bring them back.'), default=False): return False return _run_lifecycle([sys.executable, str(project / 'remote/oci_remove.py'), str(vmid)],