Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,13 @@ Contract:
`subject` and `message`.
- `max_quota_per_alias` and `bounce_webhook` cannot be read back from the API: they are sent with
every domain update but never produce a diff on their own.
- Forward Email re-validates an alias's **stored** regex pattern on every write to that alias, and
no longer accepts Perl-style look-around (`(?!`, `(?=`, `(?<`). A grandfathered look-around
alias exports and plans normally but 400s on any update — even one that changes nothing else,
and even when the update body omits the name. The plan prints a WARNING when it is about to
touch such an alias; changing anything on it means recreating it with a supported pattern (or
pinning its current live values in git so no update is planned). One failing alias write does
not skip the domain's remaining writes; the run still exits 1.

## Quick Start with mise

Expand Down
33 changes: 24 additions & 9 deletions src/octodns_gitops/cli/forward_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def _print_plan(plan: DomainPlan, out: TextIO) -> None:
f" expect: {m.field} is {m.live!r}, expected {m.desired!r} (read-only, not written)\n"
for m in plan.expect_mismatch
)
out.writelines(f" WARNING: {w}\n" for w in plan.warnings)
out.writelines(f" ERROR: {e}\n" for e in plan.errors)


Expand Down Expand Up @@ -142,11 +143,14 @@ def _prune_still_safe(plan: DomainPlan, deletes: list, client, out: TextIO) -> b


