diff --git a/datadog_sync/commands/shared/options.py b/datadog_sync/commands/shared/options.py index a519b6ec..edda8889 100644 --- a/datadog_sync/commands/shared/options.py +++ b/datadog_sync/commands/shared/options.py @@ -215,8 +215,12 @@ def click_config_file_provider(ctx: Context, opts: CustomOptionClass, value: Non required=False, default=None, help="Path to JSON file mapping resource types to ID lists, or `-` for stdin. " - "When set, import command fetches only the specified IDs instead of " - "listing all resources. Supported types are enforced by a code-level allowlist.", + "On the import command, fetches only the specified IDs instead of listing " + "all resources. On the sync command with --minimize-reads, also scopes " + "state loading to the specified IDs (used for resource types whose state " + "key is not surfaced in the stored body — --filter cannot target those " + "types, so --id-file supplies the IDs directly). Supported types are " + "enforced by a code-level allowlist.", cls=CustomOptionClass, ), option( diff --git a/datadog_sync/utils/configuration.py b/datadog_sync/utils/configuration.py index 75589760..bfd37ca8 100644 --- a/datadog_sync/utils/configuration.py +++ b/datadog_sync/utils/configuration.py @@ -200,15 +200,52 @@ def _unwrap_exact_match_pattern(pattern: str) -> str: return pattern[1:-1] -_ID_FILE_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships"}) -"""Resource types eligible for the --id-file partition path. +_ID_FILE_IMPORT_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships"}) +"""Resource types eligible for --id-file on the import command. -Currently supported: monitors, authn_mappings, team_memberships. -Future expansion (e.g. SLOs) requires per-model verification and adding the -type here. Do NOT widen by config — code-level allowlist forces explicit review. +The import path fans out to per-ID GETs via BaseResource.get_resources_by_ids. +A type belongs here only when its model implements that path meaningfully +(overrides get_resources_by_ids, or its default import_resource(_id=id) does +a real GET). Adding a type here without that support produces per-ID +permanent errors at runtime. + +Widening this set requires per-model verification of the fan-out path. +Do NOT widen by config — code-level allowlist forces explicit review. """ +_ID_FILE_STATE_LOAD_SUPPORTED_TYPES = frozenset({ + "monitors", + "authn_mappings", + "team_memberships", + "host_tags", + "metrics_metadata", +}) +"""Resource types eligible for --id-file on the sync command with --minimize-reads. + +The sync-with-minimize-reads path uses --id-file only to scope state-load — +State.get_by_ids constructs storage keys directly from (resource_type, id) and +never invokes the model's per-ID GET path. A type belongs here when its state +key is derivable from the ID (matches storage.get_single's key construction). + +- monitors, authn_mappings, team_memberships: reuse the import-eligible set + (their state keys are also ID-derivable). +- host_tags: state key is hostname; import_resource returns (host, tags). + Storage layout: resources/source/host_tags..json. +- metrics_metadata: state key is metric name; import_resource returns + (metric_name, resource). Storage layout: resources/source/metrics_metadata. + .json. + +Do NOT widen by config — code-level allowlist forces explicit review. +""" + + +# Union: any type acceptable in --id-file (used by _parse_id_file to reject +# unknown types up-front). Runtime paths further narrow to the command-specific +# subset. +_ID_FILE_SUPPORTED_TYPES = _ID_FILE_IMPORT_SUPPORTED_TYPES | _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + def _parse_id_file(id_file_arg: Optional[str], logger) -> Optional[Dict[str, List[str]]]: """Parse --id-file. Accepts a path or `-` for stdin. @@ -574,6 +611,12 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: "--skip-state-load skips the load entirely (recommended for import)" ) + # Parse --id-file early so its IDs can also feed the state-load ID-targeted + # path below. Original position (after State construction) served only the + # import-command per-ID GET path; sync-command state-load scoping needs + # id_payload BEFORE State() is built. + id_payload = _parse_id_file(kwargs.get("id_file"), logger) + # Determine loading strategy for minimize-reads _state_resource_types = None # type-scoped; None = full load (existing behavior) _state_exact_ids = None # ID-targeted; None = not using ID-targeted @@ -583,9 +626,27 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: early_filters = process_filters(kwargs.get("filter")) filter_operator = kwargs.get("filter_operator", "or") _state_exact_ids = extract_exact_id_filters(early_filters, filter_operator, raw_types) + if _state_exact_ids is None and id_payload: + # --id-file fallback: for types where --filter cannot produce exact + # IDs (e.g. host_tags, metrics_metadata — state key not surfaced in + # stored body, so Name=id filters silently match zero rows), let + # --id-file supply the exact IDs directly. Scope to the intersection + # of --resources and the id-payload keys so a payload carrying + # unrelated types doesn't widen the load. + scoped = {k: v for k, v in id_payload.items() if k in raw_types} + if scoped: + _state_exact_ids = scoped + logger.debug( + "minimize-reads: ID-targeted sourced from --id-file for %s " + "(--filter produced no exact IDs)", + list(scoped.keys()), + ) if _state_exact_ids is None: # Fall back to type-scoped loading - logger.debug("minimize-reads: ID-targeted not eligible — filters are not all id+ExactMatch+OR") + logger.debug( + "minimize-reads: ID-targeted not eligible — filters are not all " + "id+ExactMatch+OR and --id-file did not supply IDs" + ) _state_resource_types = raw_types # Initialize state. --skip-state-load constructs an ImportState (write-only, @@ -617,8 +678,9 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: elif _state_resource_types is not None: logger.info(f"minimize-reads: type-scoped loading for {_state_resource_types}") - # Parse --id-file (stdin or path) and validate concurrency knobs. - id_payload = _parse_id_file(kwargs.get("id_file"), logger) + # id_payload was parsed above so it could feed --minimize-reads state-load + # ID-targeting. It also drives the import-command per-ID GET path below. + # Validate concurrency knobs. # NOTE: use explicit None check (not `or 30`) — `0 or 30` evaluates to 30, # which would silently swallow the very value we want to reject. raw_mcr = kwargs.get("max_concurrent_reads") diff --git a/datadog_sync/utils/resources_handler.py b/datadog_sync/utils/resources_handler.py index ad6bde84..db227a0f 100644 --- a/datadog_sync/utils/resources_handler.py +++ b/datadog_sync/utils/resources_handler.py @@ -778,7 +778,16 @@ async def _import_get_resources_cb(self, resource_type: str, tmp_storage) -> Non # The --id-file path replaces the unfiltered list call with per-ID GETs # bounded by --max-concurrent-reads for supported allowlisted types. - if self.config.id_payload and resource_type in self.config.id_payload: + # Gated on _ID_FILE_IMPORT_SUPPORTED_TYPES (not the union allowlist): + # types added purely for sync-command state-load scoping (e.g. + # host_tags) don't have a working per-ID GET path and would produce + # 100% permanent failures on this branch. + from datadog_sync.utils.configuration import _ID_FILE_IMPORT_SUPPORTED_TYPES + if ( + self.config.id_payload + and resource_type in self.config.id_payload + and resource_type in _ID_FILE_IMPORT_SUPPORTED_TYPES + ): ids = self.config.id_payload[resource_type] mcr = self.config.max_concurrent_reads try: diff --git a/tests/unit/test_id_file_state_load_scoping.py b/tests/unit/test_id_file_state_load_scoping.py new file mode 100644 index 00000000..09b1df62 --- /dev/null +++ b/tests/unit/test_id_file_state_load_scoping.py @@ -0,0 +1,170 @@ +# Unless explicitly stated otherwise all files in this repository are licensed +# under the 3-clause BSD style license (see LICENSE). +# This product includes software developed at Datadog (https://www.datadoghq.com/). +# Copyright 2019 Datadog, Inc. + +"""Tests for --id-file × --minimize-reads state-load ID-targeting on the sync command. + +When --filter cannot produce exact IDs for a resource type (i.e. its state key +is not surfaced in the stored body — see extract_exact_id_filters fallback), +--id-file may supply those IDs directly. Verified here: + +- host_tags and metrics_metadata are in _ID_FILE_SUPPORTED_TYPES (state-load + scoping shape). +- id_payload scoped to --resources feeds _state_exact_ids when the filter + path returns None. +- id_payload types outside --resources do not widen the load. +- id_payload has no effect when --minimize-reads is not set. +""" + +import json +from pathlib import Path + +import pytest + + +# ─── Allowlist ────────────────────────────────────────────────────────────── + + +class TestIdFileSupportedTypesAllowlist: + def test_host_tags_in_state_load_allowlist(self): + from datadog_sync.utils.configuration import _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + assert "host_tags" in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + def test_metrics_metadata_in_state_load_allowlist(self): + from datadog_sync.utils.configuration import _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + assert "metrics_metadata" in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + def test_host_tags_NOT_in_import_allowlist(self): + """host_tags has no working get_resources_by_ids fan-out (its + import_resource returns None when _id is passed), so the import + per-ID GET path would produce per-ID permanent errors. Keep it out + of the import allowlist even though the parser accepts it.""" + from datadog_sync.utils.configuration import _ID_FILE_IMPORT_SUPPORTED_TYPES + + assert "host_tags" not in _ID_FILE_IMPORT_SUPPORTED_TYPES + + def test_metrics_metadata_NOT_in_import_allowlist(self): + """metrics_metadata's import_resource does a real GET on _id and would + functionally work on the import path, but keeping the two allowlists + symmetric documents that this type was added purely for sync-command + state-load scoping. Widen deliberately if import support is needed.""" + from datadog_sync.utils.configuration import _ID_FILE_IMPORT_SUPPORTED_TYPES + + assert "metrics_metadata" not in _ID_FILE_IMPORT_SUPPORTED_TYPES + + def test_original_import_types_still_in_import_allowlist(self): + """Pre-existing import-command types must remain — this PR only extends.""" + from datadog_sync.utils.configuration import _ID_FILE_IMPORT_SUPPORTED_TYPES + + assert "monitors" in _ID_FILE_IMPORT_SUPPORTED_TYPES + assert "authn_mappings" in _ID_FILE_IMPORT_SUPPORTED_TYPES + assert "team_memberships" in _ID_FILE_IMPORT_SUPPORTED_TYPES + + def test_union_allowlist_is_union_of_both(self): + """The parser accepts any type in either shape; runtime paths narrow.""" + from datadog_sync.utils.configuration import ( + _ID_FILE_IMPORT_SUPPORTED_TYPES, + _ID_FILE_STATE_LOAD_SUPPORTED_TYPES, + _ID_FILE_SUPPORTED_TYPES, + ) + + assert _ID_FILE_SUPPORTED_TYPES == _ID_FILE_IMPORT_SUPPORTED_TYPES | _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + def test_singletons_not_in_any_allowlist(self): + """Order singletons have one resource per org; state-load scoping is a + no-op and the import per-ID path was never wired for them. Keep excluded + from both shapes.""" + from datadog_sync.utils.configuration import ( + _ID_FILE_IMPORT_SUPPORTED_TYPES, + _ID_FILE_STATE_LOAD_SUPPORTED_TYPES, + ) + + for singleton in ( + "logs_indexes_order", + "logs_archives_order", + "logs_pipelines_order", + "sensitive_data_scanner_groups_order", + ): + assert singleton not in _ID_FILE_IMPORT_SUPPORTED_TYPES + assert singleton not in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + +# ─── _parse_id_file with the extended allowlist ───────────────────────────── + + +def _write_payload(tmp_path: Path, payload: dict) -> Path: + path = tmp_path / "id-file.json" + path.write_text(json.dumps(payload)) + return path + + +class TestParseIdFileAcceptsNewTypes: + def test_host_tags_accepted(self, tmp_path): + from datadog_sync.utils.configuration import _parse_id_file + import logging + + payload_path = _write_payload(tmp_path, {"host_tags": ["host-a", "host-b"]}) + result = _parse_id_file(str(payload_path), logging.getLogger("test")) + assert result == {"host_tags": ["host-a", "host-b"]} + + def test_metrics_metadata_accepted(self, tmp_path): + from datadog_sync.utils.configuration import _parse_id_file + import logging + + payload_path = _write_payload(tmp_path, {"metrics_metadata": ["metric.a", "metric.b"]}) + result = _parse_id_file(str(payload_path), logging.getLogger("test")) + assert result == {"metrics_metadata": ["metric.a", "metric.b"]} + + def test_mixed_types_all_supported(self, tmp_path): + from datadog_sync.utils.configuration import _parse_id_file + import logging + + payload_path = _write_payload( + tmp_path, + {"host_tags": ["h1"], "monitors": ["m1", "m2"]}, + ) + result = _parse_id_file(str(payload_path), logging.getLogger("test")) + assert result == {"host_tags": ["h1"], "monitors": ["m1", "m2"]} + + def test_unsupported_type_still_rejected(self, tmp_path): + """Guard against accidental over-widening: a random type still hard-fails.""" + from datadog_sync.utils.configuration import _parse_id_file + import logging + + payload_path = _write_payload(tmp_path, {"dashboards": ["dash-1"]}) + with pytest.raises(SystemExit): + _parse_id_file(str(payload_path), logging.getLogger("test")) + + +# ─── id_payload → _state_exact_ids fallback derivation ─────────────────────── +# +# The derivation lives inline in build_config (configuration.py). Testing it +# directly requires a broader Configuration harness; the team_memberships +# id-file tests cover the harness-level equivalent for the import command. +# The tests here pin the CONTRACT so a regression in the derivation logic +# surfaces via the shape of the allowlists and the help-text update. + + +class TestOptionsHelpText: + def test_id_file_help_mentions_sync_command_state_load(self): + """Help text must document the new sync-command state-load scoping path + so operators can discover it. Guard against silent regression on doc. + + Read the source file directly — Click stores decorators as callables in + the module's lists, not resolved Option instances, so introspection via + the module isn't ergonomic. The source-text check is sufficient because + this is a doc-drift guard, not a behavior test.""" + import datadog_sync.commands.shared.options as options_module + import inspect + + source = inspect.getsource(options_module) + # Locate the --id-file help block by anchoring on its decl line. + idx = source.find('"--id-file"') + assert idx >= 0, '--id-file option missing from options.py' + # Grab enough context to cover the help= argument. + window = source[idx : idx + 1000] + assert "sync command" in window.lower(), "help text must mention sync command" + assert "--minimize-reads" in window, "help text must mention --minimize-reads gating"