Skip to content

Commit 1bd3da9

Browse files
IvanHuntersIvanHunters
committed
fix(monitoring): close self-review blockers for the OIDC PR
Address the two blocking-class findings from the cozy-review pass on this PR: (1) Missing PackageSource dependency on cozystack.keycloak-operator (MAJOR cold-install race). Without this edge, the monitoring chart can reconcile before the v1.edp.epam.com CRDs are registered on a fresh cluster — the `.Capabilities.APIVersions.Has` guard in templates/grafana/oidc-keycloak.yaml then silently drops all Keycloak-side objects for mode: System, and Grafana's auth.generic_oauth block points at a client Keycloak never provisions. Add the ordering edge, plus a sources_monitoring_application_dependson_test.yaml regression guard so a future edit cannot drop it silently. (2) allow_assign_grafana_admin driven by `.Release.Name == monitoring-system` (MINOR fragile coupling). Replace the release-name sniff with an explicit `oidc.grafanaAdmin: false` values field, and flip it on for the platform Grafana release via Package.spec.components["monitoring-system"].values.oidc.grafanaAdmin in packages/core/platform/templates/bundles/system.yaml — mirroring the pattern monitoring-agents already uses for per-component overrides. Tenant Grafana releases inherit the chart default false and stay at org-level Admin. Two new helm-unittest cases in packages/system/monitoring/tests/ lock the values-driven behaviour on both sides (grafanaAdmin=true → Admin uplift, default → tenant stays at org Admin), plus bundles_monitoring_grafana_admin_test.yaml in packages/core/platform/tests/ regression-guards the Package's component override so the platform release keeps its uplift on future edits. Also document the mode-toggle group-membership loss in docs/oidc-grafana.md — mode: System → CustomConfig/None deletes the three chart-owned KeycloakRealmGroups; membership is not restored on toggle-back. Not a bug, but the operator UX cost of the toggle deserves a bullet in Failure modes. Regenerated artifacts (cozyvalues-gen + hack/update-crd.sh): packages/extra/monitoring/values.schema.json packages/extra/monitoring/README.md packages/system/monitoring-rd/cozyrds/monitoring.yaml Test suites still green: packages/system/monitoring 23/23, packages/core/platform 81/81. Signed-off-by: Ivan Okhotnikov <[email protected]>
1 parent c458dd7 commit 1bd3da9

12 files changed

Lines changed: 154 additions & 29 deletions

File tree

‎docs/oidc-grafana.md‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,16 @@ CR:
8181
6. A `GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET` env on the Grafana
8282
Deployment sourced from the Secret in step 4.
8383

84-
For the platform release (release name `monitoring-system`), the chart
85-
sets `allow_assign_grafana_admin: true` so an `Admin`-group member is
86-
auto-promoted to server-level `GrafanaAdmin` (necessary because the
87-
platform Grafana serves as the meta-admin dashboard). Tenant Grafana
88-
instances keep it `false` — organisation-level Admin is enough.
84+
Server-level `GrafanaAdmin` promotion is opt-in via
85+
`spec.oidc.grafanaAdmin: true`. When set, an `<clientId>-admin` group
86+
member is auto-promoted to `GrafanaAdmin` (via
87+
`allow_assign_grafana_admin: true` in the rendered `[auth.generic_oauth]`
88+
block). The platform bundle
89+
(`packages/core/platform/templates/bundles/system.yaml`) flips it on for
90+
the platform Grafana release via
91+
`Package.spec.components["monitoring-system"].values.oidc.grafanaAdmin: true`;
92+
tenant Grafana releases inherit the chart default `false` and stay at
93+
org-level Admin.
8994

9095
## CustomConfig mode
9196

