Skip to content

feat: replace local CosTool fork with cosl implementation - #607

Merged
lucabello merged 7 commits into
mainfrom
copilot/use-cos-lib-instead-of-fork
May 7, 2026
Merged

lucabello merged 7 commits into
mainfrom
copilot/use-cos-lib-instead-of-fork

Conversation

Copilot AI commented May 6, 2026

Copy link
Copy Markdown
Contributor

Issue

v1/loki_push_api.py maintained a local fork of CosTool that diverged from the upstream cosl implementation, contributing to bugs from the deviation. AlertRules had already been migrated to cosl.rules in a prior change; CosTool remained local.

Solution

  • Import CosTool from cosl instead of defining it locally: from cosl import CosTool, JujuTopology
  • Update instantiation in LokiPushApiProvider.__init__: CosTool("logql") — cosl's CosTool takes a default_query_type instead of a charm reference; "logql" is correct for all Loki query expressions
  • Remove the local CosTool class (~120 lines of duplicated/drifted code)
  • Remove orphaned helpers _is_official_alert_rule_format and _is_single_alert_rule_format — unused in v1 since AlertRules was moved to cosl
  • Drop subprocess and tempfile imports — only needed by the removed local class
  • Bump LIBPATCH 23 → 25
  • Pin cosl>=1.9.1 in pyproject.toml; regenerate uv.lock
  • Guard app-level relation data writes in LokiPushApiProvider.alerts with a leader check — only the leader may write to the app data bag; non-leaders attempting this write would fail with "permission denied"
  • Guard _has_alert_rule_errors() in charm.py with an early if not self.unit.is_leader(): return False — non-leader units running non-relation hooks (e.g. pebble-ready) cannot read their own app's relation data and would crash with "permission denied"

InvalidAlertRulePathError and _resolve_dir_against_charm_path are retained — they are charm-specific (resolve paths against charm.charm_dir) with no cosl equivalent.

Per the issue guidance, v0/loki_push_api.py is unchanged.

Context

The cosl CosTool differs from the previous local fork in two key ways:

  • Constructor: takes default_query_type: Optional[QueryType] instead of a charm object; query type flows to all method calls automatically
  • Uses platform.machine() (correct) instead of platform.processor() (unreliable in containers/CI) to locate the cos-tool binary

In Juju, non-leader units running non-relation hooks (like pebble-ready) get "permission denied" when calling relation-get --app to access their own application's relation data bag. Any read or write of relation.data[self.app] outside a relation hook must be guarded by self.unit.is_leader().

Testing Instructions

tox -e unit

Note: tests that validate topology label injection into LogQL expressions (test_rules_have_correct_labels) require the cos-tool binary, which tox -e unit downloads automatically via curl. These tests pass in CI; they fail locally without the binary — this is pre-existing behaviour unrelated to this change (those tests target v0, which is unchanged).

Upgrade Notes

Users of LokiPushApiProvider from charms.loki_k8s.v1.loki_push_api should ensure cosl>=1.9.1 is available in their charm's environment. No API-level changes to the public interface.

Copilot AI linked an issue May 6, 2026 that may be closed by this pull request
- Replace local CosTool class with cosl's implementation
- Remove unused helper functions _is_official_alert_rule_format and _is_single_alert_rule_format
- Remove now-unused subprocess and tempfile imports
- Bump LIBPATCH to 24
- Update pyproject.toml to require cosl>=1.9.1
- Regenerate uv.lock

Agent-Logs-Url: https://github.com/canonical/loki-k8s-operator/sessions/c05ad25f-2364-4e26-ba4e-e72a980f28f4

Co-authored-by: lucabello <36242061+lucabello@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor to use cos-lib instead of a fork of AlertRules feat(lib/v1): replace local CosTool fork with cosl implementation May 6, 2026
Copilot AI requested a review from lucabello May 6, 2026 07:19
@lucabello lucabello changed the title feat(lib/v1): replace local CosTool fork with cosl implementation feat: replace local CosTool fork with cosl implementation May 6, 2026
lucabello and others added 2 commits May 6, 2026 09:53
The cosl CosTool properly captures stderr from cos-tool validation,
meaning invalid alert rules are now correctly intercepted before being
pushed to Loki. Previously, the old local CosTool had a bug where
stderr was not captured, causing validation errors to be silently
ignored and bad rules to flow through to Loki's HTTP API.

This fix ensures the charm goes blocked when the provider detects
validation errors in relation data, maintaining the expected behavior
that invalid rules from consumers result in blocked status.

Also fixes unit test ALERT_RULES to use valid LogQL instead of the
%%juju_topology%% placeholder (which is only substituted when topology
labels are present).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lucabello

Copy link
Copy Markdown
Contributor

@copilot Can you figure out why those integration tests are failing and provide a fix?

…units

Non-leader units running pebble-ready hooks cannot access their own app's
relation data (permission denied in Juju). Fix both the charm's
_has_alert_rule_errors and the library's alerts property to only read/write
app-level relation data when the unit is the leader.

