From 6eab344c6e1f3f8bb0cdcf937b3c2674b81888c4 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 3 Oct 2026 07:31:38 +0000 Subject: [PATCH] feat(controller-peers-env-fallback): accept name= prefixes and report malformed controller entries cleanly (task 4.1) Co-Authored-By: Claude Opus 5.5 (1M context) --- .../controller-peers-env-fallback/tasks.md | 2 +- src/xceadmin/src/xceadmin/discovery.py | 36 +++++++++++++--- src/xceadmin/src/xcecommon/errors.py | 16 +++++++ src/xceadmin/tests/test_discovery.py | 42 ++++++++++++++++++- 4 files changed, 89 insertions(+), 7 deletions(-) diff --git a/.kiro/specs/controller-peers-env-fallback/tasks.md b/.kiro/specs/controller-peers-env-fallback/tasks.md index c20f06bf..3e557597 100644 --- a/.kiro/specs/controller-peers-env-fallback/tasks.md +++ b/.kiro/specs/controller-peers-env-fallback/tasks.md @@ -71,7 +71,7 @@ - _Requirements: 1.1, 1.2, 1.3, 1.4, 1.5, 1.7_ - [ ] 4. xceadmin -- [ ] 4.1 (P) Accept `name=` prefixes and report malformed entries cleanly when parsing controller candidates +- [x] 4.1 (P) Accept `name=` prefixes and report malformed entries cleanly when parsing controller candidates - Write the failing tests first: a single `name=host:port`; a mixed named/bare list keeping its order; a non-numeric port raising the new controller-value error that quotes the entry; existing bare-host and list cases unchanged - Add the controller-value error next to the existing controller-reachability errors - Strip everything up to the first `=` in each entry before the existing host/port split; turn the failing port conversion into the new error diff --git a/src/xceadmin/src/xceadmin/discovery.py b/src/xceadmin/src/xceadmin/discovery.py index efa7d99e..ed936ec0 100644 --- a/src/xceadmin/src/xceadmin/discovery.py +++ b/src/xceadmin/src/xceadmin/discovery.py @@ -35,7 +35,13 @@ import sys import capnp -from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, ReplyDecodeError, TooManyRedirectsError +from xcecommon.errors import ( + ConnectionTimeoutError, + ControllerValueError, + NoPrimaryError, + ReplyDecodeError, + TooManyRedirectsError, +) from xcecommon.schema import load_schema #: "Follow at most 3 redirect hops before reporting an error" (3.2) -- up @@ -67,17 +73,37 @@ def resolve_controllers(controller: str) -> list[tuple[str, int]]: Whitespace around each comma-separated entry is stripped and empty entries (e.g. a stray trailing comma) are dropped, so ``"a:1, b:2,"`` parses the same as ``"a:1,b:2"``. + + Each entry may carry an optional ``name=`` prefix (``"ctrl-1=host:port"``), + the ``xce_parse_peers()`` form ``XCE_CONTROLLER_PEERS`` uses: everything + up to and including the entry's first ``=`` is discarded before the + host/port split, so ``"a=h1:1, h2:2,c=h3"`` parses to + ``[("h1", 1), ("h2", 2), ("h3", 9991)]``. This applies to every caller, + an explicit ``--controller`` value included. No host validation is added: + ``"a=:5"`` parses exactly like ``":5"`` always has. + + Raises :class:`ControllerValueError` for an entry whose port is not an + integer (``"h:x"``, ``"a=h:"``); the message quotes the entry as the user + wrote it, ``name=`` prefix included, so it can be found in the value. """ candidates: list[tuple[str, int]] = [] for part in controller.split(","): part = part.strip() if not part: continue - if ":" in part: - host, port_str = part.rsplit(":", 1) - candidates.append((host, int(port_str))) + address = part.split("=", 1)[1] if "=" in part else part + if ":" in address: + host, port_str = address.rsplit(":", 1) + try: + port = int(port_str) + except ValueError: + raise ControllerValueError( + f"invalid --controller entry '{part}': port '{port_str}' is not an integer", + entry=part, + ) from None + candidates.append((host, port)) else: - candidates.append((part, _DEFAULT_PORT)) + candidates.append((address, _DEFAULT_PORT)) return candidates diff --git a/src/xceadmin/src/xcecommon/errors.py b/src/xceadmin/src/xcecommon/errors.py index 354040c7..480af1c7 100644 --- a/src/xceadmin/src/xcecommon/errors.py +++ b/src/xceadmin/src/xcecommon/errors.py @@ -115,6 +115,22 @@ class TooManyRedirectsError(XceAdminError): self.hops = hops +class ControllerValueError(XceAdminError): + """One comma-separated entry of a ``--controller``-shaped value + (``[name=]host[:port]``) is malformed, e.g. its port is not an integer. + + controller-peers-env-fallback Requirement 2.8 (raised by + ``xceadmin.discovery.resolve_controllers``): report "invalid + --controller entry ''" instead of an uncaught ``ValueError``. + ``entry`` is the offending entry as the user wrote it (``name=`` + included, surrounding whitespace stripped). + """ + + def __init__(self, message: str, entry: str) -> None: + super().__init__(message) + self.entry = entry + + class NoPrimaryError(XceAdminError): """No primary controller could be reached within the discovery timeout. diff --git a/src/xceadmin/tests/test_discovery.py b/src/xceadmin/tests/test_discovery.py index 9d2660dc..8bd40463 100644 --- a/src/xceadmin/tests/test_discovery.py +++ b/src/xceadmin/tests/test_discovery.py @@ -8,7 +8,13 @@ import socket import pytest -from xcecommon.errors import ConnectionTimeoutError, NoPrimaryError, ReplyDecodeError, TooManyRedirectsError +from xcecommon.errors import ( + ConnectionTimeoutError, + ControllerValueError, + NoPrimaryError, + ReplyDecodeError, + TooManyRedirectsError, +) from xcecommon.schema import load_schema from xceadmin import discovery @@ -261,6 +267,40 @@ def test_resolve_controllers_strips_whitespace_and_drops_empty_entries(): ] +def test_resolve_controllers_strips_name_prefix_from_single_entry(): + # XCE_CONTROLLER_PEERS uses xce_parse_peers()'s "name=host:port" form; + # the name is the controller's Raft identity, not part of the address. + assert discovery.resolve_controllers("ctrl-1=somehost:1234") == [("somehost", 1234)] + + +def test_resolve_controllers_mixed_named_and_bare_entries_keep_order(): + assert discovery.resolve_controllers("a=h1:1, h2:2,c=h3") == [ + ("h1", 1), + ("h2", 2), + ("h3", 9991), + ] + + +def test_resolve_controllers_strips_only_up_to_the_first_equals_sign(): + assert discovery.resolve_controllers("a=b=h:5") == [("b=h", 5)] + + +def test_resolve_controllers_empty_host_after_name_passes_through_like_a_bare_entry(): + # "a=:5" parses exactly like ":5" does today: the name= stripping adds + # no host validation of its own. + assert discovery.resolve_controllers("a=:5") == discovery.resolve_controllers(":5") == [("", 5)] + + +@pytest.mark.parametrize("entry", ["a=h:x", "h:x", "ctrl-2=h:"]) +def test_resolve_controllers_non_numeric_port_raises_controller_value_error(entry): + with pytest.raises(ControllerValueError) as excinfo: + discovery.resolve_controllers(f"good:1, {entry} ,other:2") + # The entry is quoted exactly as the user wrote it (name= included, + # surrounding whitespace stripped) so it can be found in the value. + assert f"'{entry}'" in excinfo.value.message + assert excinfo.value.entry == entry + + # --------------------------------------------------------------------------- # follow_redirects_any(): tries each (host, port) candidate in order, # failing over past ones that are unreachable at the network level.