@@ -185,6 +190,16 @@ work — useful when Keycloak is down or misconfigured.
185190
mappings** — Grafana rejects the callback and the login screen
186191
shows an error; the `admin_user`/`admin_password` Secret keeps
187192
working as break-glass.
193+
- **Mode toggle destroys chart-owned `KeycloakRealmGroup`
194+
membership.** The three groups (`<clientId>-{admin,editor,viewer}`)
195+
are release-owned. Flipping `spec.oidc.mode` from `System` to
196+
`CustomConfig` or `None` runs a `helm upgrade` that deletes the
197+
three `KeycloakRealmGroup` objects — and, with them, every user's
198+
membership. Flipping back to `System` recreates the groups empty;
199+
the platform operator has to re-populate them by hand (via the
200+
Keycloak UI or new `KeycloakRealmUser` CRs). Not a bug, but a
201+
one-way UX cost on the toggle; treat it like rotating an admin
202+
Secret.
188203
- **`emailVerified` on Keycloak users is a prescriptive requirement,
189204
not a chart-enforced one.** The chart does not emit any
190205
`claimValidationRules` — the layered guarantees you rely on

‎packages/core/platform/sources/monitoring-application.yaml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ spec:
1818
- cozystack.victoria-metrics-operator
1919
# cozystack-engine ships the ApplicationDefinition CRD rendered by the *-rd component below.
2020
- cozystack.cozystack-engine
21+
# keycloak-operator ships the v1.edp.epam.com CRDs consumed by
22+
# the OIDC selector (spec.oidc.mode: System renders KeycloakClient,
23+
# KeycloakClientScope and three KeycloakRealmGroups in the cozy
24+
# realm). Without this dep the monitoring chart can reconcile
25+
# before the CRDs are registered on cold install — the
26+
# .Capabilities.APIVersions.Has guard then silently drops all
27+
# Keycloak-side objects and Grafana's auth.generic_oauth block
28+
# points at a client that never gets provisioned.
29+
- cozystack.keycloak-operator
2130
libraries:
2231
- name: cozy-lib
2332
path: library/cozy-lib

‎packages/core/platform/templates/bundles/system.yaml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,13 @@
192192
{{include "cozystack.platform.package.default" (list "cozystack.gateway-application" $) }}
193193
{{include "cozystack.platform.package.default" (list "cozystack.seaweedfs-application" $) }}
194194
{{include "cozystack.platform.package.default" (list "cozystack.info-application" $) }}
195-
{{include "cozystack.platform.package.default" (list "cozystack.monitoring-application" $) }}
195+
{{- /* Platform Grafana (`monitoring-system`) — flip GrafanaAdmin uplift on
196+
so `<clientId>-admin` group members become server-level Grafana
197+
admins. Tenant Grafana releases (component `monitoring`) inherit
198+
the chart default `oidc.grafanaAdmin: false` and stay at
199+
org-level Admin. See docs/oidc-grafana.md and cozystack#3176. */}}
200+
{{- $monitoringApplicationComponents := dict "monitoring-system" (dict "values" (dict "oidc" (dict "grafanaAdmin" true))) -}}
201+
{{include "cozystack.platform.package" (list "cozystack.monitoring-application" "default" $ $monitoringApplicationComponents) }}
196202
{{include "cozystack.platform.package.default" (list "cozystack.etcd-application" $) }}
197203
{{include "cozystack.platform.package.default" (list "cozystack.cozystack-basics" $) }}
198204
{{include "cozystack.platform.package.default" (list "cozystack.backupstrategy-controller" $) }}

‎packages/core/platform/tests/bundles_monitoring_grafana_admin_test.yaml‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
suite: bundles.system monitoring-application component wiring (Grafana OIDC uplift)
2+
templates:
3+
- templates/bundles/system.yaml
4+
release:
5+
name: cozystack
6+
namespace: cozy-system
7+
tests:
8+
- it: monitoring-application Package emits the GrafanaAdmin uplift for the platform Grafana release
9+
set:
10+
bundles:
11+
system:
12+
enabled: true
13+
variant: isp-full
14+
documentSelector:
15+
path: metadata.name
16+
value: cozystack.monitoring-application
17+
asserts:
18+
- equal:
19+
path: kind
20+
value: Package
21+
- equal:
22+
path: spec.variant
23+
value: default
24+
# The platform Grafana release (component `monitoring-system`) is
25+
# promoted to server-level GrafanaAdmin via the explicit values
26+
# override. Tenant Grafana (component `monitoring`) is NOT listed
27+
# in .spec.components — inherits the chart default
28+
# oidc.grafanaAdmin: false and stays at org-level Admin.
29+
- equal:
30+
path: spec.components.monitoring-system.values.oidc.grafanaAdmin
31+
value: true
32+
- notExists:
33+
path: spec.components.monitoring.values.oidc.grafanaAdmin

