Skip to content

Commit ab7cdd9

Browse files
authored
Merge pull request #9 from domainaware/fix/migrate-lookup-propagate-exceptions
Fix migrate refill-enrichment silently skipping source_name/source_type
2 parents 445abc7 + 60e770d commit ab7cdd9

5 files changed

Lines changed: 62 additions & 16 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ A domain can only belong to one client at a time. Offboarded domains can be re-a
132132
- **OSD index-pattern field cache — refresh after any field-schema change.** Dashboards index-pattern saved objects cache `attributes.fields` at import time and never auto-refresh. Any operation that adds or renames fields on existing indices (parsedmarc upgrades, `migrate rename-asn-fields`, custom mapping changes) must call `DashboardService.refresh_index_pattern_fields(tenant_name)` per active tenant, or Discover shows "no cached mapping" warnings on the new/renamed fields. `migrate rename-asn-fields` and `migrate all` do this automatically via the `_refresh_tenant_index_patterns` helper in `cli/migrate.py`.
133133
- **Don't use broad `except Exception` to make external calls "safe".** If the only thing a block does is call one external function, `except Exception` inside it almost certainly masks programming errors (bad kwargs, `ImportError`, `AttributeError`) as silent no-ops. `migrate refill-enrichment` silently produced zero updates for an entire release because a per-IP `try/except Exception` in the parsedmarc lookup script turned `TypeError: unexpected keyword argument 'parallel'` into `{ip: None}`. Only catch exceptions you have an expected recovery path for (e.g., `json.JSONDecodeError` when parsing untrusted input). Let the rest propagate so the traceback surfaces on the first failure.
134134
- **Verify external library signatures before calling.** The `parallel=False` bug was a hallucinated kwarg — no such parameter exists on `parsedmarc.utils.get_ip_address_info`. When writing code that calls into parsedmarc or another library, confirm the function signature against the installed version (`python3 -c "import inspect, parsedmarc.utils; print(inspect.signature(parsedmarc.utils.get_ip_address_info))"`) rather than relying on memory or similar-looking code elsewhere.
135+
- **Inspect the parsedmarc that actually runs, not the local pip copy.** The migrate commands shell into the `parsedmarc` container via `docker exec`, so the behavior that matters is the `ghcr.io/domainaware/parsedmarc:latest` image — not whatever the developer happens to have in a venv. These can diverge: `get_ip_address_info` in local pip-installed 9.6.0 returns only `{ip_address, reverse_dns, country, base_domain, name, type}`, but the container build also returns `asn`, `as_name`, and `as_domain`. Dropping `source_as_name`/`source_as_domain` from `FIELD_TO_PARSEDMARC_KEY` based on the local signature would have silently broken ASN enrichment in production. When the code path runs inside a container, verify signatures and return shapes against that container (`docker compose exec parsedmarc python3 -c "…"`), not a local install.
135136

136137
## Concurrency Model
137138

CHANGELOG.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,22 @@
11
# Changelog
22

3+
## 0.6.3 2026-04-23
4+
5+
### Fixed
6+
7+
- `dmarcmsp migrate refill-enrichment` now actually re-enriches `source_name`
8+
and `source_type`. The lookup helper called
9+
`parsedmarc.utils.get_ip_address_info(ip, offline=True)`, which
10+
short-circuits the PTR lookup; that in turn skipped the reverse-DNS map
11+
entirely (the map is only consulted after a successful PTR), so `name` and
12+
`type` always came back `None` and the patch loop silently no-op'd those
13+
fields. Only `source_country` (and ASN fields, when the container's
14+
parsedmarc returns them) were ever getting updated. The in-container helper
15+
now runs online and preloads the reverse-DNS map once per batch so it
16+
isn't re-fetched from GitHub per IP. The 0.6.2 fix unblocked the
17+
subprocess from crashing but `source_name` / `source_type` were never
18+
going to update regardless.
19+
320
## 0.6.2 2026-04-23
421

522
### Fixed

