fix(mariadb): clean up orphaned operator-generated secrets (#3056) - #3071

Code Review

This pull request adds a test target to the MariaDB package Makefile, introduces cleanup of operator-generated secrets to the post-delete cleanup hook, and adds corresponding unit tests. The review feedback suggests restricting the RBAC permissions granted to the cleanup hook by using resourceNames and limiting the verbs to get and delete for secrets to adhere to the principle of least privilege, and updating the unit tests accordingly.

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/apps/mariadb/templates/hooks/cleanup-pvc.yaml`:
- Around line 42-50: The cleanup hook in the PVC/Secret deletion script is
masking real failures because the `kubectl delete` commands are followed by `||
echo ...`, which still allows the hook to succeed even when RBAC or API errors
occur. Update the cleanup logic in the hook script so `cleanup-pvc.yaml` still
warns on missing resources via `--ignore-not-found` but only treats actual
delete failures as failures; keep the warning messages, and make sure the Job
exits non-zero when PVC or operator-generated Secret deletion fails in the
cleanup block.

Verdict

NOT LGTM

The hook closes two of (likely) three operator-generated Secrets, and the soft-fail pattern hides the failure mode the hook is meant to eliminate — fix both before merging rather than as follow-ups.

Findings

[MAJOR] packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml:42-56 — Incomplete fix: <release>-metrics-config likely orphaned by the same mechanism.

Reading mariadb-operator upstream, <release>-metrics-config is constructed via the same SecretReconciler codepath (api/v1alpha1/mariadb_keys.go: "%s-metrics-config", callsite internal/controller/mariadb_controller_metrics.go:reconcileExporterConfig) as <release>-metrics-password. Upstream sets Owner: mariadb for all three (password, repl-password, metrics-config), yet #3056 empirically observed the OwnerReference path failing for the password Secrets — there is no plausible reason metrics-config would behave differently in a production cluster. The original repro grep -E 'metrics-password|repl-password' was scoped narrowly and would have missed it. Re-run the create→delete leak sweep on a live cluster with kubectl get secret -n <ns> (no grep) and, if <release>-metrics-config survives, extend resourceNames in the Role and the kubectl delete secret line in the hook before this lands. Tightening RBAC and the cleanup Job together is much easier than bolting it on in a second PR after the partial fix is already in customer clusters.

[MAJOR] packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml:50-56 — Soft-fail makes the post-delete hook lie about success.

Both kubectl delete secret invocations end in || echo "WARNING: ... " >&2, and echo always exits 0. Combined with helm.sh/hook-delete-policy: hook-succeeded, the hook Job reports success and is immediately garbage-collected even when the cleanup itself failed (apiserver outage mid-uninstall, RBAC denial on a Secret name not in resourceNames, network blip). The orphan Secrets the PR is supposed to prevent then survive, but with the operator now believing the cleanup ran cleanly — the failure mode is silent. The pre-existing || true on PVCs has the same problem, but extending the pattern to Secrets compounds it. Either drop the suppression and let the Job fail (Helm surfaces post-delete failures, an operator at least gets a signal), or replace it with kubectl get secret <name> -o name && kubectl delete secret <name> per-Secret so "not found" is the only swallowed case.

Operational risks

• The Role pins resourceNames to <release>-metrics-password / <release>-repl-password, matching the post-UPGRADE_25.10.0 mariadb-operator naming. The vendored operator is currently appVersion: 25.10.2. A future bump that returns to (or introduces) different naming silently neutralises the cleanup: the Role no longer permits access to the renamed Secret, the soft-fail above swallows the RBAC error, and orphans return without any signal. Either pre-populate resourceNames with both naming schemes the operator has shipped, or move this check into the packages/system/mariadb-operator bump checklist explicitly.

Recommended follow-ups

• Apply the same audit to sibling operator-backed app charts (packages/apps/postgres, packages/apps/kafka, packages/apps/redis, …) where operators likely generate similar side-effect Secrets that survive helm uninstall.
• If a future maintainer concludes the soft-fail behaviour is correct after all, leave a one-line comment on the || echo WARNING redirect so the next reader does not "improve" it by removing the suppression.

Actionable comments posted: 2

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/apps/mariadb/tests/cleanup_hook_test.yaml`:
- Around line 51-64: The RBAC test in cleanup_hook_test.yaml is too permissive
because the current `contains` assertion on `rules` can still pass if another
broader `secrets` rule is present. Update the check around the `Role` assertion
to either require an exact match for the rendered `rules` entry or add a
negative assertion ensuring no additional `secrets` rules exist beyond the one
with `resourceNames` for `mariadb-test-metrics-password` and
`mariadb-test-repl-password`.
- Around line 27-37: The cleanup test only verifies the final exit in the Job
command, so it can miss regressions where the failure flag is never set. Update
the cleanup hook assertions in cleanup_hook_test.yaml to also match the PVC and
Secret delete failure branches in the Job command generated by the cleanup
logic, using the same command path currently checked, so both delete paths are
asserted to set cleanup_failed before the final exit.

Verification of the prior reviewer's blocker

Claim under test

mariadb-operator generates <release>-metrics-config (ConfigMap/Secret with exporter's my.cnf) as a side-effect of metrics.enabled=true, this PR misses cleaning it up, so it ALSO survives helm uninstall.

Result: claim is false. The Secret exists, but it is owner-referenced to the MariaDB CR and garbage-collected automatically.

Evidence (upstream mariadb-operator on main, paths and code excerpts)

1. The third resource does exist. internal/controller/mariadb_controller_metrics.go reconcileMetrics() calls four sub-reconciles in order: reconcileMetricsPassword, reconcileAuth, reconcileExporterConfig, reconcileExporterDeployment, reconcileExporterService, reconcileServiceMonitor. The third one creates a Secret holding the exporter's [client] user=… password=… (and TLS paths if enabled).

2. The naming pattern is <mariadb-name>-metrics-config (key exporter.cnf), from api/v1alpha1/mariadb_keys.go:

func (m *MariaDB) MetricsConfigSecretKeyRef() GeneratedSecretKeyRef {
    return GeneratedSecretKeyRef{
        SecretKeySelector: SecretKeySelector{
            LocalObjectReference: LocalObjectReference{
                Name: fmt.Sprintf("%s-metrics-config", m.Name),
            },
            Key: "exporter.cnf",
        },
        Generate: true,
    }
}

3. Critical: the code path that creates -metrics-config DOES set a controller reference back to the MariaDB CR. Two cooperating files:

internal/controller/mariadb_controller_metrics.go::reconcileExporterConfig:

secretReq := secret.SecretRequest{
    Owner:    mariadb,
    Metadata: ...,
    Key:      ...,
    Data:     map[string][]byte{ secretKeyRef.Key: buf.Bytes() },
}
return r.SecretReconciler.Reconcile(ctx, &secretReq)

pkg/controller/secret/controller.go::Reconcile:

secret, err := r.Builder.BuildSecret(secretOpts, req.Owner)

pkg/builder/secret_builder.go::BuildSecret:

if owner != nil {
    if err := controllerutil.SetControllerReference(owner, secret, b.scheme); err != nil {
        return nil, fmt.Errorf("error setting controller reference to Secret: %v", err)
    }
}

So -metrics-config is created with metadata.ownerReferences[].controller=true pointing at the MariaDB CR. When helm uninstall deletes the MariaDB CR (which IS Helm-owned via the templates/mariadb.yaml template), Kubernetes garbage collection cascades to -metrics-config. No orphan, no work for the cleanup hook.

4. Compare to the path that DOES orphan (the two Secrets the PR targets): Both -metrics-password and -repl-password go through SecretReconciler.ReconcilePassword, which contains this anti-ownerRef shortcut:

var owner = req.Owner
if req.Generate {
    owner = nil
}
secret, err := r.Builder.BuildSecret(opts, owner)

Both Secrets are created with Generate: true, so the operator deliberately strips the owner before BuildSecret. These are the real orphans, and the PR sweeps exactly them.

Adjacent operator-generated artifacts probed

The set {-metrics-password, -repl-password} is exhaustive given the chart's value flags.

Verdict

LGTM. The PR addresses the complete orphan set (-metrics-password, -repl-password); the -metrics-config Secret is owner-referenced to the MariaDB CR upstream and is cleaned up by Kubernetes garbage collection, so it does not need to be in the hook.