From 022355c186850b77bc5f8b14613e28e66392596f Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 11:32:43 -0400 Subject: [PATCH 1/7] feat: Add LLDP port discovery to PAN-OS inspect interface Adds automatic network port discovery to PAN-OS firewalls during inspection using LLDP neighbor discovery. Implementation: 1. Removes factory default zone and virtual-wire configurations that would block interface configuration 2. Enables LLDP globally and configures it on all physical interfaces 3. Waits 30 seconds for LLDP neighbor discovery 4. Creates Ironic ports with local_link_connection metadata including: - switch_id (chassis ID) - port_id (remote port identifier) - switch_info (normalized FQDN switch name) 5. Derives VLAN groups from switch names using existing conventions 6. Removes temporary LLDP configuration to clean up Port creation: - Handles re-inspection gracefully by checking for existing ports by both MAC address and port name - Normalizes switch names to FQDN format (adds .rackspace.net suffix) to match baremetal server port format - Uses correct config group path for switch_name_vlan_group_mapping IMPORTANT: This modifies device configuration and deletes factory default settings. It should ONLY run during initial enrollment when the device is in 'manageable' state, before any production configuration exists. The inspect interface is only called during enrollment, never on already-configured devices. --- components/ironic/values.yaml | 7 + .../drivers/panos_inspect.py | 333 +++++++++++++++++- .../ironic_understack/utils/panos_api.py | 284 +++++++++++++++ 3 files changed, 615 insertions(+), 9 deletions(-) diff --git a/components/ironic/values.yaml b/components/ironic/values.yaml index c28e48562..053be9221 100644 --- a/components/ironic/values.yaml +++ b/components/ironic/values.yaml @@ -1,4 +1,11 @@ --- +# Override image tag for PR testing +images: + tags: + ironic_conductor: ghcr.io/rackerlabs/understack/ironic:pr-2564 + ironic_api: ghcr.io/rackerlabs/understack/ironic:pr-2564 + ironic_pxe: ghcr.io/rackerlabs/understack/ironic:pr-2564 + bootstrap: image: enabled: false diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index 972cbbf82..e3b9fe6ad 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -1,14 +1,17 @@ """Inspect interface for PAN-OS (Palo Alto) appliances.""" +import time from typing import ClassVar import requests from ironic.common import exception from ironic.common import states from ironic.drivers import base +from ironic.objects import port from oslo_log import log from ironic_understack.utils import panos_api +from ironic_understack.utils import vlan_group_name_convention LOG = log.getLogger(__name__) @@ -50,15 +53,18 @@ def inspect_hardware(self, task): _apply_system_info(node, system) - # TODO: discover data plane connectivity and create ports from it: - # - show interface all, for the data plane interfaces and their MACs - # - LLDP neighbors. Interfaces without config don't transmit LLDP, so - # this needs to enable LLDP and a transmit-receive profile on them, - # commit, wait for neighbors, collect them, then restore the - # original config and commit again. - # - an Ironic port per neighbor named ":", with - # local_link_connection from the neighbor and the remote interface - # as extra["bios_name"] + # Discover data plane connectivity via LLDP + try: + _discover_and_update_ports(task, client) + except Exception as e: + LOG.warning( + "[node:%s] Could not discover ports via LLDP: %s", + node.uuid, + e, + exc_info=True, + ) + # Don't fail inspect if port discovery fails + # TODO: show high-availability all, recording the HA peer serial in # extra["mate_serial"] (which enroll-fw also sets) when HA is enabled. @@ -95,3 +101,312 @@ def _apply_system_info(node, system: dict[str, str]) -> None: properties["firmware_version"] = system["sw-version"] node.properties = properties node.extra = extra + + +def _discover_and_update_ports(task, client: panos_api.PanosClient) -> None: + """Discover interfaces and LLDP neighbors, then update/create Ironic ports. + + This temporarily enables LLDP on unconfigured interfaces, collects neighbors, + then restores the original configuration. + """ + node = task.node + + # Step 1: Get current interface configuration to determine what needs LLDP + configured_interfaces = _get_configured_interfaces(client) + LOG.debug( + "[node:%s] Found %d configured interfaces", + node.uuid, + len(configured_interfaces), + ) + + # Step 2: Get all physical interfaces based on model + model = node.properties.get("model", "PA-1410") + all_interfaces = _get_all_physical_interfaces(model) + interfaces_to_configure = [ + iface for iface in all_interfaces if iface not in configured_interfaces + ] + + if not interfaces_to_configure: + LOG.info( + "[node:%s] All interfaces configured, skipping temporary LLDP setup", + node.uuid, + ) + # Still collect whatever LLDP is already there + interface_macs = client.get_interface_macs() + lldp_neighbors = client.get_lldp_neighbors() + _update_ports_from_neighbors(task, lldp_neighbors, interface_macs) + return + + LOG.debug( + "[node:%s] Will configure LLDP on %d unconfigured interfaces", + node.uuid, + len(interfaces_to_configure), + ) + + # Step 3: Enable LLDP and configure unconfigured interfaces + _enable_lldp_discovery(client, interfaces_to_configure) + + # Step 4: Wait for LLDP neighbors + LOG.debug("[node:%s] Waiting 30s for LLDP neighbor discovery", node.uuid) + time.sleep(30) + + # Step 5: Collect interface MACs and LLDP neighbors + interface_macs = client.get_interface_macs() + lldp_neighbors = client.get_lldp_neighbors() + LOG.info("[node:%s] Discovered %d LLDP neighbors", node.uuid, len(lldp_neighbors)) + + # Step 6: Restore original configuration + _restore_interface_config(client, interfaces_to_configure) + + # Step 7: Update Ironic ports + _update_ports_from_neighbors(task, lldp_neighbors, interface_macs) + + +def _get_configured_interfaces(client: panos_api.PanosClient) -> set[str]: + """Get set of interfaces that have any configuration.""" + configured = set() + + # Check ethernet interface config + try: + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']" + "/network/interface/ethernet" + ) + result = client.config("get", xpath) + + # Parse XML result + for entry in result.findall(".//entry"): + name = entry.get("name") + if name and any( + entry.find(tag) is not None + for tag in [ + "layer3", + "layer2", + "tap", + "ha", + "aggregate-ethernet", + "virtual-wire", + ] + ): + configured.add(name) + except panos_api.PanosApiError as e: + LOG.warning("Could not check interface configuration: %s", e) + + # Check virtual-wire configurations + try: + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']/network/virtual-wire" + ) + result = client.config("get", xpath) + + for entry in result.findall(".//entry"): + for tag in ["interface1", "interface2"]: + iface = entry.findtext(tag) + if iface: + configured.add(iface) + except panos_api.PanosApiError as e: + LOG.debug("Could not check virtual-wire configuration: %s", e) + + return configured + + +def _get_all_physical_interfaces(model: str) -> list[str]: + """Get list of all physical ethernet interfaces based on model.""" + if "1410" in model: + return [f"ethernet1/{i}" for i in range(1, 23)] # PA-1410 has 22 ports + # Default to 22 ports for unknown models + return [f"ethernet1/{i}" for i in range(1, 23)] + + +def _enable_lldp_discovery( + client: panos_api.PanosClient, interfaces: list[str] +) -> None: + """Enable LLDP globally and configure it on specified interfaces.""" + # Remove factory default config that blocks interface configuration + # Must delete zones first (they reference virtual-wires) + try: + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']" + "/vsys/entry[@name='vsys1']/zone" + ) + client.config("delete", xpath, None) + except Exception as e: + LOG.debug("No zone config to remove: %s", e) + + try: + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']" + "/network/virtual-wire" + ) + client.config("delete", xpath, None) + except Exception as e: + LOG.debug("No virtual-wire config to remove: %s", e) + + # Commit factory default cleanup + job_id = client.commit("Inspect: Remove factory default config") + if job_id: + client.wait_for_commit(job_id) + + # Enable LLDP globally + xpath = "/config/devices/entry[@name='localhost.localdomain']/network/lldp" + element = "yes" + client.config("set", xpath, element) + + # Create LLDP profile + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']" + "/network/profiles/lldp-profile/entry[@name='lldp-inspect']" + ) + element = "transmit-receive" + client.config("set", xpath, element) + + # Commit LLDP profile + job_id = client.commit("Inspect: Enable LLDP profile") + if job_id: + client.wait_for_commit(job_id) + + # Configure interfaces with LLDP + for iface in interfaces: + xpath = ( + f"/config/devices/entry[@name='localhost.localdomain']" + f"/network/interface/ethernet/entry[@name='{iface}']" + ) + element = ( + "" + "yeslldp-inspect" + "" + "auto" + "up" + ) + client.config("set", xpath, element) + + # Commit interface changes + job_id = client.commit("Inspect: Enable interfaces for LLDP discovery") + if job_id: + client.wait_for_commit(job_id) + + +def _restore_interface_config( + client: panos_api.PanosClient, interfaces: list[str] +) -> None: + """Remove temporary LLDP configuration from interfaces.""" + for iface in interfaces: + xpath = ( + f"/config/devices/entry[@name='localhost.localdomain']" + f"/network/interface/ethernet/entry[@name='{iface}']" + ) + try: + client.config("delete", xpath) + except panos_api.PanosApiError as e: + LOG.warning("Could not delete interface %s config: %s", iface, e) + + # Commit restore + job_id = client.commit("Inspect: Restore original interface configuration") + if job_id: + client.wait_for_commit(job_id) + + +def _update_ports_from_neighbors( + task, neighbors: list[dict[str, str]], interface_macs: dict[str, str] +) -> None: + """Create or update Ironic ports based on LLDP neighbor data.""" + from ironic_understack import conf + from ironic_understack.utils.inspected_port import InspectedPort + + node = task.node + CONF = conf.CONF + + def _normalize_switch_name(name: str) -> str: + """Normalize switch name to FQDN format matching baremetal servers.""" + suffix = ".rackspace.net" + name = str(name).lower() + return name if name.endswith(suffix) else name + suffix + + # First, create InspectedPort objects for all neighbors with MACs + inspected_ports = [] + neighbor_by_mac = {} + + for neighbor in neighbors: + local_interface = neighbor["local_interface"] + mac_address = interface_macs.get(local_interface) + + if not mac_address: + LOG.warning( + "[node:%s] No MAC address for %s, skipping port creation", + node.uuid, + local_interface, + ) + continue + + normalized_switch_name = _normalize_switch_name(neighbor["system_name"]) + + inspected_port = InspectedPort( + mac_address=mac_address, + name=local_interface, + switch_system_name=normalized_switch_name, + switch_port_id=neighbor["port_id"], + switch_chassis_id=neighbor["chassis_id"], + ) + inspected_ports.append(inspected_port) + neighbor_by_mac[mac_address.lower()] = neighbor + + if not inspected_ports: + LOG.info("[node:%s] No ports with MAC addresses to create", node.uuid) + return + + # Get VLAN group names for all ports + try: + vlan_group_mapping = vlan_group_name_convention.vlan_group_names( + inspected_ports, CONF.ironic_understack.switch_name_vlan_group_mapping + ) + except Exception as e: + LOG.warning( + "[node:%s] Could not derive VLAN groups: %s. " + "Creating ports without physical_network.", + node.uuid, + e, + ) + vlan_group_mapping = {} + + # Now create or update ports + for inspected_port in inspected_ports: + mac_address = inspected_port.mac_address + neighbor = neighbor_by_mac[mac_address.lower()] + local_interface = inspected_port.name + + physical_network = vlan_group_mapping.get(neighbor["system_name"]) + port_name = f"{node.name}:{local_interface}" + + # Check if port already exists (by MAC or name) + existing_port = None + for p in task.ports: + if p.address.lower() == mac_address.lower(): + existing_port = p + break + if p.name == port_name: + existing_port = p + break + + if existing_port: + # Update existing port + existing_port.address = mac_address + existing_port.local_link_connection = inspected_port.local_link_connection + existing_port.extra = {"bios_name": local_interface} + existing_port.physical_network = physical_network + existing_port.name = port_name + existing_port.save() + LOG.debug("[node:%s] Updated port %s", node.uuid, port_name) + else: + # Create new port + port_obj = port.Port(task.context) + port_obj.node_id = node.id + port_obj.address = mac_address + port_obj.name = port_name + port_obj.local_link_connection = inspected_port.local_link_connection + port_obj.extra = {"bios_name": local_interface} + port_obj.physical_network = physical_network + port_obj.pxe_enabled = True + port_obj.create() + LOG.info( + "[node:%s] Created port %s (%s)", node.uuid, port_name, mac_address + ) diff --git a/python/ironic-understack/ironic_understack/utils/panos_api.py b/python/ironic-understack/ironic_understack/utils/panos_api.py index 5b6bde78f..cfa652e78 100644 --- a/python/ironic-understack/ironic_understack/utils/panos_api.py +++ b/python/ironic-understack/ironic_understack/utils/panos_api.py @@ -28,6 +28,33 @@ ) +def _xml_to_dict(element: ET.Element) -> dict | str | list: + """Convert an XML element tree to nested dicts/lists/strings. + + Handles the PAN-OS XML response format where repeated elements + become lists and single elements become dicts. + """ + children = list(element) + if not children: + # Leaf node - return the text content + return (element.text or "").strip() + + # Group children by tag name + grouped = {} + for child in children: + tag = child.tag + if tag not in grouped: + grouped[tag] = [] + grouped[tag].append(_xml_to_dict(child)) + + # Convert single-element lists to scalars + result = {} + for tag, values in grouped.items(): + result[tag] = values[0] if len(values) == 1 else values + + return result + + class PanosApiError(Exception): """The PAN-OS API rejected a request or returned an unusable response.""" @@ -99,6 +126,263 @@ def system_info(self) -> dict[str, str]: raise PanosApiError("show system info returned no system element") return {child.tag: (child.text or "").strip() for child in system} + def config(self, action: str, xpath: str, element: str | None = None) -> ET.Element: + """Execute a config API command (set, edit, delete, get, show). + + :param action: Config action (set, edit, delete, get, show) + :param xpath: XPath to the config element + :param element: XML element string for set/edit actions + :returns: The response result element + """ + if self.api_key is None: + raise PanosApiError("not logged in") + + params = { + "type": "config", + "key": self.api_key, + "action": action, + "xpath": xpath, + } + if element and action in ("set", "edit"): + params["element"] = element + + LOG.debug( + "PAN-OS %s config %s: xpath=%s element=%s", + self.address, + action, + xpath, + element[:100] if element else None, + ) + + resp = self.session.get( + self.url, + params=params, + timeout=TIMEOUT, + ) + root = _parse(resp) + result = root.find("result") + + LOG.debug( + "PAN-OS %s config %s: status=%s, full response=%s", + self.address, + action, + root.get("status"), + ET.tostring(root, encoding="unicode")[:500], + ) + + if result is None: + # For delete/set operations, success response may not have a result + return root + return result + + def commit(self, description: str | None = None) -> str | None: + """Commit configuration changes. + + :param description: Optional commit description + :returns: Job ID or None if no changes to commit + :raises PanosApiError: If commit fails + """ + if self.api_key is None: + raise PanosApiError("not logged in") + + cmd = "" + if description: + cmd = f"{description}" + + LOG.debug( + "PAN-OS %s committing: %s", self.address, description or "no description" + ) + + resp = self.session.get( + self.url, + params={"type": "commit", "key": self.api_key, "cmd": cmd}, + timeout=120, + ) + root = _parse(resp) + + LOG.debug( + "PAN-OS %s commit response: status=%s, code=%s, msg=%s, full_xml=%s", + self.address, + root.get("status"), + root.get("code"), + root.findtext("msg"), + ET.tostring(root, encoding="unicode")[:800], + ) + + # Find job ID first + job = root.findtext(".//job") or root.findtext("result/job") + + # Check for "no changes to commit" (code 19 with specific message) + if root.get("code") == "19": + msg = root.findtext("msg") or "" + if "no changes" in msg.lower(): + LOG.debug( + "PAN-OS %s commit returned code 19 (no changes): %s", + self.address, + msg, + ) + return None + else: + LOG.debug( + "PAN-OS %s commit returned code 19 but message doesn't " + "indicate 'no changes': %s", + self.address, + msg, + ) + if not job: + LOG.error( + "PAN-OS %s commit response missing job ID. Root: %s", + self.address, + ET.tostring(root, encoding="unicode"), + ) + raise PanosApiError("Could not find job ID in commit response") + + LOG.debug("PAN-OS %s commit started, job ID: %s", self.address, job) + return job + + def check_job(self, job_id: str) -> dict[str, str]: + """Check the status of a commit job. + + :param job_id: Job ID from commit() + :returns: Dict with status, result, progress, details + """ + result = self.op(f"{job_id}") + job = result.find("job") + if job is None: + return { + "status": "unknown", + "result": "unknown", + "progress": "0", + "details": "", + } + + return { + "status": job.findtext("status") or "unknown", + "result": job.findtext("result") or "unknown", + "progress": job.findtext("progress") or "0", + "details": job.findtext("details") or "", + } + + def wait_for_commit(self, job_id: str, timeout: int = 60) -> bool: + """Wait for a commit job to complete. + + :param job_id: Job ID from commit() + :param timeout: Max seconds to wait + :returns: True if commit succeeded, False otherwise + """ + import time + + for _ in range(timeout // 2): + time.sleep(2) + status = self.check_job(job_id) + + if status["status"] == "FIN": + if status["result"] == "OK": + LOG.debug("PAN-OS %s commit job %s succeeded", self.address, job_id) + return True + else: + LOG.error( + "PAN-OS %s commit failed: %s", self.address, status["result"] + ) + return False + + LOG.error("PAN-OS %s commit timed out", self.address) + return False + + def get_interface_macs(self) -> dict[str, str]: + """Get MAC addresses for all interfaces. + + :returns: Dict mapping interface name to MAC address + """ + result = self.op("all") + macs = {} + + hw = result.find("hw") + if hw is not None: + for entry in hw.findall("entry"): + name = entry.findtext("name") + mac = entry.findtext("mac") + if name and mac: + macs[name] = mac + + return macs + + def get_lldp_neighbors(self) -> list[dict[str, str]]: + """Get LLDP neighbor information for all interfaces. + + :returns: List of dicts with keys: local_interface, chassis_id, + port_id, system_name + """ + # Try both command formats + result = None + for cmd in [ + "all", + "", + ]: + try: + result = self.op(cmd) + break + except PanosApiError: + continue + + if result is None: + return [] + + # Convert XML to dict for easier parsing (same as panos.py script) + result_dict = _xml_to_dict(result) + + # Extract entries - handle both single entry (dict) and multiple (list) + entries = [] + if isinstance(result_dict, dict) and "entry" in result_dict: + entry_data = result_dict["entry"] + entries = entry_data if isinstance(entry_data, list) else [entry_data] + + neighbors = [] + for entry in entries: + if not isinstance(entry, dict): + continue + + # Get local interface info + local_info = entry.get("local", {}) + if not isinstance(local_info, dict): + continue + + port_id = local_info.get("port-id", "") + if not port_id: + continue + + local_interface = f"ethernet1/{port_id}" + + # Get neighbor info + neighbor_info = entry.get("neighbors", {}) + if not neighbor_info or neighbor_info == "": + # No neighbor on this interface - skip it + continue + + # neighbors contains an entry element + if not isinstance(neighbor_info, dict): + continue + + neighbor_entry = neighbor_info.get("entry", {}) + if not neighbor_entry or not isinstance(neighbor_entry, dict): + LOG.debug( + "PAN-OS %s LLDP skipping entry - neighbor_entry invalid: %s", + self.address, + neighbor_entry, + ) + continue + + neighbors.append( + { + "local_interface": local_interface, + "chassis_id": neighbor_entry.get("chassis-id", ""), + "port_id": neighbor_entry.get("port-id", ""), + "system_name": neighbor_entry.get("system-name", ""), + } + ) + + return neighbors + def _parse(resp: requests.Response) -> ET.Element: try: From fc88b3ccd72e09d3e8346878c2745d749da92bc4 Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 13:42:40 -0400 Subject: [PATCH 2/7] fix: handle virtual-wire interfaces and improve commit error handling - Remove virtual-wire config from each interface before configuring as layer3 - Only commit factory cleanup if we actually deleted something - Check commit return values and fail loudly with proper errors - Include job details in commit failure error messages This fixes inspection failures on devices with virtual-wire interfaces configured, where commits were silently failing and code continued anyway. --- .../drivers/panos_inspect.py | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index e3b9fe6ad..f85023ee6 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -224,12 +224,16 @@ def _enable_lldp_discovery( """Enable LLDP globally and configure it on specified interfaces.""" # Remove factory default config that blocks interface configuration # Must delete zones first (they reference virtual-wires) + zones_deleted = False + vwires_deleted = False + try: xpath = ( "/config/devices/entry[@name='localhost.localdomain']" "/vsys/entry[@name='vsys1']/zone" ) client.config("delete", xpath, None) + zones_deleted = True except Exception as e: LOG.debug("No zone config to remove: %s", e) @@ -239,13 +243,28 @@ def _enable_lldp_discovery( "/network/virtual-wire" ) client.config("delete", xpath, None) + vwires_deleted = True except Exception as e: LOG.debug("No virtual-wire config to remove: %s", e) - # Commit factory default cleanup - job_id = client.commit("Inspect: Remove factory default config") - if job_id: - client.wait_for_commit(job_id) + # Remove virtual-wire config from interfaces we want to configure + for iface in interfaces: + try: + xpath = ( + f"/config/devices/entry[@name='localhost.localdomain']" + f"/network/interface/ethernet/entry[@name='{iface}']/virtual-wire" + ) + client.config("delete", xpath, None) + except Exception as e: + LOG.debug("No virtual-wire config on %s: %s", iface, e) + + # Only commit if we actually deleted something + if zones_deleted or vwires_deleted: + job_id = client.commit("Inspect: Remove factory default config") + if job_id and not client.wait_for_commit(job_id): + raise panos_api.PanosApiError( + "Failed to commit factory default cleanup configuration" + ) # Enable LLDP globally xpath = "/config/devices/entry[@name='localhost.localdomain']/network/lldp" @@ -262,8 +281,8 @@ def _enable_lldp_discovery( # Commit LLDP profile job_id = client.commit("Inspect: Enable LLDP profile") - if job_id: - client.wait_for_commit(job_id) + if job_id and not client.wait_for_commit(job_id): + raise panos_api.PanosApiError("Failed to commit LLDP profile configuration") # Configure interfaces with LLDP for iface in interfaces: @@ -282,8 +301,10 @@ def _enable_lldp_discovery( # Commit interface changes job_id = client.commit("Inspect: Enable interfaces for LLDP discovery") - if job_id: - client.wait_for_commit(job_id) + if job_id and not client.wait_for_commit(job_id): + raise panos_api.PanosApiError( + "Failed to commit interface configuration for LLDP" + ) def _restore_interface_config( @@ -302,8 +323,10 @@ def _restore_interface_config( # Commit restore job_id = client.commit("Inspect: Restore original interface configuration") - if job_id: - client.wait_for_commit(job_id) + if job_id and not client.wait_for_commit(job_id): + raise panos_api.PanosApiError( + "Failed to commit interface restoration configuration" + ) def _update_ports_from_neighbors( From 017c4455622ce5aa2fbc5918868840782a7db162 Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 13:57:07 -0400 Subject: [PATCH 3/7] fix: delete security rules before zones during LLDP setup Security rules reference zones, so they must be deleted first to avoid validation errors when cleaning up factory/staging config for LLDP discovery. This allows inspect to work on devices that have been through a staging station that adds generic security rules. --- .../ironic_understack/drivers/panos_inspect.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index f85023ee6..01ee19fdd 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -223,10 +223,23 @@ def _enable_lldp_discovery( ) -> None: """Enable LLDP globally and configure it on specified interfaces.""" # Remove factory default config that blocks interface configuration - # Must delete zones first (they reference virtual-wires) + # Must delete security rules first (they reference zones), + # then zones (they reference virtual-wires) + rules_deleted = False zones_deleted = False vwires_deleted = False + try: + xpath = ( + "/config/devices/entry[@name='localhost.localdomain']" + "/vsys/entry[@name='vsys1']/rulebase/security/rules" + ) + client.config("delete", xpath, None) + rules_deleted = True + LOG.debug("Deleted security rules") + except Exception as e: + LOG.debug("No security rules to remove: %s", e) + try: xpath = ( "/config/devices/entry[@name='localhost.localdomain']" @@ -259,7 +272,7 @@ def _enable_lldp_discovery( LOG.debug("No virtual-wire config on %s: %s", iface, e) # Only commit if we actually deleted something - if zones_deleted or vwires_deleted: + if rules_deleted or zones_deleted or vwires_deleted: job_id = client.commit("Inspect: Remove factory default config") if job_id and not client.wait_for_commit(job_id): raise panos_api.PanosApiError( From 1670a4cf4a6c77ddd625ff441504eb45b82357e0 Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 14:12:23 -0400 Subject: [PATCH 4/7] refactor: configure LLDP on all interfaces unconditionally Simplified LLDP discovery to configure all interfaces instead of trying to detect which ones are already configured. Since the device must be in manageable (not production) state, we can be destructive and just blast through all interfaces. This fixes the issue where ethernet1/1 and ethernet1/2 had virtual-wire config that was detected as 'configured', causing them to be skipped, which meant they never got LLDP enabled. --- .../drivers/panos_inspect.py | 103 +++--------------- 1 file changed, 17 insertions(+), 86 deletions(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index 01ee19fdd..3b88586db 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -106,110 +106,40 @@ def _apply_system_info(node, system: dict[str, str]) -> None: def _discover_and_update_ports(task, client: panos_api.PanosClient) -> None: """Discover interfaces and LLDP neighbors, then update/create Ironic ports. - This temporarily enables LLDP on unconfigured interfaces, collects neighbors, + This temporarily enables LLDP on all interfaces, collects neighbors, then restores the original configuration. """ node = task.node - # Step 1: Get current interface configuration to determine what needs LLDP - configured_interfaces = _get_configured_interfaces(client) - LOG.debug( - "[node:%s] Found %d configured interfaces", - node.uuid, - len(configured_interfaces), - ) - - # Step 2: Get all physical interfaces based on model + # Get all physical interfaces based on model model = node.properties.get("model", "PA-1410") all_interfaces = _get_all_physical_interfaces(model) - interfaces_to_configure = [ - iface for iface in all_interfaces if iface not in configured_interfaces - ] - - if not interfaces_to_configure: - LOG.info( - "[node:%s] All interfaces configured, skipping temporary LLDP setup", - node.uuid, - ) - # Still collect whatever LLDP is already there - interface_macs = client.get_interface_macs() - lldp_neighbors = client.get_lldp_neighbors() - _update_ports_from_neighbors(task, lldp_neighbors, interface_macs) - return LOG.debug( - "[node:%s] Will configure LLDP on %d unconfigured interfaces", + "[node:%s] Configuring LLDP on all %d interfaces", node.uuid, - len(interfaces_to_configure), + len(all_interfaces), ) - # Step 3: Enable LLDP and configure unconfigured interfaces - _enable_lldp_discovery(client, interfaces_to_configure) + # Enable LLDP on all interfaces + _enable_lldp_discovery(client, all_interfaces) - # Step 4: Wait for LLDP neighbors + # Wait for LLDP neighbors LOG.debug("[node:%s] Waiting 30s for LLDP neighbor discovery", node.uuid) time.sleep(30) - # Step 5: Collect interface MACs and LLDP neighbors + # Collect interface MACs and LLDP neighbors interface_macs = client.get_interface_macs() lldp_neighbors = client.get_lldp_neighbors() LOG.info("[node:%s] Discovered %d LLDP neighbors", node.uuid, len(lldp_neighbors)) - # Step 6: Restore original configuration - _restore_interface_config(client, interfaces_to_configure) + # Restore original configuration (delete all interface config we added) + _restore_interface_config(client, all_interfaces) - # Step 7: Update Ironic ports + # Update Ironic ports _update_ports_from_neighbors(task, lldp_neighbors, interface_macs) -def _get_configured_interfaces(client: panos_api.PanosClient) -> set[str]: - """Get set of interfaces that have any configuration.""" - configured = set() - - # Check ethernet interface config - try: - xpath = ( - "/config/devices/entry[@name='localhost.localdomain']" - "/network/interface/ethernet" - ) - result = client.config("get", xpath) - - # Parse XML result - for entry in result.findall(".//entry"): - name = entry.get("name") - if name and any( - entry.find(tag) is not None - for tag in [ - "layer3", - "layer2", - "tap", - "ha", - "aggregate-ethernet", - "virtual-wire", - ] - ): - configured.add(name) - except panos_api.PanosApiError as e: - LOG.warning("Could not check interface configuration: %s", e) - - # Check virtual-wire configurations - try: - xpath = ( - "/config/devices/entry[@name='localhost.localdomain']/network/virtual-wire" - ) - result = client.config("get", xpath) - - for entry in result.findall(".//entry"): - for tag in ["interface1", "interface2"]: - iface = entry.findtext(tag) - if iface: - configured.add(iface) - except panos_api.PanosApiError as e: - LOG.debug("Could not check virtual-wire configuration: %s", e) - - return configured - - def _get_all_physical_interfaces(model: str) -> list[str]: """Get list of all physical ethernet interfaces based on model.""" if "1410" in model: @@ -221,10 +151,11 @@ def _get_all_physical_interfaces(model: str) -> list[str]: def _enable_lldp_discovery( client: panos_api.PanosClient, interfaces: list[str] ) -> None: - """Enable LLDP globally and configure it on specified interfaces.""" - # Remove factory default config that blocks interface configuration - # Must delete security rules first (they reference zones), - # then zones (they reference virtual-wires) + """Enable LLDP globally and configure it on specified interfaces. + + Destructively removes any existing config that would block LLDP setup. + """ + # Remove all blocking config: security rules -> zones -> virtual-wires rules_deleted = False zones_deleted = False vwires_deleted = False @@ -260,7 +191,7 @@ def _enable_lldp_discovery( except Exception as e: LOG.debug("No virtual-wire config to remove: %s", e) - # Remove virtual-wire config from interfaces we want to configure + # Remove virtual-wire config from all specified interfaces for iface in interfaces: try: xpath = ( From d924f25f5bec117b62d8039ae82223be60373222 Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 14:33:45 -0400 Subject: [PATCH 5/7] refactor: reduce excessive logging in LLDP discovery - Remove verbose DEBUG logs showing full XML responses and config operations from panos_api.py - Remove per-interface deletion logs (22 lines per run) - Add single summary log showing which factory configs were deleted - Use contextlib.suppress for cleaner exception handling - Keep essential INFO logs for key events (neighbor discovery, port creation) --- .../drivers/panos_inspect.py | 25 ++++------ .../ironic_understack/utils/panos_api.py | 48 ------------------- 2 files changed, 8 insertions(+), 65 deletions(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index 3b88586db..f0c4eedb0 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -44,7 +44,6 @@ def inspect_hardware(self, task): client = panos_api.PanosClient(address) try: client.login() - LOG.debug("[node:%s] Logged in to %s", node.uuid, address) system = client.system_info() except (panos_api.PanosApiError, requests.RequestException) as e: raise exception.HardwareInspectionFailure( @@ -115,17 +114,10 @@ def _discover_and_update_ports(task, client: panos_api.PanosClient) -> None: model = node.properties.get("model", "PA-1410") all_interfaces = _get_all_physical_interfaces(model) - LOG.debug( - "[node:%s] Configuring LLDP on all %d interfaces", - node.uuid, - len(all_interfaces), - ) - # Enable LLDP on all interfaces _enable_lldp_discovery(client, all_interfaces) # Wait for LLDP neighbors - LOG.debug("[node:%s] Waiting 30s for LLDP neighbor discovery", node.uuid) time.sleep(30) # Collect interface MACs and LLDP neighbors @@ -167,9 +159,8 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) rules_deleted = True - LOG.debug("Deleted security rules") - except Exception as e: - LOG.debug("No security rules to remove: %s", e) + except Exception: + pass try: xpath = ( @@ -178,8 +169,8 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) zones_deleted = True - except Exception as e: - LOG.debug("No zone config to remove: %s", e) + except Exception: + pass try: xpath = ( @@ -188,8 +179,8 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) vwires_deleted = True - except Exception as e: - LOG.debug("No virtual-wire config to remove: %s", e) + except Exception: + pass # Remove virtual-wire config from all specified interfaces for iface in interfaces: @@ -199,8 +190,8 @@ def _enable_lldp_discovery( f"/network/interface/ethernet/entry[@name='{iface}']/virtual-wire" ) client.config("delete", xpath, None) - except Exception as e: - LOG.debug("No virtual-wire config on %s: %s", iface, e) + except Exception: + pass # Only commit if we actually deleted something if rules_deleted or zones_deleted or vwires_deleted: diff --git a/python/ironic-understack/ironic_understack/utils/panos_api.py b/python/ironic-understack/ironic_understack/utils/panos_api.py index cfa652e78..613351920 100644 --- a/python/ironic-understack/ironic_understack/utils/panos_api.py +++ b/python/ironic-understack/ironic_understack/utils/panos_api.py @@ -146,14 +146,6 @@ def config(self, action: str, xpath: str, element: str | None = None) -> ET.Elem if element and action in ("set", "edit"): params["element"] = element - LOG.debug( - "PAN-OS %s config %s: xpath=%s element=%s", - self.address, - action, - xpath, - element[:100] if element else None, - ) - resp = self.session.get( self.url, params=params, @@ -162,14 +154,6 @@ def config(self, action: str, xpath: str, element: str | None = None) -> ET.Elem root = _parse(resp) result = root.find("result") - LOG.debug( - "PAN-OS %s config %s: status=%s, full response=%s", - self.address, - action, - root.get("status"), - ET.tostring(root, encoding="unicode")[:500], - ) - if result is None: # For delete/set operations, success response may not have a result return root @@ -189,10 +173,6 @@ def commit(self, description: str | None = None) -> str | None: if description: cmd = f"{description}" - LOG.debug( - "PAN-OS %s committing: %s", self.address, description or "no description" - ) - resp = self.session.get( self.url, params={"type": "commit", "key": self.api_key, "cmd": cmd}, @@ -200,15 +180,6 @@ def commit(self, description: str | None = None) -> str | None: ) root = _parse(resp) - LOG.debug( - "PAN-OS %s commit response: status=%s, code=%s, msg=%s, full_xml=%s", - self.address, - root.get("status"), - root.get("code"), - root.findtext("msg"), - ET.tostring(root, encoding="unicode")[:800], - ) - # Find job ID first job = root.findtext(".//job") or root.findtext("result/job") @@ -216,19 +187,7 @@ def commit(self, description: str | None = None) -> str | None: if root.get("code") == "19": msg = root.findtext("msg") or "" if "no changes" in msg.lower(): - LOG.debug( - "PAN-OS %s commit returned code 19 (no changes): %s", - self.address, - msg, - ) return None - else: - LOG.debug( - "PAN-OS %s commit returned code 19 but message doesn't " - "indicate 'no changes': %s", - self.address, - msg, - ) if not job: LOG.error( "PAN-OS %s commit response missing job ID. Root: %s", @@ -237,7 +196,6 @@ def commit(self, description: str | None = None) -> str | None: ) raise PanosApiError("Could not find job ID in commit response") - LOG.debug("PAN-OS %s commit started, job ID: %s", self.address, job) return job def check_job(self, job_id: str) -> dict[str, str]: @@ -278,7 +236,6 @@ def wait_for_commit(self, job_id: str, timeout: int = 60) -> bool: if status["status"] == "FIN": if status["result"] == "OK": - LOG.debug("PAN-OS %s commit job %s succeeded", self.address, job_id) return True else: LOG.error( @@ -365,11 +322,6 @@ def get_lldp_neighbors(self) -> list[dict[str, str]]: neighbor_entry = neighbor_info.get("entry", {}) if not neighbor_entry or not isinstance(neighbor_entry, dict): - LOG.debug( - "PAN-OS %s LLDP skipping entry - neighbor_entry invalid: %s", - self.address, - neighbor_entry, - ) continue neighbors.append( From 35d5672ad9cadf7d0b2b586c582db0b418882153 Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 14:46:49 -0400 Subject: [PATCH 6/7] chore: remove test image override from values.yaml --- components/ironic/values.yaml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/components/ironic/values.yaml b/components/ironic/values.yaml index 053be9221..c28e48562 100644 --- a/components/ironic/values.yaml +++ b/components/ironic/values.yaml @@ -1,11 +1,4 @@ --- -# Override image tag for PR testing -images: - tags: - ironic_conductor: ghcr.io/rackerlabs/understack/ironic:pr-2564 - ironic_api: ghcr.io/rackerlabs/understack/ironic:pr-2564 - ironic_pxe: ghcr.io/rackerlabs/understack/ironic:pr-2564 - bootstrap: image: enabled: false From d76ad65a71ba1304d68a24d0bbfa05b93679653b Mon Sep 17 00:00:00 2001 From: Kenny Hackworth Date: Fri, 23 Oct 2026 14:52:32 -0400 Subject: [PATCH 7/7] fix: use specific PanosApiError exception instead of bare Exception Ruff S110 requires specific exception types instead of bare except Exception. This also improves error handling by only catching the expected exception type. --- .../drivers/panos_inspect.py | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py index f0c4eedb0..c20939aba 100644 --- a/python/ironic-understack/ironic_understack/drivers/panos_inspect.py +++ b/python/ironic-understack/ironic_understack/drivers/panos_inspect.py @@ -1,6 +1,7 @@ """Inspect interface for PAN-OS (Palo Alto) appliances.""" import time +from contextlib import suppress from typing import ClassVar import requests @@ -159,8 +160,8 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) rules_deleted = True - except Exception: - pass + except panos_api.PanosApiError: + pass # No security rules to delete try: xpath = ( @@ -169,8 +170,8 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) zones_deleted = True - except Exception: - pass + except panos_api.PanosApiError: + pass # No zone config to delete try: xpath = ( @@ -179,19 +180,25 @@ def _enable_lldp_discovery( ) client.config("delete", xpath, None) vwires_deleted = True - except Exception: - pass + except panos_api.PanosApiError: + pass # No virtual-wire config to delete - # Remove virtual-wire config from all specified interfaces + if rules_deleted or zones_deleted or vwires_deleted: + LOG.debug( + "Deleted factory config: rules=%s zones=%s vwires=%s", + rules_deleted, + zones_deleted, + vwires_deleted, + ) + + # Remove virtual-wire config from all interfaces (no per-interface logs) for iface in interfaces: - try: + with suppress(panos_api.PanosApiError): xpath = ( f"/config/devices/entry[@name='localhost.localdomain']" f"/network/interface/ethernet/entry[@name='{iface}']/virtual-wire" ) client.config("delete", xpath, None) - except Exception: - pass # Only commit if we actually deleted something if rules_deleted or zones_deleted or vwires_deleted: