From b53ee4c778534697571edf2ba7c547013b318289 Mon Sep 17 00:00:00 2001 From: "riyaz.shiraguppi" Date: Tue, 1 Sep 2026 17:06:20 -0500 Subject: [PATCH] Fix dashboard list logging and restricted monitor filtering Signed-off-by: riyaz.shiraguppi --- datadog_sync/commands/shared/options.py | 3 +- datadog_sync/model/dashboard_lists.py | 4 +- datadog_sync/model/monitors.py | 29 ++++++---- tests/unit/test_dashboard_lists.py | 72 +++++++++++++++---------- tests/unit/test_monitors.py | 41 +++++++++++++- 5 files changed, 107 insertions(+), 42 deletions(-) diff --git a/datadog_sync/commands/shared/options.py b/datadog_sync/commands/shared/options.py index 31091142..b084fa21 100644 --- a/datadog_sync/commands/shared/options.py +++ b/datadog_sync/commands/shared/options.py @@ -583,7 +583,8 @@ def click_config_file_provider(ctx: Context, opts: CustomOptionClass, value: Non is_flag=True, default=False, show_default=True, - help="Filter out monitors whose source payload has a non-empty restricted_roles list. " + help="Filter out monitors whose source payload has a non-empty restricted_roles list " + "or restriction_policy bindings. " "This is an explicit access-control escape hatch for DDR destinations where " "role/user activation is not ready yet; filtered monitors are not created or updated.", cls=CustomOptionClass, diff --git a/datadog_sync/model/dashboard_lists.py b/datadog_sync/model/dashboard_lists.py index e0bed730..4f9f9d57 100644 --- a/datadog_sync/model/dashboard_lists.py +++ b/datadog_sync/model/dashboard_lists.py @@ -158,10 +158,10 @@ def _drop_integration_dashboards(self, _id: str, resource: Dict) -> None: ) self.config.logger.info( "dropping integration dashboards from dashboard list before sync; " - "integration dashboard IDs are not portable across orgs", + "integration dashboard IDs are not portable across orgs; " + f"dropped_dashboard_ids={','.join(dropped)}", resource_type=self.resource_type, _id=_id, - dropped_dashboard_ids=",".join(dropped), ) resource["dashboards"] = portable_dashboards diff --git a/datadog_sync/model/monitors.py b/datadog_sync/model/monitors.py index bdc155ea..a449b2e0 100644 --- a/datadog_sync/model/monitors.py +++ b/datadog_sync/model/monitors.py @@ -39,6 +39,7 @@ def _variables_have_group_by(variables) -> bool: return True return False + if TYPE_CHECKING: from datadog_sync.utils.custom_client import CustomClient @@ -182,9 +183,7 @@ async def pre_resource_action_hook(self, _id, resource: Dict) -> None: and not _variables_have_group_by(options.get("variables")) ): options.pop("notify_by") - self.config.logger.info( - f"monitor {_id}: dropped no-op options.notify_by=['*'] on ungrouped query" - ) + self.config.logger.info(f"monitor {_id}: dropped no-op options.notify_by=['*'] on ungrouped query") # org: principals are remapped here (before connect_resources runs). # user:/role:/team: principals are remapped by connect_id via resource_connections paths. @@ -243,11 +242,11 @@ def filter(self, resource: Dict) -> bool: if not super().filter(resource): return False - if getattr(self.config, "skip_monitors_with_restricted_roles", False) is True and resource.get( - "restricted_roles" + if getattr(self.config, "skip_monitors_with_restricted_roles", False) is True and _has_access_restrictions( + resource ): self.config.logger.info( - "filtering monitor with restricted_roles because --skip-monitors-with-restricted-roles is enabled", + "filtering monitor with access restrictions because --skip-monitors-with-restricted-roles is enabled", resource_type=self.resource_type, _id=str(resource.get("id", "")), ) @@ -293,9 +292,7 @@ def connect_resources(self, _id: str, resource: Dict) -> ResourceConnectionResul empty_risk = empty_risk or roles_risk return ResourceConnectionResult( - empty_binding_escalation=self._raise_connection_error_if_any( - _id, failed_connections_dict, empty_risk - ) + 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]]: @@ -390,6 +387,20 @@ def extract_source_ids(self, key: str, r_obj: Dict, resource_to_connect: str) -> return super(Monitors, self).extract_source_ids(key, r_obj, resource_to_connect) +def _has_access_restrictions(resource: Dict) -> bool: + if resource.get("restricted_roles"): + return True + + restriction_policy = resource.get("restriction_policy") + if not isinstance(restriction_policy, dict): + return False + + for binding in restriction_policy.get("bindings") or []: + if isinstance(binding, dict) and binding.get("principals"): + return True + return False + + _MONITOR_LOG_QUERY_MAX_CHARS = 2000 _MONITOR_LOG_REASON_MAX_CHARS = 200 _MONITOR_APPLICATION_ID_RE = re.compile(r"@application\.id:[A-Za-z0-9\-]+") diff --git a/tests/unit/test_dashboard_lists.py b/tests/unit/test_dashboard_lists.py index 768a193e..0e6c2b7b 100644 --- a/tests/unit/test_dashboard_lists.py +++ b/tests/unit/test_dashboard_lists.py @@ -26,11 +26,17 @@ def _make_dashboard_lists() -> DashboardLists: return DashboardLists(config) +class _SignatureCheckedLogger: + def __init__(self): + self.info_calls = [] + + def info(self, msg: str, *arg, _id: str = "", resource_type: str = "") -> None: + self.info_calls.append({"msg": msg, "args": arg, "_id": _id, "resource_type": resource_type}) + + def test_pre_resource_action_hook_drops_integration_dashboards_before_apply(): dashboard_lists = _make_dashboard_lists() - dashboard_lists.config.state.destination["dashboards"]["dash-src"] = { - "id": "dash-dst" - } + dashboard_lists.config.state.destination["dashboards"]["dash-src"] = {"id": "dash-dst"} resource = { "id": 510887, "dashboards": [ @@ -48,11 +54,37 @@ def test_pre_resource_action_hook_drops_integration_dashboards_before_apply(): ] -def test_connect_resources_ignores_unmapped_integration_dashboards(): +def test_drop_integration_dashboards_uses_supported_logger_kwargs(): dashboard_lists = _make_dashboard_lists() - dashboard_lists.config.state.destination["dashboards"]["dash-src"] = { - "id": "dash-dst" + logger = _SignatureCheckedLogger() + dashboard_lists.config.logger = logger + resource = { + "id": 510887, + "dashboards": [ + {"id": "dash-src", "type": "custom_timeboard"}, + {"id": "62", "type": "integration_timeboard"}, + {"id": "30516", "type": "integration_timeboard"}, + ], } + + asyncio.run(dashboard_lists.pre_resource_action_hook("510887", resource)) + + assert resource["dashboards"] == [{"id": "dash-src", "type": "custom_timeboard"}] + assert logger.info_calls == [ + { + "msg": "dropping integration dashboards from dashboard list before sync; " + "integration dashboard IDs are not portable across orgs; " + "dropped_dashboard_ids=30516,62", + "args": (), + "_id": "510887", + "resource_type": "dashboard_lists", + } + ] + + +def test_connect_resources_ignores_unmapped_integration_dashboards(): + dashboard_lists = _make_dashboard_lists() + dashboard_lists.config.state.destination["dashboards"]["dash-src"] = {"id": "dash-dst"} resource = { "id": 510887, "dashboards": [ @@ -125,9 +157,7 @@ def test_update_dash_list_items_drops_integration_dashboards_from_payload(): dashboard_lists.dash_list_items_path.format("dst-list"), {"dashboards": [{"id": "dash-dst", "type": "custom_timeboard"}]}, ) - assert dashboard_list == { - "dashboards": [{"id": "dash-dst", "type": "custom_timeboard"}] - } + assert dashboard_list == {"dashboards": [{"id": "dash-dst", "type": "custom_timeboard"}]} def _http_error(status: int) -> CustomClientHTTPError: @@ -156,30 +186,18 @@ def test_500_on_items_fetch_propagates(self): classifies it as http_5xx (transient) — counted as failure, logged at WARNING, no exit-code poisoning, no incomplete state written.""" dashboard_lists = _make_dashboard_lists() - dashboard_lists.config.source_client.get = AsyncMock( - side_effect=_http_error(500) - ) + dashboard_lists.config.source_client.get = AsyncMock(side_effect=_http_error(500)) with pytest.raises(CustomClientHTTPError): - asyncio.run( - dashboard_lists.import_resource( - resource={"id": "42", "name": "my-list"} - ) - ) + asyncio.run(dashboard_lists.import_resource(resource={"id": "42", "name": "my-list"})) def test_503_on_items_fetch_propagates(self): """Any 5xx propagates — same treatment as 500.""" dashboard_lists = _make_dashboard_lists() - dashboard_lists.config.source_client.get = AsyncMock( - side_effect=_http_error(503) - ) + dashboard_lists.config.source_client.get = AsyncMock(side_effect=_http_error(503)) with pytest.raises(CustomClientHTTPError): - asyncio.run( - dashboard_lists.import_resource( - resource={"id": "42", "name": "my-list"} - ) - ) + asyncio.run(dashboard_lists.import_resource(resource={"id": "42", "name": "my-list"})) def test_items_fetch_success_populates_dashboards(self): """Happy path: items endpoint returns dashboard IDs that are @@ -193,8 +211,6 @@ async def fake_get(path, **kwargs): dashboard_lists.config.source_client.get = AsyncMock(side_effect=fake_get) - _id, resource = asyncio.run( - dashboard_lists.import_resource(resource={"id": "42", "name": "my-list"}) - ) + _id, resource = asyncio.run(dashboard_lists.import_resource(resource={"id": "42", "name": "my-list"})) assert resource["dashboards"] == [{"id": "dash-1", "type": "custom_timeboard"}] diff --git a/tests/unit/test_monitors.py b/tests/unit/test_monitors.py index 6a17df99..c6d73f6e 100644 --- a/tests/unit/test_monitors.py +++ b/tests/unit/test_monitors.py @@ -143,15 +143,52 @@ def test_restricted_roles_filtered_when_flag_enabled(self): assert monitors.filter(resource) is False monitors.config.logger.info.assert_called_once() + def test_restriction_policy_principals_allowed_by_default(self): + monitors = self._make_monitors() + resource = { + "id": 269290576, + "restriction_policy": { + "bindings": [ + { + "relation": "editor", + "principals": ["user:source-user"], + } + ] + }, + } + + assert monitors.filter(resource) is True + + def test_restriction_policy_principals_filtered_when_flag_enabled(self): + monitors = self._make_monitors(skip_restricted=True) + resource = { + "id": 269290576, + "restriction_policy": { + "bindings": [ + { + "relation": "editor", + "principals": ["user:source-user"], + } + ] + }, + } + + assert monitors.filter(resource) is False + monitors.config.logger.info.assert_called_once() + @pytest.mark.parametrize( "resource", [ {"id": 1}, {"id": 2, "restricted_roles": []}, {"id": 3, "restricted_roles": None}, + {"id": 4, "restriction_policy": None}, + {"id": 5, "restriction_policy": {}}, + {"id": 6, "restriction_policy": {"bindings": []}}, + {"id": 7, "restriction_policy": {"bindings": [{"relation": "editor", "principals": []}]}}, ], ) - def test_empty_or_missing_restricted_roles_are_not_filtered(self, resource): + def test_empty_or_missing_access_restrictions_are_not_filtered(self, resource): monitors = self._make_monitors(skip_restricted=True) assert monitors.filter(resource) is True @@ -403,7 +440,7 @@ def test_notify_by_star_preserved_on_log_alert_grouped_query(self): resource = { "id": 14, "type": "log alert", - "query": "logs(\"service:foo\").index(\"*\").rollup(\"count\").by(\"host\").last(\"5m\") > 0", + "query": 'logs("service:foo").index("*").rollup("count").by("host").last("5m") > 0', "options": {"notify_by": ["*"]}, } asyncio.run(monitors.pre_resource_action_hook("14", resource))