fix(ping): post the settings where the API expects them, not one node above
CI / test (3.11) (push) Failing after 7s
CI / test (3.10) (push) Failing after 12s
CI / test (3.9) (push) Failing after 6s
CI / test (3.12) (push) Failing after 18s

Every ping against a live OPNsense failed:

    ping job creation failed for 10.30.0.1: {'result': 'failed',
    'validations': {'ping.settings.hostname': 'A value is required.'}}

_ping_model_node read GET /api/diagnostics/ping/get and took the single
dict-valued key as the node to post under. The real model nests two levels:

    {"ping": {"settings": {"hostname": "", "fam": {"ip": {...}, "ip6": {...}},
                           "source_address": "", "packetsize": "", ...}}}

so the helper answered "ping" and the job was created with the fields sitting
where the settings node belongs. The hostname never arrived, and the firewall
said so on every single call.

_ping_model_path walks the whole chain of single-dict wrappers and stops at the
first level holding more than one key — the field level, where fam is a dict
too and one more step would land inside a form field. _wrap_in_model nests the
settings accordingly, so a one-level model keeps working and the default, for
when /get cannot be read, is what current firmware ships.

The tests missed this because FakePingAPI answered /get with a one-level model
and read the posted payload back through the same assumption: the fake agreed
with the code about a shape neither of them shares with a device. It now speaks
what an OPNsense speaks, reads the payload through the model path, and a second
test keeps the one-level case covered.

Verified against a live firewall: the job is accepted ({"result": "ok"}) and
10.30.0.1 answers 3 of 3 at 0.116 ms.

Closes #3
This commit is contained in:
Christian Manivong
2026-08-22 15:44:56 +07:00
parent efba4334ff
commit 5d193ba7e8
2 changed files with 130 additions and 44 deletions
+47 -18
View File
@@ -52,6 +52,10 @@ logger = logging.getLogger(__name__)
_PING_API = "/api/diagnostics/ping"
#: Where the ping fields live when the model cannot be read. Matches what
#: OPNsense 24/25 ship: {"ping": {"settings": {...}}}.
_DEFAULT_PING_MODEL_PATH = ("ping", "settings")
#: Seconds between two ``search_jobs`` polls while waiting for probe results.
_POLL_INTERVAL = 0.5
@@ -74,7 +78,7 @@ class OPNsensePingMixin:
PING_SWEEP_MAX_TARGETS: int = 512
# Provided by the driver this mixin is mixed into.
_ping_model_node_cache: Optional[str] = None
_ping_model_path_cache: Optional[tuple[str, ...]] = None
# ── NAPALM ───────────────────────────────────────────────────────────────
@@ -202,7 +206,7 @@ class OPNsensePingMixin:
if source:
settings["source_address"] = source
response = self._post(f"{_PING_API}/set", {self._ping_model_node(): settings})
response = self._post(f"{_PING_API}/set", self._wrap_in_model(settings))
job_id = response.get("uuid") or response.get("id")
if not job_id:
raise RuntimeError(f"ping job creation failed for {destination}: {response}")
@@ -262,27 +266,52 @@ class OPNsensePingMixin:
result[str(job_id)] = row
return result
def _ping_model_node(self) -> str:
"""Name of the model's root node, i.e. the key ``set`` expects.
def _wrap_in_model(self, settings: dict[str, Any]) -> dict[str, Any]:
"""Nest *settings* under the keys ``set`` expects, outermost last."""
payload: dict[str, Any] = settings
for key in reversed(self._ping_model_path()):
payload = {key: payload}
return payload
Read once from ``GET /api/diagnostics/ping/get`` instead of hardcoded,
so a model rename in a future OPNsense release does not silently break
job creation. Falls back to ``"settings"``.
def _ping_model_path(self) -> tuple[str, ...]:
"""Keys from the model root down to the fields, e.g. ``("ping", "settings")``.
Read once from ``GET /api/diagnostics/ping/get`` rather than hardcoded,
so a model rename in a future release does not silently break job
creation. The response mirrors the model, so the path is the chain of
single-dict wrappers around the fields::
{"ping": {"settings": {"hostname": "", "fam": {...}, ...}}}
The descent stops at the first level that holds more than one key —
that is the field level, where ``fam`` is a dict too and following it
would land inside a form field.
Getting this wrong is expensive and quiet: OPNsense answers a job whose
hostname arrived at the wrong node with ``ping.settings.hostname: A
value is required``, and a sweep turns 254 such refusals into a network
that appears to hold nothing.
"""
if self._ping_model_node_cache is not None:
return self._ping_model_node_cache
if self._ping_model_path_cache is not None:
return self._ping_model_path_cache
node = "settings"
path = _DEFAULT_PING_MODEL_PATH
try:
response = self._get(f"{_PING_API}/get")
candidates = [key for key, value in response.items() if isinstance(value, dict)]
if len(candidates) == 1:
node = candidates[0]
except Exception as exc: # noqa: BLE001 - the default is a safe guess
logger.debug("Could not read ping model node, assuming %r: %s", node, exc)
node = self._get(f"{_PING_API}/get")
found: list[str] = []
while isinstance(node, dict) and len(node) == 1:
key, value = next(iter(node.items()))
if not isinstance(value, dict):
break
found.append(key)
node = value
if found:
path = tuple(found)
except Exception as exc: # noqa: BLE001 - the default is what firmware ships
logger.debug("Could not read ping model path, assuming %r: %s", path, exc)
self._ping_model_node_cache = node
return node
self._ping_model_path_cache = path
return path
# ── Parsing ──────────────────────────────────────────────────────────────