From c5f8d0f940fc19cacb60ca953b3443196f234e6b Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Fri, 10 Jul 2026 18:01:14 +1000 Subject: [PATCH] feat(ndo): enforce service-graph device-existence + uniform site redirect (v0.24.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes F4 + F12: the sim NDO silently accepted two writes that real NDO 4.x rejects server-side (the two documented walls behind the two-phase deploy): - F4: binding a site-local service graph whose serviceNodes[].device.dn does not exist on the target APIC -> now 400 "Service graph device does not exist in tenant in Fabric " (cross-plane check against the per-site APIC MITStore; single-process sandbox shares live store objects). - F12: partial per-fabric serviceGraphRelationship redirect coverage -> now 400 "must have uniform redirect policy configured on all fabrics"; validated on POST-request state (deepcopy->apply->validate->commit) so a single atomic all-fabric PATCH passes, matching real NDO. Coverage-based per (template, contract) — sibling templates with same-named contracts cannot cross-reject. Non-service-graph PATCHes keep the in-place fast path. Fail-safe default-allow; all-or-nothing on rejection. +33 tests (1131 passed). Live gate: MS-TN2 two-phase chain 12/12 green (no false-reject); phase2-only 400s with golden message + zero residue; single-fabric PATCH 400s while the atomic both-fabric control returns 200. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017zdUTqU9fvCCvF3uVGZsu1 --- CHANGELOG.md | 27 ++ aci_sim/ndo/app.py | 39 +- aci_sim/ndo/service_graph_validation.py | 379 +++++++++++++++ pyproject.toml | 2 +- tests/test_f12_uniform_redirect.py | 609 ++++++++++++++++++++++++ tests/test_f4_service_graph_device.py | 389 +++++++++++++++ 6 files changed, 1442 insertions(+), 3 deletions(-) create mode 100644 aci_sim/ndo/service_graph_validation.py create mode 100644 tests/test_f12_uniform_redirect.py create mode 100644 tests/test_f4_service_graph_device.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f7087b0..a73c510 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,33 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) (pre-1.0: minor bumps may include breaking changes to the sim's behavior). +## [0.24.0] - 2026-07-10 + +### Added +- **NDO service-graph server-side enforcement (F4 + F12)** — the sim's NDO now + faithfully rejects, at `PATCH /mso/api/v1/schemas/{id}` time, the two + documented real-NDO 4.x validation walls that drove the two-phase deploy + workflow (new `aci_sim/ndo/service_graph_validation.py`): + - **F4 — device-existence (NDO<->APIC ordering)**: binding a site-local + service graph whose `serviceNodes[].device.dn` (`uni/tn-/lDevVip-`) + does not exist on the target site's APIC returns + `400 "Service graph device does not exist in tenant in Fabric + "` — matching real NDO. Legal two-phase flows (phase1 creates the + APIC device, phase2 binds) are unaffected; a phase2-only push is now + rejected instead of silently binding a dangling graph. + - **F12 — uniform site redirect**: per-fabric `serviceGraphRelationship` + redirect writes are validated against the POST-request state (deepcopy -> + apply -> validate -> commit): partial coverage (some but not all of a + template's fabrics configured) returns + `400 "must have uniform redirect policy configured on all fabrics"`, + while a single atomic all-fabric PATCH passes — matching real NDO's + post-state validation. Coverage-based (not DN-equality; redirect DNs are + legitimately per-fabric). Uniformity is scoped per (template, contract), + so same-named contracts in sibling templates cannot cross-reject. + Non-service-graph PATCHes keep the original in-place fast path unchanged. + Fail-safe default-allow throughout; all-or-nothing (rejected writes leave + zero residue). +33 tests. + ## [0.23.0] - 2026-07-09 ### Added diff --git a/aci_sim/ndo/app.py b/aci_sim/ndo/app.py index 521f52d..7aa3d87 100644 --- a/aci_sim/ndo/app.py +++ b/aci_sim/ndo/app.py @@ -7,6 +7,7 @@ never validated on subsequent requests (accept present-or-absent). """ from __future__ import annotations +import copy import hashlib import uuid from typing import Any @@ -18,6 +19,12 @@ from aci_sim.control.persist import apply_ndo, load_json, save_json, serialize_n from .deploy_mirror import mirror_template_to_sites from .model import NdoState from .patch import PatchError, apply_json_patch, normalize_site, normalize_template +from .service_graph_validation import ( + ServiceGraphValidationError, + _validate_service_graph_device_refs, + _validate_uniform_site_redirect, + service_graph_relevant, +) def make_ndo_app(state: NdoState, apic_states: dict[str, Any] | None = None) -> FastAPI: @@ -748,9 +755,37 @@ def make_ndo_app(state: NdoState, apic_states: dict[str, Any] | None = None) -> # PATCH client-side and never sends it, but stay tolerant. return detail + # F4 + F12 (design doc `_F4_F12_VALIDATION_DESIGN.md`): only PATCHes + # that actually touch service-graph surface (a site-local + # `serviceGraphs` add/replace, or a site-local contract's + # `serviceGraphRelationship`) take the gated copy-validate-commit + # path below. Every other PATCH (the ~95% that are BD/EPG/subnet/ + # contract-filter) stays on the original in-place fast path, + # byte-identical to before this change. try: - apply_json_patch(detail, ops) - except PatchError as exc: + if not service_graph_relevant(ops): + apply_json_patch(detail, ops) + else: + # F4 is a pure reference check — device existence doesn't + # depend on the mutation — so it pre-scans `ops` against the + # CURRENT (pre-mutation) `detail` before anything is applied. + # Raising here is naturally all-or-nothing (design §2.2). + _validate_service_graph_device_refs(ops, apic_states, state, detail) + + # F12 is a post-request-final-state check (design §3.1): it + # cannot be evaluated per-op without false-rejecting the + # legal atomic multi-fabric PATCH mid-batch. Apply the ops to + # a deepcopy, validate the RESULT, and only commit the copy + # back into state.schema_details on success — a 400 here + # leaves the live stored schema completely untouched (and, + # incidentally, fixes the pre-existing partial-write-on- + # PatchError bug for service-graph PATCHes — design §3.2). + working = copy.deepcopy(detail) + apply_json_patch(working, ops) + _validate_uniform_site_redirect(working, ops) + state.schema_details[schema_id] = working + detail = working + except (PatchError, ServiceGraphValidationError) as exc: raise HTTPException(status_code=400, detail=str(exc)) from exc detail["id"] = schema_id # ops must never be able to clobber the id diff --git a/aci_sim/ndo/service_graph_validation.py b/aci_sim/ndo/service_graph_validation.py new file mode 100644 index 0000000..5dc546d --- /dev/null +++ b/aci_sim/ndo/service_graph_validation.py @@ -0,0 +1,379 @@ +"""F4 + F12 — NDO server-side service-graph fidelity enforcements. + +See `_F4_F12_VALIDATION_DESIGN.md` (repo root) for the full design. Summary: + +- **F4 — device-existence gate.** When a site-local service-graph binding op + (`/sites/{siteId}-{templateName}/serviceGraphs/...`) references an L4-L7 + device (`serviceNodes[].device.dn = uni/tn-/lDevVip-`), that + device must already exist as a `vnsLDevVip` MO on the TARGET SITE's APIC + (cross-plane check via `apic_states`). A phase2-only bind (device never + created in phase1) is the confirmed sim gap this closes. +- **F12 — uniform per-fabric redirect gate.** A per-fabric redirect + (`serviceGraphRelationship` carrying a `redirectPolicy`) written onto a + site-local contract must land on EVERY fabric of that contract's template + in the SAME request (NDO validates the final post-request state, not + per-op) — a partial (single-fabric) write is illegal; one atomic + multi-op PATCH covering every fabric is legal. + +Both enforcements are fail-safe / default-allow (design §4): they only raise +on a *proven* violation with the real-gear golden string; every ambiguity +(unparsable DN, unsimulated site, missing key, empty op set, single-fabric +topology) passes through untouched. Mounted from `ndo/app.py::patch_schema` +only — `apply_json_patch` (`ndo/patch.py`) stays pure and unaware of either +rule (the template PATCH route reuses it and has no service graphs). +""" +from __future__ import annotations + +import re +from typing import Any + + +class ServiceGraphValidationError(Exception): + """Raised when a service-graph PATCH violates an NDO server-side + structural fidelity rule (F4 device-existence or F12 uniform redirect). + + Caught by `ndo/app.py::patch_schema` and mapped to + `HTTPException(status_code=400, detail=str(exc))` — the same 400 + envelope `PatchError` already uses on that route. + """ + + +# --------------------------------------------------------------------------- +# Shared gate — does this PATCH's op list touch service-graph surface at all? +# --------------------------------------------------------------------------- +# +# `patch_schema` uses this to decide whether to take the gated +# copy-validate-commit path (needed for F12's post-request-state check) or +# leave the ~95% of PATCHes that are BD/EPG/subnet/contract-filter on the +# current in-place fast path, byte-identical (design §3.2/§4). + + +def _touches_service_graph(op: Any) -> bool: + """True if a single op's path names either service-graph surface: + the site-local `serviceGraphs` collection (F4's trigger) or a + site-local contract's `serviceGraphRelationship` (F12's trigger).""" + if not isinstance(op, dict): + return False + path = op.get("path") + if not isinstance(path, str): + return False + return "/serviceGraphs" in path or path.endswith("/serviceGraphRelationship") + + +def service_graph_relevant(ops: list[dict] | None) -> bool: + """True if any op in *ops* touches service-graph surface — the gate for + `patch_schema`'s copy-validate-commit path.""" + return any(_touches_service_graph(op) for op in (ops or [])) + + +# --------------------------------------------------------------------------- +# F4 — device-existence gate at service-graph bind +# --------------------------------------------------------------------------- + +#: `serviceNodes[].device.dn` shape (design §1.3): +#: `uni/tn-/lDevVip-`. Anything that doesn't match this exact +#: device-ref shape is not a ref F4 owns — skip it (fail-safe). +_DEVICE_DN_RE = re.compile(r"^uni/tn-(?P[^/]+)/lDevVip-(?P.+)$") + + +def _iter_device_dns(value: Any): + """Yield every `device.dn` string carried by a service-graph op's + *value* — both the whole-object shape (`serviceNodes[*].device.dn`) and + a top-level `value["device"]["dn"]` (forward-safety for a narrower + single-node add/replace, design §2.3 step 3).""" + if not isinstance(value, dict): + return + device = value.get("device") + if isinstance(device, dict): + dn = device.get("dn") + if isinstance(dn, str) and dn: + yield dn + nodes = value.get("serviceNodes") + if isinstance(nodes, list): + for node in nodes: + if not isinstance(node, dict): + continue + node_device = node.get("device") + if isinstance(node_device, dict): + dn = node_device.get("dn") + if isinstance(dn, str) and dn: + yield dn + + +def _fabric_name(state: Any, site_id: str) -> str: + """Resolve *site_id* to its NDO fabric display name + (`NdoState.sites[].name`, e.g. `"LAB1-IT-ACI"`), falling back to the + bare site_id if the site isn't found (should not normally happen, but + keep the golden-message builder total).""" + for entry in getattr(state, "sites", None) or []: + if isinstance(entry, dict) and str(entry.get("id")) == site_id: + name = entry.get("name") + if name: + return name + return site_id + + +def _validate_service_graph_device_refs( + ops: list[dict], apic_states: dict | None, state: Any, detail: dict | None +) -> None: + """Pre-scan *ops* (BEFORE `apply_json_patch` mutates *detail*) for + site-local service-graph device bindings and raise + `ServiceGraphValidationError` if a referenced device doesn't exist as a + `vnsLDevVip` MO on the target site's APIC store. + + *detail* is the CURRENT (pre-mutation) schema doc — needed to resolve + the `{siteId}-{templateName}` composite path segment to a concrete + `siteId` via `detail["sites"]`, the same composite `_find_by_name` + (`ndo/patch.py`) uses. Not part of the design doc's abbreviated 3-arg + signature shorthand, but required by its own §2.3 algorithm (which + reads `detail["sites"]`) — see this module's docstring / the + implementer's report for this minor, functionally-necessary deviation. + + Fail-safe on every ambiguity (design §4): missing ops/apic_states/ + detail, an unresolvable site composite, an unparsable device DN, a + site absent from *apic_states* (not simulated) — all pass through. + Only raises when a device DN parses AND its site IS simulated AND the + store proves the device absent (or present under the wrong class). + """ + if not ops or not apic_states or not detail: + return + + sites = detail.get("sites") + if not isinstance(sites, list) or not sites: + return + + for entry in ops: + if not isinstance(entry, dict) or entry.get("op") not in ("add", "replace"): + continue + path = entry.get("path") + if not isinstance(path, str) or not path.startswith("/sites/") or "/serviceGraphs" not in path: + continue + + # design §2.3 step 1: `seg = path.split("/")[2]` — the + # "{siteId}-{templateName}" composite. Never split on "-" + # (template names may contain it). + segments = path.split("/") + if len(segments) < 3: + continue + seg = segments[2] + + site_entry = None + for candidate in sites: + if not isinstance(candidate, dict): + continue + if f"{candidate.get('siteId')}-{candidate.get('templateName')}" == seg: + site_entry = candidate + break + if site_entry is None: + # Unresolvable composite — can't prove anything. Fail-safe. + continue + + site_id = str(site_entry.get("siteId", "")) + if not site_id: + continue + + for dn in _iter_device_dns(entry.get("value")): + match = _DEVICE_DN_RE.match(dn) + if not match: + # Not a device ref this rule owns — fail-safe skip. + continue + + apic_state = apic_states.get(site_id) + if apic_state is None: + # Site not simulated in this process — can't prove + # absence. Fail-safe skip (design §2.3 step 3 bullet 2). + continue + store = getattr(apic_state, "store", None) + if store is None: + continue + + mo = store.get(dn) + if mo is not None and getattr(mo, "class_name", None) == "vnsLDevVip": + continue # device exists — legal (phase1 already built it) + + tenant = match.group("tenant") + dev = match.group("dev") + fabric_name = _fabric_name(state, site_id) + raise ServiceGraphValidationError( + f"Service graph device {dev} does not exist in tenant {tenant} " + f"in Fabric {fabric_name}" + ) + + +# --------------------------------------------------------------------------- +# F12 — uniform per-fabric redirect gate +# --------------------------------------------------------------------------- + +#: Matches a site-local contract's redirect bind path exactly: +#: `/sites/{siteId}-{templateName}/contracts/{contract}/serviceGraphRelationship`. +#: Template-level graph binds (`/templates/...`) never match this (they +#: don't start with `/sites/`), so the template-level "Bind Service Graph to +#: Contract" task never trips F12 (design §3.3 step 1 / §4). +#: +#: The composite `{siteId}-{templateName}` segment is captured too (not just +#: split off) — see `_resolve_template_name`, which resolves it back to a +#: templateName so a touched op can be scoped to ITS OWN template (fixes the +#: cross-template false-reject below). +_CONTRACT_REDIRECT_PATH_RE = re.compile( + r"^/sites/(?P[^/]+)/contracts/(?P[^/]+)/serviceGraphRelationship$" +) + + +def _basename(ref: Any) -> str: + """Bare object name from a canonical NDO string ref + (`/schemas/.../contracts/con-X` -> `con-X`). `working` is the + POST-`apply_json_patch` document, so every `contractRef` has already + been through `_stringify_refs` — no dict-form handling needed here + (unlike `deploy_mirror._basename`, which must also read PRE-patch dict + refs).""" + if isinstance(ref, str) and ref: + return ref.rsplit("/", 1)[-1] + return "" + + +def _value_has_redirect_policy(value: Any) -> bool: + """True if a `serviceGraphRelationship` *value* carries at least one + `serviceNodesRelationship[*].{consumerConnector|providerConnector} + .redirectPolicy` — i.e. it's an actual redirect bind, not merely a + graph-to-contract bind with no redirect (design §3.3 step 1 / §1.4). + + Used BOTH to decide whether an incoming op counts as "touching" a + redirect (trigger detection) AND, on the post-state `working` doc, to + decide whether a given fabric's site-local contract shadow already + carries a redirect (coverage detection) — one shared predicate keeps + the two checks from silently drifting apart. + """ + if not isinstance(value, dict): + return False + nodes = value.get("serviceNodesRelationship") + if not isinstance(nodes, list): + return False + for node in nodes: + if not isinstance(node, dict): + continue + for connector_key in ("consumerConnector", "providerConnector"): + connector = node.get(connector_key) + if isinstance(connector, dict) and connector.get("redirectPolicy"): + return True + return False + + +def _resolve_template_name(sites: list, seg: str) -> str | None: + """Resolve a `{siteId}-{templateName}` composite path segment (*seg*) + back to its bare templateName via *sites* (`working["sites"]`) — the + same composite `_find_by_name` (`ndo/patch.py`) uses to address a + site-local op's target entry in the first place, and the same + resolution `_validate_service_graph_device_refs` (F4, above) already + does against `detail["sites"]`. Never split on "-" (template names may + legitimately contain it). Returns None if the composite can't be + resolved (fail-safe — see caller).""" + for candidate in sites or []: + if not isinstance(candidate, dict): + continue + if f"{candidate.get('siteId')}-{candidate.get('templateName')}" == seg: + return candidate.get("templateName") + return None + + +def _touched_redirect_contracts(sites: list, ops: list[dict] | None) -> set[tuple[str, str]]: + """`(templateName, contract)` pairs touched by a redirect-carrying + site-local `serviceGraphRelationship` op in *ops* (design §3.3 step 1). + + Carries the owning templateName alongside the bare contract name — + two DIFFERENT templates in the same schema may legitimately carry a + same-named contract (e.g. `con-X` in both "LAB1-LAB2" and "LAB1"), and + a touched op only proves ITS OWN template was touched. A bare-name-only + touched set (pre-fix) let `_validate_uniform_site_redirect` sweep every + template's same-named contract into one coverage check, so a legal + atomic write on template A's `con-X` could be 400'd by template B's + unrelated, pre-existing non-uniform `con-X` state — a scope-invariant + violation (design §3.3 step 3: "only contracts touched by THIS + request... never retroactively reject unrelated/pre-existing state"). + *sites* (`working["sites"]`) resolves each op's composite path segment + to a concrete templateName; an unresolvable composite is fail-safe + skipped (can't prove which template was touched, so can't prove a + violation either).""" + touched: set[tuple[str, str]] = set() + for op in ops or []: + if not isinstance(op, dict) or op.get("op") not in ("add", "replace"): + continue + path = op.get("path") + if not isinstance(path, str): + continue + match = _CONTRACT_REDIRECT_PATH_RE.match(path) + if not match: + continue + if not _value_has_redirect_policy(op.get("value")): + continue + template_name = _resolve_template_name(sites, match.group("site_tmpl")) + if template_name is None: + continue + touched.add((template_name, match.group("contract"))) + return touched + + +def _site_configures_redirect(site_entry: dict, contract: str) -> bool: + """True if *site_entry*'s mirrored `contracts[]` shadow for *contract* + already carries a redirect-policy-bearing `serviceGraphRelationship`.""" + for candidate in site_entry.get("contracts", []) or []: + if not isinstance(candidate, dict): + continue + if _basename(candidate.get("contractRef")) != contract: + continue + return _value_has_redirect_policy(candidate.get("serviceGraphRelationship")) + return False + + +def _validate_uniform_site_redirect(working: dict, ops: list[dict]) -> None: + """Post-request-state check (design §3.1/§3.3): after *ops* have been + applied into *working* (a deepcopy — see `patch_schema`'s + copy-validate-commit), every (template, contract) pair touched by a + redirect bind in *ops* must carry that redirect on ALL fabrics of ITS + OWN template — never on only SOME. Raises `ServiceGraphValidationError` + with the golden message on a partial (non-uniform) write. + + "Uniform" is COVERAGE (presence of a redirectPolicy on every fabric), + never DN equality — redirect-policy DNs are legitimately per-fabric + (design §3.4). Only the (template, contract) pairs actually touched by + THIS request's ops are checked, so pre-existing state, unrelated + contracts, AND a differently-scoped same-named contract in a sibling + template are never retroactively rejected (design §3.3 step 3 — see + `_touched_redirect_contracts`'s docstring for the cross-template + false-reject this scoping fixes). A template with only one associated + fabric is trivially uniform (design §3.4 / §4). + """ + sites = working.get("sites") + if not isinstance(sites, list): + return + + touched = _touched_redirect_contracts(sites, ops) + if not touched: + return + + for template_name, contract in touched: + # The coverage set is every site entry belonging to THIS touched + # op's OWN template that also carries a mirrored contracts[] shadow + # for this contract — never a same-named contract in a different + # template (that was the bug: grouping by templateName across ALL + # site entries carrying the bare contract name, regardless of + # which template the incoming op actually touched). + group = [ + site_entry + for site_entry in sites + if isinstance(site_entry, dict) + and site_entry.get("templateName") == template_name + and any( + isinstance(c, dict) and _basename(c.get("contractRef")) == contract + for c in (site_entry.get("contracts") or []) + ) + ] + + total = len(group) + if total <= 1: + # Single-fabric topology — trivially uniform (design §3.4). + continue + configured = sum(1 for s in group if _site_configures_redirect(s, contract)) + if 0 < configured < total: + raise ServiceGraphValidationError( + "must have uniform redirect policy configured on all fabrics" + ) diff --git a/pyproject.toml b/pyproject.toml index 6e5e2d5..35fa625 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "aci-sim" -version = "0.23.0" +version = "0.24.0" description = "Faithful REST simulator of a 2-site Cisco ACI fabric (per-site APIC + ND/NDO)" readme = "README.md" license = "PolyForm-Noncommercial-1.0.0" diff --git a/tests/test_f12_uniform_redirect.py b/tests/test_f12_uniform_redirect.py new file mode 100644 index 0000000..4bd5a7e --- /dev/null +++ b/tests/test_f12_uniform_redirect.py @@ -0,0 +1,609 @@ +"""F12 — NDO uniform per-fabric service-graph redirect gate. + +Design: `_F4_F12_VALIDATION_DESIGN.md` §3 (repo root). Confirmed sim gap: a +single-fabric (partial) `serviceGraphRelationship` redirect write used to +succeed in the sim while real NDO 400s with `must have uniform redirect +policy configured on all fabrics` — NDO validates the FINAL POST-REQUEST +state, so a legal atomic multi-op PATCH covering every fabric of a +template in ONE request must still pass. + +Replicates the mso-model role's "Atomic PATCH — bind service-graph redirect +on ALL fabrics in one request" task (design §1.4) — the exact shape +`tests/test_pr13_ndo_dhcp_svcgraph.py:: +test_atomic_service_graph_redirect_patch_on_site_local_contract_end_to_end` +already exercises (that pre-existing test's own atomic 2-op PATCH must stay +green under F12 — this file duplicates its shape as a dedicated regression +anchor), then adds the partial-write / single-fabric-topology / fail-safe +cases design §4.1's evidence-gate table calls for. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + +from aci_sim.ndo.app import make_ndo_app +from aci_sim.ndo.model import build_ndo_model +from aci_sim.ndo.service_graph_validation import ( + ServiceGraphValidationError, + _validate_uniform_site_redirect, + service_graph_relevant, +) +from aci_sim.topology.loader import load_topology + +TOPOLOGY_YAML = Path(__file__).parent.parent / "topology.yaml" + + +@pytest.fixture() +def topo(): + return load_topology(TOPOLOGY_YAML) + + +@pytest.fixture() +def client(topo): + state = build_ndo_model(topo, sandbox=True) + app = make_ndo_app(state) + with TestClient(app) as c: + yield c + + +def _redirect_value(schema_id, tmpl_name, consumer_dn, provider_dn): + """A per-fabric redirect bind — design §1.4 wire shape. Consumer/ + provider redirect-policy DNs are deliberately DISTINCT per call site, + matching the real per-fabric DNs the design's §3.4 "coverage, not + equality" rule is built around.""" + return { + "serviceGraphRef": { + "schemaId": schema_id, + "serviceGraphName": "sgt-FW_LAB0", + "templateName": tmpl_name, + }, + "serviceNodesRelationship": [ + { + "serviceNodeRef": { + "schemaId": schema_id, + "serviceGraphName": "sgt-FW_LAB0", + "serviceNodeName": "node1", + "templateName": tmpl_name, + }, + "consumerConnector": { + "clusterInterface": {"dn": "uni/tn-MS-TN2/x"}, + "redirectPolicy": {"dn": consumer_dn}, + "subnets": [], + }, + "providerConnector": { + "clusterInterface": {"dn": "uni/tn-MS-TN2/x"}, + "redirectPolicy": {"dn": provider_dn}, + }, + } + ], + } + + +def _two_site_schema(client): + payload = { + "displayName": "F12-TEST-MS", + "templates": [ + {"name": "LAB1-LAB2", "displayName": "LAB1-LAB2", "tenantId": "t-f12", "contracts": []} + ], + "sites": [ + {"siteId": "1", "templateName": "LAB1-LAB2"}, + {"siteId": "2", "templateName": "LAB1-LAB2"}, + ], + } + schema_id = client.post("/mso/api/v1/schemas", json=payload).json()["id"] + client.patch( + f"/mso/api/v1/schemas/{schema_id}", + json=[ + { + "op": "add", + "path": "/templates/LAB1-LAB2/contracts/-", + "value": {"name": "con-Firewall_LAB0", "scope": "vrf", "filterRelationships": []}, + } + ], + ) + return schema_id + + +def _one_site_schema(client): + payload = { + "displayName": "F12-TEST-SF", + "templates": [ + {"name": "LAB1-only", "displayName": "LAB1-only", "tenantId": "t-f12-sf", "contracts": []} + ], + "sites": [{"siteId": "1", "templateName": "LAB1-only"}], + } + schema_id = client.post("/mso/api/v1/schemas", json=payload).json()["id"] + client.patch( + f"/mso/api/v1/schemas/{schema_id}", + json=[ + { + "op": "add", + "path": "/templates/LAB1-only/contracts/-", + "value": {"name": "con-Firewall_LAB0", "scope": "vrf", "filterRelationships": []}, + } + ], + ) + return schema_id + + +# --------------------------------------------------------------------------- +# App-level: the legal atomic flow that MUST stay green (design §4.1) +# --------------------------------------------------------------------------- + + +def test_atomic_all_fabric_redirect_in_one_patch_passes(client): + schema_id = _two_site_schema(client) + ops = [ + { + "op": "add", + "path": f"/sites/{site_id}-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value( + schema_id, "LAB1-LAB2", f"uni/tn-MS-TN2/rp-{site_id}-c", f"uni/tn-MS-TN2/rp-{site_id}-p" + ), + } + for site_id in ("1", "2") + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + fresh = client.get(f"/mso/api/v1/schemas/{schema_id}").json() + for site_id in ("1", "2"): + site = next(s for s in fresh["sites"] if s["siteId"] == site_id) + contract = next( + c for c in site["contracts"] if c["contractRef"].endswith("/contracts/con-Firewall_LAB0") + ) + assert contract["serviceGraphRelationship"]["serviceGraphRef"].endswith( + "/serviceGraphs/sgt-FW_LAB0" + ) + + +# --------------------------------------------------------------------------- +# The confirmed violation F12 closes: a single-fabric PARTIAL write +# --------------------------------------------------------------------------- + + +def test_single_fabric_partial_write_400s_with_golden_message(client): + schema_id = _two_site_schema(client) + ops = [ + { + "op": "add", + "path": "/sites/1-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value(schema_id, "LAB1-LAB2", "uni/tn-MS-TN2/rp-1-c", "uni/tn-MS-TN2/rp-1-p"), + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 400 + assert resp.json()["detail"] == "must have uniform redirect policy configured on all fabrics" + + +def test_single_fabric_partial_write_does_not_mutate_stored_schema(client): + """All-or-nothing: a rejected partial redirect must not leave site '1' + carrying the redirect either — the copy-validate-commit path only + commits `working` back into `state.schema_details` on success (design + §3.2, incidentally also fixing the pre-existing partial-write-on- + PatchError bug for these PATCHes).""" + schema_id = _two_site_schema(client) + ops = [ + { + "op": "add", + "path": "/sites/1-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value(schema_id, "LAB1-LAB2", "uni/tn-MS-TN2/rp-1-c", "uni/tn-MS-TN2/rp-1-p"), + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 400 + + fresh = client.get(f"/mso/api/v1/schemas/{schema_id}").json() + site1 = next(s for s in fresh["sites"] if s["siteId"] == "1") + contract = next( + c for c in site1["contracts"] if c["contractRef"].endswith("/contracts/con-Firewall_LAB0") + ) + assert contract.get("serviceGraphRelationship") is None + + +def test_second_atomic_patch_after_rejected_partial_still_succeeds(client): + """A retried, now-complete atomic PATCH (covering both fabrics) after + an earlier rejected partial write must still succeed cleanly — proves + the rejected write left no residue that could poison a later uniform + check.""" + schema_id = _two_site_schema(client) + bad_ops = [ + { + "op": "add", + "path": "/sites/1-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value(schema_id, "LAB1-LAB2", "uni/tn-MS-TN2/rp-1-c", "uni/tn-MS-TN2/rp-1-p"), + } + ] + assert client.patch(f"/mso/api/v1/schemas/{schema_id}", json=bad_ops).status_code == 400 + + good_ops = [ + { + "op": "add", + "path": f"/sites/{site_id}-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value( + schema_id, "LAB1-LAB2", f"uni/tn-MS-TN2/rp-{site_id}-c", f"uni/tn-MS-TN2/rp-{site_id}-p" + ), + } + for site_id in ("1", "2") + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=good_ops) + assert resp.status_code == 200 + + +# --------------------------------------------------------------------------- +# Single-fabric topology — trivially uniform (design §3.4 / §4) +# --------------------------------------------------------------------------- + + +def test_single_fabric_topology_redirect_write_passes(client): + schema_id = _one_site_schema(client) + ops = [ + { + "op": "add", + "path": "/sites/1-LAB1-only/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value(schema_id, "LAB1-only", "uni/tn-SF/rp-c", "uni/tn-SF/rp-p"), + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + +# --------------------------------------------------------------------------- +# Cross-template scope regression (reviewer-confirmed MEDIUM false-reject) +# +# `_touched_redirect_contracts` used to collect BARE contract names, losing +# which template the touched op actually belonged to. +# `_validate_uniform_site_redirect`'s coverage grouping then evaluated +# EVERY template's same-named contract, not just the touched op's own +# template — so a legal atomic full-fabric redirect PATCH on template A's +# `con-X` could be 400'd purely because an UNRELATED template B also had a +# `con-X` sitting in a pre-existing non-uniform state. Fixed by scoping the +# touched set to `(templateName, contract)` pairs (design §3.3 step 3's +# scope invariant — "only contracts touched by THIS request... never +# retroactively reject unrelated state" — extended to also mean "never a +# different template's same-named contract"). +# --------------------------------------------------------------------------- + + +def _two_template_schema_with_non_uniform_sibling(client): + """A schema born (via a single `POST /schemas`, the body stored + verbatim per `create_schema`'s own docstring) already carrying TWO + templates that both have a contract named `con-Firewall_LAB0`: + + - "LAB1-LAB2": 2 fabrics (sites "1","2"), NEITHER configured yet — + the template under test. + - "LAB1": 2 fabrics (sites "1","2" again — a site can legitimately + be associated with more than one template in the same schema), + already non-uniform: fabric "1" carries a redirect, fabric "2" + does not. + + This is the reviewer's probe scenario reached the only way it's + actually reachable: as the schema's STARTING state (e.g. an + imported/migrated schema), never by hand-mutating in-process Python + state — F12's own write gate makes a non-uniform state unreachable via + any PATCH once a contract has 2+ associated fabrics, so + "LAB1"'s non-uniform shadow can only be POSTed directly. The exact + schema_id prefix on each `contractRef` is irrelevant to `_basename` + (design: bare trailing segment only), so a placeholder is fine. + """ + contract_tmpl = {"name": "con-Firewall_LAB0", "scope": "vrf", "filterRelationships": []} + payload = { + "displayName": "F12-XTMPL-TEST", + "templates": [ + {"name": "LAB1-LAB2", "displayName": "LAB1-LAB2", "tenantId": "t-f12x", "contracts": [contract_tmpl]}, + {"name": "LAB1", "displayName": "LAB1", "tenantId": "t-f12x", "contracts": [contract_tmpl]}, + ], + "sites": [ + { + "siteId": "1", + "templateName": "LAB1-LAB2", + "contracts": [{"contractRef": "/schemas/x/templates/LAB1-LAB2/contracts/con-Firewall_LAB0"}], + }, + { + "siteId": "2", + "templateName": "LAB1-LAB2", + "contracts": [{"contractRef": "/schemas/x/templates/LAB1-LAB2/contracts/con-Firewall_LAB0"}], + }, + { + "siteId": "1", + "templateName": "LAB1", + "contracts": [ + { + "contractRef": "/schemas/x/templates/LAB1/contracts/con-Firewall_LAB0", + "serviceGraphRelationship": _redirect_value( + "x", "LAB1", "uni/tn-X/rp-b1-c", "uni/tn-X/rp-b1-p" + ), + } + ], + }, + { + "siteId": "2", + "templateName": "LAB1", + "contracts": [{"contractRef": "/schemas/x/templates/LAB1/contracts/con-Firewall_LAB0"}], + }, + ], + } + return client.post("/mso/api/v1/schemas", json=payload).json()["id"] + + +def test_atomic_full_fabric_redirect_not_rejected_by_unrelated_template_same_contract_name(client): + """The reviewer's confirmed MEDIUM false-reject: a second template + ("LAB1", same schema) also carries a `con-Firewall_LAB0` contract and + is already non-uniform (1-of-2 fabrics configured). A legal atomic + full-fabric redirect PATCH on the UNRELATED template "LAB1-LAB2"'s own + `con-Firewall_LAB0` must still succeed — pre-fix this 400'd, sunk by + template "LAB1"'s unrelated non-uniform state leaking into the + coverage check via the old bare-contract-name touched set.""" + schema_id = _two_template_schema_with_non_uniform_sibling(client) + + ops = [ + { + "op": "add", + "path": f"/sites/{site_id}-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value( + schema_id, "LAB1-LAB2", f"uni/tn-MS-TN2/rp-{site_id}-c", f"uni/tn-MS-TN2/rp-{site_id}-p" + ), + } + for site_id in ("1", "2") + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + fresh = client.get(f"/mso/api/v1/schemas/{schema_id}").json() + for site_id in ("1", "2"): + site = next(s for s in fresh["sites"] if s["siteId"] == site_id and s["templateName"] == "LAB1-LAB2") + contract = next( + c for c in site["contracts"] if c["contractRef"].endswith("/contracts/con-Firewall_LAB0") + ) + assert contract["serviceGraphRelationship"]["serviceGraphRef"].endswith( + "/serviceGraphs/sgt-FW_LAB0" + ) + + # Template "LAB1"'s own unrelated non-uniform state is untouched by any + # of this — still 1-of-2, never retroactively "fixed" or re-evaluated, + # and never what actually gated the request above. + lab1_sites = [s for s in fresh["sites"] if s["templateName"] == "LAB1"] + configured_count = sum( + 1 + for s in lab1_sites + for c in s.get("contracts", []) + if c.get("contractRef", "").endswith("/contracts/con-Firewall_LAB0") + and c.get("serviceGraphRelationship") is not None + ) + assert configured_count == 1 + + +def test_own_template_partial_write_still_rejected_with_unrelated_sibling_non_uniform(client): + """Reverse check, same fixture: with template "LAB1" ALSO already + non-uniform, a PARTIAL (single-fabric) redirect write on template + "LAB1-LAB2"'s own `con-Firewall_LAB0` must still 400 with the golden + message — the (templateName, contract) scoping fix must narrow F12's + check to the touched op's own template, not accidentally widen it into + "never raise for this contract name again".""" + schema_id = _two_template_schema_with_non_uniform_sibling(client) + + ops = [ + { + "op": "add", + "path": "/sites/1-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value(schema_id, "LAB1-LAB2", "uni/tn-MS-TN2/rp-1-c", "uni/tn-MS-TN2/rp-1-p"), + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 400 + assert resp.json()["detail"] == "must have uniform redirect policy configured on all fabrics" + + +# --------------------------------------------------------------------------- +# Fail-safe / default-allow — never reject on ambiguity (design §4) +# --------------------------------------------------------------------------- + + +def test_template_level_graph_bind_never_triggers_f12(client): + """The template-level "Bind Service Graph to Contract" task (no + redirect, `/templates/...`) must never trip F12 (design §3.3/§4) — + even though its path also ends `/serviceGraphRelationship` (so the + sg_relevant gate DOES engage the copy path), F12's own contract-touch + scan requires a `/sites/...` prefix and must find nothing to check.""" + schema_id = _two_site_schema(client) + ops = [ + { + "op": "add", + "path": "/templates/LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": { + "serviceGraphRef": { + "schemaId": schema_id, + "serviceGraphName": "sgt-FW_LAB0", + "templateName": "LAB1-LAB2", + }, + "serviceNodesRelationship": [], + }, + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + +def test_non_service_graph_patch_untouched(client): + """A plain BD add must never engage the copy-validate-commit path, and + must succeed exactly as before F4/F12 existed — a named regression + anchor for the "byte-identical fast path" guarantee (design §3.2/§4), + on top of the full pre-existing NDO suite this same wiring is verified + against.""" + schema_id = _two_site_schema(client) + ops = [ + { + "op": "add", + "path": "/templates/LAB1-LAB2/bds/-", + "value": {"name": "bd-App1_LAB0", "displayName": "bd-App1_LAB0", "subnets": []}, + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + +def test_service_graph_relevant_gate_scope(): + assert ( + service_graph_relevant([{"op": "add", "path": "/sites/1-LAB1/serviceGraphs/-", "value": {}}]) + is True + ) + assert ( + service_graph_relevant( + [{"op": "add", "path": "/sites/1-LAB1/contracts/c/serviceGraphRelationship", "value": {}}] + ) + is True + ) + assert service_graph_relevant([{"op": "add", "path": "/templates/LAB1/bds/-", "value": {}}]) is False + assert service_graph_relevant([]) is False + assert service_graph_relevant(None) is False + + +# --------------------------------------------------------------------------- +# Unit level: _validate_uniform_site_redirect directly +# --------------------------------------------------------------------------- + + +def _contract_entry(configured: bool, rp_suffix: str) -> dict: + entry = {"contractRef": "/schemas/s/templates/LAB1-LAB2/contracts/con-Firewall_LAB0"} + if configured: + entry["serviceGraphRelationship"] = _redirect_value( + "s", "LAB1-LAB2", f"uni/tn-X/rp-{rp_suffix}-c", f"uni/tn-X/rp-{rp_suffix}-p" + ) + return entry + + +def _working_doc(site1_configured: bool, site2_configured: bool) -> dict: + return { + "sites": [ + {"siteId": "1", "templateName": "LAB1-LAB2", "contracts": [_contract_entry(site1_configured, "1")]}, + {"siteId": "2", "templateName": "LAB1-LAB2", "contracts": [_contract_entry(site2_configured, "2")]}, + ] + } + + +def _touch_op(site_id: str) -> dict: + return { + "op": "add", + "path": f"/sites/{site_id}-LAB1-LAB2/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value("s", "LAB1-LAB2", "uni/tn-X/rp-c", "uni/tn-X/rp-p"), + } + + +def test_unit_uniform_both_configured_passes(): + working = _working_doc(True, True) + _validate_uniform_site_redirect(working, [_touch_op("1"), _touch_op("2")]) # must not raise + + +def test_unit_partial_one_configured_raises(): + working = _working_doc(True, False) + with pytest.raises( + ServiceGraphValidationError, match="must have uniform redirect policy configured on all fabrics" + ): + _validate_uniform_site_redirect(working, [_touch_op("1")]) + + +def test_unit_pre_existing_non_uniform_state_not_retroactively_rejected(): + """Design §3.3 step 3: only contracts TOUCHED by this request's ops are + checked — pre-existing (already non-uniform) state on an untouched + contract must never be retroactively rejected.""" + working = _working_doc(True, False) # already non-uniform pre-existing state + unrelated_op = {"op": "add", "path": "/templates/LAB1-LAB2/bds/-", "value": {}} + _validate_uniform_site_redirect(working, [unrelated_op]) # must not raise + + +def test_unit_empty_ops_is_noop(): + working = _working_doc(True, False) + _validate_uniform_site_redirect(working, []) # must not raise + + +def test_unit_single_fabric_group_never_raises(): + working = { + "sites": [ + {"siteId": "1", "templateName": "LAB1-only", "contracts": [_contract_entry(True, "1")]}, + ] + } + op = { + "op": "add", + "path": "/sites/1-LAB1-only/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value("s", "LAB1-only", "uni/tn-X/rp-c", "uni/tn-X/rp-p"), + } + _validate_uniform_site_redirect(working, [op]) # must not raise + + +# --------------------------------------------------------------------------- +# Unit level: cross-template scope regression, minimal reproduction +# +# Same reviewer-confirmed bug as the app-level +# `test_atomic_full_fabric_redirect_not_rejected_by_unrelated_template_ +# same_contract_name` above, isolated to the pure function with a +# hand-built `working` doc — the fastest, most direct probe of +# `_touched_redirect_contracts`'s (templateName, contract) scoping fix. +# --------------------------------------------------------------------------- + + +def _cross_template_working_doc( + a1_configured: bool, a2_configured: bool, b1_configured: bool, b2_configured: bool +) -> dict: + """Two templates, same schema, same-named contract `con-Firewall_LAB0`: + - "LAB1-LAB2" (template A, sites "1"/"2"): the template under test — + *a1_configured*/*a2_configured* must reflect the POST-PATCH state + (i.e. already include whatever the touched ops under test would have + applied — `_validate_uniform_site_redirect` checks `working` AFTER + `apply_json_patch`, it never applies *ops* itself; same convention + `_working_doc` above uses). + - "LAB1" (template B, sites "3"/"4"): an UNTOUCHED sibling template, + whose configured/unconfigured shape is caller-controlled — used to + seed a non-uniform sibling that must never leak into template A's + coverage check. + """ + return { + "sites": [ + {"siteId": "1", "templateName": "LAB1-LAB2", "contracts": [_contract_entry(a1_configured, "a1")]}, + {"siteId": "2", "templateName": "LAB1-LAB2", "contracts": [_contract_entry(a2_configured, "a2")]}, + {"siteId": "3", "templateName": "LAB1", "contracts": [_contract_entry(b1_configured, "b1")]}, + {"siteId": "4", "templateName": "LAB1", "contracts": [_contract_entry(b2_configured, "b2")]}, + ] + } + + +def _cross_touch_op(template_name: str, site_id: str) -> dict: + return { + "op": "add", + "path": f"/sites/{site_id}-{template_name}/contracts/con-Firewall_LAB0/serviceGraphRelationship", + "value": _redirect_value("s", template_name, f"uni/tn-X/rp-{site_id}-c", f"uni/tn-X/rp-{site_id}-p"), + } + + +def test_unit_atomic_full_fabric_write_not_rejected_by_sibling_template_non_uniform_state(): + """The reviewer's minimal probe: template "LAB1" (untouched by this + request) is non-uniform (1-of-2 fabrics configured) for + `con-Firewall_LAB0`. An atomic, full-fabric redirect write on the + UNRELATED template "LAB1-LAB2"'s own `con-Firewall_LAB0` (post-patch: + both its fabrics now configured) must not raise — pre-fix, the + bare-name touched set caused this same-named-but-different-template + contract to be swept into the same coverage check, and raised.""" + working = _cross_template_working_doc( + a1_configured=True, a2_configured=True, b1_configured=True, b2_configured=False + ) + ops = [_cross_touch_op("LAB1-LAB2", "1"), _cross_touch_op("LAB1-LAB2", "2")] + _validate_uniform_site_redirect(working, ops) # must not raise + + +def test_unit_own_template_partial_write_still_rejected_with_sibling_present(): + """Reverse check: with the exact same non-uniform sibling template + "LAB1" present, a PARTIAL (single-fabric) write on template + "LAB1-LAB2"'s own `con-Firewall_LAB0` (post-patch: only fabric "1" + configured, fabric "2" still not) must still raise — the + (templateName, contract) scoping must narrow the check to the touched + op's own template, not silently widen it to "never raise for this + contract name again".""" + working = _cross_template_working_doc( + a1_configured=True, a2_configured=False, b1_configured=True, b2_configured=False + ) + ops = [_cross_touch_op("LAB1-LAB2", "1")] + with pytest.raises( + ServiceGraphValidationError, match="must have uniform redirect policy configured on all fabrics" + ): + _validate_uniform_site_redirect(working, ops) diff --git a/tests/test_f4_service_graph_device.py b/tests/test_f4_service_graph_device.py new file mode 100644 index 0000000..4725892 --- /dev/null +++ b/tests/test_f4_service_graph_device.py @@ -0,0 +1,389 @@ +"""F4 — NDO service-graph device-existence gate. + +Design: `_F4_F12_VALIDATION_DESIGN.md` §2 (repo root). Confirmed sim gap: a +phase2-only service-graph bind (skip phase1's APIC-side `vnsLDevVip` +create) used to succeed in the sim while real NDO 400s with +`Service graph device does not exist in tenant in Fabric +`. + +Replicates the "Create service graph" task's site-local device bind +(`custom_mso_schema_service_graph.py`, design §1.3) against the real MS-TN1 +(LAB1+LAB2) topology-derived schema — the same fixture +`tests/test_pr13_ndo_dhcp_svcgraph.py`'s GAP-2 tests use — but now wires a +cross-plane APIC store via `apic_states` (mirrors +`tests/test_deploy_mirror.py`'s `_FakeApicState` fixture shape) so F4 can +prove/disprove `vnsLDevVip` existence on the target site. +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from types import SimpleNamespace + +import pytest +from fastapi.testclient import TestClient + +from aci_sim.mit.mo import MO +from aci_sim.mit.store import MITStore +from aci_sim.ndo.app import make_ndo_app +from aci_sim.ndo.model import build_ndo_model +from aci_sim.ndo.service_graph_validation import ( + ServiceGraphValidationError, + _validate_service_graph_device_refs, +) +from aci_sim.topology.loader import load_topology + +TOPOLOGY_YAML = Path(__file__).parent.parent / "topology.yaml" + +DEVICE_DN = "uni/tn-MS-TN1/lDevVip-fw-FW_LAB0" + + +@dataclass +class _FakeApicState: + """Minimal stand-in for rest_aci.app.ApicSiteState — F4 only ever + touches `.store` — mirrors test_deploy_mirror.py's own fixture.""" + + store: MITStore = field(default_factory=MITStore) + + +@pytest.fixture() +def topo(): + return load_topology(TOPOLOGY_YAML) + + +@pytest.fixture() +def apic_states(): + return {"1": _FakeApicState(), "2": _FakeApicState()} + + +@pytest.fixture() +def client(topo, apic_states): + state = build_ndo_model(topo, sandbox=True) + app = make_ndo_app(state, apic_states) + with TestClient(app) as c: + yield c + + +def _ms_tn1_schema(client): + schemas = client.get("/mso/api/v1/schemas/list-identity").json()["schemas"] + return next(s for s in schemas if s["displayName"] == "MS-TN1-schema") + + +def _bind_service_graph_ops(schema_id, tmpl_name, site_key, device_dn): + """Site-local "Create service graph" op — design §1.3 wire shape.""" + return [ + { + "op": "add", + "path": f"/sites/{site_key}/serviceGraphs/-", + "value": { + "serviceGraphRef": { + "schemaId": schema_id, + "templateName": tmpl_name, + "serviceGraphName": "sgt-FW_LAB0", + }, + "serviceNodes": [ + { + "serviceNodeRef": { + "schemaId": schema_id, + "templateName": tmpl_name, + "serviceGraphName": "sgt-FW_LAB0", + "serviceNodeName": "node1", + }, + "device": {"dn": device_dn}, + } + ], + }, + } + ] + + +def _add_template_service_graph(client, schema_id, tmpl_name): + ops = [ + { + "op": "add", + "path": f"/templates/{tmpl_name}/serviceGraphs/-", + "value": { + "name": "sgt-FW_LAB0", + "displayName": "sgt-FW_LAB0", + "serviceNodes": [ + {"name": "node1", "serviceNodeTypeId": "svc-node-type-firewall", "index": 1} + ], + }, + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema_id}", json=ops) + assert resp.status_code == 200 + + +def _site_key(detail, site_id, tmpl_name): + site = next(s for s in detail["sites"] if s["siteId"] == site_id) + return f"{site['siteId']}-{tmpl_name}" + + +# --------------------------------------------------------------------------- +# App-level: the legal two-stage flow that MUST stay green (design §4.1) +# --------------------------------------------------------------------------- + + +def test_phase2_bind_passes_when_phase1_device_already_exists(client, apic_states): + """MS-TN1 two-stage create_tenant: phase1 (aci-model raw APIC POST) + creates the vnsLDevVip on the target site FIRST; phase2's "Create + service graph" site-local bind must then succeed.""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + # phase1: APIC-side device create (aci-model role's raw aci_rest POST). + apic_states["1"].store.upsert(MO("vnsLDevVip", dn=DEVICE_DN, name="fw-FW_LAB0")) + + _add_template_service_graph(client, schema["id"], tmpl_name) + resp = client.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 200 + + fresh_site = next(s for s in resp.json()["sites"] if s["siteId"] == "1") + assert fresh_site["serviceGraphs"][0]["serviceNodes"][0]["device"]["dn"] == DEVICE_DN + + +# --------------------------------------------------------------------------- +# The confirmed sim gap F4 closes: phase2-only (skip phase1) +# --------------------------------------------------------------------------- + + +def test_phase2_only_bind_400s_with_golden_message(client): + """Skip phase1 entirely (no vnsLDevVip ever created on site '1''s + APIC) — real NDO 400s; this is the confirmed sim gap.""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + _add_template_service_graph(client, schema["id"], tmpl_name) + resp = client.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 400 + assert resp.json()["detail"] == ( + "Service graph device fw-FW_LAB0 does not exist in tenant MS-TN1 " + "in Fabric LAB1-IT-ACI" + ) + + +def test_phase2_only_bind_does_not_mutate_stored_schema(client): + """The 400 must be all-or-nothing — a rejected bind must not leave a + partial serviceGraphs entry in the stored schema (design §2.2: the + pre-scan runs BEFORE apply_json_patch, so raising is naturally + all-or-nothing).""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + _add_template_service_graph(client, schema["id"], tmpl_name) + resp = client.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 400 + + fresh = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + fresh_site = next(s for s in fresh["sites"] if s["siteId"] == "1") + assert fresh_site["serviceGraphs"] == [] + + +def test_device_present_wrong_class_400s(client, apic_states): + """A DN that resolves but under the WRONG class (not vnsLDevVip) must + be treated the same as absent — the golden message must still fire.""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + apic_states["1"].store.upsert(MO("fvTenant", dn=DEVICE_DN, name="not-a-device")) + + _add_template_service_graph(client, schema["id"], tmpl_name) + resp = client.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 400 + assert "does not exist in tenant MS-TN1" in resp.json()["detail"] + + +# --------------------------------------------------------------------------- +# Fail-safe / default-allow — never reject on ambiguity (design §4) +# --------------------------------------------------------------------------- + + +def test_no_apic_states_wired_is_fail_safe(topo): + """make_ndo_app(state) with NO apic_states at all (every pre-existing + NDO test / call site) — F4 must never fire; this is the exact shape + tests/test_pr13_ndo_dhcp_svcgraph.py's own service-graph round-trip + test uses, and it must stay green.""" + state = build_ndo_model(topo, sandbox=True) + app = make_ndo_app(state) # no apic_states + with TestClient(app) as c: + schema = _ms_tn1_schema(c) + detail = c.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + _add_template_service_graph(c, schema["id"], tmpl_name) + resp = c.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 200 + + +def test_site_not_in_apic_states_is_fail_safe(topo): + """Site '1' is associated with the template but simply isn't present + in *apic_states* (this sim process isn't simulating that fabric) — F4 + cannot prove absence, so it must pass through.""" + state = build_ndo_model(topo, sandbox=True) + app = make_ndo_app(state, {}) # apic_states present but empty + with TestClient(app) as c: + schema = _ms_tn1_schema(c) + detail = c.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + _add_template_service_graph(c, schema["id"], tmpl_name) + resp = c.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops(schema["id"], tmpl_name, site_key, DEVICE_DN), + ) + assert resp.status_code == 200 + + +def test_unparsable_device_dn_is_fail_safe(client): + """A device.dn that doesn't match the `uni/tn-/lDevVip-` shape is + not a ref F4 owns — must never be rejected.""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + site_key = _site_key(detail, "1", tmpl_name) + + _add_template_service_graph(client, schema["id"], tmpl_name) + resp = client.patch( + f"/mso/api/v1/schemas/{schema['id']}", + json=_bind_service_graph_ops( + schema["id"], tmpl_name, site_key, "uni/tn-MS-TN1/not-a-device-dn" + ), + ) + assert resp.status_code == 200 + + +def test_template_level_bind_never_triggers_f4(client): + """Template-level serviceGraphs ops (`/templates/{t}/serviceGraphs/-`) + also carry `device.dn` (design §1.3) but are NOT site-scoped — F4 must + only key on the site-local op.""" + schema = _ms_tn1_schema(client) + detail = client.get(f"/mso/api/v1/schemas/{schema['id']}").json() + tmpl_name = detail["templates"][0]["name"] + + ops = [ + { + "op": "add", + "path": f"/templates/{tmpl_name}/serviceGraphs/-", + "value": { + "name": "sgt-FW_LAB0", + "displayName": "sgt-FW_LAB0", + "serviceNodes": [ + { + "name": "node1", + "serviceNodeTypeId": "svc-node-type-firewall", + "index": 1, + "device": {"dn": "uni/tn-MS-TN1/lDevVip-does-not-exist"}, + } + ], + }, + } + ] + resp = client.patch(f"/mso/api/v1/schemas/{schema['id']}", json=ops) + assert resp.status_code == 200 + + +# --------------------------------------------------------------------------- +# Unit level: _validate_service_graph_device_refs directly +# --------------------------------------------------------------------------- + + +def _bare_detail(site_id="1", tmpl="LAB1"): + return {"sites": [{"siteId": site_id, "templateName": tmpl, "serviceGraphs": []}]} + + +def _bare_state(site_id="1", name="LAB1-IT-ACI"): + return SimpleNamespace(sites=[{"id": site_id, "name": name}]) + + +def _bare_op(site_key, device_dn): + return { + "op": "add", + "path": f"/sites/{site_key}/serviceGraphs/-", + "value": {"serviceNodes": [{"device": {"dn": device_dn}}]}, + } + + +def test_unit_empty_ops_is_noop(): + _validate_service_graph_device_refs( + [], {"1": _FakeApicState()}, _bare_state(), _bare_detail() + ) # must not raise + + +def test_unit_no_apic_states_is_noop(): + ops = [_bare_op("1-LAB1", DEVICE_DN)] + _validate_service_graph_device_refs(ops, None, _bare_state(), _bare_detail()) # must not raise + + +def test_unit_no_detail_is_noop(): + ops = [_bare_op("1-LAB1", DEVICE_DN)] + _validate_service_graph_device_refs(ops, {"1": _FakeApicState()}, _bare_state(), None) # must not raise + + +def test_unit_unresolvable_site_composite_is_noop(): + """The op's `{siteId}-{templateName}` composite doesn't match ANY + entry in detail["sites"] — can't prove anything, fail-safe skip.""" + ops = [_bare_op("9-DOES-NOT-EXIST", DEVICE_DN)] + apic_states = {"1": _FakeApicState()} + _validate_service_graph_device_refs(ops, apic_states, _bare_state(), _bare_detail()) # must not raise + + +def test_unit_device_present_passes(): + apic_states = {"1": _FakeApicState()} + apic_states["1"].store.upsert(MO("vnsLDevVip", dn=DEVICE_DN, name="fw-FW_LAB0")) + ops = [_bare_op("1-LAB1", DEVICE_DN)] + _validate_service_graph_device_refs(ops, apic_states, _bare_state(), _bare_detail()) # must not raise + + +def test_unit_device_absent_raises_golden_message(): + apic_states = {"1": _FakeApicState()} + ops = [_bare_op("1-LAB1", DEVICE_DN)] + with pytest.raises( + ServiceGraphValidationError, + match=r"Service graph device fw-FW_LAB0 does not exist in tenant MS-TN1 in Fabric LAB1-IT-ACI", + ): + _validate_service_graph_device_refs(ops, apic_states, _bare_state(), _bare_detail()) + + +def test_unit_replace_op_also_validated(): + """design §2.3: op in {add, replace} — a `replace` on an existing + serviceGraphs entry must be checked exactly like an `add`.""" + apic_states = {"1": _FakeApicState()} + op = _bare_op("1-LAB1", DEVICE_DN) + op["op"] = "replace" + op["path"] = "/sites/1-LAB1/serviceGraphs/0" + with pytest.raises(ServiceGraphValidationError): + _validate_service_graph_device_refs([op], apic_states, _bare_state(), _bare_detail()) + + +def test_unit_remove_op_never_validated(): + """A `remove` op carries no meaningful `value` to check — must never + be scanned (op not in {add, replace}).""" + apic_states = {"1": _FakeApicState()} + op = {"op": "remove", "path": "/sites/1-LAB1/serviceGraphs/0"} + _validate_service_graph_device_refs([op], apic_states, _bare_state(), _bare_detail()) # must not raise