Support conditional pending-dataset replaces with entity tags

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name         = "python-otbr-api"
-version      = "2.10.0"
+version      = "2.11.0"
 license      = {text = "MIT"}
 description  = "API to interact with an OTBR via its REST API"
 readme       = "README.md"

python_otbr_api/__init__.py

@@ -113,6 +113,19 @@ class EphemeralKeyConflictError(OTBRError):
     key is already active."""
 
 
+class PendingDatasetConflictError(OTBRError):
+    """Raised when a pending dataset write is refused.
+
+    Either a pending dataset is already in place for a guarded write, or the
+    one whose entity tag the write carried has been replaced since it was
+    read.
+
+    Its own type rather than a plain OTBRError: the caller's next step is
+    different from the one a transport or protocol failure calls for, since
+    nothing was written and the state that made the write wrong is readable.
+    """
+
+
 def _rewrite_keys(data: Any, mapping: dict[str, str]) -> Any:
     """Recursively rename dict keys according to mapping; pass through others."""
     if not isinstance(data, dict):
@@ -272,6 +285,21 @@ async def get_pending_dataset_tlvs(self) -> bytes | None:
         Returns None if there is no pending operational dataset.
         Raises if the http status is 400 or higher or if the response is invalid.
         """
+        result = await self.get_pending_dataset_tlvs_with_etag()
+        return None if result is None else result[0]
+
+    async def get_pending_dataset_tlvs_with_etag(
+        self,
+    ) -> tuple[bytes, str | None] | None:
+        """Get the pending dataset TLVs together with its entity tag, or None.
+
+        Returns None if there is no pending operational dataset. The entity
+        tag is None on a border router that does not hand one out; when it
+        does (https://github.com/openthread/ot-br-posix/pull/3553), the tag
+        can be passed to set_pending_dataset_tlvs() as if_match to only
+        replace the dataset that was read here.
+        Raises if the http status is 400 or higher or if the response is invalid.
+        """
         await self._maybe_detect_key_format()
         response = await self._session.get(
             f"{self._url}/node/dataset/pending",
@@ -286,7 +314,10 @@ async def get_pending_dataset_tlvs(self) -> bytes | None:
             raise OTBRError(f"unexpected http status {response.status}")
 
         try:
-            return bytes.fromhex(await response.text("ASCII"))
+            return (
+                bytes.fromhex(await response.text("ASCII")),
+                response.headers.get("ETag"),
+            )
         except ValueError as exc:
             raise OTBRError("unexpected API response") from exc
 
@@ -372,6 +403,67 @@ async def set_active_dataset_tlvs(self, dataset: bytes) -> None:
         if response.status not in (HTTPStatus.CREATED, HTTPStatus.OK):
             raise OTBRError(f"unexpected http status {response.status}")
 
+    async def set_pending_dataset_tlvs(
+        self, dataset: bytes, *, if_match: str | None = None
+    ) -> None:
+        """Set the pending operational dataset, when none is in place yet.
+
+        A write while a pending dataset is in flight is refused outright.
+        Superseding one is never safe to do implicitly: the replacement races
+        the delay timer on every device that already holds the old dataset, so
+        a late replacement can split the mesh, and it would also silently undo
+        whatever the in-flight dataset was doing, such as a channel change.
+        A caller that finds the write refused should surface that to whoever
+        asked for it, not retry.
+
+        The check runs locally first, and again on the border router for one
+        that honors If-None-Match on this endpoint
+        (https://github.com/openthread/ot-br-posix/pull/3552), where it is
+        atomic with the write; older border routers ignore the header.
+
+        The one deliberate way to replace an in-flight dataset is by its
+        entity tag: a caller that read it via
+        get_pending_dataset_tlvs_with_etag() passes the tag as if_match, and
+        the write then only replaces exactly the dataset that was read,
+        refused if it changed in between. That check happens on the border
+        router, atomically with the write, so no local check runs on this
+        path; an older border router that hands out no entity tags ignores
+        the header, making the replace there unconditional, which is why a
+        caller must only offer this as an explicit, informed choice.
+
+        Raises PendingDatasetConflictError when any of these checks refuses
+        the write, and OTBRError if the http status is 400 or higher for any
+        other reason or the response is invalid.
+        """
+        if if_match is None:
+            if await self.get_pending_dataset_tlvs() is not None:
+                raise PendingDatasetConflictError(
+                    "a pending dataset is already in place"
+                )
+        await self._maybe_detect_key_format()
+        headers = {"Content-Type": "text/plain"}
+        if if_match is not None:
+            headers["If-Match"] = if_match
+        else:
+            headers["If-None-Match"] = "*"
+        response = await self._session.put(
+            f"{self._url}/node/dataset/pending",
+            data=dataset.hex(),
+            headers=headers,
+            timeout=aiohttp.ClientTimeout(total=10),
+        )
+
+        if response.status == HTTPStatus.CONFLICT:
+            raise ThreadNetworkActiveError
+        if response.status == HTTPStatus.PRECONDITION_FAILED:
+            if if_match is not None:
+                raise PendingDatasetConflictError(
+                    "the pending dataset changed since it was read"
+                )
+            raise PendingDatasetConflictError("a pending dataset is already in place")
+        if response.status not in (HTTPStatus.CREATED, HTTPStatus.OK):
+            raise OTBRError(f"unexpected http status {response.status}")
+
     async def set_channel(
         self, channel: int, delay: int = PENDING_DATASET_DELAY_TIMER
     ) -> None:

python_otbr_api/tlv_parser.py

@@ -116,11 +116,62 @@ def __post_init__(self) -> None:
         """Decode the timestamp."""
         # The timestamps are packed in 8 bytes:
         # [seconds 48 bits][ticks 15 bits][authoritative flag 1 bit]
-        unpacked: int = struct.unpack("!Q", self.data)[0]
+        try:
+            unpacked: int = struct.unpack("!Q", self.data)[0]
+        except struct.error as err:
+            raise TLVError(f"invalid timestamp '{self.data.hex()}'") from err
         self.authoritative = bool(unpacked & 1)
         self.seconds = unpacked >> 16
         self.ticks = (unpacked >> 1) & 0x7FFF
 
+    @classmethod
+    def from_values(
+        cls,
+        tag: MeshcopTLVType,
+        seconds: int,
+        ticks: int = 0,
+        authoritative: bool = False,
+    ) -> Timestamp:
+        """Construct a timestamp from its field values.
+
+        Raises TLVError if seconds or ticks don't fit the wire format.
+        """
+        if not 0 <= seconds < 2**48:
+            raise TLVError(f"timestamp seconds out of range: {seconds}")
+        if not 0 <= ticks < 2**15:
+            raise TLVError(f"timestamp ticks out of range: {ticks}")
+        packed = (seconds << 16) | (ticks << 1) | int(authoritative)
+        return cls(tag, struct.pack("!Q", packed))
+
+
+@dataclass
+class DelayTimer(MeshcopTLVItem):
+    """Delay timer."""
+
+    delay: int = field(init=False)
+
+    def __post_init__(self) -> None:
+        """Decode the delay in milliseconds.
+
+        Raises TLVError if the data is not the four bytes the wire format
+        defines, so a malformed delay timer is rejected instead of being
+        accepted at whatever width it arrived with.
+        """
+        try:
+            self.delay = struct.unpack("!L", self.data)[0]
+        except struct.error as err:
+            raise TLVError(f"invalid delay timer '{self.data.hex()}'") from err
+
+    @classmethod
+    def from_milliseconds(cls, delay: int) -> DelayTimer:
+        """Construct a delay timer from a delay in milliseconds.
+
+        Raises TLVError if the delay doesn't fit the wire format.
+        """
+        if not 0 <= delay < 2**32:
+            raise TLVError(f"delay timer out of range: {delay}")
+        return cls(MeshcopTLVType.DELAYTIMER, struct.pack("!L", delay))
+
 
 def _encode_item(item: MeshcopTLVItem) -> bytes:
     """Encode a dataset item to TLV format."""
@@ -143,10 +194,12 @@ def encode_tlv(items: dict[MeshcopTLVType | int, MeshcopTLVItem]) -> str:
 
 def _parse_item(tag: MeshcopTLVType | int, data: bytes) -> MeshcopTLVItem:
     """Parse a TLV encoded dataset item."""
-    if tag == MeshcopTLVType.ACTIVETIMESTAMP:
+    if tag in (MeshcopTLVType.ACTIVETIMESTAMP, MeshcopTLVType.PENDINGTIMESTAMP):
         return Timestamp(tag, data)
     if tag == MeshcopTLVType.CHANNEL:
         return Channel(tag, data)
+    if tag == MeshcopTLVType.DELAYTIMER:
+        return DelayTimer(tag, data)
     if tag == MeshcopTLVType.NETWORKNAME:
         return NetworkName(tag, data)
 
@@ -190,9 +243,10 @@ def parse_tlv(data: str) -> dict[MeshcopTLVType | int, MeshcopTLVItem]:
         val = data_bytes[pos : pos + _len]
         pos += _len
 
-        # Once we have the value, we can log a warning about the unknown TLV
+        # Log unknown TLVs by type and length only: the value may hold
+        # network credentials, which don't belong in the log.
         if not isinstance(tag, MeshcopTLVType):
-            _LOGGER.warning("unknown TLV type %d=%r", raw_tag, val)
+            _LOGGER.warning("unknown TLV type %d (%d bytes)", raw_tag, _len)
 
         if tag in result:
             raise TLVError(f"duplicated tag {tag!r}")

tests/test_init_legacy.py

@@ -790,3 +790,174 @@ async def test_get_coprocessor_version_invalid(aioclient_mock: AiohttpClientMock
 
     with pytest.raises(python_otbr_api.OTBRError):
         await otbr.get_coprocessor_version()
+
+
+async def test_set_pending_dataset_tlvs(aioclient_mock: AiohttpClientMocker) -> None:
+    """Test set_pending_dataset_tlvs."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    dataset = bytes.fromhex(
+        "0E080000000000010000000300000F35060004001FFFE0020811111111222222220708FDAD"
+        "70BFE5AA15DD051000112233445566778899AABBCCDDEEFF030E4F70656E54687265616444"
+        "656D6F010212340410445F2B5CA6F2A93A55CE570A70EFEECB0C0402A0F7F8"
+    )
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT)
+    aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.CREATED)
+
+    await otbr.set_pending_dataset_tlvs(dataset)
+    assert aioclient_mock.call_count == 2
+    assert aioclient_mock.mock_calls[-1][0] == "PUT"
+    assert aioclient_mock.mock_calls[-1][1].path == "/node/dataset/pending"
+    assert aioclient_mock.mock_calls[-1][2] == dataset.hex()
+    assert aioclient_mock.mock_calls[-1][3]["If-None-Match"] == "*"
+
+
+async def test_set_pending_dataset_tlvs_thread_active(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test set_pending_dataset_tlvs with error."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT)
+    aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.CONFLICT)
+
+    with pytest.raises(python_otbr_api.ThreadNetworkActiveError):
+        await otbr.set_pending_dataset_tlvs(b"")
+
+
+async def test_set_pending_dataset_tlvs_refused_while_pending(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test a write is refused while a pending dataset is in place."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    in_flight = "0E080000000000010000340400006699000300000C"
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", text=in_flight)
+
+    with pytest.raises(python_otbr_api.PendingDatasetConflictError):
+        await otbr.set_pending_dataset_tlvs(bytes.fromhex(in_flight))
+    assert aioclient_mock.call_count == 1
+
+
+async def test_set_pending_dataset_tlvs_refused_by_router(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test the border router refusing the precondition is surfaced."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT)
+    aioclient_mock.put(
+        f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.PRECONDITION_FAILED
+    )
+
+    # The dedicated type, so a caller can tell "nothing was written because a
+    # dataset is in place" from a transport or protocol failure.
+    with pytest.raises(python_otbr_api.PendingDatasetConflictError):
+        await otbr.set_pending_dataset_tlvs(b"")
+
+
+async def test_get_pending_dataset_tlvs_with_etag(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test get_pending_dataset_tlvs_with_etag."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    mock_response = "0E080000000000010000340400006699000300000C"
+    aioclient_mock.get(
+        f"{BASE_URL}/node/dataset/pending",
+        text=mock_response,
+        headers={"ETag": '"8311BDCD94E7107C"'},
+    )
+
+    assert await otbr.get_pending_dataset_tlvs_with_etag() == (
+        bytes.fromhex(mock_response),
+        '"8311BDCD94E7107C"',
+    )
+
+
+async def test_get_pending_dataset_tlvs_with_etag_no_tag(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test the tag is None on a border router that hands out none."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    mock_response = "0E080000000000010000340400006699000300000C"
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", text=mock_response)
+
+    assert await otbr.get_pending_dataset_tlvs_with_etag() == (
+        bytes.fromhex(mock_response),
+        None,
+    )
+
+
+async def test_get_pending_dataset_tlvs_with_etag_empty(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test get_pending_dataset_tlvs_with_etag without a pending dataset."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT)
+    assert await otbr.get_pending_dataset_tlvs_with_etag() is None
+
+
+async def test_set_pending_dataset_tlvs_if_match(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test if_match makes the replace conditional on the tag, with no local check."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    dataset = bytes.fromhex("0E080000000000010000340400006699000300000C")
+    aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.OK)
+
+    await otbr.set_pending_dataset_tlvs(dataset, if_match='"8311BDCD94E7107C"')
+    assert aioclient_mock.call_count == 1
+    assert aioclient_mock.mock_calls[-1][0] == "PUT"
+    assert aioclient_mock.mock_calls[-1][3]["If-Match"] == '"8311BDCD94E7107C"'
+    assert "If-None-Match" not in aioclient_mock.mock_calls[-1][3]
+
+
+async def test_set_pending_dataset_tlvs_if_match_changed(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test a conditional replace of a dataset that changed is surfaced."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    aioclient_mock.put(
+        f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.PRECONDITION_FAILED
+    )
+
+    with pytest.raises(python_otbr_api.PendingDatasetConflictError):
+        await otbr.set_pending_dataset_tlvs(b"", if_match='"8311BDCD94E7107C"')
+
+
+async def test_set_pending_dataset_tlvs_202(
+    aioclient_mock: AiohttpClientMocker,
+) -> None:
+    """Test set_pending_dataset_tlvs with error."""
+    otbr = python_otbr_api.OTBR(
+        BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE
+    )
+
+    aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT)
+    aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.ACCEPTED)
+
+    with pytest.raises(python_otbr_api.OTBRError):
+        await otbr.set_pending_dataset_tlvs(b"")