Backend support for CRDs visualization #1947
Conversation
WalkthroughAdds kubectl-based CRD inspection features (new playbook actions and implementation), kubectl installation and Release.key handling in the Docker image, Helm values and RBAC updates, a new KUBECTL_CMD_TIMEOUT_SEC env var, and both user-facing and internal CRD documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Event as Execution Event
participant Runner as Robusta Runner
participant Action as CRD Action
participant Kubectl as kubectl (subprocess)
participant Output as Finding / Block
Event->>Runner: trigger action (params)
Runner->>Action: invoke handler
Action->>Action: build kubectl args (include timeout)
Action->>Kubectl: run command
alt success
Kubectl-->>Action: stdout
Action->>Action: parse & format into Block(s)
Action->>Output: create Finding & attach Block
Output-->>Runner: enriched event
else failure / timeout
Kubectl-->>Action: stderr / exit code
Action->>Runner: raise ActionException (error message)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)src/robusta/core/playbooks/internal/crds.py (8)
🪛 Ruff (0.14.2)src/robusta/core/playbooks/internal/crds.py20-20: (S603) 27-27: Consider moving this statement to an (TRY300) 35-35: Use Replace with (TRY400) 36-36: Within an (B904) 39-39: Use Remove exception name (TRY201) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
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. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
src/robusta/core/model/env_vars.py (1)
143-145: Expose timeout looks good; add docs and minor style nit.Keep default 180; please document how to set KUBECTL_CMD_TIMEOUT_SEC in CRDs docs and consider spacing for consistency.
docs/setup-robusta/crds.rst (1)
86-96: Add note on kubectl timeout configuration.Document KUBECTL_CMD_TIMEOUT_SEC so operators can tune long-running CR listings.
Example:
.. note:: You can control kubectl action timeouts via the KUBECTL_CMD_TIMEOUT_SEC environment variable (default: 180 seconds).src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md (4)
39-51: Specify a language for fenced code blocks.Add a language to this fenced block to pass MD040.
-``` +```text File: production_deployment_nginx-deployment_describe.txt --- Name: nginx-deployment ...
75-89: Use yaml language for YAML examples.Improves syntax highlighting and lints cleanly.
-``` +```yaml File: default_configmap_app-config.yaml --- apiVersion: v1 ...
115-121: Surround tables with blank lines.Add a blank line before and after to satisfy MD058.
-#### Example Response -| Type | Reason | Age | From | Message | +#### Example Response + +| Type | Reason | Age | From | Message | |------|--------|-----|------|---------| ... +
191-196: Surround the second table with blank lines and set header context.Keeps consistent formatting and passes MD058.
-**Table Summary:** -| Kind | API Version | Scope | Plural | +**Table Summary:** + +| Kind | API Version | Scope | Plural | |------|------------|-------|--------| ... +src/robusta/core/playbooks/internal/crds.py (3)
116-176: Use modern Event fields and show a precise timestamp instead of date-only "Age".events.k8s.io/v1 uses reportingController/reportingInstance; source is deprecated. Show lastTimestamp/eventTime ISO or compute relative if desired.
- headers = ["Type", "Reason", "Age", "From", "Message"] + headers = ["Type", "Reason", "Time", "From", "Message"] @@ - last_timestamp = item.get("lastTimestamp") or item.get("eventTime", "") - age = last_timestamp.split("T")[0] if last_timestamp else "Unknown" + last_timestamp = item.get("lastTimestamp") or item.get("eventTime") or item.get("firstTimestamp", "") + time_str = last_timestamp or "Unknown" @@ - source = item.get("source", {}) - from_str = source.get("component", "Unknown") - if source.get("host"): - from_str += f" ({source.get('host')})" + # Prefer modern fields, fallback to deprecated 'source' + reporting_controller = item.get("reportingController") + reporting_instance = item.get("reportingInstance") + if reporting_controller: + from_str = reporting_controller + if reporting_instance: + from_str += f" ({reporting_instance})" + else: + source = item.get("source", {}) or item.get("deprecatedSource", {}) + from_str = source.get("component", "Unknown") + if source.get("host"): + from_str += f" ({source.get('host')})" @@ - rows.append([event_type, reason, age, from_str, message]) + rows.append([event_type, reason, time_str, from_str, message]) @@ - except Exception: + except ActionException: + raise + except Exception as e: msg = f"Error running fetch_resource_events for {params}" logging.exception(msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) from e
179-242: Minor cleanups: remove unused variable and chain exception.Keep exception variable used and re-raise with cause.
- except Exception as e: - msg = "Error running fetch_crds" - logging.exception(msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) + except ActionException: + raise + except Exception as e: + msg = "Error running fetch_crds" + logging.exception(msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) from e
249-310: Preserve ActionException; optional: normalize kind and chain cause.Prevents double-wrapping and improves robustness for mixed-case kinds.
- cmd = ["kubectl", "get", params.kind, "-o", "json"] + kind = params.kind + cmd = ["kubectl", "get", kind, "-o", "json"] @@ - except Exception: + except ActionException: + raise + except Exception as e: msg = f"Error running fetch_cr_instances for {params}" logging.exception(msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(3 hunks)docs/index.rst(1 hunks)docs/setup-robusta/crds.rst(1 hunks)helm/robusta/values.yaml(1 hunks)src/robusta/core/model/env_vars.py(1 hunks)src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md(1 hunks)src/robusta/core/playbooks/internal/crds.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/robusta/core/playbooks/internal/crds.py (7)
tests/conftest.py (1)
robusta(94-108)src/robusta/core/model/base_params.py (1)
ActionParams(60-71)src/robusta/core/playbooks/actions_registry.py (1)
action(16-21)src/robusta/core/model/events.py (2)
ExecutionBaseEvent(49-175)add_finding(129-138)src/robusta/core/reporting/blocks.py (4)
FileBlock(72-133)JsonBlock(303-314)TableBlock(321-448)MarkdownBlock(42-61)src/robusta/core/reporting/base.py (1)
Finding(248-406)src/robusta/utils/error_codes.py (2)
ActionException(42-47)ErrorCodes(5-40)
🪛 Flake8 (7.3.0)
src/robusta/core/playbooks/internal/crds.py
[error] 238-238: local variable 'e' is assigned to but never used
(F841)
🪛 markdownlint-cli2 (0.18.1)
src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
191-191: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Ruff (0.14.2)
src/robusta/core/playbooks/internal/crds.py
17-17: subprocess call: check for execution of untrusted input
(S603)
24-24: Consider moving this statement to an else block
(TRY300)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
36-36: Use raise without specifying exception name
Remove exception name
(TRY201)
77-77: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
112-112: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
175-175: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
238-238: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
241-241: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
310-310: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
- GitHub Check: Deploy docs
🔇 Additional comments (1)
docs/index.rst (1)
90-90: LGTM: navigation entry added.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/robusta/core/playbooks/internal/crds.py (3)
16-37: Critical exception handling gaps in kubectl wrapper.Past review correctly identified that this function needs to:
- Handle
subprocess.TimeoutExpiredseparately with clear messaging- Handle
FileNotFoundErrorfor missing kubectl binary- Use
logging.exception()instead oflogging.error()(line 33)- Chain exceptions with
from e(lines 34, 37)These issues affect debugging and error visibility across all kubectl-based actions.
Apply the fix from the past review:
def run_kubectl_command(command: List[str]) -> str: try: result = subprocess.run( command, capture_output=True, text=True, check=True, timeout=KUBECTL_CMD_TIMEOUT_SEC ) return result.stdout except subprocess.CalledProcessError as e: error_msg = f"kubectl command failed with exit code {e.returncode}\n" error_msg += f"Command: {' '.join(command)}\n" if e.stderr: error_msg += f"Error: {e.stderr}" else: error_msg += f"Error: {e.stdout}" - logging.error(error_msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, error_msg) - except Exception as e: + logging.exception(error_msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, error_msg) from e + except subprocess.TimeoutExpired as e: + logging.exception(f"kubectl command timed out after {KUBECTL_CMD_TIMEOUT_SEC}s: {' '.join(command)}") + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, "kubectl command timed out") from e + except FileNotFoundError as e: + logging.exception("kubectl not found in PATH") + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, "kubectl binary not found") from e + except Exception as e: logging.exception(f"Unexpected error running kubectl command {command}") - raise e + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, str(e)) from e
75-78: Avoid double-wrapping ActionException.The current code catches all exceptions including
ActionExceptionand wraps them again. PreserveActionExceptionas-is and only wrap unknown exceptions.Apply this diff:
+ except ActionException: + raise except Exception: msg = f"Error running kubectl_describe for {params}" logging.exception(msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) from e
81-113: CRITICAL: Block Secret YAML fetches to prevent credential leakage.This function currently allows fetching YAML for any resource, including Secrets, which would expose sensitive credentials. This is a critical security vulnerability.
Apply this fix to block Secrets and support a configurable blocklist:
@action def fetch_resource_yaml(event: ExecutionBaseEvent, params: ResourceParams): """ Fetch resource YAML using kubectl get -o yaml. Output format is a FileBlock containing the YAML. """ + # Block secrets to prevent credential leakage + if params.kind.lower() in {"secret", "secrets"}: + raise ActionException(ErrorCodes.RESOURCE_NOT_PERMITTED, "Fetching Secret YAML is not permitted") + finding = Finding( title=f"YAML for {params.kind}/{params.name}", aggregation_key="fetch_resource_yaml", ) try: cmd = ["kubectl", "get", params.kind, params.name, "-o", "yaml"] if params.namespace: cmd.extend(["-n", params.namespace]) output = run_kubectl_command(cmd) filename = f"{params.kind}_{params.name}.yaml" if params.namespace: filename = f"{params.namespace}_{filename}" file_block = FileBlock( filename=filename, contents=output.encode() ) finding.add_enrichment([file_block]) event.add_finding(finding) + except ActionException: + raise - except Exception: + except Exception as e: msg = f"Error running fetch_resource_yaml for {params}" logging.exception(msg) - raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) + raise ActionException(ErrorCodes.ACTION_UNEXPECTED_ERROR, msg) from e
🧹 Nitpick comments (4)
docs/setup-robusta/crds.rst (1)
39-39: Add language identifier to fenced code block.Specify
textor another appropriate language identifier for the code block to improve rendering and accessibility.Apply this diff:
-.. code-block:: +.. code-block:: textBased on static analysis hints.
src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md (3)
39-50: Add language identifier to fenced code block.Specify
yamlas the language identifier for better syntax highlighting.Apply this diff:
-``` +```text File: production_deployment_nginx-deployment_describe.txtBased on static analysis hints.
115-122: Add blank lines around table.Tables should be surrounded by blank lines per markdown best practices.
Apply this diff:
#### Example Response + | Type | Reason | Age | From | Message |Based on static analysis hints.
193-198: Add blank lines around table.Tables should be surrounded by blank lines per markdown best practices.
Apply this diff:
**Table Summary:** + | Kind | API Version | Scope | Plural | Created At |Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/setup-robusta/crds.rst(1 hunks)helm/robusta/templates/runner-service-account.yaml(1 hunks)src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md(1 hunks)src/robusta/core/playbooks/internal/crds.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/robusta/core/playbooks/internal/crds.py (6)
src/robusta/core/model/base_params.py (1)
ActionParams(60-71)src/robusta/core/playbooks/actions_registry.py (1)
action(16-21)src/robusta/core/model/events.py (2)
ExecutionBaseEvent(49-175)add_finding(129-138)src/robusta/core/reporting/blocks.py (4)
FileBlock(72-133)JsonBlock(303-314)TableBlock(321-448)MarkdownBlock(42-61)src/robusta/core/reporting/base.py (1)
Finding(248-406)src/robusta/utils/error_codes.py (2)
ActionException(42-47)ErrorCodes(5-40)
🪛 Flake8 (7.3.0)
src/robusta/core/playbooks/internal/crds.py
[error] 241-241: local variable 'e' is assigned to but never used
(F841)
🪛 markdownlint-cli2 (0.18.1)
src/robusta/core/playbooks/internal/CRD_API_DOCUMENTATION.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
193-193: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Ruff (0.14.2)
src/robusta/core/playbooks/internal/crds.py
18-18: subprocess call: check for execution of untrusted input
(S603)
25-25: Consider moving this statement to an else block
(TRY300)
33-33: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
34-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
37-37: Use raise without specifying exception name
Remove exception name
(TRY201)
78-78: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
113-113: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
176-176: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
241-241: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
244-244: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
360-360: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
helm/robusta/templates/runner-service-account.yaml (1)
10-17: LGTM! Appropriate RBAC permissions for CRD monitoring.The read-only permissions (list, get) on customresourcedefinitions are correctly scoped for the CRD monitoring feature and follow the principle of least privilege.
src/robusta/core/playbooks/internal/crds.py (1)
288-331: Type coercion issue identified: filter_value always treated as string.The code extracts
filter_valueas a string from the regex pattern and performs a direct==comparison. This fails for non-string field values: comparing123 == "123"returns False. If filter fields contain integers, booleans, or other types, filters will silently fail to match.The other two edge cases are handled correctly:
- Missing filter fields return
Nonevia.get(), which doesn't match stringfilter_value, so no exception occurs.- Nested arrays are traversed via the loop at lines 307–311, supporting arbitrary depth.
Consider converting
filter_valueto match the actual field type before comparison, or document that filter values must be strings matching the string representation of the field.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/robusta/core/playbooks/internal/crds.py (1)
258-322: Consider using a JSONPath library for more robust field extraction.The custom regex-based JSONPath parsing (lines 266-322) implements a subset of JSONPath functionality. While it works for the current use cases, it may be fragile for edge cases and harder to maintain than using an established library like
jsonpath-ng.If you'd like, a JSONPath library would provide:
- More comprehensive JSONPath syntax support
- Better error handling for malformed expressions
- Reduced maintenance burden
- Better test coverage
Example using
jsonpath-ng:from jsonpath_ng import parse # For simple paths jsonpath_expr = parse(clean_field) matches = jsonpath_expr.find(item) value = matches[0].value if matches else None # For filter expressions jsonpath_expr = parse(field) # e.g., "status.conditions[?type='Reconciled'].status" matches = jsonpath_expr.find(item) value = matches[0].value if matches else NoneHowever, the current implementation is functional and may be adequate for your needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/setup-robusta/crds.rst(1 hunks)src/robusta/core/playbooks/internal/crds.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/robusta/core/playbooks/internal/crds.py (8)
src/robusta/core/reporting/consts.py (1)
SlackAnnotations(80-82)src/robusta/core/playbooks/common.py (1)
get_resource_events_table(18-79)src/robusta/core/model/base_params.py (1)
ActionParams(60-71)src/robusta/core/playbooks/actions_registry.py (1)
action(16-21)src/robusta/core/model/events.py (2)
ExecutionBaseEvent(49-175)add_finding(129-138)src/robusta/core/reporting/blocks.py (4)
FileBlock(72-133)JsonBlock(303-314)TableBlock(321-448)MarkdownBlock(42-61)src/robusta/core/reporting/base.py (2)
Finding(248-406)EnrichmentType(115-126)src/robusta/utils/error_codes.py (2)
ActionException(42-47)ErrorCodes(5-40)
🪛 Flake8 (7.3.0)
src/robusta/core/playbooks/internal/crds.py
[error] 13-13: 'robusta.core.reporting.blocks.TableBlock' imported but unused
(F401)
[error] 13-13: 'robusta.core.reporting.blocks.MarkdownBlock' imported but unused
(F401)
[error] 216-216: local variable 'e' is assigned to but never used
(F841)
🪛 Ruff (0.14.2)
src/robusta/core/playbooks/internal/crds.py
20-20: subprocess call: check for execution of untrusted input
(S603)
27-27: Consider moving this statement to an else block
(TRY300)
35-35: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
36-36: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
39-39: Use raise without specifying exception name
Remove exception name
(TRY201)
80-80: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
115-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
151-151: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
216-216: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
335-335: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
No description provided.