Repository navigation
Redact Secret values from Helm diffs for AI callers and bind radar:ai for background Diagnose - #1949
Conversation
The manifest diff and the notes diff reached MCP callers unredacted; only values_diff went through RedactSecrets. Secret documents now have their data and stringData values replaced by [REDACTED] before diffing, keys kept so additions and removals still show, and RedactSecrets runs over both diffs as a second layer. The REST/UI diff is unchanged: it is the user's own view.
Every AI Diagnose run, manual or background, will read the cluster as the radar:ai group. cloud.aiRbac (default true) binds it to view plus the cluster-read and integration-read add-ons, with no Secret role, and stays on when cloud.defaultRbac.create=false so Diagnose works when IdP groups decide cluster access. An absent value means off, as with cloud.systemRbac, so a --reuse-values upgrade never gains it silently. The binding does nothing until the hub sends the group.
…ts, and expect radar:ai bindings in the chart test
22fd1f5 to
03a4e19
Compare
PR Summary by QodoRedact Helm secrets for MCP and bind background Diagnose to radar:ai
AI Description
Diagram
High-Level Assessment
Files changed (22)
|
Code Review by Qodo
1.
|
Linear: RAD-657
Why
Radar Cloud's AI Diagnose is blind on clusters whose role bindings are off (
cloud.defaultRbac.create=false, IdP groups decide access), because it reads asradar:viewer, which is unbound there. After this release:radar:aigroup (skyhook-dev/radar-hub#288), which this chart binds.MCP clients are not affected: they keep reading with the user's own identity.
What changes
1. Secret redaction in
get_helm_release(every MCP caller)include=diff(the manifest diff) andinclude=notes_diffwere returned unredacted; onlyvalues_diffwent throughRedactSecrets.kind: Secretdocument now has itsdata/stringDatavalues replaced by[REDACTED]before diffing. The keys stay, so added and removed keys still show. Secrets insidekind: Listitems are covered, thelast-applied-configurationannotation (a full copy of the object) is dropped, and a document that doesn't parse is withheld whole. Other documents are left byte-for-byte untouched.RedactSecretsthen runs over both diffs as a second layer.include=valuesandinclude=values_diffnow use key-aware redaction built for Helm values (aicontext.RedactHelmValues). Chart authors name credentials freely (dbPassword,auth.postgresPassword), so any key ending inpassword,token,apiKey,secretKey,clientSecret,credentialsand similar is masked, numbers included. Secret references (existingSecret,*SecretName,*Ref) keep their values. The values diff masks both revisions before diffing. CRD specs keep the existing exact-key matching.2.
radar:aichart bindingtemplates/cloud-rbac-ai.yaml: bindsradar:aitoview. No Secret role, no writes.radar:aiis added to the cluster-read and integration-read add-ons.cloud.aiRbac, defaulttrue, independent ofcloud.defaultRbac. As withcloud.systemRbac, an absent value means off, so a--reuse-valuesupgrade never gains the binding silently.docs/authentication.md.The binding does nothing until Radar Cloud sends the group. Background runs send
radar:aiwith no tier group, so the AI's access is exactly this binding:cloud.aiRbac=falsereally turns it off, and a widened viewer role doesn't widen the AI. Clusters on older charts get no background Diagnose until they upgrade. Radar Cloud refuses manual runs on a Radar older than this release, rather than reading with less protection.Tests
credentials, both revisions of a values diff.resource_diffmasks Secret values.TestIntegrationReadBindingsexpects theradar:aibindings alongsideradar:system, with cases foraiRbac=false.tests/cloud_rbac_ai_test.yaml: renders withdefaultRbac.create=false, absent orfalsemeans off, bound toviewonly, add-ons present whatever the tier settings.scripts/test-chart.sh: new render cases; the "no orphan role" cases now also setcloud.aiRbac=false.helm unittest: theservice_test.yamlfailures also fail onmainlocally; CI's Helm chart job passes.Live test on EKS (proxy auth, simulated Diagnose identities)
radar:ai,radar:org:<org>) with the chart'sradar:aibinding: pods in every namespace, no Secrets.viewin one namespace: pods in that namespace only; listing all namespaces returns only that one.dbPassword/apiKeyvalues, read throughget_helm_releaseby a user who can read Secrets: no sentinel value invalues,diff,notes_difforvalues_diff. The REST diff (the user's own view in the UI) still shows them, as intended.End to end on kind
aiRbac=true: the AI identity reads pods, events and nodes; Secrets are forbidden.aiRbac=false: it reads nothing.get_helm_release include=diff,notes_diff, called by an owner who can read Secrets: no Secret values in the output. The Let Kubernetes RBAC, not the Cloud role, decide user-run operations #1899 binary returns the plaintext values and aghp_token from the notes.Note
Medium Risk
Changes cluster RBAC defaults and how credentials appear in MCP Helm diffs; misconfiguration could block background Diagnose or leak secrets if redaction regresses, but grants stay read-only and UI paths are unchanged.
Overview
Adds default Kubernetes RBAC for automatic Radar Cloud Diagnose via new
cloud.aiRbac(default on): chart-owned aggregated view role forradar:ai, plus cluster-read and integration-read add-ons—no Secret access. Refactorsradar:systemthe same way (owned aggregated view role instead of binding directly toview), with Secret read unchanged for alerts/timeline. Docs and Helm tests cover independence fromcloud.defaultRbac,--reuse-valuesabsent-key behavior, and installerescalaterequirements.Hardens Helm MCP output for AI callers while leaving REST/UI diffs as the user’s own view: manifest diffs strip Secret
data/stringData(and unparseable Secret docs) before diffing; values diffs use newRedactHelmValues(suffix/key-aware credentials, preserved secret references); notes diffs get pattern redaction.get_helm_releaseinclude=diff,values_diff, andnotes_diffroute through these paths.Reviewed by Cursor Bugbot for commit 16be4f7. Bugbot is set up for automated code reviews on this repo. Configure here.