fix: server-side apply, idempotent Helm install, RBAC pre-flight (SKY-843) (#605)

Author: eliran-ops

deploy/helm/radar/README.md

@@ -99,6 +99,8 @@ Disabled by default for security:
 | Terminal | `rbac.podExec: true` | Shell access to pods |
 | Port Forward | `rbac.portForward: true` | Port forwarding to pods |
 | Logs | `rbac.podLogs: true` | View pod logs (**enabled by default**) |
+| Helm Write | `rbac.helm: true` | Install/upgrade/rollback/uninstall Helm releases. Under auth or cloud-mode, also emits a split helm add-on ClusterRole — `radar-helm` (member-safe: CRDs, storage, namespaces) and `radar-helm-admin` (owner-only: RBAC, webhooks, ApiServices) |
+| RBAC view | `rbac.viewRBAC: true` | Show ClusterRoles, ClusterRoleBindings, Roles, RoleBindings in the resource browser. Off by default — cache-served reads bypass per-user RBAC, so this exposes the cluster's authorization graph to every authenticated Radar user |
 
 ### In-app Agent Upgrades (opt-in, for Radar Cloud users)
 

deploy/helm/radar/templates/cloud-rbac-helm.yaml

@@ -0,0 +1,55 @@
+{{- /*
+Bind Helm add-on ClusterRoles to cloud tiers. radar-helm goes to owner +
+member; radar-helm-admin goes to owner only — see clusterrole-helm-admin.yaml
+for the rationale. Viewer is intentionally not bound; viewers never install.
+*/}}
+{{- if and .Values.cloud.enabled .Values.cloud.defaultRbac.create .Values.rbac.create .Values.rbac.helm (ne .Values.auth.mode "none") -}}
+{{- if .Values.cloud.defaultRbac.owner }}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: {{ include "radar.fullname" . }}-cloud-owner-helm
+  labels:
+    {{- include "radar.labels" . | nindent 4 }}
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: {{ include "radar.fullname" . }}-helm
+subjects:
+  - kind: Group
+    name: cloud:owner
+    apiGroup: rbac.authorization.k8s.io
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: {{ include "radar.fullname" . }}-cloud-owner-helm-admin
+  labels:
+    {{- include "radar.labels" . | nindent 4 }}
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: {{ include "radar.fullname" . }}-helm-admin
+subjects:
+  - kind: Group
+    name: cloud:owner
+    apiGroup: rbac.authorization.k8s.io
+{{- end }}
+{{- if .Values.cloud.defaultRbac.member }}
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: {{ include "radar.fullname" . }}-cloud-member-helm
+  labels:
+    {{- include "radar.labels" . | nindent 4 }}
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: {{ include "radar.fullname" . }}-helm
+subjects:
+  - kind: Group
+    name: cloud:member
+    apiGroup: rbac.authorization.k8s.io
+{{- end }}
+{{- end -}}

deploy/helm/radar/templates/clusterrole-helm-admin.yaml

@@ -0,0 +1,35 @@
+{{- /*
+Cluster-admin-equivalent add-on ClusterRole. Emitted under the same gate as
+radar-helm but bound only to cloud:owner — never cloud:member.
+
+The verbs in this role are individually sufficient for self-promotion to
+cluster-admin or for bypassing admission control:
+  - rbac writes: bind your own user to cluster-admin in one apply
+  - webhook writes: delete the policy that blocks privileged pods
+  - apiservices: register a rogue aggregated API
+Granting these to a tier weaker than owner collapses the owner/member
+distinction. Charts that bundle their own RBAC, webhooks, or APIServices
+require an owner identity to install.
+*/}}
+{{- if and .Values.rbac.create .Values.rbac.helm (or (ne .Values.auth.mode "none") .Values.cloud.enabled) -}}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: {{ include "radar.fullname" . }}-helm-admin
+  labels:
+    {{- include "radar.labels" . | nindent 4 }}
+rules:
+  - apiGroups: ["rbac.authorization.k8s.io"]
+    resources: ["clusterroles", "clusterrolebindings", "roles", "rolebindings"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["admissionregistration.k8s.io"]
+    resources:
+      - validatingwebhookconfigurations
+      - mutatingwebhookconfigurations
+      - validatingadmissionpolicies
+      - validatingadmissionpolicybindings
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["apiregistration.k8s.io"]
+    resources: ["apiservices"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+{{- end }}

deploy/helm/radar/templates/clusterrole-helm.yaml

@@ -0,0 +1,33 @@
+{{- /*
+Helm add-on ClusterRole — member-safe tier. Covers CRDs, storage classes,
+runtime/priority classes, PDBs, namespaces. Cluster-admin-equivalent verbs
+(RBAC writes, webhooks, APIServices) live in radar-helm-admin; see that
+template for the owner-only rationale.
+*/}}
+{{- if and .Values.rbac.create .Values.rbac.helm (or (ne .Values.auth.mode "none") .Values.cloud.enabled) -}}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: {{ include "radar.fullname" . }}-helm
+  labels:
+    {{- include "radar.labels" . | nindent 4 }}
+rules:
+  - apiGroups: ["apiextensions.k8s.io"]
+    resources: ["customresourcedefinitions"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["storage.k8s.io"]
+    resources: ["storageclasses", "csidrivers", "volumeattachments"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["scheduling.k8s.io"]
+    resources: ["priorityclasses"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["node.k8s.io"]
+    resources: ["runtimeclasses"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["policy"]
+    resources: ["poddisruptionbudgets"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: [""]
+    resources: ["namespaces"]
+    verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+{{- end }}

deploy/helm/radar/templates/clusterrole.yaml

@@ -76,6 +76,20 @@ rules:
       - selfsubjectaccessreviews
     verbs: ["create"]
 
+  {{- if .Values.rbac.viewRBAC }}
+  # RBAC objects: opt-in read for the resource browser. Off by default —
+  # cache-served reads are not gated by per-user RBAC, so granting this
+  # exposes the cluster's authorization graph to anyone who can reach
+  # Radar's API regardless of their own permissions.
+  - apiGroups: ["rbac.authorization.k8s.io"]
+    resources:
+      - roles
+      - rolebindings
+      - clusterroles
+      - clusterrolebindings
+    verbs: ["get", "list", "watch"]
+  {{- end }}
+
   {{- if .Values.rbac.podLogs }}
   # Pod logs (opt-in - enables log viewer)
   - apiGroups: [""]

deploy/helm/radar/values.yaml

@@ -40,6 +40,13 @@ rbac:
   # Allow reading secrets (shows secrets in resource list)
   secrets: false
 
+  # Allow reading RBAC objects (Roles, RoleBindings, ClusterRoles, ClusterRoleBindings)
+  # so the resource browser can list them. Off by default: anyone reaching Radar's
+  # API sees the full RBAC graph through the shared informer cache regardless of
+  # their own K8s permissions, which is reconnaissance value for a low-trust user.
+  # Enable for clusters where Radar's audience is trusted with cluster RBAC visibility.
+  viewRBAC: false
+
   # Allow pod exec (enables terminal feature)
   podExec: false
 

docs/authentication.md

@@ -11,7 +11,7 @@ User → [Auth Layer] → Radar Backend → K8s API (as user, via impersonation)
 ```
 
 1. **Authentication** identifies the user (proxy headers or OIDC login)
-2. **Reads** are filtered — Radar discovers which namespaces the user can access via `SubjectAccessReview` and only returns resources from those namespaces
+2. **Reads** are filtered by namespace — Radar discovers which namespaces the user can access via `SubjectAccessReview` and only returns resources from those namespaces. Cluster-scoped resources (Nodes, ClusterRoles when `rbac.viewRBAC` is on, StorageClasses, etc.) are served from the ServiceAccount-populated informer cache without per-user RBAC re-checks, so anyone reaching Radar's API sees them regardless of their own K8s permissions on those kinds.
 3. **Writes** use K8s impersonation — Radar makes the K8s API call as the authenticated user, so K8s RBAC decides whether it's allowed
 4. **UI adapts** — capability checks run per-user, so buttons (exec, restart, scale, Helm) only appear if the user has permission
 
@@ -165,6 +165,14 @@ Under cloud-mode (`RADAR_CLOUD_MODE=true`, set automatically by the chart when `
 - Ships three default ClusterRoleBindings mapping Cloud's `cloud:owner` / `cloud:member` / `cloud:viewer` groups to the standard K8s `admin` / `edit` / `view` ClusterRoles. Configurable via `cloud.defaultRbac.*` in `values.yaml`.
 - Hardens the listener (no `/debug/pprof/*`, narrower exempt paths).
 
+<a id="cloud-mode-helm-bindings"></a>
+**Helm-specific bindings (when `rbac.helm=true`).** Helm's pre-flight existence check needs cluster-scoped reads/writes that the K8s built-in `admin`/`edit`/`view` ClusterRoles don't grant. The chart emits two add-on ClusterRoles, split by trust tier:
+
+- `radar-helm` — CRDs, StorageClasses, RuntimeClasses, PriorityClasses, PodDisruptionBudgets, Namespaces. Bound to `cloud:owner` AND `cloud:member`.
+- `radar-helm-admin` — RBAC objects (Roles/Bindings, Cluster variants), validating/mutating webhooks, ApiServices. Bound to `cloud:owner` ONLY. Granting these to a tier weaker than owner would let a member self-promote to cluster-admin in one `ClusterRoleBinding` write, collapsing the owner/member distinction.
+
+A `cloud:member` attempting to install a chart that bundles its own RBAC will get a typed `rbac_preflight` 403 with an actionable "ask an owner" message. Day-to-day app charts and operator-CRD installs still work for members.
+
 Customer-facing documentation for Radar Cloud lives on [radarhq.io](https://radarhq.io). The authoritative reference for the Cloud-mode chart values is the comment block in [`deploy/helm/radar/values.yaml`](../deploy/helm/radar/values.yaml) under `cloud:`.
 
 ## Setting Up User Permissions

docs/in-cluster.md

@@ -135,7 +135,8 @@ Some features require additional permissions. Most are disabled by default for s
 | Terminal | `rbac.podExec: true` | `false` | Shell access to pods |
 | Port Forward | `rbac.portForward: true` | `false` | Port forwarding to pods/services |
 | Logs | `rbac.podLogs: true` | `true` | View pod logs |
-| Helm Write | `rbac.helm: true` | `false` | Install/upgrade/rollback/uninstall Helm releases (grants broad write access; auto-enables secrets) |
+| Helm Write | `rbac.helm: true` | `false` | Install/upgrade/rollback/uninstall Helm releases (grants broad write access; auto-enables secrets). When auth or cloud is on, also emits a split helm add-on: `radar-helm` (CRDs/storage/PDBs/namespaces, bound to owner+member) and `radar-helm-admin` (RBAC/webhooks/APIServices, owner-only) — see [authentication.md](authentication.md#cloud-mode-helm-bindings) |
+| RBAC view | `rbac.viewRBAC: true` | `false` | Show ClusterRoles, ClusterRoleBindings, Roles, RoleBindings in the resource browser. Off by default: cache-served reads bypass per-user RBAC, so granting this exposes the cluster's authorization graph to every authenticated Radar user |
 | Traffic TLS | `rbac.traffic: true` | `true` | Read Hubble relay TLS certs for Cilium traffic observation |
 
 > **Node management** (cordon, uncordon, drain) is available via the MCP server and API. These operations require `patch` on nodes, `list` on pods, and `create` on `pods/eviction`, which are not included in the default ClusterRole. Add them via `rbac.additionalRules` or use [per-user authentication](authentication.md) so each user's own RBAC governs node operations.
@@ -283,6 +284,7 @@ See [Helm Chart README](../deploy/helm/radar/README.md) for all available values
 | `rbac.portForward` | Enable port forwarding | `false` |
 | `rbac.secrets` | Show secrets in resource list | `false` |
 | `rbac.helm` | Enable Helm write operations | `false` |
+| `rbac.viewRBAC` | Show RBAC objects in resource browser | `false` |
 | `rbac.traffic` | Read Hubble TLS certs | `true` |
 | `rbac.crdGroups.all` | Wildcard CRD read access | `false` |
 

internal/helm/client.go

@@ -1885,28 +1885,27 @@ func (c *Client) installWith(actionConfig *action.Configuration, req *InstallReq
 		}
 	}
 
-	// Create install action
-	installAction := action.NewInstall(actionConfig)
-	installAction.ReleaseName = req.ReleaseName
-	installAction.Namespace = req.Namespace
-	installAction.CreateNamespace = req.CreateNamespace
-	installAction.Timeout = 120 * time.Second
-	installAction.Version = req.Version
+	mode, err := preInstallCheck(actionConfig, req.ReleaseName, req.Namespace)
+	if err != nil {
+		return nil, err
+	}
 
-	// Locate/download chart
-	cp, err := installAction.ChartPathOptions.LocateChart(chartURL, c.settings)
+	// action.Install carries ChartPathOptions; instantiated here as a locator only.
+	locator := action.NewInstall(actionConfig)
+	locator.Version = req.Version
+	cp, err := locator.ChartPathOptions.LocateChart(chartURL, c.settings)
 	if err != nil {
 		return nil, fmt.Errorf("failed to locate chart: %w", err)
 	}
-
-	// Load chart
 	chart, err := loader.Load(cp)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load chart: %w", err)
 	}
 
-	// Run install
-	rel, err := installAction.Run(chart, req.Values)
+	if mode != installFresh {
+		log.Printf("[helm] install %q/%q: prior release record exists, recovering via %s", req.Namespace, req.ReleaseName, recoveryMode(mode))
+	}
+	rel, err := runInstallOrUpgrade(actionConfig, req, chart, mode)
 	if err != nil {
 		return nil, fmt.Errorf("install failed: %w", err)
 	}
@@ -2071,6 +2070,14 @@ func (c *Client) installWithProgressUsing(actionConfig *action.Configuration, re
 		}
 	}
 
+	// Pre-flight before downloading: a deployed/pending release is knowable
+	// from local Helm storage and we shouldn't waste bandwidth + show
+	// "Downloading..." progress to a user who'll get a 409 anyway.
+	mode, err := preInstallCheck(actionConfig, req.ReleaseName, req.Namespace)
+	if err != nil {
+		return nil, err
+	}
+
 	sendProgress("downloading", fmt.Sprintf("Downloading chart %s-%s...", req.ChartName, req.Version), chartURL)
 
 	// Download the chart archive directly via HTTP, bypassing the Helm SDK's
@@ -2106,19 +2113,19 @@ func (c *Client) installWithProgressUsing(actionConfig *action.Configuration, re
 		return nil, fmt.Errorf("failed to load chart: %w", err)
 	}
 
-	installAction := action.NewInstall(actionConfig)
-	installAction.ReleaseName = req.ReleaseName
-	installAction.Namespace = req.Namespace
-	installAction.CreateNamespace = req.CreateNamespace
-	installAction.Timeout = 120 * time.Second
-
-	sendProgress("installing", fmt.Sprintf("Installing %s to namespace %s...", req.ReleaseName, req.Namespace), "")
-
-	if req.CreateNamespace {
-		sendProgress("installing", fmt.Sprintf("Creating namespace %s if needed...", req.Namespace), "")
+	switch mode {
+	case installFresh:
+		sendProgress("installing", fmt.Sprintf("Installing %s to namespace %s...", req.ReleaseName, req.Namespace), "")
+		if req.CreateNamespace {
+			sendProgress("installing", fmt.Sprintf("Creating namespace %s if needed...", req.Namespace), "")
+		}
+	case installReplace:
+		sendProgress("installing", fmt.Sprintf("Replacing prior uninstalled release %s in %s...", req.ReleaseName, req.Namespace), "")
+	case installUpgrade:
+		sendProgress("installing", fmt.Sprintf("Recovering prior failed release %s in %s...", req.ReleaseName, req.Namespace), "")
 	}
 
-	rel, err := installAction.Run(chart, req.Values)
+	rel, err := runInstallOrUpgrade(actionConfig, req, chart, mode)
 	if err != nil {
 		return nil, fmt.Errorf("install failed: %w", err)
 	}

internal/helm/handlers.go

@@ -822,11 +822,8 @@ func (h *Handlers) handleInstall(w http.ResponseWriter, r *http.Request) {
 		release, installErr = client.Install(&req)
 	}
 	if err := installErr; err != nil {
-		if IsForbiddenError(err) {
-			writeError(w, http.StatusForbidden, "insufficient permissions to install Helm release")
-			return
-		}
-		writeError(w, http.StatusInternalServerError, err.Error())
+		log.Printf("[helm] install %q/%q (chart=%q repo=%q) failed: %v", req.Namespace, req.ReleaseName, req.ChartName, req.Repository, err)
+		writeInstallError(w, err)
 		return
 	}
 
@@ -924,11 +921,8 @@ func (h *Handlers) handleInstallStream(w http.ResponseWriter, r *http.Request) {
 
 		case result := <-resultCh:
 			if result.err != nil {
-				event := map[string]any{
-					"type":    "error",
-					"message": result.err.Error(),
-				}
-				data, _ := json.Marshal(event)
+				log.Printf("[helm] install %q/%q (chart=%q repo=%q) failed: %v", req.Namespace, req.ReleaseName, req.ChartName, req.Repository, result.err)
+				data, _ := json.Marshal(installStreamErrorEvent(result.err))
 				w.Write([]byte("data: " + string(data) + "\n\n"))
 			} else {
 				event := map[string]any{