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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions datadog_sync/utils/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,10 @@ class Configuration(object):
allow_partial_permissions_roles: List[str] = field(default_factory=list)
resources: Dict[str, BaseResource] = field(default_factory=dict)
resources_arg: List[str] = field(default_factory=list)
# --id-file: id-targeted import via stdin or file payload.
# Supported resource types are explicitly allowlisted in
# _ID_FILE_SUPPORTED_TYPES (currently monitors, authn_mappings,
# team_memberships). Other types use the legacy list-everything path.
# Future expansion (e.g. SLOs) is mechanical via
# BaseResource.get_resources_by_ids inheritance.
# --id-file: id-targeted import and sync state-load scoping via stdin or
# file payload. Supported resource types are explicitly allowlisted in
# _ID_FILE_SUPPORTED_TYPES; runtime paths narrow that union to the
# command-specific import and state-load sets.
id_payload: Optional[Dict[str, List[str]]] = None
max_concurrent_reads: int = 30
transient_failure_threshold_pct: int = 5
Expand Down Expand Up @@ -253,7 +251,7 @@ def _unwrap_exact_match_pattern(pattern: str) -> str:
return _regex_literal_from_exact_match_body(pattern[1:-1])


_ID_FILE_IMPORT_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships"})
_ID_FILE_IMPORT_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships", "dashboards"})
Comment thread
michael-richey marked this conversation as resolved.
"""Resource types eligible for --id-file on the import command.

The import path fans out to per-ID GETs via BaseResource.get_resources_by_ids.
Expand All @@ -264,6 +262,14 @@ def _unwrap_exact_match_pattern(pattern: str) -> str:

Widening this set requires per-model verification of the fan-out path.
Do NOT widen by config — code-level allowlist forces explicit review.

dashboards: Dashboards.import_resource(_id=...) performs a real GET to
/api/v1/dashboard/{id} for the full body (widgets are omitted by the LIST
endpoint). The model also short-circuits when the caller passes a body
already carrying widgets, so the id-file path's get_resources_by_ids ->
import_resource(_id=...) -> queue-handler _import_resource(resource=body)
sequence does exactly one GET per dashboard (no double-fetch). Verified via
tests/unit/test_dashboards_id_file.py.
"""


Expand All @@ -273,6 +279,7 @@ def _unwrap_exact_match_pattern(pattern: str) -> str:
"team_memberships",
"host_tags",
"metrics_metadata",
"dashboards",
})
"""Resource types eligible for --id-file on the sync command with --minimize-reads.

Expand All @@ -288,6 +295,11 @@ def _unwrap_exact_match_pattern(pattern: str) -> str:
- metrics_metadata: state key is metric name; import_resource returns
(metric_name, resource). Storage layout: resources/source/metrics_metadata.
<metric_name>.json.
- dashboards: state key is the dashboard id (e.g. abc-def-ghi). Storage
layout: resources/source/dashboards.<id>.json. ID-derivable, so
State.get_by_ids constructs the correct key. Added alongside the import
allowlist entry so the union (_ID_FILE_SUPPORTED_TYPES) accepts dashboards
on both paths by design rather than incidentally via the import set.

Do NOT widen by config — code-level allowlist forces explicit review.
"""
Expand Down
149 changes: 149 additions & 0 deletions tests/unit/test_dashboards_id_file.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
# 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 dashboards support in _ID_FILE_IMPORT_SUPPORTED_TYPES.