Agent-Logs-Url: https://github.com/canonical/loki-k8s-operator/sessions/4e9aa569-5770-42e9-a35f-4d6ff98120f8

Co-authored-by: lucabello <36242061+lucabello@users.noreply.github.com>

Copilot AI commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Can you figure out why those integration tests are failing and provide a fix?

The failures were caused by two related bugs introduced in the commit that added _has_alert_rule_errors:

  1. _has_alert_rule_errors in charm.py: Accessing relation.data[self.app] (our own app's relation data bag) fails with "permission denied" for non-leader units when running outside a relation hook (e.g. pebble-ready). Fixed by returning False early when not self.unit.is_leader().

  2. LokiPushApiProvider.alerts in v1/loki_push_api.py: The write relation.data[self._charm.app]["event"] = ... (error reporting) also has no leader guard. If cos-tool were present and a validation error occurred, this would similarly crash on non-leaders. Fixed by guarding the write with if self._charm.unit.is_leader().

Commit: e554754

@lucabello
lucabello marked this pull request as ready for review May 7, 2026 09:36
@lucabello
lucabello requested a review from a team as a code owner May 7, 2026 09:36
@lucabello

Copy link
Copy Markdown
Contributor

Diff: CosTool class — loki-k8s-operator (main) vs cos-lib (main)

Left (-): lib/charms/loki_k8s/v0/loki_push_api.py in canonical/loki-k8s-operator
Right (+): src/cosl/cos_tool.py in canonical/cos-lib

--- costool_loki.py	2026-05-07 14:48:03.287869146 +0200
+++ costool_coslib.py	2026-05-07 14:48:03.292869183 +0200
@@ -1,11 +1,18 @@
 class CosTool:
-    """Uses cos-tool to inject label matchers into alert rule expressions and validate rules."""
+    """Uses cos-tool to inject label matchers into alert rule expressions and validate rules.
+
+    Args:
+        default_query_type: an optional querytype to use for all invocations of this class, if
+          not specified per-method. Either :default_query_type: or per-method :query_type:
+          **must** be used, or a :TypeError: will be raised.
+    """
 
     _path = None
     _disabled = False
+    query_type: Union[QueryType, None] = None
 
-    def __init__(self, charm):
-        self._charm = charm
+    def __init__(self, default_query_type: Optional[QueryType] = None):
+        self.query_type = default_query_type
 
     @property
     def path(self):
@@ -19,16 +26,21 @@
                 self._disabled = True
         return self._path
 
-    def apply_label_matchers(self, rules) -> dict:
+    @ensure_querytype
+    def apply_label_matchers(
+        self, rules: OfficialRuleFileFormat, query_type: Optional[QueryType] = None
+    ) -> OfficialRuleFileFormat:
         """Will apply label matchers to the expression of all alerts in all supplied groups."""
+        query_type = query_type or self.query_type
         if not self.path:
             return rules
-        for group in rules["groups"]:
+        for group in rules.get("groups", []):
             rules_in_group = group.get("rules", [])
             for rule in rules_in_group:
                 topology = {}
                 # if the user for some reason has provided juju_unit, we'll need to honor it
                 # in most cases, however, this will be empty
+                labels = rule.get("labels", {})
                 for label in [
                     "juju_model",
                     "juju_model_uuid",
@@ -36,14 +48,18 @@
                     "juju_charm",
                     "juju_unit",
                 ]:
-                    if label in rule["labels"]:
-                        topology[label] = rule["labels"][label]
+                    if label in labels:
+                        topology[label] = labels[label]
 
-                rule["expr"] = self.inject_label_matchers(rule["expr"], topology)
+                rule["expr"] = self.inject_label_matchers(rule["expr"], topology, query_type)  # type: ignore
         return rules
 
-    def validate_alert_rules(self, rules: dict) -> Tuple[bool, str]:
+    @ensure_querytype
+    def validate_alert_rules(
+        self, rules: OfficialRuleFileFormat, query_type: Optional[QueryType] = None
+    ) -> Tuple[bool, str]:
         """Will validate correctness of alert rules, returning a boolean and any errors."""
+        query_type = query_type or self.query_type
         if not self.path:
             logger.debug("`cos-tool` unavailable. Not validating alert correctness.")
             return True, ""
@@ -51,7 +67,8 @@
         with tempfile.TemporaryDirectory() as tmpdir:
             rule_path = Path(tmpdir + "/validate_rule.yaml")
 
-            # Smash "our" rules format into what upstream actually uses, which is more like:
+            # Smash "our" rules format into what upstream actually uses for Loki,
+            # which is more like:
             #
             # groups:
             #   - name: foo
@@ -60,43 +77,74 @@
             #         expr: up
             #       - alert: OtherAlert
             #         expr: up
-            transformed_rules = {"groups": []}  # type: ignore
-            for rule in rules["groups"]:
-                transformed_rules["groups"].append(rule)
+            if query_type == "logql":
+                transformed_rules = OfficialRuleFileFormat(groups=[])
+                for rule in rules.get("groups", []):
+                    transformed_rules.get("groups", []).append(rule)
+
+                rules = transformed_rules
 
-            rule_path.write_text(yaml.dump(transformed_rules))
-            args = [str(self.path), "--format", "logql", "validate", str(rule_path)]
+            rule_path.write_text(yaml.dump(rules))
+
+            args = [str(self.path), "--format", query_type, "validate", str(rule_path)]
             # noinspection PyBroadException
             try:
-                self._exec(args)
+                self._exec(args)  # type: ignore
                 return True, ""
             except subprocess.CalledProcessError as e:
-                logger.debug("Validating the rules failed: %s", e.output)
-                return False, ", ".join([line for line in e.output if "error validating" in line])
-
-    def inject_label_matchers(self, expression, topology) -> str:
+                logger.debug("Validating the rules failed: %s", e.output.decode("utf-8"))
+                return False, ", ".join(
+                    [
+                        line
+                        for line in e.output.decode("utf-8").splitlines()
+                        if "error validating" in line
+                    ]
+                )
+
+    @ensure_querytype
+    def inject_label_matchers(
+        self,
+        expression: str,
+        topology: Dict[str, str],
+        query_type: Optional[QueryType] = None,
+        dashboard_variable: Optional[bool] = False,
+    ) -> str:
         """Add label matchers to an expression."""
+        query_type = query_type or self.query_type
+
         if not topology:
             return expression
         if not self.path:
             logger.debug("`cos-tool` unavailable. Leaving expression unchanged: %s", expression)
             return expression
-        args = [str(self.path), "--format", "logql", "transform"]
+        args = [str(self.path), "--format", query_type, "transform"]
+
+        value_tmpl = r"${}" if dashboard_variable else "{}"
+
+        variable_topology = {k: value_tmpl.format(topology[k]) for k in topology.keys()}
         args.extend(
-            ["--label-matcher={}={}".format(key, value) for key, value in topology.items()]
+            [
+                "--label-matcher={}={}".format(key, value)
+                for key, value in variable_topology.items()
+            ]
         )
 
-        args.extend(["{}".format(expression)])
+        # Pass a leading "--" so expressions with a negation or subtraction aren't interpreted as
+        # flags
+        args.extend(["--", "{}".format(expression)])
         # noinspection PyBroadException
         try:
-            return self._exec(args)
+            return (
+                re.sub(r'="\$juju', r'=~"$juju', self._exec(args))  # type: ignore
+                if dashboard_variable
+                else self._exec(args)  # type: ignore
+            )
         except subprocess.CalledProcessError as e:
             logger.debug('Applying the expression failed: "%s", falling back to the original', e)
-            print('Applying the expression failed: "{}", falling back to the original'.format(e))
             return expression
 
     def _get_tool_path(self) -> Optional[Path]:
-        arch = platform.processor()
+        arch = platform.machine()
         arch = "amd64" if arch == "x86_64" else arch
         res = "cos-tool-{}".format(arch)
         try:
@@ -106,7 +154,6 @@
             logger.debug('Could not locate cos-tool at: "{}"'.format(res))
         return None
 
-    def _exec(self, cmd) -> str:
-        result = subprocess.run(cmd, check=True, stdout=subprocess.PIPE)
-        output = result.stdout.decode("utf-8").strip()
-        return output
+    def _exec(self, cmd: List[str]) -> str:
+        result = subprocess.run(cmd, check=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        return result.stdout.decode("utf-8").strip()

Key differences summary:

  • cos-lib is generic (supports promql/logql via query_type param); loki-k8s-operator hardcodes "logql"
  • cos-lib uses @ensure_querytype decorator for runtime validation
  • cos-lib uses typed signatures (OfficialRuleFileFormat, Dict[str, str], etc.)
  • cos-lib adds dashboard_variable support in inject_label_matchers
  • cos-lib uses platform.machine() (correct); loki uses platform.processor() (can return empty string)
  • cos-lib captures stderr in _exec; loki does not
  • cos-lib properly decodes error output (e.output.decode("utf-8").splitlines()); loki iterates raw bytes
  • cos-lib passes "--" separator before expressions to avoid flag misinterpretation
  • cos-lib removes print() call (uses only logger.debug)
  • cos-lib constructor takes default_query_type instead of charm

Comment thread src/charm.py
Comment thread src/charm.py Outdated
Comment thread lib/charms/loki_k8s/v1/loki_push_api.py Outdated
lucabello and others added 2 commits May 7, 2026 15:21
Co-authored-by: Sina P <55766091+sinapah@users.noreply.github.com>
Signed-off-by: Luca Bello <36242061+lucabello@users.noreply.github.com>
Co-authored-by: Sina P <55766091+sinapah@users.noreply.github.com>
Signed-off-by: Luca Bello <36242061+lucabello@users.noreply.github.com>
@lucabello
lucabello merged commit 5bdc703 into main May 7, 2026
27 of 29 checks passed
@lucabello
lucabello deleted the copilot/use-cos-lib-instead-of-fork branch May 7, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use cos-lib instead of a fork of AlertRules

4 participants