diff --git a/.kiro/specs/admin-config-tool/tasks.md b/.kiro/specs/admin-config-tool/tasks.md index 8abde439..32de582f 100644 --- a/.kiro/specs/admin-config-tool/tasks.md +++ b/.kiro/specs/admin-config-tool/tasks.md @@ -66,7 +66,7 @@ - _Boundary: xceadmin.build_ - _Depends: 2.1, 2.3_ -- [ ] 3.2 (P) Follow controller redirects to find the primary +- [x] 3.2 (P) Follow controller redirects to find the primary - A reply pointing at a different host/port is followed automatically, up to a bounded number of hops - Exceeding the hop limit or waiting past the connection timeout produces a clear error naming the last host contacted - Observable: a chain of up to three redirects resolves to a final destination without operator intervention @@ -151,3 +151,4 @@ - **2.3**: `map_all_sections()` (a `SECTION_MAP`-driven orchestrator, not explicitly named in design.md's Service Interface but consistent with its Contracts) is the single call `xceadmin.build` (task 3.1) needs — it treats a missing YAML file identically to a present-but-empty section (byte-for-byte), satisfying Requirement 1.5 structurally. `target.init(field, 0)` and never touching an empty/absent list field produce *different* serialized bytes for the same logical "nothing here" value (verified empirically) — the mapper always picks "never touch" for byte-determinism (9.1). Calling `.to_bytes()` twice on the same capnp builder triggers a spurious "message already written" `UserWarning` even when no field was mutated between calls — capture the bytes once and reuse them rather than calling `.to_bytes()` again. - **2.4**: `errors.py`'s exception subclasses (task 1.2) all take `message` as an explicit first constructor argument in addition to their structured fields (e.g. `ConnectionTimeoutError(message, host, port)`, `ReplyDecodeError(message, reason)`) — design.md's abbreviated `Service Interface` type stubs (`class ConnectionTimeoutError(XceAdminError): host: str; port: int`) read as if only the structured fields were needed; always check the real constructor in `errors.py`, not the design doc's shorthand. `xcecommon.wire` is deliberately opcode-agnostic — `send_request`/`send_frame`/`recv_frame` move raw `bytes` only and never import a capnp type; callers own `.to_bytes()`/`.from_bytes()`. Reused `ha/data/xce.crt`/`xce.key` (the repo's existing dev cert pair, same one `deploy-ha.sh` already copies for controller TLS) as the test fixture for both the TLS test server and the client `cert_dir` — no synthetic cert generation needed. - **3.1**: Requirement 1.7's "unrecognized-but-schema-valid field" warning is real and fires on essentially every real build using `src/configs/resources.yaml` — `resources.yaml`'s `type` key is *required* by `resources.schema.json` (`required: [name, type]`) but has no `ClusterConfig` destination (per `xcecommon.mapping`'s documented decision not to guess the `valtype`/`ordtype`/`flags` integer encoding), so every valid resource entry triggers a "field 'type' ... ignored" warning unconditionally, not just for unusual input. This is a disclosed spec tradeoff, not a bug — flagged here so a future pass over `xceadmin.build`'s warning UX (e.g. only warning once per field-name per run instead of once per item) knows the noise is expected, not a regression. `xceadmin.build`'s unmapped-field table (`_UNMAPPED_BUT_VALID_ITEM_KEYS`) was derived by cross-referencing `xcecommon.mapping`'s per-item builder functions against the real `src/configs/schema/*.schema.json` property lists directly (not from prose alone) to avoid drift. +- **3.2**: design.md's Dependencies list for `xceadmin.discovery` names only `xcecommon.wire`, but decoding a reply to inspect `RequestReply.redirectHost`/`.redirectPort` genuinely requires `xcecommon.schema` too (verified real field names against `controller.capnp:594-595`, which matched design.md exactly this time — unlike task 2.2's `Reply`/`RequestReply` struct-name slip). Treated as a design-doc omission, not license to hand-roll capnp decoding. `follow_redirects`'s `cert_dir` parameter is accepted for signature parity with design.md but unused internally — all I/O, including TLS setup, is delegated to the caller's `send_fn` closure, which is expected to already carry its own `cert_dir`. A connection timeout to any hop (including a non-initial one reached via redirect) raises `NoPrimaryError` naming that specific hop, not the original target. diff --git a/src/xceadmin/src/xceadmin/discovery.py b/src/xceadmin/src/xceadmin/discovery.py new file mode 100644 index 00000000..3b585022 --- /dev/null +++ b/src/xceadmin/src/xceadmin/discovery.py @@ -0,0 +1,90 @@ +"""Generic, opcode-agnostic redirect-following for xceadmin (design.md, +``xceadmin.discovery``; Requirements 3.1, 3.2, 3.3, 3.4). + +Implemented against the ``RequestReply`` schema shape only (verified real +field names: ``redirectHost``/``redirectPort``, ``controller.capnp:594-595``) +-- never against ``OP_upgradeConfig`` specifically, so this is correct +today for ``OPCode_join``-style redirects and automatically correct for +``upgradeConfig`` once the controller wires ``fill_redirect_reply()`` for +that opcode too (design.md, Simplification). + +Note: design.md's Dependencies list for this component names only +``xcecommon.wire``, but decoding ``RequestReply`` to inspect the redirect +fields genuinely requires ``xcecommon.schema`` too -- treated as a design +doc omission, not a license to hand-roll capnp decoding here. +""" + +from __future__ import annotations + +import sys + +from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, TooManyRedirectsError +from xcecommon.schema import load_schema + +#: "Follow at most 3 redirect hops before reporting an error" (3.2) -- up +#: to 3 hops are followed; a 4th redirect is the error. +_MAX_REDIRECT_HOPS = 3 + + +def _redirect_target(reply_bytes: bytes) -> tuple[str, int] | None: + """``None`` when ``reply_bytes`` carries no redirect (empty + ``redirectHost`` or zero ``redirectPort``) -- both must be non-empty + for this to be a real redirect (3.1). + """ + mod = load_schema() + with mod.RequestReply.from_bytes(reply_bytes) as reply: + host, port = reply.redirectHost, reply.redirectPort + if host and port: + return host, port + return None + + +def follow_redirects( + initial_host: str, + initial_port: int, + cert_dir: str, # noqa: ARG001 (signature parity with design.md; all + # I/O, including TLS setup, is delegated to send_fn's own closure) + send_fn, +) -> tuple[str, int, bytes]: + """Send via ``send_fn(host, port) -> reply_bytes``, following up to 3 + redirects (3.2) before giving up. Returns ``(final_host, final_port, + reply_bytes)`` for whichever node actually answered without + redirecting. + + A connection timeout while contacting any hop (3.4, delegated to + ``xcecommon.wire.connect``'s own 30s-per-attempt timeout) is + interpreted here as "no primary available" and raised as + :class:`NoPrimaryError`, naming the last host contacted. + + Raises :class:`TooManyRedirectsError` after the 4th redirect, + :class:`NoPrimaryError` on a connection timeout to any hop. + """ + host, port = initial_host, initial_port + hops = 0 + while True: + try: + reply_bytes = send_fn(host, port) + except ConnectionTimeoutError as exc: + raise NoPrimaryError( + f"no primary available: timed out contacting {host}:{port}", host, port + ) from exc + + target = _redirect_target(reply_bytes) + if target is None: + return host, port, reply_bytes + + hops += 1 + if hops > _MAX_REDIRECT_HOPS: + raise TooManyRedirectsError( + f"too many redirects (> {_MAX_REDIRECT_HOPS}) starting from " + f"{initial_host}:{initial_port}, last contacted {host}:{port}", + hops, + ) + + redirect_host, redirect_port = target + # Requirement 3.3: display the redirect target -- most visibly + # relevant when a standby redirects to the primary, but every hop + # is displayed identically since this module has no notion of + # which node is a standby vs. primary (opcode/role-agnostic). + print(f"redirected: {host}:{port} -> {redirect_host}:{redirect_port}", file=sys.stderr) + host, port = redirect_host, redirect_port diff --git a/src/xceadmin/tests/test_discovery.py b/src/xceadmin/tests/test_discovery.py new file mode 100644 index 00000000..282b9e55 --- /dev/null +++ b/src/xceadmin/tests/test_discovery.py @@ -0,0 +1,140 @@ +"""Unit tests for ``xceadmin.discovery`` (design.md, ``xceadmin.discovery``; +Requirements 3.1, 3.2, 3.3, 3.4). +""" + +from __future__ import annotations + +import pytest + +from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, TooManyRedirectsError +from xcecommon.schema import load_schema +from xceadmin import discovery + + +def _reply_bytes(redirect_host: str = "", redirect_port: int = 0) -> bytes: + mod = load_schema() + msg = mod.RequestReply.new_message() + if redirect_host: + msg.redirectHost = redirect_host + if redirect_port: + msg.redirectPort = redirect_port + return msg.to_bytes() + + +def test_follow_redirects_no_redirect_returns_first_reply_unchanged(): + reply = _reply_bytes() + calls = [] + + def send_fn(host, port): + calls.append((host, port)) + return reply + + final_host, final_port, reply_bytes = discovery.follow_redirects( + "node1", 9991, "ha/data", send_fn + ) + + assert (final_host, final_port) == ("node1", 9991) + assert reply_bytes == reply + assert calls == [("node1", 9991)] + + +def test_follow_redirects_follows_one_redirect(capsys): + first_reply = _reply_bytes(redirect_host="node2", redirect_port=9991) + second_reply = _reply_bytes() + responses = {("node1", 9991): first_reply, ("node2", 9991): second_reply} + calls = [] + + def send_fn(host, port): + calls.append((host, port)) + return responses[(host, port)] + + final_host, final_port, reply_bytes = discovery.follow_redirects( + "node1", 9991, "ha/data", send_fn + ) + + assert (final_host, final_port) == ("node2", 9991) + assert reply_bytes == second_reply + assert calls == [("node1", 9991), ("node2", 9991)] + + stderr = capsys.readouterr().err + assert "node1:9991 -> node2:9991" in stderr + + +def test_follow_redirects_follows_exactly_three_hops(capsys): + replies = { + ("a", 1): _reply_bytes("b", 2), + ("b", 2): _reply_bytes("c", 3), + ("c", 3): _reply_bytes("d", 4), + ("d", 4): _reply_bytes(), # final answer, no more redirects + } + calls = [] + + def send_fn(host, port): + calls.append((host, port)) + return replies[(host, port)] + + final_host, final_port, _ = discovery.follow_redirects("a", 1, "ha/data", send_fn) + + assert (final_host, final_port) == ("d", 4) + assert calls == [("a", 1), ("b", 2), ("c", 3), ("d", 4)] + assert capsys.readouterr().err.count("redirected:") == 3 + + +def test_follow_redirects_raises_too_many_redirects_on_fourth_hop(): + replies = { + ("a", 1): _reply_bytes("b", 2), + ("b", 2): _reply_bytes("c", 3), + ("c", 3): _reply_bytes("d", 4), + ("d", 4): _reply_bytes("e", 5), # 4th redirect -- exceeds the limit + } + + def send_fn(host, port): + return replies[(host, port)] + + with pytest.raises(TooManyRedirectsError) as exc_info: + discovery.follow_redirects("a", 1, "ha/data", send_fn) + + assert exc_info.value.hops == 4 + + +def test_follow_redirects_raises_no_primary_error_on_connection_timeout(): + def send_fn(host, port): + raise ConnectionTimeoutError(f"timeout connecting to {host}:{port}", host, port) + + with pytest.raises(NoPrimaryError) as exc_info: + discovery.follow_redirects("node1", 9991, "ha/data", send_fn) + + assert exc_info.value.host == "node1" + assert exc_info.value.port == 9991 + + +def test_follow_redirects_no_primary_error_names_last_hop_not_initial_hop(): + replies = {("a", 1): _reply_bytes("b", 2)} + + def send_fn(host, port): + if (host, port) == ("b", 2): + raise ConnectionTimeoutError("timeout connecting to b:2", host, port) + return replies[(host, port)] + + with pytest.raises(NoPrimaryError) as exc_info: + discovery.follow_redirects("a", 1, "ha/data", send_fn) + + assert exc_info.value.host == "b" + assert exc_info.value.port == 2 + + +def test_follow_redirects_zero_port_is_not_treated_as_a_redirect(): + """redirectHost set but redirectPort left at its capnp default (0) is + not a real redirect -- both fields must be non-empty (3.1). + """ + reply = _reply_bytes(redirect_host="orphan-host", redirect_port=0) + + def send_fn(host, port): + return reply + + final_host, final_port, reply_bytes = discovery.follow_redirects( + "node1", 9991, "ha/data", send_fn + ) + + assert (final_host, final_port) == ("node1", 9991) + assert reply_bytes == reply