def _apply(plan: DomainPlan, client, out: TextIO) -> bool:
"""Perform the writes for one domain. Returns False if the prune guard aborted.
"""Perform the writes for one domain. Returns False if the prune guard aborted or any
alias write failed — the domain's remaining, independent alias writes are still attempted
(ginsys/octodns-gitops#2: one 400 must not hold the rest of its domain hostage).

Order: settings PUT, prune re-list, creates/updates, deletes. The re-list must precede
our own creates — they change the id set, so a create+delete plan would otherwise always
abort half-applied.
abort half-applied. A settings PUT failure still aborts the domain: it is domain-level,
not one alias's problem.
"""
body = plan.settings_body()
if body:
Expand All @@ -155,18 +159,29 @@ def _apply(plan: DomainPlan, client, out: TextIO) -> bool:
deletes = [c for c in plan.aliases if c.action == "delete"]
if deletes and not _prune_still_safe(plan, deletes, client, out):
return False
ok = True
for chg in plan.aliases:
if chg.action == "create":
client.create_alias(plan.domain, chg.body)
elif chg.action == "update":
client.update_alias(plan.domain, chg.alias_id, chg.body)
else:
try:
if chg.action == "create":
client.create_alias(plan.domain, chg.body)
elif chg.action == "update":
client.update_alias(plan.domain, chg.alias_id, chg.body)
else:
continue
except ForwardEmailApiError as e:
out.write(f" ERROR alias {chg.action} {chg.name}: {e}\n")
ok = False
continue
out.write(f" applied alias {chg.action} {chg.name}\n")
for chg in deletes:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not prune aliases after a replacement create fails

With PRUNE=1, a rename or migration is planned as a create for the new alias plus a delete for the old alias. If that create raises ForwardEmailApiError, ok becomes false but this loop still deletes the old, working alias, leaving the address with no delivery target; previously the exception reached the outer handler before any deletes ran. Continue attempting the remaining creates/updates, but skip the delete phase if any of those writes failed.

Useful? React with 👍 / 👎.

client.delete_alias(plan.domain, chg.alias_id)
try:
client.delete_alias(plan.domain, chg.alias_id)
except ForwardEmailApiError as e:
out.write(f" ERROR alias delete {chg.name}: {e}\n")
ok = False
continue
out.write(f" applied alias delete {chg.name}\n")
return True
return ok


def _preserved_write_only(path: Path) -> dict:
Expand Down
18 changes: 18 additions & 0 deletions src/octodns_gitops/forward_email/reconcile.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class DomainPlan:
unmanaged: list[str] = field(default_factory=list)
expect_mismatch: list[SettingChange] = field(default_factory=list)
errors: list[str] = field(default_factory=list)
warnings: list[str] = field(default_factory=list)
# ids seen in the listing the plan was built from; the apply path re-lists before a prune
live_alias_ids: set = field(default_factory=set)

Expand All @@ -91,6 +92,21 @@ def settings_body(self) -> dict:
# Alias fields compared field-by-field (everything else needs special handling).
_ALIAS_SIMPLE = ("is_enabled", "error_code_if_disabled", "has_imap", "has_pgp", "has_recipient_verification")

# Forward Email rejects Perl-style look-around in alias regex names on EVERY write to the alias —
# it re-validates the stored pattern even when the update body omits `name` (probed live
# 2026-08-30, ginsys/octodns-gitops#2). Such aliases exist grandfathered, so export happily
# round-trips them; warn the moment a change is planned instead of failing mid-apply.
_LOOKAROUND = ("(?=", "(?!", "(?<")


def _warn_lookaround(plan: DomainPlan, name: str, action: str) -> None:
if any(tok in name for tok in _LOOKAROUND):
plan.warnings.append(
f"alias {name}: Forward Email rejects Perl-style look-around patterns on every write "
f"(it re-validates the stored pattern), so this {action} will fail until the alias is "
"recreated with a supported pattern"
)


def _resolve_alias(desired: DesiredAlias, defaults: dict) -> dict:
"""Desired alias with repo/package defaults filled in for undeclared fields."""
Expand Down Expand Up @@ -222,10 +238,12 @@ def plan_domain(
continue
if live is None:
plan.aliases.append(AliasChange("create", want.name, None, _alias_body(resolved), ["new"]))
_warn_lookaround(plan, want.name, "create")
continue
chg = _diff_alias(resolved, live, cfg.alias, domain_quota)
if chg:
plan.aliases.append(chg)
_warn_lookaround(plan, want.name, "update")

wanted = {a.name for a in desired.aliases}
for live in live_aliases:
Expand Down
49 changes: 49 additions & 0 deletions tests/cli/test_forward_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
run,
zone_lookup,
)
from octodns_gitops.forward_email.api import ForwardEmailApiError
from octodns_gitops.forward_email.config import (
DEFAULT_ALIAS,
DEFAULT_EXPECT,
Expand Down Expand Up @@ -140,6 +141,32 @@ def test_zero_diff_exits_0_and_writes_nothing(self, tmp_path):
assert "no changes" in out
assert client.writes() == []

def test_lookaround_alias_with_a_planned_change_warns_at_plan_time(self, tmp_path):
# FE rejects Perl look-around on every write to such an alias (it validates the stored
# pattern, so even a quota-only update fails): say so at plan time, not mid-apply.
name = "/^(?!keep$)(.*)$/"
_write_domain_file(
tmp_path,
"x.be",
"aliases:\n - name: '/^(?!keep$)(.*)$/'\n recipients: [new@y.z]\n",
)
client = FakeClient([_live_domain("x.be")], {"x.be": [_live_alias(name)]})
rc, out = _run(_cfg(tmp_path), client)
assert rc == 0
assert "WARNING" in out and "look-around" in out

def test_lookaround_alias_with_no_planned_change_stays_silent(self, tmp_path):
name = "/^(?!keep$)(.*)$/"
_write_domain_file(
tmp_path,
"x.be",
"aliases:\n - name: '/^(?!keep$)(.*)$/'\n recipients: [serge@ginsys.eu]\n",
)
client = FakeClient([_live_domain("x.be")], {"x.be": [_live_alias(name)]})
rc, out = _run(_cfg(tmp_path), client)
assert rc == 0
assert "WARNING" not in out and "no changes" in out

def test_changes_are_printed_but_not_applied_without_doit(self, tmp_path):
_write_domain_file(tmp_path, "x.be", "aliases:\n - {name: a, recipients: [serge@ginsys.eu]}\n")
client = FakeClient([_live_domain("x.be", retention_days=0)], {"x.be": [_live_alias("a")]})
Expand Down Expand Up @@ -259,6 +286,28 @@ def test_mailbox_guard_blocks_the_whole_domain_apply(self, tmp_path):
assert "mailbox" in out
assert client.writes() == []

def test_one_failing_alias_write_does_not_abort_the_domains_remaining_writes(self, tmp_path):
# ginsys/octodns-gitops#2: FE 400s updates to a grandfathered look-around alias; the
# domain's other, independent alias writes must still be attempted (and rc stay 1).
_write_domain_file(
tmp_path,
"x.be",
"aliases:\n - {name: bad, recipients: [new@y.z]}\n - {name: good, recipients: [new@y.z]}\n",
)

class Rejecting(FakeClient):
def update_alias(self, domain, alias_id, body):
if alias_id == "id-bad":
raise ForwardEmailApiError(400, "invalid perl operator: (?!", "PUT", "u")
return super().update_alias(domain, alias_id, body)

client = Rejecting([_live_domain("x.be")], {"x.be": [_live_alias("bad"), _live_alias("good")]})
rc, out = _run(_cfg(tmp_path), client, doit=True)
assert rc == 1
assert "ERROR" in out and "bad" in out and "400" in out
applied = {c[2] for c in client.calls if c[0] == "update_alias"}
assert "id-good" in applied

def test_unmanaged_alias_without_prune_is_reported_only(self, tmp_path):
_write_domain_file(tmp_path, "x.be", "aliases: []\n")
client = FakeClient([_live_domain("x.be")], {"x.be": [_live_alias("stray")]})
Expand Down
Loading