Skip to content

fix(foundationdb): don't gate WorkloadMonitor behind monitoring.enabled - #3490

Open
Timofei Larkin (lllamnyp) wants to merge 1 commit into
mainfrom
fix/foundationdb-workloadmonitor-ungate
Open

Timofei Larkin (lllamnyp) wants to merge 1 commit into
mainfrom
fix/foundationdb-workloadmonitor-ungate

Conversation

@lllamnyp

@lllamnyp Timofei Larkin (lllamnyp) commented Jul 30, 2026 •

Copy link
Copy Markdown
Member

What this PR does

packages/apps/foundationdb/templates/workloadmonitor.yaml gated its WorkloadMonitor behind the tenant-facing monitoring.enabled flag — the only app in the catalog that does this. A WorkloadMonitor is 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 setting monitoring.enabled: false. Every other app in packages/apps/* ships its WorkloadMonitor unconditionally. This PR removes the {{- if .Values.monitoring.enabled }} / {{- end }} gate so foundationdb matches, and updates the monitoring.enabled description to reflect what it actually still controls (the dashboard resource map, see below), since it no longer touches the WorkloadMonitor.

monitoring.enabled remains as a value: packages/apps/foundationdb/templates/dashboard-resourcemap.yaml (a ConfigMap listing 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 unlike WorkloadMonitor, foundationdb is the only app with this particular ConfigMap-based resourcemap pattern at all — every other app instead ships an RBAC Role/RoleBinding granting the tenant read access to dashboard resources, unconditionally. Given that, I did not treat dashboard-resourcemap.yaml as 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 template with monitoring.enabled: true and monitoring.enabled: false: the WorkloadMonitor now renders in both cases, and its rendered output is byte-identical to what it rendered before this change when the flag was true.
  • Added packages/apps/foundationdb/tests/workloadmonitor_test.yaml (a helm unittest suite, matching the pattern already used by sibling apps such as kafka's tests/workloadmonitor_test.yaml) asserting the WorkloadMonitor renders with both monitoring.enabled: true and monitoring.enabled: false. Wired it into make test via a new test target in the package Makefile, again matching the sibling convention, so it runs under make unit-tests / hack/helm-unit-tests.sh.
  • Regenerated values.schema.json, the README parameter table, the generated Go types, and the cozyrds CRD via make generate in the package.

Screenshots

Not applicable — no UI change.

Downstream repositories

  • No downstream repository is affected by this change

This change only removes a template-level gate and updates a description string; it does not add, remove, or rename a values.schema.json field, change a version enum, rename a CRD kind or release.prefix, or touch any of the other triggers listed in docs/agents/contributing.md.

Release note

fix(foundationdb): the FoundationDB `WorkloadMonitor` now renders unconditionally instead of being gated behind `monitoring.enabled`, since it is the metering resource used for billing and tenants should not be able to disable it.

Summary by CodeRabbit

  • New Features

    • WorkloadMonitor resources are now created regardless of the monitoring.enabled setting.
    • Added automated validation covering both enabled and disabled monitoring configurations.
  • Documentation

    • Clarified that monitoring.enabled controls the tenant UI dashboard resource map, while WorkloadMonitor integration is always enabled.
    • Updated related configuration references and schema descriptions.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions github-actions Bot added size/M This PR changes 30-99 lines, ignoring generated files area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/bug Categorizes issue or PR as related to a bug labels Jul 30, 2026
@lllamnyp
Timofei Larkin (lllamnyp) marked this pull request as ready for review July 30, 2026 14:18
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@dosubot dosubot Bot added the area/monitoring Issues or PRs related to the monitoring stack (vlogs, vmstack, grafana, workloadmonitor) label Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2dc74309-f470-47f1-918e-7abbdf2cd46c

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbc8b6 and ad95a15.

📒 Files selected for processing (8)
  • api/apps/v1alpha1/foundationdb/types.go
  • packages/apps/foundationdb/Makefile
  • packages/apps/foundationdb/README.md
  • packages/apps/foundationdb/templates/workloadmonitor.yaml
  • packages/apps/foundationdb/tests/workloadmonitor_test.yaml
  • packages/apps/foundationdb/values.schema.json
  • packages/apps/foundationdb/values.yaml
  • packages/system/foundationdb-rd/cozyrds/foundationdb.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
  • packages/apps/foundationdb/values.yaml
  • packages/apps/foundationdb/values.schema.json
  • packages/system/foundationdb-rd/cozyrds/foundationdb.yaml
  • packages/apps/foundationdb/templates/workloadmonitor.yaml
  • packages/apps/foundationdb/README.md
  • api/apps/v1alpha1/foundationdb/types.go
  • packages/apps/foundationdb/tests/workloadmonitor_test.yaml

📝 Walkthrough

Walkthrough

Changes

FoundationDB monitoring

Layer / File(s) Summary
Always-rendered WorkloadMonitor and tests
packages/apps/foundationdb/templates/workloadmonitor.yaml, packages/apps/foundationdb/tests/workloadmonitor_test.yaml, packages/apps/foundationdb/Makefile
The WorkloadMonitor renders for both monitoring states. Helm tests verify its output. The package Makefile adds a test target.
Monitoring descriptions and schemas
api/apps/v1alpha1/foundationdb/types.go, packages/apps/foundationdb/values.schema.json, packages/apps/foundationdb/values.yaml, packages/system/foundationdb-rd/cozyrds/foundationdb.yaml, packages/apps/foundationdb/README.md
Comments, schemas, values documentation, and README text describe monitoring.enabled as controlling the tenant UI dashboard resource map. WorkloadMonitor creation remains unconditional.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: area/database, area/testing

Suggested reviewers: myasnikovdaniil, sircthulhu, kvaps

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: WorkloadMonitor no longer depends on monitoring.enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/foundationdb-workloadmonitor-ungate

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.enabled gates only templates/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 on clickhouseKeeper.enabled (a separate secondary monitor), not a tenant toggle on the primary. The "foundationdb was the only one" claim holds.
  • The rendered WorkloadMonitor body is unchanged; only the if/end wrapper was removed.
  • helm unittest . passes locally (2/2). The suite covers both monitoring.enabled: true and false, asserting kind, name, and document count — an adequate config matrix for this change.
  • The new .PHONY: test target matches the sibling convention and is discovered by hack/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: false will get the WorkloadMonitor created on upgrade — the intended restoration of metering, no breakage.

Non-blocking:

  • monitoring.enabled is 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.yaml gated 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]>
@lllamnyp
Timofei Larkin (lllamnyp) force-pushed the fix/foundationdb-workloadmonitor-ungate branch from 9dbc8b6 to ad95a15 Compare August 3, 2026 11:34
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Sep 30, 2026
#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:

![ConfigMaps
tab](https://raw.githubusercontent.com/yankawai/cozystack/29ae3c94e539edc05b29700788a9b875b5a13d54/foundationdb-configmaps-tab.png)

### 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 -->
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

This PR has had no activity for 60 days and was marked lifecycle/stale.
It will be closed in 14 days unless commented or labelled lifecycle/frozen.

@github-actions github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale label Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/monitoring Issues or PRs related to the monitoring stack (vlogs, vmstack, grafana, workloadmonitor) area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/bug Categorizes issue or PR as related to a bug lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants