From 6213034116dea0b8f304a067b14336762e66aeea Mon Sep 17 00:00:00 2001 From: michael-richey Date: Tue, 14 Jul 2026 17:14:41 -0400 Subject: [PATCH 1/2] fix(restriction_policies): drop stale principals instead of skipping the whole policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Overrides `connect_resources` so that, under `--drop-unresolvable-principals`, a principal absent from both destination and source state (permanently gone — e.g. a role deleted before the org's first-ever import) is dropped from its binding and the policy still syncs, instead of the entire restriction policy being skipped on a single dead reference. - Principals present in source but not destination keep today's hard-fail/retry behavior. - If dropping empties a binding whose source list was non-empty, the resource is still skipped and flagged as an access-elevation risk (ERROR + metric), governed by `--skip-failed-resource-connections` like any other connection failure. - The "id" (dashboard/slo/notebook) connections keep the generic path unchanged. - `extract_source_ids` is intentionally left unaffected (documented) so minimize-reads lazy-loading can still resolve which principals to check. Co-Authored-By: Claude Sonnet 5 --- datadog_sync/model/restriction_policies.py | 46 ++++- tests/unit/test_restriction_policies.py | 199 +++++++++++++++++++++ 2 files changed, 243 insertions(+), 2 deletions(-) diff --git a/datadog_sync/model/restriction_policies.py b/datadog_sync/model/restriction_policies.py index 392c4733..992cb43f 100644 --- a/datadog_sync/model/restriction_policies.py +++ b/datadog_sync/model/restriction_policies.py @@ -4,10 +4,15 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations +from collections import defaultdict from typing import TYPE_CHECKING, Optional, List, Dict, Tuple -from datadog_sync.utils.base_resource import BaseResource, ResourceConfig -from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource +from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, ResourceConnectionResult +from datadog_sync.utils.resource_utils import ( + CustomClientHTTPError, + SkipResource, + find_attr, +) if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient @@ -147,6 +152,38 @@ 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) -> ResourceConnectionResult: + """Drop-aware override. + + The "id" connections (dashboards/slos/notebooks) keep the generic + find_attr/connect_id hard-fail path. The bindings' composite principals go through + the shared per-binding filter so that principals permanently absent from source can + be dropped (under --drop-unresolvable-principals) instead of failing the whole + policy, while an emptied binding still hard-fails as an access-elevation guard. + """ + if not self.resource_config.resource_connections: + return ResourceConnectionResult() + + 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 == "attributes.bindings.principals": + continue # handled per-binding below + c = find_attr(attr_connection, resource_to_connect, resource, self.connect_id) + if c: + failed_connections_dict[resource_to_connect].extend(c) + + bindings = (resource.get("attributes") or {}).get("bindings") + principal_failed, empty_binding_risk = self._filter_stale_binding_principals(_id, bindings) + for rt, ids in principal_failed.items(): + failed_connections_dict[rt].extend(ids) + + return ResourceConnectionResult( + empty_binding_escalation=self._raise_connection_error_if_any( + _id, failed_connections_dict, empty_binding_risk + ) + ) + def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: dashboards = self.config.state.destination["dashboards"] slos = self.config.state.destination["service_level_objectives"] @@ -198,6 +235,11 @@ def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optiona def extract_source_ids(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: # Mirror of connect_id -- keep in sync when connect_id changes. + # Intentionally UNAFFECTED by --drop-unresolvable-principals: this must keep + # returning every referenced id (including ones connect_resources will later drop), + # because --minimize-reads lazy-loading relies on it to decide WHICH principals to + # look up in source in order to make the drop decision. Do NOT "fix" this to match + # connect_resources' filtering. if key == "id": _type, _id = r_obj[key].split(":", 1) type_map = {"dashboard": "dashboards", "slo": "service_level_objectives", "notebook": "notebooks"} diff --git a/tests/unit/test_restriction_policies.py b/tests/unit/test_restriction_policies.py index ebb42fdd..b916ba14 100644 --- a/tests/unit/test_restriction_policies.py +++ b/tests/unit/test_restriction_policies.py @@ -11,10 +11,13 @@ """ import asyncio +from collections import defaultdict import pytest from unittest.mock import AsyncMock, MagicMock from datadog_sync.model.restriction_policies import RestrictionPolicies +from datadog_sync.utils.resource_utils import ResourceConnectionError +from datadog_sync.utils.workers import Counter class TestRestrictionPoliciesOrgPrincipal: @@ -99,3 +102,199 @@ def test_pre_resource_action_hook_skips_when_org_principal_none(self): assert r["attributes"]["bindings"][0]["principals"][0] == "org:source-pub-id" assert r["attributes"]["bindings"][0]["principals"][1] == "user:some-user" + + +class TestRestrictionPoliciesConnectResources: + """connect_resources drop/keep/hard-fail behavior for --drop-unresolvable-principals. + + 'stale' == absent from BOTH destination and source (permanently gone). + 'pending' == present in source, absent from destination ('not yet synced'). + 'valid' == present in destination (resolves and remaps). + """ + + def _make_resource(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() + resource = RestrictionPolicies(config) + # Seed the primary "id" connection so it always resolves; keeps these tests + # focused on principal handling rather than the dashboard id link. + config.state.destination["dashboards"]["dash-src"] = {"id": "dash-dst"} + return resource + + def _seed_valid_role(self, resource, src="role-good", dst="role-good-dst"): + resource.config.state.destination["roles"][src] = {"id": dst} + + def _seed_pending_role(self, resource, src="role-pending"): + resource.config.state.source["roles"][src] = {"id": src} + + def _policy(self, bindings): + return {"id": "dashboard:dash-src", "attributes": {"bindings": bindings}} + + # --- flag OFF: byte-for-byte unchanged behavior -------------------------------- + + def test_flag_off_stale_principal_hard_fails(self): + resource = self._make_resource(drop=False) + policy = self._policy([{"principals": ["role:role-gone"], "relation": "editor"}]) + + with pytest.raises(ResourceConnectionError): + resource.connect_resources("dashboard:dash-src", policy) + + resource.config.counter # no drop recorded + assert resource.config.counter.stale_principals_dropped_by_type == {} + + def test_flag_off_valid_principal_remaps_and_succeeds(self): + resource = self._make_resource(drop=False) + self._seed_valid_role(resource) + policy = self._policy([{"principals": ["role:role-good"], "relation": "editor"}]) + + resource.connect_resources("dashboard:dash-src", policy) # no raise + + assert policy["attributes"]["bindings"][0]["principals"] == ["role:role-good-dst"] + + # --- flag ON: drop stale, keep syncing ----------------------------------------- + + def test_flag_on_drops_stale_keeps_valid_and_syncs(self): + resource = self._make_resource(drop=True) + self._seed_valid_role(resource) + policy = self._policy([{"principals": ["role:role-good", "role:role-gone"], "relation": "editor"}]) + + resource.connect_resources("dashboard:dash-src", policy) # no raise + + assert policy["attributes"]["bindings"][0]["principals"] == ["role:role-good-dst"] + resource.config.logger.warning.assert_called() + assert resource.config.counter.stale_principals_dropped_by_type["restriction_policies"] == [ + "dashboard:dash-src" + ] + + def test_flag_on_pending_principal_still_hard_fails(self): + resource = self._make_resource(drop=True) + self._seed_pending_role(resource) # in source, not destination + policy = self._policy([{"principals": ["role:role-pending"], "relation": "editor"}]) + + with pytest.raises(ResourceConnectionError) as exc_info: + resource.connect_resources("dashboard:dash-src", policy) + + # Not an access-elevation case -- it's the legitimate retry-later path. + assert exc_info.value.empty_binding_risk is False + assert resource.config.counter.stale_principals_dropped_by_type == {} + + def test_flag_on_binding_emptied_raises_empty_binding_risk(self): + resource = self._make_resource(drop=True) + policy = self._policy([{"principals": ["role:role-gone"], "relation": "editor"}]) + + with pytest.raises(ResourceConnectionError) as exc_info: + resource.connect_resources("dashboard:dash-src", policy) + + assert exc_info.value.empty_binding_risk is True + resource.config.logger.error.assert_called() + assert policy["attributes"]["bindings"][0]["principals"] == [] + + def test_multiple_bindings_only_one_empties_still_raises(self): + resource = self._make_resource(drop=True) + self._seed_valid_role(resource) + policy = self._policy( + [ + {"principals": ["role:role-good"], "relation": "viewer"}, + {"principals": ["role:role-gone"], "relation": "editor"}, + ] + ) + + with pytest.raises(ResourceConnectionError) as exc_info: + resource.connect_resources("dashboard:dash-src", policy) + + assert exc_info.value.empty_binding_risk is True + # Resource-level skip: the surviving binding was still filtered/remapped in place. + assert policy["attributes"]["bindings"][0]["principals"] == ["role:role-good-dst"] + assert policy["attributes"]["bindings"][1]["principals"] == [] + + def test_skip_failed_resource_connections_suppresses_empty_binding_raise(self): + resource = self._make_resource(drop=True, skip_failed=True) + policy = self._policy([{"principals": ["role:role-gone"], "relation": "editor"}]) + + # skip_failed_resource_connections stays authoritative: no raise even for the + # empty-binding risk case. + connection_result = resource.connect_resources("dashboard:dash-src", policy) + + assert policy["attributes"]["bindings"][0]["principals"] == [] + assert connection_result.empty_binding_escalation is True + message = resource.config.logger.error.call_args.args[0] + assert "continuing sync" in message + assert "DESTINATION RESOURCE MAY BE UNRESTRICTED" in message + assert "refusing to sync" not in message + + def test_middle_principal_drop_does_not_skip_neighbors(self): + # Enumerate/index-shift regression: stale entry in the MIDDLE of 3. + resource = self._make_resource(drop=True) + self._seed_valid_role(resource, src="role-a", dst="role-a-dst") + self._seed_valid_role(resource, src="role-c", dst="role-c-dst") + policy = self._policy([{"principals": ["role:role-a", "role:role-gone", "role:role-c"], "relation": "editor"}]) + + resource.connect_resources("dashboard:dash-src", policy) # no raise + + # Both neighbors survive, in order; the middle stale one is gone. + assert policy["attributes"]["bindings"][0]["principals"] == [ + "role:role-a-dst", + "role:role-c-dst", + ] + + def test_extract_source_ids_unaffected_by_drop_logic(self): + # extract_source_ids must still surface an id that connect_resources would drop. + resource = self._make_resource(drop=True) + binding = {"principals": ["role:role-gone"], "relation": "editor"} + + assert resource.extract_source_ids("principals", binding, "roles") == ["role-gone"] + + def test_non_principal_id_connection_still_hard_fails(self): + # A dangling dashboard "id" link fails via the generic path regardless of the flag. + resource = self._make_resource(drop=True) + self._seed_valid_role(resource) + # Point at a dashboard id that is NOT in destination state. + policy = { + "id": "dashboard:missing-dash", + "attributes": {"bindings": [{"principals": ["role:role-good"], "relation": "editor"}]}, + } + + with pytest.raises(ResourceConnectionError) as exc_info: + resource.connect_resources("dashboard:missing-dash", policy) + + assert exc_info.value.empty_binding_risk is False + + def test_inert_when_off_end_to_end_raises_like_today(self): + # Full connect_resources with the flag off + a would-be-dropped principal must + # raise exactly as today (no silent drop). + resource = self._make_resource(drop=False) + policy = self._policy([{"principals": ["role:role-gone"], "relation": "editor"}]) + + with pytest.raises(ResourceConnectionError): + resource.connect_resources("dashboard:dash-src", policy) + + # Flag off => principal is NOT dropped; it stays in the (rebuilt) list. + assert policy["attributes"]["bindings"][0]["principals"] == ["role:role-gone"] + + def test_org_and_non_composite_principals_pass_through(self): + # org: principals (already remapped by the hook) and any non-user/role/team or + # colon-less token must pass through untouched; an empty-principals binding is skipped. + resource = self._make_resource(drop=True) + self._seed_valid_role(resource) + policy = self._policy( + [ + {"principals": ["org:dest-org", "weirdnoprefix", "role:role-good"], "relation": "editor"}, + {"principals": [], "relation": "viewer"}, + ] + ) + + resource.connect_resources("dashboard:dash-src", policy) # no raise + + assert policy["attributes"]["bindings"][0]["principals"] == [ + "org:dest-org", + "weirdnoprefix", + "role:role-good-dst", + ] + assert policy["attributes"]["bindings"][1]["principals"] == [] From 0f5e61a27121451cd433caaa508586faf236c204 Mon Sep 17 00:00:00 2001 From: Michael Richey <41595765+michael-richey@users.noreply.github.com> Date: Wed, 15 Jul 2026 13:44:24 -0400 Subject: [PATCH 2/2] fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag (3/4) (#631) * fix(monitors,synthetics_tests): drop stale restricted_roles/principals under new flag Applies the same drop-aware pattern (PR1 helpers) to the two resource types that carry both a flat `restricted_roles` list and a `restriction_policy.bindings.principals` composite: - monitors: `restricted_roles` + `restriction_policy.bindings.principals` - synthetics_tests: `options.restricted_roles` + `restriction_policy.bindings.principals` Each `connect_resources` override keeps all non-access-control connections on the generic path, filters access-control references through the shared helpers, and preserves the empty-binding/empty-list access-elevation hard-fail. `extract_source_ids` left unaffected (documented). Behavior is unchanged when `--drop-unresolvable-principals` is off. Co-Authored-By: Claude Sonnet 5 * fix(dashboards,synthetics_private_locations): drop stale restricted_roles under new flag (#632) 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 --------- Co-authored-by: Claude Sonnet 5 --- datadog_sync/model/dashboards.py | 42 ++++++- datadog_sync/model/monitors.py | 51 ++++++++- .../model/synthetics_private_locations.py | 34 +++++- datadog_sync/model/synthetics_tests.py | 50 ++++++++- tests/unit/test_dashboards.py | 66 ++++++++++- tests/unit/test_monitors.py | 103 +++++++++++++++++- .../unit/test_synthetics_private_locations.py | 86 +++++++++++++++ tests/unit/test_synthetics_tests.py | 91 +++++++++++++++- 8 files changed, 512 insertions(+), 11 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..a2a660ab 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.base_resource import BaseResource, ResourceConfig, ResourceConnectionResult +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,36 @@ 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) -> ResourceConnectionResult: + """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 ResourceConnectionResult() + + 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 ResourceConnectionResult( + empty_binding_escalation=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/monitors.py b/datadog_sync/model/monitors.py index 754c74e8..e8e86c27 100644 --- a/datadog_sync/model/monitors.py +++ b/datadog_sync/model/monitors.py @@ -4,14 +4,15 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations +from collections import defaultdict import logging import re from typing import TYPE_CHECKING, Optional, List, Dict, Tuple, cast from datadog_sync.constants import LOGGER_NAME -from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, TaggingConfig +from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, ResourceConnectionResult, TaggingConfig from datadog_sync.utils.custom_client import PaginationConfig -from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource +from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource, find_attr _log = logging.getLogger(LOGGER_NAME) @@ -238,6 +239,49 @@ async def delete_resource(self, _id: str) -> None: params={"force": "true"}, ) + def connect_resources(self, _id: str, resource: Dict) -> ResourceConnectionResult: + """Drop-aware override. + + query / composite-monitor / slo-alert connections keep the generic + find_attr/connect_id path. The two access-control shapes -- the flat + `restricted_roles` list and the `restriction_policy.bindings.principals` composites + -- go through the shared drop-aware filters so permanently-stale references can be + dropped (under --drop-unresolvable-principals) while an emptied binding/list still + hard-fails as an access-elevation guard. + """ + if not self.resource_config.resource_connections: + return ResourceConnectionResult() + + 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 in ("restricted_roles", "restriction_policy.bindings.principals"): + continue # handled by the drop-aware filters below + c = find_attr(attr_connection, resource_to_connect, resource, self.connect_id) + if c: + failed_connections_dict[resource_to_connect].extend(c) + + empty_risk = False + restriction_policy = resource.get("restriction_policy") + if restriction_policy: + principal_failed, binding_risk = self._filter_stale_binding_principals( + _id, restriction_policy.get("bindings") + ) + for rt, ids in principal_failed.items(): + failed_connections_dict[rt].extend(ids) + empty_risk = empty_risk or binding_risk + + role_failed, roles_risk = self._filter_stale_flat_roles(_id, resource, "restricted_roles") + if role_failed: + failed_connections_dict["roles"].extend(role_failed) + empty_risk = empty_risk or roles_risk + + return ResourceConnectionResult( + empty_binding_escalation=self._raise_connection_error_if_any( + _id, failed_connections_dict, empty_risk + ) + ) + def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: monitors = self.config.state.destination[resource_to_connect] @@ -308,6 +352,9 @@ def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optiona def extract_source_ids(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: # Mirror of connect_id -- keep in sync when connect_id changes. + # Intentionally UNAFFECTED by --drop-unresolvable-principals: must keep returning + # every referenced id (including ones connect_resources will later drop) so + # --minimize-reads lazy-loading can look them up in source to make the drop decision. if key == "query" and r_obj.get("type") == "composite" and resource_to_connect != "service_level_objectives": return re.findall("[0-9]+", r_obj[key]) elif key == "query" and resource_to_connect == "service_level_objectives" and r_obj.get("type") == "slo alert": diff --git a/datadog_sync/model/synthetics_private_locations.py b/datadog_sync/model/synthetics_private_locations.py index dadca4a2..a2544b68 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.base_resource import BaseResource, ResourceConfig, ResourceConnectionResult, TaggingConfig +from datadog_sync.utils.resource_utils import SkipResource, find_attr if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient @@ -102,5 +103,34 @@ 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) -> ResourceConnectionResult: + """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 ResourceConnectionResult() + + 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 ResourceConnectionResult( + empty_binding_escalation=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/datadog_sync/model/synthetics_tests.py b/datadog_sync/model/synthetics_tests.py index 39c65a5c..81d6485c 100644 --- a/datadog_sync/model/synthetics_tests.py +++ b/datadog_sync/model/synthetics_tests.py @@ -9,11 +9,13 @@ import certifi import json import ssl +from collections import defaultdict from copy import deepcopy from typing import TYPE_CHECKING, Optional, List, Dict, Tuple, cast from yarl import URL -from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, TaggingConfig +from datadog_sync.utils.base_resource import BaseResource, ResourceConfig, ResourceConnectionResult, TaggingConfig +from datadog_sync.utils.resource_utils import find_attr from datadog_sync.model.synthetics_mobile_applications_versions import SyntheticsMobileApplicationsVersions if TYPE_CHECKING: @@ -422,6 +424,49 @@ async def delete_resource(self, _id: str) -> None: dest_resource = self.config.state.destination[self.resource_type][_id] await self._delete_test(destination_client, dest_resource.get("type"), dest_resource["public_id"]) + def connect_resources(self, _id: str, resource: Dict) -> ResourceConnectionResult: + """Drop-aware override. + + All non-access-control connections (private locations, subtests, global variables, + rum/mobile apps) keep the generic find_attr/connect_id path. The flat + `options.restricted_roles` list and the `restriction_policy.bindings.principals` + composites go through the shared drop-aware filters so permanently-stale references + can be dropped (under --drop-unresolvable-principals) while an emptied binding/list + still hard-fails as an access-elevation guard. + """ + if not self.resource_config.resource_connections: + return ResourceConnectionResult() + + 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 in ("options.restricted_roles", "restriction_policy.bindings.principals"): + continue # handled by the drop-aware filters below + c = find_attr(attr_connection, resource_to_connect, resource, self.connect_id) + if c: + failed_connections_dict[resource_to_connect].extend(c) + + empty_risk = False + restriction_policy = resource.get("restriction_policy") + if restriction_policy: + principal_failed, binding_risk = self._filter_stale_binding_principals( + _id, restriction_policy.get("bindings") + ) + for rt, ids in principal_failed.items(): + failed_connections_dict[rt].extend(ids) + empty_risk = empty_risk or binding_risk + + role_failed, roles_risk = self._filter_stale_flat_roles(_id, resource.get("options"), "restricted_roles") + if role_failed: + failed_connections_dict["roles"].extend(role_failed) + empty_risk = empty_risk or roles_risk + + return ResourceConnectionResult( + empty_binding_escalation=self._raise_connection_error_if_any( + _id, failed_connections_dict, empty_risk + ) + ) + def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: failed_connections: List[str] = [] if resource_to_connect == "synthetics_private_locations": @@ -504,6 +549,9 @@ def connect_id(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optiona def extract_source_ids(self, key: str, r_obj: Dict, resource_to_connect: str) -> Optional[List[str]]: # Mirror of connect_id -- keep in sync when connect_id changes. + # Intentionally UNAFFECTED by --drop-unresolvable-principals: must keep returning + # every referenced id (including ones connect_resources will later drop) so + # --minimize-reads lazy-loading can look them up in source to make the drop decision. # Only synthetics_private_locations and mobile application versions need special handling. # rum_applications, synthetics_tests (subtests), synthetics_global_variables, roles, and # synthetics_mobile_applications (applicationId key) all use plain IDs at the leaf — diff --git a/tests/unit/test_dashboards.py b/tests/unit/test_dashboards.py index a0932065..c087870d 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).empty_binding_escalation 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_monitors.py b/tests/unit/test_monitors.py index c951246e..bdc7dc0a 100644 --- a/tests/unit/test_monitors.py +++ b/tests/unit/test_monitors.py @@ -11,11 +11,13 @@ """ import asyncio +from collections import defaultdict import pytest from unittest.mock import MagicMock from datadog_sync.model.monitors import Monitors -from datadog_sync.utils.resource_utils import SkipResource +from datadog_sync.utils.resource_utils import SkipResource, ResourceConnectionError +from datadog_sync.utils.workers import Counter class TestMonitorsPreResourceActionHook: @@ -635,3 +637,102 @@ def test_pre_apply_hook_skips_current_user_when_source_empty(self): asyncio.run(monitors.pre_apply_hook()) assert monitors.org_principal is None mock_client.get.assert_not_awaited() + + +class TestMonitorsConnectResourcesDrop: + """connect_resources drop/keep/hard-fail for monitors' access-control shapes: + the flat `restricted_roles` list and `restriction_policy.bindings.principals` composites. + 'stale' == absent from destination AND source; 'valid' == present in destination. + """ + + def _make_monitors(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 Monitors(config) + + def _seed_valid_role(self, m, src="role-good", dst="role-good-dst"): + m.config.state.destination["roles"][src] = {"id": dst} + + def test_restriction_policy_flag_off_stale_hard_fails(self): + m = self._make_monitors(drop=False) + resource = { + "id": 1, + "query": "avg(last_5m):sum:x{*} > 1", + "restriction_policy": {"bindings": [{"principals": ["role:role-gone"], "relation": "editor"}]}, + } + with pytest.raises(ResourceConnectionError): + m.connect_resources("1", resource) + + def test_restriction_policy_flag_on_drops_stale(self): + m = self._make_monitors(drop=True) + self._seed_valid_role(m) + resource = { + "id": 1, + "query": "avg(last_5m):sum:x{*} > 1", + "restriction_policy": { + "bindings": [{"principals": ["role:role-good", "role:role-gone"], "relation": "editor"}] + }, + } + m.connect_resources("1", resource) # no raise + assert resource["restriction_policy"]["bindings"][0]["principals"] == ["role:role-good-dst"] + assert m.config.counter.stale_principals_dropped_by_type["monitors"] == ["1"] + + def test_restriction_policy_empty_binding_raises_risk(self): + m = self._make_monitors(drop=True) + resource = { + "id": 1, + "restriction_policy": {"bindings": [{"principals": ["role:role-gone"], "relation": "editor"}]}, + } + with pytest.raises(ResourceConnectionError) as exc_info: + m.connect_resources("1", resource) + assert exc_info.value.empty_binding_risk is True + + def test_restricted_roles_flat_flag_on_drops_stale(self): + m = self._make_monitors(drop=True) + self._seed_valid_role(m, src="role-good", dst="role-good-dst") + resource = {"id": 1, "restricted_roles": ["role-good", "role-gone"]} + m.connect_resources("1", resource) # no raise + assert resource["restricted_roles"] == ["role-good-dst"] + + def test_restricted_roles_flat_all_stale_empty_risk(self): + m = self._make_monitors(drop=True) + resource = {"id": 1, "restricted_roles": ["role-gone"]} + with pytest.raises(ResourceConnectionError) as exc_info: + m.connect_resources("1", resource) + assert exc_info.value.empty_binding_risk is True + assert resource["restricted_roles"] == [] + + def test_restricted_roles_flat_empty_risk_is_returned_when_connection_failure_is_suppressed(self): + m = self._make_monitors(drop=True, skip_failed=True) + resource = {"id": 1, "restricted_roles": ["role-gone"]} + + assert m.connect_resources("1", resource).empty_binding_escalation is True + assert resource["restricted_roles"] == [] + + def test_restricted_roles_flat_middle_drop_keeps_neighbors(self): + m = self._make_monitors(drop=True) + self._seed_valid_role(m, src="ra", dst="ra-dst") + self._seed_valid_role(m, src="rc", dst="rc-dst") + resource = {"id": 1, "restricted_roles": ["ra", "role-gone", "rc"]} + m.connect_resources("1", resource) # no raise + assert resource["restricted_roles"] == ["ra-dst", "rc-dst"] + + def test_flag_off_restricted_roles_unchanged_behavior(self): + m = self._make_monitors(drop=False) + resource = {"id": 1, "restricted_roles": ["role-gone"]} + with pytest.raises(ResourceConnectionError): + m.connect_resources("1", resource) + # Flag off => not dropped + assert resource["restricted_roles"] == ["role-gone"] + + def test_extract_source_ids_principals_unaffected(self): + m = self._make_monitors(drop=True) + binding = {"principals": ["role:role-gone"], "relation": "editor"} + assert m.extract_source_ids("principals", binding, "roles") == ["role-gone"] diff --git a/tests/unit/test_synthetics_private_locations.py b/tests/unit/test_synthetics_private_locations.py new file mode 100644 index 00000000..3e216d36 --- /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).empty_binding_escalation 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 diff --git a/tests/unit/test_synthetics_tests.py b/tests/unit/test_synthetics_tests.py index f8a1d3f8..fce54a11 100644 --- a/tests/unit/test_synthetics_tests.py +++ b/tests/unit/test_synthetics_tests.py @@ -15,11 +15,14 @@ import asyncio import copy +from collections import defaultdict import pytest from unittest.mock import AsyncMock, MagicMock from datadog_sync.model.synthetics_tests import SyntheticsTests from datadog_sync.utils.configuration import Configuration +from datadog_sync.utils.resource_utils import ResourceConnectionError +from datadog_sync.utils.workers import Counter class TestSyntheticsTestsStatusBehavior: @@ -474,5 +477,89 @@ def test_extract_source_ids_org_excluded(self): assert synthetics_tests.extract_source_ids("principals", binding, "teams") == [] -if __name__ == "__main__": - pytest.main([__file__, "-v"]) +class TestSyntheticsTestsConnectResourcesDrop: + """connect_resources drop/keep/hard-fail for synthetics_tests' access-control shapes: + flat `options.restricted_roles` and `restriction_policy.bindings.principals` composites. + """ + + def _make_test(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 SyntheticsTests(config) + + def _seed_valid_role(self, t, src="role-good", dst="role-good-dst"): + t.config.state.destination["roles"][src] = {"id": dst} + + def test_restriction_policy_flag_on_drops_stale(self): + t = self._make_test(drop=True) + self._seed_valid_role(t) + resource = { + "public_id": "abc-def-ghi", + "restriction_policy": { + "bindings": [{"principals": ["role:role-good", "role:role-gone"], "relation": "editor"}] + }, + } + t.connect_resources("abc-def-ghi", resource) # no raise + assert resource["restriction_policy"]["bindings"][0]["principals"] == ["role:role-good-dst"] + assert t.config.counter.stale_principals_dropped_by_type["synthetics_tests"] == ["abc-def-ghi"] + + def test_restriction_policy_flag_off_stale_hard_fails(self): + t = self._make_test(drop=False) + resource = { + "public_id": "abc-def-ghi", + "restriction_policy": {"bindings": [{"principals": ["role:role-gone"], "relation": "editor"}]}, + } + with pytest.raises(ResourceConnectionError): + t.connect_resources("abc-def-ghi", resource) + + def test_restriction_policy_empty_binding_raises_risk(self): + t = self._make_test(drop=True) + resource = { + "public_id": "abc-def-ghi", + "restriction_policy": {"bindings": [{"principals": ["role:role-gone"], "relation": "editor"}]}, + } + with pytest.raises(ResourceConnectionError) as exc_info: + t.connect_resources("abc-def-ghi", resource) + assert exc_info.value.empty_binding_risk is True + + def test_options_restricted_roles_flat_drops_stale(self): + t = self._make_test(drop=True) + self._seed_valid_role(t, src="role-good", dst="role-good-dst") + resource = {"public_id": "abc", "options": {"restricted_roles": ["role-good", "role-gone"]}} + t.connect_resources("abc", resource) # no raise + assert resource["options"]["restricted_roles"] == ["role-good-dst"] + + def test_options_restricted_roles_all_stale_empty_risk(self): + t = self._make_test(drop=True) + resource = {"public_id": "abc", "options": {"restricted_roles": ["role-gone"]}} + with pytest.raises(ResourceConnectionError) as exc_info: + t.connect_resources("abc", resource) + assert exc_info.value.empty_binding_risk is True + assert resource["options"]["restricted_roles"] == [] + + def test_options_restricted_roles_empty_risk_is_returned_when_connection_failure_is_suppressed(self): + t = self._make_test(drop=True, skip_failed=True) + resource = {"public_id": "abc", "options": {"restricted_roles": ["role-gone"]}} + + assert t.connect_resources("abc", resource).empty_binding_escalation is True + assert resource["options"]["restricted_roles"] == [] + + def test_options_restricted_roles_middle_drop_keeps_neighbors(self): + t = self._make_test(drop=True) + self._seed_valid_role(t, src="ra", dst="ra-dst") + self._seed_valid_role(t, src="rc", dst="rc-dst") + resource = {"public_id": "abc", "options": {"restricted_roles": ["ra", "role-gone", "rc"]}} + t.connect_resources("abc", resource) # no raise + assert resource["options"]["restricted_roles"] == ["ra-dst", "rc-dst"] + + def test_extract_source_ids_principals_unaffected(self): + t = self._make_test(drop=True) + binding = {"principals": ["role:role-gone"], "relation": "editor"} + assert t.extract_source_ids("principals", binding, "roles") == ["role-gone"]