fix(monitoring): surface missing VictoriaLogs and gate dead-end vlinsert ExternalName - #3215

Code Review

This pull request addresses issue #3181 by ensuring that the cozy-monitoring ExternalName redirects are only created when the root tenant actually hosts the platform monitoring stack. It also adds a readiness gate to the monitoring HelmRelease based on the VLCluster status and fails rendering if logsStorages is empty. The feedback highlights two potential nil pointer dereferences in the Helm templates: one when .Values.monitoring is omitted or null in apps.yaml, and another when .Values._cluster is undefined in monitoring-external-services.yaml.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@packages/system/cozystack-basics/templates/monitoring-external-services.yaml`:
- Line 31: The monitoring template logic currently uses default on
_cluster.monitoring-enabled, which converts an explicit false into "true" and
enables fallback Services unintentionally. Update the value resolution in
monitoring-external-services.yaml to explicitly check whether the key exists in
.Values._cluster before falling back, and only use "true" when the key is
missing; keep the existing $enabled handling and the surrounding template flow
intact.

Verdict

NOT LGTM

The empty-logsStorages fail and the readiness gate are sound, but the third layer (lookup-gated ExternalName render on an engine-generated HelmRelease) rests on an incorrect assumption about helm-controller re-render behaviour and creates a stale-state hole in both directions.

Findings

[MAJOR] packages/system/cozystack-basics/templates/monitoring-external-services.yaml:26-33 — lookup-gated render will go stale; the "cannot drift" claim is false

The template gates the three vlinsert/vminsert ExternalName Services on lookup of Tenant/root spec.monitoring, and the comment (line 15-16) plus the commit body assert that "the ExternalNames and the tenant's actual monitoring can never drift." That is not how helm-controller works. helm-controller re-renders a chart only on install/upgrade/rollback (chart-source change or HR values change) — never on a reconcile tick, and never when a lookup result changes. cozystack-basics is an operator-generated HelmRelease; your own in-tree note at packages/core/platform/sources/cozystack-basics.yaml:32-36 spells this out ("Drift detection is off on operator-generated HelmReleases, so a capability gate in the templates would render the policy out at first install and never add it back").

Tenant/root spec.monitoring is a runtime bool on the Tenant CR (packages/apps/tenant/values.yaml:12), toggled out-of-band and independent of the platform monitoring.rootEnabled value. So the two divergence scenarios are:

• Root monitoring ON → later turned OFF via Tenant/root spec.monitoring: no re-render is triggered, the three ExternalNames stay, and the exact dead-end redirect / silent log-drop this PR set out to eliminate persists until an unrelated chart bump forces a re-render.
• Root monitoring OFF at install → later turned ON via Tenant/root spec.monitoring: the redirects were never created and are never added back, so ingestion into a now-valid VictoriaLogs never routes — the fix silently fails to activate.

The only reliable re-render trigger is a change to the HR's valuesFrom cozystack-values Secret, i.e. flipping monitoring.rootEnabled in platform values (that mutates the Secret, which helm-controller watches). Toggling the Tenant CR alone does nothing. Either (a) make monitoring.rootEnabled the single source of truth and drop the lookup branch entirely (the lookup adds the drift risk without a reliable refresh path), or (b) keep the lookup but document, in the template comment and PR body, that the redirects only refresh when monitoring.rootEnabled is changed, and ship an operator recipe for forcing the refresh after a Tenant/root spec.monitoring toggle. As written, the code comment and commit message make a correctness claim that is false and will mislead the next maintainer.

Claim mismatches

[PARTIAL] "reads the live Tenant/root spec.monitoring via lookup (source of truth, so it cannot drift from the tenant's real state)" — the lookup is only re-evaluated on a Helm action, not on Tenant CR change; on an engine-generated HR with drift detection off, the rendered ExternalNames drift from Tenant/root spec.monitoring in both directions (see Findings). The claim is true only at the instant of an install/upgrade/values-change, not continuously.

