diff --git a/.kiro/specs/admin-config-tool/spec.json b/.kiro/specs/admin-config-tool/spec.json index fb15669f..59e40dce 100644 --- a/.kiro/specs/admin-config-tool/spec.json +++ b/.kiro/specs/admin-config-tool/spec.json @@ -1,23 +1,34 @@ { "feature_name": "admin-config-tool", "created_at": "2026-07-06T01:30:00Z", - "updated_at": "2026-07-20T00:19:00Z", + "updated_at": "2026-07-22T02:35:00Z", "language": "en", - "phase": "requirements-generated", + "phase": "tasks-generated", "approvals": { "requirements": { "generated": true, - "approved": false + "approved": true }, "design": { - "generated": false, - "approved": false + "generated": true, + "approved": true }, "tasks": { - "generated": false, - "approved": false + "generated": true, + "approved": true } }, - "ready_for_implementation": false, - "blocked_on": [] + "ready_for_implementation": true, + "blocked_on": [ + "controller OP_upgradeConfig dispatch defect (cluster.c:667-679 vs ha.c:3926-3951) blocks real-cluster config push E2E validation — see research.md Risks", + "RESOLVED 2026-07-22 (bug-1461, commit e53c086): controller.capnp's struct Data shadowed the capnp builtin blob type, miscompiling UpgradeConfigRequest.sha256/.bytes — fixed at the schema root (renamed to XceData), unblocking push_config()'s request construction. The dispatch defect above is separate and still open — push_config() remains mock-tested only, no live E2E." + ], + "gitea": { + "milestone_id": 1, + "issues": { + "requirements_design": 22, + "implementation": 23, + "validation": 24 + } + } } diff --git a/.kiro/specs/admin-config-tool/tasks.md b/.kiro/specs/admin-config-tool/tasks.md index 32de582f..3e800f13 100644 --- a/.kiro/specs/admin-config-tool/tasks.md +++ b/.kiro/specs/admin-config-tool/tasks.md @@ -74,7 +74,7 @@ - _Boundary: xceadmin.discovery_ - _Depends: 2.4_ -- [ ] 3.3 Push a cluster binary to the controller, with a dry-run mode +- [x] 3.3 Push a cluster binary to the controller, with a dry-run mode - A real push reports the assigned configuration version, acknowledgement count, and any error, exiting non-zero on failure - A dry-run reports whether the controller would accept the config without ever committing it - Every connection failure, protocol-level rejection, corrupt input, or missing schema maps to a specific, distinct error @@ -152,3 +152,5 @@ - **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. +- **bug-1461 (out-of-band, between 3.2 and 3.3)**: `controller.capnp` had a real, confirmed schema bug — a locally-declared `struct Data` (an `OPCode_get` query-result tagged union) shadowed Cap'n Proto's builtin `Data` blob type for every later `: Data` field in the file, miscompiling `UpgradeConfigRequest.sha256`/`.bytes` (and 5 other fields across `ReplConfigReply`/`ReplConfigPushRequest`/`ReplEnvelope`/`SnapshotTransfer`/`WalCatchupEntry`/`CtrlHelloRequest`, several HA-critical) as a struct-pointer type instead of a byte blob — blocking `push_config()` from constructing a valid request via pycapnp. Fixed at the root (renamed the local struct to `XceData`, `$C.mapname("xce_data_t")` unchanged so the C typedef is stable) across 20 files spanning controller/lib/scheduler/Go — see commit `e53c086` and `.wolf/buglog.json` bug-1461 for the full investigation and fix. This was NOT an admin-config-tool-boundary change; it's a pre-existing, cross-component schema defect that happened to block this task. `push_config()`'s real controller counterpart remains untested live — a *separate*, already-documented dispatch-gate defect (`ha_dispatch_gate()`/`cluster.c`, spec.json `blocked_on`) still blocks that independently of bug-1461. +- **3.3**: `probe_config()` does NOT go through `xceadmin.discovery.follow_redirects` — `ProbeConfigTagReply` is a direct struct with only `configTag`, no `redirectHost`/`.Port` fields exist on it at all, so there is no redirect signal for `follow_redirects` (which unconditionally decodes replies as `RequestReply`) to read; only `push_config()`'s `Request`/`RequestReply`-wrapped exchange supports redirects. `ProbeConfigTagReply` also carries no accept/reject signal — `DryRunResult.accepted` is a client-side inference (true on any successful round-trip), matching the real C reference client (`config_repl.c`'s `xce_probe_config_tag`, which also always sends an empty `name` and returns only a bare tag). pycapnp union field assignment for Void/Text-typed variants (e.g. `reply.data.nodata = None`, `reply.data.debugmsg = "..."`) must use direct assignment, not `.init(name)` — `.init()` without a size is only valid for struct/list/data-typed variants, confirmed via `KjException: init() without a size is only valid for struct and object fields`. diff --git a/src/xceadmin/src/xceadmin/push.py b/src/xceadmin/src/xceadmin/push.py new file mode 100644 index 00000000..a5eedfb5 --- /dev/null +++ b/src/xceadmin/src/xceadmin/push.py @@ -0,0 +1,162 @@ +"""``config push`` orchestration for xceadmin (design.md, ``xceadmin.push``; +Requirements 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 6.1, 6.2, 6.3, 7.1, 7.2, +7.3, 7.4, 7.5, 7.6). + +Two functions, not one boolean-flagged function (Design Review +correction, ``research.md``): ``push_config()`` sends ``Request{opcode: +upgradeConfig, upgradeconfigrequestmsg}`` (``Request``/``RequestReply`` +wrapped) and ``probe_config()`` sends ``ProbeConfigTagRequest`` (a direct +struct, no wrapper) -- their replies share no fields, so a shared return +type would misrepresent one of the two. + +Both delegate connection + redirect-following to +``xceadmin.discovery.follow_redirects`` before decoding a reply, and both +build/decode using the real ``controller.capnp`` schema +(``xcecommon.schema.load_schema()``) -- never touching the socket +directly except through ``xcecommon.wire``. + +``probe_config()`` has no live controller counterpart today +(``research.md``, "ProbeConfigTag liveness": no ``OPCode_probeConfigTag`` +dispatch case exists in ``cluster.c``) -- implemented against the +documented wire contract, exercised only by a mock TLS server until the +controller gains a handler. The real C reference client +(``config_repl.c:1907``, ``xce_probe_config_tag``) always sends an empty +``name`` ("anonymous probe; peer logs remote addr") and returns only a +bare config tag with no accept/reject signal -- ``ProbeConfigTagReply`` +has no field to carry one. ``probe_config()`` matches this exactly: +``accepted`` is a client-side inference (true on any successful +round-trip), not a value read off the wire, since the wire contract has +nothing else to say. Real SHA/version-compatibility validation +(Requirement 6.1's aspirational description) is future controller-side +work, not something this client can check today. + +``probe_config()`` connects directly and does NOT go through +``xceadmin.discovery.follow_redirects``: ``ProbeConfigTagReply`` is a +direct struct with only a ``configTag`` field -- no ``redirectHost``/ +``redirectPort`` exist on it at all, so there is no redirect signal for +``follow_redirects`` (which decodes every reply as ``RequestReply``) to +read. Feeding ``ProbeConfigTagReply`` bytes through it would misinterpret +them as a different struct shape. Only ``push_config()``'s +``Request``/``RequestReply``-wrapped exchange supports redirects. +""" + +from __future__ import annotations + +import hashlib +from dataclasses import dataclass +from pathlib import Path + +from xcecommon import wire +from xcecommon.errors import ReplyDecodeError +from xcecommon.schema import load_schema +from xceadmin.discovery import follow_redirects + +_DEFAULT_PORT = 9991 + + +@dataclass(frozen=True) +class PushResult: + version_tag: int + acked_count: int + error: str + + +@dataclass(frozen=True) +class DryRunResult: + current_generation: int + would_be_generation: int + accepted: bool + reason: str + + +def _split_controller(controller: str) -> tuple[str, int]: + """``"host:port"`` -> ``(host, port)``; a bare host uses the default + port 9991 (Requirement 2.2's default). + """ + if ":" in controller: + host, port_str = controller.rsplit(":", 1) + return host, int(port_str) + return controller, _DEFAULT_PORT + + +def _send_via(request_bytes: bytes, cert_dir: str): + def _send(host: str, port: int) -> bytes: + sock = wire.connect(host, port, cert_dir) + try: + return wire.send_request(sock, request_bytes) + finally: + sock.close() + + return _send + + +def push_config(cluster_bin_path: str, controller: str, cert_dir: str) -> PushResult: + """Push ``cluster_bin_path`` to the controller cluster (2.1). Follows + redirects to the primary (3.1-3.4) before sending. + + Postconditions: exit 0 iff ``acked_count > 0`` and ``error == ""`` + (2.6); otherwise the caller exits non-zero with ``error`` displayed + (2.7) -- this function only reports the result, it does not exit. + """ + data = Path(cluster_bin_path).read_bytes() + sha256 = hashlib.sha256(data).digest() + + mod = load_schema() + request = mod.Request.new_message() + request.opcode = "upgradeConfig" + request.seq = 1 + upgrade = request.body.init("upgradeconfigrequestmsg") + upgrade.sha256 = sha256 + upgrade.bytes = data + request_bytes = request.to_bytes() + + host, port = _split_controller(controller) + _, _, reply_bytes = follow_redirects(host, port, cert_dir, _send_via(request_bytes, cert_dir)) + + with mod.RequestReply.from_bytes(reply_bytes) as reply: + if reply.status != 0: + return PushResult( + version_tag=0, acked_count=0, error=f"controller returned status {reply.status}" + ) + if reply.data.which() != "upgradeconfigreplymsg": + reason = f"unexpected reply variant {reply.data.which()!r}, expected upgradeconfigreplymsg" + raise ReplyDecodeError(f"invalid reply: {reason}", reason) + upgrade_reply = reply.data.upgradeconfigreplymsg + return PushResult( + version_tag=upgrade_reply.versionTag, + acked_count=upgrade_reply.ackedCount, + error=upgrade_reply.error, + ) + + +def probe_config(cluster_bin_path: str, controller: str, cert_dir: str) -> DryRunResult: + """Probe the controller's current config generation without pushing + anything (6.1). ``cluster_bin_path`` is read only to confirm it + exists and is readable before spending a network round-trip -- its + content plays no role in ``ProbeConfigTagRequest``, which carries no + config data (see module docstring). + + Postconditions: exit 0 and "OK: controller will accept this config + (generation N->M)" iff ``accepted`` (6.2); otherwise the caller exits + non-zero with ``reason`` displayed (6.3) -- this function only + reports the result, it does not exit. + """ + Path(cluster_bin_path).read_bytes() + + mod = load_schema() + request = mod.ProbeConfigTagRequest.new_message() + request.name = "" # matches config_repl.c's xce_probe_config_tag exactly + request_bytes = request.to_bytes() + + host, port = _split_controller(controller) + reply_bytes = _send_via(request_bytes, cert_dir)(host, port) + + with mod.ProbeConfigTagReply.from_bytes(reply_bytes) as reply: + current_generation = reply.configTag + + return DryRunResult( + current_generation=current_generation, + would_be_generation=current_generation + 1, + accepted=True, + reason="", + ) diff --git a/src/xceadmin/tests/test_config_push_mock.py b/src/xceadmin/tests/test_config_push_mock.py new file mode 100644 index 00000000..71805b62 --- /dev/null +++ b/src/xceadmin/tests/test_config_push_mock.py @@ -0,0 +1,270 @@ +"""Mock-controller tests for ``xceadmin.push`` (design.md, ``xceadmin.push``; +Requirements 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3, 3.4, 6.1, +6.2, 6.3, 7.1, 7.2, 7.3). + +``push_config``'s real controller counterpart is blocked on a separate, +already-documented controller-side dispatch-gate defect +(``ha_dispatch_gate()``/``cluster.c``'s inverted primary check -- +spec.json ``blocked_on``), and ``probe_config``'s ``OPCode_probeConfigTag`` +has no server dispatch case at all (``research.md``, "ProbeConfigTag +liveness"). Both are therefore tested against a real local TLS mock +server built from the actual bundled schema -- not a live cluster -- +exactly as design.md's ``xceadmin.push`` Boundary Commitments anticipate. +""" + +from __future__ import annotations + +import ssl +import socket +import threading +from pathlib import Path + +import pytest + +from xcecommon import wire +from xcecommon.errors import ReplyDecodeError +from xcecommon.schema import load_schema +from xceadmin import push + +REPO_HA_DATA_DIR = Path(__file__).resolve().parents[3] / "ha" / "data" +CERT_FILE = REPO_HA_DATA_DIR / "xce.crt" +KEY_FILE = REPO_HA_DATA_DIR / "xce.key" +CERT_DIR = str(REPO_HA_DATA_DIR) + +pytestmark = pytest.mark.skipif( + not (CERT_FILE.is_file() and KEY_FILE.is_file()), + reason="ha/data/xce.crt or ha/data/xce.key not present", +) + + +def _server_context() -> ssl.SSLContext: + context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + context.load_cert_chain(certfile=str(CERT_FILE), keyfile=str(KEY_FILE)) + return context + + +class _MockController: + """A one-shot TLS server: accepts one connection, hands the raw + accepted TLS socket to ``handler``, then closes. + """ + + def __init__(self, handler): + self._handler = handler + self._listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + self._listener.bind(("127.0.0.1", 0)) + self._listener.listen(1) + self.port = self._listener.getsockname()[1] + self._thread = threading.Thread(target=self._run, daemon=True) + self._thread.start() + + def _run(self) -> None: + try: + raw_conn, _ = self._listener.accept() + except OSError: + return + try: + tls_conn = _server_context().wrap_socket(raw_conn, server_side=True) + except (ssl.SSLError, OSError): + raw_conn.close() + return + try: + self._handler(tls_conn) + except OSError: + pass + finally: + tls_conn.close() + + def close(self) -> None: + self._listener.close() + self._thread.join(timeout=5) + + +@pytest.fixture +def mock_controller(): + servers: list[_MockController] = [] + + def _make(handler) -> _MockController: + srv = _MockController(handler) + servers.append(srv) + return srv + + yield _make + for srv in servers: + srv.close() + + +@pytest.fixture +def cluster_bin(tmp_path): + path = tmp_path / "cluster.bin" + path.write_bytes(b"fake cluster config binary contents") + return str(path) + + +# --------------------------------------------------------------------------- +# push_config(): happy path, redirect-following, error mapping +# --------------------------------------------------------------------------- + + +def test_push_config_successful_commit(mock_controller, cluster_bin): + mod = load_schema() + received = {} + + def _handler(conn): + request_bytes = wire.recv_frame(conn) + received["request_bytes"] = request_bytes + with mod.Request.from_bytes(request_bytes) as req: + received["opcode"] = req.opcode + received["sha256"] = bytes(req.body.upgradeconfigrequestmsg.sha256) + received["bytes"] = bytes(req.body.upgradeconfigrequestmsg.bytes) + + reply = mod.RequestReply.new_message() + reply.status = 0 + upgrade_reply = reply.data.init("upgradeconfigreplymsg") + upgrade_reply.versionTag = 7 + upgrade_reply.ackedCount = 3 + upgrade_reply.error = "" + wire.send_frame(conn, reply.to_bytes()) + + srv = mock_controller(_handler) + + result = push.push_config(cluster_bin, f"127.0.0.1:{srv.port}", CERT_DIR) + + assert result.version_tag == 7 + assert result.acked_count == 3 + assert result.error == "" + assert received["opcode"] == "upgradeConfig" + assert received["bytes"] == Path(cluster_bin).read_bytes() + import hashlib + + assert received["sha256"] == hashlib.sha256(Path(cluster_bin).read_bytes()).digest() + + +def test_push_config_follows_one_redirect_to_primary(mock_controller, cluster_bin): + mod = load_schema() + + def _standby_handler(conn): + wire.recv_frame(conn) + reply = mod.RequestReply.new_message() + reply.status = 0 + reply.redirectHost = "127.0.0.1" + # port filled in once the primary server exists, see below + reply.redirectPort = _standby_handler.primary_port + reply.data.nodata = None + wire.send_frame(conn, reply.to_bytes()) + + def _primary_handler(conn): + wire.recv_frame(conn) + reply = mod.RequestReply.new_message() + reply.status = 0 + upgrade_reply = reply.data.init("upgradeconfigreplymsg") + upgrade_reply.versionTag = 9 + upgrade_reply.ackedCount = 2 + upgrade_reply.error = "" + wire.send_frame(conn, reply.to_bytes()) + + primary_srv = mock_controller(_primary_handler) + _standby_handler.primary_port = primary_srv.port + standby_srv = mock_controller(_standby_handler) + + result = push.push_config(cluster_bin, f"127.0.0.1:{standby_srv.port}", CERT_DIR) + + assert result.version_tag == 9 + assert result.acked_count == 2 + + +def test_push_config_non_zero_status_reports_error_not_exception(mock_controller, cluster_bin): + mod = load_schema() + + def _handler(conn): + wire.recv_frame(conn) + reply = mod.RequestReply.new_message() + reply.status = 1 + reply.data.debugmsg = "simulated failure" + wire.send_frame(conn, reply.to_bytes()) + + srv = mock_controller(_handler) + + result = push.push_config(cluster_bin, f"127.0.0.1:{srv.port}", CERT_DIR) + + assert result.acked_count == 0 + assert "status 1" in result.error + + +def test_push_config_wrong_reply_variant_raises_reply_decode_error(mock_controller, cluster_bin): + mod = load_schema() + + def _handler(conn): + wire.recv_frame(conn) + reply = mod.RequestReply.new_message() + reply.status = 0 + reply.data.nodata = None # not upgradeconfigreplymsg + wire.send_frame(conn, reply.to_bytes()) + + srv = mock_controller(_handler) + + with pytest.raises(ReplyDecodeError): + push.push_config(cluster_bin, f"127.0.0.1:{srv.port}", CERT_DIR) + + +def test_push_config_default_port_used_when_omitted(): + host, port = push._split_controller("somehost") + assert host == "somehost" + assert port == 9991 + + +def test_push_config_explicit_port_parsed(): + host, port = push._split_controller("somehost:1234") + assert host == "somehost" + assert port == 1234 + + +# --------------------------------------------------------------------------- +# probe_config(): dry-run against the mock controller +# --------------------------------------------------------------------------- + + +def test_probe_config_reports_current_and_would_be_generation(mock_controller, cluster_bin): + mod = load_schema() + received = {} + + def _handler(conn): + request_bytes = wire.recv_frame(conn) + received["request_bytes"] = request_bytes + with mod.ProbeConfigTagRequest.from_bytes(request_bytes) as req: + received["name"] = req.name + + reply = mod.ProbeConfigTagReply.new_message() + reply.configTag = 41 + wire.send_frame(conn, reply.to_bytes()) + + srv = mock_controller(_handler) + + result = push.probe_config(cluster_bin, f"127.0.0.1:{srv.port}", CERT_DIR) + + assert result.current_generation == 41 + assert result.would_be_generation == 42 + assert result.accepted is True + assert result.reason == "" + assert received["name"] == "" # matches config_repl.c's xce_probe_config_tag exactly + + +def test_probe_config_zero_generation_still_accepted(mock_controller, cluster_bin): + """A fresh cluster with no config committed yet (configTag == 0) is + still a successful probe -- there is no rejection signal on this + wire shape (module docstring); the caller decides significance. + """ + mod = load_schema() + + def _handler(conn): + wire.recv_frame(conn) + reply = mod.ProbeConfigTagReply.new_message() + reply.configTag = 0 + wire.send_frame(conn, reply.to_bytes()) + + srv = mock_controller(_handler) + + result = push.probe_config(cluster_bin, f"127.0.0.1:{srv.port}", CERT_DIR) + + assert result.current_generation == 0 + assert result.would_be_generation == 1 + assert result.accepted is True