diff --git a/src/octodns_gitops/cli/drift.py b/src/octodns_gitops/cli/drift.py index 5d4c515..08e10e8 100644 --- a/src/octodns_gitops/cli/drift.py +++ b/src/octodns_gitops/cli/drift.py @@ -1,13 +1,19 @@ -#!/usr/bin/env python3 """ Check for drift between live DNS and local zone files. Uses octodns-sync in reverse direction (live as source, local as target) to detect if live DNS has drifted from the configured zones. +One reversed run per live provider: reversing a multi-target zone's full +targets list into a single sources list would populate one octoDNS zone +object from every live provider, and any record present in more than one +of them raises DuplicateRecordException (octoDNS populates with +lenient=False). With a shadow-provider setup every record collides, so a +combined run can never work (#4). + Exit codes: - 0 - No drift detected (live matches local) - 1 - Drift detected (live differs from local) + 0 - No drift detected (live matches local for every provider) + 1 - Drift detected (live differs from local in at least one provider) 2 - Error occurred """ @@ -25,13 +31,34 @@ ) -def generate_drift_config(config_path: str, output_path: str) -> None: - """ - Generate a config for drift detection by reversing source/target. +def live_providers(zones: dict) -> list: + """Every live provider named in any zone's targets, deduplicated, + in first-appearance order.""" + providers = [] + for zone_cfg in zones.values(): + # a bare `targets:` key loads as None -- treat as empty, like + # octoDNS's own manager does + for target in (zone_cfg or {}).get("targets") or []: + if target not in providers: + providers.append(target) + return providers - For each zone: - - Original: sources=[zones], targets=[live-provider] - - Reversed: sources=[live-provider], targets=[zones] + +def generate_drift_config(config_path: str, output_path: str, provider: str) -> dict: + """ + Generate a drift-detection config for ONE live provider. + + For each zone that targets `provider`: + - Original: sources=[zones], targets=[..., provider, ...] + - Reversed: sources=[provider], targets=[zones] + + Zones that do not target `provider` are kept as inert blockers + (original sources, targets: []): octoDNS skips them ("no eligible + targets") without populating anything, but the key stays present, so + a dynamic ('*'-prefixed) entry's expansion still subtracts it from + its candidates exactly as it would in the original config, and a + --zone filter naming such a zone resolves cleanly instead of + erroring. Returns the zones mapping that was written. """ with open(config_path, "r") as f: cfg = yaml.safe_load(f) @@ -39,17 +66,26 @@ def generate_drift_config(config_path: str, output_path: str) -> None: providers = cfg.get("providers", {}) zones = cfg.get("zones", {}) - # Build reversed zone config + # Build reversed zone config, scoped to this provider reversed_zones = {} for zone_name, zone_cfg in zones.items(): - targets = zone_cfg.get("targets", []) - if not targets: - continue - - reversed_zones[zone_name] = { - "sources": list(targets), # Live providers become sources - "targets": ["zones"], # Local YAML becomes target - } + zone_cfg = zone_cfg or {} + # a bare `targets:` key loads as None -- treat as empty, like + # octoDNS's own manager does + targets = zone_cfg.get("targets") or [] + if provider in targets: + reversed_zones[zone_name] = { + "sources": [provider], # This live provider becomes the source + "targets": ["zones"], # Local YAML becomes target + } + else: + # Inert blocker: never planned, but blocks dynamic expansion. + # Original sources are kept so a dynamic blocker expands into + # concrete blockers the same way octoDNS expands the original. + reversed_zones[zone_name] = { + "sources": zone_cfg.get("sources") or [], + "targets": [], + } out_cfg = { "providers": providers, @@ -60,11 +96,18 @@ def generate_drift_config(config_path: str, output_path: str) -> None: if "processors" in cfg: out_cfg["processors"] = cfg["processors"] if "manager" in cfg: - out_cfg["manager"] = cfg["manager"] + # plan_outputs writes to a fixed filename; with one run per + # provider each run would overwrite the previous provider's plan + manager = dict(cfg["manager"] or {}) + manager.pop("plan_outputs", None) + if manager: + out_cfg["manager"] = manager with open(output_path, "w") as f: yaml.safe_dump(out_cfg, f, sort_keys=False) + return reversed_zones + def main() -> int: p = argparse.ArgumentParser( @@ -78,72 +121,96 @@ def main() -> int: bin_dir = os.path.dirname(sys.executable) sync_bin = os.path.join(bin_dir, "octodns-sync") - # Generate reversed config in temp file - with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: - drift_config_path = f.name - - try: - generate_drift_config(args.config, drift_config_path) - - # Run octodns-sync in dry-run mode (no --doit) - cmd = [ - sync_bin, - "--config-file", - drift_config_path, - "--force", # Show all changes regardless of threshold - ] - - debug = os.environ.get("DEBUG") - quiet = os.environ.get("QUIET", "1") + with open(args.config, "r") as f: + cfg = yaml.safe_load(f) + providers = live_providers(cfg.get("zones", {})) - if args.logging_config: - cmd.extend(["--logging-config", args.logging_config]) - elif debug: - cmd.append("--debug") - elif quiet: - cmd.append("--quiet") + debug = os.environ.get("DEBUG") + quiet = os.environ.get("QUIET", "1") + env = os.environ.copy() + env["PYTHONPATH"] = os.getcwd() - if args.zone: - cmd.append(args.zone) + drifted = {} # provider -> octodns-sync plan output + temp_paths = [] + try: + for provider in providers: + # One generated config and one octodns-sync run per provider + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yaml", delete=False + ) as f: + drift_config_path = f.name + temp_paths.append(drift_config_path) + + generate_drift_config(args.config, drift_config_path, provider) + + # --zone is passed straight through: octoDNS applies the + # filter itself (IdnaDict case/IDNA normalization, dynamic + # zone expansion), and a provider not serving the zone hits + # its inert blocker entry -> "No changes were planned" + + # Run octodns-sync in dry-run mode (no --doit) + cmd = [ + sync_bin, + "--config-file", + drift_config_path, + "--force", # Show all changes regardless of threshold + ] + + if args.logging_config: + cmd.extend(["--logging-config", args.logging_config]) + elif debug: + cmd.append("--debug") + elif quiet: + cmd.append("--quiet") + + if args.zone: + cmd.append(args.zone) + + result = subprocess.run( + cmd, env=env, capture_output=True, text=True, check=False + ) + + if result.returncode != 0: + stderr = result.stderr or "" + if is_credentials_error(stderr): + print( + format_missing_credentials_error(args.config, stderr), + file=sys.stderr, + ) + else: + print( + f"Failed to check drift (provider: {provider})", + file=sys.stderr, + ) + if stderr: + lines = stderr.strip().split("\n") + for line in lines[-10:]: + print(f" {line}", file=sys.stderr) + return 2 - env = os.environ.copy() - env["PYTHONPATH"] = os.getcwd() + stderr = result.stderr or "" - result = subprocess.run(cmd, env=env, capture_output=True, text=True) + # "No changes were planned" means this provider matches local + if "No changes were planned" not in stderr: + drifted[provider] = stderr - if result.returncode != 0: - stderr = result.stderr or "" - if is_credentials_error(stderr): - print( - format_missing_credentials_error(args.config, stderr), - file=sys.stderr, - ) - else: - print("Failed to check drift", file=sys.stderr) - if stderr: - lines = stderr.strip().split("\n") - for line in lines[-10:]: - print(f" {line}", file=sys.stderr) - return 2 - - stderr = result.stderr or "" - - # Check if there are no changes (no drift) - if "No changes were planned" in stderr: + if not drifted: print("No drift detected") return 0 - # Drift detected - show what's different + # Drift detected - show what's different, per provider print("Drift detected: live DNS differs from local zones") - print() - print("Changes needed to sync live -> local:") - print(stderr) + for provider, stderr in drifted.items(): + print() + print(f"Changes needed to sync live -> local (provider: {provider}):") + print(stderr) return 1 finally: - # Clean up temp file - if os.path.exists(drift_config_path): - os.unlink(drift_config_path) + # Clean up temp files + for path in temp_paths: + if os.path.exists(path): + os.unlink(path) if __name__ == "__main__": diff --git a/tests/cli/test_drift.py b/tests/cli/test_drift.py index e4f100e..dd86ae1 100644 --- a/tests/cli/test_drift.py +++ b/tests/cli/test_drift.py @@ -1,12 +1,11 @@ """Tests for cli/drift.py""" -import pytest -from unittest.mock import patch, MagicMock -import os import tempfile from pathlib import Path +from unittest.mock import MagicMock, patch -from octodns_gitops.cli.drift import generate_drift_config, main +import pytest +from octodns_gitops.cli.drift import generate_drift_config, live_providers, main class TestGenerateDriftConfig: @@ -34,7 +33,7 @@ def test_reverses_sources_targets(self, tmp_path): - hetzner """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml @@ -65,7 +64,7 @@ def test_preserves_providers(self, tmp_path): - hetzner """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml @@ -99,7 +98,7 @@ def test_preserves_processors(self, tmp_path): class: octodns_gitops.processors.ExternalDnsFilter """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml @@ -132,7 +131,7 @@ def test_preserves_manager(self, tmp_path): max_workers: 4 """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml @@ -142,8 +141,10 @@ def test_preserves_manager(self, tmp_path): assert "manager" in result assert result["manager"]["max_workers"] == 4 - def test_skips_zones_without_targets(self, tmp_path): - """Zones without targets should be skipped.""" + def test_zone_without_targets_becomes_blocker(self, tmp_path): + """A zone not targeting the provider stays in the config as an + inert blocker (targets: []) so octoDNS skips it but dynamic + expansion still sees the key.""" config_in = tmp_path / "config.yaml" config_out = tmp_path / "drift.yaml" @@ -159,14 +160,55 @@ def test_skips_zones_without_targets(self, tmp_path): # No targets """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml with open(config_out) as f: result = yaml.safe_load(f) - assert "example.com." not in result.get("zones", {}) + assert result["zones"]["example.com."] == { + "sources": ["zones"], + "targets": [], + } + + def test_manager_plan_outputs_stripped(self, tmp_path): + """manager.plan_outputs writes to a fixed filename; with one run + per provider each run would overwrite the previous provider's + plan, so it must not be copied (PR #5 review round 2).""" + config_in = tmp_path / "config.yaml" + config_out = tmp_path / "drift.yaml" + + config_in.write_text(""" +providers: + zones: + class: octodns.provider.yaml.YamlProvider + hetzner: + class: octodns_hetzner.HetznerProvider + +zones: + example.com.: + sources: + - zones + targets: + - hetzner + +manager: + max_workers: 4 + plan_outputs: + json: + class: octodns.provider.plan.PlanJson + filename: plan.json +""") + + generate_drift_config(str(config_in), str(config_out), provider="hetzner") + + import yaml + + with open(config_out) as f: + result = yaml.safe_load(f) + + assert result["manager"] == {"max_workers": 4} def test_uses_zones_as_target(self, tmp_path): """Local YAML provider should be target in reversed config.""" @@ -188,7 +230,7 @@ def test_uses_zones_as_target(self, tmp_path): - hetzner """) - generate_drift_config(str(config_in), str(config_out)) + generate_drift_config(str(config_in), str(config_out), provider="hetzner") import yaml @@ -365,9 +407,9 @@ def test_cleans_up_temp(self, mock_subprocess, tmp_path): # Check no new temp yaml files remain temp_files_after = set(Path(tempfile.gettempdir()).glob("*.yaml")) - new_files = temp_files_after - temp_files_before # May have some other yaml files, but drift config should be cleaned # This is a weak test - just ensure it doesn't crash + assert temp_files_after >= temp_files_before def test_zone_filter(self, mock_subprocess, tmp_path): """--zone flag should filter to specific zone.""" @@ -400,3 +442,289 @@ def test_zone_filter(self, mock_subprocess, tmp_path): cmd = mock_subprocess.call_args[0][0] assert "example.com." in cmd + + +MULTI_TARGET_CONFIG = """ +providers: + zones: + class: octodns.provider.yaml.YamlProvider + hetzner: + class: octodns_hetzner.HetznerProvider + token: env/TOKEN + desec: + class: octodns_desec.DesecProvider + token: env/TOKEN2 + +zones: + example.com.: + sources: + - zones + targets: + - hetzner + - desec + single.example.: + sources: + - zones + targets: + - hetzner +""" + + +class TestMultiTargetZones: + """One reversed config per live provider. + + Regression tests for #4: reversing a multi-target zone's full targets + list into sources populates one octoDNS zone object from every live + provider, and any record present in more than one of them raises + DuplicateRecordException (lenient=False). With a shadow-provider setup + every record collides, so drift-check could never run at all. + """ + + def test_live_providers_ordered_dedup(self, tmp_path): + """All live providers, in first-appearance order, deduplicated.""" + import yaml + + cfg = yaml.safe_load(MULTI_TARGET_CONFIG) + assert live_providers(cfg["zones"]) == ["hetzner", "desec"] + + def test_config_scoped_to_provider(self, tmp_path): + """A provider's config carries only that provider as source, and + only the zones that target it.""" + config_in = tmp_path / "config.yaml" + config_in.write_text(MULTI_TARGET_CONFIG) + + import yaml + + out_desec = tmp_path / "drift-desec.yaml" + generate_drift_config(str(config_in), str(out_desec), provider="desec") + with open(out_desec) as f: + result = yaml.safe_load(f) + assert result["zones"]["example.com."]["sources"] == ["desec"] + assert result["zones"]["example.com."]["targets"] == ["zones"] + # single.example. does not target desec -> inert blocker: octoDNS + # skips it (no targets) but the key still blocks dynamic expansion + assert result["zones"]["single.example."] == { + "sources": ["zones"], + "targets": [], + } + + out_hetzner = tmp_path / "drift-hetzner.yaml" + generate_drift_config(str(config_in), str(out_hetzner), provider="hetzner") + with open(out_hetzner) as f: + result = yaml.safe_load(f) + assert set(result["zones"]) == {"example.com.", "single.example."} + for zone_cfg in result["zones"].values(): + assert zone_cfg["sources"] == ["hetzner"] + + def test_live_providers_null_targets(self): + """`targets:` with no value loads as None -- treat as empty, like + octoDNS's own manager does (PR #5 review, P2).""" + zones = {"a.example.": {"targets": None}, "b.example.": None} + assert live_providers(zones) == [] + + def test_null_targets_zone_becomes_blocker(self, tmp_path): + """A zone with a null `targets:` must become a blocker, not crash.""" + config_in = tmp_path / "config.yaml" + config_in.write_text(""" +providers: + zones: + class: octodns.provider.yaml.YamlProvider + hetzner: + class: octodns_hetzner.HetznerProvider + token: env/TOKEN + +zones: + example.com.: + sources: + - zones + targets: + - hetzner + empty.example.: + sources: + - zones + targets: +""") + out = tmp_path / "drift.yaml" + generate_drift_config(str(config_in), str(out), provider="hetzner") + + import yaml + + with open(out) as f: + result = yaml.safe_load(f) + assert result["zones"]["example.com."]["sources"] == ["hetzner"] + assert result["zones"]["empty.example."]["targets"] == [] + + def test_shadowed_explicit_zone_kept_as_blocker(self, tmp_path): + """'*' targets [p1, p2] while special.example. targets only [p1]: + p2's config must keep special.example. as a blocker, or octoDNS's + wildcard expansion re-includes it for p2 (explicit keys are + subtracted from dynamic candidates -- PR #5 review round 2).""" + config_in = tmp_path / "config.yaml" + config_in.write_text(""" +providers: + zones: + class: octodns.provider.yaml.YamlProvider + p1: + class: octodns_hetzner.HetznerProvider + token: env/TOKEN + p2: + class: octodns_desec.DesecProvider + token: env/TOKEN2 + +zones: + '*': + sources: + - zones + targets: + - p1 + - p2 + special.example.: + sources: + - zones + targets: + - p1 +""") + + import yaml + + out = tmp_path / "drift-p2.yaml" + generate_drift_config(str(config_in), str(out), provider="p2") + with open(out) as f: + result = yaml.safe_load(f) + assert result["zones"]["*"]["sources"] == ["p2"] + assert result["zones"]["special.example."] == { + "sources": ["zones"], + "targets": [], + } + + +DYNAMIC_ZONE_CONFIG = """ +providers: + zones: + class: octodns.provider.yaml.YamlProvider + hetzner: + class: octodns_hetzner.HetznerProvider + token: env/TOKEN + +zones: + '*': + sources: + - zones + targets: + - hetzner +""" + + +class TestZoneFilterDynamicZones: + """--zone must not skip a provider whose config uses dynamic zone + entries ('*'-prefixed keys): only octoDNS can expand those, so the + concrete zone name never appears as a key here (PR #5 review, P1).""" + + def test_dynamic_zone_config_not_skipped(self, tmp_path): + config = tmp_path / "config.yaml" + config.write_text(DYNAMIC_ZONE_CONFIG) + + with patch("subprocess.run") as mock_run: + mock_run.return_value = MagicMock( + returncode=0, stdout="", stderr="No changes were planned" + ) + argv = ["drift", "--config", str(config), "--zone", "example.com."] + with patch("sys.argv", argv): + result = main() + + assert result == 0 + assert mock_run.call_count == 1 + cmd = mock_run.call_args[0][0] + assert "example.com." in cmd + + +class TestMainMultiTarget: + """main() runs octodns-sync once per live provider and aggregates.""" + + @pytest.fixture + def mock_subprocess(self): + with patch("subprocess.run") as mock_run: + yield mock_run + + @pytest.fixture + def config(self, tmp_path): + config = tmp_path / "config.yaml" + config.write_text(MULTI_TARGET_CONFIG) + return config + + @staticmethod + def _result(returncode=0, stderr="No changes were planned"): + return MagicMock(returncode=returncode, stdout="", stderr=stderr) + + def test_runs_sync_once_per_provider(self, mock_subprocess, config, capsys): + """Two live providers -> two octodns-sync runs; all clean -> 0.""" + mock_subprocess.side_effect = [self._result(), self._result()] + + with patch("sys.argv", ["drift", "--config", str(config)]): + result = main() + + assert result == 0 + assert mock_subprocess.call_count == 2 + captured = capsys.readouterr() + assert "No drift" in captured.out + + def test_each_run_gets_its_own_config(self, mock_subprocess, config): + """The two runs must use two different generated config files.""" + mock_subprocess.side_effect = [self._result(), self._result()] + + with patch("sys.argv", ["drift", "--config", str(config)]): + main() + + config_files = [ + call.args[0][call.args[0].index("--config-file") + 1] + for call in mock_subprocess.call_args_list + ] + assert len(set(config_files)) == 2 + + def test_drift_in_one_provider_returns_one_and_names_it( + self, mock_subprocess, config, capsys + ): + """Drift in the second provider only -> 1, report names the provider.""" + mock_subprocess.side_effect = [ + self._result(), + self._result(stderr="* example.com.\n* Create \n"), + ] + + with patch("sys.argv", ["drift", "--config", str(config)]): + result = main() + + assert result == 1 + captured = capsys.readouterr() + assert "Drift detected" in captured.out + # provider order is first-appearance: hetzner then desec + assert "desec" in captured.out + + def test_zone_passed_to_all_providers(self, mock_subprocess, config): + """--zone runs against every provider: octoDNS itself applies the + filter (IdnaDict normalization, dynamic expansion), and a + provider not serving the zone hits its inert blocker entry and + planning is skipped (PR #5 review round 2).""" + mock_subprocess.side_effect = [self._result(), self._result()] + + argv = ["drift", "--config", str(config), "--zone", "single.example."] + with patch("sys.argv", argv): + result = main() + + assert result == 0 + assert mock_subprocess.call_count == 2 + for call in mock_subprocess.call_args_list: + assert "single.example." in call.args[0] + + def test_error_in_first_provider_returns_two( + self, mock_subprocess, config, capsys + ): + """A failed sync run -> 2, remaining providers not reached.""" + mock_subprocess.side_effect = [ + self._result(returncode=1, stderr="Some error occurred"), + ] + + with patch("sys.argv", ["drift", "--config", str(config)]): + result = main() + + assert result == 2 + assert mock_subprocess.call_count == 1