[PARTIAL] "Covered by helm-unittest across all four affected charts (141 tests)" — the four suites run and pass (verified: 51 + 7 + 3 + 82 = 143 tests total, including pre-existing), but the new tests exercise only the fallback branch; the comment at monitoring-external-services_test.yaml:19-21 correctly notes the lookup branch is unit-untested. The drift behaviour in Findings is therefore not covered by any test. Not blocking on its own, but it means the failure mode ships unverified.

Operational risks

• Stale-lookup on Tenant/root spec.monitoring toggle (monitoring-external-services.yaml:26-33): an operator who disables/enables root monitoring the documented way (editing the Tenant CR, the PR's stated "source of truth") gets no redirect update and no failing signal — a silent regression of the very behaviour #3181 tracks. At 3am this surfaces as "logs stopped / never started" with a Ready monitoring release and correct-looking Tenant spec, with nothing in Flux events pointing at the stale ExternalName set.

Caveats

Phase 5b — existing-customer upgrade:

• packages/system/monitoring/templates/vlogs/vlogs.yaml:8-10 — the new fail on empty logsStorages is a deliberate behaviour change. logsStorages is user-facing (exposed in packages/system/monitoring-rd/cozyrds/monitoring.yaml openAPISchema, default one generic entry, no minItems). A customer running today with logsStorages: [] (currently a silent-drop config) will, on upgrade, have the monitoring-system HR fail to render and loop on retries: -1. That is the intended "loud" outcome, but it is a hard upgrade break for that config — acceptable given the rationale, noted so it is not a surprise.
• packages/core/platform/values.yaml:80-92 — new monitoring.rootEnabled: true default preserves the historical always-create behaviour for the fallback branch, so existing clusters whose cozystack-values Secret predates the key are unaffected (dig ... default true and index ... | default "true" both resolve to the create-redirects path). Verified via the three wiring tests and by reading apps.yaml:20.
• No migration script needed: no resource rename/move/adoption, no CRD change, no RBAC contraction. packages/core/platform has no values.schema.json/README.md, so no make generate artifact is owed for the values.yaml addition (verified: files absent).

Phase 5b — fresh install:

• waitStrategy/healthCheckExprs on packages/extra/monitoring/templates/helmrelease.yaml:26-48 are supported: the identical shape is already in production at packages/extra/seaweedfs/templates/seaweedfs-db.yaml:24-35 (same helm-controller). The <release>-system HR targets chartRef: ExternalArtifact (a real Flux HR), so it can carry these fields — distinct from the legacy cozystack.monitoring source. The PR's "Known limitation" is accurate: cozystack.monitoring is referenced by no active bundle (only cozystack.monitoring-application and cozystack.monitoring-agents appear in packages/core/platform/templates/bundles/system.yaml:195,208).
• CEL initial-empty-status is safe: on a freshly-applied VLCluster with no status.updateStatus, all of current/inProgress/failed are false, so helm-controller holds the release not-Ready and polls until timeout: 10m — the intended gate behaviour, not a spurious failure.

Recommended follow-ups

• Add minItems: 1 to the logsStorages schema in packages/system/monitoring values.schema.json / cozyvalues-gen source so the dashboard/API rejects an empty list up front, instead of the operator discovering it only as a stuck HR at render time. This complements the fail guard rather than replacing it.

Verdict

LGTM with non-blocking notes

The prior drift hole is resolved — the vlinsert/vminsert ExternalName gate no longer uses lookup; it now reads _cluster.monitoring-enabled (fed from the new monitoring.rootEnabled platform value), which lands in the cozystack-values Secret that cozystack-basics consumes via valuesFrom, so flipping it re-renders the chart. Remaining items are a stale PR-body description and two edge-case UX notes, none blocking.

Findings

[MINOR] packages/system/monitoring/templates/vlogs/vlogs.yaml:8-10 — empty-logsStorages hard-fail is reachable by user input with no schema guard

logsStorages is a user-settable array in the monitoring ApplicationDefinition openAPISchema (packages/system/monitoring-rd/cozyrds/monitoring.yaml:11) and carries no minItems constraint, so the dashboard/API accepts an explicit logsStorages: []. With this PR that configuration now hard-fails the HelmRelease render (helm fail) instead of rendering nothing. For an existing monitoring app CR that was deliberately set to an empty list, an upgrade flips it from "silently renders no VLCluster" to "HR stuck in render failure". The loud-fail is the intended behaviour and the default ([{generic...}]) is non-empty, so this is not a regression for standard installs — but the failure would be more legible if minItems: 1 were added to the logsStorages schema in the RD so the API rejects the empty list up front with a validation message, rather than surfacing as an HR reconcile failure. Consider adding minItems: 1 (regenerated via make generate) as a follow-up.

Claim mismatches

[PARTIAL] "The template reads the live Tenant/root spec.monitoring via lookup (source of truth, so it cannot drift from the tenant's real state)" (PR body, layer 3) — the shipped code does NOT use lookup. packages/system/cozystack-basics/templates/monitoring-external-services.yaml:26 gates on index (.Values._cluster | default dict) "monitoring-enabled", and the in-file comment (lines 13-20) explicitly explains why lookup was rejected (operator-generated HR, drift detection off, re-renders only on install/upgrade/values-change). The PR body still describes the earlier lookup-based revision and repeats the "cannot drift" framing, which is now inaccurate — the value can go stale if an operator changes Tenant/root spec.monitoring out-of-band without updating monitoring.rootEnabled. The CodeRabbit summary and the git commit body correctly describe the value-driven approach; only the top "What this PR does" section is stale. Update the PR body layer-3 text to match the shipped mechanism before merge so reviewers and operators do not carry a false mental model.

Caveats

• Upgrade impact (existing customer): verified no migration is needed and no breakage for the default path. Clusters on N-1 carry a cozystack-values Secret without the monitoring-enabled key; the template's | default "true" (line 26) keeps all three ExternalName redirects, and once packages/core/platform re-renders the Secret the key is emitted as "true" by default (apps.yaml:140, dig "rootEnabled" true verified: default→"true", explicit false→"false", null→"true", no panic). So existing behaviour is preserved unless an operator explicitly sets rootEnabled: false. repo_invariants and cozy_invariants both empty; no numbered migration required.
• Fresh install: verified safe. system/monitoring default logsStorages is non-empty (packages/system/monitoring/values.yaml:43, generic), so the new render-fail does not fire on cold install. The waitStrategy: poller + healthCheckExprs shape is an established precedent (packages/extra/seaweedfs/templates/seaweedfs-db.yaml:24-35, identical structure), confirming the in-use helm-controller supports these HelmRelease v2 fields. packages/core/platform has no values.schema.json/cozyvalues-gen target, so the monitoring.rootEnabled addition needs no make generate artifacts.
• CI wiring: verified the added test: target in packages/extra/monitoring/Makefile does wire the suite into CI — hack/helm-unit-tests.sh iterates chart dirs and runs make -C <dir> test only where the target exists (make -C "$dir" -n test guard), so a chart with no test: target was silently skipped before. All four charts' suites pass locally (143 tests: 51 + 7 + 3 + 82).
• The functional correctness of the CEL healthCheckExprs (operational/expanding/catch-all-failed mapping on status.updateStatus) is not exercised by helm-unittest (the tests only pin the string values); it can only be validated live against victoria-metrics-operator. The mapping is defensible on paper (unknown status can never read as Ready), but end-to-end confirmation on a dev cluster is recommended before relying on it as the log-store backstop.
• The residual out-of-band drift (operator toggles Tenant/root spec.monitoring without updating monitoring.rootEnabled) is an accepted, documented tradeoff — surfaced in both the template comment (lines 13-20) and values.yaml (packages/core/platform/values.yaml:88-95, "operators MUST set it to match their actual Tenant/root spec.monitoring"). This is the correct honest framing; the only gap is the stale PR-body text noted under Claim mismatches.