Repository navigation
fix(foundationdb): don't gate WorkloadMonitor behind monitoring.enabled - #3490
Timofei Larkin (lllamnyp) wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughChangesFoundationDB monitoring
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-03T11:35:36Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: kubernetes scan error: fs filter error: fs filter error: walk error range error: stat .golangci.yml: no such file or directory: range error: stat .golangci.yml: no such file or directory Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. Correct root-cause fix: gating WorkloadMonitor behind the tenant-facing monitoring.enabled let a tenant disable their own billing/metering, and foundationdb was the only app doing this.
Verified:
- After the change,
monitoring.enabledgates onlytemplates/dashboard-resourcemap.yaml(a UI ConfigMap), so the reworded description is accurate — no prose drift. - Of the 12 apps shipping a
WorkloadMonitor, the only remaining gate is clickhouse's, and that one is onclickhouseKeeper.enabled(a separate secondary monitor), not a tenant toggle on the primary. The "foundationdb was the only one" claim holds. - The rendered
WorkloadMonitorbody is unchanged; only theif/endwrapper was removed. helm unittest .passes locally (2/2). The suite covers bothmonitoring.enabled: trueandfalse, asserting kind, name, and document count — an adequate config matrix for this change.- The new
.PHONY: testtarget matches the sibling convention and is discovered byhack/helm-unit-tests.sh. - The new description string is consistent across the source (
values.yaml) and all regenerated artifacts (types.go,values.schema.json,README.md, cozyrds CRD); no stale "Enable WorkloadMonitor integration" left anywhere. - Existing tenants with
monitoring.enabled: falsewill get theWorkloadMonitorcreated on upgrade — the intended restoration of metering, no breakage.
Non-blocking:
monitoring.enabledis now effectively a misnomer (it controls only the dashboard resource map, not monitoring). Renaming the field would be a breaking schema change, so fixing just the description is the conservative choice; a follow-up rename could be considered.- Leaving
dashboard-resourcemap.yamlgated is a reasonable scope call.
WorkloadMonitor is a billing/metering meta-resource, not a tenant convenience feature: hiding it behind monitoring.enabled let a tenant turn off their own metering by flipping a values toggle. Every other app in the catalog ships its WorkloadMonitor unconditionally; drop the gate here so foundationdb matches. monitoring.enabled still gates dashboard-resourcemap.yaml (a dashboard/UI resource list, not a metering one), so the value stays. Assisted-By: Claude <[email protected]> Signed-off-by: Timofei Larkin <[email protected]>
9dbc8b6 to
ad95a15
Compare
#4612) ## What this PR does Fixes #4611. Since #4148 a tenant may read the `<release>-config` ConfigMap that holds the FoundationDB cluster file, but the console has no view for ConfigMaps, so the connection string was still reachable only with kubectl. Chart: - `<release>-resourcemap` is rendered regardless of `monitoring.enabled`, because the console now reads it; - the tenant Role grants `get`, `list` and `watch` on `<release>-resourcemap` next to `<release>-config`, still by `resourceNames`, with no wider ConfigMap access. Console: - the application page gets a ConfigMaps tab when the application's resource map lists at least one ConfigMap in the same namespace; - each listed ConfigMap is read and watched with a `metadata.name` field selector, which RBAC checks against `resourceNames`, so a tenant never needs `list` on all ConfigMaps; - values are shown as plain text with a copy button, the cluster file needs no decoding; - a resource map that is missing or forbidden hides the tab, a listed ConfigMap that cannot be read shows the API error instead of looking empty, and a malformed map or an invalid name is reported. The tab is generic, but today only FoundationDB ships a resource map, so it is the only application that gets it. #3490 keeps `monitoring.enabled` as the switch for the resource map and asks whether the map should render unconditionally. This PR answers that for FoundationDB by rendering it always, so the wording in #3490 would need to follow if both land. Tests: `helm unittest` for the chart (6 tests), `tsc --noEmit`, `vitest run` (438 tests in 60 files) and `eslint` on the changed files pass. The new tests cover reading only the listed names in the active namespace, the watch, copying, a clipboard failure, a read denial, and malformed maps. The chart renders with `monitoring.enabled` true and false. ### Screenshots The ConfigMaps tab of a FoundationDB application, rendered by the console from this branch against a local stub API with synthetic data:  ### Downstream repositories Walked the trigger map against the diff: it changes a FoundationDB chart template and its test, and the console. No `values.yaml`, `values.schema.json`, README, CRD or `hack/` file is touched. The website FoundationDB page is generated from the chart README and says nothing about connecting, and the Terraform provider does not read these ConfigMaps. The console is vendored in this repository. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(dashboard, foundationdb): the application page shows the FoundationDB connection ConfigMap with the cluster file to tenants who may read it ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Application details now include a ConfigMaps tab when configuration maps are available. View their values and copy them to the clipboard. * The tab displays loading, empty, unavailable, and error states. * **Bug Fixes** * The resource map remains available when monitoring is disabled, and its access permissions are included. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
This PR has had no activity for 60 days and was marked |
What this PR does
packages/apps/foundationdb/templates/workloadmonitor.yamlgated itsWorkloadMonitorbehind the tenant-facingmonitoring.enabledflag — the only app in the catalog that does this. AWorkloadMonitoris a billing/metering meta-resource, not a tenant convenience feature, so gating it behind a values toggle let a tenant turn off their own metering simply by settingmonitoring.enabled: false. Every other app inpackages/apps/*ships itsWorkloadMonitorunconditionally. This PR removes the{{- if .Values.monitoring.enabled }}/{{- end }}gate so foundationdb matches, and updates themonitoring.enableddescription to reflect what it actually still controls (the dashboard resource map, see below), since it no longer touches theWorkloadMonitor.monitoring.enabledremains as a value:packages/apps/foundationdb/templates/dashboard-resourcemap.yaml(aConfigMaplisting resources for the tenant dashboard UI) is still gated on it and is left untouched by this PR. That resource is a UI/dashboard concern rather than a billing one, and unlikeWorkloadMonitor, foundationdb is the only app with this particularConfigMap-based resourcemap pattern at all — every other app instead ships an RBACRole/RoleBindinggranting the tenant read access to dashboard resources, unconditionally. Given that, I did not treatdashboard-resourcemap.yamlas an instance of the same bug and left it alone, but flagging it here since a reviewer may want to weigh in: should this resource also render unconditionally (matching the RBAC-based pattern other apps use), or is gating a pure dashboard/UI resource behind a monitoring toggle fine as-is?Verification
helm templatewithmonitoring.enabled: trueandmonitoring.enabled: false: theWorkloadMonitornow renders in both cases, and its rendered output is byte-identical to what it rendered before this change when the flag wastrue.packages/apps/foundationdb/tests/workloadmonitor_test.yaml(ahelm unittestsuite, matching the pattern already used by sibling apps such as kafka'stests/workloadmonitor_test.yaml) asserting theWorkloadMonitorrenders with bothmonitoring.enabled: trueandmonitoring.enabled: false. Wired it intomake testvia a newtesttarget in the packageMakefile, again matching the sibling convention, so it runs undermake unit-tests/hack/helm-unit-tests.sh.values.schema.json, the README parameter table, the generated Go types, and thecozyrdsCRD viamake generatein the package.Screenshots
Not applicable — no UI change.
Downstream repositories
This change only removes a template-level gate and updates a description string; it does not add, remove, or rename a
values.schema.jsonfield, change a version enum, rename a CRD kind orrelease.prefix, or touch any of the other triggers listed indocs/agents/contributing.md.Release note
Summary by CodeRabbit
New Features
monitoring.enabledsetting.Documentation
monitoring.enabledcontrols the tenant UI dashboard resource map, while WorkloadMonitor integration is always enabled.