ROB-0000 add labels to robusta alerts using subject information#2065
ROB-0000 add labels to robusta alerts using subject information#2065
Conversation
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:0b9174b
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:0b9174b me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0b9174b
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0b9174bPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:0b9174b |
WalkthroughThis PR adds support for label relabeling in findings via configurable regex rules. It introduces a ChangesFinding Label Relabeling Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_customise_finding_relabel.py (1)
179-208: ⚡ Quick winAdd a malformed-regex regression test.
Coverage is solid; one high-value gap is a rule with invalid
regexto ensurecustomise_findingskips the bad rule without crashing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_customise_finding_relabel.py` around lines 179 - 208, Add a test that verifies customise_finding/FindingOverrides tolerate an invalid regex by creating a FindingSubject (use _run or _make_event) and a rule where "regex" is malformed (e.g. unmatched bracket), then call customise_finding or _run with that rule and assert the function does not raise and the finding's labels remain unchanged (e.g. existing "team" label stays the same or target_label is not added); reference customise_finding, FindingOverrides, _run, _make_event and FindingSubject to locate where to add the test and ensure the bad rule is skipped rather than crashing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/robusta_playbooks/common_actions.py`:
- Around line 110-111: The current logging in customise_finding logs full
labels_to_inject which can leak sensitive metadata; change the call in the
customise_finding block so you do not log full label values—either downgrade to
logging.debug or log only the label keys (e.g., list(labels_to_inject.keys()))
before calling event.inject_finding_labels(labels_to_inject); keep the injection
via event.inject_finding_labels unchanged but ensure only non-sensitive key
names or a debug-level message is emitted.
- Around line 102-103: customise_finding currently calls
re.fullmatch(rule.regex, source_value) which will raise re.error for malformed
user regexes; wrap the re.fullmatch call in a try/except catching re.error, log
the invalid pattern and exception (including rule.regex and any identifying rule
id) using the existing logger, then continue to the next rule so the bad pattern
is skipped instead of crashing customise_finding.
In `@src/robusta/core/model/events.py`:
- Around line 158-161: The issue is that Finding uses a shared mutable default
FindingSubject which causes inject_finding_labels to mutate a shared labels
dict; fix by changing the Finding constructor signature (class Finding) to use
subject: Optional[FindingSubject] = None instead of a default instance, and
inside the constructor create a fresh FindingSubject() when subject is None
(assign self.subject = subject or FindingSubject()); keep inject_finding_labels
as-is but this ensures each Finding has its own subject.labels so .update()
won't bleed labels across Findings.
---
Nitpick comments:
In `@tests/test_customise_finding_relabel.py`:
- Around line 179-208: Add a test that verifies
customise_finding/FindingOverrides tolerate an invalid regex by creating a
FindingSubject (use _run or _make_event) and a rule where "regex" is malformed
(e.g. unmatched bracket), then call customise_finding or _run with that rule and
assert the function does not raise and the finding's labels remain unchanged
(e.g. existing "team" label stays the same or target_label is not added);
reference customise_finding, FindingOverrides, _run, _make_event and
FindingSubject to locate where to add the test and ensure the bad rule is
skipped rather than crashing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e57f4745-7ead-4c6f-b04e-e9ac2d6aa0d8
📒 Files selected for processing (3)
playbooks/robusta_playbooks/common_actions.pysrc/robusta/core/model/events.pytests/test_customise_finding_relabel.py
| m = re.fullmatch(rule.regex, source_value) | ||
| if m: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n playbooks/robusta_playbooks/common_actions.py | head -150 | tail -60Repository: robusta-dev/robusta
Length of output: 3457
🏁 Script executed:
head -50 playbooks/robusta_playbooks/common_actions.pyRepository: robusta-dev/robusta
Length of output: 2026
🏁 Script executed:
sed -n '70,110p' playbooks/robusta_playbooks/common_actions.pyRepository: robusta-dev/robusta
Length of output: 2220
Guard malformed regex rules to avoid crashing customise_finding.
At line 102, re.fullmatch() will raise re.error on invalid user-provided regex patterns in finding_label_rules, causing the entire action to crash instead of skipping the bad rule. Wrap the call in a try-except block, log the invalid pattern, and continue to the next rule:
Proposed fix
- m = re.fullmatch(rule.regex, source_value)
+ try:
+ m = re.fullmatch(rule.regex, source_value)
+ except re.error:
+ logging.warning("[customise_finding] invalid finding_label_rules regex: %r", rule.regex)
+ continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m = re.fullmatch(rule.regex, source_value) | |
| if m: | |
| try: | |
| m = re.fullmatch(rule.regex, source_value) | |
| except re.error: | |
| logging.warning("[customise_finding] invalid finding_label_rules regex: %r", rule.regex) | |
| continue | |
| if m: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/robusta_playbooks/common_actions.py` around lines 102 - 103,
customise_finding currently calls re.fullmatch(rule.regex, source_value) which
will raise re.error for malformed user regexes; wrap the re.fullmatch call in a
try/except catching re.error, log the invalid pattern and exception (including
rule.regex and any identifying rule id) using the existing logger, then continue
to the next rule so the bad pattern is skipped instead of crashing
customise_finding.
| logging.info(f"[customise_finding] injecting labels into finding: {labels_to_inject}") | ||
| event.inject_finding_labels(labels_to_inject) |
There was a problem hiding this comment.
Avoid logging injected label values at INFO level.
At Line 110, logging full labels_to_inject may leak sensitive metadata into centralized logs. Prefer logging only keys (or downgrade to debug).
🛠️ Proposed fix
- logging.info(f"[customise_finding] injecting labels into finding: {labels_to_inject}")
+ logging.debug(
+ "[customise_finding] injecting %d labels into finding (keys=%s)",
+ len(labels_to_inject),
+ sorted(labels_to_inject.keys()),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logging.info(f"[customise_finding] injecting labels into finding: {labels_to_inject}") | |
| event.inject_finding_labels(labels_to_inject) | |
| logging.debug( | |
| "[customise_finding] injecting %d labels into finding (keys=%s)", | |
| len(labels_to_inject), | |
| sorted(labels_to_inject.keys()), | |
| ) | |
| event.inject_finding_labels(labels_to_inject) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/robusta_playbooks/common_actions.py` around lines 110 - 111, The
current logging in customise_finding logs full labels_to_inject which can leak
sensitive metadata; change the call in the customise_finding block so you do not
log full label values—either downgrade to logging.debug or log only the label
keys (e.g., list(labels_to_inject.keys())) before calling
event.inject_finding_labels(labels_to_inject); keep the injection via
event.inject_finding_labels unchanged but ensure only non-sensitive key names or
a debug-level message is emitted.
| def inject_finding_labels(self, labels: Dict[str, str]): | ||
| for sink in self.named_sinks: | ||
| for finding in self.sink_findings[sink]: | ||
| finding.subject.labels.update(labels) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify mutable default subject exists
rg -nP 'subject:\s*FindingSubject\s*=\s*FindingSubject\(\)' src/robusta/core/reporting/base.py
# Verify default findings are created without explicit subject
rg -nP 'def create_default_finding|return Finding\(title="Robusta notification"' src/robusta/core/model/events.pyRepository: robusta-dev/robusta
Length of output: 264
🏁 Script executed:
# Read the inject_finding_labels method context
sed -n '155,165p' src/robusta/core/model/events.pyRepository: robusta-dev/robusta
Length of output: 505
🏁 Script executed:
# Check FindingSubject class definition and labels attribute
rg -A 20 'class FindingSubject' src/robusta/core/reporting/base.py | head -40Repository: robusta-dev/robusta
Length of output: 728
🏁 Script executed:
# Verify the Finding class __init__ with the mutable default argument
sed -n '250,270p' src/robusta/core/reporting/base.pyRepository: robusta-dev/robusta
Length of output: 862
Prevent cross-event label bleed from shared default subjects.
At line 161, inject_finding_labels mutates finding.subject.labels in place. The Finding class uses a mutable default argument for the subject parameter at line 261 of src/robusta/core/reporting/base.py. This causes all Finding instances created without an explicit subject to share the same FindingSubject instance, including its labels dict. The .update() call then mutates this shared dict, leaking labels across unrelated findings and events.
The codebase already acknowledges this as bug-prone (see TODO comment at line 258-259). Fix by changing the default parameter to Optional[FindingSubject] = None and instantiating a new FindingSubject() in the method body when subject is None.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/robusta/core/model/events.py` around lines 158 - 161, The issue is that
Finding uses a shared mutable default FindingSubject which causes
inject_finding_labels to mutate a shared labels dict; fix by changing the
Finding constructor signature (class Finding) to use subject:
Optional[FindingSubject] = None instead of a default instance, and inside the
constructor create a fresh FindingSubject() when subject is None (assign
self.subject = subject or FindingSubject()); keep inject_finding_labels as-is
but this ensures each Finding has its own subject.labels so .update() won't
bleed labels across Findings.
1 participant
No description provided.