From 6630c1ec337ec8a52eeedcf018bfb498fa84dee5 Mon Sep 17 00:00:00 2001 From: michael-richey Date: Tue, 14 Jul 2026 17:15:08 -0400 Subject: [PATCH] fix(dashboards,synthetics_private_locations): drop stale restricted_roles under new flag Completes the rollout by applying the flat-list drop-aware filter to the two remaining resource types with a `restricted_roles` list: - dashboards: `restricted_roles` - synthetics_private_locations: `metadata.restricted_roles` Both previously had trivial pass-through `connect_id` overrides; they now get a `connect_resources` override that filters stale roles under `--drop-unresolvable-principals` while keeping widget/other connections on the generic path and preserving the empty-list access-elevation hard-fail. Adds a new unit test module for synthetics_private_locations. Co-Authored-By: Claude Sonnet 5 --- datadog_sync/model/dashboards.py | 36 +++++++- .../model/synthetics_private_locations.py | 28 +++++- tests/unit/test_dashboards.py | 66 +++++++++++++- .../unit/test_synthetics_private_locations.py | 86 +++++++++++++++++++ 4 files changed, 213 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_synthetics_private_locations.py diff --git a/datadog_sync/model/dashboards.py b/datadog_sync/model/dashboards.py index 101d4246..2d80b98e 100644 --- a/datadog_sync/model/dashboards.py +++ b/datadog_sync/model/dashboards.py @@ -4,11 +4,18 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations +from collections import defaultdict 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, check_diff, prep_resource +from datadog_sync.utils.resource_utils import ( + CustomClientHTTPError, + SkipResource, + check_diff, + find_attr, + prep_resource, +) if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient @@ -172,5 +179,32 @@ async def delete_resource(self, _id: str) -> None: self.resource_config.base_path + f"/{self.config.state.destination[self.resource_type][_id]['id']}" ) + def connect_resources(self, _id: str, resource: Dict) -> bool: + """Drop-aware override. + + Widget connections (monitor alert_ids, powerpacks, slos) keep the generic + find_attr/connect_id path. The flat `restricted_roles` list goes through the shared + drop-aware filter so a permanently-stale role can be dropped (under + --drop-unresolvable-principals) while an emptied list still hard-fails as an + access-elevation guard. + """ + if not self.resource_config.resource_connections: + return False + + failed_connections_dict = defaultdict(list) + for resource_to_connect, attrs in self.resource_config.resource_connections.items(): + for attr_connection in attrs: + if attr_connection == "restricted_roles": + continue # handled by the drop-aware filter below + c = find_attr(attr_connection, resource_to_connect, resource, self.connect_id) + if c: + failed_connections_dict[resource_to_connect].extend(c) + + role_failed, roles_risk = self._filter_stale_flat_roles(_id, resource, "restricted_roles") + if role_failed: + failed_connections_dict["roles"].extend(role_failed) + + return self._raise_connection_error_if_any(_id, failed_connections_dict, roles_risk) + def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: return super(Dashboards, self).connect_id(key, r_obj, resource_to_connect) diff --git a/datadog_sync/model/synthetics_private_locations.py b/datadog_sync/model/synthetics_private_locations.py index dadca4a2..c1f7d9b8 100644 --- a/datadog_sync/model/synthetics_private_locations.py +++ b/datadog_sync/model/synthetics_private_locations.py @@ -6,11 +6,12 @@ from __future__ import annotations import json import re +from collections import defaultdict from typing import TYPE_CHECKING, List, Dict, Optional, Tuple from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, TaggingConfig -from datadog_sync.utils.resource_utils import SkipResource +from datadog_sync.utils.resource_utils import SkipResource, find_attr if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient @@ -102,5 +103,30 @@ async def delete_resource(self, _id: str) -> None: self.resource_config.base_path + f"/{self.config.state.destination[self.resource_type][_id]['id']}" ) + def connect_resources(self, _id: str, resource: Dict) -> bool: + """Drop-aware override for the flat `metadata.restricted_roles` list. + + A permanently-stale role can be dropped (under --drop-unresolvable-principals) while + an emptied list still hard-fails as an access-elevation guard. Any future + non-restricted_roles connection keeps the generic find_attr/connect_id path. + """ + if not self.resource_config.resource_connections: + return False + + failed_connections_dict = defaultdict(list) + for resource_to_connect, attrs in self.resource_config.resource_connections.items(): + for attr_connection in attrs: + if attr_connection == "metadata.restricted_roles": + continue # handled by the drop-aware filter below + c = find_attr(attr_connection, resource_to_connect, resource, self.connect_id) + if c: + failed_connections_dict[resource_to_connect].extend(c) + + role_failed, roles_risk = self._filter_stale_flat_roles(_id, resource.get("metadata"), "restricted_roles") + if role_failed: + failed_connections_dict["roles"].extend(role_failed) + + return self._raise_connection_error_if_any(_id, failed_connections_dict, roles_risk) + def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: return super(SyntheticsPrivateLocations, self).connect_id(key, r_obj, resource_to_connect) diff --git a/tests/unit/test_dashboards.py b/tests/unit/test_dashboards.py index a0932065..1b2faa01 100644 --- a/tests/unit/test_dashboards.py +++ b/tests/unit/test_dashboards.py @@ -12,12 +12,14 @@ """ import asyncio +from collections import defaultdict from unittest.mock import AsyncMock, MagicMock import pytest from datadog_sync.model.dashboards import Dashboards -from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource +from datadog_sync.utils.resource_utils import CustomClientHTTPError, ResourceConnectionError, SkipResource +from datadog_sync.utils.workers import Counter def _read_only_error() -> CustomClientHTTPError: @@ -285,3 +287,65 @@ def test_non_403_error_reraised(self): with pytest.raises(CustomClientHTTPError): asyncio.run(dashboards.update_resource("abc-123", {"title": "src"})) dashboards.config.destination_client.post.assert_not_awaited() + + +class TestDashboardsConnectResourcesDrop: + """connect_resources drop/keep/hard-fail for dashboards' flat `restricted_roles`.""" + + def _make_dashboard(self, drop=False, skip_failed=False): + config = MagicMock() + config.state = MagicMock() + config.state.source = defaultdict(dict) + config.state.destination = defaultdict(dict) + config.state.ensure_resource_loaded = MagicMock() + config.drop_unresolvable_principals = drop + config.skip_failed_resource_connections = skip_failed + config.counter = Counter() + config.logger = MagicMock() + return Dashboards(config) + + def _seed_valid_role(self, d, src="role-good", dst="role-good-dst"): + d.config.state.destination["roles"][src] = {"id": dst} + + def test_flag_off_stale_role_hard_fails(self): + d = self._make_dashboard(drop=False) + resource = {"id": "abc-def-ghi", "restricted_roles": ["role-gone"]} + with pytest.raises(ResourceConnectionError): + d.connect_resources("abc-def-ghi", resource) + assert resource["restricted_roles"] == ["role-gone"] # not dropped when flag off + + def test_flag_on_drops_stale_keeps_valid(self): + d = self._make_dashboard(drop=True) + self._seed_valid_role(d) + resource = {"id": "abc-def-ghi", "restricted_roles": ["role-good", "role-gone"]} + d.connect_resources("abc-def-ghi", resource) # no raise + assert resource["restricted_roles"] == ["role-good-dst"] + assert d.config.counter.stale_principals_dropped_by_type["dashboards"] == ["abc-def-ghi"] + + def test_flag_on_all_stale_empty_list_raises_risk(self): + d = self._make_dashboard(drop=True) + resource = {"id": "abc-def-ghi", "restricted_roles": ["role-gone"]} + with pytest.raises(ResourceConnectionError) as exc_info: + d.connect_resources("abc-def-ghi", resource) + assert exc_info.value.empty_binding_risk is True + assert resource["restricted_roles"] == [] + + def test_empty_list_risk_is_returned_when_connection_failure_is_suppressed(self): + d = self._make_dashboard(drop=True, skip_failed=True) + resource = {"id": "abc-def-ghi", "restricted_roles": ["role-gone"]} + + assert d.connect_resources("abc-def-ghi", resource) is True + assert resource["restricted_roles"] == [] + + def test_flag_on_middle_drop_keeps_neighbors(self): + d = self._make_dashboard(drop=True) + self._seed_valid_role(d, src="ra", dst="ra-dst") + self._seed_valid_role(d, src="rc", dst="rc-dst") + resource = {"id": "abc", "restricted_roles": ["ra", "role-gone", "rc"]} + d.connect_resources("abc", resource) # no raise + assert resource["restricted_roles"] == ["ra-dst", "rc-dst"] + + def test_no_restricted_roles_is_noop(self): + d = self._make_dashboard(drop=True) + resource = {"id": "abc"} # no restricted_roles at all + d.connect_resources("abc", resource) # no raise, nothing to do diff --git a/tests/unit/test_synthetics_private_locations.py b/tests/unit/test_synthetics_private_locations.py new file mode 100644 index 00000000..57ee53ca --- /dev/null +++ b/tests/unit/test_synthetics_private_locations.py @@ -0,0 +1,86 @@ +# 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. + +"""Unit tests for synthetics_private_locations connect_resources drop behavior. + +The only access-control connection is the flat `metadata.restricted_roles` list. Under +--drop-unresolvable-principals a permanently-stale role is dropped; an emptied list still +hard-fails as an access-elevation guard. +""" + +from collections import defaultdict +from unittest.mock import MagicMock + +import pytest + +from datadog_sync.model.synthetics_private_locations import SyntheticsPrivateLocations +from datadog_sync.utils.resource_utils import ResourceConnectionError +from datadog_sync.utils.workers import Counter + + +def _make_spl(drop=False, skip_failed=False): + config = MagicMock() + config.state = MagicMock() + config.state.source = defaultdict(dict) + config.state.destination = defaultdict(dict) + config.state.ensure_resource_loaded = MagicMock() + config.drop_unresolvable_principals = drop + config.skip_failed_resource_connections = skip_failed + config.counter = Counter() + config.logger = MagicMock() + return SyntheticsPrivateLocations(config) + + +def _seed_valid_role(spl, src="role-good", dst="role-good-dst"): + spl.config.state.destination["roles"][src] = {"id": dst} + + +def test_flag_off_stale_role_hard_fails(): + spl = _make_spl(drop=False) + resource = {"id": "pl:abc", "metadata": {"restricted_roles": ["role-gone"]}} + with pytest.raises(ResourceConnectionError): + spl.connect_resources("pl:abc", resource) + assert resource["metadata"]["restricted_roles"] == ["role-gone"] # not dropped when off + + +def test_flag_on_drops_stale_keeps_valid(): + spl = _make_spl(drop=True) + _seed_valid_role(spl) + resource = {"id": "pl:abc", "metadata": {"restricted_roles": ["role-good", "role-gone"]}} + spl.connect_resources("pl:abc", resource) # no raise + assert resource["metadata"]["restricted_roles"] == ["role-good-dst"] + assert spl.config.counter.stale_principals_dropped_by_type["synthetics_private_locations"] == ["pl:abc"] + + +def test_flag_on_all_stale_empty_list_raises_risk(): + spl = _make_spl(drop=True) + resource = {"id": "pl:abc", "metadata": {"restricted_roles": ["role-gone"]}} + with pytest.raises(ResourceConnectionError) as exc_info: + spl.connect_resources("pl:abc", resource) + assert exc_info.value.empty_binding_risk is True + assert resource["metadata"]["restricted_roles"] == [] + + +def test_empty_list_risk_is_returned_when_connection_failure_is_suppressed(): + spl = _make_spl(drop=True, skip_failed=True) + resource = {"id": "pl:abc", "metadata": {"restricted_roles": ["role-gone"]}} + + assert spl.connect_resources("pl:abc", resource) is True + assert resource["metadata"]["restricted_roles"] == [] + + +def test_flag_on_middle_drop_keeps_neighbors(): + spl = _make_spl(drop=True) + _seed_valid_role(spl, src="ra", dst="ra-dst") + _seed_valid_role(spl, src="rc", dst="rc-dst") + resource = {"id": "pl:abc", "metadata": {"restricted_roles": ["ra", "role-gone", "rc"]}} + spl.connect_resources("pl:abc", resource) # no raise + assert resource["metadata"]["restricted_roles"] == ["ra-dst", "rc-dst"] + + +def test_no_metadata_is_noop(): + spl = _make_spl(drop=True) + resource = {"id": "pl:abc"} # no metadata/restricted_roles + spl.connect_resources("pl:abc", resource) # no raise