From 67cc361191f18d1737beb52ec872d68fee4e2164 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 22 Jul 2026 10:05:31 +0000 Subject: [PATCH] feat(admin-config-tool): task 5.6 - mock-controller push/dry-run coverage Adds end-to-end propagation tests through push_config's real composition with xceadmin.discovery (not just discovery.py's own unit tests): a 4-mock-server redirect chain proving TooManyRedirectsError surfaces correctly, and a monkeypatched-timeout test proving NoPrimaryError does too, without waiting out a real 30s socket timeout. Also locks in and documents a real wire-contract limitation: ProbeConfigTagReply carries no rejection signal, so dry-run's accepted=False path is unreachable today by design, not an oversight. This completes all 18 sub-tasks in tasks.md. --- .kiro/specs/admin-config-tool/tasks.md | 16 ++-- src/xceadmin/tests/test_config_push_mock.py | 90 ++++++++++++++++++++- 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/.kiro/specs/admin-config-tool/tasks.md b/.kiro/specs/admin-config-tool/tasks.md index a6366e42..e267906c 100644 --- a/.kiro/specs/admin-config-tool/tasks.md +++ b/.kiro/specs/admin-config-tool/tasks.md @@ -1,6 +1,6 @@ # Implementation Plan -- [ ] 1. Foundation: package scaffolding, shared errors, build/toolchain integration +- [x] 1. Foundation: package scaffolding, shared errors, build/toolchain integration - [x] 1.1 Set up the xceadmin Python package skeleton - Editable install registers a real `xceadmin` console-script entry point backed by an empty, working click group (no subcommands yet — `config build`/`config push` are added by task 4.1, not this task) - `xcecommon` is importable as a standalone package independent of `xceadmin`'s CLI @@ -21,7 +21,7 @@ - Observable: running the build command for this component from a clean checkout succeeds and reports test pass/fail - _Requirements: 5.5, 8.1, 8.2, 8.6_ -- [ ] 2. Core: xcecommon shared library +- [x] 2. Core: xcecommon shared library - [x] 2.1 (P) Validate operator-authored YAML against the bundled schemas - Each of the seven config file types can be checked against its own bundled validation schema, independent of anything the operator supplies - Reserved queue/host names are rejected even when otherwise schema-valid @@ -56,7 +56,7 @@ - _Boundary: xcecommon.wire_ - _Depends: 2.2_ -- [ ] 3. Core: xceadmin CLI orchestration +- [x] 3. Core: xceadmin CLI orchestration - [x] 3.1 (P) Compile a configs directory into a deployable cluster binary - Running the build command against a directory of YAML files produces a binary output file with a printed checksum - A missing individual YAML file is treated as an empty section, not an error @@ -82,7 +82,7 @@ - _Boundary: xceadmin.push_ - _Depends: 2.4, 3.2_ -- [ ] 4. Integration: wire the CLI entry point and hook xceadmin into cluster deployment +- [x] 4. Integration: wire the CLI entry point and hook xceadmin into cluster deployment - [x] 4.1 Expose config build and config push as a single command-line tool - `xceadmin config build` and `xceadmin config push` (with `--dry-run`) are both reachable from one installed command - Any failure surfaces as a specific, human-readable message with a non-zero exit code, not a raw traceback @@ -99,7 +99,7 @@ - _Boundary: deploy-ha.sh_ - _Depends: 4.1_ -- [ ] 5. Validation: test coverage across build, push, and packaging +- [x] 5. Validation: test coverage across build, push, and packaging - [x] 5.1 (P) Unit test the field-mapping logic in isolation - Both mapper shapes (list sections and the scalar section) are verified against in-memory input with no file I/O - The one file that feeds two separate output fields is verified explicitly @@ -128,13 +128,13 @@ - _Boundary: xceadmin.build_ - _Depends: 3.1_ -- [ ] 5.5 (P) Test every YAML validation failure mode +- [x] 5.5 (P) Test every YAML validation failure mode - Each invalid-fixture case (schema violation, reserved name, duplicate name including a cross-type collision, undefined reference) produces the documented outcome — hard failure or warning, matching the requirement - _Requirements: 4.1, 4.2, 4.3, 4.4, 4.5_ - _Boundary: xcecommon.validation_ - _Depends: 2.1_ -- [ ] 5.6 (P) Test config push and dry-run against a mock controller +- [x] 5.6 (P) Test config push and dry-run against a mock controller - A mock TLS server standing in for the controller confirms the full push flow: successful push, redirect-following, and every documented error path - The same mock confirms dry-run reports both an accepted and a rejected outcome correctly - _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_ @@ -159,3 +159,5 @@ - **5.1, 5.2**: Already fully satisfied by task 2.3's `test_mapping.py` and task 2.4's `test_wire.py` respectively — same requirement lists, same acceptance bullets (in-memory-only mapper coverage + the explicit two-field case for 5.1; a fixed known byte sequence + real loopback round-trips for 5.2). No new tests written; would have been pure duplication. - **5.3**: Found and fixed a real gap while writing `test_schema_bundling.py` — `xcecommon.validation.validate_yaml()`'s `json.loads(schema_path.read_text())` call (loading the *bundled JSON validation schema itself*, not the operator's YAML) had no error handling, so a corrupted bundled JSON schema crashed with a raw `json.JSONDecodeError` instead of a clean `ValidationError` — the same "crash vs. clean error" class of bug task 2.2 already fixed for `controller.capnp`, just on the JSON-schema side, and not caught by task 2.1 since that task's tests only exercised operator-YAML failure modes, never a corrupted *schema* file. Fixed by wrapping it and raising `ValidationError(..., constraint="schema-corrupted")`, matching the existing missing-schema handling's pattern one function above it. **Gotcha for any future task touching `xcecommon`/`xceadmin` source**: editing a source file does NOT propagate to a running `.venv` until reinstalled — always re-run `xce-build.sh xceadmin` after a source edit before trusting a bare `.venv/bin/python -m pytest` result; a stale venv (especially one left in `release`/pinned mode by a prior profile run) will silently test the *old* code and produce a false failure (or false pass) that looks like a real bug. - **5.4**: `tests/fixtures/{minimal,full}/configs/*.yaml` + checked-in `expected.bin` golden files added to `test_config_build.py` (not a new file — design.md's File Structure Plan lists the SAME requirement set for `test_config_build.py` under both 3.1 and 5.4). `full/configs/*.yaml` is a byte-for-byte copy of `src/configs/*.yaml`; its golden SHA (`860cfd4f59ef...`) independently matches the tag `deploy-ha.sh` produced in task 4.2's live verification against the same source files — an unplanned but welcome additional confirmation of Requirement 9's cross-context determinism guarantee. Golden-file tests catch representation drift (bytes change even though decoded content is still semantically correct) that purely structural/decode-based assertions elsewhere in the suite cannot detect; regenerate `expected.bin` deliberately (via `build_cluster_config()`, sanity-checked by decoding it back) whenever a real, intentional field-mapping change lands — never hand-edit the binary. +- **5.5**: Already fully satisfied by task 2.1's `test_validation.py` — every sub-case (schema violation, reserved name, cross-type duplicate collision via `test_check_duplicates_allows_limit_name_to_collide_with_queue_name`, undefined reference) already has an explicit test with the exact same requirement list. No new tests written. +- **5.6**: `probe_config()`'s dry-run has no reachable `accepted=False` path — `ProbeConfigTagReply` genuinely carries no rejection signal on the wire (see task 3.3's note). 5.6's bullet asking for both an accepted *and* a rejected outcome cannot be satisfied without inventing undocumented client-side rejection logic; instead added `test_probe_config_has_no_wire_level_rejection_signal_by_design` (parametrized over several `configTag` values) that locks in and documents the limitation explicitly rather than leaving it silently uncovered. Added two new end-to-end propagation tests through `push_config`'s real composition with `xceadmin.discovery` (not just discovery.py's own unit tests): a real 4-mock-server redirect chain proving `TooManyRedirectsError` surfaces correctly, and a `NoPrimaryError`-on-timeout test that monkeypatches `xcecommon.wire.connect` to raise `ConnectionTimeoutError` immediately rather than waiting out a real 30s socket timeout. diff --git a/src/xceadmin/tests/test_config_push_mock.py b/src/xceadmin/tests/test_config_push_mock.py index 71805b62..f5a073ad 100644 --- a/src/xceadmin/tests/test_config_push_mock.py +++ b/src/xceadmin/tests/test_config_push_mock.py @@ -22,7 +22,7 @@ from pathlib import Path import pytest from xcecommon import wire -from xcecommon.errors import ReplyDecodeError +from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, ReplyDecodeError, TooManyRedirectsError from xcecommon.schema import load_schema from xceadmin import push @@ -268,3 +268,91 @@ def test_probe_config_zero_generation_still_accepted(mock_controller, cluster_bi assert result.current_generation == 0 assert result.would_be_generation == 1 assert result.accepted is True + + +@pytest.mark.parametrize("tag_value", [0, 1, 999999]) +def test_probe_config_has_no_wire_level_rejection_signal_by_design( + mock_controller, cluster_bin, tag_value +): + """5.6 asks for both an accepted and a rejected dry-run outcome, but + ``ProbeConfigTagReply`` (module docstring, ``push.py`` module + docstring) has only a bare ``configTag`` -- no field exists on the + real wire contract to signal rejection, matching the real C + reference client (``config_repl.c``'s ``xce_probe_config_tag``, which + also returns only a bare tag). This test locks in that limitation + explicitly rather than silently having no coverage for it: every + successful round-trip, regardless of the returned ``configTag`` + value, yields ``accepted=True`` -- there is no reachable code path + that produces ``accepted=False`` today. A real "would this SHA be + accepted" check is future controller-side work (Requirement 6.1). + """ + mod = load_schema() + + def _handler(conn): + wire.recv_frame(conn) + reply = mod.ProbeConfigTagReply.new_message() + reply.configTag = tag_value + 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.accepted is True + assert result.reason == "" + + +# --------------------------------------------------------------------------- +# End-to-end redirect/discovery failure propagation through push_config +# --------------------------------------------------------------------------- + + +def test_push_config_propagates_too_many_redirects_end_to_end(mock_controller, cluster_bin): + """A real chain of 4 redirecting mock servers -- push_config's own + composition with xceadmin.discovery must surface TooManyRedirectsError, + not silently follow forever or return a bogus result. + """ + mod = load_schema() + servers = {} + + def _make_redirect_handler(next_port_getter): + def _handler(conn): + wire.recv_frame(conn) + reply = mod.RequestReply.new_message() + reply.status = 0 + reply.redirectHost = "127.0.0.1" + reply.redirectPort = next_port_getter() + reply.data.nodata = None + wire.send_frame(conn, reply.to_bytes()) + + return _handler + + # 5 servers: a -> b -> c -> d -> e (4 redirects, one too many). + names = ["a", "b", "c", "d", "e"] + for i, name in enumerate(names[:-1]): + next_name = names[i + 1] + servers[name] = mock_controller(_make_redirect_handler(lambda n=next_name: servers[n].port)) + # The last server ('e') also redirects -- this is hop #4, exceeding the + # 3-hop limit before it would ever be contacted. + servers["e"] = mock_controller(_make_redirect_handler(lambda: 1)) + + with pytest.raises(TooManyRedirectsError): + push.push_config(cluster_bin, f"127.0.0.1:{servers['a'].port}", CERT_DIR) + + +def test_push_config_propagates_no_primary_error_on_connection_timeout(cluster_bin, monkeypatch): + """xceadmin.discovery already unit-tests this conversion directly + (test_discovery.py); this proves push_config's actual composition + with it surfaces the same NoPrimaryError, without waiting out a real + 30s socket timeout -- xcecommon.wire.connect is monkeypatched to + raise ConnectionTimeoutError immediately, exactly what it would + eventually raise on a genuine timeout. + """ + + def _fake_connect(host, port, cert_dir, timeout_s=30.0): + raise ConnectionTimeoutError(f"timeout connecting to {host}:{port}", host, port) + + monkeypatch.setattr(wire, "connect", _fake_connect) + + with pytest.raises(NoPrimaryError): + push.push_config(cluster_bin, "127.0.0.1:9991", CERT_DIR)