feat(workloadmonitor): bucket metrics endpoint override - #3443

Verdict

LGTM with non-blocking notes — the change is opt-in and backwards-compatible, the billing-safety fix is real and protected by non-vacuous tests, and the one behavioural trade-off worth naming is a deliberate design choice, not a regression.

Findings

No blocking findings. What was verified (executed, not reasoned):

• Build/vet/tests: go build ./cmd/cozystack-controller/... ./internal/controller/... and go vet ./internal/controller/... clean; go test ./internal/controller/... for the touched tests passes; gofmt -l clean on all three Go files; chart helm unittest passes.
• Non-vacuity (Phase 5d): reverted the retention-seeding loop in reconcileBucketClaimForMonitor to its pre-fix shape and re-ran — TestReconcileRetainsLastKnownSizesOnQueryFailure and TestReconcileRetainsLastKnownSizesWhenBucketMissingFromResult both went RED (got 0 (present=false)), so the regression tests genuinely guard the fix.
• Chart matrix (Phase 5c): rendered both corners of the new {{- with .Values.cozystackController.seaweedfsMetricsEndpoint }} toggle. Default (empty) omits the arg; set value emits - --seaweedfs-metrics-endpoint=.... No dot-scope bug. This system chart has no values.schema.json/README.md and no ApplicationDefinition, so the make generate convention does not apply.

Claim mismatches

None. Spot-checked the load-bearing self-claims and each held under independent re-derivation:

• "No existing deployment changes behaviour" — confirmed: resolvePrometheusURL returns the override early only when the field is non-empty; the default empty string falls through to the identical label-discovery path.
• "Failures no longer collapse to zero" — confirmed: queryAllBucketMetrics now returns errors, and reconcileBucketClaimForMonitor seeds resources from the persisted workload.Status.Resources before overlaying fresh metrics. Note Workload has NO +kubebuilder:subresource:status marker and its CRD ships subresources: {} (api/v1alpha1/workload_types.go:52, packages/system/cozystack-controller/definitions/cozystack.io_workloads.yaml:88), so .status is a regular field that ctrl.CreateOrUpdate's plain Update persists — the retention actually works in production, and the tests' non-modelling of a Workload status subresource matches reality.
• "Settable exclusively by platform administrators, no new tenant surface" — confirmed: plumbed only as a controller CLI arg via values.yaml + deployment.yaml; the reconciler field is set once at startup and never mutated. The rejected WorkloadMonitor-spec-field alternative (SSRF via tenant-writable values) is correctly reasoned.

Caveats

• Success-with-missing-series retention (billing accuracy, could-not-verify): reconcileBucketClaimForMonitor retains the last known size both when the query FAILS (unambiguously correct) and when the query SUCCEEDS but returns no series for a bucket that previously had a size (internal/controller/workloadmonitor_controller.go:328-355). The old code dropped the size to nothing in the success-missing-series case. If SeaweedFS reports 0 for an existing-but-empty bucket, the new code updates to 0 correctly; if SeaweedFS instead DROPS the series for an empty bucket, retention would keep a stale non-zero size and over-bill. I could not verify SeaweedFS's empty-bucket export behaviour hermetically (no live cluster; Execution boundary). Given the fix's billing-safety intent, retention is a reasonable default, but the two cases are conflated deliberately.
• Credential-leak concern refuted: the endpoint may embed basic-auth userinfo, and the failure path emits a Warning Event on the WorkloadMonitor, which lives in a TENANT namespace (tenant-readable). I reproduced the transport-error path locally — net/http redacts the password to *** in the *url.Error it returns from Client.Do, so the Event/log carry http://billing:***@.../api/v1/query, not the cleartext secret. The non-200 / JSON-parse / status-not-success error branches carry no URL. No tenant-facing credential leak. (The startup validation error setupLog.Error(err, "invalid --seaweedfs-metrics-endpoint") can log the raw admin-supplied string with credentials, but that is the admin's own value in the admin-only controller log — not flagged.)
• RBAC: the new +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch marker is already satisfied by the shipped ClusterRole cozystack-controller (packages/system/cozystack-controller/templates/rbac.yaml:44-45, cluster-scoped events: [create, patch]), so the new Recorder works in tenant namespaces. No RBAC gap.
• Upgrade impact (Phase 5b.A): controller-code-only change, no migration/CRD/default-flip. On upgrade the new binary rolls out; empty flag = prior behaviour; existing Workloads' sizes are retained on the first post-upgrade reconcile. On persistent metrics failure the requeue cadence changes from the fixed 60s to controller-runtime rate-limited backoff (returns the error) — intended, bounded, and events are deduplicated by the recorder, so no event flood.
• Modules synth-ready-contract, patch-helper-mutation, annotation-state-ha were loaded but are not applicable: no status-condition synthesis, no client.Patch(..., MergeFrom(...)), no annotation-based reconcile state in this diff.

Recommended follow-ups

• Verify SeaweedFS's SeaweedFS_s3_bucket_size_bytes behaviour for an existing empty bucket (reports 0 vs drops the series). If it can drop the series, consider distinguishing "query failed -> retain" from "query succeeded, series absent -> age-out/zero" so a genuinely-emptied bucket is not over-billed indefinitely.
• Add a test asserting the BucketMetricsUnavailable Warning Event is actually recorded (e.g. via record.NewFakeRecorder), and cover the non-NotFound namespace-read error branch in resolvePrometheusURL — both are currently unexercised.