diff --git a/setup-environment.sh b/setup-environment.sh index c932003bd..bd62017b9 100755 --- a/setup-environment.sh +++ b/setup-environment.sh @@ -421,13 +421,21 @@ tailnet_node_name() { } # Peers on this tailnet whose name looks like a hummingbot-api node, newline -# separated. Matches the requested name and the -1/-2 suffixes Tailscale adds, -# so the machine we are looking for is found under whichever one it got. +# separated. +# +# The suffix is deliberately `-.+` and not `-[0-9]+`. Tailscale's own +# de-duplication only ever appends numbers, but TAILSCALE_HOSTNAME is a +# setting, and a deployment that names its API per desk -- hummingbot-api-cornell, +# hummingbot-api-eu -- is the normal way to run more than one. Matching only +# numbers found nothing for those, and "nothing" is the branch that warns +# "deploy it on that machine first" about a node sitting right there in +# `tailscale status`, then asks the operator to type the name it just declined +# to recognise. tailnet_api_peers() { local want="${1:-hummingbot-api}" tailscale status --peers 2>/dev/null \ | awk '{print $2}' \ - | grep -E "^${want}(-[0-9]+)?$" || true + | grep -E "^${want}(-.+)?$" || true } # Make sure a tailscaled is running, then `tailscale up`. @@ -1164,16 +1172,42 @@ if [ -z "${DEPLOY_HUMMINGBOT_API:-}" ] || [ "$finish_remote_api" = true ]; then ts_hostname="" _ts_candidates="$(tailnet_api_peers hummingbot-api)" _ts_count="$(printf '%s' "$_ts_candidates" | grep -c . || true)" - if [ "${_ts_count:-0}" -eq 1 ]; then + if [ "${_ts_count:-0}" -eq 1 ] && [ "$_ts_candidates" = "hummingbot-api" ]; then + # The unsuffixed name, and the only match. Nothing to choose + # between, and it is the name hummingbot-api's own setup asks + # for, so this is the node in every ordinary install. ts_hostname="$_ts_candidates" msg_ok "Found it on your tailnet: $ts_hostname" + elif [ "${_ts_count:-0}" -eq 1 ]; then + # One match, but under a suffix -- hummingbot-api-1, or a name + # somebody chose. Being alone is not evidence it is the right + # one: a stale node, a colleague's staging box, or another + # desk's API is just as alone, and accepting it silently writes + # it into config.yml where the next sign of trouble is a 401 + # against a machine the operator never meant to reach. Shown + # and confirmed instead, with Enter as the answer when it is + # right, which is the common case. + msg_ok "Found one hummingbot-api node on your tailnet: $_ts_candidates" + msg_info "That is not the default name, so it is worth a look before it" + msg_info "goes in config.yml. Press Enter to use it, or type another." + prompt_visible "hummingbot-api host" "$_ts_candidates" "ts_hostname" + ts_hostname="${ts_hostname:-$_ts_candidates}" elif [ "${_ts_count:-0}" -gt 1 ]; then msg_warn "More than one hummingbot-api node on this tailnet:" printf '%s\n' "$_ts_candidates" | while IFS= read -r _c; do [ -n "$_c" ] && echo " • $_c" done - msg_info "Tailscale adds -1, -2 ... when a name is taken, so these are" - msg_info "different machines. Pick the one you just deployed." + # Deliberately does NOT say "Tailscale adds -1, -2 when a name + # is taken, so pick the one you just deployed". Running several + # APIs on purpose -- hummingbot-api-1, hummingbot-api-2 -- is a + # normal deployment, and there those suffixes are chosen names, + # not collision artifacts. The old wording told such an operator + # their own naming was an accident, and "the one you just + # deployed" is the wrong instruction anyway when Condor is being + # pointed at an instance that has been up for weeks. + msg_info "These are separate machines -- deployments named on purpose, or" + msg_info "names Tailscale suffixed because one was already taken." + msg_info "Pick the one this Condor should talk to." prompt_required_visible "Which node is your hummingbot-api?" "ts_hostname" "Name cannot be empty" else msg_warn "No hummingbot-api node is visible on this tailnet yet." diff --git a/tests/test_setup_tailnet_peers.py b/tests/test_setup_tailnet_peers.py new file mode 100644 index 000000000..d399369ed --- /dev/null +++ b/tests/test_setup_tailnet_peers.py @@ -0,0 +1,144 @@ +"""`tailnet_api_peers` in setup-environment.sh, exercised as shell. + +The helper decides which tailnet node the wizard writes into config.yml as +the hummingbot-api host, and getting it wrong is not a visible failure: the +wrong node is very likely running the same software, so it answers, and the +mistake surfaces as "401 Incorrect username or password" against a password +that was never wrong. + +There is no bats in this repo and no reason to add one for a single awk/grep +pipeline, so the function is sourced out of the script with a `tailscale` +stub ahead of it on PATH. That runs the real line, not a Python +reimplementation of it -- a copy of the regex in a test asserts only that the +copy matches itself. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +SETUP = Path(__file__).resolve().parent.parent / "setup-environment.sh" + + +def run_peers( + tmp_path: Path, status_output: str, want: str = "hummingbot-api" +) -> list[str]: + """Return `tailnet_api_peers ` over a stubbed `tailscale status`.""" + stub_dir = tmp_path / "bin" + stub_dir.mkdir(exist_ok=True) + stub = stub_dir / "tailscale" + peers = tmp_path / "peers.txt" + peers.write_text(status_output) + stub.write_text(f'#!/bin/sh\ncat "{peers}"\n') + stub.chmod(0o755) + + # Pull just the function out: sourcing the whole script would run an + # installer. Everything from its definition to the closing brace. + src = SETUP.read_text() + start = src.index("tailnet_api_peers() {") + end = src.index("\n}\n", start) + len("\n}\n") + func = src[start:end] + + proc = subprocess.run( + ["bash", "-c", f'PATH="{stub_dir}:$PATH"\n{func}\ntailnet_api_peers "{want}"'], + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 0, proc.stderr + return [ln for ln in proc.stdout.splitlines() if ln.strip()] + + +def status(*names: str) -> str: + return "".join( + f"100.64.0.{i + 1}\t{n}\tdavid@\tlinux\t-\n" for i, n in enumerate(names) + ) + + +# ── the suffixes Tailscale itself assigns ────────────────────────────── + + +def test_finds_the_unsuffixed_name(tmp_path): + assert run_peers(tmp_path, status("hummingbot-api")) == ["hummingbot-api"] + + +def test_finds_numeric_collision_suffixes(tmp_path): + out = run_peers(tmp_path, status("hummingbot-api-1", "hummingbot-api-2")) + assert out == ["hummingbot-api-1", "hummingbot-api-2"] + + +# ── the suffixes an operator assigns, which is what this PR is about ─── + + +@pytest.mark.parametrize("name", ["hummingbot-api-cornell", "hummingbot-api-eu"]) +def test_finds_desk_named_nodes(tmp_path, name): + """The regression: TAILSCALE_HOSTNAME is a setting, not only a collision.""" + assert run_peers(tmp_path, status(name)) == [name] + + +def test_deliberate_numeric_fleet_all_found(tmp_path): + """A planned multi-instance deployment, the case that motivated the reword.""" + out = run_peers( + tmp_path, status("hummingbot-api", "hummingbot-api-1", "hummingbot-api-2") + ) + assert out == ["hummingbot-api", "hummingbot-api-1", "hummingbot-api-2"] + assert len(out) > 1, "must reach the pick-one branch, never auto-select" + + +# ── names that must NOT be claimed ───────────────────────────────────── + + +@pytest.mark.parametrize( + "name", + [ + "condor", + "condor-hackathon", + "condor-hackathon-server", + "condor1", + "hbapi", + "my-hummingbot-api", # suffix match, not prefix -- anchored ^ + ], +) +def test_ignores_unrelated_nodes(tmp_path, name): + assert run_peers(tmp_path, status(name)) == [] + + +def test_ignores_the_prefix_without_a_separator(tmp_path): + """`hummingbot-apis` is a different word, not a suffixed hummingbot-api.""" + assert run_peers(tmp_path, status("hummingbot-apis")) == [] + + +def test_picks_only_the_api_out_of_a_mixed_tailnet(tmp_path): + out = run_peers( + tmp_path, + status( + "condor-hackathon", + "hummingbot-api-cornell", + "condor1", + "someones-laptop", + ), + ) + assert out == ["hummingbot-api-cornell"] + + +# ── shape of the output ──────────────────────────────────────────────── + + +def test_empty_tailnet_is_empty_not_an_error(tmp_path): + """The no-match branch must be reachable: `|| true` keeps grep's 1 quiet.""" + assert run_peers(tmp_path, "") == [] + + +def test_reads_the_name_column_not_the_address(tmp_path): + out = run_peers(tmp_path, status("hummingbot-api")) + assert out == ["hummingbot-api"] + assert not any(c.startswith("100.") for c in out) + + +def test_custom_want_is_honoured(tmp_path): + """The parameter is real, even though today's only caller hardcodes it.""" + out = run_peers(tmp_path, status("condor", "condor-hackathon"), want="condor") + assert out == ["condor", "condor-hackathon"]