From 730d232cae7070996b127089f3aa0b665442e0d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20V=C3=A1zquez=20Bretal?= Date: Mon, 29 Sep 2025 10:20:57 +0200 Subject: [PATCH 1/5] WFM - Values normalization in CEL comparison String vs Bool --- keep/workflowmanager/workflowmanager.py | 53 ++++++++++++++++++------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/keep/workflowmanager/workflowmanager.py b/keep/workflowmanager/workflowmanager.py index 627b033d92..f92db8635b 100644 --- a/keep/workflowmanager/workflowmanager.py +++ b/keep/workflowmanager/workflowmanager.py @@ -428,24 +428,49 @@ def insert_events(self, tenant_id, events: typing.List[AlertDto | IncidentDto]): except (ValueError, AttributeError): # If severity conversion fails, keep original value pass - activation = celpy.json_to_cel(event_payload) try: should_run = program.evaluate(activation) except celpy.evaluation.CELEvalError as e: - self.logger.exception( - "Error evaluating CEL for event in insert_events", - extra={ - "exception": e, - "event": event, - "trigger": trigger, - "workflow_id": workflow_model.id, - "tenant_id": tenant_id, - "cel": trigger["cel"], - "deprecated_filters": trigger.get("filters"), - }, - ) - continue + if "StringType" in str(e) and "BoolType" in str(e): + # Normilize boolean strings to actual booleans base on AlertDTO + for field_name, model_field in AlertDto.__fields__.items(): + if issubclass(model_field.type_, bool) and isinstance(event_payload.get(field_name), str): + if event_payload[field_name].lower() == "true": + event_payload[field_name] = True + elif event_payload[field_name].lower() == "false": + event_payload[field_name] = False + activation = celpy.json_to_cel(event_payload) + try: + should_run = program.evaluate(activation) + except celpy.evaluation.CELEvalError as exc: + self.logger.exception( + "Error evaluating CEL for event in insert_events after normalizing boolean strings", + extra={ + "exception": exc, + "event": event, + "trigger": trigger, + "workflow_id": workflow_model.id, + "tenant_id": tenant_id, + "cel": trigger["cel"], + "deprecated_filters": trigger.get("filters"), + }, + ) + continue + else: + self.logger.exception( + "Error evaluating CEL for event in insert_events", + extra={ + "exception": e, + "event": event, + "trigger": trigger, + "workflow_id": workflow_model.id, + "tenant_id": tenant_id, + "cel": trigger["cel"], + "deprecated_filters": trigger.get("filters"), + }, + ) + continue if bool(should_run) is False: self.logger.debug( From 8635c28ba6c21d9a2f0e33dfb62d575fadef465e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20V=C3=A1zquez=20Bretal?= Date: Mon, 29 Sep 2025 10:27:25 +0200 Subject: [PATCH 2/5] Add test to check enrichment value vs cel wf --- tests/test_workflow_filters.py | 81 +++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/tests/test_workflow_filters.py b/tests/test_workflow_filters.py index 80d99e2a9f..baf6232043 100644 --- a/tests/test_workflow_filters.py +++ b/tests/test_workflow_filters.py @@ -1,5 +1,10 @@ +from datetime import datetime + +import pytest +from keep.api.core.db import get_workflow_executions from keep.api.core.dependencies import SINGLE_TENANT_UUID -from keep.api.models.alert import AlertDto +from keep.api.models.alert import AlertDto, AlertStatus +from keep.api.models.db.mapping import MappingRule from keep.api.models.db.workflow import Workflow as WorkflowDB from keep.workflowmanager.workflowmanager import WorkflowManager @@ -1150,3 +1155,77 @@ def test_cel_expression_filter(db_session): assert all(a.severity == "critical" for a in triggered_alerts) assert not any(a.id == "alert-3" for a in triggered_alerts) assert not any(a.id == "alert-4" for a in triggered_alerts) + + +@pytest.mark.parametrize( + "enrich_mapping_value, wf_value_activation, should_be_executed", + [ + ("true", "true", True), + ("false", "true", False), + ("true", "false", False), + ("false", "false", True), + ] +) +def test_dismissed_cel( + db_session, + create_alert, + enrich_mapping_value, + wf_value_activation, + should_be_executed + ): + """ + Feature: Dismissed Alerts Handling with CEL + Scenario: Using Mapping feature to dismiss alerts, + CEL expresion should recognice the dismissed status. + """ + #GIVEN The mapping rule modify the "dismissed" attribute + mapping_data = [ + {"service": "app1", "dismissed": enrich_mapping_value}, + ] + + #AND The workflow is filtering using CEL expression on "dismissed" attribute + workflow_definition = f"""workflow: + id: service-check + triggers: + - type: alert + cel: dismissed=={wf_value_activation} + """ + + mapping_rule = MappingRule( + tenant_id=SINGLE_TENANT_UUID, + name="Service Mapping", + description="Map service to additional attributes", + type="csv", + matchers=[["service"]], + rows=mapping_data, + file_name="service_mapping.csv", + priority=1, + created_by=SINGLE_TENANT_UUID, + ) + db_session.add(mapping_rule) + db_session.commit() + + workflow = WorkflowDB( + id="dimissed-cel-wf", + name="dimissed-cel-wf", + tenant_id=SINGLE_TENANT_UUID, + description="Handle alerts for specific services", + created_by="test@keephq.dev", + interval=0, + workflow_raw=workflow_definition, + ) + db_session.add(workflow) + db_session.commit() + #AND An alert coming to be enriched by mapping rule + create_alert( + "fpw1", + AlertStatus.FIRING, + datetime.utcnow(), + {"service": "app1"} + ) + #WHEN The workflow evaluates CEL Workflow vs Alert values enriched + total_execs = len(WorkflowManager.get_instance().scheduler.workflows_to_run) + + WorkflowManager.get_instance().scheduler.workflows_to_run.clear() + #THEN The workflow should be executed or not depending on the values + assert total_execs == (1 if should_be_executed else 0) From 393d4a39644f64769dd1aaa9e41d9c953115ee2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20V=C3=A1zquez=20Bretal?= Date: Mon, 29 Sep 2025 13:15:06 +0200 Subject: [PATCH 3/5] Incidents - Values normalization in CEL comparison --- keep/rulesengine/rulesengine.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/keep/rulesengine/rulesengine.py b/keep/rulesengine/rulesengine.py index 5953c492e9..a75f5f624e 100644 --- a/keep/rulesengine/rulesengine.py +++ b/keep/rulesengine/rulesengine.py @@ -490,6 +490,22 @@ def _check_if_rule_apply(self, rule: Rule, event: AlertDto) -> List[str]: e ): try: + if "StringType" in str(e) and "BoolType" in str(e): + # Normilize boolean strings to actual booleans base on AlertDTO + for field_name, model_field in AlertDto.__fields__.items(): + if issubclass(model_field.type_, bool) and isinstance(payload.get(field_name), str): + if payload[field_name].lower() == "true": + payload[field_name] = True + elif payload[field_name].lower() == "false": + payload[field_name] = False + activation = celpy.json_to_cel(payload) + try: + r = prgm.evaluate(activation) + if r: + sub_rules_matched.append(sub_rule) + continue + except celpy.evaluation.CELEvalError: + pass coerced = self._coerce_eq_type_error( sub_rule, prgm, activation, event ) From 182632122b92f1a050c6d40de9f0494fd3efa86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20V=C3=A1zquez=20Bretal?= Date: Mon, 29 Sep 2025 14:11:17 +0200 Subject: [PATCH 4/5] Test to check incidents and wf --- tests/test_alert_evaluation.py | 152 ++++++++++++++++++++++++++++++++- tests/test_workflow_filters.py | 80 +---------------- 2 files changed, 152 insertions(+), 80 deletions(-) diff --git a/tests/test_alert_evaluation.py b/tests/test_alert_evaluation.py index 81b9ab94c4..904513a949 100644 --- a/tests/test_alert_evaluation.py +++ b/tests/test_alert_evaluation.py @@ -3,15 +3,21 @@ # Shahar: since js2py is not secured, I've commented out this tests # TODO: fix js2py and uncomment the tests -from datetime import timedelta +from datetime import timedelta, datetime import pytest from freezegun import freeze_time +from keep.api.core.db import get_incidents_by_alert_fingerprint +from keep.api.core.dependencies import SINGLE_TENANT_UUID from keep.api.models.alert import AlertStatus +from keep.api.models.db.mapping import MappingRule +from keep.api.models.db.rule import Rule from keep.contextmanager.contextmanager import ContextManager from keep.providers.keep_provider.keep_provider import KeepProvider from keep.searchengine.searchengine import SearchEngine +from keep.workflowmanager.workflowmanager import WorkflowManager +from keep.api.models.db.workflow import Workflow as WorkflowDB steps_dict = { # this is the step that will be used to trigger the alert @@ -1074,3 +1080,147 @@ def test_check_if_rule_apply_int_str_type_coercion(db_session): assert ( len(matched_rules4) == 1 ), "Rule with 'field == \"2\"' should match alert with field='2'" + +@pytest.mark.parametrize( + "enrich_mapping_value, rule_value_activation, should_be_executed", + [ + ("true", "true", True), + ("false", "true", False), + ("true", "false", False), + ("false", "false", True), + ] +) +def test_check_if_rule_apply_dismissed_incident( + db_session, + create_alert, + enrich_mapping_value, + rule_value_activation, + should_be_executed + ): + """ + Feature: Dismissed Alerts Handling with CEL + Scenario: Using Mapping feature to dismiss alerts, + CEL expresion should recognice the dismissed status. + """ + #GIVEN The mapping rule modify the "dismissed" attribute + mapping_data = [ + {"service": "app1", "dismissed": enrich_mapping_value}, + ] + + mapping_rule = MappingRule( + tenant_id=SINGLE_TENANT_UUID, + name="Service Mapping", + description="Map service to additional attributes", + type="csv", + matchers=[["service"]], + rows=mapping_data, + file_name="service_mapping.csv", + priority=1, + created_by=SINGLE_TENANT_UUID, + ) + db_session.add(mapping_rule) + db_session.commit() + + #AND The rule use CEL expression to check the "dismissed" attribute + rule = Rule( + id="test-rule-1", + tenant_id=SINGLE_TENANT_UUID, + name="Test Rule - Dismissed Alerts", + definition_cel=f'dismissed == {rule_value_activation} && service == "app1"', + definition={}, + timeframe=60, + timeunit="seconds", + created_by="test@keephq.dev", + creation_time=datetime.utcnow(), + grouping_criteria=[], + threshold=1, + ) + db_session.add(rule) + db_session.commit() + #AND An alert coming to be enriched by mapping rule + create_alert( + "fpw1", + AlertStatus.FIRING, + datetime.utcnow(), + {"service": "app1"} + ) + #WHEN The rules engine process the alert + total_execs = len(get_incidents_by_alert_fingerprint( + SINGLE_TENANT_UUID, "fpw1" + )) + + #THEN The incidents should be executed or not depending on the values + assert total_execs == (1 if should_be_executed else 0) + +@pytest.mark.parametrize( + "enrich_mapping_value, wf_value_activation, should_be_executed", + [ + ("true", "true", True), + ("false", "true", False), + ("true", "false", False), + ("false", "false", True), + ] +) +def test_check_if_rule_apply_dismissed_workflow( + db_session, + create_alert, + enrich_mapping_value, + wf_value_activation, + should_be_executed + ): + """ + Feature: Dismissed Alerts Handling with CEL + Scenario: Using Mapping feature to dismiss alerts, + CEL expresion should recognice the dismissed status. + """ + #GIVEN The mapping rule modify the "dismissed" attribute + mapping_data = [ + {"service": "app1", "dismissed": enrich_mapping_value}, + ] + + #AND The workflow is filtering using CEL expression on "dismissed" attribute + workflow_definition = f"""workflow: +id: service-check +triggers: +- type: alert + cel: dismissed=={wf_value_activation} +""" + + mapping_rule = MappingRule( + tenant_id=SINGLE_TENANT_UUID, + name="Service Mapping", + description="Map service to additional attributes", + type="csv", + matchers=[["service"]], + rows=mapping_data, + file_name="service_mapping.csv", + priority=1, + created_by=SINGLE_TENANT_UUID, + ) + db_session.add(mapping_rule) + db_session.commit() + + workflow = WorkflowDB( + id="dimissed-cel-wf", + name="dimissed-cel-wf", + tenant_id=SINGLE_TENANT_UUID, + description="Handle alerts for specific services", + created_by="test@keephq.dev", + interval=0, + workflow_raw=workflow_definition, + ) + db_session.add(workflow) + db_session.commit() + #AND An alert coming to be enriched by mapping rule + create_alert( + "fpw1", + AlertStatus.FIRING, + datetime.utcnow(), + {"service": "app1"} + ) + #WHEN The workflow evaluates CEL Workflow vs Alert values enriched + total_execs = len(WorkflowManager.get_instance().scheduler.workflows_to_run) + + WorkflowManager.get_instance().scheduler.workflows_to_run.clear() + #THEN The workflow should be executed or not depending on the values + assert total_execs == (1 if should_be_executed else 0) diff --git a/tests/test_workflow_filters.py b/tests/test_workflow_filters.py index baf6232043..cbfd0a0145 100644 --- a/tests/test_workflow_filters.py +++ b/tests/test_workflow_filters.py @@ -1,10 +1,5 @@ -from datetime import datetime - -import pytest -from keep.api.core.db import get_workflow_executions from keep.api.core.dependencies import SINGLE_TENANT_UUID -from keep.api.models.alert import AlertDto, AlertStatus -from keep.api.models.db.mapping import MappingRule +from keep.api.models.alert import AlertDto from keep.api.models.db.workflow import Workflow as WorkflowDB from keep.workflowmanager.workflowmanager import WorkflowManager @@ -1156,76 +1151,3 @@ def test_cel_expression_filter(db_session): assert not any(a.id == "alert-3" for a in triggered_alerts) assert not any(a.id == "alert-4" for a in triggered_alerts) - -@pytest.mark.parametrize( - "enrich_mapping_value, wf_value_activation, should_be_executed", - [ - ("true", "true", True), - ("false", "true", False), - ("true", "false", False), - ("false", "false", True), - ] -) -def test_dismissed_cel( - db_session, - create_alert, - enrich_mapping_value, - wf_value_activation, - should_be_executed - ): - """ - Feature: Dismissed Alerts Handling with CEL - Scenario: Using Mapping feature to dismiss alerts, - CEL expresion should recognice the dismissed status. - """ - #GIVEN The mapping rule modify the "dismissed" attribute - mapping_data = [ - {"service": "app1", "dismissed": enrich_mapping_value}, - ] - - #AND The workflow is filtering using CEL expression on "dismissed" attribute - workflow_definition = f"""workflow: - id: service-check - triggers: - - type: alert - cel: dismissed=={wf_value_activation} - """ - - mapping_rule = MappingRule( - tenant_id=SINGLE_TENANT_UUID, - name="Service Mapping", - description="Map service to additional attributes", - type="csv", - matchers=[["service"]], - rows=mapping_data, - file_name="service_mapping.csv", - priority=1, - created_by=SINGLE_TENANT_UUID, - ) - db_session.add(mapping_rule) - db_session.commit() - - workflow = WorkflowDB( - id="dimissed-cel-wf", - name="dimissed-cel-wf", - tenant_id=SINGLE_TENANT_UUID, - description="Handle alerts for specific services", - created_by="test@keephq.dev", - interval=0, - workflow_raw=workflow_definition, - ) - db_session.add(workflow) - db_session.commit() - #AND An alert coming to be enriched by mapping rule - create_alert( - "fpw1", - AlertStatus.FIRING, - datetime.utcnow(), - {"service": "app1"} - ) - #WHEN The workflow evaluates CEL Workflow vs Alert values enriched - total_execs = len(WorkflowManager.get_instance().scheduler.workflows_to_run) - - WorkflowManager.get_instance().scheduler.workflows_to_run.clear() - #THEN The workflow should be executed or not depending on the values - assert total_execs == (1 if should_be_executed else 0) From 6f03dc27c9c82a1bcfc366a50eb6c9abec956b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20V=C3=A1zquez=20Bretal?= Date: Mon, 6 Oct 2025 09:15:38 +0200 Subject: [PATCH 5/5] Avoid modify dict in every loop --- keep/rulesengine/rulesengine.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/keep/rulesengine/rulesengine.py b/keep/rulesengine/rulesengine.py index a75f5f624e..21fee2e31a 100644 --- a/keep/rulesengine/rulesengine.py +++ b/keep/rulesengine/rulesengine.py @@ -3,6 +3,7 @@ import logging import re from typing import List, Optional +from copy import deepcopy import celpy import celpy.c7nlib @@ -455,11 +456,12 @@ def _check_if_rule_apply(self, rule: Rule, event: AlertDto) -> List[str]: Evaluates if a rule applies to an event using CEL. Handles type coercion for ==/!= between int and str. """ sub_rules = self._extract_subrules(rule.definition_cel) - payload = event.dict() + payload = deepcopy(event.dict()) # workaround since source is a list # todo: fix this in the future payload["source"] = payload["source"][0] payload = RulesEngine.sanitize_cel_payload(payload) + normalized_payload = None # what we do here is to compile the CEL rule and evaluate it # https://github.com/cloud-custodian/cel-python @@ -477,7 +479,8 @@ def _check_if_rule_apply(self, rule: Rule, event: AlertDto) -> List[str]: sub_rule = sub_rule.replace("null", '""') ast = self.env.compile(sub_rule) prgm = self.env.program(ast) - activation = celpy.json_to_cel(json.loads(json.dumps(payload, default=str))) + current_payload = normalized_payload or payload + activation = celpy.json_to_cel(json.loads(json.dumps(current_payload, default=str))) try: r = prgm.evaluate(activation) except celpy.evaluation.CELEvalError as e: @@ -490,15 +493,19 @@ def _check_if_rule_apply(self, rule: Rule, event: AlertDto) -> List[str]: e ): try: - if "StringType" in str(e) and "BoolType" in str(e): + if normalized_payload is None and "StringType" in str(e) and "BoolType" in str(e): + normalized_payload = deepcopy(payload) # Normilize boolean strings to actual booleans base on AlertDTO for field_name, model_field in AlertDto.__fields__.items(): + val = normalized_payload.get(field_name) if issubclass(model_field.type_, bool) and isinstance(payload.get(field_name), str): - if payload[field_name].lower() == "true": - payload[field_name] = True - elif payload[field_name].lower() == "false": - payload[field_name] = False - activation = celpy.json_to_cel(payload) + if val is not None: + lower = val.lower() + if lower == "true": + normalized_payload[field_name] = True + elif lower == "false": + normalized_payload[field_name] = False + activation = celpy.json_to_cel(json.loads(json.dumps(normalized_payload, default=str))) try: r = prgm.evaluate(activation) if r: