Permission guards #9
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive "Permission Guards" system that provides enhanced safety and security for bulk actions in the Kaptn Kubernetes dashboard. The implementation adds multiple layers of protection including safety validation, audit logging, action coordination, and frontend confirmation dialogs to prevent accidental destructive operations.
- Adds a new enhanced actions system with safety guards, audit logging, and action coordination
- Implements frontend bulk action API integration with confirmation dialogs and safety validation
- Introduces comprehensive safety rules for protecting critical namespaces and resources
- Creates a documentation proposal for future API package reorganization
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| proposed_reorganization.md | Comprehensive analysis and proposal for reorganizing the API package from current monolithic structure to domain-driven architecture |
| internal/k8s/actions/safety.go | Safety guard implementation with namespace protection, label-based security, and destructive action validation |
| internal/k8s/actions/manager.go | Enhanced actions manager that orchestrates safety guards, audit logging, and action coordination |
| internal/k8s/actions/integration_example.go | Example code showing how to integrate the enhanced actions system into existing server structure |
| internal/k8s/actions/handlers.go | HTTP handlers for enhanced bulk actions with safety validation and user confirmation workflows |
| internal/k8s/actions/coordinator.go | Action coordinator that manages RBAC checks, safety validation, and action execution with comprehensive audit logging |
| internal/k8s/actions/audit.go | Structured audit logging system for tracking all action requests, safety violations, and security events |
| internal/api/server.go | Integration of enhanced actions system into main server with safety guards and audit logging |
| internal/api/handlers_actions_stubs.go | Stub implementations for deployment, service, configmap, and secret bulk actions |
| internal/api/handlers_actions_pods.go | Enhanced pod bulk actions handler with safety validation and confirmation workflows |
| internal/api/handlers_actions_common.go | Common action validation and routing logic with safety checks |
| go.mod | Added dependencies for JWT handling and YAML processing |
| frontend/src/lib/api/bulk-actions.ts | TypeScript API client for bulk actions with safety violation handling |
| frontend/src/hooks/useActionConfirmation.ts | React hook for managing action confirmation dialogs |
| frontend/src/hooks/use-bulk-actions.ts | React hook for executing bulk actions with error handling and confirmation |
| frontend/src/components/ui/action-confirmation-dialog.tsx | Reusable confirmation dialog component with safety violation display |
| frontend/src/components/data_tables/PodsDataTable.tsx | Integration of bulk actions with confirmation dialogs in pods data table |
| frontend/src/components/ActionConfirmationDialog.tsx | Example confirmation dialog implementation with destructive action protection |
| PROJECT_FILES.md | Updated project structure documentation |
| Backend_Refactor.md | Detailed step-by-step refactoring plan for transforming monolithic API package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| deniedNamespaces := map[string]bool{ | ||
| "kube-system": true, | ||
| "kube-public": true, | ||
| "kube-node-lease": true, | ||
| "monitoring": true, | ||
| "prometheus": true, | ||
| "grafana": true, | ||
| "istio-system": true, | ||
| "cert-manager": true, | ||
| "ingress-nginx": true, | ||
| "metallb-system": true, | ||
| "calico-system": true, | ||
| "tigera-operator": true, | ||
| "rook-ceph": true, | ||
| "longhorn-system": true, | ||
| "velero": true, | ||
| "argocd": true, | ||
| "flux-system": true, | ||
| "kaptn": true, // Protect Kaptn's own namespace | ||
| } |
There was a problem hiding this comment.
The hardcoded list of denied namespaces should be configurable rather than hardcoded. Consider making this configurable through environment variables or configuration files to allow different deployment scenarios.
Copilot uses AI. Check for mistakes.
| DryRun bool `json:"dry_run"` | ||
| Duration string `json:"duration,omitempty"` |
There was a problem hiding this comment.
The Duration field is stored as a string, which makes it difficult to query or analyze. Consider using a numeric field (e.g., duration in milliseconds) alongside the human-readable string, or use a proper duration type that can be unmarshaled correctly.
| DryRun bool `json:"dry_run"` | |
| Duration string `json:"duration,omitempty"` | |
| DryRun bool `json:"dry_run"` | |
| // Duration in human-readable format (e.g., "1m23s") | |
| Duration string `json:"duration,omitempty"` | |
| // DurationMs stores the duration in milliseconds for easier querying/analysis | |
| DurationMs int64 `json:"duration_ms,omitempty"` |
Copilot uses AI. Check for mistakes.
| func parseAction(action string) (resource, verb string) { | ||
| // Map action strings to resource types and verbs | ||
| switch action { | ||
| case "restart-pods": | ||
| return "pods", "update" | ||
| case "delete-pods": | ||
| return "pods", "delete" | ||
| case "get-logs": | ||
| return "pods", "get" | ||
| case "describe-pods": | ||
| return "pods", "get" | ||
| case "export-yaml": | ||
| return "pods", "get" | ||
| case "restart-deployments": | ||
| return "deployments", "update" | ||
| case "scale-deployments": | ||
| return "deployments", "update" | ||
| case "delete-deployments": | ||
| return "deployments", "delete" | ||
| case "describe-deployments": | ||
| return "deployments", "get" | ||
| case "delete-services": | ||
| return "services", "delete" | ||
| case "describe-services": | ||
| return "services", "get" | ||
| case "delete-configmaps": | ||
| return "configmaps", "delete" | ||
| case "edit-configmaps": | ||
| return "configmaps", "update" | ||
| case "describe-configmaps": | ||
| return "configmaps", "get" | ||
| case "delete-secrets": | ||
| return "secrets", "delete" | ||
| case "edit-secrets": | ||
| return "secrets", "update" | ||
| case "view-secrets": | ||
| return "secrets", "get" | ||
| case "describe-secrets": | ||
| return "secrets", "get" | ||
| default: | ||
| return "unknown", "unknown" | ||
| } | ||
| } |
There was a problem hiding this comment.
This parseAction function in the pods file contains logic for multiple resource types (deployments, services, configmaps, secrets), violating the single responsibility principle. As noted in the reorganization proposal, this should be moved to a centralized action parser to eliminate cross-domain logic.
Copilot uses AI. Check for mistakes.
| if (response.success) { | ||
| options.onSuccess?.(response) | ||
| } else { | ||
| const error = new Error(response.message || 'Action failed') | ||
| setError(error) | ||
| options.onError?.(error) | ||
| } |
There was a problem hiding this comment.
The hook treats a response with success: false as an error case, but this should still be considered a successful HTTP response. The error handling should distinguish between HTTP errors (network issues, 4xx/5xx status codes) and business logic failures (success: false).
| if (response.success) { | |
| options.onSuccess?.(response) | |
| } else { | |
| const error = new Error(response.message || 'Action failed') | |
| setError(error) | |
| options.onError?.(error) | |
| } | |
| options.onSuccess?.(response) |
Copilot uses AI. Check for mistakes.
| type requestBody struct { | ||
| data interface{} | ||
| } | ||
|
|
||
| func (rb *requestBody) Read(p []byte) (n int, err error) { | ||
| // This is a simplified implementation | ||
| // In practice, you'd marshal the data and provide a proper reader | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
The requestBody.Read method always returns 0 bytes and nil error, which means no data is actually read. This will cause request body parsing to fail. The implementation needs to properly marshal the data and provide it through the Read interface.
Copilot uses AI. Check for mistakes.
1 participant
No description provided.