diff --git a/datadog_sync/model/dashboards.py b/datadog_sync/model/dashboards.py index a5189d43..101d4246 100644 --- a/datadog_sync/model/dashboards.py +++ b/datadog_sync/model/dashboards.py @@ -4,14 +4,24 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations +from copy import deepcopy from typing import TYPE_CHECKING, Optional, List, Dict, Tuple, cast from datadog_sync.utils.base_resource import BaseResource, ResourceConfig -from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource +from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource, check_diff, prep_resource if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient +# Substrings we treat as "destination refuses this update because the sync +# identity cannot edit this dashboard in place". Match must be specific +# enough that a generic 403 (missing scope, RBAC failure, edge/WAF) +# does not fall through to the clone path. +_READ_ONLY_MARKERS = ( + "This dashboard is read-only", + "user does not have permission to edit", +) + class Dashboards(BaseResource): resource_type = "dashboards" @@ -92,13 +102,70 @@ async def create_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]: async def update_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]: destination_client = self.config.destination_client - resp = await destination_client.put( - self.resource_config.base_path + f"/{self.config.state.destination[self.resource_type][_id]['id']}", - resource, - ) + destination_id = self.config.state.destination[self.resource_type][_id]["id"] + try: + resp = await destination_client.put( + self.resource_config.base_path + f"/{destination_id}", + resource, + ) + except CustomClientHTTPError as e: + if e.status_code == 403 and self._is_read_only_conflict(e): + return await self._handle_read_only_conflict(_id, resource, destination_id) + raise return _id, resp + @staticmethod + def _is_read_only_conflict(err: CustomClientHTTPError) -> bool: + # PUT returns 403 with one of the messages in _READ_ONLY_MARKERS when + # the destination copy cannot be edited in place. The markers are + # deliberately specific to avoid swallowing unrelated 403s (RBAC, + # missing scope, edge/WAF). + body = str(err) + return any(marker in body for marker in _READ_ONLY_MARKERS) + + async def _handle_read_only_conflict( + self, _id: str, resource: Dict, destination_id: str + ) -> Tuple[str, Dict]: + destination_client = self.config.destination_client + destination_state = self.config.state.destination[self.resource_type][_id] + description = destination_state.get("description") or "" + is_suggested_preset = "[[suggested_dashboards]]" in description + + if is_suggested_preset and self._suggested_preset_matches(resource, destination_state): + # Datadog auto-provisions "suggested" dashboards when an integration + # is installed. If source content matches the preset byte-for-byte + # in the sync-relevant fields, the update is a no-op — skipping is + # safe. If the customer forked the preset, fall through to clone. + raise SkipResource( + _id, + self.resource_type, + "Destination is an unmodified Datadog-suggested dashboard preset; source content matches.", + ) + + clone_resp = await destination_client.post(self.resource_config.base_path, resource) + new_destination_id = clone_resp.get("id") if isinstance(clone_resp, dict) else None + self.config.logger.warning( + f"dashboard {_id}: destination copy is read-only for the sync identity; " + f"cloned as a new dashboard. old dst id={destination_id} new dst id={new_destination_id}. " + f"The old copy is left in place on destination and is no longer referenced by sync state.", + ) + return _id, clone_resp + + @classmethod + def _suggested_preset_matches(cls, source: Dict, destination_state: Dict) -> bool: + # Use the same predicate the sync handler uses to decide whether an + # update is needed at all (resources_handler._apply_resource_cb). A + # partial field enumeration would miss dashboard-shape fields like + # layout_type / reflow_type / restricted_roles and cause a + # false-match skip that drops customer edits on the floor. If the + # handler saw a diff (that's why update_resource was invoked), and + # check_diff still sees a diff after prep, the customer forked the + # preset — fall through to clone. + destination_copy = deepcopy(destination_state) + prep_resource(cls.resource_config, destination_copy) + return not check_diff(cls.resource_config, source, destination_copy) + async def delete_resource(self, _id: str) -> None: destination_client = self.config.destination_client await destination_client.delete( diff --git a/tests/unit/test_dashboards.py b/tests/unit/test_dashboards.py index 72029565..a0932065 100644 --- a/tests/unit/test_dashboards.py +++ b/tests/unit/test_dashboards.py @@ -6,14 +6,50 @@ """ Unit tests for dashboards resource configuration and short-circuit. -Pins the list_omitted_attr_prefixes opt-in and the prefetched-body -short-circuit shared with notebooks. +Pins the list_omitted_attr_prefixes opt-in, the prefetched-body +short-circuit shared with notebooks, and the read-only-conflict +clone-on-update fallback. """ import asyncio from unittest.mock import AsyncMock, MagicMock +import pytest + from datadog_sync.model.dashboards import Dashboards +from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource + + +def _read_only_error() -> CustomClientHTTPError: + resp = MagicMock() + resp.status = 403 + resp.message = "Forbidden" + return CustomClientHTTPError(resp, message='{"errors": ["This dashboard is read-only"]}') + + +def _user_no_permission_error() -> CustomClientHTTPError: + resp = MagicMock() + resp.status = 403 + resp.message = "Forbidden" + return CustomClientHTTPError( + resp, + message=( + '{"errors":[{"status":"403","title":"Forbidden",' + '"detail":"user does not have permission to edit this dashboard"}]}' + ), + ) + + +def _unrelated_403_error() -> CustomClientHTTPError: + resp = MagicMock() + resp.status = 403 + resp.message = "Forbidden" + return CustomClientHTTPError( + resp, + # Generic "does not have permission" wording that must NOT match — the + # marker is "user does not have permission to edit", not just "permission". + message='{"errors":["user does not have permission for widget datasource"]}', + ) def test_resource_config_list_omitted_attr_prefixes(): @@ -53,3 +89,199 @@ def test_import_resource_short_circuits_when_caller_supplies_full_body(): mock_config.source_client.get.assert_not_awaited() assert _id == "abc-123" assert resource["widgets"] + + +def _make_dashboards_with_destination(dst_state: dict) -> Dashboards: + mock_config = MagicMock() + mock_config.state = MagicMock() + mock_config.state.destination = {"dashboards": {"abc-123": dst_state}} + mock_config.destination_client = AsyncMock() + mock_config.logger = MagicMock() + dashboards = Dashboards(mock_config) + return dashboards + + +class TestDashboardsReadOnlyConflict: + """Coverage for the PUT-403-read-only fallback path introduced in T29. + + When the destination refuses the update because the copy on that side is + read-only for the sync identity, we fall back to POST (clone) so the + customer's source content still lands on destination. If the destination + is an unmodified Datadog-suggested preset AND source matches it byte-for- + byte on the sync-relevant fields, we SkipResource instead. + """ + + def test_update_falls_back_to_clone_on_read_only_error(self): + """PUT returns 403 'This dashboard is read-only' -> POST is issued + with the source body and its response is returned as the new state.""" + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error()) + dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new", "title": "src"}) + source = {"title": "src", "widgets": [], "tags": []} + + _id, resp = asyncio.run(dashboards.update_resource("abc-123", source)) + + dashboards.config.destination_client.put.assert_awaited_once() + dashboards.config.destination_client.post.assert_awaited_once_with("/api/v1/dashboard", source) + assert _id == "abc-123" + assert resp == {"id": "dst-new", "title": "src"} + + def test_update_falls_back_to_clone_on_user_permission_error(self): + """The alternate JSON:API-shaped 403 body triggers the same clone path.""" + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + dashboards.config.destination_client.put = AsyncMock(side_effect=_user_no_permission_error()) + dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"}) + source = {"title": "src", "widgets": []} + + _id, resp = asyncio.run(dashboards.update_resource("abc-123", source)) + + dashboards.config.destination_client.post.assert_awaited_once() + assert resp["id"] == "dst-new" + + def test_update_reraises_unrelated_403(self): + """A 403 that is not the read-only class must not silently clone — + the caller upstream needs to see it as a real failure.""" + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + err = _unrelated_403_error() + dashboards.config.destination_client.put = AsyncMock(side_effect=err) + dashboards.config.destination_client.post = AsyncMock() + + with pytest.raises(CustomClientHTTPError): + asyncio.run(dashboards.update_resource("abc-123", {"title": "src"})) + dashboards.config.destination_client.post.assert_not_awaited() + + def test_update_reraises_generic_permission_403_without_edit_marker(self): + """A 403 whose body says 'does not have permission' but WITHOUT the + read-only edit marker must propagate. Pins the narrowed marker so a + future change back to a broad substring doesn't silently start + cloning dashboards for unrelated RBAC / scope failures.""" + resp = MagicMock() + resp.status = 403 + resp.message = "Forbidden" + err = CustomClientHTTPError( + resp, + message='{"errors":["scope check failed: does not have permission"]}', + ) + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + dashboards.config.destination_client.put = AsyncMock(side_effect=err) + dashboards.config.destination_client.post = AsyncMock() + + with pytest.raises(CustomClientHTTPError): + asyncio.run(dashboards.update_resource("abc-123", {"title": "src"})) + dashboards.config.destination_client.post.assert_not_awaited() + + def test_suggested_preset_matching_source_is_skipped(self): + """Unmodified suggested-preset on destination with matching source + content across all diffed fields: skip. Nothing to sync.""" + dst_state = { + "id": "dst-old", + "description": "some intro [[suggested_dashboards]]", + "title": "preset title", + "widgets": [{"definition": {"type": "timeseries"}}], + "template_variables": [], + "tags": [], + "layout_type": "ordered", + "reflow_type": "auto", + } + source = { + "title": "preset title", + "widgets": [{"definition": {"type": "timeseries"}}], + "template_variables": [], + "tags": [], + "description": "some intro [[suggested_dashboards]]", + "layout_type": "ordered", + "reflow_type": "auto", + } + dashboards = _make_dashboards_with_destination(dst_state) + dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error()) + dashboards.config.destination_client.post = AsyncMock() + + with pytest.raises(SkipResource) as exc: + asyncio.run(dashboards.update_resource("abc-123", source)) + assert "suggested" in str(exc.value).lower() + dashboards.config.destination_client.post.assert_not_awaited() + + def test_suggested_preset_with_layout_type_divergence_is_cloned(self): + """Regression pin: the preset-match predicate must not restrict itself + to a hand-picked subset of fields. If source diverges from destination + on layout_type (a dashboard-shape field), the customer forked the + preset — clone, do not skip.""" + dst_state = { + "id": "dst-old", + "description": "[[suggested_dashboards]]", + "title": "preset title", + "widgets": [], + "template_variables": [], + "tags": [], + "layout_type": "ordered", + "reflow_type": "auto", + } + dashboards = _make_dashboards_with_destination(dst_state) + dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error()) + dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"}) + source = { + "title": "preset title", + "widgets": [], + "template_variables": [], + "tags": [], + "description": "[[suggested_dashboards]]", + # layout_type changed on source; the old five-field match would + # incorrectly declare "unchanged" and skip. check_diff catches it. + "layout_type": "free", + "reflow_type": "auto", + } + + _id, resp = asyncio.run(dashboards.update_resource("abc-123", source)) + dashboards.config.destination_client.post.assert_awaited_once() + assert resp["id"] == "dst-new" + + def test_suggested_preset_with_diverged_source_is_cloned(self): + """Source customized on top of a suggested preset: clone, don't skip + (would otherwise lose customer edits).""" + dst_state = { + "id": "dst-old", + "description": "[[suggested_dashboards]]", + "title": "preset title", + "widgets": [], + "template_variables": [], + "tags": [], + } + dashboards = _make_dashboards_with_destination(dst_state) + dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error()) + dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"}) + source = { + "title": "preset title", + "widgets": [{"definition": {"type": "timeseries"}}], # customer added a widget + "template_variables": [], + "tags": [], + "description": "[[suggested_dashboards]]", + } + + _id, resp = asyncio.run(dashboards.update_resource("abc-123", source)) + dashboards.config.destination_client.post.assert_awaited_once_with("/api/v1/dashboard", source) + assert resp["id"] == "dst-new" + + def test_read_only_conflict_with_no_description_still_clones(self): + """If destination state has no `description` field, the + suggested-preset check must not crash and must clone.""" + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error()) + dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"}) + source = {"title": "src"} + + _id, resp = asyncio.run(dashboards.update_resource("abc-123", source)) + assert resp["id"] == "dst-new" + + def test_non_403_error_reraised(self): + """A 500 during update propagates as-is (no clone attempt).""" + dashboards = _make_dashboards_with_destination({"id": "dst-old"}) + resp = MagicMock() + resp.status = 500 + resp.message = "Internal Server Error" + err = CustomClientHTTPError(resp, message="boom") + dashboards.config.destination_client.put = AsyncMock(side_effect=err) + dashboards.config.destination_client.post = AsyncMock() + + with pytest.raises(CustomClientHTTPError): + asyncio.run(dashboards.update_resource("abc-123", {"title": "src"})) + dashboards.config.destination_client.post.assert_not_awaited()