diff --git a/README.md b/README.md index a9c3a7c..c25e670 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,7 @@ srxsync check --inventory inv.yaml [--verbose] [--transport {pyez,rustez}] | Flag | Meaning | |---|---| -| `--replace` / `--merge` | Mutually exclusive. `--replace` annotates each category root with `replace="replace"` so Junos wipes slave-specific siblings within synced subtrees. `--merge` only adds/updates. | +| `--replace` / `--merge` | Mutually exclusive. `--replace` annotates each category root with `replace="replace"` so Junos wipes slave-specific siblings within synced subtrees. `--merge` only adds/updates. Security zones are always merged, never replaced. | | `--commit-confirmed N` | Commit with an N-minute auto-rollback timer; srxsync calls `commit` again to seal. Default: 5. | | `--max-parallel N` | Concurrent target workers. Default: 5. | | `--on-error continue\|abort` | On a per-target failure, continue (exit non-zero at end) or cancel remaining work. Default: continue. | @@ -133,6 +133,42 @@ ntp: Then add `ntp` to the `include:` list of whichever targets should receive it. +## Zone translation + +A target can use different zone names than the master. Add a `zone_map` +(master zone → slave zone) to the target; unlisted zones keep their name. + +```yaml +targets: + - host: srx-site-a.example.net + auth: { provider: env } + include: [objects, policies, nat, zones] + zone_map: + APP1: APPS + APP2: APPS + APP3: APPS +``` + +- Zone names are rewritten in policies, global policies, NAT rule-sets and + address-book `attach`. +- Policy contexts that collapse into one (`APP1→UNTRUST`, `APP2→UNTRUST` → + `APPS→UNTRUST`) are concatenated in master order. A duplicate policy name in + a merged context fails that target. `APP1→APP2` becomes `APPS→APPS`. +- **Zones are never deleted.** `security zones` is always merged, even with + `--replace`. A zone fed by several master zones (`APPS`) is pushed as an + empty zone: created if missing, otherwise left exactly as the slave has it. +- srxsync warns (stderr, never fails) when it creates a zone, or a zone exists, + with no interfaces — bind one on the slave. +- Translation is single-pass: `{A: B, B: C}` maps A to B and B to C (no chaining). +- A warning is also printed when a translated policy references a zone that is + missing on the slave and `zones` is not in `include` (the slave commit will fail). +- A `zone_map` key that isn't a zone on the master prints a warning (likely a + typo) and is ignored. +- `check` only reports zones srxsync manages: zones that exist only on the slave + are not drift, and N:1 zones are checked for existence only. +- Entries in a master zone's *own* address-book are not carried to a merged + zone; keep shared objects in a global address-book. + ## Secrets Credentials never live in the inventory file. `auth.provider` selects a diff --git a/docs/superpowers/specs/2026-10-16-zone-translation-design.md b/docs/superpowers/specs/2026-10-16-zone-translation-design.md new file mode 100644 index 0000000..f6284c1 --- /dev/null +++ b/docs/superpowers/specs/2026-10-16-zone-translation-design.md @@ -0,0 +1,164 @@ +# Zone translation — Design Spec + +**Date:** 2026-10-16 +**Status:** Approved, ready for implementation planning +**Issue:** #11 (add zone translation) +**Branches:** `master` (rustperformance was merged into master in #7, so no separate port) + +## Purpose + +Master and slave SRXs do not always share a zone layout. The master may have +`APP1`, `APP2`, `APP3` and `UNTRUST` while a slave has only `APPS` and +`UNTRUST`. Each target gets a configurable master→slave zone mapping; srxsync +rewrites every zone reference in the synced config before pushing it. + +srxsync runs after every change on the master, so the translation must be +idempotent and must never delete anything on the slave that the operator +created there. + +## Inventory + +New optional per-target key `zone_map`, master zone → slave zone: + +```yaml +targets: + - host: srx-site-a.example.net + auth: { provider: env } + include: [objects, policies, nat, zones] + zone_map: + APP1: APPS + APP2: APPS + APP3: APPS +``` + +- Zones not listed keep their name (identity). `UNTRUST` above stays `UNTRUST`. +- `zone_map` must be a mapping of non-empty strings to non-empty strings; + anything else is an `InventoryError` at load time. +- Translation is single-pass, never chained: with `{A: B, B: C}`, master `A` + becomes `B` and master `B` becomes `C`. +- A key that does not exist as a zone on the master is logged as a warning, not + an error. +- `Target` gains `zone_map: dict[str, str]` (empty when absent). No `zone_map` + means behaviour is byte-for-byte unchanged. + +### Terms + +For a target, the **preimage** of a slave zone is the set of master zones that +translate to it (mapped keys plus an identically named, unmapped master zone). + +- **1:1 zone** — preimage has exactly one master zone (identity or rename). +- **N:1 zone** — preimage has two or more master zones (e.g. `APPS`). + +## Translation — `srxsync/zonemap.py` + +`ZoneTranslator(zone_map).apply(config) -> etree._Element` returns a translated +deep copy of the fetched master config; the input is never mutated. The +orchestrator calls it once per target, before `DiffBuilder`. Errors raise +`ZoneTranslationError`, which fails only that target. + +Zone references rewritten: + +| Category | Element | Rule | +|---|---|---| +| `policies` | `policy` context `from-zone-name` / `to-zone-name` | Rename, then merge contexts (below) | +| `policies` | `global/policy/match/from-zone`, `to-zone` | Rename each entry, de-duplicate, keep first-seen order | +| `nat` | `source`/`destination`/`static` `rule-set/from/zone`, `rule-set/to/zone` | Rename, de-duplicate | +| `objects` | `address-book/attach/zone` | Rename, de-duplicate | +| `zones` | `security-zone/name` | 1:1 renamed and kept; N:1 handled below | + +### Merging policy contexts + +After renaming, two or more contexts can share a `(from, to)` pair — e.g. +`APP1→UNTRUST`, `APP2→UNTRUST`, `APP3→UNTRUST` all become `APPS→UNTRUST`. + +- Contexts are merged into one, its policies concatenated in master document + order: all of the first context's policies (internal order kept), then the + second's, and so on. Rule order on the slave therefore matches master order. +- A duplicate policy name within a merged context raises + `ZoneTranslationError` naming the policy and both source contexts. No + auto-renaming. +- Contexts between zones that merge (`APP1→APP2`) become intra-zone + (`APPS→APPS`) and are kept, merged by the same rule. + +## Zones are additive only + +srxsync never deletes a zone on a slave. + +- The `zones` category is always loaded as merge. `DiffBuilder._mark_replace` + never sets `replace="replace"` on `security/zones`, even under `--replace`. + Other categories keep their `--replace` behaviour. This also stops + `--replace` from wiping zone interface bindings, a side effect of the + interface prune that exists today. +- **1:1 zones** — the translated zone (interfaces pruned as today) is merged + onto the slave. A zone the slave lacks is created. +- **N:1 zones** — only an empty shell, `APPS`, + is pushed. The master zones' settings (host-inbound-traffic, screen, + zone address-book) are not carried over. In merge mode the shell creates the + zone if missing and is a no-op if present, so the slave's own settings for + `APPS` are never touched. + +Verified 2026-10-16 with `commit check` on lab `vsrx-ci`: zones with no +interfaces, plus policies and source-NAT rule-sets referencing them, commit +cleanly. An empty zone does not block a push; it only carries no traffic. + +## Empty-zone warning + +Before loading, the push fetches the target's `/configuration/security/zones` +(one extra RPC). For every zone the translated payload references on the slave: + +- missing on the slave → `WARNING : creating zone APPS with no interfaces — bind an interface on the slave (set security zones security-zone APPS interfaces )` +- present with no `interfaces` → same message, "zone APPS exists with no interfaces". + +Warnings never fail a target or change the exit code. They are emitted for +`push`, `push --dry-run` and `check` (check already fetches target zones when +`zones` is included; otherwise it fetches them for this purpose). + +## Results and CLI + +- `TargetResult` and `DriftLine` gain `warnings: list[str]` (default empty). +- The CLI prints each warning on its own line after the summary table, prefixed + with `WARNING`, to stderr. +- `ZoneTranslationError` surfaces as a normal per-target failure (`FAIL` row, + `--on-error` honoured). + +## Drift detection + +`check` translates the source with the same `ZoneTranslator` before comparing. + +- Policies, NAT and address-book compare the translated source against the target. +- 1:1 zones compare content (interfaces pruned, as today). +- N:1 zones are dropped from the content comparison on both sides; drift is + reported only when the zone is missing on the slave. + +## Known limitation + +Address entries defined in a master zone's own address-book (`security-zone +APP1 address-book`) are not carried to an N:1 slave zone. A pushed policy that +references one fails the slave's commit, and commit-confirmed rolls it back. +The fix is to put shared objects in a global address-book. + +## Testing + +Unit tests with XML fixtures (no devices): + +- `zone_map` inventory validation: absent, valid, wrong types, empty strings +- identity when `zone_map` is empty — payload unchanged +- 1:1 rename across policies, global policies, NAT, address-book attach, zones +- N:1 policy contexts concatenated in master order +- duplicate policy name in a merged context → `ZoneTranslationError` +- `APP1→APP2` becomes `APPS→APPS` +- de-duplication of global-policy, NAT and attach zone lists +- N:1 zone pushed as an empty shell; master zone settings absent +- `security/zones` never carries `replace` under `--replace`; other roots do +- warning generated for missing zone and for an existing zone with no interfaces +- drift: translated source in sync with a correctly translated target; N:1 zone + existence-only + +Integration (lab, optional): push a mapped inventory to a lab vSRX and confirm +the commit and the warning. + +## Out of scope + +- Merging or templating N:1 zone settings +- Translating names other than zones (routing instances, interfaces) +- Deleting slave zones that no longer have a master counterpart diff --git a/srxsync/cli.py b/srxsync/cli.py index e34a2cc..4f8e1c1 100644 --- a/srxsync/cli.py +++ b/srxsync/cli.py @@ -5,6 +5,7 @@ import argparse import asyncio import sys +from collections.abc import Iterable from pathlib import Path from srxsync.categories import CategoryModel @@ -99,6 +100,7 @@ def _print_push_summary(summary: PushSummary) -> None: status = "OK" if r.ok else "FAIL" err = r.error or "" print(f"{r.host:40s} {status:10s} {r.duration_s:>8.2f}s {err}") + _print_warnings(warning for result in summary.results for warning in result.warnings) def _print_drift_summary(summary: DriftSummary, *, verbose: bool) -> None: @@ -116,6 +118,13 @@ def _print_drift_summary(summary: DriftSummary, *, verbose: bool) -> None: if verbose: for p in line.differing_paths: print(f" - {p}") + _print_warnings(warning for line in summary.reports for warning in line.warnings) + + +def _print_warnings(warnings: Iterable[str]) -> None: + """Print each warning to stderr prefixed with ``WARNING ``.""" + for warning in warnings: + print(f"WARNING {warning}", file=sys.stderr) if __name__ == "__main__": diff --git a/srxsync/diff.py b/srxsync/diff.py index c5cd267..1ed71a5 100644 --- a/srxsync/diff.py +++ b/srxsync/diff.py @@ -7,6 +7,8 @@ from lxml import etree +from srxsync.zonemap import ZONES_PATH + @dataclass(frozen=True) class DiffBuilder: @@ -38,8 +40,13 @@ def _mark_replace(self, out: etree._Element) -> None: the element at that path in the payload and set replace="replace". Junos then replaces that subtree wholesale on the target device — the semantic users expect from --replace. + + security/zones is always merged: srxsync never deletes a slave zone, and + replacing it would also wipe the (pruned) interface bindings. """ for abs_path in self.paths: + if abs_path == ZONES_PATH: + continue rel = abs_path.removeprefix("/configuration/") for node in out.xpath(rel): node.set("replace", "replace") diff --git a/srxsync/drift.py b/srxsync/drift.py index 360e5f0..0bc1c46 100644 --- a/srxsync/drift.py +++ b/srxsync/drift.py @@ -7,6 +7,7 @@ from lxml import etree from srxsync.diff import DiffBuilder +from srxsync.zonemap import ZONES_PATH @dataclass(frozen=True) @@ -23,11 +24,17 @@ def in_sync(self) -> bool: class DriftDetector: paths: list[str] prune: list[str] + merged_zones: frozenset[str] = frozenset() def diff(self, source: etree._Element, target: etree._Element, host: str = "") -> DriftReport: builder = DiffBuilder(paths=self.paths, prune=self.prune) src_scoped = builder.build(source) tgt_scoped = builder.build(target) + missing_merged = self._missing_merged_zones(tgt_scoped) + if ZONES_PATH in self.paths: + self._drop_slave_only_zones(src_scoped, tgt_scoped) + self._drop_merged_zones(src_scoped) + self._drop_merged_zones(tgt_scoped) differing: list[str] = [] for abs_path in self.paths: @@ -40,10 +47,39 @@ def diff(self, source: etree._Element, target: etree._Element, host: str = "") - # not be sorted: a reordered-but-identical-set list is drift. src_nodes = [_canonicalize(n) for n in src_scoped.findall(rel)] tgt_nodes = [_canonicalize(n) for n in tgt_scoped.findall(rel)] - if src_nodes != tgt_nodes: + zone_missing = abs_path == ZONES_PATH and bool(missing_merged) + if zone_missing or src_nodes != tgt_nodes: differing.append(abs_path) return DriftReport(host=host, differing_paths=differing) + def _missing_merged_zones(self, target_scoped: etree._Element) -> set[str]: + """N:1 zones are slave-owned: only their existence on the target is checked.""" + present = { + name.text for name in target_scoped.xpath("security/zones/security-zone/name") + } + return set(self.merged_zones) - present + + def _drop_slave_only_zones( + self, source_scoped: etree._Element, target_scoped: etree._Element + ) -> None: + """Ignore zones only the target has: push never deletes zones, so they are unfixable.""" + source_names = { + name.text for name in source_scoped.xpath("security/zones/security-zone/name") + } + for zone in target_scoped.xpath("security/zones/security-zone"): + if zone.findtext("name") not in source_names: + zone.getparent().remove(zone) + if source_scoped.xpath("security/zones"): + return + for zones in target_scoped.xpath("security/zones[not(*)]"): + zones.getparent().remove(zones) + + def _drop_merged_zones(self, scoped: etree._Element) -> None: + """Remove merged zones' security-zone elements so their content is not compared.""" + for zone in scoped.xpath("security/zones/security-zone"): + if zone.findtext("name") in self.merged_zones: + zone.getparent().remove(zone) + def _canonicalize(node: etree._Element) -> bytes: result = etree.tostring(node, method="c14n2") diff --git a/srxsync/inventory.py b/srxsync/inventory.py index 2094a91..98416ed 100644 --- a/srxsync/inventory.py +++ b/srxsync/inventory.py @@ -2,7 +2,7 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -31,6 +31,7 @@ class Target: host: str auth: Auth include: list[str] + zone_map: dict[str, str] = field(default_factory=dict) @dataclass(frozen=True) @@ -90,4 +91,26 @@ def _parse_target(raw: dict[str, Any], *, known_categories: set[str]) -> Target: host=raw["host"], auth=_parse_auth(raw["auth"]), include=include, + zone_map=_parse_zone_map(raw["host"], raw.get("zone_map")), ) + + +def _parse_zone_map(host: str, raw: Any) -> dict[str, str]: + """Validate a target's master→slave zone_map; None or absent means no translation.""" + if raw is None: + return {} + if not isinstance(raw, dict): + raise InventoryError(f"target {host} zone_map must be a mapping") + for master_zone, slave_zone in raw.items(): + valid = ( + isinstance(master_zone, str) + and isinstance(slave_zone, str) + and master_zone + and slave_zone + ) + if not valid: + raise InventoryError( + f"target {host} zone_map entries must be non-empty strings: " + f"{master_zone!r}: {slave_zone!r}" + ) + return dict(raw) diff --git a/srxsync/orchestrator.py b/srxsync/orchestrator.py index c091fe3..5be815e 100644 --- a/srxsync/orchestrator.py +++ b/srxsync/orchestrator.py @@ -17,6 +17,8 @@ from srxsync.results import DriftLine, DriftSummary, PushSummary, TargetResult from srxsync.secrets import get_secret from srxsync.transport import PyEZTransport, Transport +from srxsync.zonemap import ZONES_PATH, ZoneTranslator +from srxsync.zonewarn import zone_warnings @dataclass(frozen=True) @@ -112,15 +114,20 @@ def _push_target( t = self._tx() try: paths, prune = self._cats.resolve(target.include) + translator = ZoneTranslator(target.zone_map) + translated = translator.apply(source_xml) secret = get_secret(host=target.host, auth=target.auth) t.connect(target.host, secret.username, secret.password, ssh_key=secret.ssh_key_path) - payload = DiffBuilder(paths=paths, prune=list(prune)).build(source_xml, mode=cfg.mode) + payload = DiffBuilder(paths=paths, prune=list(prune)).build(translated, mode=cfg.mode) + warnings = _unknown_key_warnings(translator, source_xml, target.host) + warnings += zone_warnings(payload, t.fetch([ZONES_PATH]), host=target.host) if cfg.dry_run: return TargetResult( host=target.host, ok=True, duration_s=time.monotonic() - start, + warnings=warnings, ) t.load(payload, mode=cfg.mode) @@ -130,6 +137,7 @@ def _push_target( host=target.host, ok=True, duration_s=time.monotonic() - start, + warnings=warnings, ) except Exception as e: # Deliberately broad: lock, timeout, and secret/Vault failures are @@ -153,15 +161,26 @@ def _check_target(self, target: Target, source_xml: etree._Element) -> DriftLine t = self._tx() try: paths, prune = self._cats.resolve(target.include) + translator = ZoneTranslator(target.zone_map) + translated = translator.apply(source_xml) secret = get_secret(host=target.host, auth=target.auth) t.connect(target.host, secret.username, secret.password, ssh_key=secret.ssh_key_path) target_xml = t.fetch(paths) - detector = DriftDetector(paths=paths, prune=list(prune)) - rep: DriftReport = detector.diff(source_xml, target_xml, host=target.host) + zone_view = target_xml if ZONES_PATH in paths else t.fetch([ZONES_PATH]) + detector = DriftDetector( + paths=paths, + prune=list(prune), + merged_zones=translator.merged_zones(source_xml), + ) + rep: DriftReport = detector.diff(translated, target_xml, host=target.host) + payload = DiffBuilder(paths=paths, prune=list(prune)).build(translated) + warnings = _unknown_key_warnings(translator, source_xml, target.host) + warnings += zone_warnings(payload, zone_view, host=target.host) return DriftLine( host=target.host, in_sync=rep.in_sync, differing_paths=list(rep.differing_paths), + warnings=warnings, ) except Exception as e: # See _push_target: lock, timeout, and secret/Vault failures are @@ -170,3 +189,13 @@ def _check_target(self, target: Target, source_xml: etree._Element) -> DriftLine finally: with contextlib.suppress(Exception): t.close() + + +def _unknown_key_warnings( + translator: ZoneTranslator, source_xml: etree._Element, host: str +) -> list[str]: + """Warn about zone_map keys that name no zone on the master (likely typos).""" + return [ + f"{host}: zone_map key {key} is not a zone on the master" + for key in translator.unknown_keys(source_xml) + ] diff --git a/srxsync/results.py b/srxsync/results.py index 4239af1..a6592ff 100644 --- a/srxsync/results.py +++ b/srxsync/results.py @@ -11,6 +11,7 @@ class TargetResult: ok: bool error: str | None = None duration_s: float = 0.0 + warnings: list[str] = field(default_factory=list) @dataclass(frozen=True) @@ -28,6 +29,7 @@ class DriftLine: in_sync: bool differing_paths: list[str] = field(default_factory=list) error: str | None = None + warnings: list[str] = field(default_factory=list) @dataclass(frozen=True) diff --git a/srxsync/zonemap.py b/srxsync/zonemap.py new file mode 100644 index 0000000..765e741 --- /dev/null +++ b/srxsync/zonemap.py @@ -0,0 +1,152 @@ +"""Per-target zone-name translation of a fetched master config.""" + +from __future__ import annotations + +import copy +from dataclasses import dataclass + +from lxml import etree + +ZONES_PATH = "/configuration/security/zones" + +# Leaf-list zone references: (XPath of the parent, child tag holding a zone name). +_ZONE_LISTS = ( + ("security/policies/global/policy/match", "from-zone"), + ("security/policies/global/policy/match", "to-zone"), + ("security/nat/*/rule-set/from", "zone"), + ("security/nat/*/rule-set/to", "zone"), +) + + +class ZoneTranslationError(ValueError): + """Raised when a translated config cannot be built for a target.""" + + +@dataclass(frozen=True) +class ZoneTranslator: + """Rewrites master zone names to a target's zone names (master → slave).""" + + zone_map: dict[str, str] + + def translate(self, zone: str) -> str: + """Return the slave name for a master zone; unmapped zones keep their name.""" + return self.zone_map.get(zone, zone) + + def apply(self, config: etree._Element) -> etree._Element: + """Return a translated deep copy of ``config``; the input is never mutated.""" + translated = copy.deepcopy(config) + if not self.zone_map: + return translated + self._rename_zone_lists(translated) + self._rename_attach_zones(translated) + self._rename_policy_contexts(translated) + self._rename_zone_definitions(translated) + return translated + + def _rename_zone_lists(self, config: etree._Element) -> None: + for parent_xpath, tag in _ZONE_LISTS: + for parent in config.xpath(parent_xpath): + seen: set[str] = set() + for entry in parent.findall(tag): + entry.text = self.translate(entry.text or "") + if entry.text in seen: + parent.remove(entry) + continue + seen.add(entry.text) + + def _rename_attach_zones(self, config: etree._Element) -> None: + for attach in config.xpath("security/address-book/attach"): + seen: set[str] = set() + for zone in attach.findall("zone"): + name = zone.find("name") + if name is None: + continue + name.text = self.translate(name.text or "") + if name.text in seen: + attach.remove(zone) + continue + seen.add(name.text) + + def merged_zones(self, config: etree._Element) -> frozenset[str]: + """Slave zones fed by two or more master zones (N:1); their settings stay slave-owned.""" + if not config.xpath("security/zones"): + return frozenset() + master_zones = set(_master_zone_names(config)) + preimage_sizes: dict[str, int] = {} + for master_zone in self.zone_map: + if master_zone not in master_zones: + continue + slave_zone = self.zone_map[master_zone] + preimage_sizes[slave_zone] = preimage_sizes.get(slave_zone, 0) + 1 + for master_zone in master_zones: + if master_zone in self.zone_map: + continue + if master_zone in preimage_sizes: + preimage_sizes[master_zone] += 1 + return frozenset(zone for zone, size in preimage_sizes.items() if size >= 2) + + def unknown_keys(self, config: etree._Element) -> list[str]: + """zone_map keys that are not zones on the master; empty if zones were not fetched.""" + if not config.xpath("security/zones"): + return [] + master_zones = set(_master_zone_names(config)) + return [key for key in self.zone_map if key not in master_zones] + + def _rename_policy_contexts(self, config: etree._Element) -> None: + """Rename context zones and merge contexts that collapse onto the same pair.""" + for policies in config.xpath("security/policies"): + keepers: dict[tuple[str, str], etree._Element] = {} + # rule name -> "FROM->TO" of the master context it came from, per kept pair + rule_origins: dict[tuple[str, str], dict[str, str]] = {} + for context in policies.findall("policy"): + from_node = context.find("from-zone-name") + to_node = context.find("to-zone-name") + if from_node is None or to_node is None: + continue + origin = f"{from_node.text}->{to_node.text}" + from_node.text = self.translate(from_node.text or "") + to_node.text = self.translate(to_node.text or "") + pair = (from_node.text, to_node.text) + keeper = keepers.get(pair) + if keeper is None: + keepers[pair] = context + rule_origins[pair] = { + rule.findtext("name") or "": origin for rule in context.findall("policy") + } + continue + origins = rule_origins[pair] + for rule in context.findall("policy"): + rule_name = rule.findtext("name") or "" + if rule_name in origins: + raise ZoneTranslationError( + f"duplicate policy {rule_name} in merged context " + f"{pair[0]}->{pair[1]}: from {origins[rule_name]} and {origin}" + ) + origins[rule_name] = origin + keeper.append(rule) + policies.remove(context) + + def _rename_zone_definitions(self, config: etree._Element) -> None: + """Rename 1:1 zones; reduce N:1 zones to one empty shell (settings stay slave-owned).""" + merged = self.merged_zones(config) + for zones in config.xpath("security/zones"): + shells_written: set[str] = set() + for zone in zones.findall("security-zone"): + name = zone.find("name") + if name is None: + continue + slave_name = self.translate(name.text or "") + if slave_name not in merged: + name.text = slave_name + continue + if slave_name in shells_written: + zones.remove(zone) + continue + shell = etree.Element("security-zone") + etree.SubElement(shell, "name").text = slave_name + zones.replace(zone, shell) + shells_written.add(slave_name) + + +def _master_zone_names(config: etree._Element) -> list[str]: + return [name.text or "" for name in config.xpath("security/zones/security-zone/name")] diff --git a/srxsync/zonewarn.py b/srxsync/zonewarn.py new file mode 100644 index 0000000..3865130 --- /dev/null +++ b/srxsync/zonewarn.py @@ -0,0 +1,67 @@ +"""Warn about slave zones that srxsync creates or references but that carry no interfaces.""" + +from __future__ import annotations + +from lxml import etree + +_SPECIAL_ZONES = frozenset({"junos-host", "any"}) + +_REFERENCE_XPATHS = ( + "security/policies/policy/from-zone-name", + "security/policies/policy/to-zone-name", + "security/policies/global/policy/match/from-zone", + "security/policies/global/policy/match/to-zone", + "security/nat/*/rule-set/from/zone", + "security/nat/*/rule-set/to/zone", + "security/address-book/attach/zone/name", +) + + +def zone_warnings( + payload: etree._Element, target_config: etree._Element, host: str +) -> list[str]: + """Return warnings for payload zones that are new, empty, or missing on the target.""" + defined_in_payload = set(_zone_names(payload)) + referenced = set(defined_in_payload) + for xpath in _REFERENCE_XPATHS: + referenced.update(node.text or "" for node in payload.xpath(xpath)) + referenced -= _SPECIAL_ZONES + referenced.discard("") + + target_has_interfaces = { + zone.findtext("name") or "": zone.find("interfaces") is not None + for zone in target_config.xpath("security/zones/security-zone") + } + + warnings: list[str] = [] + for zone in sorted(referenced): + warning = _warning_for(zone, host, target_has_interfaces, defined_in_payload) + if warning is not None: + warnings.append(warning) + return warnings + + +def _warning_for( + zone: str, + host: str, + target_has_interfaces: dict[str, bool], + defined_in_payload: set[str], +) -> str | None: + hint = ( + "bind an interface on the slave " + f"(set security zones security-zone {zone} interfaces )" + ) + if zone in target_has_interfaces: + if target_has_interfaces[zone]: + return None + return f"{host}: zone {zone} exists with no interfaces — {hint}" + if zone in defined_in_payload: + return f"{host}: creating zone {zone} with no interfaces — {hint}" + return ( + f'{host}: zone {zone} is referenced but missing on the slave — add "zones" to include ' + "or create it" + ) + + +def _zone_names(config: etree._Element) -> list[str]: + return [name.text or "" for name in config.xpath("security/zones/security-zone/name")] diff --git a/tests/fixtures/configs/zones_master.xml b/tests/fixtures/configs/zones_master.xml new file mode 100644 index 0000000..5e86100 --- /dev/null +++ b/tests/fixtures/configs/zones_master.xml @@ -0,0 +1,80 @@ + + + + global +
WEB10.1.0.10/32
+ + APP1 + APP2 + + + + + APP1 + ping + ge-0/0/1.0 + + + APP2 + app2-screen + ge-0/0/2.0 + + + APP3 + ge-0/0/3.0 + + + UNTRUST + untrust-screen + ge-0/0/0.0 + + + + + APP1 + UNTRUST + app1-webanyanyjunos-http + + + APP1 + APP2 + app1-to-app2anyanyany + + + APP2 + UNTRUST + app2-webanyanyjunos-http + app2-dnsanyanyjunos-dns-udp + + + APP3 + UNTRUST + app3-sshanyanyjunos-ssh + + + + global-deny-telnet + anyanyjunos-telnetAPP1APP2UNTRUST + + + + + + + + apps-out + APP1APP2APP3 + UNTRUST + r10.0.0.0/0 + + + + + inbound + UNTRUST + d1203.0.113.10/32 + + + + + diff --git a/tests/fixtures/inventory/zone_map.yaml b/tests/fixtures/inventory/zone_map.yaml new file mode 100644 index 0000000..b2bfb5b --- /dev/null +++ b/tests/fixtures/inventory/zone_map.yaml @@ -0,0 +1,14 @@ +source: + host: srx-master.example.net + auth: { provider: env } +targets: + - host: srx-a.example.net + auth: { provider: env } + include: [policies, zones] + zone_map: + APP1: APPS + APP2: APPS + APP3: APPS + - host: srx-b.example.net + auth: { provider: env } + include: [policies] diff --git a/tests/fixtures/inventory/zone_map_bad.yaml b/tests/fixtures/inventory/zone_map_bad.yaml new file mode 100644 index 0000000..622ac2e --- /dev/null +++ b/tests/fixtures/inventory/zone_map_bad.yaml @@ -0,0 +1,8 @@ +source: + host: srx-master.example.net + auth: { provider: env } +targets: + - host: srx-a.example.net + auth: { provider: env } + include: [policies] + zone_map: [APP1, APPS] diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 0806e33..4897c87 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -4,7 +4,8 @@ import pytest -from srxsync.cli import build_parser +from srxsync.cli import _print_drift_summary, _print_push_summary, build_parser +from srxsync.results import DriftLine, DriftSummary, PushSummary, TargetResult def test_push_requires_mode(): @@ -97,3 +98,21 @@ def test_transport_rejects_unknown_value() -> None: parser = build_parser() with pytest.raises(SystemExit): parser.parse_args(["push", "--inventory", "inv.yaml", "--merge", "--transport", "grpc"]) + + +def test_push_summary_prints_warnings_to_stderr(capsys): + summary = PushSummary( + results=[TargetResult(host="srx-a", ok=True, warnings=["srx-a: creating zone APPS"])] + ) + _print_push_summary(summary) + captured = capsys.readouterr() + assert "WARNING srx-a: creating zone APPS" in captured.err + assert "WARNING" not in captured.out + + +def test_drift_summary_prints_warnings_to_stderr(capsys): + summary = DriftSummary( + reports=[DriftLine(host="srx-a", in_sync=True, warnings=["srx-a: zone APPS exists"])] + ) + _print_drift_summary(summary, verbose=False) + assert "WARNING srx-a: zone APPS exists" in capsys.readouterr().err diff --git a/tests/unit/test_diff.py b/tests/unit/test_diff.py index aefb4a6..16a9d22 100644 --- a/tests/unit/test_diff.py +++ b/tests/unit/test_diff.py @@ -131,3 +131,14 @@ def test_list_category_keeps_every_sibling_entry(): assert len(servers) == 2 names = {s.findtext("name") for s in servers} assert names == {"8.8.8.8", "1.1.1.1"} + + +def test_replace_mode_never_marks_zones(): + src = _load() + builder = DiffBuilder( + paths=["/configuration/security/policies", "/configuration/security/zones"], + prune=["security-zone/interfaces"], + ) + out = builder.build(src, mode="replace") + assert out.find(".//policies").get("replace") == "replace" + assert out.find(".//zones").get("replace") is None diff --git a/tests/unit/test_drift.py b/tests/unit/test_drift.py index de0989f..5eca6fd 100644 --- a/tests/unit/test_drift.py +++ b/tests/unit/test_drift.py @@ -98,3 +98,58 @@ def test_detects_system_category_drift_granularity(): assert "/configuration/system/name-server" in rep.differing_paths assert "/configuration/system/syslog" not in rep.differing_paths assert "/configuration/system/domain-name" not in rep.differing_paths + + +def _zones(*zones: str) -> etree._Element: + body = "".join(zones) + return etree.fromstring( + f"{body}" + ) + + +ZONES = "/configuration/security/zones" + + +def test_merged_zone_content_ignored(): + src = _zones("APPS") + tgt = _zones("APPSlocal") + det = DriftDetector(paths=[ZONES], prune=[], merged_zones=frozenset({"APPS"})) + assert det.diff(src, tgt).in_sync is True + + +def test_merged_zone_missing_on_target_is_drift(): + src = _zones("APPS") + tgt = _zones() + det = DriftDetector(paths=[ZONES], prune=[], merged_zones=frozenset({"APPS"})) + assert det.diff(src, tgt).differing_paths == [ZONES] + + +def test_non_merged_zone_content_still_compared(): + src = _zones("UNTRUSTa") + tgt = _zones("UNTRUSTb") + det = DriftDetector(paths=[ZONES], prune=[], merged_zones=frozenset({"APPS"})) + assert det.diff(src, tgt).differing_paths == [ZONES] + + +def test_slave_only_zone_is_not_drift(): + src = _zones("UNTRUST") + tgt = _zones( + "UNTRUST" + "MGMT" + ) + det = DriftDetector(paths=[ZONES], prune=[]) + assert det.diff(src, tgt).in_sync is True + + +def test_slave_only_zone_ignored_but_shared_zone_drift_counts(): + src = _zones( + "APPS" + "UNTRUSTa" + ) + tgt = _zones( + "APPSlocal" + "UNTRUSTb" + "MGMT" + ) + det = DriftDetector(paths=[ZONES], prune=[], merged_zones=frozenset({"APPS"})) + assert det.diff(src, tgt).differing_paths == [ZONES] diff --git a/tests/unit/test_inventory.py b/tests/unit/test_inventory.py index e9a768c..af0d2f9 100644 --- a/tests/unit/test_inventory.py +++ b/tests/unit/test_inventory.py @@ -54,3 +54,34 @@ def test_unknown_category_raises(): def test_missing_file_raises(): with pytest.raises(InventoryError, match="not found"): load_inventory(FIXTURES / "does_not_exist.yaml", known_categories=KNOWN_CATEGORIES) + + +def test_zone_map_parsed(): + inv = load_inventory(FIXTURES / "zone_map.yaml", known_categories=KNOWN_CATEGORIES) + assert inv.targets[0].zone_map == {"APP1": "APPS", "APP2": "APPS", "APP3": "APPS"} + + +def test_zone_map_defaults_empty(): + inv = load_inventory(FIXTURES / "zone_map.yaml", known_categories=KNOWN_CATEGORIES) + assert inv.targets[1].zone_map == {} + + +def test_zone_map_must_be_mapping(): + with pytest.raises(InventoryError, match="zone_map must be a mapping"): + load_inventory(FIXTURES / "zone_map_bad.yaml", known_categories=KNOWN_CATEGORIES) + + +@pytest.mark.parametrize("bad_map", [{"APP1": ""}, {"": "APPS"}, {"APP1": 5}, {"APP1": None}]) +def test_zone_map_entries_must_be_non_empty_strings(tmp_path, bad_map): + import yaml + + doc = { + "source": {"host": "m", "auth": {"provider": "env"}}, + "targets": [ + {"host": "t", "auth": {"provider": "env"}, "include": ["policies"], "zone_map": bad_map} + ], + } + inv_file = tmp_path / "inv.yaml" + inv_file.write_text(yaml.safe_dump(doc)) + with pytest.raises(InventoryError, match="zone_map entries must be non-empty strings"): + load_inventory(inv_file, known_categories=KNOWN_CATEGORIES) diff --git a/tests/unit/test_zone_orchestrator.py b/tests/unit/test_zone_orchestrator.py new file mode 100644 index 0000000..dd2ad30 --- /dev/null +++ b/tests/unit/test_zone_orchestrator.py @@ -0,0 +1,117 @@ +import asyncio +from pathlib import Path +from typing import ClassVar, Literal + +import pytest +from lxml import etree + +from srxsync import orchestrator as orchestrator_module +from srxsync.categories import CategoryModel +from srxsync.diff import DiffBuilder +from srxsync.inventory import Auth, Device, Inventory, Target +from srxsync.orchestrator import Orchestrator, RunConfig +from srxsync.secrets.base import Secret +from srxsync.transport.base import Transport + +FX = Path(__file__).parent.parent / "fixtures" / "configs" +APPS_MAP = {"APP1": "APPS", "APP2": "APPS", "APP3": "APPS"} +SLAVE = ( + "" + "UNTRUSTge-0/0/0.0" + "" +) + + +class FakeTransport(Transport): + configs: ClassVar[dict[str, etree._Element]] = {} + loaded: ClassVar[dict[str, etree._Element]] = {} + + def connect( + self, + host: str, + username: str, + password: str | None = None, + ssh_key: str | None = None, + port: int = 22, + ) -> None: + self._host = host + + def fetch(self, paths: list[str]) -> etree._Element: + return DiffBuilder(paths=paths, prune=[]).build(self.configs[self._host]) + + def load(self, xml: etree._Element, mode: Literal["replace", "merge"]) -> None: + self.loaded[self._host] = xml + + def commit_confirmed(self, minutes: int) -> None: + pass + + def confirm(self) -> None: + pass + + def rollback(self) -> None: + pass + + def close(self) -> None: + pass + + +@pytest.fixture(autouse=True) +def _fake_world(monkeypatch): + FakeTransport.configs = { + "master": etree.parse(str(FX / "zones_master.xml")).getroot(), + "slave": etree.fromstring(SLAVE), + } + FakeTransport.loaded = {} + monkeypatch.setattr(orchestrator_module, "get_secret", lambda host, auth: Secret(username="u")) + + +def _orchestrator(zone_map: dict[str, str], include: list[str]) -> Orchestrator: + auth = Auth(provider="env") + inventory = Inventory( + source=Device(host="master", auth=auth), + targets=[Target(host="slave", auth=auth, include=include, zone_map=zone_map)], + ) + return Orchestrator(inventory, CategoryModel.default(), transport_factory=FakeTransport) + + +def _run_config(mode: Literal["replace", "merge"] = "replace") -> RunConfig: + return RunConfig(mode=mode, commit_confirmed_minutes=1, max_parallel=1, on_error="continue") + + +def test_push_translates_and_warns(): + summary = asyncio.run(_orchestrator(APPS_MAP, ["policies", "zones"]).push(_run_config())) + result = summary.results[0] + assert result.ok, result.error + payload = FakeTransport.loaded["slave"] + contexts = { + (ctx.findtext("from-zone-name"), ctx.findtext("to-zone-name")) + for ctx in payload.xpath("security/policies/policy") + } + assert contexts == {("APPS", "UNTRUST"), ("APPS", "APPS")} + assert payload.find("security/zones").get("replace") is None + assert any("creating zone APPS with no interfaces" in w for w in result.warnings) + + +def test_push_translation_error_fails_only_that_target(): + master = FakeTransport.configs["master"] + master.xpath("security/policies/policy[from-zone-name='APP3']/policy/name")[0].text = "app1-web" + summary = asyncio.run(_orchestrator(APPS_MAP, ["policies"]).push(_run_config())) + assert summary.results[0].ok is False + assert "duplicate policy app1-web" in (summary.results[0].error or "") + assert "slave" not in FakeTransport.loaded + + +def test_push_without_zone_map_payload_unchanged(): + asyncio.run(_orchestrator({}, ["policies"]).push(_run_config("merge"))) + expected = DiffBuilder(paths=["/configuration/security/policies"], prune=[]).build( + FakeTransport.configs["master"] + ) + assert etree.tostring(FakeTransport.loaded["slave"]) == etree.tostring(expected) + + +def test_check_reports_missing_merged_zone_and_warns_unknown_key(): + zone_map = {**APPS_MAP, "NOPE": "APPS"} + summary = asyncio.run(_orchestrator(zone_map, ["zones"]).check(max_parallel=1)) + line = summary.reports[0] + assert "/configuration/security/zones" in line.differing_paths + assert "slave: zone_map key NOPE is not a zone on the master" in line.warnings diff --git a/tests/unit/test_zonemap.py b/tests/unit/test_zonemap.py new file mode 100644 index 0000000..0e910bd --- /dev/null +++ b/tests/unit/test_zonemap.py @@ -0,0 +1,164 @@ +from pathlib import Path + +import pytest +from lxml import etree + +from srxsync.zonemap import ZoneTranslationError, ZoneTranslator + +FIXTURE = Path(__file__).parent.parent / "fixtures" / "configs" / "zones_master.xml" +APPS_MAP = {"APP1": "APPS", "APP2": "APPS", "APP3": "APPS"} + + +def _load() -> etree._Element: + return etree.parse(str(FIXTURE)).getroot() + + +def _texts(config: etree._Element, xpath: str) -> list[str]: + return [node.text for node in config.xpath(xpath)] + + +def test_empty_map_is_identity(): + source = _load() + translated = ZoneTranslator({}).apply(source) + assert etree.tostring(translated) == etree.tostring(source) + + +def test_apply_does_not_mutate_input(): + source = _load() + before = etree.tostring(source) + ZoneTranslator(APPS_MAP).apply(source) + assert etree.tostring(source) == before + + +def test_translate_unmapped_is_identity(): + translator = ZoneTranslator(APPS_MAP) + assert translator.translate("APP1") == "APPS" + assert translator.translate("UNTRUST") == "UNTRUST" + + +def test_single_pass_no_chaining(): + translator = ZoneTranslator({"A": "B", "B": "C"}) + assert translator.translate("A") == "B" + assert translator.translate("B") == "C" + + +def test_one_to_one_rename_everywhere(): + translated = ZoneTranslator({"UNTRUST": "OUTSIDE"}).apply(_load()) + assert "UNTRUST" not in etree.tostring(translated).decode() + assert _texts(translated, "security/nat/destination/rule-set/from/zone") == ["OUTSIDE"] + assert "OUTSIDE" in _texts(translated, "security/zones/security-zone/name") + + +def test_global_policy_zones_renamed_and_deduped(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + match = "security/policies/global/policy/match" + assert _texts(translated, f"{match}/from-zone") == ["APPS"] + assert _texts(translated, f"{match}/to-zone") == ["UNTRUST"] + + +def test_nat_zone_lists_renamed_and_deduped(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + assert _texts(translated, "security/nat/source/rule-set/from/zone") == ["APPS"] + assert _texts(translated, "security/nat/source/rule-set/to/zone") == ["UNTRUST"] + + +def test_address_book_attach_renamed_and_deduped(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + assert _texts(translated, "security/address-book/attach/zone/name") == ["APPS"] + + +def _context_rules(config: etree._Element, from_zone: str, to_zone: str) -> list[str]: + contexts = [ + ctx + for ctx in config.xpath("security/policies/policy") + if ctx.findtext("from-zone-name") == from_zone and ctx.findtext("to-zone-name") == to_zone + ] + assert len(contexts) == 1, f"expected one {from_zone}->{to_zone} context, got {len(contexts)}" + return [rule.findtext("name") for rule in contexts[0].findall("policy")] + + +def test_collapsed_contexts_concatenated_in_master_order(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + assert _context_rules(translated, "APPS", "UNTRUST") == [ + "app1-web", + "app2-web", + "app2-dns", + "app3-ssh", + ] + + +def test_contexts_between_merged_zones_become_intra_zone(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + assert _context_rules(translated, "APPS", "APPS") == ["app1-to-app2"] + + +def test_duplicate_rule_name_in_merged_context_raises(): + source = _load() + app3_rule = source.xpath("security/policies/policy[from-zone-name='APP3']/policy/name")[0] + app3_rule.text = "app1-web" + with pytest.raises(ZoneTranslationError, match=r"app1-web.*APP1->UNTRUST.*APP3->UNTRUST"): + ZoneTranslator(APPS_MAP).apply(source) + + +def test_merged_zones(): + source = _load() + assert ZoneTranslator(APPS_MAP).merged_zones(source) == frozenset({"APPS"}) + assert ZoneTranslator({"UNTRUST": "OUTSIDE"}).merged_zones(source) == frozenset() + + +def test_merged_zones_counts_unmapped_master_zone_with_same_name(): + source = _load() + assert ZoneTranslator({"APP1": "UNTRUST"}).merged_zones(source) == frozenset({"UNTRUST"}) + + +def test_n_to_one_zone_becomes_single_empty_shell(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + apps = translated.xpath("security/zones/security-zone[name='APPS']") + assert len(apps) == 1 + assert [child.tag for child in apps[0]] == ["name"] + + +def test_one_to_one_zone_keeps_settings(): + translated = ZoneTranslator(APPS_MAP).apply(_load()) + untrust = translated.xpath("security/zones/security-zone[name='UNTRUST']")[0] + assert untrust.findtext("screen") == "untrust-screen" + + +def test_unknown_keys(): + source = _load() + translator = ZoneTranslator({"APP1": "APPS", "NOPE": "APPS"}) + assert translator.unknown_keys(source) == ["NOPE"] + + +def test_unknown_keys_empty_when_zones_not_fetched(): + config = etree.fromstring("") + assert ZoneTranslator({"NOPE": "APPS"}).unknown_keys(config) == [] + + +def test_merged_zones_ignores_map_keys_absent_on_master(): + translator = ZoneTranslator({"APP1": "APPS", "TYPO": "APPS"}) + assert translator.merged_zones(_load()) == frozenset() + translated = translator.apply(_load()) + apps = [ + zone + for zone in translated.xpath("security/zones/security-zone") + if zone.findtext("name") == "APPS" + ] + assert len(apps) == 1 + assert apps[0].find("host-inbound-traffic") is not None + + +def test_merged_zones_empty_without_master_zones(): + config = etree.fromstring("") + assert ZoneTranslator(APPS_MAP).merged_zones(config) == frozenset() + + +def test_apply_maps_into_existing_zone_yields_single_empty_shell(): + translated = ZoneTranslator({"APP1": "UNTRUST"}).apply(_load()) + untrust = [ + zone + for zone in translated.xpath("security/zones/security-zone") + if zone.findtext("name") == "UNTRUST" + ] + assert len(untrust) == 1 + assert [child.tag for child in untrust[0]] == ["name"] diff --git a/tests/unit/test_zonewarn.py b/tests/unit/test_zonewarn.py new file mode 100644 index 0000000..ea49827 --- /dev/null +++ b/tests/unit/test_zonewarn.py @@ -0,0 +1,72 @@ +from lxml import etree + +from srxsync.zonewarn import zone_warnings + +BIND_HINT = ( + "bind an interface on the slave " + "(set security zones security-zone {zone} interfaces )" +) + + +def _xml(text: str) -> etree._Element: + return etree.fromstring(text) + + +TARGET = _xml( + "" + "UNTRUST" + "ge-0/0/0.0" + "EMPTY" + "" +) + + +def test_creating_zone_warns(): + payload = _xml( + "" + "APPS" + "" + ) + assert zone_warnings(payload, TARGET, host="srx-a") == [ + f"srx-a: creating zone APPS with no interfaces — {BIND_HINT.format(zone='APPS')}" + ] + + +def test_existing_zone_without_interfaces_warns(): + payload = _xml( + "" + "EMPTYUNTRUST" + "" + ) + assert zone_warnings(payload, TARGET, host="srx-a") == [ + f"srx-a: zone EMPTY exists with no interfaces — {BIND_HINT.format(zone='EMPTY')}" + ] + + +def test_zone_with_interfaces_is_silent(): + payload = _xml( + "rs" + "UNTRUST" + ) + assert zone_warnings(payload, TARGET, host="srx-a") == [] + + +def test_referenced_but_missing_zone_warns(): + payload = _xml( + "g" + "APPSUNTRUST" + "" + ) + assert zone_warnings(payload, TARGET, host="srx-a") == [ + 'srx-a: zone APPS is referenced but missing on the slave — add "zones" to include ' + "or create it" + ] + + +def test_special_zones_ignored(): + payload = _xml( + "" + "junos-hostUNTRUST" + "" + ) + assert zone_warnings(payload, TARGET, host="srx-a") == []