Merge pull request #395 from Vaso73/fix/oci-firewall-lifecycle

fix(oci): complete host firewall lifecycle follow-ups

.github/scripts/tests/test_oci_recovery_inventory_wording.py

@@ -74,6 +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,
                  'run': lambda *args: events.append(('run', args)),
                  'subprocess': SimpleNamespace(run=lambda *args, **kwargs: None),
                  'Path': Path, 'shutil': SimpleNamespace(rmtree=lambda path: None),

lang/de.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Hostverzeichnis:",
   "Host firewall": "Host-Firewall",
   "Host firewall already allows:": "Die Host-Firewall erlaubt bereits:",
+  "Host firewall rule removed:": "Host-Firewallregel entfernt:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "Die verwaltete Host-Firewallregel konnte nicht entfernt werden und blieb unverändert.",
   "Host firewall rule added:": "Host-Firewall-Regel hinzugefügt:",
   "Host fstab CIFS Mounts:": "Host fstab CIFS-Mounts:",
   "Host fstab CIFS mounts (not registered with pvesm):": "Host fstab CIFS-Mounts (nicht bei pvesm registriert):",

lang/es.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Directorio del host:",
   "Host firewall": "Cortafuegos del host",
   "Host firewall already allows:": "El cortafuegos del host ya permite:",
+  "Host firewall rule removed:": "Regla del cortafuegos del host eliminada:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "No se pudo eliminar la regla administrada del cortafuegos del host y se dejó sin cambios.",
   "Host firewall rule added:": "Regla añadida al cortafuegos del host:",
   "Host fstab CIFS Mounts:": "Montajes CIFS del host fstab:",
   "Host fstab CIFS mounts (not registered with pvesm):": "Montajes CIFS del host fstab (no registrados con pvesm):",

lang/fr.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Répertoire hôte :",
   "Host firewall": "Pare-feu hôte",
   "Host firewall already allows:": "Le pare-feu hôte permet déjà :",
+  "Host firewall rule removed:": "Règle du pare-feu hôte supprimée :",
+  "The managed host firewall rule could not be removed and was left unchanged.": "La règle gérée du pare-feu hôte n’a pas pu être supprimée et est restée inchangée.",
   "Host firewall rule added:": "Règle de pare-feu hôte ajoutée :",
   "Host fstab CIFS Mounts:": "Hôte des montages fstab CIFS :",
   "Host fstab CIFS mounts (not registered with pvesm):": "Hôte des montages fstab CIFS (non enregistrés auprès de pvesm) :",

lang/it.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Directory host:",
   "Host firewall": "firewall host",
   "Host firewall already allows:": "il firewall host consente già:",
+  "Host firewall rule removed:": "Regola del firewall host rimossa:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "Non è stato possibile rimuovere la regola gestita del firewall host ed è rimasta invariata.",
   "Host firewall rule added:": "Aggiunta regola del firewall host:",
   "Host fstab CIFS Mounts:": "Montaggi CIFS host fstab:",
   "Host fstab CIFS mounts (not registered with pvesm):": "Montaggi CIFS host fstab (non registrato con pvesm):",

lang/pt.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Diretório de host:",
   "Host firewall": "Firewall de host",
   "Host firewall already allows:": "O firewall do host já permite:",
+  "Host firewall rule removed:": "Regra do firewall do host removida:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "Não foi possível remover a regra gerida do firewall do host, que permaneceu inalterada.",
   "Host firewall rule added:": "regra de firewall do host adicionada:",
   "Host fstab CIFS Mounts:": "Host fstab montagens CIFS:",
   "Host fstab CIFS mounts (not registered with pvesm):": "Host fstab montagens CIFS (não registradas no pvesm):",

lang/sk.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Priečinok hosta:",
   "Host firewall": "Firewall hostiteľa",
   "Host firewall already allows:": "Firewall hostiteľa už povoľuje:",
+  "Host firewall rule removed:": "Pravidlo firewallu hostiteľa odstránené:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "Spravované pravidlo firewallu hostiteľa sa nepodarilo odstrániť a zostalo bez zmeny.",
   "Host firewall rule added:": "Pravidlo firewallu hostiteľa bolo pridané:",
   "Host fstab CIFS Mounts:": "CIFS mounty v host fstab:",
   "Host fstab CIFS mounts (not registered with pvesm):": "CIFS mounty v host fstab (nie sú registrované cez pvesm):",

lang/sv.json

@@ -2595,6 +2595,8 @@
   "Host directory:": "Värdkatalog:",
   "Host firewall": "Värdbrandvägg",
   "Host firewall already allows:": "Värdbrandväggen tillåter redan:",
+  "Host firewall rule removed:": "Värdbrandväggsregel borttagen:",
+  "The managed host firewall rule could not be removed and was left unchanged.": "Den hanterade värdbrandväggsregeln kunde inte tas bort och lämnades oförändrad.",
   "Host firewall rule added:": "Värdbrandväggsregel har lagts till:",
   "Host fstab CIFS Mounts:": "Värdens CIFS-monteringar i fstab:",
   "Host fstab CIFS mounts (not registered with pvesm):": "Värdens CIFS-monteringar i fstab (inte registrerade med pvesm):",