‎packages/core/platform/tests/sources_monitoring_application_dependson_test.yaml‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
suite: monitoring-application waits for the Keycloak operator that owns the OIDC CRDs
2+
# Regression guard for a fresh-install ordering race introduced by the Phase-1
3+
# OIDC selector (cozystack/cozystack#3176). Without the keycloak-operator edge,
4+
# the monitoring chart can reconcile before the v1.edp.epam.com CRDs are
5+
# registered on cold install — the `.Capabilities.APIVersions.Has` guard in
6+
# templates/grafana/oidc-keycloak.yaml silently drops the whole Keycloak side
7+
# of System mode (KeycloakClient / KeycloakClientScope / KeycloakRealmGroups),
8+
# Grafana boots with an auth.generic_oauth block pointing at a client that was
9+
# never provisioned, and OIDC login fails at request time with no build-time
10+
# signal.
11+
templates:
12+
- templates/sources.yaml
13+
release:
14+
name: cozystack
15+
namespace: cozy-system
16+
tests:
17+
- it: monitoring-application dependsOn keycloak-operator (OIDC CRDs)
18+
documentSelector:
19+
path: metadata.name
20+
value: cozystack.monitoring-application
21+
asserts:
22+
- contains:
23+
path: spec.variants[0].dependsOn
24+
content: cozystack.keycloak-operator
25+
# The other deps we rely on transitively for the Grafana instance to
26+
# even come up on a fresh cluster; regression-guard them together so
27+
# a future edit does not accidentally drop the ordering edges.
28+
- contains:
29+
path: spec.variants[0].dependsOn
30+
content: cozystack.networking
31+
- contains:
32+
path: spec.variants[0].dependsOn
33+
content: cozystack.grafana-operator
34+
- contains:
35+
path: spec.variants[0].dependsOn
36+
content: cozystack.postgres-operator
37+
- contains:
38+
path: spec.variants[0].dependsOn
39+
content: cozystack.victoria-metrics-operator

‎packages/extra/monitoring/README.md‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,13 @@
104104

105105
### OIDC authentication
106106

