allow Holmes to read CRDs - #1088
Conversation
Will help to generate the CRDs feature required cluster roles fix aws mcp instructions
WalkthroughThe PR modifies Kubernetes RBAC configuration to grant CRD list/get permissions and significantly enhances AWS MCP server instructions by increasing CloudTrail result limits and adding comprehensive memory optimization guidelines, pagination best practices, and investigation strategies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/holmes/templates/holmesgpt-service-account.yaml (1)
6-6: ClusterRole is cluster-scoped; drop metadata.namespace.ClusterRoles must not be namespaced. Keeping namespace can confuse linters and operators.
Apply:
metadata: name: {{ .Release.Name }}-holmes-cluster-role - namespace : {{ .Release.Namespace }}
🧹 Nitpick comments (8)
helm/holmes/templates/holmesgpt-service-account.yaml (2)
77-84: Duplicate HPA permissions; keep one.HorizontalPodAutoscaler rules under apiGroup autoscaling appear twice. Remove the second to reduce noise and drift.
- - apiGroups: - - autoscaling - resources: - - horizontalpodautoscalers - verbs: - - get - - listAlso applies to: 150-156
99-112: extensions API group is deprecated; consider removal or gating per cluster version.You already cover apps/* and networking.k8s.io/ingresses. Keeping extensions/* is legacy. Gate behind a values flag or remove if you don’t support very old clusters.
helm/holmes/templates/mcp-servers/aws/_helpers.tpl (6)
36-39: Be consistent with pagination flags across examples.You mix CLI paginator flags (--max-items/--starting-token) with service-level ones (--max-results/--next-token). Prefer one style per section and mention the alternative once to avoid confusion.
Also applies to: 178-184
46-50: Tighten EKS logs query to cut noise and payload.Add a basic filter-pattern and show region placeholder for clarity.
aws logs filter-log-events \ --log-group-name /aws/containerinsights/CLUSTER_NAME/application \ --start-time $(date -d '1 hour ago' +%s)000 \ - --max-items 500 + --filter-pattern "ERROR || Exception" \ + --max-items 500 \ + --region REGION
69-70: Use explicit UTC designator in ISO time.Append Z to avoid locale ambiguity in some shells/environments.
-aws cloudtrail lookup-events --start-time $(date -u -d '1 hour ago' +%Y-%m-%dT%H:%M:%S) --max-items 100 +aws cloudtrail lookup-events --start-time $(date -u -d '1 hour ago' +%Y-%m-%dT%H:%M:%SZ) --max-items 100
210-218: Clarify paginator tokens per service.For CloudWatch Logs, examples often use --next-token (service) while CloudTrail uses --starting-token (CLI paginator). Document both to match the flags used in preceding examples.
Example addition:
# CloudWatch Logs: either aws logs filter-log-events ... --max-items 200 --starting-token <Token> # or service-level aws logs filter-log-events ... --limit 100 --next-token <Token>Also applies to: 223-229
162-173: Optional: show JMESPath --query to shrink payloads.Helps memory further when scanning logs.
aws logs filter-log-events ... --max-items 300 --query 'events[].{ts:timestamp,msg:message,stream:logStreamName}'
145-156: Optional: demonstrate --page-size with --max-items.Smaller pages reduce peak memory while preserving total cap.
aws cloudtrail lookup-events --start-time ... --max-items 200 --page-size 50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helm/holmes/templates/holmesgpt-service-account.yaml(1 hunks)helm/holmes/templates/mcp-servers/aws/_helpers.tpl(5 hunks)
⏰ 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: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (2)
helm/holmes/templates/holmesgpt-service-account.yaml (1)
132-139: CRD read RBAC: looks correct; confirm if watch is needed.Granting list/get on apiextensions.k8s.io/customresourcedefinitions aligns with “read CRDs.” If Holmes needs to react to CRD changes, add watch; otherwise current scope is least-privilege.
If watch is required, apply:
verbs: - - "list" - - "get" + - "list" + - "get" + - "watch"helm/holmes/templates/mcp-servers/aws/_helpers.tpl (1)
35-39: LGTM on raising CloudTrail cap to 100.Matches the new memory guidance while staying safe for typical investigations.
2 participants
Will help to generate the CRDs feature required cluster roles fix aws mcp instructions