Repository navigation
Conversation
6 tasks done
arlenskh
marked this pull request as ready for review
September 29, 2026 21:05
PR Summary by QodoConfigure cluster access for new Radar installs through hub OIDC settings
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. New installs keep hub-role access
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| # | ||
| # Requires Hub 1.6.0 or newer; an older Hub ignores it. | ||
| useRefreshToken: false | ||
| useRefreshToken: true |
There was a problem hiding this comment.
2. Refresh-token guidance gives wrong default 🔗 Cross-repo conflict ≡ Correctness
The chart changes auth.oidc.useRefreshToken to true, but radar-docs still lists its default as false and says the Hub does not refresh tokens by default. Operators following the self-hosted authentication guidance therefore get an incorrect account of when their sessions contact the IdP and update groups.
Agent Prompt
## Issue description
The chart enables refresh tokens by default, while radar-docs describes the previous default and its session behavior.
## Fix Focus Areas
- charts/radar-hub/values.yaml[457-473]
- cloud/self-hosted/configuration.mdx[101-104]
- cloud/self-hosted/auth.mdx[319-333]
## Recommended Fix
Update radar-docs’ configuration reference and authentication guidance to describe refresh as the chart default, including the documented behavior when the IdP supplies no refresh token.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
There was a problem hiding this comment.
Updated in radar-docs#126.
Comment on lines
+490
to
+492
| # Requires a Hub that reads RADAR_HUB_OIDC_CLUSTER_ACCESS; an older Hub | ||
| # ignores it and behaves as roles. | ||
| clusterAccess: "" |
There was a problem hiding this comment.
3. Google setup risks inaccessible clusters 🔗 Cross-repo conflict ≡ Correctness
The chart’s empty clusterAccess now selects IdP mode when the default groupsClaim is set, while radar-docs’ Google Workspace recipe leaves that claim enabled despite stating Google emits no groups. Once the dependent web wizard applies IdP mode to new installs, users following that recipe need clusterAccess: roles or another source of group subjects to avoid clusters with no usable access bindings.
Agent Prompt
## Issue description
The documented Google Workspace configuration leaves group passthrough enabled even though Google supplies no groups, while the new cluster-access default selects IdP mode.
## Fix Focus Areas
- charts/radar-hub/values.yaml[480-492]
- cloud/self-hosted/auth.mdx[136-148]
- cloud/self-hosted/auth.mdx[228-249]
## Recommended Fix
Update radar-docs’ Google Workspace recipe and group-passthrough guidance to require `auth.oidc.clusterAccess: roles` when usable IdP groups are unavailable. Coordinate the guidance with the web wizard’s IdP-mode release.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
There was a problem hiding this comment.
Fixed in radar-docs#126 (a6c5c5d): the Google recipe sets groupsClaim: "".
nadaverell
pushed a commit
to skyhook-io/radar
that referenced
this pull request
Oct 4, 2026
…1899) ## Why Part of RAD-578: in Radar Cloud, hub roles (`owner` / `member` / `viewer`) should cover Radar's own features, and for orgs with an IdP, IdP groups (`radar:idp:<id>`) should decide cluster access. Those orgs turn the chart's hub-role bindings off (`cloud.defaultRbac.create=false`). Two things broke in that setup: 1. **Hub background jobs lost cluster access.** The alerts worker and timeline puller only reached the cluster through `radar:owner` → `admin`, so alerts and the hub timeline went silent. The same thing happens today to anyone who sets `defaultRbac.owner=false`. 2. **The Cloud role overrode Kubernetes.** Helm reads and writes and in-cluster probe pods were refused on the Cloud role before Kubernetes was asked. They run as the impersonated user, so a `viewer` whose IdP group grants `edit` in a namespace couldn't roll back there. ## What **Chart** - New `cloud-rbac-system.yaml`: binds `radar:system` (the hub's alerts worker and timeline puller) to `view` plus Secret `get/list/watch`. The cluster-read and integration-read add-ons also bind it, and now still render when `defaultRbac.create=false`. AI diagnose does not use it; it stays on `radar:viewer`. - Secret read is there for Helm release alerts and Secret changes in the hub timeline: Radar lists releases as the caller, and Helm stores releases as Secrets. It's read-only, and it's far less than today, where these identities get `admin` through `radar:owner`. - New value `cloud.systemRbac` (default `true`), all or nothing. An absent value counts as **off**, like `defaultRbac.create`, so a `--reuse-values` upgrade from an older release never gains cluster-wide Secret read without someone choosing it. Schema, chart README and `docs/authentication.md` updated. **Backend** - Removed `requireCloudRole` from all 16 Helm handlers and the MCP `get_helm_release` gate. - Removed the role gate from the in-cluster reachability test (capability check, run endpoint, MCP `diagnose`). Probe pods are created with the user's impersonated client, and `reachability.Capability` already checks Kubernetes permissions. - The Cloud role still gates what runs as Radar's service account: settings, Argo CD and Kubecost integration, audit settings, self-upgrade. **UI** - `RoleGatedPanel` is removed from the Helm Manifest, Values and Compare tabs. `useCanHelmAct(namespace)` checks the user's own Helm write permission in the release's namespace (see below). - `RestrictedState` keeps the exact group, resource and verbs for the denied kind in its RBAC snippet, and leaves the subject as a placeholder the admin fills in. OSS without a Hub is unaffected: the cloud templates need `cloud.enabled`, and the role gates already did nothing without a Cloud role. ## Test plan - [x] `helm unittest` (new `cloud_rbac_system_test.yaml`, updated integration-read suite) and `scripts/test-chart.sh` - [x] `go test ./...` and `pkg` module tests. The chart-rendering `TestIntegrationReadBindings` now expects `radar:system`. - [x] New `TestHelmHandlers_NotGatedOnCloudRole` and `TestTraceInClusterNotGatedOnCloudRole`, both red before the change - [x] `make tsc`; web and k8s-ui vitest, plus a new `RestrictedState.test.tsx` - [x] Visual test on EKS, with proxy auth simulating Cloud users: - a Cloud `viewer` with admin-level RBAC sees Manifest, Values, Rollback and Compare; - a `viewer` with only `view` gets Kubernetes' "Access Restricted" instead of a role message; - the RBAC snippet binds the user's first IdP group; - OSS without auth is unchanged; - a user with no access sees the banner on Home, Resources and Topology; 25 groups stay collapsed; checked in dark and light themes. - [x] `TestHandleAuthMe_NoNamespaceAccess` (reported only when connected with zero namespaces) and `noClusterAccess.test.ts` **No-access banner** - A user who can read no namespace used to see an empty cluster ("No Node found", every count "–") with no explanation. That's the day-one view for an IdP org before any binding exists, and it also happens to OSS proxy/OIDC users with no RBAC. - `/api/auth/me` now reports `noNamespaceAccess: true` for such a user, and only once connected: before that, namespace discovery fails closed. - The shell shows a banner above every view, including when the Hub embeds Radar. It says why the views are empty and who is signed in, with the user's bindable groups in a collapsed list, so they can tell a cluster admin which group to bind. - It's based on the user's own access, not on how the org means to set up the cluster, so it never claims a cluster is IdP-only. - While shown, it re-checks every minute, so it clears once a binding lands. ## Release order Release this chart before the Hub web wizard can default new installs to IdP-only access. The Hub PRs keep sending `radar:owner` for one release so older charts keep working. Related: skyhook-dev/radar-hub#272, skyhook-dev/radar-hub-web#326, skyhook-io/helm-charts#51. ## Cross-review follow-up (2026-09-29) - **Helm writes check the signed-in user, not Radar's ServiceAccount** (`55fc3efa`). With a user attached, `requireHelmWrite` checks the caller's `secrets/create` in the release's namespace, before any chart is fetched (so a caller who could never install can't make Radar fetch an arbitrary repo URL). Chart-cache and OCI-source changes need no cluster permission. No-auth installs keep the ServiceAccount check. `/api/capabilities?namespace=` reports `helmWrite` per namespace; with auth on, a denial names the user's RBAC, not `rbac.helm`. Also: streaming rollback runs as the user, and a forbidden release read returns 403. - **No write-everything ServiceAccount under `rbac.helm` in Cloud/auth mode** (`acbdbad4`). The `*/*` create/update/patch/delete rule renders only for no-auth, non-Cloud installs; every Helm action there runs as the user, and no ServiceAccount write path depends on it (audited). - **`radar:system` is one all-or-nothing grant; absent means off** (`acbdbad4`, `c4018889`). - **`/auth/me` reads the permission cache only** (`e51efe09`), so app startup no longer runs a SubjectAccessReview per namespace. The banner title now reads "You can't read any namespace in this cluster". - Known limit: on a first visit to a large cluster, the banner's `/auth/me` check can arrive before namespace discovery fills the cache, so the banner may need one reload to appear. A retry loop for this (`6eb752a8`) was reverted (`c0f51b96`) as not worth the extra state. - **The banner's group list is `ph-no-capture`** (`af4afc7c`): group IDs name the customer's directory structure, and Radar Cloud records this view in session replay. - `go test` for `internal/helm`, `internal/k8s`, `internal/server` pass (the one `internal/server` failure, `TestNoEffortNotesReferencedFromSource`, comes from other local worktrees under `.claude/`). `helm unittest`: 70/72, the 2 failures (`service_test.yaml`) also fail on HEAD. Web vitest for contexts/helm/api pass. - Follow-ups: RAD-656 (reads through the cache aren't checked per kind), RAD-657 (diagnose permissions; redact Helm manifest/notes diffs before any diagnose identity gets Secret read). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **High Risk** > Changes cluster RBAC (including optional cluster-wide Secret read for `radar:system`) and broadens who can attempt Helm and probe operations based on Kubernetes permissions instead of Cloud role, which affects security boundaries for IdP-only orgs. > > **Overview** > Radar Cloud orgs that turn off default hub-role bindings (`cloud.defaultRbac.create=false`) can keep **alerts and the hub timeline** working via a new **`cloud.systemRbac`** path that binds **`radar:system`** to read-only access (`view`, cluster-read/integration-read add-ons, and cluster-wide **Secret** read). That grant is separate from tier bindings; **`--reuse-values` upgrades without the key stay off** until explicitly enabled. > > **Authorization shifts from Cloud tier to Kubernetes RBAC** for user-impersonated work: Helm reads/writes, previews, rollbacks (including streaming), in-cluster reachability probes, and MCP Helm/diagnose paths **no longer call `requireCloudRole`**. Writes are gated by the caller’s **`secrets/create` in the release namespace** (before chart fetch); release read failures surface as **403** when RBAC denies Secret access. The chart **stops granting cluster-wide `*/*` write on Radar’s ServiceAccount** when auth or cloud mode is on—only no-auth installs keep that for Helm-as-SA. > > The UI drops **member-only Helm panels** and uses **namespace-scoped `useCanHelmAct`**, aligned with per-namespace **`helmWrite`** capabilities. **`/api/auth/me`** adds **`noNamespaceAccess`** and a **warning banner** when a connected user can read no namespaces. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 812e48f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Arlen Vasconcelos <arlenvasconcelos@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
5 tasks done
nadaverell
added a commit
that referenced
this pull request
Oct 7, 2026
Hub 1.10.0 ends a self-hosted OIDC session that carries IdP groups and has no refresh token when its ID token expires, instead of keeping it and silently dropping the groups. With the default groupsClaim and refresh off, that signs users out every ID token lifetime: 5 minutes on a default Keycloak, an hour on Okta or Entra. useRefreshToken now defaults to true, so sessions last session.maxLifetime when the IdP issues a refresh token (Keycloak does by default; Okta and Entra need offline_access). An IdP that returns none behaves as before. NOTES says after every install or upgrade whether refresh tokens are on and what the IdP client needs. Split out of #51 so it can ship with Hub 1.10.0 ahead of clusterAccess. Staged as 1.10.0-rc.1 with appVersion unchanged; the Hub v1.10.0 tag promotes it to 1.10.0.
3 tasks done
Self-hosted hubs choose how new Radar installs grant cluster access: roles keeps the hub-role ClusterRoleBindings, idp turns them off so IdP groups decide. The value is passed to the hub as RADAR_HUB_OIDC_CLUSTER_ACCESS and only applies when OIDC is configured. It never changes clusters that are already installed.
…p that can't work Unset now lets the Hub pick idp while IdP group passthrough is on and roles otherwise; roles is the explicit opt-out. idp without auth.oidc.issuer or auth.oidc.groupsClaim fails the install instead of leaving new clusters with no access. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A groups claim the IdP doesn't emit was described as harmless. With clusterAccess left unset it now follows groupsClaim to idp, so clusters installed then get no hub-role bindings and, with no groups arriving, nobody sees them. The comment says so and says to check that groups arrive first. useRefreshToken now says what happens without it: a session carrying IdP groups ends when the first ID token expires and the user signs in again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
…ogether - NOTES.txt now says, after every install or upgrade, which cluster access mode new clusters get (and that an unset clusterAccess follows groupsClaim to idp), and whether refresh tokens are on and what they need from the IdP. Both defaults change behaviour on upgrade. - The clusterAccess=idp checks move from deployment-hub.yaml to secret.yaml, next to the chart's other value checks. - The unknown-value test matches the schema's enum error, so it fails if the schema check stops running. The values comment says idp also needs an issuer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017J25KHG5W4DzS32bU1J8ou
nadaverell
force-pushed
the
arlen/rad-578-radar-hub-cluster-access
branch
from
October 7, 2026 18:46
ae20b04 to
698ce9d
Compare
Contributor
|
I rebased this onto
Checks: |
The connect page applies the cluster access mode from Hub 1.11.0, so name that as the minimum.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
RAD-578. On a self-hosted hub, the org's cluster access mode (
roles|idp) is configured next to the IdP, in Helm values. New Radar installs generated by the hub wizard inherit it:idpwhile IdP group forwarding is on (groupsClaimset),rolesotherwise.roles: the Radar chart keeps the hub-role ClusterRoleBindings. The explicit opt-out.idp: new installs getcloud.defaultRbac.create=false, and IdP groups (radar:idp:<id>) decide cluster access. Refused at template time withoutauth.oidc.issuerorauth.oidc.groupsClaim: those installs would give nobody access.Existing clusters are never changed by this value.
What
values.yaml: documentedauth.oidc.clusterAccess: "".values.schema.json: enum"" | roles | idp.templates/deployment-hub.yaml: setsRADAR_HUB_OIDC_CLUSTER_ACCESSinside the existing OIDC block, only when the value is set;fails onidpwithout OIDC orgroupsClaim.tests/deployment_hub_oidc_cluster_access_test.yaml.No chart version bump in this draft. I'll bump when this is released together with the hub change.
Test plan
helm unittest charts/radar-hub: 23/23 (the new suite failed before the change)tests/render-matrix.shpasseshelm lintpassesDepends on skyhook-dev/radar-hub#272, which reads the env var.
Cross-review follow-up (2026-09-29)
values.yamlcomments (1a9cfc9):groupsClaimno longer calls a missing claim harmless (an unsetclusterAccessfollows it toidp, so check that groups arrive first), anduseRefreshTokensays that without it a session carrying IdP groups ends when the first ID token expires (skyhook-dev/radar-hub#272).6439f2f): after install or upgrade it prints which cluster access mode new clusters get (unset followsgroupsClaimtoidp) and whether refresh tokens are on and what the IdP client needs. Theidpchecks moved tosecret.yamlwith the other value checks; the unknown-value test matches the schema's enum error.main, bump to1.10.0(two defaults change behaviour on upgrade), setappVersionto the Hub release with RAD-578, and name that Hub version in theclusterAccesscomment. Merging publishes the chart, so this needs explicit approval of the version.1.8.0-rc.2; main is1.9.0-rc.3. Rebase after the hub and web images with RAD-578 are released; the version bump goes in the rc release PR.🤖 Generated with Claude Code
Note
Medium Risk
Changes default behavior for new cluster installs when OIDC group forwarding is enabled (unless operators set
clusterAccess=roles), which affects authorization for newly connected clusters.Overview
Adds
auth.oidc.clusterAccess("",roles, oridp) so self-hosted installs control how new Radar cluster installs grant Kubernetes access, wired to the hub asRADAR_HUB_OIDC_CLUSTER_ACCESSwhen explicitly set. Unset defaults are documented as IdP groups whengroupsClaimis on, otherwise hub role bindings; existing clusters are unchanged.Install-time guards refuse
idpwithout OIDC issuer orgroupsClaim, and post-install NOTES spell out which mode new clusters will use. Chart version bumps to 1.11.0-rc.1 with matchingappVersion, plus schema, values docs, and helm-unittest coverage for env rendering and validation failures.Reviewed by Cursor Bugbot for commit 56c16d3. Bugbot is set up for automated code reviews on this repo. Configure here.