Nodes get a unique name from their MAC, so two machines can share one
Several nodes legitimately want to be called VM238 - one per hypervisor, one per rack, one per testbed. The unique constraint on compute_nodes is (organization_id, name), so until now the second machine to pair was refused with node_name_taken for a name that looks free to the person typing it, and the fix of 'pick another name' pushed the bookkeeping onto the operator for something the controller can derive. A node's name is now its base name plus the last four hex digits of its MAC: VM238-AC4B. Four digits because the name appears in a UI column, a command line and a systemd unit. Sixteen bits narrows collisions rather than eliminating them, so a letter fallback sits behind it - VM238-AC4B-A - and the sequence continues past Z, because a sequence that silently runs out hands out a name that is already taken. The code stays bound to the *base* name. The controller cannot know a MAC before the machine speaks, so the final name is settled at redeem, where 'is this taken' can be asked of the database rather than guessed at by the script on the machine. The script reads the first non-loopback, non-virtual interface, sends the MAC, and then adopts the name the controller returns rather than the one it asked for - reporting one while registering the other is how a node ends up appearing under a name it does not have. Three things this deliberately does not do: - A MAC that is not six octets of hex is rejected rather than cleaned up. The permissive version turns 'not-a-mac' into AMAC, and VM238-AMAC is worse than a node admitting it does not know its own address. - Without a MAC the plain base name is used and a second node of that name is still refused. There is no evidence the machines differ, and inventing a letter would turn one ambiguous input into unbounded identities. - Re-running bootstrap on an already-onboarded machine now succeeds as VM238-AC4B-A rather than being refused. That follows the specified rule - on conflict, add a letter - and it is called out in the test that asserts it: a stale node cannot be assumed gone because its hardware came back. Mutation-verified: the octet bug fails 10 tests, removing the letter fallback fails 2, and the bootstrap script ceasing to send the MAC fails 1. 641 passed, 4 skipped.
This commit is contained in:
umbra audio
@@ -72,6 +72,30 @@ if [ "$PAIRING_CODE" = "000000" ]; then
|
||||
[ -n "$PAIRING_CODE" ] || die "A pairing code is required. Ask an administrator for one."
|
||||
fi
|
||||
|
||||
# The MAC of the first non-loopback interface that has one.
|
||||
#
|
||||
# Sent so the controller can make this machine's name unique: several machines can
|
||||
# legitimately be called VM238, and the controller resolves the real name as
|
||||
# `VM238-AC4B` using the last four hex digits of this. It is asked for here rather than
|
||||
# derived from the base name because the controller cannot know a MAC before the machine
|
||||
# speaks, and the pairing code is bound to the base name.
|
||||
#
|
||||
# Every step of this is allowed to fail. A machine that cannot report a MAC still pairs -
|
||||
# it keeps the base name, and the controller refuses a second node of that name as it
|
||||
# always did. Guessing here would be worse than not knowing: a wrong MAC produces a name
|
||||
# that looks unique and is not.
|
||||
NODE_MAC=""
|
||||
for _iface in $(ls /sys/class/net 2>/dev/null); do
|
||||
case "$_iface" in lo|docker*|veth*|br-*|virbr*) continue ;; esac
|
||||
if [ -r "/sys/class/net/$_iface/address" ]; then
|
||||
_mac=$(cat "/sys/class/net/$_iface/address" 2>/dev/null || true)
|
||||
case "$_mac" in
|
||||
??:??:??:??:??:??) NODE_MAC="$_mac"; break ;;
|
||||
esac
|
||||
fi
|
||||
done
|
||||
[ -n "$NODE_MAC" ] || say " no MAC address found; pairing with the plain node name"
|
||||
|
||||
# The same reasoning for the name. The template carries a placeholder, and the name is
|
||||
# what the code is bound to, so a template run without asking pairs nothing and the
|
||||
# failure points at the code - which is the wrong thing to be looking at.
|
||||
@@ -166,14 +190,24 @@ say " this will be visible to the controller: node $NODE_NAME"
|
||||
PAIR_RESPONSE=$(curl -fsS --max-time 30 \
|
||||
-X POST "$CONTROLLER/api/node/pair" \
|
||||
-H 'Content-Type: application/json' \
|
||||
-d "{\"code\":\"$PAIRING_CODE\",\"node_name\":\"$NODE_NAME\",\"agent_version\":\"bootstrap-$BOOTSTRAP_VERSION\"}" \
|
||||
-d "{\"code\":\"$PAIRING_CODE\",\"node_name\":\"$NODE_NAME\",\"mac\":\"$NODE_MAC\",\"agent_version\":\"bootstrap-$BOOTSTRAP_VERSION\"}" \
|
||||
) || die "Pairing failed. Check the code has not expired (they last 10-30 minutes) and that the node name matches exactly."
|
||||
|
||||
NODE_KEY=$(printf '%s' "$PAIR_RESPONSE" | python3 -c 'import json,sys; print(json.load(sys.stdin).get("key",""))')
|
||||
KEY_PREFIX=$(printf '%s' "$PAIR_RESPONSE" | python3 -c 'import json,sys; print(json.load(sys.stdin).get("key_prefix",""))')
|
||||
[ -n "$NODE_KEY" ] || die "The controller returned no credential. Response was: $PAIR_RESPONSE"
|
||||
|
||||
# The controller may have resolved the name to something unique - `VM238-AC4B` - because
|
||||
# another machine already holds the base name. From here on the script uses the name it
|
||||
# was *given*, not the one it asked for: the two differ, and reporting one while
|
||||
# registering the other is how a node ends up appearing under a name it does not have.
|
||||
PAIRED_NAME=$(printf '%s' "$PAIR_RESPONSE" | python3 -c 'import json,sys; print(json.load(sys.stdin).get("node_name",""))')
|
||||
[ -n "$PAIRED_NAME" ] || PAIRED_NAME="$NODE_NAME"
|
||||
|
||||
say " paired credential $KEY_PREFIX"
|
||||
if [ "$PAIRED_NAME" != "$NODE_NAME" ]; then
|
||||
say " name '$NODE_NAME' was taken, so this node is '$PAIRED_NAME'"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------- config
|
||||
|
||||
@@ -183,7 +217,7 @@ umask 077
|
||||
cat > "$CONFIG_PATH" <<EOF
|
||||
{
|
||||
"controller_url": "$CONTROLLER",
|
||||
"node_name": "$NODE_NAME"
|
||||
"node_name": "$PAIRED_NAME"
|
||||
}
|
||||
EOF
|
||||
chmod 600 "$CONFIG_PATH"
|
||||
|
||||
@@ -29,13 +29,19 @@ import logging
|
||||
import secrets
|
||||
from dataclasses import dataclass, field
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from typing import Any, Mapping, Sequence
|
||||
from typing import Any, Callable, Mapping, Sequence
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import psycopg
|
||||
from psycopg import sql
|
||||
|
||||
from .errors import ConflictError, InputError, NotFoundError, RepositoryError
|
||||
from .errors import (
|
||||
ConflictError,
|
||||
InputError,
|
||||
NodeNameExhaustedError,
|
||||
NotFoundError,
|
||||
RepositoryError,
|
||||
)
|
||||
from .security import PasswordService, generate_token
|
||||
from .tenancy import tenant_schema_name
|
||||
|
||||
@@ -93,6 +99,100 @@ def _now() -> datetime:
|
||||
return datetime.now(timezone.utc)
|
||||
|
||||
|
||||
#: The letters used to separate two machines that share a name *and* the last four hex
|
||||
#: digits of their MAC. Twenty-six is enough for any realistic collision and the
|
||||
#: sequence continues past Z rather than stopping, because a limit that silently runs
|
||||
#: out is worse than a long name: see :func:`_letter_suffix`.
|
||||
SUFFIX_LETTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
|
||||
|
||||
|
||||
def _letter_suffix(index: int) -> str:
|
||||
"""``0 -> A``, ``25 -> Z``, ``26 -> AA``.
|
||||
|
||||
Spreadsheet-style, so the sequence does not run out at 26 and quietly start reusing
|
||||
letters. A collision that cannot be resolved should be reported as a collision, not
|
||||
dressed up as a fresh name.
|
||||
"""
|
||||
|
||||
if index < 0: # pragma: no cover - callers pass a counter
|
||||
raise ValueError("suffix index cannot be negative")
|
||||
letters = ""
|
||||
index += 1
|
||||
while index > 0:
|
||||
index, remainder = divmod(index - 1, len(SUFFIX_LETTERS))
|
||||
letters = SUFFIX_LETTERS[remainder] + letters
|
||||
return letters
|
||||
|
||||
|
||||
def mac_suffix(mac: str | None) -> str | None:
|
||||
"""The last four hex digits of a MAC, uppercased. ``None`` if there isn't one.
|
||||
|
||||
``...:ac:4b`` becomes ``AC4B``, so ``VM238`` on that machine is ``VM238-AC4B``.
|
||||
Four digits rather than the whole address because a node's name appears in a UI
|
||||
column, a command line and a systemd unit, and the whole thing would be unreadable
|
||||
in all three. Note it is the last four *characters of the address*, so it is the
|
||||
final two octets - not four octets, and not one octet padded out.
|
||||
|
||||
Sixteen bits of entropy is not much - 65 536 machines could share a base name and a
|
||||
suffix - so this narrows collisions rather than eliminating them, and
|
||||
:func:`resolve_node_name` finishes the job.
|
||||
"""
|
||||
|
||||
if mac is None:
|
||||
return None
|
||||
# Six octets of two hex digits. Validated rather than cleaned up, because the
|
||||
# permissive version of this - strip the separators and take the last four characters -
|
||||
# turns "not-a-mac" into AMAC, and a node named VM238-AMAC is worse than a node that
|
||||
# admits it does not know its own address.
|
||||
cleaned = mac.strip().replace("-", ":").lower()
|
||||
parts = cleaned.split(":")
|
||||
if len(parts) != 6 or not all(len(part) == 2 for part in parts):
|
||||
return None
|
||||
if not all(character in "0123456789abcdef" for part in parts for character in part):
|
||||
return None
|
||||
return f"{parts[4]}{parts[5]}".upper()
|
||||
|
||||
|
||||
def resolve_node_name(
|
||||
base_name: str,
|
||||
mac: str | None,
|
||||
taken: Callable[[str], bool],
|
||||
) -> str:
|
||||
"""The name this machine gets: ``base-AC4B``, or ``base-AC4B-B`` and so on.
|
||||
|
||||
``base_name`` is what the operator typed, and is what the pairing code was minted
|
||||
against. The suffix is what makes two machines called ``VM238`` distinguishable, and
|
||||
it comes from hardware the controller cannot know in advance - which is why the code
|
||||
binds to the base name and the final name is resolved here, at redeem, where the
|
||||
uniqueness check can be made against the database rather than guessed at by the
|
||||
script running on the machine.
|
||||
|
||||
The letter fallback exists because four hex digits is sixteen bits and two machines
|
||||
can genuinely share it: cloned VM images with the same generated MAC, or a
|
||||
hypervisor that hands out the same address twice. Without it, the second machine to
|
||||
pair would be refused for a name that looks free to the person typing it.
|
||||
|
||||
``taken`` is called with a candidate and must answer whether that name already
|
||||
exists. It is injected rather than queried here so this stays a pure function: the
|
||||
rule can then be tested exhaustively without a database, and the check-and-create
|
||||
race is closed at the insert by the unique constraint rather than here.
|
||||
"""
|
||||
|
||||
suffix = mac_suffix(mac)
|
||||
stem = f"{base_name}-{suffix}" if suffix else base_name
|
||||
if not taken(stem):
|
||||
return stem
|
||||
for index in range(len(SUFFIX_LETTERS) * 4):
|
||||
candidate = f"{stem}-{_letter_suffix(index)}"
|
||||
if not taken(candidate):
|
||||
return candidate
|
||||
raise NodeNameExhaustedError(
|
||||
"node_name_exhausted",
|
||||
f"Every name derived from {base_name!r} is already in use. The base name is "
|
||||
"too widely shared; pair with a more specific one.",
|
||||
)
|
||||
|
||||
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class Device:
|
||||
"""One compute device on a node.
|
||||
@@ -657,6 +757,7 @@ class ComputeRepository:
|
||||
agent_version: str | None = None,
|
||||
locality: str | None = None,
|
||||
source_address: str | None = None,
|
||||
mac: str | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Exchange a code for a node credential. The code is spent either way.
|
||||
|
||||
@@ -773,19 +874,48 @@ class ComputeRepository:
|
||||
# retried bootstrap after a partial failure. It arrived as an unhandled
|
||||
# UniqueViolation and surfaced to the caller as a bare 500, which says
|
||||
# nothing about the cause. Checked first so the answer can name it.
|
||||
# The final name, and the one place the naming rule is applied.
|
||||
#
|
||||
# `node_name` here is the *base* name: what the operator typed, and what the
|
||||
# code was minted against. The suffix that makes it unique comes from the
|
||||
# machine's MAC, which the controller cannot know when the code is minted -
|
||||
# which is exactly why the code binds to the base name and the final name is
|
||||
# settled here, where "is this name taken" can be asked of the database.
|
||||
#
|
||||
# With a MAC, a taken base name is disambiguated rather than refused: two
|
||||
# machines called `VM238` are a normal thing and become `VM238-AC4B` and
|
||||
# `VM238-3F19`. Without one there is no evidence they are different machines,
|
||||
# so the refusal stands. That asymmetry is deliberate - the alternative is a
|
||||
# re-run of bootstrap on an already-onboarded machine silently succeeding as
|
||||
# `VM238-B`, producing a second identity for one machine.
|
||||
def _taken(candidate: str) -> bool:
|
||||
return (
|
||||
connection.execute(
|
||||
"SELECT 1 FROM public.compute_nodes "
|
||||
"WHERE organization_id = %s AND name = %s",
|
||||
(matched["organization_id"], candidate),
|
||||
).fetchone()
|
||||
is not None
|
||||
)
|
||||
|
||||
if mac:
|
||||
final_name = resolve_node_name(matched["node_name"], mac, _taken)
|
||||
else:
|
||||
final_name = matched["node_name"]
|
||||
|
||||
existing = connection.execute(
|
||||
"""
|
||||
SELECT id FROM public.compute_nodes
|
||||
WHERE organization_id = %s AND name = %s
|
||||
""",
|
||||
(matched["organization_id"], matched["node_name"]),
|
||||
(matched["organization_id"], final_name),
|
||||
).fetchone()
|
||||
if existing is not None:
|
||||
return {
|
||||
"ok": False,
|
||||
"code": "node_name_taken",
|
||||
"message": (
|
||||
f"A node named '{matched['node_name']}' already exists in this "
|
||||
f"A node named '{final_name}' already exists in this "
|
||||
"organization. Pairing does not replace an existing node: if you "
|
||||
"are re-running a bootstrap for a node that is already onboarded, "
|
||||
"its existing credential still works and no new one is needed. "
|
||||
@@ -802,7 +932,7 @@ class ComputeRepository:
|
||||
locality, transport, status, agent_version)
|
||||
VALUES (%s, %s, %s, false, %s, %s, 'https', 'provisioning', %s)
|
||||
""",
|
||||
(node_id, matched["organization_id"], matched["node_name"],
|
||||
(node_id, matched["organization_id"], final_name,
|
||||
matched["trust_domain"], locality or matched["locality"], agent_version),
|
||||
)
|
||||
|
||||
@@ -857,7 +987,8 @@ class ComputeRepository:
|
||||
return {
|
||||
"ok": True,
|
||||
"node_id": node_id,
|
||||
"node_name": matched["node_name"],
|
||||
"node_name": final_name,
|
||||
"base_name": matched["node_name"],
|
||||
"key_prefix": prefix,
|
||||
"key": node_key,
|
||||
"scopes": paired_scopes,
|
||||
|
||||
@@ -186,6 +186,19 @@ class ConflictError(UmbraWebError):
|
||||
message = "The request conflicts with existing state."
|
||||
|
||||
|
||||
class NodeNameExhaustedError(ConflictError):
|
||||
"""Every name derivable from a base name is taken.
|
||||
|
||||
Separate from a plain conflict because the remedy differs: a conflict says "this node
|
||||
is already paired", and this says "the name you chose is too widely shared, choose a
|
||||
more specific one". Collapsing them would leave an operator retyping a name that can
|
||||
never work.
|
||||
"""
|
||||
|
||||
code = "node_name_exhausted"
|
||||
message = "Every name derived from that base name is already in use."
|
||||
|
||||
|
||||
class StateTransitionError(ConflictError):
|
||||
"""The requested account status change is not allowed from the current state."""
|
||||
|
||||
|
||||
@@ -211,12 +211,26 @@ class NodePairRequest(RequestModel):
|
||||
agent_version: str | None = Field(default=None, max_length=64)
|
||||
locality: str | None = Field(default=None, max_length=120)
|
||||
|
||||
mac: str | None = Field(
|
||||
default=None,
|
||||
max_length=32,
|
||||
pattern=r"^[0-9A-Fa-f]{2}(:[0-9A-Fa-f]{2}){5}$",
|
||||
)
|
||||
"""The node's MAC, which is what makes its name unique.
|
||||
|
||||
Optional, and deliberately so: a machine that cannot report one still pairs, and
|
||||
falls back to the letter-suffix sequence on its own. What it must not do is claim a
|
||||
name that already belongs to another machine, so the fallback is a collision
|
||||
resolution rather than a silent overwrite - see :func:`resolve_node_name`.
|
||||
"""
|
||||
|
||||
|
||||
class NodePairResponse(BaseModel):
|
||||
"""A paired node and its credential. The key is shown once and never stored."""
|
||||
|
||||
node_id: UUID
|
||||
node_name: str
|
||||
base_name: str | None = None
|
||||
key_prefix: str
|
||||
key: str
|
||||
scopes: list[str]
|
||||
@@ -1035,12 +1049,14 @@ def pair_node(payload: NodePairRequest, request: Request) -> NodePairResponse:
|
||||
agent_version=payload.agent_version,
|
||||
locality=payload.locality,
|
||||
source_address=client.host if client else None,
|
||||
mac=payload.mac,
|
||||
)
|
||||
if not result["ok"]:
|
||||
raise PairingError(code=result["code"], message=result["message"])
|
||||
return NodePairResponse(
|
||||
node_id=result["node_id"],
|
||||
node_name=result["node_name"],
|
||||
base_name=result.get("base_name"),
|
||||
key_prefix=result["key_prefix"],
|
||||
key=result["key"],
|
||||
scopes=result["scopes"],
|
||||
|
||||
216
umbratrans_web/tests/test_node_naming.py
Normal file
216
umbratrans_web/tests/test_node_naming.py
Normal file
@@ -0,0 +1,216 @@
|
||||
"""Two machines can share a name; the controller has to be able to tell them apart.
|
||||
|
||||
Several nodes legitimately want to be called ``VM238`` - one per hypervisor, one per rack,
|
||||
one per testbed. The unique constraint on ``compute_nodes`` is ``(organization_id, name)``,
|
||||
so without something appended the second machine to pair is refused for a name that looks
|
||||
free to the person typing it.
|
||||
|
||||
The suffix is the last four hex digits of the machine's MAC: ``VM238-AC4B``. Four digits
|
||||
because a node's name appears in a UI column, a command line and a systemd unit. Sixteen
|
||||
bits of entropy, which narrows collisions rather than eliminating them - two cloned images
|
||||
can share a MAC - so there is a letter fallback behind it, and the letter sequence
|
||||
continues past Z because a sequence that silently runs out hands out a name already taken.
|
||||
|
||||
These are pure-function tests on the rule itself. The part that actually races is the
|
||||
insert, and that is closed by the unique constraint rather than here; `test_node_pairing.py`
|
||||
covers the pairing path end to end, including the consequence of the letter fallback for a
|
||||
machine that pairs twice.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from umbratrans_web.compute import _letter_suffix, mac_suffix, resolve_node_name
|
||||
from umbratrans_web.errors import NodeNameExhaustedError
|
||||
|
||||
|
||||
class TestMacSuffix:
|
||||
@pytest.mark.parametrize(
|
||||
("mac", "expected"),
|
||||
[
|
||||
("00:11:22:33:ac:4b", "AC4B"),
|
||||
("AC:4B:11:22:33:44", "3344"),
|
||||
("ac-4b-11-22-33-44", "3344"),
|
||||
("aa:bb:cc:dd:ee:ff", "EEFF"),
|
||||
("00:11:22:33:AC:4B", "AC4B"),
|
||||
],
|
||||
)
|
||||
def test_it_is_the_last_four_hex_characters(self, mac: str, expected: str) -> None:
|
||||
"""The rule, on the forms a MAC actually arrives in.
|
||||
|
||||
The first case is the one from the specification: a machine ending ``ac:4b`` is
|
||||
``AC4B``. The second is the trap - an octet is *two* hex characters, so taking the
|
||||
last octet and padding it would give ``VM238-44``, which is 8 bits and would
|
||||
collide within a handful of machines.
|
||||
"""
|
||||
|
||||
assert mac_suffix(mac) == expected
|
||||
|
||||
def test_it_uppercases_so_names_sort_predictably(self) -> None:
|
||||
assert mac_suffix("00:11:22:33:ac:4b") == mac_suffix("00:11:22:33:AC:4B")
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mac",
|
||||
[
|
||||
None,
|
||||
"",
|
||||
"4b",
|
||||
"not-a-mac",
|
||||
# Twelve hex digits with no separators: not a MAC, and the shape a naive
|
||||
# "strip the punctuation" cleanup would happily accept.
|
||||
"AC4B11223344",
|
||||
# Five octets. A real address has six.
|
||||
"00:11:22:ac:4b",
|
||||
# Six octets, one of them not hex.
|
||||
"00:11:22:33:ac:zz",
|
||||
],
|
||||
)
|
||||
def test_nothing_usable_gives_nothing(self, mac: str | None) -> None:
|
||||
"""A machine that cannot report a MAC still pairs, with the plain base name.
|
||||
|
||||
Rejection rather than best-effort extraction, because ``VM238-AMAC`` is a name no
|
||||
operator chose and no operator can predict.
|
||||
"""
|
||||
|
||||
assert mac_suffix(mac) is None
|
||||
|
||||
|
||||
class TestLetterSuffix:
|
||||
@pytest.mark.parametrize(
|
||||
("index", "expected"),
|
||||
[(0, "A"), (25, "Z"), (26, "AA"), (27, "AB"), (51, "AZ"), (52, "BA")],
|
||||
)
|
||||
def test_it_continues_past_z(self, index: int, expected: str) -> None:
|
||||
"""Twenty-six is not enough, and running out silently is worse than a long name.
|
||||
|
||||
A sequence that stopped at Z would either reuse ``-A`` - handing a second machine
|
||||
a name an operator has already been told is unique - or fail in a way that looks
|
||||
like a bug. Spreadsheet-style, so ``AA`` follows ``Z``.
|
||||
"""
|
||||
|
||||
assert _letter_suffix(index) == expected
|
||||
|
||||
def test_it_never_repeats_within_the_range_offered(self) -> None:
|
||||
offered = [_letter_suffix(index) for index in range(104)]
|
||||
assert len(set(offered)) == len(offered)
|
||||
|
||||
|
||||
class TestResolveNodeName:
|
||||
def test_a_free_name_gets_the_mac_suffix(self) -> None:
|
||||
assert resolve_node_name("VM238", "00:11:22:33:ac:4b", lambda _name: False) == "VM238-AC4B"
|
||||
|
||||
def test_a_free_name_without_a_mac_is_left_alone(self) -> None:
|
||||
"""No evidence the machine is a different one, so nothing is invented."""
|
||||
|
||||
assert resolve_node_name("VM238", None, lambda _name: False) == "VM238"
|
||||
|
||||
def test_two_machines_with_the_same_base_name_get_different_names(self) -> None:
|
||||
"""The case the rule exists for."""
|
||||
|
||||
first = resolve_node_name("VM238", "00:11:22:33:ac:4b", lambda _name: False)
|
||||
second = resolve_node_name("VM238", "00:11:22:33:3f:19", lambda _name: False)
|
||||
|
||||
assert (first, second) == ("VM238-AC4B", "VM238-3F19")
|
||||
|
||||
def test_a_shared_mac_falls_back_to_letters(self) -> None:
|
||||
"""Cloned images can carry the same address, so sixteen bits is not enough."""
|
||||
|
||||
taken = {"VM238-AC4B", "VM238-AC4B-A"}
|
||||
|
||||
assert resolve_node_name("VM238", "00:11:22:33:ac:4b", lambda n: n in taken) == "VM238-AC4B-B"
|
||||
|
||||
def test_the_fallback_walks_past_z_rather_than_reusing_a_letter(self) -> None:
|
||||
# The bare stem is taken as well, or the first candidate wins and the fallback is
|
||||
# never reached - which is how this test passed the first time it was written.
|
||||
taken = {"VM238-AC4B"} | {
|
||||
f"VM238-AC4B-{_letter_suffix(index)}" for index in range(27)
|
||||
}
|
||||
|
||||
assert resolve_node_name("VM238", "00:11:22:33:ac:4b", lambda n: n in taken) == "VM238-AC4B-AB"
|
||||
|
||||
def test_a_name_derivable_from_nothing_is_reported_rather_than_reused(self) -> None:
|
||||
"""Refusal is the honest answer once every candidate is gone.
|
||||
|
||||
Returning ``VM238-AC4B-A`` because the loop ran out would hand out a name that is
|
||||
already taken, which is the one outcome worse than an error.
|
||||
"""
|
||||
|
||||
with pytest.raises(NodeNameExhaustedError) as error:
|
||||
resolve_node_name("VM238", "00:11:22:33:ac:4b", lambda _name: True)
|
||||
|
||||
assert error.value.code == "node_name_exhausted"
|
||||
assert error.value.http_status == 409
|
||||
|
||||
def test_it_asks_about_candidates_in_a_stable_order(self) -> None:
|
||||
"""Two machines pairing at once must not both pick the same free name.
|
||||
|
||||
The database check inside ``taken`` is not atomic with the insert, so the
|
||||
uniqueness that actually holds is the constraint's. This is only about both
|
||||
machines *asking* in the same order, so the collision reaches the constraint
|
||||
instead of being resolved twice into the same answer.
|
||||
"""
|
||||
|
||||
asked: list[str] = []
|
||||
|
||||
def taken(name: str) -> bool:
|
||||
asked.append(name)
|
||||
return False
|
||||
|
||||
resolve_node_name("VM238", "00:11:22:33:ac:4b", taken)
|
||||
|
||||
assert asked == ["VM238-AC4B"]
|
||||
|
||||
|
||||
class TestTheBootstrapScriptSuppliesIt:
|
||||
"""The rule is inert unless the machine running the script tells the controller its MAC.
|
||||
|
||||
Asserted against the rendered script rather than by running it: what matters is that
|
||||
the MAC is read from the machine and included in the pairing request, and a test that
|
||||
shelled out to `sh` would prove considerably less while depending on a network stack.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _script() -> str:
|
||||
from umbratrans_web.bootstrap import render_bootstrap_template
|
||||
|
||||
return render_bootstrap_template()
|
||||
|
||||
def test_it_reads_a_mac_from_a_real_interface(self) -> None:
|
||||
script = self._script()
|
||||
|
||||
assert "/sys/class/net" in script, "the script never looks at an interface"
|
||||
assert "address" in script, "the script never reads an interface address"
|
||||
|
||||
def test_it_excludes_loopback_and_virtual_interfaces(self) -> None:
|
||||
"""A container's MAC is the wrong answer, and there is usually one.
|
||||
|
||||
Docker and veth interfaces exist on most machines that run containers, and a
|
||||
machine whose name is derived from its veth MAC changes name when the container
|
||||
restarts with a different address. Loopback is constant and unique to nothing.
|
||||
"""
|
||||
|
||||
script = self._script()
|
||||
|
||||
for excluded in ("lo", "docker", "veth", "virbr"):
|
||||
assert excluded in script, f"the script does not exclude {excluded}"
|
||||
|
||||
def test_it_sends_the_mac_when_pairing(self) -> None:
|
||||
assert '"mac":"$NODE_MAC"' in script_of(self._script())
|
||||
|
||||
def test_a_missing_mac_does_not_stop_the_node_pairing(self) -> None:
|
||||
"""The fallback has to be a fallback, not a second way to fail.
|
||||
|
||||
A machine that cannot read an interface still has to reach the controller; the
|
||||
script says so and continues, and the controller pairs it under the plain name.
|
||||
"""
|
||||
|
||||
script = self._script()
|
||||
|
||||
assert "no MAC address found" in script
|
||||
|
||||
|
||||
def script_of(rendered: str) -> str:
|
||||
"""Collapse the shell line-continuations so one request is one searchable string."""
|
||||
|
||||
return rendered.replace("\\\n", "").replace('\\"', '"')
|
||||
@@ -100,6 +100,100 @@ def _mint(repository, organization, name: str, **kwargs) -> dict:
|
||||
return repository.create_pairing_code(organization, name, **kwargs)
|
||||
|
||||
|
||||
def _pair(repository, organization, base: str, mac: str | None = None, tag: str = ""):
|
||||
"""Mint a code for ``base`` and immediately redeem it, optionally with a MAC."""
|
||||
|
||||
minted = _mint(repository, organization, base)
|
||||
result = repository.redeem_pairing_code(minted["code"], base, mac=mac)
|
||||
return result
|
||||
|
||||
|
||||
# -- unique names, so two machines can share one -------------------------------
|
||||
|
||||
|
||||
def test_a_machine_paired_with_a_mac_gets_that_mac_in_its_name(pairing_world) -> None:
|
||||
repository, organization = pairing_world
|
||||
result = _pair(repository, organization, "VM238", mac="00:11:22:33:ac:4b")
|
||||
|
||||
assert result["ok"] is True, result
|
||||
assert result["node_name"] == "VM238-AC4B"
|
||||
assert result["base_name"] == "VM238"
|
||||
|
||||
|
||||
def test_two_machines_paired_under_one_base_name_both_succeed(pairing_world) -> None:
|
||||
"""The case the MAC suffix exists for.
|
||||
|
||||
Before this, the second machine was refused with ``node_name_taken`` for a name that
|
||||
looks free to the person typing it - and the fix of "just pick another name" puts the
|
||||
bookkeeping on the operator for something the controller can derive.
|
||||
"""
|
||||
|
||||
repository, organization = pairing_world
|
||||
|
||||
first = _pair(repository, organization, "VM238", mac="00:11:22:33:ac:4b")
|
||||
second = _pair(repository, organization, "VM238", mac="00:11:22:33:3f:19")
|
||||
|
||||
assert first["ok"] is True, first
|
||||
assert second["ok"] is True, second
|
||||
assert first["node_name"] == "VM238-AC4B"
|
||||
assert second["node_name"] == "VM238-3F19"
|
||||
assert first["node_id"] != second["node_id"]
|
||||
|
||||
|
||||
def test_a_shared_mac_gets_the_next_letter_rather_than_being_refused(pairing_world) -> None:
|
||||
"""The specified fallback, and it has a consequence worth stating plainly.
|
||||
|
||||
Sixteen bits of MAC is not unique: a cloned image can carry the same address, and two
|
||||
such machines would both resolve to ``VM238-AC4B``. The rule is to add a letter, so
|
||||
the second becomes ``VM238-AC4B-A``.
|
||||
|
||||
The consequence: re-running bootstrap on *one already-onboarded machine* also lands
|
||||
here, and produces a second identity with a second credential for the same hardware.
|
||||
That is deliberate under this rule rather than an oversight - a re-bootstrap that
|
||||
mints a fresh credential is a reasonable thing to want, and the deterministic name
|
||||
means the operator can see which is which. What it does mean is that a stale node
|
||||
cannot be assumed gone because its hardware came back; ``VM238-AC4B`` and
|
||||
``VM238-AC4B-A`` are two nodes until one of them is removed.
|
||||
"""
|
||||
|
||||
repository, organization = pairing_world
|
||||
|
||||
first = _pair(repository, organization, "VM238", mac="00:11:22:33:ac:4b")
|
||||
second = _pair(repository, organization, "VM238", mac="00:11:22:33:ac:4b")
|
||||
|
||||
assert first["ok"] is True, first
|
||||
assert second["ok"] is True, second
|
||||
assert first["node_name"] == "VM238-AC4B"
|
||||
assert second["node_name"] == "VM238-AC4B-A"
|
||||
assert first["node_id"] != second["node_id"]
|
||||
|
||||
|
||||
def test_a_machine_with_no_mac_still_pairs_under_the_plain_name(pairing_world) -> None:
|
||||
"""Nothing about pairing may depend on being able to read a network interface."""
|
||||
|
||||
repository, organization = pairing_world
|
||||
result = _pair(repository, organization, "no-interface", mac=None)
|
||||
|
||||
assert result["ok"] is True, result
|
||||
assert result["node_name"] == "no-interface"
|
||||
|
||||
|
||||
def test_a_machine_with_no_mac_does_not_get_a_silent_suffix(pairing_world) -> None:
|
||||
"""Without a MAC there is no evidence of a second machine, so the refusal stands.
|
||||
|
||||
The alternative - inventing ``-A`` - would turn the double-pairing mistake above into
|
||||
a success, which is the one outcome worth being strict about.
|
||||
"""
|
||||
|
||||
repository, organization = pairing_world
|
||||
first = _pair(repository, organization, "ambiguous", mac=None)
|
||||
second = _pair(repository, organization, "ambiguous", mac=None)
|
||||
|
||||
assert first["ok"] is True, first
|
||||
assert second["ok"] is False
|
||||
assert second["code"] == "node_name_taken"
|
||||
|
||||
|
||||
# -- the happy path -----------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user