From b304cf8639bfcb7e7a6cbb5ba1756bcb52aac7a9 Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:48:00 -0400 Subject: [PATCH] Cover index_prefix_domain_map and index_suffix in index migrations (#868) (#869) * Cover index_prefix_domain_map and index_suffix in index migrations (#868) The startup Elasticsearch/OpenSearch index migrations built their target index names from the [elasticsearch]/[opensearch] index_prefix and index_suffix options alone, while the save path also honors general.index_prefix_domain_map. A multi-tenant deployment therefore ran the backfill guard `count` against dmarc_aggregate*/smtp_tls*, patterns matching none of its real _dmarc_aggregate-* indexes. The query passes allow_no_indices=True, so a zero-match wildcard returns count 0 -- indistinguishable from "already backfilled" -- and the backfill was skipped silently, with no log line at any level. Resolve migration index names through a new _migration_index_names() helper that widens both configurable axes: one name per tenant prefix in the map plus the unprefixed name (unmapped domains are still saved unprefixed), and, when an index_suffix is set, the unsuffixed name alongside the suffixed one so history predating the suffix is covered. A configured index_prefix still wins outright and suppresses the map fan-out, matching save-time precedence. The key normalization is now shared with get_index_prefix() via _normalize_index_prefix(), so the names parsedmarc migrates cannot drift from the ones it writes. The resolved lists are logged at DEBUG, and the SIGHUP reload path passes the freshly parsed map, so a newly onboarded tenant is covered without a restart. Also repair the legacy published_policy.fo migration, which has been unable to complete since mapping types were removed in Elasticsearch 7 (and never existed in OpenSearch): it read the field mapping in the type-keyed response shape, so the check always fell through, and its put_mapping() call passed a doc_type argument neither current client accepts. It now reads either response shape, uses each client's current signature, and takes its index names in a separate legacy_fo_indexes argument -- exact names, since 5.0.0 introduced date-suffixed index names in the same release that fixed the fo declaration, but prefixed and suffixed where configured, since both options date back to 4.1.0. Tenant prefixes are excluded: index_prefix_domain_map arrived in 8.19.0, and this migration renames the index it rebuilds. The Elasticsearch copy, removed as unreachable during the #806 client migration, is restored now that the cause is understood. Finally, reject an index_prefix_domain_map YAML file that is not a mapping of string tenant names to lists of domains, instead of raising mid-save on a non-string key or silently matching the wrong domains on a scalar value -- `in` on a str is a substring test, so "example.co" matches "example.com" (https://docs.python.org/3/reference/expressions.html#membership-test-operations). Co-Authored-By: Claude Opus 5 (1M context) * Fix OpenSearch capitalization and spacing in the usage docs Copilot review of #869: the `index_prefix_domain_map` option line spelled "OpenSearch" as "Opensearch" and had a doubled space before the type. Both predate this branch but sit inside a hunk it rewrites. Also wrapped the line to match the continuation-indent style of every other option in the list, and corrected the same misspelling in the multi-tenant section a few lines above the paragraph this branch added. Co-Authored-By: Claude Opus 5 (1M context) * Require index_prefix_domain_map domain lists to hold strings Copilot review of #869: the shape check verified that each value was a list but not what the list contained, so `tenant_a: [42]` passed and then never compared equal to any domain the save path looks up -- the same silent-misbehavior class the check exists to reject, and a contradiction of the "any other shape is rejected at startup" claim in the docs. Check the list's items too, and reword the error message, comment, CHANGELOG and docs to state the rule the check now enforces: a mapping of tenant names to lists of domain names, all strings. Co-Authored-By: Claude Opus 5 (1M context) * Make the legacy fo migration retry-safe, and nest its mapping body Copilot review of #869, verified against live Elasticsearch 8.19 and OpenSearch 3 containers rather than mocks. Retry safety (a real defect): an attempt interrupted between creating the -v2 index and deleting the original left debris that made create() fail with "resource already exists" on every later startup, inside the same try that swallows the error -- so the index was never migrated and the debris document survived. Reproduced on a live cluster. Discard a leftover target first; that is safe precisely because reaching this point means the original still holds the data, since it is deleted only once the reindex has succeeded. Mapping body: the reviewer's concern that a dotted key under `properties` risks a runtime failure does not hold -- both clusters accept it and produce a byte-identical mapping, with the dot expanded into published_policy -> properties -> fo. Switch to the nested object form anyway, since dot expansion is conditional on the object's `subobjects` setting and this shape never is, and derive the object/leaf names from the dotted constant so the write cannot drift from the field the read looks up. Both fo-migration suites now build per-name Index mocks. A single shared mock cannot express "the original exists but its migration target does not", which is the ordinary case and the one the retry fix turns on; the happy path now also asserts the target index is never deleted. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- CHANGELOG.md | 7 + docs/source/elasticsearch.md | 24 +- docs/source/usage.md | 9 +- parsedmarc/cli.py | 270 +++++++++++++--- parsedmarc/constants.py | 2 +- parsedmarc/elastic.py | 123 +++++++- parsedmarc/opensearch.py | 131 ++++++-- tests/test_cli.py | 574 ++++++++++++++++++++++++++++++++++- tests/test_elastic.py | 235 ++++++++++++++ tests/test_opensearch.py | 200 +++++++++--- 10 files changed, 1438 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a9d15ce..b9ce15a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## 10.4.1 + +### Bug fixes + +- **The automatic `dkim_results_combined`/`spf_results_combined` and `policies_combined`/`failure_details_combined` backfills now cover the indexes named by `index_prefix_domain_map` and by a previous `index_suffix`** ([#868](https://github.com/domainaware/parsedmarc/issues/868)). The startup migration derived its Elasticsearch/OpenSearch index names from the `[elasticsearch]`/`[opensearch]` `index_prefix`/`index_suffix` options alone, while the save path honors `index_prefix_domain_map` as well. In a multi-tenant deployment whose per-tenant prefixes come from that map — with no `index_prefix` set, as the multi-tenant documentation instructs — the guard `count` therefore ran against `dmarc_aggregate*`/`smtp_tls*`, patterns that match none of the real `_dmarc_aggregate-*` indexes. Since the query passes `allow_no_indices=True`, a zero-match wildcard returns a count of 0, which is indistinguishable from "already backfilled", so the backfill was skipped **silently, with no log line at any level**, leaving the 10.4.0 dashboards' "DKIM details" table empty and every "SPF details" row labelled `none`. The migration now targets one index name per tenant prefix in the map — normalized by the same helper the save path uses, so the two cannot drift — plus the unprefixed name, since aggregate and failure reports for a domain that is absent from the map are still saved without a prefix and every report type may have indexes predating the map. A configured `index_prefix` still wins outright and suppresses the fan-out, matching save-time precedence, so such a deployment never submits an `_update_by_query` against an index pattern it does not write to. The `index_suffix` axis is widened the same way: the unsuffixed pattern is now targeted alongside the suffixed one, so documents indexed before the suffix was configured are backfilled too — note that on a shared cluster the unsuffixed pattern also matches other deployments' suffixes. The resolved target index names are logged at debug level, and a `SIGHUP` configuration reload re-resolves them, so a newly onboarded tenant is covered without a restart. Finally, an `index_prefix_domain_map` YAML file that is not a mapping of tenant names to *lists* of domain names, all strings, is now rejected at startup with a `ConfigurationError`. Every other shape failed silently rather than loudly: a non-string key raised mid-save, a scalar value matched the wrong domains (`in` on a `str` is a substring test, so `"example.co" in "example.com"` is `True`), and a non-string list item such as `tenant_a: [42]` simply never compared equal to any domain. +- **The legacy `published_policy.fo` index migration works again, and is back for Elasticsearch as well as OpenSearch.** parsedmarc releases before 5.0.0 declared that field as an integer, so their indexes mapped it as `long`, which cannot hold the multi-value `fo` settings reports carry (`0:1`, `d:s`); 5.0.0 both fixed the declaration and added a migration that rebuilds such an index as a `-v2` index with the text/keyword shape. That migration worked against the Elasticsearch 6 clusters of the day, but has been unable to complete since mapping types were removed from Elasticsearch 7 — and they never existed in OpenSearch — for two reasons: it read the field mapping in the old response shape that nests fields under a mapping type name, so the type check always fell through; and its `put_mapping()` call passed a `doc_type` argument that neither current client accepts, which would have raised `TypeError` if the check had ever passed. It now reads either response shape and uses each client's current `put_mapping()` signature. It is also retry-safe: an attempt interrupted between creating the `-v2` index and deleting the original used to leave debris that made every later startup fail on "resource already exists", so the index was never migrated; a leftover target is now discarded first, which is safe precisely because the original is only deleted once the reindex has succeeded. The whole migration is verified end to end against live Elasticsearch 8.19 and OpenSearch 3 clusters, including that recovery path. The index names it checks are also corrected: it takes exact names rather than wildcard patterns, because 5.0.0 introduced date-suffixed index names in the same release that fixed the declaration, so an affected index has no date component — but it *can* be prefixed or suffixed, since both options date back to 4.1.0, so the configured `index_prefix`/`index_suffix` are applied. As with the backfill, an `index_suffix` also gets the unsuffixed name checked, for history predating the suffix. `index_prefix_domain_map` prefixes are deliberately not applied, since that option arrived in 8.19.0, long after the mapping was fixed; this migration renames the index it rebuilds — documents are reindexed into the `-v2` index before the original is deleted, so none are lost, but the name changes — and so it must not be pointed at indexes belonging to a tenant it is not migrating. The Elasticsearch copy of the migration, removed as unreachable during the [#806](https://github.com/domainaware/parsedmarc/issues/806) client migration, is restored now that the cause is understood. Note that Elasticsearch 8 refuses to open an index created before 7.0, so an affected index reaches a supported cluster only by being carried forward through a reindex, which preserves the old mapping whenever the destination index is pre-created from it — the standard reindex procedure. + ## 10.4.0 ### New features diff --git a/docs/source/elasticsearch.md b/docs/source/elasticsearch.md index 2d0a1a43..ef95fae9 100644 --- a/docs/source/elasticsearch.md +++ b/docs/source/elasticsearch.md @@ -247,6 +247,25 @@ OpenSearch. Any error talking to the cluster (for example, no indexes yet on a fresh install) is logged as a warning and retried on the next startup, rather than aborting parsedmarc. +Which index patterns it targets follows the ones parsedmarc writes to, and +is logged at debug level on startup: + +- With `index_prefix_domain_map` configured in `[general]` and no + `index_prefix` set, every tenant prefix in the map gets its own index + pattern, alongside the unprefixed one — aggregate and failure reports for + a domain that is not in the map are still saved without a prefix. A + configuration reload (`SIGHUP`) re-reads the map, so a newly onboarded + tenant is covered without a restart. +- With an `index_prefix` set in `[elasticsearch]`/`[opensearch]`, only that + prefix is targeted, and the map is not consulted. That is deliberate: such + a deployment writes only under its own prefix, and an `_update_by_query` + against a pattern it does not write to could reach another deployment's + data on a shared cluster. +- With an `index_suffix` set, both the suffixed and the unsuffixed pattern + are targeted, so documents indexed before the suffix was configured are + backfilled too. Note that the unsuffixed pattern also matches any *other* + suffix on the same cluster. + If you upgrade the dashboards without pointing the new parsedmarc version at the cluster, or you'd rather control when the write load happens, you can still run the backfill manually. It is idempotent (documents that @@ -314,7 +333,10 @@ curl -X POST "http://localhost:9200/dmarc_aggregate*/_update_by_query?conflicts= `wait_for_completion=false` returns a task ID — check progress with `GET _tasks/`. Adjust the index pattern if you use a custom -`index_prefix`/`index_suffix`. After backfilling, re-import the updated +`index_prefix`/`index_suffix`; with `index_prefix_domain_map`, run the +command once per tenant prefix (`acme_corp_dmarc_aggregate*`) plus once for +the unprefixed pattern, or widen it to `*dmarc_aggregate*` to cover every +tenant in one pass. After backfilling, re-import the updated dashboards ndjson (the index pattern saved object changed too) per the import instructions above. diff --git a/docs/source/usage.md b/docs/source/usage.md index e8374578..774985b3 100644 --- a/docs/source/usage.md +++ b/docs/source/usage.md @@ -127,7 +127,8 @@ The full set of configuration options are: Elasticsearch, Splunk and/or S3 - `save_smtp_tls` - bool: Save SMTP-STS report data to Elasticsearch, Splunk and/or S3 - - `index_prefix_domain_map` - bool: A path mapping of Opensearch/Elasticsearch index prefixes to domain names + - `index_prefix_domain_map` - str: Path to a YAML file mapping + OpenSearch/Elasticsearch index prefixes to domain names - `strip_attachment_payloads` - bool: Remove attachment payloads from results - `silent` - bool: Set this to `False` to output results to STDOUT @@ -1167,12 +1168,16 @@ whalensolutions: Save it to disk where the user running ParseDMARC can read it, then set `index_prefix_domain_map` to that filepath in the `[general]` section of the ParseDMARC configuration file and do not set an `index_prefix` option in the `[elasticsearch]` or `[opensearch]` sections. -When configured correctly, if ParseDMARC finds that a report is related to a domain in the mapping, the report will be saved in an index name that has the tenant name prefixed to it with a trailing underscore. Then, you can use the security features of Opensearch or the ELK stack to only grant users access to the indexes that they need. +When configured correctly, if ParseDMARC finds that a report is related to a domain in the mapping, the report will be saved in an index name that has the tenant name prefixed to it with a trailing underscore. Then, you can use the security features of OpenSearch or the ELK stack to only grant users access to the indexes that they need. :::{note} A domain cannot be used in multiple tenant lists. Only the first prefix list that contains the matching domain is used. ::: +Each key must be a tenant name and each value a *list* of domain names, all strings; a file of any other shape is rejected at startup. + +The index migrations and backfills that run at startup cover every tenant prefix in the map, in addition to the unprefixed indexes that hold reports for domains the map does not list. A configuration reload (`SIGHUP`) re-reads the map, so a newly onboarded tenant is covered without restarting parsedmarc. See [Backfilling the combined DKIM/SPF result fields](elasticsearch.md#backfilling-the-combined-dkimspf-result-fields) for the details. + ## Running parsedmarc as a systemd service Use systemd to run `parsedmarc` as a service and process reports as diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index 6e8e8257..1d2d394f 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -663,6 +663,27 @@ def _parse_config(config: ConfigParser, opts): if "index_prefix_domain_map" in general_config: with open(_expand_path(general_config["index_prefix_domain_map"])) as f: index_prefix_domain_map = yaml.safe_load(f) + # An empty file loads as None, which means "unset". Anything else + # must be a mapping of tenant name to a list of domain names, all + # strings: the save path iterates the keys as index name prefixes + # and tests `get_base_domain(...).lower() in `. Every other + # shape fails silently rather than loudly -- a scalar value makes + # that an `in` on a str, which is a substring test, so it matches + # the wrong domains ("example.co" in "example.com" is True), and a + # non-string list item simply never compares equal to any domain. + if index_prefix_domain_map is not None and ( + not isinstance(index_prefix_domain_map, dict) + or not all( + isinstance(key, str) + and isinstance(value, list) + and all(isinstance(domain, str) for domain in value) + for key, value in index_prefix_domain_map.items() + ) + ): + raise ConfigurationError( + "index_prefix_domain_map must be a YAML mapping of tenant " + "name to a list of domain names, all strings" + ) if "offline" in general_config: opts.offline = bool(general_config.getboolean("offline")) if "strip_attachment_payloads" in general_config: @@ -1419,9 +1440,122 @@ class _OpenSearchHandle: pass -def _init_output_clients(opts): +def _normalize_index_prefix(prefix): + """Normalize an ``index_prefix_domain_map`` key into an index name prefix. + + Lowercases, strips surrounding whitespace and then surrounding + underscores, replaces the remaining spaces and hyphens with + underscores, and appends a trailing ``_``. + + Shared by the save path (``get_index_prefix()`` in :func:`_main`) and the + migration path (:func:`_migration_index_names`) so that the indexes + parsedmarc migrates cannot drift from the ones it writes to. + + A key that normalizes to the empty string (``"_"``, ``" "``) yields the + literal ``"_"``. That is deliberate: it is exactly the prefix the save + path produces for such a key, so it is the prefix its documents live + under. + + The ``[elasticsearch]``/``[opensearch]`` ``index_prefix`` option is never + passed through this function -- the save path uses that option verbatim, + so the migration path must too. + + Args: + prefix (str): A key from ``index_prefix_domain_map``. + + Returns: + str: The index name prefix, including its trailing underscore. + """ + prefix = prefix.lower().strip().strip("_").replace(" ", "_").replace("-", "_") + return f"{prefix}_" + + +def _migration_index_names( + base_name, index_suffix, configured_prefix, index_prefix_domain_map +): + """Resolve every index name an index migration should target. + + Mirrors the way save time builds index names, which is + ``{prefix}{base_name}_{index_suffix}-{date}``, and widens each of the + two configurable axes so that a migration cannot silently skip indexes + the deployment holds data in (issue #868). + + **Suffix axis.** When ``index_suffix`` is set, the suffixed name (what + this deployment writes today) and the bare ``base_name`` are both + returned, suffixed first. The bare name covers + documents indexed before the suffix was configured, or under a previous + one; without it, ``dmarc_aggregate_prod*`` matches none of the + operator's own ``dmarc_aggregate-*`` indexes. Because callers turn each + name into an ``f"{name}*"`` pattern, the bare name's pattern is a strict + superset of the suffixed one: on a shared cluster it also matches other + deployments' suffixes, and on the first run after an upgrade both + patterns can submit an overlapping ``update_by_query``. That is safe -- + the backfill scripts only set a field that is missing, and submissions + use ``conflicts="proceed"`` -- but it is a deliberate trade of + narrowness for coverage of the operator's own history. + + **Prefix axis.** A configured ``index_prefix`` wins outright and + suppresses the ``index_prefix_domain_map`` fan-out, because such a + deployment writes only under that prefix and must not touch index + patterns it does not own. Truthiness decides, matching the save path's + ``opts.*_index_prefix or get_index_prefix(report)``. Otherwise the + unprefixed name comes first, followed by one name per map key -- + normalized by :func:`_normalize_index_prefix`, in map order. The + unprefixed name has to stay: aggregate and failure reports for a domain + that is absent from the map are still saved without a prefix, and + indexes predating the map exist for every report type. + + ``configured_prefix`` is used verbatim, never normalized, for parity + with the save path. + + Args: + base_name (str): The unprefixed, unsuffixed index name, e.g. + ``"dmarc_aggregate"``. + index_suffix (str | None): The configured ``index_suffix``, or + ``None``/``""`` when none is configured. + configured_prefix (str | None): The configured ``index_prefix``, or + ``None``/``""`` when none is configured. + index_prefix_domain_map (dict | None): The parsed + ``general.index_prefix_domain_map``, or ``None`` when + multi-tenant prefixing is not configured. + + Returns: + list: Index names, deduplicated, in first-seen order. With nothing + configured this is just ``[base_name]``. + """ + bases = [base_name] + if index_suffix: + bases.insert(0, f"{base_name}_{index_suffix}") + + if configured_prefix: + prefixes = [configured_prefix] + else: + prefixes = [""] + for key in index_prefix_domain_map or {}: + prefix = _normalize_index_prefix(key) + if prefix not in prefixes: + prefixes.append(prefix) + + names = [] + for prefix in prefixes: + for base in bases: + name = f"{prefix}{base}" + if name not in names: + names.append(name) + return names + + +def _init_output_clients(opts, index_prefix_domain_map=None): """Create output clients based on current opts. + Args: + opts: Namespace of parsed configuration values. + index_prefix_domain_map (dict | None): The parsed + ``general.index_prefix_domain_map``. ``None`` -- the default -- + means multi-tenant prefixing is not configured, so Elasticsearch + and OpenSearch index migrations target only the names derived + from ``index_prefix``/``index_suffix``. + Returns: dict of client instances keyed by name. @@ -1563,19 +1697,34 @@ def _init_output_clients(opts): opts.elasticsearch_hosts, opts.elasticsearch_ssl, ) - es_aggregate_index = "dmarc_aggregate" - es_failure_index = "dmarc_failure" - es_smtp_tls_index = "smtp_tls" - if opts.elasticsearch_index_suffix: - suffix = opts.elasticsearch_index_suffix - es_aggregate_index = f"{es_aggregate_index}_{suffix}" - es_failure_index = f"{es_failure_index}_{suffix}" - es_smtp_tls_index = f"{es_smtp_tls_index}_{suffix}" - if opts.elasticsearch_index_prefix: - prefix = opts.elasticsearch_index_prefix - es_aggregate_index = f"{prefix}{es_aggregate_index}" - es_failure_index = f"{prefix}{es_failure_index}" - es_smtp_tls_index = f"{prefix}{es_smtp_tls_index}" + es_aggregate_indexes = _migration_index_names( + "dmarc_aggregate", + opts.elasticsearch_index_suffix, + opts.elasticsearch_index_prefix, + index_prefix_domain_map, + ) + es_failure_indexes = _migration_index_names( + "dmarc_failure", + opts.elasticsearch_index_suffix, + opts.elasticsearch_index_prefix, + index_prefix_domain_map, + ) + es_smtp_tls_indexes = _migration_index_names( + "smtp_tls", + opts.elasticsearch_index_suffix, + opts.elasticsearch_index_prefix, + index_prefix_domain_map, + ) + # The legacy published_policy.fo migration gets the same + # names minus the tenant fan-out: index_prefix_domain_map + # arrived in 8.19.0, long after 5.0.0 fixed the mapping, so + # no index it names can carry the old one. + es_legacy_fo_indexes = _migration_index_names( + "dmarc_aggregate", + opts.elasticsearch_index_suffix, + opts.elasticsearch_index_prefix, + None, + ) elastic_timeout_value = ( float(opts.elasticsearch_timeout) if opts.elasticsearch_timeout is not None @@ -1592,10 +1741,19 @@ def _init_output_clients(opts): timeout=elastic_timeout_value, serverless=opts.elasticsearch_serverless, ) + logger.debug( + "Elasticsearch index migration targets: aggregate=%s, " + "failure=%s, smtp_tls=%s, legacy_fo=%s", + es_aggregate_indexes, + es_failure_indexes, + es_smtp_tls_indexes, + es_legacy_fo_indexes, + ) elastic.migrate_indexes( - aggregate_indexes=[es_aggregate_index], - failure_indexes=[es_failure_index], - smtp_tls_indexes=[es_smtp_tls_index], + aggregate_indexes=es_aggregate_indexes, + failure_indexes=es_failure_indexes, + smtp_tls_indexes=es_smtp_tls_indexes, + legacy_fo_indexes=es_legacy_fo_indexes, ) clients["elasticsearch"] = _ElasticsearchHandle() except Exception as e: @@ -1608,19 +1766,34 @@ def _init_output_clients(opts): opts.opensearch_hosts, opts.opensearch_ssl, ) - os_aggregate_index = "dmarc_aggregate" - os_failure_index = "dmarc_failure" - os_smtp_tls_index = "smtp_tls" - if opts.opensearch_index_suffix: - suffix = opts.opensearch_index_suffix - os_aggregate_index = f"{os_aggregate_index}_{suffix}" - os_failure_index = f"{os_failure_index}_{suffix}" - os_smtp_tls_index = f"{os_smtp_tls_index}_{suffix}" - if opts.opensearch_index_prefix: - prefix = opts.opensearch_index_prefix - os_aggregate_index = f"{prefix}{os_aggregate_index}" - os_failure_index = f"{prefix}{os_failure_index}" - os_smtp_tls_index = f"{prefix}{os_smtp_tls_index}" + os_aggregate_indexes = _migration_index_names( + "dmarc_aggregate", + opts.opensearch_index_suffix, + opts.opensearch_index_prefix, + index_prefix_domain_map, + ) + os_failure_indexes = _migration_index_names( + "dmarc_failure", + opts.opensearch_index_suffix, + opts.opensearch_index_prefix, + index_prefix_domain_map, + ) + os_smtp_tls_indexes = _migration_index_names( + "smtp_tls", + opts.opensearch_index_suffix, + opts.opensearch_index_prefix, + index_prefix_domain_map, + ) + # The legacy published_policy.fo migration gets the same + # names minus the tenant fan-out: index_prefix_domain_map + # arrived in 8.19.0, long after 5.0.0 fixed the mapping, so + # no index it names can carry the old one. + os_legacy_fo_indexes = _migration_index_names( + "dmarc_aggregate", + opts.opensearch_index_suffix, + opts.opensearch_index_prefix, + None, + ) opensearch_timeout_value = ( float(opts.opensearch_timeout) if opts.opensearch_timeout is not None @@ -1639,10 +1812,19 @@ def _init_output_clients(opts): aws_region=opts.opensearch_aws_region, aws_service=opts.opensearch_aws_service, ) + logger.debug( + "OpenSearch index migration targets: aggregate=%s, " + "failure=%s, smtp_tls=%s, legacy_fo=%s", + os_aggregate_indexes, + os_failure_indexes, + os_smtp_tls_indexes, + os_legacy_fo_indexes, + ) opensearch.migrate_indexes( - aggregate_indexes=[os_aggregate_index], - failure_indexes=[os_failure_index], - smtp_tls_indexes=[os_smtp_tls_index], + aggregate_indexes=os_aggregate_indexes, + failure_indexes=os_failure_indexes, + smtp_tls_indexes=os_smtp_tls_indexes, + legacy_fo_indexes=os_legacy_fo_indexes, ) clients["opensearch"] = _OpenSearchHandle() except Exception as e: @@ -1728,17 +1910,9 @@ def _main(): domain = get_base_domain(domain) if domain: domain = domain.lower() - for prefix in index_prefix_domain_map: - if domain in index_prefix_domain_map[prefix]: - prefix = ( - prefix.lower() - .strip() - .strip("_") - .replace(" ", "_") - .replace("-", "_") - ) - prefix = f"{prefix}_" - return prefix + for key in index_prefix_domain_map: + if domain in index_prefix_domain_map[key]: + return _normalize_index_prefix(key) return None def filter_smtp_tls_reports_for_index_prefix(tls_reports): @@ -2506,7 +2680,9 @@ def _main(): retry_delay = 5 for attempt in range(max_retries + 1): try: - clients = _init_output_clients(opts) + clients = _init_output_clients( + opts, index_prefix_domain_map=index_prefix_domain_map + ) break except ConfigurationError as e: logger.critical(str(e)) @@ -3060,7 +3236,9 @@ def _main(): new_opts = Namespace(**vars(opts_from_cli)) new_config = _load_config(config_file) new_index_prefix_domain_map = _parse_config(new_config, new_opts) - new_clients = _init_output_clients(new_opts) + new_clients = _init_output_clients( + new_opts, index_prefix_domain_map=new_index_prefix_domain_map + ) # All steps succeeded — commit the changes atomically. _close_output_clients(clients) diff --git a/parsedmarc/constants.py b/parsedmarc/constants.py index 509c7fb0..d8f7ed43 100644 --- a/parsedmarc/constants.py +++ b/parsedmarc/constants.py @@ -1,4 +1,4 @@ -__version__ = "10.4.0" +__version__ = "10.4.1" USER_AGENT = f"parsedmarc/{__version__}" diff --git a/parsedmarc/elastic.py b/parsedmarc/elastic.py index ba8bf3d7..64865f41 100644 --- a/parsedmarc/elastic.py +++ b/parsedmarc/elastic.py @@ -20,6 +20,7 @@ from elasticsearch.dsl import ( Text, connections, ) +from elasticsearch.helpers import reindex from parsedmarc import InvalidFailureReport from parsedmarc.log import logger @@ -677,16 +678,65 @@ def create_indexes(names: list[str], settings: dict[str, Any] | None = None): raise ElasticsearchError(f"Elasticsearch error: {e.__str__()}") +_LEGACY_FO_FIELD = "published_policy.fo" +# The same field split into the object/leaf names a mapping body nests it +# under. Derived from the dotted name so the mapping written below cannot +# drift from the field _legacy_fo_field_type() reads. +_LEGACY_FO_OBJECT, _LEGACY_FO_LEAF = _LEGACY_FO_FIELD.split(".") + + +def _legacy_fo_field_type(index: Index) -> str | None: + """Return the mapped type of ``published_policy.fo`` in *index*. + + Returns ``None`` when the index does not map the field at all. + + Elasticsearch 6-era clusters keyed field mappings by the mapping type + name (``doc``); mapping types are gone from Elasticsearch 8, whose + responses put the field directly under ``mappings``. The type-keyed + shape is only descended into when such a key is actually present, so + this reads either shape. + + Args: + index (Index): The index to inspect. + + Returns: + str | None: The mapped field type, e.g. ``"long"`` or ``"text"``. + """ + response = index.get_field_mapping(fields=[_LEGACY_FO_FIELD]) + mappings = response[list(response.keys())[0]]["mappings"] + if _LEGACY_FO_FIELD not in mappings: + for type_name in ("doc", "_doc"): + if type_name in mappings: + mappings = mappings[type_name] + break + field_mapping = mappings.get(_LEGACY_FO_FIELD) + if not field_mapping: + return None + return field_mapping.get("mapping", {}).get(_LEGACY_FO_LEAF, {}).get("type") + + def migrate_indexes( aggregate_indexes: list[str] | None = None, failure_indexes: list[str] | None = None, smtp_tls_indexes: list[str] | None = None, + legacy_fo_indexes: list[str] | None = None, ): """ - Backfills the ``dkim_results_combined``/``spf_results_combined`` fields - (added for issue #169) on aggregate report documents, and the - ``policies_combined``/``failure_details_combined`` fields on SMTP TLS - report documents, that were saved before those fields existed. + Runs index migrations and backfills. + + First, the legacy ``published_policy.fo`` migration, for each name in + ``legacy_fo_indexes``: parsedmarc releases before 5.0.0 declared that + field as an integer, so those indexes mapped it as ``long``, which + cannot hold the multi-value ``fo`` settings reports carry (``0:1``, + ``d:s``). Such an index is rebuilt as a ``-v2`` index with the + text/keyword shape, the documents are reindexed into it, and the + original is deleted. + + Second, it backfills the ``dkim_results_combined``/ + ``spf_results_combined`` fields (added for issue #169) on aggregate + report documents, and the ``policies_combined``/ + ``failure_details_combined`` fields on SMTP TLS report documents, that + were saved before those fields existed. For each name in ``aggregate_indexes``/``smtp_tls_indexes``, this submits an ``update_by_query`` against the ``f"{name}*"`` index pattern @@ -706,7 +756,72 @@ def migrate_indexes( failure_indexes (list): A list of failure index names (accepted for API compatibility; unused) smtp_tls_indexes (list): A list of SMTP TLS index names + legacy_fo_indexes (list): A list of index names to check for the + pre-5.0.0 ``published_policy.fo`` ``long`` mapping. Unlike the + backfill arguments these are exact names, not patterns: + 5.0.0 introduced date-suffixed index names in the same release + that fixed the mapping, so an affected index has no date + component. It may still be prefixed or suffixed -- both + options date back to 4.1.0 -- so callers should pass the + names their own ``index_prefix``/``index_suffix`` + configuration produces. """ + if not aggregate_indexes and not smtp_tls_indexes and not legacy_fo_indexes: + return + + version = 2 + for legacy_index_name in legacy_fo_indexes or []: + try: + legacy_index = Index(legacy_index_name) + if not legacy_index.exists(): + continue + if _legacy_fo_field_type(legacy_index) != "long": + continue + new_index_name = f"{legacy_index_name}-v{version}" + # Nested object form rather than the dotted key this used to + # send. Both are accepted and produce an identical mapping + # (verified against Elasticsearch 8.19 and OpenSearch 3), but + # dot expansion is conditional on the object field's + # `subobjects` setting, and this shape never is. + properties = { + _LEGACY_FO_OBJECT: { + "properties": { + _LEGACY_FO_LEAF: { + "type": "text", + "fields": { + "keyword": {"type": "keyword", "ignore_above": 256} + }, + } + } + } + } + logger.info( + f"Migrating {legacy_index_name} to {new_index_name}: " + f"{_LEGACY_FO_FIELD} is mapped as long, as parsedmarc " + "releases before 5.0.0 declared it" + ) + # Reaching here means the original index still holds the data: + # it is deleted only after the reindex below succeeds. So a + # leftover target index is the debris of an earlier attempt + # that died between create() and delete(), and keeping it would + # fail every later attempt on "resource already exists". + if Index(new_index_name).exists(): + logger.warning( + f"Discarding {new_index_name} left behind by an earlier " + f"interrupted migration of {legacy_index_name}" + ) + Index(new_index_name).delete() + Index(new_index_name).create() + Index(new_index_name).put_mapping(properties=properties) + reindex(connections.get_connection(), legacy_index_name, new_index_name) + Index(legacy_index_name).delete() + except Exception as e: + logger.warning( + "Failed the legacy published_policy.fo migration for " + f"{legacy_index_name}: {e}. This will be retried at the " + "next startup." + ) + if not aggregate_indexes and not smtp_tls_indexes: return diff --git a/parsedmarc/opensearch.py b/parsedmarc/opensearch.py index c92448a5..41ba5ab9 100644 --- a/parsedmarc/opensearch.py +++ b/parsedmarc/opensearch.py @@ -599,18 +599,59 @@ def create_indexes(names: list[str], settings: dict[str, Any] | None = None): raise OpenSearchError(f"OpenSearch error: {e.__str__()}") +_LEGACY_FO_FIELD = "published_policy.fo" +# The same field split into the object/leaf names a mapping body nests it +# under. Derived from the dotted name so the mapping written below cannot +# drift from the field _legacy_fo_field_type() reads. +_LEGACY_FO_OBJECT, _LEGACY_FO_LEAF = _LEGACY_FO_FIELD.split(".") + + +def _legacy_fo_field_type(index: Index) -> str | None: + """Return the mapped type of ``published_policy.fo`` in *index*. + + Returns ``None`` when the index does not map the field at all. + + Elasticsearch 6-era clusters keyed field mappings by the mapping type + name (``doc``); mapping types are gone from both OpenSearch and + Elasticsearch 8, whose responses put the field directly under + ``mappings``. The type-keyed shape is only descended into when such a + key is actually present, so this reads either shape. + + Args: + index (Index): The index to inspect. + + Returns: + str | None: The mapped field type, e.g. ``"long"`` or ``"text"``. + """ + response = index.get_field_mapping(fields=[_LEGACY_FO_FIELD]) + mappings = response[list(response.keys())[0]]["mappings"] + if _LEGACY_FO_FIELD not in mappings: + for type_name in ("doc", "_doc"): + if type_name in mappings: + mappings = mappings[type_name] + break + field_mapping = mappings.get(_LEGACY_FO_FIELD) + if not field_mapping: + return None + return field_mapping.get("mapping", {}).get(_LEGACY_FO_LEAF, {}).get("type") + + def migrate_indexes( aggregate_indexes: list[str] | None = None, failure_indexes: list[str] | None = None, smtp_tls_indexes: list[str] | None = None, + legacy_fo_indexes: list[str] | None = None, ): """ Runs index migrations and backfills. - First, the legacy ``published_policy.fo`` migration: indexes where that - field was mapped as ``long`` (data indexed by very old parsedmarc - releases under the Elasticsearch 6-era ``doc`` mapping type) are rebuilt - as a ``-v2`` index with the text/keyword shape. + First, the legacy ``published_policy.fo`` migration, for each name in + ``legacy_fo_indexes``: parsedmarc releases before 5.0.0 declared that + field as an integer, so those indexes mapped it as ``long``, which + cannot hold the multi-value ``fo`` settings reports carry (``0:1``, + ``d:s``). Such an index is rebuilt as a ``-v2`` index with the + text/keyword shape, the documents are reindexed into it, and the + original is deleted. Second, the ``dkim_results_combined``/``spf_results_combined`` backfill (added for issue #169) for aggregate report documents that were saved @@ -637,51 +678,77 @@ def migrate_indexes( (accepted for API compatibility; no migrations are currently needed for failure indexes) smtp_tls_indexes (list): A list of SMTP TLS index names + legacy_fo_indexes (list): A list of index names to check for the + pre-5.0.0 ``published_policy.fo`` ``long`` mapping. Unlike the + backfill arguments these are exact names, not patterns: + 5.0.0 introduced date-suffixed index names in the same release + that fixed the mapping, so an affected index has no date + component. It may still be prefixed or suffixed -- both + options date back to 4.1.0 -- so callers should pass the + names their own ``index_prefix``/``index_suffix`` + configuration produces. """ - if not aggregate_indexes and not smtp_tls_indexes: + if not aggregate_indexes and not smtp_tls_indexes and not legacy_fo_indexes: return version = 2 - for aggregate_index_name in aggregate_indexes or []: + for legacy_index_name in legacy_fo_indexes or []: try: - if not Index(aggregate_index_name).exists(): + legacy_index = Index(legacy_index_name) + if not legacy_index.exists(): continue - aggregate_index = Index(aggregate_index_name) - doc = "doc" - fo_field = "published_policy.fo" - fo = "fo" - fo_mapping = aggregate_index.get_field_mapping(fields=[fo_field]) - fo_mapping = fo_mapping[list(fo_mapping.keys())[0]]["mappings"] - if doc not in fo_mapping: + if _legacy_fo_field_type(legacy_index) != "long": continue - - fo_mapping = fo_mapping[doc][fo_field]["mapping"][fo] - fo_type = fo_mapping["type"] - if fo_type == "long": - new_index_name = f"{aggregate_index_name}-v{version}" - body = { - "properties": { - "published_policy.fo": { - "type": "text", - "fields": { - "keyword": {"type": "keyword", "ignore_above": 256} - }, + new_index_name = f"{legacy_index_name}-v{version}" + # Nested object form rather than the dotted key this used to + # send. Both are accepted and produce an identical mapping + # (verified against Elasticsearch 8.19 and OpenSearch 3), but + # dot expansion is conditional on the object field's + # `subobjects` setting, and this shape never is. + body = { + "properties": { + _LEGACY_FO_OBJECT: { + "properties": { + _LEGACY_FO_LEAF: { + "type": "text", + "fields": { + "keyword": {"type": "keyword", "ignore_above": 256} + }, + } } } } - Index(new_index_name).create() - Index(new_index_name).put_mapping(doc_type=doc, body=body) - reindex( - connections.get_connection(), aggregate_index_name, new_index_name + } + logger.info( + f"Migrating {legacy_index_name} to {new_index_name}: " + f"{_LEGACY_FO_FIELD} is mapped as long, as parsedmarc " + "releases before 5.0.0 declared it" + ) + # Reaching here means the original index still holds the data: + # it is deleted only after the reindex below succeeds. So a + # leftover target index is the debris of an earlier attempt + # that died between create() and delete(), and keeping it would + # fail every later attempt on "resource already exists". + if Index(new_index_name).exists(): + logger.warning( + f"Discarding {new_index_name} left behind by an earlier " + f"interrupted migration of {legacy_index_name}" ) - Index(aggregate_index_name).delete() + Index(new_index_name).delete() + Index(new_index_name).create() + Index(new_index_name).put_mapping(body=body) + reindex(connections.get_connection(), legacy_index_name, new_index_name) + Index(legacy_index_name).delete() except Exception as e: logger.warning( "Failed the legacy published_policy.fo migration for " - f"{aggregate_index_name}: {e}. This will be retried at the " + f"{legacy_index_name}: {e}. This will be retried at the " "next startup." ) + if not aggregate_indexes and not smtp_tls_indexes: + return + try: client = connections.get_connection() except Exception as e: diff --git a/tests/test_cli.py b/tests/test_cli.py index 680ff4b1..e1b406c2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3368,6 +3368,83 @@ watch = true # Old clients should NOT have been closed (reload failed before swap) initial_clients["s3_client"].close.assert_not_called() + @unittest.skipUnless( + hasattr(signal, "SIGHUP"), + "SIGHUP not available on this platform", + ) + @patch("parsedmarc.cli._init_output_clients") + @patch("parsedmarc.cli._parse_config") + @patch("parsedmarc.cli._load_config") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.watch_inbox") + @patch("parsedmarc.cli.IMAPConnection") + def testReloadPassesFreshDomainMapToOutputClients( + self, + mock_imap, + mock_watch, + mock_get_reports, + mock_load_config, + mock_parse_config, + mock_init_clients, + ): + """The index migrations resolve their target index names from + index_prefix_domain_map, so a reload has to hand the *reloaded* map + to _init_output_clients() -- otherwise a tenant onboarded into the + map is not covered until the process restarts (issue #868).""" + import signal as signal_module + + mock_imap.return_value = object() + mock_get_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + mock_load_config.return_value = ConfigParser() + + first_map = {"tenant_a": ["example.com"]} + second_map = {"tenant_a": ["example.com"], "tenant_b": ["example.net"]} + maps = [first_map, second_map] + + def parse_side_effect(config, opts): + opts.imap_host = "imap.example.com" + opts.imap_user = "user" + opts.imap_password = "pass" + opts.mailbox_watch = True + return maps.pop(0) if maps else second_map + + mock_parse_config.side_effect = parse_side_effect + mock_init_clients.return_value = {} + + watch_calls = [0] + + def watch_side_effect(*args, **kwargs): + watch_calls[0] += 1 + if watch_calls[0] == 1: + os.kill(os.getpid(), signal_module.SIGHUP) + return + raise FileExistsError("stop-watch-loop") + + mock_watch.side_effect = watch_side_effect + + with tempfile.NamedTemporaryFile("w", suffix=".ini", delete=False) as cfg: + cfg.write(self._BASE_CONFIG) + cfg_path = cfg.name + self.addCleanup(lambda: os.path.exists(cfg_path) and os.remove(cfg_path)) + + with patch.object(sys, "argv", ["parsedmarc", "-c", cfg_path]): + with self.assertRaises(SystemExit): + parsedmarc.cli._main() + + self.assertEqual(mock_init_clients.call_count, 2) + self.assertIs( + mock_init_clients.call_args_list[0].kwargs["index_prefix_domain_map"], + first_map, + ) + self.assertIs( + mock_init_clients.call_args_list[1].kwargs["index_prefix_domain_map"], + second_map, + ) + @unittest.skipUnless( hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform", @@ -3412,7 +3489,7 @@ watch = true new_client = MagicMock() init_call = [0] - def init_side_effect(opts): + def init_side_effect(opts, index_prefix_domain_map=None): init_call[0] += 1 if init_call[0] == 1: return {"kafka_client": old_client} @@ -3509,7 +3586,7 @@ watch = true # Capture opts used on each _init_output_clients call init_opts_captures = [] - def init_side_effect(opts): + def init_side_effect(opts, index_prefix_domain_map=None): from argparse import Namespace as NS init_opts_captures.append(NS(**vars(opts))) @@ -4187,6 +4264,427 @@ to = admin@example.com self.assertEqual(report_ids, {"allowed-1", "mixed-case-1"}) +class TestNormalizeIndexPrefix(unittest.TestCase): + """_normalize_index_prefix() is the single definition of how an + index_prefix_domain_map key becomes an index name prefix. The save path + and the migration path both call it, so these assertions pin the exact + strings both sides must agree on.""" + + def test_lowercases_and_strips(self): + self.assertEqual(parsedmarc.cli._normalize_index_prefix(" Acme "), "acme_") + + def test_replaces_spaces_and_hyphens(self): + self.assertEqual( + parsedmarc.cli._normalize_index_prefix("Acme Corp-2"), "acme_corp_2_" + ) + + def test_strips_surrounding_underscores(self): + self.assertEqual(parsedmarc.cli._normalize_index_prefix("_tenant_"), "tenant_") + + def test_key_that_normalizes_to_nothing_yields_a_bare_underscore(self): + """Documents rather than special-cases the degenerate result: the + save path writes such a report's documents to `_dmarc_aggregate-*`, + so the migration path has to target the same name.""" + self.assertEqual(parsedmarc.cli._normalize_index_prefix("_"), "_") + self.assertEqual(parsedmarc.cli._normalize_index_prefix(" "), "_") + + +class TestMigrationIndexNames(unittest.TestCase): + """_migration_index_names() resolves every index name an index + migration should target, widening both configurable axes (issue #868). + Each case asserts the whole list, in order, so it observes the names + that must be absent as well as the ones that must be present.""" + + def test_nothing_configured_is_unchanged(self): + self.assertEqual( + parsedmarc.cli._migration_index_names("dmarc_aggregate", None, None, None), + ["dmarc_aggregate"], + ) + self.assertEqual( + parsedmarc.cli._migration_index_names("dmarc_aggregate", None, None, {}), + ["dmarc_aggregate"], + ) + + def test_suffix_also_targets_the_unsuffixed_name(self): + """`dmarc_aggregate_prod*` matches none of the operator's own + pre-suffix `dmarc_aggregate-*` indexes, so both are targeted.""" + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", "prod", None, None + ), + ["dmarc_aggregate_prod", "dmarc_aggregate"], + ) + + def test_empty_suffix_adds_no_duplicate(self): + self.assertEqual( + parsedmarc.cli._migration_index_names("dmarc_aggregate", "", None, None), + ["dmarc_aggregate"], + ) + + def test_domain_map_fans_out_after_the_unprefixed_name(self): + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", + None, + None, + {"Acme Corp": ["acme.example"], "widgets-inc": ["widgets.example"]}, + ), + [ + "dmarc_aggregate", + "acme_corp_dmarc_aggregate", + "widgets_inc_dmarc_aggregate", + ], + ) + + def test_keys_that_normalize_identically_are_deduplicated(self): + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", + None, + None, + {"acme corp": ["a.example"], "Acme-Corp": ["b.example"]}, + ), + ["dmarc_aggregate", "acme_corp_dmarc_aggregate"], + ) + + def test_configured_prefix_suppresses_the_domain_map_fan_out(self): + """A deployment with its own index_prefix writes only under that + prefix; submitting an _update_by_query against map-derived patterns + would touch another deployment's data on a shared cluster. The + suffix axis still applies.""" + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", "prod", "corp_", {"acme": ["acme.example"]} + ), + ["corp_dmarc_aggregate_prod", "corp_dmarc_aggregate"], + ) + + def test_configured_prefix_is_used_verbatim(self): + """The save path passes index_prefix through untouched, so the + migration path must not normalize it either.""" + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", None, "My-Prefix ", None + ), + ["My-Prefix dmarc_aggregate"], + ) + + def test_both_axes_compose_in_save_time_order(self): + """Save time builds `{prefix}{base}_{suffix}-{date}`: the prefix + precedes the base name and the suffix follows it.""" + self.assertEqual( + parsedmarc.cli._migration_index_names( + "smtp_tls", "prod", None, {"acme": ["acme.example"]} + ), + ["smtp_tls_prod", "smtp_tls", "acme_smtp_tls_prod", "acme_smtp_tls"], + ) + + def test_key_normalizing_to_an_underscore_is_targeted(self): + self.assertEqual( + parsedmarc.cli._migration_index_names( + "dmarc_aggregate", None, None, {"_": ["acme.example"]} + ), + ["dmarc_aggregate", "_dmarc_aggregate"], + ) + + +class TestMigrateIndexesDomainMapWiring(unittest.TestCase): + """Issue #868: the startup index migrations built their index names + from index_prefix/index_suffix alone, so a multi-tenant deployment + whose prefixes come from index_prefix_domain_map had its backfill + guard run against `dmarc_aggregate*`, which matches none of its real + `_dmarc_aggregate-*` indexes -- a silent no-op. These tests + assert the index names actually handed to migrate_indexes().""" + + def _write_config(self, config, domain_map=None): + paths = {} + if domain_map is not None: + import yaml + + with NamedTemporaryFile("w", suffix=".yaml", delete=False) as map_file: + yaml.dump(domain_map, map_file) + paths["map"] = map_file.name + self.addCleanup( + lambda: os.path.exists(paths["map"]) and os.remove(paths["map"]) + ) + config = config.format(map_path=paths["map"]) + with NamedTemporaryFile("w", suffix=".ini", delete=False) as config_file: + config_file.write(config) + paths["config"] = config_file.name + self.addCleanup( + lambda: os.path.exists(paths["config"]) and os.remove(paths["config"]) + ) + return paths["config"] + + _IMAP = """ +[imap] +host = imap.example.com +user = test-user +password = test-password +""" + + @patch("parsedmarc.cli.elastic.migrate_indexes") + @patch("parsedmarc.cli.elastic.set_hosts") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testElasticsearchMigrationFansOutOverDomainMap( + self, + mock_imap_connection, + mock_get_reports, + _mock_set_hosts, + mock_migrate, + ): + mock_imap_connection.return_value = object() + mock_get_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_path = self._write_config( + """[general] +save_aggregate = true +save_smtp_tls = true +silent = true +index_prefix_domain_map = {map_path} +""" + + self._IMAP + + """ +[elasticsearch] +hosts = localhost +""", + domain_map={"Tenant-A": ["example.com"], "tenant_b": ["example.net"]}, + ) + + with patch.object(sys, "argv", ["parsedmarc", "-c", config_path]): + parsedmarc.cli._main() + + kwargs = mock_migrate.call_args.kwargs + self.assertEqual( + kwargs["aggregate_indexes"], + [ + "dmarc_aggregate", + "tenant_a_dmarc_aggregate", + "tenant_b_dmarc_aggregate", + ], + ) + self.assertEqual( + kwargs["failure_indexes"], + ["dmarc_failure", "tenant_a_dmarc_failure", "tenant_b_dmarc_failure"], + ) + self.assertEqual( + kwargs["smtp_tls_indexes"], + ["smtp_tls", "tenant_a_smtp_tls", "tenant_b_smtp_tls"], + ) + # The legacy fo migration does not fan out over the map: it + # deletes the index it reindexes, and no index it could apply to + # can carry a tenant prefix. + self.assertEqual(kwargs["legacy_fo_indexes"], ["dmarc_aggregate"]) + + @patch("parsedmarc.cli.opensearch.migrate_indexes") + @patch("parsedmarc.cli.opensearch.set_hosts") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testOpenSearchMigrationFansOutWithSuffix( + self, + mock_imap_connection, + mock_get_reports, + _mock_set_hosts, + mock_migrate, + ): + """The OpenSearch block carries its own copy of the fan-out, so it + is asserted separately. With an index_suffix set, the unsuffixed + name is targeted too, for documents indexed before the suffix was + configured.""" + mock_imap_connection.return_value = object() + mock_get_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_path = self._write_config( + """[general] +save_aggregate = true +silent = true +index_prefix_domain_map = {map_path} +""" + + self._IMAP + + """ +[opensearch] +hosts = localhost +index_suffix = prod +""", + domain_map={"Tenant-A": ["example.com"]}, + ) + + with patch.object(sys, "argv", ["parsedmarc", "-c", config_path]): + parsedmarc.cli._main() + + kwargs = mock_migrate.call_args.kwargs + self.assertEqual( + kwargs["aggregate_indexes"], + [ + "dmarc_aggregate_prod", + "dmarc_aggregate", + "tenant_a_dmarc_aggregate_prod", + "tenant_a_dmarc_aggregate", + ], + ) + self.assertEqual( + kwargs["smtp_tls_indexes"], + [ + "smtp_tls_prod", + "smtp_tls", + "tenant_a_smtp_tls_prod", + "tenant_a_smtp_tls", + ], + ) + + @patch("parsedmarc.cli.elastic.migrate_indexes") + @patch("parsedmarc.cli.elastic.set_hosts") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testConfiguredPrefixSuppressesDomainMapFanout( + self, + mock_imap_connection, + mock_get_reports, + _mock_set_hosts, + mock_migrate, + ): + """The negative half of the fan-out contract, and the one case here + that also passed before #868 was fixed: a deployment that sets its + own index_prefix must never submit an _update_by_query against a + map-derived index pattern it does not write to.""" + mock_imap_connection.return_value = object() + mock_get_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_path = self._write_config( + """[general] +save_aggregate = true +silent = true +index_prefix_domain_map = {map_path} +""" + + self._IMAP + + """ +[elasticsearch] +hosts = localhost +index_prefix = corp_ +""", + domain_map={"Tenant-A": ["example.com"]}, + ) + + with patch.object(sys, "argv", ["parsedmarc", "-c", config_path]): + parsedmarc.cli._main() + + kwargs = mock_migrate.call_args.kwargs + self.assertEqual(kwargs["aggregate_indexes"], ["corp_dmarc_aggregate"]) + self.assertEqual(kwargs["failure_indexes"], ["corp_dmarc_failure"]) + self.assertEqual(kwargs["smtp_tls_indexes"], ["corp_smtp_tls"]) + + @patch("parsedmarc.cli.elastic.migrate_indexes") + @patch("parsedmarc.cli.elastic.set_hosts") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testLegacyFoMigrationHonorsPrefixAndSuffixButNotTheDomainMap( + self, + mock_imap_connection, + mock_get_reports, + _mock_set_hosts, + mock_migrate, + ): + """The published_policy.fo migration takes exact index names, and + index_prefix/index_suffix both date back to 4.1.0, so an affected + index may carry either. It cannot carry a tenant prefix, though: + index_prefix_domain_map arrived in 8.19.0, long after 5.0.0 fixed + the mapping. Asserting the full list covers both halves -- the + configured prefix and suffix are applied, and no map-derived name + is, which matters because this migration deletes the index it + reindexes.""" + mock_imap_connection.return_value = object() + mock_get_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_path = self._write_config( + """[general] +save_aggregate = true +silent = true +index_prefix_domain_map = {map_path} +""" + + self._IMAP + + """ +[elasticsearch] +hosts = localhost +index_prefix = corp_ +index_suffix = prod +""", + domain_map={"Tenant-A": ["example.com"]}, + ) + + with patch.object(sys, "argv", ["parsedmarc", "-c", config_path]): + parsedmarc.cli._main() + + self.assertEqual( + mock_migrate.call_args.kwargs["legacy_fo_indexes"], + ["corp_dmarc_aggregate_prod", "corp_dmarc_aggregate"], + ) + + @patch("parsedmarc.cli.elastic.save_aggregate_report_to_elasticsearch") + @patch("parsedmarc.cli.elastic.migrate_indexes") + @patch("parsedmarc.cli.elastic.set_hosts") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testMigrationTargetsMatchSaveTimePrefix( + self, + mock_imap_connection, + mock_get_reports, + _mock_set_hosts, + mock_migrate, + mock_save_aggregate, + ): + """Both ends of the shared-normalization contract in one test: the + prefix a report is *saved* under has to appear among the index + names the migration targets, or the backfill misses that tenant's + data.""" + mock_imap_connection.return_value = object() + report = _sample_aggregate_reports()[0] + report["policy_published"]["domain"] = "example.com" + mock_get_reports.side_effect = _fetch_invoking_save_callback( + { + "aggregate_reports": [report], + "failure_reports": [], + "smtp_tls_reports": [], + } + ) + config_path = self._write_config( + """[general] +save_aggregate = true +silent = true +index_prefix_domain_map = {map_path} +""" + + self._IMAP + + """ +[elasticsearch] +hosts = localhost +""", + domain_map={"Tenant-A": ["example.com"]}, + ) + + with patch.object(sys, "argv", ["parsedmarc", "-c", config_path]): + parsedmarc.cli._main() + + self.assertEqual( + mock_save_aggregate.call_args.kwargs["index_prefix"], "tenant_a_" + ) + self.assertIn( + "tenant_a_dmarc_aggregate", + mock_migrate.call_args.kwargs["aggregate_indexes"], + ) + + class TestMailboxSaveCallbackWiring(unittest.TestCase): """The CLI's end of the #242 contract: the callback it hands to get_dmarc_reports_from_mailbox() (and to watch_inbox()) reports whether @@ -5157,6 +5655,78 @@ class TestParseConfigGeneral(unittest.TestCase): self.assertEqual(opts.failure_json_filename, "fa.json") self.assertEqual(opts.failure_csv_filename, "fa.csv") + def _index_prefix_domain_map_config(self, yaml_text): + with NamedTemporaryFile("w", suffix=".yaml", delete=False) as map_file: + map_file.write(yaml_text) + map_path = map_file.name + self.addCleanup(lambda: os.path.exists(map_path) and os.remove(map_path)) + return _config_with("general", {"index_prefix_domain_map": map_path}) + + def test_index_prefix_domain_map_accepts_a_mapping_of_lists(self): + from parsedmarc.cli import _parse_config + + cp = self._index_prefix_domain_map_config( + "tenant_a:\n - example.com\n - example.net\n" + ) + self.assertEqual( + _parse_config(cp, _opts()), + {"tenant_a": ["example.com", "example.net"]}, + ) + + def test_index_prefix_domain_map_empty_file_is_unset(self): + """An empty file loads as None, which keeps multi-tenant prefixing + switched off rather than tripping the shape check.""" + from parsedmarc.cli import _parse_config + + cp = self._index_prefix_domain_map_config("") + self.assertIsNone(_parse_config(cp, _opts())) + + def test_index_prefix_domain_map_rejects_a_non_mapping(self): + """A list-shaped file has no tenant names, so the save path cannot + derive index prefixes from it and the migration path would resolve + garbage index names from its items.""" + from parsedmarc.cli import ConfigurationError, _parse_config + + cp = self._index_prefix_domain_map_config("- example.com\n- example.net\n") + with self.assertRaises(ConfigurationError) as ctx: + _parse_config(cp, _opts()) + self.assertIn("index_prefix_domain_map", str(ctx.exception)) + + def test_index_prefix_domain_map_rejects_a_scalar_domain_value(self): + """The save path tests `domain in `, and `in` on a str is a + substring test, not equality + (https://docs.python.org/3/reference/expressions.html + #membership-test-operations) -- so a scalar value silently claims + every domain that contains it, e.g. "example.co" matching + "example.com".""" + from parsedmarc.cli import ConfigurationError, _parse_config + + cp = self._index_prefix_domain_map_config("tenant_a: example.co\n") + with self.assertRaises(ConfigurationError) as ctx: + _parse_config(cp, _opts()) + self.assertIn("list of domain names", str(ctx.exception)) + + def test_index_prefix_domain_map_rejects_a_non_string_domain(self): + """A non-string list item never compares equal to the base domain + the save path looks up, so `tenant_a: [42]` would match nothing at + all -- the same silent-misbehavior class as a scalar value, and the + reason the check covers the list's items and not just its type.""" + from parsedmarc.cli import ConfigurationError, _parse_config + + cp = self._index_prefix_domain_map_config("tenant_a:\n - 42\n") + with self.assertRaises(ConfigurationError) as ctx: + _parse_config(cp, _opts()) + self.assertIn("all strings", str(ctx.exception)) + + def test_index_prefix_domain_map_rejects_a_non_string_key(self): + """A non-string tenant name cannot be normalized into an index + prefix; it used to raise AttributeError mid-save instead.""" + from parsedmarc.cli import ConfigurationError, _parse_config + + cp = self._index_prefix_domain_map_config("42:\n - example.com\n") + with self.assertRaises(ConfigurationError): + _parse_config(cp, _opts()) + def test_general_dns_settings_with_defaults(self): from parsedmarc.cli import _parse_config diff --git a/tests/test_elastic.py b/tests/test_elastic.py index e739b49c..a1b28c88 100644 --- a/tests/test_elastic.py +++ b/tests/test_elastic.py @@ -533,6 +533,241 @@ class TestMigrateIndexes(unittest.TestCase): mock_client.update_by_query.assert_not_called() +def _typeless_fo_mapping(index_name, fo_type): + """An indices.get_field_mapping response in the modern, typeless shape. + + Elasticsearch 8 has no mapping types, so the field sits directly under + ``mappings``. This is the only shape a supported cluster returns; the + ES 6-era type-keyed shape is covered separately. + """ + return { + index_name: { + "mappings": { + "published_policy.fo": { + "full_name": "published_policy.fo", + "mapping": {"fo": {"type": fo_type}}, + } + } + } + } + + +class TestMigrateIndexesFoMigration(unittest.TestCase): + """parsedmarc releases before 5.0.0 declared published_policy.fo as an + integer, so their indexes mapped it as `long`, which cannot hold the + multi-value `fo` settings reports carry (`0:1`, `d:s`). migrate_indexes + detects that and rebuilds the index with the text/keyword shape. + + Elasticsearch 8 refuses to open an index created before 7.0, so an + affected index reaches a supported cluster only by being carried + forward through a reindex — which keeps the old mapping whenever the + destination is pre-created from it, as the standard reindex procedure + does. Each test stubs the combined-field backfill that runs afterwards + in the same call (count 0 → no-op).""" + + @staticmethod + def _noop_backfill_client(): + client = MagicMock() + client.count.return_value = {"count": 0} + return client + + @staticmethod + def _index_mocks(*, v2_exists): + """Distinct Index() mocks per name, so the original and the -v2 + target can be told apart. A single shared mock cannot express + "the original exists but its migration target does not", which is + the ordinary case.""" + original = MagicMock(name="dmarc_aggregate") + original.exists.return_value = True + original.get_field_mapping.return_value = _typeless_fo_mapping( + "dmarc_aggregate", "long" + ) + v2 = MagicMock(name="dmarc_aggregate-v2") + v2.exists.return_value = v2_exists + return original, v2, lambda name: v2 if name.endswith("-v2") else original + + def test_skips_non_existent_index(self): + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_get_conn.return_value = self._noop_backfill_client() + mock_index_cls.return_value.exists.return_value = False + migrate_indexes(legacy_fo_indexes=["missing"]) + mock_index_cls.return_value.get_field_mapping.assert_not_called() + + def test_skips_when_field_is_unmapped(self): + """An index that does not map published_policy.fo at all (e.g. an + empty index with the default mapping) is left alone.""" + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + patch("parsedmarc.elastic.reindex") as mock_reindex, + ): + mock_get_conn.return_value = self._noop_backfill_client() + idx = mock_index_cls.return_value + idx.exists.return_value = True + idx.get_field_mapping.return_value = {"dmarc_aggregate": {"mappings": {}}} + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + mock_reindex.assert_not_called() + idx.create.assert_not_called() + idx.delete.assert_not_called() + + def test_migrates_when_fo_is_long(self): + """The actual migration path: when fo is mapped as 'long', a v2 + index is created with the corrected text/keyword mapping, data is + reindexed into it, and the old index is deleted.""" + original, v2, factory = self._index_mocks(v2_exists=False) + with ( + patch("parsedmarc.elastic.Index", side_effect=factory) as mock_index_cls, + patch("parsedmarc.elastic.reindex") as mock_reindex, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_client = self._noop_backfill_client() + mock_get_conn.return_value = mock_client + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + + self.assertIn(call("dmarc_aggregate-v2"), mock_index_cls.call_args_list) + v2.create.assert_called_once_with() + mapping_kwargs = v2.put_mapping.call_args.kwargs + self.assertNotIn("body", mapping_kwargs) + self.assertEqual( + mapping_kwargs["properties"]["published_policy"]["properties"]["fo"], + { + "type": "text", + "fields": {"keyword": {"type": "keyword", "ignore_above": 256}}, + }, + ) + + # reindex old -> new (v2) with the connection's client, and only + # then is the original dropped. The v2 index is never deleted here: + # nothing was left over to discard. + mock_reindex.assert_called_once_with( + mock_client, "dmarc_aggregate", "dmarc_aggregate-v2" + ) + original.delete.assert_called_once_with() + v2.delete.assert_not_called() + + def test_retries_after_an_interrupted_earlier_attempt(self): + """A run that died between create() and delete() leaves a -v2 index + behind. Reaching this code means the original still holds the data + -- it is deleted only after the reindex succeeds -- so the leftover + is debris. Without discarding it, create() raises "resource already + exists" on every later startup and the index is never migrated; + confirmed against a live cluster, where the unfixed code left the + original in place and the debris document in the v2 index.""" + original, v2, factory = self._index_mocks(v2_exists=True) + with ( + patch("parsedmarc.elastic.Index", side_effect=factory), + patch("parsedmarc.elastic.reindex") as mock_reindex, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_client = self._noop_backfill_client() + mock_get_conn.return_value = mock_client + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + + self.assertTrue( + any("Discarding dmarc_aggregate-v2" in msg for msg in cm.output) + ) + # The stale target is dropped, then recreated, and the migration + # runs to completion instead of aborting on "already exists". + v2.delete.assert_called_once_with() + v2.create.assert_called_once_with() + mock_reindex.assert_called_once_with( + mock_client, "dmarc_aggregate", "dmarc_aggregate-v2" + ) + original.delete.assert_called_once_with() + + def test_migrates_when_fo_is_long_under_a_mapping_type(self): + """The Elasticsearch 6-era response nested the field under the + mapping type name. No cluster either client can connect to still + reports mappings that way, so this covers the fallback branch + rather than a reachable deployment -- but the branch is what lets + the type check stay a check on the mapped type instead of on the + response shape, which is what broke this migration before.""" + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.reindex") as mock_reindex, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_get_conn.return_value = self._noop_backfill_client() + idx = mock_index_cls.return_value + idx.exists.return_value = True + idx.get_field_mapping.return_value = { + "dmarc_aggregate": { + "mappings": { + "doc": { + "published_policy.fo": {"mapping": {"fo": {"type": "long"}}} + } + } + } + } + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + mock_reindex.assert_called_once() + + def test_skips_when_fo_already_text(self): + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + patch("parsedmarc.elastic.reindex") as mock_reindex, + ): + mock_get_conn.return_value = self._noop_backfill_client() + idx = mock_index_cls.return_value + idx.exists.return_value = True + idx.get_field_mapping.return_value = _typeless_fo_mapping( + "dmarc_aggregate", "text" + ) + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + mock_reindex.assert_not_called() + idx.create.assert_not_called() + idx.delete.assert_not_called() + + def test_index_exists_failure_does_not_raise(self): + """A cluster error inside the per-index fo-migration loop (e.g. + Index(...).exists() raising because the cluster is unreachable) + must not abort startup: it is caught, logged, and the loop moves + on to the combined-field backfill, which is exercised here with + its own connection failure so both warnings are asserted.""" + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_index_cls.return_value.exists.side_effect = ConnectionError( + "cluster unreachable" + ) + mock_get_conn.side_effect = RuntimeError("no connection") + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + migrate_indexes( + aggregate_indexes=["dmarc_aggregate"], + legacy_fo_indexes=["dmarc_aggregate"], + ) + + self.assertTrue( + any( + "legacy published_policy.fo migration" in msg + and "cluster unreachable" in msg + for msg in cm.output + ) + ) + self.assertTrue( + any("Skipping the dkim_results_combined" in msg for msg in cm.output) + ) + self.assertTrue(any("no connection" in msg for msg in cm.output)) + + def test_no_legacy_indexes_means_no_lookup(self): + """The combined-field backfill must not drag the fo migration + along: passing only aggregate_indexes leaves the legacy loop + untouched, so a modern deployment never probes for it.""" + with ( + patch("parsedmarc.elastic.Index") as mock_index_cls, + patch("parsedmarc.elastic.connections.get_connection") as mock_get_conn, + ): + mock_get_conn.return_value = self._noop_backfill_client() + migrate_indexes(aggregate_indexes=["dmarc_aggregate"]) + mock_index_cls.return_value.exists.assert_not_called() + + # --------------------------------------------------------------------------- # save_aggregate_report_to_elasticsearch # --------------------------------------------------------------------------- diff --git a/tests/test_opensearch.py b/tests/test_opensearch.py index c1748466..5bf02de4 100644 --- a/tests/test_opensearch.py +++ b/tests/test_opensearch.py @@ -385,11 +385,8 @@ class TestMigrateIndexes(unittest.TestCase): def test_backfill_submitted_when_old_docs_exist(self): with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - # The legacy fo migration that runs first sees no base index. - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.return_value = {"count": 42} mock_get_conn.return_value = mock_client @@ -416,10 +413,8 @@ class TestMigrateIndexes(unittest.TestCase): def test_backfill_skipped_when_no_old_docs(self): with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.return_value = {"count": 0} mock_get_conn.return_value = mock_client @@ -436,10 +431,8 @@ class TestMigrateIndexes(unittest.TestCase): def test_backfill_failure_does_not_raise(self): with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.side_effect = RuntimeError("cluster unreachable") mock_get_conn.return_value = mock_client @@ -456,11 +449,8 @@ class TestMigrateIndexes(unittest.TestCase): still not propagate the exception, per its docstring's promise that any cluster error is caught and logged.""" with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - # The legacy fo migration that runs first sees no base index. - mock_index_cls.return_value.exists.return_value = False mock_get_conn.side_effect = RuntimeError("no connection") with self.assertLogs("parsedmarc.log", level="WARNING") as cm: migrate_indexes(aggregate_indexes=["dmarc_aggregate"]) @@ -475,11 +465,8 @@ class TestMigrateIndexes(unittest.TestCase): policies_combined/failure_details_combined backfill (also issue #169) is submitted with its own guard query and painless script.""" with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - # The legacy fo migration that runs first sees no base index. - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.return_value = {"count": 7} mock_get_conn.return_value = mock_client @@ -506,10 +493,8 @@ class TestMigrateIndexes(unittest.TestCase): def test_smtp_tls_backfill_skipped_when_no_old_docs(self): with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.return_value = {"count": 0} mock_get_conn.return_value = mock_client @@ -529,10 +514,8 @@ class TestMigrateIndexes(unittest.TestCase): error from the cluster during the smtp_tls_indexes loop is caught and logged rather than raised.""" with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): - mock_index_cls.return_value.exists.return_value = False mock_client = MagicMock() mock_client.count.side_effect = RuntimeError("cluster unreachable") mock_get_conn.return_value = mock_client @@ -543,13 +526,37 @@ class TestMigrateIndexes(unittest.TestCase): mock_client.update_by_query.assert_not_called() +def _typeless_fo_mapping(index_name, fo_type): + """An indices.get_field_mapping response in the modern, typeless shape. + + OpenSearch has no mapping types, so the field sits directly under + ``mappings``. This is the only shape a real cluster returns; the + ES 6-era type-keyed shape is covered separately. + """ + return { + index_name: { + "mappings": { + "published_policy.fo": { + "full_name": "published_policy.fo", + "mapping": {"fo": {"type": fo_type}}, + } + } + } + } + + class TestMigrateIndexesFoMigration(unittest.TestCase): - """The legacy `published_policy.fo` field was mapped as `long` in - older indexes. migrate_indexes detects that and rebuilds the index - with the text/keyword shape. The branch is gnarly; a regression - would silently leave old data un-migrated. Each test stubs the - combined-field backfill that now runs afterwards in the same call - (count 0 → no-op).""" + """parsedmarc releases before 5.0.0 declared published_policy.fo as an + integer, so their indexes mapped it as `long`, which cannot hold the + multi-value `fo` settings reports carry (`0:1`, `d:s`). migrate_indexes + detects that and rebuilds the index with the text/keyword shape. + + These tests drive the modern typeless get_field_mapping response. + Until this was fixed, the code read the response in the Elasticsearch + 6-era mapping-type-keyed shape, which no OpenSearch cluster returns — + so the migration could never run against a real cluster even though + tests using the old shape passed. Each test stubs the combined-field + backfill that runs afterwards in the same call (count 0 → no-op).""" @staticmethod def _noop_backfill_client(): @@ -557,6 +564,21 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): client.count.return_value = {"count": 0} return client + @staticmethod + def _index_mocks(*, v2_exists): + """Distinct Index() mocks per name, so the original and the -v2 + target can be told apart. A single shared mock cannot express + "the original exists but its migration target does not", which is + the ordinary case.""" + original = MagicMock(name="dmarc_aggregate") + original.exists.return_value = True + original.get_field_mapping.return_value = _typeless_fo_mapping( + "dmarc_aggregate", "long" + ) + v2 = MagicMock(name="dmarc_aggregate-v2") + v2.exists.return_value = v2_exists + return original, v2, lambda name: v2 if name.endswith("-v2") else original + def test_skips_non_existent_index(self): with ( patch("parsedmarc.opensearch.Index") as mock_index_cls, @@ -564,13 +586,13 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): ): mock_get_conn.return_value = self._noop_backfill_client() mock_index_cls.return_value.exists.return_value = False - migrate_indexes(aggregate_indexes=["missing"]) + migrate_indexes(legacy_fo_indexes=["missing"]) # exists() returned False — no field_mapping fetch. mock_index_cls.return_value.get_field_mapping.assert_not_called() - def test_skips_when_doc_mapping_absent(self): - """An index that has 'fo' but not under the 'doc' type - (e.g., empty index with default mapping) is left alone.""" + def test_skips_when_field_is_unmapped(self): + """An index that does not map published_policy.fo at all (e.g. an + empty index with the default mapping) is left alone.""" with ( patch("parsedmarc.opensearch.Index") as mock_index_cls, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, @@ -579,25 +601,97 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): mock_get_conn.return_value = self._noop_backfill_client() idx = mock_index_cls.return_value idx.exists.return_value = True - idx.get_field_mapping.return_value = {"some_key": {"mappings": {}}} - migrate_indexes(aggregate_indexes=["dmarc_aggregate-2023-01-01"]) + idx.get_field_mapping.return_value = {"dmarc_aggregate": {"mappings": {}}} + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) mock_reindex.assert_not_called() + idx.create.assert_not_called() + idx.delete.assert_not_called() def test_migrates_when_fo_is_long(self): - """The actual migration path: when fo is mapped as 'long', - a v2 index is created with the corrected mapping, data is - reindexed, and the old index is deleted.""" + """The actual migration path: when fo is mapped as 'long', a v2 + index is created with the corrected text/keyword mapping, data is + reindexed into it, and the old index is deleted.""" + original, v2, factory = self._index_mocks(v2_exists=False) with ( - patch("parsedmarc.opensearch.Index") as mock_index_cls, + patch("parsedmarc.opensearch.Index", side_effect=factory) as mock_index_cls, patch("parsedmarc.opensearch.reindex") as mock_reindex, patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, ): mock_client = self._noop_backfill_client() mock_get_conn.return_value = mock_client + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + + self.assertIn(call("dmarc_aggregate-v2"), mock_index_cls.call_args_list) + v2.create.assert_called_once_with() + mapping_kwargs = v2.put_mapping.call_args.kwargs + self.assertNotIn("doc_type", mapping_kwargs) + self.assertEqual( + mapping_kwargs["body"]["properties"]["published_policy"]["properties"][ + "fo" + ], + { + "type": "text", + "fields": {"keyword": {"type": "keyword", "ignore_above": 256}}, + }, + ) + + # reindex old -> new (v2) with the connection's client, and only + # then is the original dropped. The v2 index is never deleted here: + # nothing was left over to discard. + mock_reindex.assert_called_once_with( + mock_client, "dmarc_aggregate", "dmarc_aggregate-v2" + ) + original.delete.assert_called_once_with() + v2.delete.assert_not_called() + + def test_retries_after_an_interrupted_earlier_attempt(self): + """A run that died between create() and delete() leaves a -v2 index + behind. Reaching this code means the original still holds the data + -- it is deleted only after the reindex succeeds -- so the leftover + is debris. Without discarding it, create() raises "resource already + exists" on every later startup and the index is never migrated; + confirmed against a live cluster, where the unfixed code left the + original in place and the debris document in the v2 index.""" + original, v2, factory = self._index_mocks(v2_exists=True) + with ( + patch("parsedmarc.opensearch.Index", side_effect=factory), + patch("parsedmarc.opensearch.reindex") as mock_reindex, + patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, + ): + mock_client = self._noop_backfill_client() + mock_get_conn.return_value = mock_client + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) + + self.assertTrue( + any("Discarding dmarc_aggregate-v2" in msg for msg in cm.output) + ) + # The stale target is dropped, then recreated, and the migration + # runs to completion instead of aborting on "already exists". + v2.delete.assert_called_once_with() + v2.create.assert_called_once_with() + mock_reindex.assert_called_once_with( + mock_client, "dmarc_aggregate", "dmarc_aggregate-v2" + ) + original.delete.assert_called_once_with() + + def test_migrates_when_fo_is_long_under_a_mapping_type(self): + """The Elasticsearch 6-era response nested the field under the + mapping type name. No cluster either client can connect to still + reports mappings that way, so this covers the fallback branch + rather than a reachable deployment -- but the branch is what lets + the type check stay a check on the mapped type instead of on the + response shape, which is what broke this migration before.""" + with ( + patch("parsedmarc.opensearch.Index") as mock_index_cls, + patch("parsedmarc.opensearch.reindex") as mock_reindex, + patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, + ): + mock_get_conn.return_value = self._noop_backfill_client() idx = mock_index_cls.return_value idx.exists.return_value = True idx.get_field_mapping.return_value = { - "dmarc_aggregate-2023-01-01": { + "dmarc_aggregate": { "mappings": { "doc": { "published_policy.fo": {"mapping": {"fo": {"type": "long"}}} @@ -605,11 +699,8 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): } } } - migrate_indexes(aggregate_indexes=["dmarc_aggregate-2023-01-01"]) - # reindex called from old → new (v2) index, with the client from - # connections.get_connection(). + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) mock_reindex.assert_called_once() - self.assertIs(mock_reindex.call_args.args[0], mock_client) def test_skips_when_fo_already_text(self): with ( @@ -620,17 +711,13 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): mock_get_conn.return_value = self._noop_backfill_client() idx = mock_index_cls.return_value idx.exists.return_value = True - idx.get_field_mapping.return_value = { - "dmarc_aggregate-2024-01-01": { - "mappings": { - "doc": { - "published_policy.fo": {"mapping": {"fo": {"type": "text"}}} - } - } - } - } - migrate_indexes(aggregate_indexes=["dmarc_aggregate-2024-01-01"]) + idx.get_field_mapping.return_value = _typeless_fo_mapping( + "dmarc_aggregate", "text" + ) + migrate_indexes(legacy_fo_indexes=["dmarc_aggregate"]) mock_reindex.assert_not_called() + idx.create.assert_not_called() + idx.delete.assert_not_called() def test_index_exists_failure_does_not_raise(self): """A cluster error inside the per-index fo-migration loop (e.g. @@ -647,7 +734,10 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): ) mock_get_conn.side_effect = RuntimeError("no connection") with self.assertLogs("parsedmarc.log", level="WARNING") as cm: - migrate_indexes(aggregate_indexes=["dmarc_aggregate"]) + migrate_indexes( + aggregate_indexes=["dmarc_aggregate"], + legacy_fo_indexes=["dmarc_aggregate"], + ) self.assertTrue( any( @@ -661,6 +751,18 @@ class TestMigrateIndexesFoMigration(unittest.TestCase): ) self.assertTrue(any("no connection" in msg for msg in cm.output)) + def test_no_legacy_indexes_means_no_lookup(self): + """The combined-field backfill must not drag the fo migration + along: passing only aggregate_indexes leaves the legacy loop + untouched, so a modern deployment never probes for it.""" + with ( + patch("parsedmarc.opensearch.Index") as mock_index_cls, + patch("parsedmarc.opensearch.connections.get_connection") as mock_get_conn, + ): + mock_get_conn.return_value = self._noop_backfill_client() + migrate_indexes(aggregate_indexes=["dmarc_aggregate"]) + mock_index_cls.return_value.exists.assert_not_called() + # --------------------------------------------------------------------------- # save_aggregate_report_to_opensearch