107-
| Name | Description | Type | Value |
108-
| ---------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ------ |
109-
| `oidc` | OIDC authentication for the Grafana instance. See docs/oidc-grafana.md for the operator guide. | `object` | `{}` |
110-
| `oidc.mode` | Identity mode. `None`: no OIDC, only the `grafana-admin-password` Secret works. `System`: trust the platform `cozy` realm via a per-instance confidential client and audience binding; zero-config default. `CustomConfig`: trust a tenant-supplied issuer directly (BYO); `cozy` is not in the path. | `string` | `None` |
111-
| `oidc.customConfig` | Tenant-supplied `[auth.generic_oauth]` payload; consumed only when `mode: CustomConfig`. | `object` | `{}` |
112-
| `oidc.customConfig.config` | Inline map of `[auth.generic_oauth]` keys. Written verbatim into the Grafana CR `spec.config`. Mutually exclusive with `secretRef.name`. | `object` | `{}` |
113-
| `oidc.customConfig.secretRef` | Reference to a Secret holding an `auth.ini` fragment; mounted under `/etc/grafana/oidc` and loaded via `GF_PATHS_CUSTOM_INI`. | `object` | `{}` |
114-
| `oidc.customConfig.secretRef.name` | Name of an existing Secret in the release namespace whose `auth.ini` key holds a ready-made `[auth.generic_oauth]` block. Mutually exclusive with `customConfig.config`. | `string` | `""` |
107+
| Name | Description | Type | Value |
108+
| ---------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ------- |
109+
| `oidc` | OIDC authentication for the Grafana instance. See docs/oidc-grafana.md for the operator guide. | `object` | `{}` |
110+
| `oidc.mode` | Identity mode. `None`: no OIDC, only the `grafana-admin-password` Secret works. `System`: trust the platform `cozy` realm via a per-instance confidential client and audience binding; zero-config default. `CustomConfig`: trust a tenant-supplied issuer directly (BYO); `cozy` is not in the path. | `string` | `None` |
111+
| `oidc.customConfig` | Tenant-supplied `[auth.generic_oauth]` payload; consumed only when `mode: CustomConfig`. | `object` | `{}` |
112+
| `oidc.customConfig.config` | Inline map of `[auth.generic_oauth]` keys. Written verbatim into the Grafana CR `spec.config`. Mutually exclusive with `secretRef.name`. | `object` | `{}` |
113+
| `oidc.customConfig.secretRef` | Reference to a Secret holding an `auth.ini` fragment; mounted under `/etc/grafana/oidc` and loaded via `GF_PATHS_CUSTOM_INI`. | `object` | `{}` |
114+
| `oidc.customConfig.secretRef.name` | Name of an existing Secret in the release namespace whose `auth.ini` key holds a ready-made `[auth.generic_oauth]` block. Mutually exclusive with `customConfig.config`. | `string` | `""` |
115+
| `oidc.grafanaAdmin` | Promote members of the `<clientId>-admin` group to server-level `GrafanaAdmin` (sets `allow_assign_grafana_admin: true` in `[auth.generic_oauth]`). Intended for the platform release; tenant Grafana keeps org-level Admin. | `bool` | `false` |
115116

‎packages/extra/monitoring/values.schema.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,11 @@
628628
}
629629
}
630630
},
631+
"grafanaAdmin": {
632+
"description": "Promote members of the `\u003cclientId\u003e-admin` group to server-level `GrafanaAdmin` (sets `allow_assign_grafana_admin: true` in `[auth.generic_oauth]`). Intended for the platform release; tenant Grafana keeps org-level Admin.",
633+
"type": "boolean",
634+
"default": false
635+
},
631636
"mode": {
632637
"description": "Identity mode. `None`: no OIDC, only the `grafana-admin-password` Secret works. `System`: trust the platform `cozy` realm via a per-instance confidential client and audience binding; zero-config default. `CustomConfig`: trust a tenant-supplied issuer directly (BYO); `cozy` is not in the path.",
633638
"type": "string",

‎packages/extra/monitoring/values.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ vmagent:
205205
## @typedef {struct} OIDC - OIDC selector for the Grafana instance (Phase 1 — per-instance audience binding; no per-tenant Keycloak realm).
206206
## @field {OIDCMode} mode - Identity mode. `None`: no OIDC, only the `grafana-admin-password` Secret works. `System`: trust the platform `cozy` realm via a per-instance confidential client and audience binding; zero-config default. `CustomConfig`: trust a tenant-supplied issuer directly (BYO); `cozy` is not in the path.
207207
## @field {OIDCCustomConfig} [customConfig] - Tenant-supplied `[auth.generic_oauth]` payload; consumed only when `mode: CustomConfig`.
208+
## @field {bool} [grafanaAdmin] - Promote members of the `<clientId>-admin` group to server-level `GrafanaAdmin` (sets `allow_assign_grafana_admin: true` in `[auth.generic_oauth]`). Intended for the platform release; tenant Grafana keeps org-level Admin.
208209

209210
## @param {OIDC} oidc - OIDC authentication for the Grafana instance. See docs/oidc-grafana.md for the operator guide.
210211
oidc:
@@ -213,3 +214,4 @@ oidc:
213214
config: {}
214215
secretRef:
215216
name: ""
217+
grafanaAdmin: false

0 commit comments

Comments
 (0)