diff --git a/src/cosl/rules.py b/src/cosl/rules.py index 64de85e..691419f 100644 --- a/src/cosl/rules.py +++ b/src/cosl/rules.py @@ -74,12 +74,11 @@ - `juju_model_uuid` - `juju_application` """ # noqa: W505 + import hashlib import logging import os import re -import textwrap -from pprint import pprint from abc import ABC, abstractmethod from pathlib import Path from typing import Any, Dict, List, Optional, Union, cast @@ -242,8 +241,60 @@ def _from_dir(self, dir_path: Path, recursive: bool) -> List[Dict[str, Any]]: return groups - def _from_str(self, yaml_str: str, *, group_name: Optional[str]=None, group_name_prefix: str) -> List[dict]: - """Process rules file (TODO: improve desc). + def _from_file( # noqa: C901 + self, root_path: Path, file_path: Path + ) -> List[OfficialRuleFileItem]: + """Read a rules file from path. + + Args: + root_path: full path to the root rules folder (used only for generating group name) + file_path: full path to a *.rule file. + + Returns: + A list of dictionaries representing the rules file, if file is valid (the structure is + formed by `yaml.safe_load` of the file); an empty list otherwise. + """ + with file_path.open() as rf: + # Load a list of rules from file then add labels and filters + try: + rule_file = yaml.safe_load(rf) + + except Exception as e: + logger.error("Failed to read rules from %s: %s", file_path.name, e) + return [] + + # Generate group name prefix + # - name, from juju topology + # - suffix, from the relative path of the rule file; + rel_path = os.path.relpath(os.path.dirname(file_path), root_path) + rel_path = "" if rel_path == "." else rel_path.replace(os.path.sep, "_") + group_name_parts = [self.topology.identifier] if self.topology else [] + group_name_parts.append(rel_path) + group_name_prefix = "_".join(filter(None, group_name_parts)) + + try: + groups = self._from_str( + rule_file, group_name=file_path.stem, group_name_prefix=group_name_prefix + ) + except ValueError as e: + logger.error("Invalid rules file: %s (%s)", file_path.name, e) + return [] + + return groups + + def _from_str( + self, + yaml_str: str, + *, + group_name: Optional[str] = None, + group_name_prefix: Optional[str] = None, + ) -> List[OfficialRuleFileItem]: + """Process rules from string, injecting juju topology. If a single-rule format is provided, a hash of the yaml file is injected into the group name to ensure uniqueness. + + Args: + yaml_str: rules content in single-rule or official-rule format as a string + group_name: a custom identifier for the rule name to include in the group name + group_name_prefix: a custom group identifier to prefix the resulting group name, likely Juju topology and relative path context Raises: ValueError, when invalid rule format given. @@ -257,15 +308,17 @@ def _from_str(self, yaml_str: str, *, group_name: Optional[str]=None, group_name yaml_str = cast(OfficialRuleFileFormat, yaml_str) groups = yaml_str["groups"] elif self._is_single_rule_format(cast(SingleRuleFormat, yaml_str), self.rule_type): - # convert to list of groups - # group name is made up from the file name yaml_str = cast(SingleRuleFormat, yaml_str) if not group_name: - # Note: the caller of this function should make sure this never happens: + # Note: the caller of this function should ensure this never happens: # Either we use the standard format, or we'd pass a group_name. # If/when we drop support for the single-rule-per-file format, this won't # be needed anymore. - group_name = hashlib.shake_256(str(yaml_str).encode("utf-8")).hexdigest(20) + group_name = hashlib.shake_256(str(yaml_str).encode("utf-8")).hexdigest(10) + else: + group_name = self._sanitize_metric_name(group_name) + + # convert to list of groups to match official rule format groups = [{"name": group_name, "rules": [yaml_str]}] else: # invalid/unsupported @@ -276,7 +329,9 @@ def _from_str(self, yaml_str: str, *, group_name: Optional[str]=None, group_name for group in groups: if not self._is_already_modified(group["name"]): # update group name with topology and sub-path - group["name"] = "_".join(filter(None, [group_name_prefix, group_name, f"{self.rule_type}s"])) + group["name"] = "_".join( + filter(None, [group_name_prefix, group["name"], f"{self.rule_type}s"]) + ) # add "juju_" topology labels for rule in group["rules"]: @@ -303,45 +358,6 @@ def _from_str(self, yaml_str: str, *, group_name: Optional[str]=None, group_name return groups - def _from_file( # noqa: C901 - self, root_path: Path, file_path: Path - ) -> List[OfficialRuleFileItem]: - """Read a rules file from path, injecting juju topology. - - Args: - root_path: full path to the root rules folder (used only for generating group name) - file_path: full path to a *.rule file. - - Returns: - A list of dictionaries representing the rules file, if file is valid (the structure is - formed by `yaml.safe_load` of the file); an empty list otherwise. - """ - with file_path.open() as rf: - # Load a list of rules from file then add labels and filters - try: - rule_file = yaml.safe_load(rf) - - except Exception as e: - logger.error("Failed to read rules from %s: %s", file_path.name, e) - return [] - - # Generate group name prefix - # - name, from juju topology - # - suffix, from the relative path of the rule file; - rel_path = os.path.relpath(os.path.dirname(file_path), root_path) - rel_path = "" if rel_path == "." else rel_path.replace(os.path.sep, "_") - group_name_parts = [self.topology.identifier] if self.topology else [] - group_name_parts.append(rel_path) - group_name_prefix = "_".join(filter(None, group_name_parts)) - - try: - groups = self._from_str(rule_file, group_name_prefix=group_name_prefix) - except ValueError as e: - logger.error("Invalid rules file: %s (%s)", file_path.name, e) - return [] - - return groups - def _is_already_modified(self, name: str) -> bool: """Detect whether a group name has already been modified with juju topology.""" modified_matcher = re.compile(r"^.*?_[\da-f]{8}_.*?alerts$") @@ -349,12 +365,31 @@ def _is_already_modified(self, name: str) -> bool: return False return True + def _sanitize_metric_name(self, metric_name: str) -> str: + """Sanitize a metric name according to https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels.""" + return "".join(char if re.match(r"[a-zA-Z0-9_:]", char) else "_" for char in metric_name) + # ---- END STATIC HELPER METHODS --- # - def add(self, yaml_str: str): - temp = self._from_str(yaml_str) - pprint(temp) - self.groups.extend(temp) + def add( + self, + yaml_str: str, + group_name: Optional[str] = None, + group_name_prefix: Optional[str] = None, + ) -> None: + """Add rules from a string to the existing ruleset. + + Args: + yaml_str: a single-rule or official-rule YAML string + group_name: a custom group name, used only if the new rule is of single-rule format + group_name_prefix: a custom group name prefix, used only if the new rule is of single-rule format + """ + kwargs = {} + if group_name is not None: + kwargs["group_name"] = group_name + if group_name_prefix is not None: + kwargs["group_name_prefix"] = group_name_prefix + self.groups.extend(self._from_str(yaml_str, **kwargs)) def add_path(self, dir_path: Union[str, Path], *, recursive: bool = False) -> None: """Add rules from a dir path. @@ -365,9 +400,6 @@ def add_path(self, dir_path: Union[str, Path], *, recursive: bool = False) -> No Args: dir_path: either a rules file or a dir of rules files. recursive: whether to read files recursively or not (no impact if `path` is a file). - - Returns: - True if path was added else False. """ path = Path(dir_path) if isinstance(dir_path, str) else dir_path if path.is_dir(): diff --git a/tests/test_rules_promql.py b/tests/test_rules_promql.py index be4c6a3..034f0b0 100644 --- a/tests/test_rules_promql.py +++ b/tests/test_rules_promql.py @@ -41,6 +41,7 @@ def test_each_alert_rule_is_topology_labeled(self): self.assertIn("juju_application", labels) self.assertIn("juju_model_uuid", labels) # alerts should not have unit information if not already present + # https://discourse.charmhub.io/t/juju-topology-labels/8874 self.assertNotIn("juju_unit", rule["labels"]) self.assertNotIn("juju_unit=", rule["expr"]) else: @@ -76,47 +77,188 @@ def test_each_alert_expression_is_topology_labeled(self): self.assertIn("juju_model_uuid", labels) self.assertIn("juju_application", labels) - def test_injecting_generic_alerts(self): - ri = AlertRules( - query_type="promql", - topology=JujuTopology( - model="unittest", - model_uuid=str(uuid.uuid4()), - unit="tester/0", - application="tester", - ), - ) - ri.add_path(Path(__file__).resolve().parent / "promql_rules" / "prometheus_alert_rules") - - # TODO SingleRuleFormat only require alert, expr, labels. Test others though - generic_alert_rules = """ - groups: - - name: GenericRules - rules: - - alert: HostDown - expr: up < 1 - for: 5m - labels: - severity: critical - - alert: HostUnavailable - expr: absent(up) - for: 5m - labels: - severity: critical - """ - - ri.add(generic_alert_rules) - alerts = ri.as_dict() - self.assertIn("groups", alerts) - self.assertEqual(len(alerts["groups"]), 5) - group = alerts["groups"][0] - for rule in group["rules"]: - self.assertIn("expr", rule) - for labels in expression_labels(rule["expr"]): - self.assertIn("juju_model", labels) - self.assertIn("juju_model_uuid", labels) - self.assertIn("juju_application", labels) +class TestAddRuleFromStr(unittest.TestCase): + def setUp(self): + rule = """ + alert: SomeRuleName + expr: up + labels: + severity: critical + """ + self.rule = yaml.safe_load(textwrap.dedent(rule)) + + def test_official_rule_add_alerts_from_string(self): + official_rule = """ + groups: + - name: SomeGroupName + rules: + - alert: FooRule + expr: up < 1 + labels: + severity: critical + - alert: BarRule + expr: absent(up) + labels: + severity: critical + """ + added_group = yaml.safe_load(textwrap.dedent(official_rule))["groups"][0] + expected_rules = { + "groups": [ + { + "name": "initial_alerts", + "rules": [self.rule], + }, + {"name": "SomeGroupName_alerts", "rules": added_group["rules"]}, + ] + } + # GIVEN an alert rule + rules = AlertRules(query_type="promql") + rules.groups = rules._from_str(self.rule, group_name="initial") + # WHEN a rule is added from string in official-rule format + rules.add(yaml.safe_load(textwrap.dedent(official_rule))) + rules_dict = rules.as_dict() + # THEN the new rule is a combination of all + self.assertEqual({}, DeepDiff(expected_rules, rules_dict)) + + def test_single_rule_add_alerts_from_string(self): + single_rule = """ + alert: BarRule + expr: up < 1 + labels: + severity: critical + """ + rules = yaml.safe_load(textwrap.dedent(single_rule)) + expected_rules = { + "groups": [ + { + "name": "initial_alerts", + "rules": [self.rule], + }, + {"name": "some_new_alerts", "rules": [rules]}, + ] + } + # GIVEN an alert rule + rules = AlertRules(query_type="promql") + rules.groups = rules._from_str(self.rule, group_name="initial") + # WHEN a rule is added from string in single-rule format with a custom group name and prefix + rules.add( + yaml.safe_load(textwrap.dedent(single_rule)), + group_name="new", + group_name_prefix="some", + ) + rules_dict = rules.as_dict() + # THEN the new rule is a combination of all + # AND the added rule group name has the custom group name and prefix + self.assertEqual({}, DeepDiff(expected_rules, rules_dict)) + + +class TestFromStrGroupName(unittest.TestCase): + def setUp(self): + official_rule = """ + groups: + - name: SomeGroupName + rules: + - alert: SomeRuleName + expr: up < 1 + for: 5m + labels: + severity: critical + """ + single_rule = """ + alert: SomeRuleName + expr: up < 1 + for: 5m + labels: + severity: critical + """ + self.official_rule = yaml.safe_load(textwrap.dedent(official_rule)) + self.single_rule = yaml.safe_load(textwrap.dedent(single_rule)) + + def test_single_rule_from_string_group_sanitized(self): + # GIVEN an alert rule in single-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string and provided an illegal custom group name + groups = rules._from_str(self.single_rule, group_name="Foo$123/Hello:World(go_od)bye") + for group in groups: + # THEN the group name only contains characters in [a-zA-Z0-9_:] + # AND specials characters are replaced with "_" + self.assertEqual(group["name"], "Foo_123_Hello:World_go_od_bye_alerts") + + def test_single_rule_from_string(self): + # GIVEN an alert rule in single-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string + groups = rules._from_str(self.single_rule) + for group in groups: + # THEN group name contains hash + self.assertNotEqual(get_hash(group["name"]), "") + + def test_single_rule_from_string_custom_group(self): + # GIVEN an alert rule in single-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string and provided custom group name + groups = rules._from_str(self.single_rule, group_name="foo") + for group in groups: + # THEN group name does not contain hash + self.assertEqual(get_hash(group["name"]), "") + # AND group name contains the custom group name + self.assertIn("foo", group["name"]) + + def test_single_rule_from_string_custom_group_prefix(self): + # GIVEN an alert rule in single-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string and provided custom group name prefix + groups = rules._from_str(self.single_rule, group_name_prefix="foo") + for group in groups: + # THEN group name does not include the original group name + self.assertNotIn("SomeGroupName", group["name"]) + # AND group name is prefixed with custom value + self.assertTrue(group["name"].startswith("foo")) + + def test_official_rule_from_string(self): + # GIVEN an alert rule in official-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string + groups = rules._from_str(self.official_rule) + for group in groups: + # THEN group name matches the group name in the alert + self.assertIn("SomeGroupName", group["name"]) + + def test_official_rule_from_string_custom_group_prefix(self): + # GIVEN an alert rule in official-rule format + rules = AlertRules(query_type="promql") + # WHEN processed as string and provided custom group name prefix + groups = rules._from_str(self.official_rule, group_name_prefix="foo") + for group in groups: + # THEN group name includes the original group name + self.assertIn("SomeGroupName", group["name"]) + # AND group name is prefixed with custom value + self.assertTrue(group["name"].startswith("foo")) + + def test_raises_value_error_empty_str(self): + rules = AlertRules(query_type="promql") + with self.assertRaises(ValueError) as ctx: + rules._from_str("") + self.assertEqual(str(ctx.exception), "Empty") + + def test_raises_value_error_not_dict(self): + rules = AlertRules(query_type="promql") + with self.assertRaises(ValueError) as ctx: + rules._from_str("[]") + self.assertEqual(str(ctx.exception), "Invalid rules (must be a dict)") + + def test_raises_value_error_invalid_rule_format(self): + invalid_rule = """ + expr: up < 1 + for: 5m + labels: + severity: critical + """ + rules = AlertRules(query_type="promql") + with self.assertRaises(ValueError) as ctx: + rules._from_str(yaml.safe_load(textwrap.dedent(invalid_rule))) + self.assertEqual(str(ctx.exception), "Invalid rule format") def sorted_matchers(matchers) -> str: @@ -142,6 +284,27 @@ def expression_labels(expr): yield labels +def get_hash(group_name: str) -> str: + """Extract the hash of the group name when a group name was not provided to _from_str method in the Rules class. + + This occurs when the single-rule format is provided rather than official-rule format. + + Args: + group_name: a string representing the group name of the rules. + + Returns: + the matching hexdigest of the group name if a match is found, otherwise an empty string. + """ + import re + + pattern = r"([a-f0-9]{20})_" + match = re.search(pattern, group_name) + if match: + return match.group(1) + else: + return "" + + class TestAlertRulesWithOneRulePerFile(unittest.TestCase): def setUp(self) -> None: free_standing_rule = {