dmarc_msp/services/migrate.py

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@
55
* ``rename_asn_fields`` — the old ``source_asn_name`` / ``source_asn_domain``
66
fields were renamed upstream to ``source_as_name`` / ``source_as_domain``.
77
Existing documents still use the old names.
8-
* ``refill_enrichment_fields`` — parsedmarc's IP-based enrichment has improved
9-
(GeoIP swap ipdb → ipinfo, better source-name/type classification). Old
10-
documents carry stale values. We call ``parsedmarc.utils.get_ip_address_info``
11-
inside the parsedmarc container so old docs get the exact same enrichment
12-
new docs get.
8+
* ``refill_enrichment_fields`` — parsedmarc's IP-based enrichment evolves over
9+
time (GeoIP data updates, new entries in the reverse-DNS map used for
10+
source-name/type classification). Old documents carry stale values. We call
11+
``parsedmarc.utils.get_ip_address_info`` inside the parsedmarc container
12+
(with the live PTR lookup and reverse-DNS map enabled) so old docs get the
13+
same enrichment new docs get.
1314
1415
``DashboardService.refresh_index_pattern_fields`` handles the third piece
1516
(refreshing the cached field list inside each tenant's index-pattern saved
@@ -86,20 +87,35 @@ def existing = ctx._source.get(f);
8687
""".strip()
8788

8889
# Script run inside the parsedmarc container. Reads a JSON list of IPs from
89-
# stdin and writes a JSON dict {ip: info_dict} to stdout. offline=True skips
90-
# DNS/WHOIS so we only hit the local GeoIP DB and name/type classifier.
90+
# stdin and writes a JSON dict {ip: info_dict} to stdout.
9191
#
92-
# No per-IP try/except: with offline=True, get_ip_address_info returns a dict
93-
# with None values for misses rather than raising — the only exceptions that
94-
# can fire here are programming errors (bad kwargs, missing imports) that we
95-
# want to see loudly, not mask as {ip: None}. Let the subprocess fail and
96-
# surface its stderr via the CalledProcessError handler in _lookup_enrichment.
92+
# offline=False is required: with offline=True, parsedmarc short-circuits the
93+
# PTR lookup (reverse_dns = None), which in turn skips the reverse-DNS-map
94+
# lookup entirely (the map is only consulted after a successful PTR). That
95+
# means name/type/reverse_dns/base_domain always come back None and the
96+
# migration silently no-ops those fields, which defeats the whole purpose of
97+
# the command. Running online matches what parsedmarc itself does at ingest.
98+
#
99+
# The reverse-DNS map is loaded once per batch and passed by reference to every
100+
# call. Without this, get_service_from_reverse_dns_base_domain would re-fetch
101+
# the map from GitHub on every IP.
102+
#
103+
# No per-IP try/except: exceptions here are programming errors (bad kwargs,
104+
# missing imports) that we want to see loudly, not mask as {ip: None}. Let the
105+
# subprocess fail and surface its stderr via the CalledProcessError handler in
106+
# _lookup_enrichment.
97107
_PARSEDMARC_LOOKUP_SCRIPT = r"""
98108
import json, sys
99-
from parsedmarc.utils import get_ip_address_info
109+
from parsedmarc.utils import get_ip_address_info, load_reverse_dns_map
110+
111+
reverse_dns_map = {}
112+
load_reverse_dns_map(reverse_dns_map)
100113
101114
ips = json.load(sys.stdin)
102-
out = {ip: get_ip_address_info(ip, offline=True) for ip in ips}
115+
out = {
116+
ip: get_ip_address_info(ip, offline=False, reverse_dns_map=reverse_dns_map)
117+
for ip in ips
118+
}
103119
json.dump(out, sys.stdout)
104120
"""
105121

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
44

55
[project]
66
name = "dmarc-msp"
7-
version = "0.6.2"
7+
version = "0.6.3"
88
description = "DMARC monitoring automation for Managed Service Providers (and everyone else)"
99
license = "Apache-2.0"
1010
readme = "README.md"

tests/test_services/test_migrate.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,19 @@ def test_lookup_script_does_not_pass_unknown_kwargs():
2424
"""parsedmarc.utils.get_ip_address_info has no ``parallel`` kwarg;
2525
passing one silently failed every IP and produced zero enrichment."""
2626
assert "parallel" not in _PARSEDMARC_LOOKUP_SCRIPT
27-
assert "offline=True" in _PARSEDMARC_LOOKUP_SCRIPT
27+
28+
29+
def test_lookup_script_runs_online_with_preloaded_map():
30+
"""offline=True short-circuits the PTR lookup, which in turn skips the
31+
reverse-DNS-map lookup entirely — so name/type/reverse_dns/base_domain
32+
always come back None and the migration silently no-ops those fields.
33+
The script must run online AND preload the reverse-DNS map once per batch
34+
(otherwise get_service_from_reverse_dns_base_domain re-downloads the map
35+
from GitHub for every IP)."""
36+
assert "offline=False" in _PARSEDMARC_LOOKUP_SCRIPT
37+
assert "offline=True" not in _PARSEDMARC_LOOKUP_SCRIPT
38+
assert "load_reverse_dns_map" in _PARSEDMARC_LOOKUP_SCRIPT
39+
assert "reverse_dns_map=reverse_dns_map" in _PARSEDMARC_LOOKUP_SCRIPT
2840

2941

3042
def test_lookup_script_does_not_swallow_exceptions():

0 commit comments

Comments
 (0)