oci/catalog/overlays/netdata.json

@@ -81,6 +81,10 @@
     },
     "installer_profile": {
       "host_monitor": "netdata",
+      "host_monitor_firewall": {
+        "protocol": "tcp",
+        "web_port": 19999
+      },
       "host_monitor_mounts": [
         {
           "source": "/proc",

oci/remote/install_oci.sh

@@ -259,7 +259,9 @@ apply_host_monitor_firewall() {
   fi
   local node comment rules existing
   node=$(hostname)
-  comment="ProxMenux OCI host monitor CT ${VMID}"
+  # This UUID makes removal safe even after a VMID is reused.  Only a rule
+  # carrying this exact marker belongs to this installation.
+  comment="ProxMenux OCI firewall ${INSTANCE_ID}"
   rules=$(pvesh get "/nodes/${node}/firewall/rules" --output-format json) \
     || die "$(translate "Could not read the host firewall rules")"
   existing=$(jq -r --arg source "$HOST_FIREWALL_SOURCE" --arg port "$HOST_FIREWALL_PORT" '
@@ -270,9 +272,13 @@ apply_host_monitor_firewall() {
     msg_ok "$(translate "Host firewall already allows:") TCP ${HOST_FIREWALL_PORT} $(translate "from") ${HOST_FIREWALL_SOURCE}"
     return 0
   fi
-  pvesh create "/nodes/${node}/firewall/rules" --type in --action ACCEPT --proto tcp \
-    --dport "$HOST_FIREWALL_PORT" --source "$HOST_FIREWALL_SOURCE" --comment "$comment" \
-    || die "$(translate "Could not add the confirmed host firewall rule")"
+  if ! pvesh create "/nodes/${node}/firewall/rules" --type in --action ACCEPT --proto tcp \
+    --dport "$HOST_FIREWALL_PORT" --source "$HOST_FIREWALL_SOURCE" --enable 1 --comment "$comment"; then
+    # The CT and its saved contract are already complete.  Do not destroy a
+    # usable installation merely because the optional network exposure failed.
+    msg_warn "$(translate "Could not add the confirmed host firewall rule")"
+    return 0
+  fi
   msg_ok "$(translate "Host firewall rule added:") TCP ${HOST_FIREWALL_PORT} $(translate "from") ${HOST_FIREWALL_SOURCE}"
 }
 

oci/remote/oci_remove.py

@@ -104,6 +104,43 @@ def release_bridge(bridge):
     subprocess.run(['pvesh', 'delete', f'/nodes/{node}/network/{bridge}'], check=False, capture_output=True)
 
 
+def remove_owned_host_firewall(record):
+    """Remove only the narrowly scoped rule created by this installation.
+
+    A matching port alone is never evidence of ownership: administrators and
+    other applications may legitimately use it.  Older CT-number comments are
+    deliberately left alone as well.
+    """
+    plan = record.get('deployment', {}).get('host_firewall') or {}
+    installation_id = record.get('installation_id', '')
+    if not isinstance(plan, dict) or not re.fullmatch(r'[0-9a-f-]{36}', installation_id):
+        return
+    source, port = plan.get('source'), plan.get('port')
+    if not isinstance(source, str) or not isinstance(port, int):
+        return
+    comment = f'ProxMenux OCI firewall {installation_id}'
+    node = socket.gethostname().split('.', 1)[0]
+    try:
+        result = subprocess.run(['pvesh', 'get', f'/nodes/{node}/firewall/rules', '--output-format', 'json'],
+                                check=True, capture_output=True, text=True)
+        rules = json.loads(result.stdout)
+        matches = [rule for rule in rules if rule.get('comment') == comment
+                   and str(rule.get('dport')) == str(port)
+                   and rule.get('source') == source
+                   and str(rule.get('proto', '')).lower() == 'tcp'
+                   and str(rule.get('type', '')).lower() == 'in'
+                   and str(rule.get('action', '')).upper() == 'ACCEPT']
+        if len(matches) != 1 or not isinstance(matches[0].get('pos'), int):
+            return
+        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}")
+    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.'))
+
+
 def remove(root, vmid):
     primary_id, primary, members = members_of(root, vmid)
     for member in members:
@@ -132,6 +169,7 @@ def remove(root, vmid):
         msg_ok(f"{translate('Private network of the application released:')} {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)
     lifecycle = Path(f'/etc/pve/priv/proxmenux-stack-{primary_id}.json')
     if lifecycle.exists() and not lifecycle.is_symlink():
         lifecycle.unlink()

oci/tests/test_glances_ports.py

@@ -1,15 +1,18 @@
 """Glances' Web UI is the only listener used by the native ``-w`` profile."""
 
 from pathlib import Path
+import sys
 import unittest
 
+ROOT = Path(__file__).resolve().parents[1]
+sys.path.insert(0, str(ROOT / "src"))
+
 from proxmenux_oci.catalog import Catalog
 
 
 class GlancesPortContractTests(unittest.TestCase):
     def test_web_profile_exposes_only_the_web_ui_port(self):
-        root = Path(__file__).resolve().parents[1]
-        template = Catalog(root).compose("glances")
+        template = Catalog(ROOT).compose("glances")
 
         ports = template["container_contract"]["ports"]
         self.assertEqual([item["container_port"] for item in ports], [61208])

oci/tests/test_host_monitor_firewall.py

@@ -4,13 +4,17 @@ from pathlib import Path
 import sys
 import unittest
 from unittest.mock import patch
+from subprocess import CompletedProcess
+import json
 
 
 ROOT = Path(__file__).resolve().parents[1]
 sys.path.insert(0, str(ROOT / "src"))
+sys.path.insert(0, str(ROOT / "remote"))
 
 from proxmenux_oci.installer import (InstallError, confirm_host_monitor_firewall,
                                      host_monitor_firewall_plan)
+import oci_remove
 
 
 class ConfirmUI:
@@ -55,13 +59,52 @@ class HostMonitorFirewallPlanTests(unittest.TestCase):
 
 
 class HostMonitorFirewallInstallerContractTests(unittest.TestCase):
+    def test_netdata_declares_its_host_web_port_for_the_firewall_plan(self):
+        catalog = json.loads((ROOT / "catalog" / "overlays" / "netdata.json").read_text(encoding="utf-8"))
+        profile = catalog["proxmox"]["installer_profile"]
+        self.assertEqual(profile["host_monitor"], "netdata")
+        self.assertEqual(profile["host_monitor_firewall"],
+                         {"protocol": "tcp", "web_port": 19999})
+
     def test_remote_installer_revalidates_and_uses_proxmox_rule_api(self):
         source = (ROOT / "remote" / "install_oci.sh").read_text(encoding="utf-8")
         self.assertIn("validate_host_monitor_firewall", source)
         self.assertIn("The host-monitor firewall subnet changed; no firewall rule was added", source)
         self.assertIn('pvesh create "/nodes/${node}/firewall/rules"', source)
         self.assertIn("--dport \"$HOST_FIREWALL_PORT\" --source \"$HOST_FIREWALL_SOURCE\"", source)
+        self.assertIn('--enable 1 --comment "$comment"', source)
         self.assertIn("only the original, separately confirmed installation", source)
+        self.assertIn('comment="ProxMenux OCI firewall ${INSTANCE_ID}"', source)
+        self.assertIn('if ! pvesh create "/nodes/${node}/firewall/rules"', source)
+
+    def test_removal_only_targets_a_uniquely_owned_firewall_rule(self):
+        source = (ROOT / "remote" / "oci_remove.py").read_text(encoding="utf-8")
+        self.assertIn("def remove_owned_host_firewall(record):", source)
+        self.assertIn("ProxMenux OCI firewall {installation_id}", source)
+        self.assertIn("len(matches) != 1", source)
+        self.assertIn("firewall/rules/{matches[0]['pos']}", source)
+
+    def test_removal_deletes_only_the_exact_rule_owned_by_the_installation(self):
+        record = {"installation_id": "123e4567-e89b-12d3-a456-426614174000",
+                  "deployment": {"host_firewall": {"source": "192.0.2.0/24", "port": 61208}}}
+        rules = [{"pos": 7, "comment": "ProxMenux OCI firewall 123e4567-e89b-12d3-a456-426614174000",
+                  "type": "in", "action": "ACCEPT", "proto": "tcp", "source": "192.0.2.0/24",
+                  "dport": "61208"},
+                 {"pos": 8, "comment": "manual rule", "type": "in", "action": "ACCEPT",
+                  "proto": "tcp", "source": "192.0.2.0/24", "dport": "61208"}]
+        with patch("oci_remove.subprocess.run", side_effect=[
+                CompletedProcess([], 0, json.dumps(rules), ""), CompletedProcess([], 0, "", "")]) as run:
+            oci_remove.remove_owned_host_firewall(record)
+        self.assertIn("/firewall/rules/7", run.call_args_list[1].args[0][-1])
+
+    def test_removal_keeps_an_unowned_matching_rule(self):
+        record = {"installation_id": "123e4567-e89b-12d3-a456-426614174000",
+                  "deployment": {"host_firewall": {"source": "192.0.2.0/24", "port": 61208}}}
+        rules = [{"pos": 8, "comment": "manual rule", "type": "in", "action": "ACCEPT",
+                  "proto": "tcp", "source": "192.0.2.0/24", "dport": "61208"}]
+        with patch("oci_remove.subprocess.run", return_value=CompletedProcess([], 0, json.dumps(rules), "")) as run:
+            oci_remove.remove_owned_host_firewall(record)
+        run.assert_called_once()
 
     def test_oci_menu_wrapper_does_not_hide_engine_errors_with_the_main_menu(self):
         wrapper = (ROOT.parent / "scripts" / "oci" / "oci_manager_apps.sh").read_text(encoding="utf-8")