WIP: add log_filter toolset to filter out unnecessary logs #506
Conversation
WalkthroughA new log filtering toolset was introduced, featuring dynamic log filter generation based on pod labels using a YAML configuration. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Holmes
participant LogFilterToolset
participant ConfigFile
User->>Holmes: Request log filter for pod label
Holmes->>LogFilterToolset: Invoke LogFilterTool
LogFilterToolset->>ConfigFile: Load and parse log_filter.yaml
ConfigFile-->>LogFilterToolset: Return label-filter mappings
LogFilterToolset-->>Holmes: Return regex filter (or default)
Holmes-->>User: Provide generated log filter
sequenceDiagram
participant User
participant Holmes
participant KubernetesLogsToolset
User->>Holmes: Request logs excluding lines matching regex
Holmes->>KubernetesLogsToolset: Invoke kubectl_logs_grep_no_match
KubernetesLogsToolset->>Holmes: Run kubectl logs | grep -v -P <regex>
Holmes-->>User: Return filtered logs
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mainred
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(1 hunks)holmes/plugins/toolsets/log_filter.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
holmes/plugins/toolsets/log_filter.py (1)
holmes/core/tools.py (7)
CallablePrerequisite(310-311)StructuredToolResult(35-58)Tool(130-160)ToolParameter(124-127)ToolResultStatus(29-32)Toolset(323-475)ToolsetTag(114-117)
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/log_filter.py
[refactor] 31-31: Too few public methods (0/2)
(R0903)
[refactor] 36-36: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (7)
holmes/plugins/toolsets/kubernetes_logs.yaml (1)
44-46: LGTM! Well-designed log exclusion tool.The new tool follows the established patterns in this toolset and correctly uses
grep -v -Pfor regex-based exclusion filtering. The parameter naming (pod_log_filter) aligns with the new LogFilterToolset functionality.holmes/plugins/toolsets/__init__.py (2)
6-6: LGTM! Proper import organization.The import reorganization improves code structure by grouping related imports together and moving standard library imports to the top.
Also applies to: 9-9, 23-23
78-78: LGTM! Correct toolset integration.The LogFilterToolset is properly added to the list of Python toolsets, ensuring it will be loaded and available in the Holmes system.
holmes/plugins/toolsets/log_filter.py (4)
31-37: Ignore static analysis warnings for Pydantic models.The Pylint warnings about "too few public methods" are false positives for Pydantic models, which primarily serve as data containers with built-in validation methods.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 31-31: Too few public methods (0/2)
(R0903)
[refactor] 36-36: Too few public methods (0/2)
(R0903)
54-68: Excellent prerequisite validation design.The callable prerequisite properly validates the log filter configuration during toolset initialization, providing early feedback if the config is invalid. The error handling gracefully allows the toolset to be enabled when no config path is provided.
101-104: LGTM! Robust label matching logic.The label matching correctly handles comma-separated label lists with exact string matching, which is appropriate for Kubernetes label selectors.
106-138: Excellent error handling with graceful fallbacks.The implementation properly handles all error scenarios (missing config, missing labels, parsing errors) by falling back to a sensible default filter. The logging provides good debugging information.
| def load_log_filter_config(file_path: str) -> str: | ||
| """Reads a file, either local or remote. | ||
|
|
||
| Args: | ||
| file_path: The path to the file, can be a local path or a URL. | ||
|
|
||
| Returns: | ||
| The content of the file as a string, or None if an error occurs. | ||
| """ | ||
| if file_path.startswith("http://") or file_path.startswith("https://"): | ||
| # Handle remote file (URL) | ||
| response = requests.get(file_path) | ||
| response.raise_for_status() # Raise an exception for bad status codes | ||
| return response.text | ||
| # Handle local file | ||
| if os.path.exists(file_path) and os.path.isfile(file_path): | ||
| with open(file_path, "r") as file: | ||
| return file.read() | ||
| raise FileNotFoundError(f"File not found: {file_path}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address security and documentation concerns.
Two issues need attention:
- Security: The function accepts arbitrary URLs without validation, which could be exploited for SSRF attacks
- Documentation: The docstring incorrectly states it returns "None if an error occurs" when it actually raises exceptions
def load_log_filter_config(file_path: str) -> str:
- """Reads a file, either local or remote.
+ """Reads a file, either local or remote.
Args:
file_path: The path to the file, can be a local path or a URL.
Returns:
- The content of the file as a string, or None if an error occurs.
+ The content of the file as a string.
+
+ Raises:
+ FileNotFoundError: If the local file doesn't exist.
+ requests.RequestException: If the remote URL request fails.
"""
if file_path.startswith("http://") or file_path.startswith("https://"):
# Handle remote file (URL)
+ # Validate URL to prevent SSRF attacks
+ if not _is_safe_url(file_path):
+ raise ValueError(f"Unsafe URL provided: {file_path}")
response = requests.get(file_path)
response.raise_for_status() # Raise an exception for bad status codes
return response.textConsider adding URL validation:
def _is_safe_url(url: str) -> bool:
"""Validate URL to prevent SSRF attacks."""
from urllib.parse import urlparse
parsed = urlparse(url)
# Add appropriate validation logic based on your security requirements
# e.g., whitelist domains, block private IP ranges, etc.
return parsed.scheme in ('http', 'https') and parsed.netloc🤖 Prompt for AI Agents
In holmes/plugins/toolsets/log_filter.py around lines 141 to 159, the function
load_log_filter_config lacks URL validation, exposing it to SSRF risks, and its
docstring incorrectly states it returns None on error instead of raising
exceptions. Fix this by adding a URL validation helper function that checks the
URL scheme and domain against a whitelist or other security criteria before
making the request. Also, update the docstring to accurately reflect that
exceptions are raised on errors rather than returning None.
| if file_path.startswith("http://") or file_path.startswith("https://"): | ||
| # Handle remote file (URL) | ||
| response = requests.get(file_path) | ||
| response.raise_for_status() # Raise an exception for bad status codes | ||
| return response.text |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to prevent hanging requests.
Remote HTTP requests should have a timeout to prevent the tool from hanging indefinitely if the remote server is unresponsive.
- response = requests.get(file_path)
+ response = requests.get(file_path, timeout=30)📝 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.
| if file_path.startswith("http://") or file_path.startswith("https://"): | |
| # Handle remote file (URL) | |
| response = requests.get(file_path) | |
| response.raise_for_status() # Raise an exception for bad status codes | |
| return response.text | |
| if file_path.startswith("http://") or file_path.startswith("https://"): | |
| # Handle remote file (URL) | |
| response = requests.get(file_path, timeout=30) | |
| response.raise_for_status() # Raise an exception for bad status codes | |
| return response.text |
🤖 Prompt for AI Agents
In holmes/plugins/toolsets/log_filter.py around lines 150 to 154, the HTTP
request to fetch remote files lacks a timeout, which can cause the program to
hang indefinitely if the server is unresponsive. Add a timeout parameter to the
requests.get call, specifying a reasonable timeout duration (e.g., 5 or 10
seconds) to ensure the request fails fast and prevents hanging.
| parameters={ | ||
| "label": ToolParameter( | ||
| type="string", | ||
| description="The pod label to filter logs by. For example, 'app=my-app'.", | ||
| ), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parameter validation inconsistency.
The tool parameter is marked as required=True (default), but the _invoke method handles the case where the label is None. This creates an inconsistency between the parameter definition and the implementation.
"label": ToolParameter(
type="string",
description="The pod label to filter logs by. For example, 'app=my-app'.",
+ required=False,
),📝 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.
| parameters={ | |
| "label": ToolParameter( | |
| type="string", | |
| description="The pod label to filter logs by. For example, 'app=my-app'.", | |
| ), | |
| }, | |
| parameters={ | |
| "label": ToolParameter( | |
| type="string", | |
| description="The pod label to filter logs by. For example, 'app=my-app'.", | |
| required=False, | |
| ), | |
| }, |
🤖 Prompt for AI Agents
In holmes/plugins/toolsets/log_filter.py around lines 79 to 84, the "label"
parameter is defined as required but the _invoke method allows it to be None,
causing inconsistency. To fix this, explicitly set required=False in the
ToolParameter definition for "label" to match the _invoke method's handling of
optional labels.
No description provided.