Pins that 'dashboards' is accepted by --id-file and that the per-ID GET
path used by get_resources_by_ids (import --id-file) does exactly one GET
per dashboard, with the prefetched-body short-circuit preventing a
double-fetch on queue-handler re-entry.
"""

import asyncio
from unittest.mock import AsyncMock, MagicMock

import pytest

from datadog_sync.model.dashboards import Dashboards
from datadog_sync.utils.configuration import (
_ID_FILE_IMPORT_SUPPORTED_TYPES,
_ID_FILE_SUPPORTED_TYPES,
)
from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource


class TestDashboardsIDFileSupport:
"""Tests for dashboards support in the --id-file allowlist."""

def test_dashboards_in_id_file_import_supported_types(self):
"""'dashboards' must be present in _ID_FILE_IMPORT_SUPPORTED_TYPES so
that `import --id-file=- < {"dashboards": [...]}` is accepted."""
assert "dashboards" in _ID_FILE_IMPORT_SUPPORTED_TYPES, (
"dashboards must be in _ID_FILE_IMPORT_SUPPORTED_TYPES. "
"If this fails, id-file import support for dashboards is missing."
)

def test_dashboards_in_id_file_supported_types_union(self):
"""'dashboards' must also be in the union allowlist consulted by
_parse_id_file (rejects unknown types up-front)."""
assert "dashboards" in _ID_FILE_SUPPORTED_TYPES

def test_dashboards_in_state_load_supported_types(self):
"""Adding dashboards to the import allowlist also adds it to the
union (_ID_FILE_SUPPORTED_TYPES), which _parse_id_file consults for
*both* import and sync --minimize-reads. Since the state-load path
scopes by --resources intersection rather than by
_ID_FILE_STATE_LOAD_SUPPORTED_TYPES, dashboards must be explicitly
in the state-load set too — otherwise it's accepted incidentally via
the union without the ID-derivability verification the set exists to
enforce. Dashboards' state key is dashboards.<id>.json (ID-derivable),
so it qualifies."""
from datadog_sync.utils.configuration import _ID_FILE_STATE_LOAD_SUPPORTED_TYPES

assert "dashboards" in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES, (
"dashboards is in _ID_FILE_IMPORT_SUPPORTED_TYPES, so the union "
"accepts it on the sync state-load path too; it must be explicitly "
"in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES (state key is ID-derivable)."
)

def test_import_resource_id_does_real_get(self):
"""import_resource(_id=...) performs a GET to /api/v1/dashboard/{id}
and returns the body — the per-ID fan-out path used by
get_resources_by_ids on id-file import runs."""
mock_config = MagicMock()
mock_client = AsyncMock()
body = {
"id": "abc-def-ghi",
"title": "test-dashboard",
"widgets": [{"definition": {"type": "timeseries"}}],
}
mock_client.get.return_value = body
mock_config.source_client = mock_client
dashboards = Dashboards(mock_config)

_id, resource = asyncio.run(dashboards.import_resource(_id="abc-def-ghi"))

mock_client.get.assert_awaited_once()
call_path = mock_client.get.call_args[0][0]
assert call_path == "/api/v1/dashboard/abc-def-ghi", (
f"import_resource(_id=...) must GET /api/v1/dashboard/{{id}}; got {call_path!r}"
)
assert _id == "abc-def-ghi"
assert resource == body

def test_import_resource_id_short_circuits_when_caller_supplies_full_body(self):
"""When the queue handler re-enters import_resource with the body
already fetched by get_resources_by_ids (detected by 'widgets'
presence), the model must NOT issue a second GET — otherwise id-file
import doubles rate-limit pressure on /api/v1/dashboard/{id}."""
mock_config = MagicMock()
mock_client = AsyncMock()
mock_config.source_client = mock_client
dashboards = Dashboards(mock_config)
full_body = {
"id": "abc-def-ghi",
"title": "prefetched",
"widgets": [{"definition": {"type": "timeseries"}}],
}

_id, resource = asyncio.run(dashboards.import_resource(resource=full_body))

mock_client.get.assert_not_awaited()
assert _id == "abc-def-ghi"
assert resource == full_body

def test_import_resource_id_403_raises_skip_resource(self):
"""A 403 on the per-ID GET raises SkipResource so the id-file path
buckets it as 'skipped' (matching get_resources_by_ids' handling)
rather than aborting the whole import."""
mock_config = MagicMock()
mock_client = AsyncMock()
resp = MagicMock()
resp.status = 403
mock_client.get.side_effect = CustomClientHTTPError(resp, message="Forbidden")
mock_config.source_client = mock_client
dashboards = Dashboards(mock_config)

with pytest.raises(SkipResource):
asyncio.run(dashboards.import_resource(_id="abc-def-ghi"))

def test_import_resource_id_propagates_non_403_errors(self):
"""A 500 on the per-ID GET propagates (get_resources_by_ids classifies
5xx as transient and retries; the model must not swallow it)."""
mock_config = MagicMock()
mock_client = AsyncMock()
resp = MagicMock()
resp.status = 500
mock_client.get.side_effect = CustomClientHTTPError(resp, message="boom")
mock_config.source_client = mock_client
dashboards = Dashboards(mock_config)

with pytest.raises(CustomClientHTTPError):
asyncio.run(dashboards.import_resource(_id="abc-def-ghi"))

def test_import_resource_id_without_explicit_id_uses_resource_id(self):
"""import_resource() with only a resource dict derives the id from
resource['id'] (the legacy list-path shape) — pins that both call
shapes produce a consistent _id."""
mock_config = MagicMock()
mock_client = AsyncMock()
body = {"id": "abc-def-ghi", "title": "t", "widgets": []}
mock_client.get.return_value = body
mock_config.source_client = mock_client
dashboards = Dashboards(mock_config)

_id, _ = asyncio.run(dashboards.import_resource(_id=None, resource={"id": "abc-def-ghi"}))

assert _id == "abc-def-ghi"
7 changes: 4 additions & 3 deletions tests/unit/test_get_resources_by_ids_experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,10 @@ def test_id_file_allowlist_rejects_unsupported_type(tmp_path, monkeypatch):
from datadog_sync.utils.configuration import _parse_id_file
from unittest.mock import MagicMock

# Write a payload with an unsupported type
# Write a payload with an unsupported type ('notebooks' is not in
# _ID_FILE_SUPPORTED_TYPES; 'dashboards' is now supported).
payload_path = tmp_path / "ids.json"
payload_path.write_text(json.dumps({"dashboards": ["abc-def-ghi"]}))
payload_path.write_text(json.dumps({"notebooks": ["abc-def-ghi"]}))

logger = MagicMock()
# _parse_id_file calls sys.exit(1) on validation failure. Patch to raise instead.
Expand All @@ -329,7 +330,7 @@ def test_id_file_allowlist_rejects_unsupported_type(tmp_path, monkeypatch):
assert excinfo.value.code == 1
# logger.error should have been called with a message naming the unsupported type
error_calls = [str(call) for call in logger.error.call_args_list]
assert any("dashboards" in c for c in error_calls), error_calls
assert any("notebooks" in c for c in error_calls), error_calls


def test_id_file_allowlist_accepts_monitors(tmp_path):
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_id_file_state_load_scoping.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def test_unsupported_type_still_rejected(self, tmp_path):
from datadog_sync.utils.configuration import _parse_id_file
import logging

payload_path = _write_payload(tmp_path, {"dashboards": ["dash-1"]})
payload_path = _write_payload(tmp_path, {"notebooks": ["nb-1"]})
with pytest.raises(SystemExit):
_parse_id_file(str(payload_path), logging.getLogger("test"))

Expand Down
8 changes: 5 additions & 3 deletions tests/unit/test_id_file_subprocess_experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,9 @@ def test_subprocess_stdin_payload_parsed(tmp_path):
@pytest.mark.experiment_subprocess
def test_subprocess_unsupported_type_in_id_file(tmp_path):
"""--id-file with unsupported type errors at config-build."""
payload = json.dumps({"dashboards": ["abc-def-ghi"]})
# 'notebooks' is not in _ID_FILE_IMPORT_SUPPORTED_TYPES; 'dashboards' is
# now supported, so it can no longer serve as the unsupported-type example.
payload = json.dumps({"notebooks": ["abc-def-ghi"]})
source_dir = tmp_path / "source"
# Use any URL — the subprocess shouldn't even reach the network.
rc, stdout, stderr = _run_sync_cli(
Expand All @@ -402,8 +404,8 @@ def test_subprocess_unsupported_type_in_id_file(tmp_path):
assert rc == 1, f"expected exit 1, got {rc}\nSTDERR:\n{stderr.decode(errors='replace')}"
combined = (stdout + stderr).decode(errors="replace")
assert (
"dashboards" in combined and "not supported" in combined.lower()
), f"expected error message mentioning 'dashboards' and 'not supported', got:\n{combined}"
"notebooks" in combined and "not supported" in combined.lower()
), f"expected error message mentioning 'notebooks' and 'not supported', got:\n{combined}"


# --- --id-file requires --resources ---
Expand Down
Loading