diff --git a/keep/providers/keep_provider/keep_provider.py b/keep/providers/keep_provider/keep_provider.py index 58b8671885..9180565dd5 100644 --- a/keep/providers/keep_provider/keep_provider.py +++ b/keep/providers/keep_provider/keep_provider.py @@ -136,7 +136,7 @@ def _query( self.logger.info("Got alerts from Keep", extra={"num_of_alerts": len(alerts)}) return alerts - def _build_alert(self, alert_data, fingerprint_fields=[], **kwargs): + def _build_alert(self, alert_data, fingerprint_fields=None, **kwargs): """ Build alerts from Keep. """ @@ -174,7 +174,9 @@ def _build_alert(self, alert_data, fingerprint_fields=[], **kwargs): fingerprint_fields = ["labels." + label for label in list(labels.keys())] # workflowId is used as the "rule id" - it's used to identify the rule that created the alert - fingerprint_fields.append("workflowId") + # build a new list instead of appending in place, since fingerprint_fields may be a + # list the caller (e.g. _notify_alert's per-alert loop) reuses across multiple alerts + fingerprint_fields = fingerprint_fields + ["workflowId"] alert.fingerprint = self.get_alert_fingerprint(alert, fingerprint_fields) return alert diff --git a/keep/providers/servicenow_provider/servicenow_provider.py b/keep/providers/servicenow_provider/servicenow_provider.py index 38e5f7cfb1..3c674ac8b1 100644 --- a/keep/providers/servicenow_provider/servicenow_provider.py +++ b/keep/providers/servicenow_provider/servicenow_provider.py @@ -306,16 +306,20 @@ def _query( sysparm_limit # Limit number of records per request ) if sysparm_offset: - params["sysparm_offset"] = 0 # Start from beginning + params["sysparm_offset"] = sysparm_offset # use the recevied offset - response = requests.get( - request_url, - headers=headers, - auth=auth, - params=params, - verify=False, - timeout=10, - ) + try: + response = requests.get( + request_url, + headers=headers, + auth=auth, + params=params, + verify=False, + timeout=10, + ) + except requests.exceptions.RequestException: + self.logger.exception(f"Failed to query {table_name}") + return [] if not response.ok: self.logger.error( @@ -681,7 +685,7 @@ def pull_topology(self) -> tuple[list[TopologyServiceInDto], dict]: "sys_id", "ip_address", "mac_address", - "owned_by.name" + "owned_by.name", "manufacturer.name", # Retrieve the name of the manufacturer "short_description", "environment", @@ -899,16 +903,14 @@ def _notify(self, table_name: str, payload: dict = {}, **kwargs: dict): def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str): url = f"{self.authentication_config.service_now_base_url}/api/now/table/{table_name}/{ticket_id}" headers = {"Content-Type": "application/json", "Accept": "application/json"} - auth = ( - ( + auth = None + if self._access_token: + headers["Authorization"] = f"Bearer {self._access_token}" + else: + auth = ( self.authentication_config.username, self.authentication_config.password, ) - if self._access_token - else None - ) - if self._access_token: - headers["Authorization"] = f"Bearer {self._access_token}" response = requests.get( url, @@ -932,7 +934,7 @@ def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str): return resp else: self.logger.info("Failed to update ticket", extra={"resp": response.text}) - resp.raise_for_status() + response.raise_for_status() if __name__ == "__main__": diff --git a/tests/test_keep_provider_fingerprint_fields_mutation.py b/tests/test_keep_provider_fingerprint_fields_mutation.py new file mode 100644 index 0000000000..0c379a9d6f --- /dev/null +++ b/tests/test_keep_provider_fingerprint_fields_mutation.py @@ -0,0 +1,86 @@ +""" +Test for KeepProvider._build_alert mutating the caller's fingerprint_fields list. + +This reproduces the issue described in https://github.com/keephq/keep/issues/6719 +where _build_alert calls fingerprint_fields.append("workflowId") on whatever list +was passed in. _notify_alert's per-alert loop passes the same fingerprint_fields +object into _build_alert on every iteration of a foreach batch, so each alert +after the first accumulates one more duplicate "workflowId" entry, which changes +its computed fingerprint. +""" + +import uuid + +from keep.api.core.dependencies import SINGLE_TENANT_UUID +from keep.contextmanager.contextmanager import ContextManager +from keep.providers.keep_provider.keep_provider import KeepProvider +from keep.providers.models.provider_config import ProviderConfig + + +def _make_provider(): + context_manager = ContextManager( + tenant_id=SINGLE_TENANT_UUID, + workflow_id=str(uuid.uuid4()), + ) + provider_config = ProviderConfig(authentication={}) + return KeepProvider( + context_manager=context_manager, + provider_id="test-keep", + config=provider_config, + ) + + +def test_build_alert_does_not_mutate_caller_fingerprint_fields(): + """ + Calling _build_alert repeatedly with the same fingerprint_fields list - + exactly what _notify_alert's per-alert loop does for a foreach batch - + must not grow or otherwise mutate that list between calls. + """ + provider = _make_provider() + fingerprint_fields = ["labels.service"] + + for _ in range(3): + provider._build_alert( + {}, + fingerprint_fields, + name="disk full", + labels={"service": "db-primary"}, + ) + + assert fingerprint_fields == ["labels.service"] + + +def test_build_alert_produces_consistent_fingerprints_across_a_batch(): + """ + Alerts that only differ in the value of the fingerprinted field should get + fingerprints computed the same way, regardless of how many other alerts + were built earlier in the same batch (same fingerprint_fields object reused, + as _notify_alert does). + """ + provider = _make_provider() + fingerprint_fields = ["labels.service"] + + services = ["db-primary", "db-replica", "cache"] + fingerprints = [] + for service in services: + alert = provider._build_alert( + {}, + fingerprint_fields, + name="disk full", + labels={"service": service}, + ) + fingerprints.append(alert.fingerprint) + + # Building the alert for "cache" independently (fresh fingerprint_fields, + # as if it were the only/first alert in its own batch) must produce the + # exact same fingerprint as when it was the 3rd alert in the loop above. + standalone_alert = provider._build_alert( + {}, + ["labels.service"], + name="disk full", + labels={"service": "cache"}, + ) + + assert fingerprints[2] == standalone_alert.fingerprint + # sanity: different services still produce different fingerprints + assert len(set(fingerprints)) == len(services) diff --git a/tests/test_servicenow_provider.py b/tests/test_servicenow_provider.py index 54baeeff40..90be793566 100644 --- a/tests/test_servicenow_provider.py +++ b/tests/test_servicenow_provider.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock, patch import pytest +import requests from keep.api.models.incident import IncidentDto, IncidentStatus, IncidentSeverity from keep.contextmanager.contextmanager import ContextManager @@ -332,3 +333,151 @@ def test_severity_mapping_coverage(self): """Test that all ServiceNow impact levels are mapped.""" for impact in ["1", "2", "3"]: assert impact in ServicenowProvider.INCIDENT_SEVERITY_MAP + + +class TestQueryPagination: + """Regression tests for the _query sysparm_offset bug.""" + + def test_query_forwards_requested_offset(self, servicenow_provider): + """_query must send the caller's offset, not always 0.""" + response = MagicMock() + response.ok = True + response.json.return_value = {"result": []} + + with patch("requests.get", return_value=response) as mock_get: + servicenow_provider._query( + "incident", sysparm_limit=50, sysparm_offset=150 + ) + + params = mock_get.call_args.kwargs["params"] + assert params["sysparm_offset"] == 150 + assert params["sysparm_limit"] == 50 + + def test_query_returns_empty_list_on_request_exception(self, servicenow_provider): + """Network failures should be caught, not raised.""" + with patch( + "requests.get", + side_effect=requests.exceptions.ConnectionError("connection refused"), + ): + result = servicenow_provider._query("incident") + + assert result == [] + + +class TestGetIncidentsPagination: + """Regression tests ensuring _get_incidents advances through pages.""" + + def _make_raw_incident(self, number): + return { + "number": number, + "sys_id": number, + "state": "1", + "impact": "1", + "sys_created_on": "2025-01-01 00:00:00", + } + + def test_paginates_across_multiple_pages(self, servicenow_provider): + page1 = [self._make_raw_incident(f"INC{i:04d}") for i in range(100)] + page2 = [self._make_raw_incident("INC0100")] + + with patch.object( + servicenow_provider, "_query", side_effect=[page1, page2] + ) as mock_query: + incidents = servicenow_provider._get_incidents() + + assert len(incidents) == 101 + calls = mock_query.call_args_list + assert len(calls) == 2 + assert calls[0].kwargs["sysparm_offset"] == 0 + assert calls[1].kwargs["sysparm_offset"] == 100 + + def test_stops_when_page_smaller_than_limit(self, servicenow_provider): + page1 = [self._make_raw_incident("INC0001")] + + with patch.object( + servicenow_provider, "_query", side_effect=[page1] + ) as mock_query: + incidents = servicenow_provider._get_incidents() + + assert len(incidents) == 1 + assert mock_query.call_count == 1 + + +class TestNotifyUpdate: + """Tests for _notify_update auth handling and error propagation.""" + + def test_basic_auth_used_when_no_access_token(self, servicenow_provider): + get_response = MagicMock() + get_response.status_code = 200 + get_response.text = json.dumps( + {"result": {"sys_id": "abc123", "number": "INC0010001"}} + ) + + with patch("requests.get", return_value=get_response) as mock_get: + result = servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp1" + ) + + assert result["sys_id"] == "abc123" + assert result["fingerprint"] == "fp1" + assert mock_get.call_args.kwargs["auth"] == ("admin", "admin") + + def test_oauth_does_not_raise_nameerror(self, servicenow_provider): + """Regression test: auth must not reference an undefined variable + when an OAuth access token is in use.""" + servicenow_provider._access_token = "faketoken" + get_response = MagicMock() + get_response.status_code = 200 + get_response.text = json.dumps({"result": {"sys_id": "abc123"}}) + + with patch("requests.get", return_value=get_response) as mock_get: + result = servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp2" + ) + + assert result["fingerprint"] == "fp2" + assert mock_get.call_args.kwargs["auth"] is None + assert mock_get.call_args.kwargs["headers"]["Authorization"] == "Bearer faketoken" + + def test_failure_raises_http_error(self, servicenow_provider): + """Regression test: failures must raise HTTPError, not NameError.""" + get_response = MagicMock() + get_response.status_code = 500 + get_response.text = "Internal Server Error" + get_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + "500 Server Error" + ) + + with patch("requests.get", return_value=get_response): + with pytest.raises(requests.exceptions.HTTPError): + servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp3" + ) + + +class TestPullTopologyFields: + """Regression test for the missing comma in the CMDB fields list.""" + + def test_cmdb_fields_are_separate_entries(self, servicenow_provider): + cmdb_response = MagicMock() + cmdb_response.ok = True + cmdb_response.json.return_value = {"result": []} + rel_type_response = MagicMock() + rel_type_response.ok = True + rel_type_response.json.return_value = {"result": []} + rel_response = MagicMock() + rel_response.ok = True + rel_response.json.return_value = {"result": []} + + with patch( + "requests.get", + side_effect=[cmdb_response, rel_type_response, rel_response], + ) as mock_get: + servicenow_provider.pull_topology() + + cmdb_call = mock_get.call_args_list[0] + fields = cmdb_call.kwargs["params"]["sysparm_fields"].split(",") + + assert "owned_by.name" in fields + assert "manufacturer.name" in fields + assert "owned_by.namemanufacturer.name" not in fields