From 841b5c46e71ccbf3a20a536256c290b67d472568 Mon Sep 17 00:00:00 2001 From: henleda Date: Thu, 30 Jul 2026 14:41:42 -0500 Subject: [PATCH] fix(lab): make the lab templates configuration instead of hardcoded Nimbus objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create_lab` builds a test LB by CLONING a known-good origin pool and HTTP LB — so every field XC requires is present — then stripping the copy back to a clean slate. That mechanism is right and is unchanged. What moved is where the two object NAMES come from. They were hardcoded defaults naming this operator's own tenant (`nimbus-bigip-pool`, `nimbus-www`), which cost three things: - `lab-create` only worked on a tenant that happened to contain objects by those names; anywhere else it failed at the first GET with a bare 404. - The CLI exposed no flag reaching either parameter, so from the command line they were effectively hardcoded. - `nimbus-www` was simultaneously the default source template here and the default PROTECTED object in `engine.protected_lbs()` — the one load balancer the tool refuses to mutate was the one it read to build every lab. Safe, since templates are only ever read and the deep copy is what gets created, but it reads oddly in a codebase this careful about blast radius. Now `--pool-template` / `--lb-template`, or `$VPCOPILOT_LAB_POOL_TEMPLATE` / `$VPCOPILOT_LAB_LB_TEMPLATE`, following the same shape `protected_lbs()` already uses for `VPCOPILOT_PROTECTED_LBS`. A missing template names both remedies and says why a template is needed at all, rather than surfacing the 404. No regressions: the defaults keep the same values, so behaviour on this tenant is identical — pinned by a test asserting the two template reads are still `nimbus-bigip-pool` and `nimbus-www` when nothing is configured. 785 tests pass with no credentials and no .env, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/TRY_IT.md | 14 +++++ src/vpcopilot/cli.py | 10 +++- src/vpcopilot/lab.py | 53 ++++++++++++++++-- tests/test_lab.py | 125 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 4 deletions(-) diff --git a/docs/TRY_IT.md b/docs/TRY_IT.md index 7af75c5..151bc74 100644 --- a/docs/TRY_IT.md +++ b/docs/TRY_IT.md @@ -99,6 +99,20 @@ vpcopilot lab-create --domain vampi.example.com --origin :5000 This creates an origin pool + a clean-slate HTTP LB and prints the **DNS records to add**. Once DNS resolves and the cert issues, the app is reachable through XC at `https://vampi.example.com`. +The lab is built by **cloning** a known-good pool and LB — so every field XC requires is present — +and then stripping the copy back to a clean slate with every security control off. Which objects to +clone is configuration, because they have to exist in *your* namespace: + +```bash +vpcopilot lab-create --domain vampi.example.com --origin :5000 \ + --pool-template --lb-template +``` + +or set `VPCOPILOT_LAB_POOL_TEMPLATE` / `VPCOPILOT_LAB_LB_TEMPLATE` once in `.env`. The templates are +only ever **read** — the copy is what gets created — so it is safe to point them at a load balancer +you would never let the tool modify. If the named object is not there, `lab-create` says so and +names both ways to change it rather than surfacing a bare 404. + ### 2. Scan, then run the flow in the console ```bash diff --git a/src/vpcopilot/cli.py b/src/vpcopilot/cli.py index ac6fad0..4e0f8ad 100644 --- a/src/vpcopilot/cli.py +++ b/src/vpcopilot/cli.py @@ -431,11 +431,19 @@ def lab_create( origin: str = typer.Option(..., "--origin", help="app origin host:port, e.g. 16.59.6.127:5000"), name: str = typer.Option(None, "--name", help="base name (default: first label of the domain)"), origin_tls: bool = typer.Option(False, "--origin-tls", help="origin serves HTTPS (default: HTTP)"), + pool_template: str = typer.Option(None, "--pool-template", help="origin pool to clone for its required XC fields (default: $VPCOPILOT_LAB_POOL_TEMPLATE)"), + lb_template: str = typer.Option(None, "--lb-template", help="HTTP LB to clone, then strip every security control from (default: $VPCOPILOT_LAB_LB_TEMPLATE)"), ): - """Stand up a clean-slate XC test LB for an app origin (pool + LB), then print the DNS to add.""" + """Stand up a clean-slate XC test LB for an app origin (pool + LB), then print the DNS to add. + + The lab is built by CLONING a known-good pool and LB, so every required XC field is present, + and then stripping the copy back to a clean slate. Which objects to clone is configuration: + `--pool-template` / `--lb-template`, or `$VPCOPILOT_LAB_POOL_TEMPLATE` / + `$VPCOPILOT_LAB_LB_TEMPLATE`. The templates are only ever READ.""" from .lab import create_lab res = create_lab(domain, origin, name=name, origin_tls=origin_tls, + pool_template=pool_template, lb_template=lb_template, log=lambda m: rprint(f"[dim]{m}[/dim]")) rprint(Panel.fit( f"[bold]LB[/bold]: {res['lb']}\n[bold]pool[/bold]: {res['pool']} -> {res['origin']}\n" diff --git a/src/vpcopilot/lab.py b/src/vpcopilot/lab.py index d75674f..f0c6767 100644 --- a/src/vpcopilot/lab.py +++ b/src/vpcopilot/lab.py @@ -9,6 +9,7 @@ import copy import ipaddress +import os import time from typing import Callable @@ -18,6 +19,30 @@ _STATUS_FIELDS = ("host_name", "auto_cert_info", "cert_state", "dns_info", "internet_vip_info", "state", "downstream_tls_certificate_expiration_timestamps") +# The objects cloned to build a lab. CONFIG, not constants — they used to be hardcoded defaults +# naming this operator's own tenant (`nimbus-bigip-pool`, `nimbus-www`), which had three costs: +# the command only worked on a tenant that happened to contain objects by those names; the CLI +# exposed no way to override them; and `nimbus-www` was simultaneously the default source template +# here and the default PROTECTED object in `engine.protected_lbs()` — the one load balancer the +# tool refuses to mutate was the one it read to build every lab. +# +# Cloning itself stays: copying a known-good object guarantees every required XC field is present, +# where composing a spec from scratch discovers a missing one at PUT time. Only the names moved to +# config, so the same values keep the same behaviour on this tenant while the tool stops assuming +# them anywhere else. +POOL_TEMPLATE_ENV = "VPCOPILOT_LAB_POOL_TEMPLATE" +LB_TEMPLATE_ENV = "VPCOPILOT_LAB_LB_TEMPLATE" +DEFAULT_POOL_TEMPLATE = "nimbus-bigip-pool" +DEFAULT_LB_TEMPLATE = "nimbus-www" + + +def pool_template_default() -> str: + return os.environ.get(POOL_TEMPLATE_ENV, "").strip() or DEFAULT_POOL_TEMPLATE + + +def lb_template_default() -> str: + return os.environ.get(LB_TEMPLATE_ENV, "").strip() or DEFAULT_LB_TEMPLATE + def _origin_server(host: str) -> dict: try: @@ -44,9 +69,29 @@ def _clean_slate(spec: dict) -> None: spec.pop(k, None) +def _template(fetch: Callable, name: str, kind: str, env: str, flag: str) -> dict: + """Read a template object, and when it is not there say what to set rather than surfacing a 404. + + This is the failure every tenant but the one these defaults were written for will hit first, so + it names the remedy: which object was looked for, and the two ways to point it somewhere else.""" + try: + return fetch(name) + except Exception as e: # noqa: BLE001 — XCError, but the client raises its own type + raise RuntimeError( + f"no {kind} named {name!r} in this namespace to clone as the lab template ({e}). " + f"`lab-create` builds a lab by copying a known-good {kind} so every required XC field " + f"is present. Point it at one of yours with {flag}, or set ${env}." + ) from None + + def create_lab(domain: str, origin: str, *, name: str | None = None, origin_tls: bool = False, - pool_template: str = "nimbus-bigip-pool", lb_template: str = "nimbus-www", + pool_template: str | None = None, lb_template: str | None = None, poll: bool = True, log: Callable = print) -> dict: + """`pool_template`/`lb_template` default to `$VPCOPILOT_LAB_POOL_TEMPLATE` / + `$VPCOPILOT_LAB_LB_TEMPLATE`, falling back to the objects this operator's tenant happens to + carry. Passing them explicitly still wins, so every existing call is unchanged.""" + pool_template = pool_template or pool_template_default() + lb_template = lb_template or lb_template_default() xc = XC() host, _, port_s = origin.partition(":") port = int(port_s) if port_s else (443 if origin_tls else 80) @@ -57,7 +102,8 @@ def create_lab(domain: str, origin: str, *, name: str | None = None, origin_tls: if xc.origin_pool_exists(pool_name): log(f"origin pool {pool_name} already exists") else: - pspec = copy.deepcopy(xc.get_origin_pool(pool_template)["spec"]) + pspec = copy.deepcopy(_template(xc.get_origin_pool, pool_template, "origin pool", + POOL_TEMPLATE_ENV, "--pool-template")["spec"]) pspec["origin_servers"] = [_origin_server(host)] pspec["port"] = port pspec.pop("use_tls", None); pspec.pop("no_tls", None) @@ -73,7 +119,8 @@ def create_lab(domain: str, origin: str, *, name: str | None = None, origin_tls: if xc.lb_exists(lb_name): log(f"LB {lb_name} already exists") else: - spec = copy.deepcopy(xc.get_lb(lb_template)["spec"]) + spec = copy.deepcopy(_template(xc.get_lb, lb_template, "load balancer", + LB_TEMPLATE_ENV, "--lb-template")["spec"]) spec["domains"] = [domain] spec["default_route_pools"] = [{"pool": {"name": pool_name, "namespace": xc.ns}, "weight": 1, "priority": 1}] diff --git a/tests/test_lab.py b/tests/test_lab.py index dc9c16d..8b507b4 100644 --- a/tests/test_lab.py +++ b/tests/test_lab.py @@ -21,3 +21,128 @@ def test_clean_slate_disables_all_controls_and_strips_status(): assert spec["data_guard_rules"] == [] assert "host_name" not in spec and "cert_state" not in spec assert not spec["more_option"].get("request_headers_to_add") + + +# ================= the lab templates are configuration, not constants ================= +class _FakeXC: + """Records which objects were READ as templates, and what was created from them. + + `create_lab` re-reads the LB it just created to pull the DNS records out, so `read` holds the + two TEMPLATE reads first and the readback after — assertions look at `read[:2]`.""" + ns = "test-ns" + + def __init__(self, have=("nimbus-bigip-pool", "nimbus-www", "mine-pool", "mine-lb")): + self.have = set(have) + self.read: list[str] = [] + self.created: list[str] = [] + + def _get(self, name): + self.read.append(name) + if name not in self.have: + raise RuntimeError(f"GET {name} -> 404") + return {"spec": {"origin_servers": [], "port": 80, "domains": ["x"], "loadbalancer_type": {}}} + + get_origin_pool = _get + get_lb = _get + + def origin_pool_exists(self, n): + return False + + def lb_exists(self, n): + return False + + def create_origin_pool(self, obj): + self.created.append(obj["metadata"]["name"]) + + def create_lb(self, obj): + self.created.append(obj["metadata"]["name"]) + self.have.add(obj["metadata"]["name"]) # it exists now, and gets read back + return obj + + +def _run(monkeypatch, fake, **kw): + from vpcopilot import lab + monkeypatch.setattr(lab, "XC", lambda *a, **k: fake) + return lab.create_lab("app.example.com", "10.0.0.1:8080", poll=False, + log=lambda m: None, **kw) + + +def test_the_defaults_are_unchanged_so_this_tenant_behaves_identically(monkeypatch): + """No regression. The two names moved into config; they did not change value, so an existing + `lab-create` on this operator's tenant clones exactly what it always did.""" + from vpcopilot import lab + for var in (lab.POOL_TEMPLATE_ENV, lab.LB_TEMPLATE_ENV): + monkeypatch.delenv(var, raising=False) + assert lab.pool_template_default() == "nimbus-bigip-pool" + assert lab.lb_template_default() == "nimbus-www" + + fake = _FakeXC() + _run(monkeypatch, fake) + assert fake.read[:2] == ["nimbus-bigip-pool", "nimbus-www"] + + +def test_the_environment_can_point_the_lab_at_another_tenants_objects(monkeypatch): + """The actual coupling cost this fixes: `lab-create` only worked on a tenant that happened to + contain objects named `nimbus-bigip-pool` and `nimbus-www`.""" + from vpcopilot import lab + monkeypatch.setenv(lab.POOL_TEMPLATE_ENV, "mine-pool") + monkeypatch.setenv(lab.LB_TEMPLATE_ENV, "mine-lb") + fake = _FakeXC() + _run(monkeypatch, fake) + assert fake.read[:2] == ["mine-pool", "mine-lb"] + + +def test_an_explicit_argument_beats_the_environment(monkeypatch): + from vpcopilot import lab + monkeypatch.setenv(lab.POOL_TEMPLATE_ENV, "env-pool") + monkeypatch.setenv(lab.LB_TEMPLATE_ENV, "env-lb") + fake = _FakeXC(have=("mine-pool", "mine-lb")) + _run(monkeypatch, fake, pool_template="mine-pool", lb_template="mine-lb") + assert fake.read[:2] == ["mine-pool", "mine-lb"] + + +def test_an_empty_environment_value_falls_back_rather_than_asking_for_an_object_named_nothing(monkeypatch): + from vpcopilot import lab + monkeypatch.setenv(lab.POOL_TEMPLATE_ENV, " ") + monkeypatch.setenv(lab.LB_TEMPLATE_ENV, "") + assert lab.pool_template_default() == "nimbus-bigip-pool" + assert lab.lb_template_default() == "nimbus-www" + + +def test_a_missing_template_names_the_remedy_instead_of_surfacing_a_404(monkeypatch): + """This is the first failure any other tenant hits, so it has to say what to set.""" + import pytest + + from vpcopilot import lab + monkeypatch.setenv(lab.POOL_TEMPLATE_ENV, "not-here") + fake = _FakeXC() + with pytest.raises(RuntimeError) as e: + _run(monkeypatch, fake) + msg = str(e.value) + assert "not-here" in msg + assert "--pool-template" in msg and lab.POOL_TEMPLATE_ENV in msg + assert "clone" in msg # says WHY a template is needed at all + + +def test_the_templates_are_only_ever_read_never_written(monkeypatch): + """`nimbus-www` is simultaneously the default template here and the default PROTECTED object in + `engine.protected_lbs()`. That is safe only because the template is read, deep-copied, and the + copy is what gets created.""" + fake = _FakeXC() + _run(monkeypatch, fake) + assert fake.read[:2] == ["nimbus-bigip-pool", "nimbus-www"] + assert "nimbus-www" not in fake.created and "nimbus-bigip-pool" not in fake.created + assert fake.created == ["app-pool", "app-lab"] + + +def test_the_cli_exposes_both_templates(monkeypatch): + """They existed on the module function but no flag reached them, so from the command line they + were effectively hardcoded.""" + import inspect + + from vpcopilot import cli + params = inspect.signature(cli.lab_create).parameters + assert "pool_template" in params and "lb_template" in params + src = inspect.getsource(cli.lab_create) + assert "--pool-template" in src and "--lb-template" in src + assert "pool_template=pool_template" in src and "lb_template=lb_template" in src