feat(platform): add implementation of backup-strategy controller for etcd - #2641

Code Review

This pull request implements a new Etcd backup strategy driver, enabling automated backups and in-place restores for Cozystack-managed Etcd applications. The implementation includes new API types, controller logic for BackupJob and RestoreJob reconciliation, and comprehensive documentation with end-to-end examples. Feedback was provided regarding the marshalEtcdBackupSnapshot function, which contains unused parameters that should be removed to simplify the signature.

Actionable comments posted: 3

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 `@examples/backups/etcd/cleanup.sh`:
- Line 16: The cleanup currently deletes every Backup in the namespace with
`--all`; narrow it to only the backup created by this scenario by replacing the
`kubectl -n "$NAMESPACE" delete backups.backups.cozystack.io --all
--ignore-not-found` call with a delete that references the specific backup
resource name (e.g. `"$BACKUP_NAME"` or the variable you used when creating the
demo backup), e.g. `kubectl -n "$NAMESPACE" delete backups.backups.cozystack.io
"$BACKUP_NAME" --ignore-not-found`; ensure the script sets or derives
BACKUP_NAME consistently from the creation step (or extracts it from the created
resource) so unrelated tenant backups are not removed.

In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 116-128: The error messages in validateEtcdApplicationRef start
with a capitalized "Etcd" which violates Go error string conventions; update the
two fmt.Errorf calls in validateEtcdApplicationRef to use lowercase "etcd" (and
keep the rest of the message intact), referencing etcdAppKind in the first
message and etcdapp.GroupName in the second so the messages read e.g. "etcd
strategy supports applicationRef.kind=%q, got %q" and "etcd strategy supports
applicationRef.apiGroup=%q, got %q".

In `@internal/backupcontroller/restorejob_controller.go`:
- Around line 121-122: cleanupOnDelete currently treats
strategyv1alpha1.EtcdStrategyKind as unknown and falls back to Velero cleanup;
update the cleanupOnDelete logic (the switch/conditional that decides no-op
strategies) to include strategyv1alpha1.EtcdStrategyKind so Etcd RestoreJobs are
treated as no-op and do not trigger the Velero fallback cleanup. Ensure this
aligns with the reconcileEtcdRestore flow for restoreJob and backup so delete
handling produces no-op/no extra Velero delete calls or logs.

---

Nitpick comments:
In `@api/backups/strategy/v1alpha1/etcd_types.go`:
- Around line 142-148: The custom EtcdLocalObjectReference type duplicates the
existing Kubernetes reference shape; replace or alias it to reuse a standard
type (e.g., corev1.LocalObjectReference or corev1.TypedLocalObjectReference with
empty APIGroup/Kind) to follow the established pattern: update
EtcdLocalObjectReference usages and the struct definition to use the chosen
Kubernetes type (or make EtcdLocalObjectReference a type alias), remove the
custom Name-only struct, and adjust imports to corev1 so callers (marshalling,
validation tags) continue to work without introducing a new reference type.

In `@internal/backupcontroller/etcdapp/types.go`:
- Around line 39-44: Replace the explicit AddKnownTypeWithName registrations in
addKnownTypes with the common AddKnownTypes pattern to match etcdtypes: call
scheme.AddKnownTypes(GroupVersion, &Etcd{}, &EtcdList{}) (keep
metav1.AddToGroupVersion(scheme, GroupVersion) intact). This aligns the
registration of GroupVersion/Kind for Etcd and EtcdList with the etcdtypes
convention and removes the explicit use of
GroupVersion.WithKind(Kind)/WithKind(ListKind).

In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 639-662: The code stores the entire EtcdCluster spec JSON into the
Condition Message (apimeta.SetStatusCondition with
etcdRestoreCondClusterSpecCaptured), which can exceed etcd per-value size
limits; instead, detect payload size after json.Marshal(liveSpec) and if it
exceeds a safe threshold (e.g., a few KB) do not put the full JSON into the
Condition.Message — persist the full payload in a dedicated place (a new
RestoreJob status field like RestoreJob.Status.CapturedSpecRef or create a
ConfigMap and set RestoreJob.Status.CapturedSpecRef to its name) and set the
Condition.Message to a short summary or the reference; update the status update
call (r.Status().Update) to include the new status field and ensure consumers
read the spec from the referenced location rather than decoding
Condition.Message.

Actionable comments posted: 4

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 `@examples/backups/etcd/02-create-bucket.sh`:
- Around line 29-31: The kubectl wait calls for the controller-created resources
can race because the resources may not exist yet; before each wait (for
"bucket-${BUCKET_NAME}", "bucket-${BUCKET_NAME}" BucketClaim on
bucketclaims.objectstorage.k8s.io, and "bucket-${BUCKET_NAME}-backup"
BucketAccess on bucketaccesses.objectstorage.k8s.io) add a short existence poll
like `until kubectl get <resource> <name> -n "$NAMESPACE" > /dev/null; do sleep
1; done` to ensure the resource exists first, then run the existing `kubectl -n
"$NAMESPACE" wait ... --for=... --timeout=300s` commands to avoid intermittent
NotFound failures.

In `@examples/backups/etcd/03-create-etcd-src.sh`:
- Around line 29-33: The readiness waits are racing because they assume the
HelmRelease (variable ETCD_NAME) and EtcdCluster resource exist immediately; add
existence backstops by polling for the resources before calling kubectl wait:
loop (with a total timeout) calling kubectl -n "$NAMESPACE" get hr "$ETCD_NAME"
and kubectl -n "$NAMESPACE" get etcdcluster.etcd.aenix.io etcd (or use their
namespaced get forms) until they succeed, then proceed to the existing kubectl
wait lines; ensure the polling uses the same NAMESPACE and ETCD_NAME variables
and fails with a clear error if the resource never appears before the timeout.

In `@hack/e2e-apps/backup-etcd.bats`:
- Around line 106-110: The waits on controller-created resources (hr
"bucket-${TEST_BUCKET_NAME}", bucketclaims.objectstorage.k8s.io
"bucket-${TEST_BUCKET_NAME}", and bucketaccesses.objectstorage.k8s.io
"bucket-${TEST_BUCKET_NAME}-backup") are race-prone because the resources may
not be created yet; add a short existence backstop before each kubectl wait by
polling for the resource with kubectl get (or kubectl wait
--for=condition=exists via a small loop with timeout) and only then call the
existing kubectl -n "$TEST_NAMESPACE" wait commands to avoid flakes.

In `@packages/extra/etcd/README.md`:
- Around line 57-69: The Parameters table under the README (the block listing
entries like `backup`, `backup.enabled`, `backup.schedule`, etc.) is out of sync
with the generator; run the canonical generator and commit its output: execute
the values/docs generator (use the project’s generator command, e.g., run `make
generate` which invokes `cozyvalues-gen`), let it overwrite the `## Parameters`
block in packages/extra/etcd/README.md, verify the table now matches the
generated output, and commit the regenerated README instead of hand-editing the
table.

---

Duplicate comments:
In `@examples/backups/etcd/cleanup.sh`:
- Line 27: The cleanup script currently deletes every Backup in the namespace by
running `kubectl ... delete backups.backups.cozystack.io --all`, which can
remove unrelated tenant backups; change this to target only the demo backups by
removing `--all` and deleting by explicit name(s) or by selector (e.g., use
`--selector`/`-l` with a demo-specific label or reference the BACKUP_NAME
variable used elsewhere in cleanup.sh) so only the intended Backup resources are
removed; update the `kubectl -n "$NAMESPACE" delete
backups.backups.cozystack.io` invocation accordingly in cleanup.sh.

In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 129-141: The error messages in validateEtcdApplicationRef start
with a capitalized "Etcd" which violates Go error string conventions; update the
fmt.Errorf messages in validateEtcdApplicationRef to start with a lowercase word
(e.g. "etcd strategy supports applicationRef.kind=%q, got %q" and "etcd strategy
supports applicationRef.apiGroup=%q, got %q"), keeping the same use of
etcdAppKind and etcdapp.GroupName to preserve context and formatting.

NOT LGTM

Target branch: main. Branch: backup-etcd. One commit on top of 92930ad1a (FoundationDB driver). PR #2641 — feat(platform): add implementation of backup-strategy controller for etcd.

The driver wiring (BackupJob/RestoreJob dispatch, scheme registration, RBAC, CRD, strategy template + validation, snapshot persistence, the to-copy rejection, the ensureEtcdBackup idempotency contract) is well-modeled on the FoundationDB driver and reads cleanly. But the in-place restore flow has a critical correctness bug that makes the headline feature unusable, plus a related stranded-HR failure mode. Both block.

Blockers

1. Captured EtcdCluster.spec loses every chart-rendered field except bootstrap — restore recreates an EtcdCluster the operator cannot bootstrap

internal/backupcontroller/etcdtypes/types.go:81-93

type EtcdClusterSpec struct {
    // Bootstrap configures initial-cluster initialization from an existing
    // data source.
    Bootstrap *EtcdClusterBootstrap `json:"bootstrap,omitempty"`
}

EtcdClusterSpec declares only the Bootstrap field. The chart-rendered etcd.aenix.io/v1alpha1 EtcdCluster carries replicas, storage.volumeClaimTemplate, security.tls.{peerSecret,peerTrustedCASecret,serverSecret,clientSecret,clientTrustedCASecret}, podTemplate, options, podDisruptionBudgetTemplate (see packages/extra/etcd/templates/etcd-cluster.yaml:15-79). When controller-runtime's typed client deserializes the live CR into this Go type (internal/backupcontroller/etcdstrategy_controller.go:579-580, 654-655), every spec field not declared on EtcdClusterSpec is silently dropped on the client side — standard Go encoding/json behavior, no preserve-unknown-fields semantics carry through runtime.Scheme.

The destructive flow then json.Marshal(live.Spec)s this stripped value into the EtcdClusterSpecCaptured condition (etcdstrategy_controller.go:601) and on recreate (etcdstrategy_controller.go:659-675) builds a new EtcdCluster with Spec: *spec plus the restore source. The result is an EtcdCluster whose entire spec is { bootstrap: { restore: { source: ... } } } — no replicas (operator default 3), no storage (operator default emptyDir per packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml), no TLS Secrets (operator-issued certs from cert-manager never get wired up), no podTemplate.

Consequence:

• The operator boots members on emptyDir (data-losing on pod restart, defeats the restore).
• TLS is unconfigured, so peer-to-peer and client TLS never come up — the cluster never reports Ready.
• Either the restore deadline (line 684, 30m) eventually fires markRestoreJobFailed, or in the unlikely case the operator declares Ready, the HR resume at line 703 re-renders the chart spec onto the cluster and triggers a second disruption.

The example demo (examples/backups/etcd/05-restore-in-place.sh:45-49) will time out at wait_for_field ... phase=Succeeded. The e2e claim in the PR commit message is unsubstantiated against this code.

Fix: capture the live spec as unstructured.Unstructured (or full JSON via the dynamic client) so all chart-rendered fields survive the round-trip. Either:

• swap live := &etcdtypes.EtcdCluster{} for live := &unstructured.Unstructured{} at lines 579 and 654, drive Get/Delete/Create via the dynamic client, and store the unstructured spec map JSON in the condition; the restore path overlays spec.bootstrap.restore.source onto that map, or
• expand etcdtypes.EtcdClusterSpec to mirror every field of the upstream CRD that the chart actually writes (replicas, storage, security, podTemplate, options, podDisruptionBudgetTemplate) and add +kubebuilder:pruning:PreserveUnknownFields plus the runtime-side handling — significantly more code, less robust against upstream CRD additions.

The unstructured approach is the canonical pattern when the controller doesn't own the downstream API; the package docstring at internal/backupcontroller/etcdtypes/types.go:7-13 claims "we avoid pulling the full upstream etcd-operator Go API … while letting us drop unstructured.Unstructured from the controller and tests" and asserts "MergeFrom-style patches preserve unknown fields" — but the destructive flow uses Create, not Patch, so that comment doesn't cover this case. It actively misleads: the comment implies typed Get+Patch is field-preserving, but the captured-spec path uses Get+Marshal+Create, which is field-dropping.

Test that fails without the fix: A unit test that builds a fake client backed by unstructured.Unstructured carrying a chart-shaped EtcdCluster (with spec.replicas=3, spec.storage.volumeClaimTemplate.spec.resources.requests.storage=1Gi, spec.security.tls.peerSecret=etcd-peer-tls), runs the destructive branch of reconcileEtcdRestore to the point where TargetPurged is stamped, then asserts the captured payload on the EtcdClusterSpecCaptured condition round-trips through the recreate path producing a new EtcdCluster whose spec.replicas, spec.storage, and spec.security match the original. With the current typed Spec, the assertion fails because the captured JSON is {} (or {"bootstrap":null}).

2. Failed bootstrap leaves the HelmRelease stranded in spec.suspend=true

internal/backupcontroller/etcdstrategy_controller.go:682-705

The restore path suspends the HR (line 620) and resumes it only after the recreated EtcdCluster reaches Ready (line 703). Every terminal-failure exit between those two points — deadline expiry (line 684), readCapturedEtcdClusterSpec error (line 661), Create non-AlreadyExists error (line 677, also non-terminal so retries; but a permanent admission rejection caused by Blocker 1 above is effectively terminal) — calls markRestoreJobFailed without resuming the HR. The HR sits suspended, helm-controller won't re-render the chart, and the tenant has no way to recover without manual kubectl patch helmrelease/etcd -p '{"spec":{"suspend":false}}'.

The CNPG driver got this right (cnpgstrategy_controller.go:699,703 — paired suspend rollback on every failure path). The etcd driver only pairs the rollback on one branch (line 624's Delete failure path, line 625). Every other failure path past the suspend leaks the suspension.

Fix: add defer-style HR-resume on every failure path past the suspend, or pair the resume with markRestoreJobFailed calls beyond line 620 the same way line 625 does for the Delete error path. The simplest structural fix is a single helper that wraps markRestoreJobFailed with setEtcdRestoreHRSuspended(false) and use it from every failure exit past suspend.

Test that fails without the fix: envtest/fake-client test in which reconcileEtcdRestore proceeds past the suspend step, the deadline elapses (e.g. set restoreJob.Status.StartedAt to >30m ago and restoreTimeoutSeconds=1), and then asserts the HelmRelease's spec.suspend is false after the reconcile that flips the RestoreJob to Failed. Today the assertion fails — spec.suspend remains true.

3. README claim "in-place restore only because the chart pins the release name to etcd" doesn't match resolveEtcdRestoreTarget, which always restores into the source app's namespace

internal/backupcontroller/etcdstrategy_controller.go:873-878

func (r *RestoreJobReconciler) resolveEtcdRestoreTarget(...) etcdRestoreTarget {
    return etcdRestoreTarget{
        Namespace: backup.Namespace,
        AppName:   backup.Spec.ApplicationRef.Name,
    }
}

backup.Namespace is the source Backup's namespace. The to-copy rejection at line 499-509 only rejects targetApplicationRef.Name != backup.Spec.ApplicationRef.Name (different name); a targetApplicationRef with the same name but a different namespace doesn't exist because TypedLocalObjectReference has no namespace field — so OK, restore is always same-namespace as the Backup.

But the public-facing README (examples/backups/etcd/README.md:13-23) and the in-code rejection message (line 501-503) both justify in-place-only by "the chart pins the release name to etcd, so two apps.cozystack.io/Etcd applications cannot coexist in the same namespace". The real reason is simpler and stronger: RestoreJob is namespaced and TargetApplicationRef is a same-namespace reference, so there's no API surface for cross-namespace restore today. The chart pin is a secondary constraint (it forbids two Etcd apps even with a hypothetical cross-namespace API). The docs justify the limitation by the weaker reason and never mention the stronger one, which will confuse future maintainers reading "can I lift the chart pin to enable to-copy?" — the answer is still no, because the core API doesn't expose it.

Fix: update the README's "Restore semantics" section and the rejection-message comment to lead with "RestoreJob.spec.targetApplicationRef is a same-namespace local reference" as the primary constraint and keep the chart pin as a secondary constraint that compounds it. This isn't a code change but it is documentation accuracy in the area the PR introduces.

Test that fails without the fix: none — this is a docs accuracy issue. Per the review checklist, docs in the neighborhood of the PR's changes must reflect reality; this one currently misleads the reader about why the limitation exists. Update the README; no test needed because there is no contract the test could pin (the two constraints both hold).

Recommended (non-blocking)

• internal/backupcontroller/etcdstrategy_controller.go:273-288 — requeueBackupJobWithReason swallows status-update conflicts with return ctrl.Result{RequeueAfter: etcdPollInterval}, nil, but stamps the condition on the in-memory j that the caller passed (already-stale on conflict). On the next requeue the condition timestamps will look right because apimeta.SetStatusCondition is idempotent on Status equality, so this is benign — but mirroring the FDB driver's retry-on-Conflict pattern (foundationdbstrategy_controller.go:requeueWithReason*) would make this consistent across drivers.
• internal/backupcontroller/etcdstrategy_controller.go:302-314 — latestEtcdBackupConditionMessage is correct (the i+1 indexing offsets the Conditions[1:] slice), but it's awkward to read; a plain for _, c := range eb.Status.Conditions[1:] would be clearer.
• internal/backupcontroller/etcdstrategy_controller.go:780-789 — forcePathStyle parsed from driverMetadata with b := v == "true" produces false for any value other than the literal string "true" (including "True", "TRUE", "1"). The backup-time write at line 419 uses fmt.Sprintf("%t", *s.ForcePathStyle) which is always lowercase "true"/"false", so the round-trip is safe — but a future change to that format would silently misparse. Use strconv.ParseBool for symmetry with the encode side.
• internal/backupcontroller/etcdstrategy_controller.go:889-894 — etcdBackupSnapshot.Storage is strategyv1alpha1.EtcdDestinationTemplate; the value type and the field name don't match the JSON tag storage vs the type name EtcdDestinationTemplate. Not a bug, just unusual — every other driver names this field for the persisted shape (BarmanObjectStore for CNPG, Blob for FDB). A name like Destination would be more honest about what's stored.

Test coverage

The test file (etcdstrategy_controller_test.go) covers the validation, templating, snapshot encode/decode, ensure-idempotency, and a couple of terminal paths well — but at 555 lines it's substantially thinner than the FoundationDB driver's 1826-line test file the etcd driver mirrors. Missing coverage:

• No test of the destructive restore branch's spec-capture-and-recreate cycle. This would have caught Blocker 1 immediately.
• No test of HR suspend/resume pairing. This would have caught Blocker 2.
• No test of BackupJob deadline expiry (the FDB driver has TestReconcileFoundationDB_DeadlineExpiry_StopsFoundationDBBackup).
• No test of conflict-with-in-chart-backup semantics (the chart can render an EtcdBackupSchedule when backup.enabled=true — should the driver detect and reject? The FDB driver has TestFindConflictingInChartBackup_IgnoresDriverManagedCRs; the etcd driver has nothing equivalent and the user can drive both a chart-rendered EtcdBackupSchedule and BackupClass-driven EtcdBackups against the same cluster — probably fine because they're different CRDs, but it should be tested or explicitly noted).

Add tests for the two blockers as part of the fix; add one for the deadline path while you're in the area.

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.

Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.

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 `@examples/backups/etcd/05-restore-in-place.sh`:
- Around line 67-70: The timeout branch currently only logs a warning and
breaks, allowing the script to continue even if bootstrap.restore remains
present; change the block that checks elapsed >= 300 to log an error (include
the BOOTSTRAP value for context) and exit with a non-zero status to fail fast
when bootstrap.restore does not converge instead of using break (modify the code
around the elapsed check and the log_warning/ break to use log_error and exit
1).

---

Duplicate comments:
In `@examples/backups/etcd/cleanup.sh`:
- Around line 25-28: The cleanup currently deletes all backups with "kubectl ...
delete backups.backups.cozystack.io --all", which is unsafe; change it to delete
only the demo's backup resource by name (use the existing variable pattern like
BACKUPJOB_NAME — add or use a BACKUP_NAME variable) and call "kubectl -n
\"$NAMESPACE\" delete backups.backups.cozystack.io \"$BACKUP_NAME\"
--ignore-not-found" instead, keeping the surrounding log_substep call and
--ignore-not-found flag so only the scenario's backup is removed.

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 `@examples/backups/etcd/00-helpers.sh`:
- Around line 97-100: Change the current early-return branch that treats any
existing non-ready pod as success: if kubectl -n "$NAMESPACE" get pod
"$ETCDCTL_POD" succeeds but kubectl -n "$NAMESPACE" wait ...
--for=condition=ready times out or fails, delete the stale pod (kubectl -n
"$NAMESPACE" delete pod "$ETCDCTL_POD" --ignore-not-found) and continue to the
pod-creation path instead of returning 0; ensure the creation code then waits
for readiness with kubectl wait and fails the script on persistent timeout. Use
the existing symbols ETCDCTL_POD, NAMESPACE and the kubectl wait/get/delete
commands to locate and implement the change.

---

Duplicate comments:
In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 799-801: When the Create call using
Resource(etcdClusterGVR).Namespace(target.Namespace).Create(...) fails (err !=
nil && !apierrors.IsAlreadyExists(err)), un-suspend the associated HelmRelease
before returning so reconciliation doesn't remain frozen; locate the HelmRelease
object (target) and clear its spec.suspend (or set .Spec.Suspend = false) then
persist the change via the client/patch API (e.g., r.Client.Update or
r.patchHelper) and log the action, then return the original error as before.

NOT LGTM

Reviewing against target branch: main (PR #2641, branch backup-etcd, single feat commit 4f2373562 on top of the merge base).

Scope

The PR adds an Etcd strategy driver to the existing backupstrategy-controller:

• api/backups/strategy/v1alpha1/etcd_types.go — Etcd strategy CR with S3 and PVC destination shapes.
• internal/backupcontroller/etcdstrategy_controller.go (+ test file) — BackupJob and RestoreJob handlers, in-place-only restore via HR-suspend / delete-EtcdCluster / recreate-with-bootstrap.restore / HR-resume.
• internal/backupcontroller/etcdtypes, internal/backupcontroller/etcdapp — typed shims for etcd.aenix.io/EtcdCluster|EtcdBackup and apps.cozystack.io/Etcd.
• Dispatch in backupjob_controller.go / restorejob_controller.go + cleanupOnDelete no-op branch.
• packages/extra/etcd/templates/etcd-cluster.yaml — adds security.tls.serverTrustedCASecret: etcd-ca-tls.
• packages/extra/etcd/values.yaml + README — marks the legacy backup.* block deprecated.
• packages/system/etcd-operator bumped to v0.4.4 (chart + CRD adds EtcdBackup.status.snapshot, operator RBAC adds pods + pods/log).
• New CRD packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml + RBAC additions for etcdclusters/etcdbackups.
• examples/backups/etcd/ walkthrough + hack/e2e-apps/backup-etcd.bats happy-path e2e.

Tests pass locally (go test ./internal/backupcontroller/...), build is clean (go build ./...), vet is clean.

Blocking issues

1. PVC restore is broken at every code path — the strategy CR accepts PVC but no PVC restore can succeed

internal/backupcontroller/etcdstrategy_controller.go:1055-1062 (and :992-1000) — for PVC destinations, the driver passes pvc.SubPath through verbatim from the snapshot to EtcdCluster.spec.bootstrap.restore.source.pvc. Upstream etcd-operator interprets the same SubPath ASYMMETRICALLY between backup and restore:

• Backup side (aenix-io/etcd-operator/internal/controller/factory/backup_job.go:217-219): when SubPath set, writes to /backup/data/<SubPath>/<backupName>.db; when empty, writes to /backup/data/<backupName>.db. The agent additionally injects BACKUP_INCLUDE_REVISION / BACKUP_TIMESTAMP suffixes (cmd/backup-agent/main.go:81,250-253).
• Restore side (aenix-io/etcd-operator/internal/controller/factory/statefulset.go:520-527): when SubPath set, reads from /backup/data/<SubPath> AS A FILE; when empty, reads /backup/data/snapshot.db.

So a tenant who configures pvc.subPath: "daily":

• backup writes to /backup/data/daily/<backupName>.db,
• restore opens /backup/data/daily and fails — it's a directory, not a file.

And with empty SubPath:

• backup writes to /backup/data/<backupName>.db,
• restore opens /backup/data/snapshot.db and 404s.

The S3 path was carefully reconstructed via buildEtcdRestoreS3Key (and s3KeyFromArtifactURI for rev/timestamp suffixes). The PVC path has no equivalent and ignores Backup.status.artifact.URI entirely, even though the upstream snapshot URI documentation explicitly defines file:///<abs-path> as the FULL final on-PVC path.

Fix (preferred — narrow the API contract): reject PVC in the strategy CR until restore is implemented end-to-end. Drop EtcdPVCTemplate from EtcdDestinationTemplate in api/backups/strategy/v1alpha1/etcd_types.go, tighten the XValidation to has(self.s3), drop the PVC branches from validateRenderedEtcdDestination, strategyToEtcdBackupDestination, etcdBackupDestinationToUnstructured, and resolveEtcdRestoreDestination. Update the CRD YAML accordingly. Document the choice in packages/extra/etcd/README.md (the new section already lists supported destinations).

Fix (alternative — implement PVC restore): reconstruct the PVC restore path from Backup.status.artifact.URI (parse file:///<abs-path>, strip the leading /backup/data/ mount prefix, set the result as pvc.subPath on the restore source). On a fallback where the artifact URI is unavailable, compute <backupName>.db (no SubPath) or <SubPath>/<backupName>.db (with SubPath) — this still won't survive BACKUP_INCLUDE_REVISION / BACKUP_TIMESTAMP, so the artifact-URI path needs to be the primary.

Test that fails without the fix: a TestResolveEtcdRestoreDestination_PVCRestoreReconstructsPath modeled on the existing TestResolveEtcdRestoreDestination_PrefersArtifactURIForRevSuffix: snapshot has pvc: { claimName: "daily-backups", subPath: "daily" }, backup has driverMetadata[etcdBackupNameKey] = "etcd-backup-job-xyz", optionally Status.Artifact.URI = "file:///backup/data/daily/etcd-backup-job-xyz-rev2.db". Assertion: the returned destination's PVC.SubPath MUST be the full filename relative to the mount ("daily/etcd-backup-job-xyz-rev2.db" from URI, or "daily/etcd-backup-job-xyz.db" from reconstruction). The current code returns just "daily", which the upstream restore-agent treats as a directory and fails to open.

If the fix is "remove PVC", the test becomes a TestEtcdStrategyCRDRejectsPVC against the kubebuilder XValidation: applying a strategy with destination.pvc set must be rejected by the apiserver.

2. Backup-without-snapshot fallback emits PVC restore that points at the wrong file even with SubPath empty

Same area as #1, but worth flagging independently: the empty-SubPath case is broken too (backup writes <backupName>.db, restore reads snapshot.db). This is in scope for whichever fix above is chosen.

internal/backupcontroller/etcdstrategy_controller.go:1055-1062. Same fix and test as #1.

Non-blocking observations

• internal/backupcontroller/etcdstrategy_controller.go:127-141 — validateEtcdApplicationRef allows APIGroup == "", relying on the dispatcher / chart conventions to fill in apps.cozystack.io. Consistent with the FDB driver pattern (verified in tests at etcdstrategy_controller_test.go:76-79). Not a problem; flagging for awareness.
• internal/template/template.go:23-26 — templateFunc swallows template execution errors and returns the unrendered input. A typo in a tenant template like {{ .Application.metadata.nme }} would yield the literal string {{ .Application.metadata.nme }} as the bucket name and pass validateRenderedEtcdDestination (non-empty). Not introduced by this PR — pre-existing helper used by every strategy driver — but worth noting that this PR widens its surface area.
• examples/backups/etcd/05-restore-in-place.sh:51-73 and the README explicitly tolerate Helm-controller lagging on evicting the stamped spec.bootstrap.restore block. That's a known fragility but well-flagged in the example. Fine.

Documentation

• packages/extra/etcd/README.md — DEPRECATED markers on the backup.* block are accurate and the new "Restore and ad-hoc backup" section correctly describes the in-place flow.
• examples/backups/etcd/README.md and the scenario .md files — read consistent with the controller code; admin/user roles match what the RBAC supports.
• The upgrade note about serverTrustedCASecret is accurate against packages/extra/etcd/templates/etcd-cluster.yaml:46.
• No stale doc drift detected.

Test plan recap (what needs to be added)

• TestResolveEtcdRestoreDestination_PVCRestoreReconstructsPath (or its admission-rejection equivalent) covering the SubPath-set and SubPath-empty cases, plus the artifact-URI-prefers path if PVC stays in the API.

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.

Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.

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 `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 799-801: The create call for the EtcdCluster (the
Resource(...).Namespace(...).Create(...) block) currently returns a raw
reconcile error on non-AlreadyExists failures causing infinite retries; instead,
when that create fails due to admission/rejection (err != nil &&
!apierrors.IsAlreadyExists(err)), mark the corresponding RestoreJob status as
Failed (set its Status.Phase/Reason/Message accordingly and persist it via the
controller client or status updater), ensure the suspended HelmRelease is
resumed (set HelmRelease.Spec.Suspend = false and update it), emit an event for
the failure, and return ctrl.Result{} with nil error so the controller does not
keep retrying the create loop; update the code paths around the
Resource(...).Create call to perform these status and HelmRelease updates rather
than returning the raw error.

In
`@packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml`:
- Around line 65-140: The checked-in CRD (fields around Destination/s3,
validation message under x-kubernetes-validations and the s3 properties like
bucket, credentialsSecretRef, endpoint) is out of sync with the canonical
generator output; run the project CRD generation task (the repository's
generation script/build task that produces
strategy.backups.cozystack.io_etcds.yaml), regenerate the CRD artifact so the
Destination description and validation message formatting match the source, and
commit the regenerated YAML to the repo.

---

Duplicate comments:
In `@examples/backups/etcd/cleanup.sh`:
- Line 30: The cleanup currently deletes all Backup CRs; change it to only
delete the demo backup by sourcing the `.backup-name.env` file (which defines a
variable like BACKUP_NAME) and then call kubectl to delete that single backup
(use the BACKUP_NAME variable) instead of `--all`; update the command that
references NAMESPACE and the Backup resource (backups.backups.cozystack.io) to
use "$BACKUP_NAME" and keep `--ignore-not-found` for safety.

In `@hack/e2e-apps/backup-etcd.bats`:
- Around line 106-110: The kubectl wait calls for controller-created resources
(e.g. hr "bucket-${TEST_BUCKET_NAME}", bucketclaims.objectstorage.k8s.io
"bucket-${TEST_BUCKET_NAME}", and bucketaccesses.objectstorage.k8s.io
"bucket-${TEST_BUCKET_NAME}-backup") can race if the resource does not yet
exist; before each kubectl wait invocation (including the similar block around
lines 161–164), poll for existence (e.g. loop/kubectl get until success or
timeout) for the specific resource name and kind, then invoke kubectl wait to
assert readiness; update the test to add these lightweight existence checks
immediately prior to each kubectl wait for those three resources.

In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Line 131: The error messages in validateEtcdApplicationRef use capitalized
"Etcd" which violates Go error-string style; update the two fmt.Errorf calls
that start with "Etcd strategy supports applicationRef..." (and the related
message for apiGroup) to begin with a lowercase "etcd" and keep the rest of the
message unchanged so lint ST1005 is satisfied (i.e., change "Etcd strategy
supports applicationRef.kind=..." and the apiGroup variant to "etcd strategy
supports applicationRef.kind=..." etc.).

In `@packages/extra/etcd/README.md`:
- Around line 70-82: The README's Parameters table for the Etcd package is out
of date (the `backup` fields such as `backup`, `backup.enabled`,
`backup.schedule`, `backup.destinationPath`, etc. were hand-edited) and must be
regenerated to match the canonical output from the generator; run the repository
generator to rewrite the `## Parameters` section (e.g. run make generate which
invokes cozyvalues-gen), verify the generated `## Parameters` block now contains
the correct `backup` entries, and commit the regenerated README rather than
manually editing those rows.

NOT LGTM

Scope reviewed

Target branch: main. One commit on the branch (bb06b3d64 feat(platform): add implementation of backup-strategy controller for etcd), ~4800 lines. Merge base ee9d1544e. Build clean (go build ./internal/backupcontroller/...), go vet clean, go test ./internal/backupcontroller/... passes. The Go logic, the new CRD, RBAC, and the vendored etcd-operator chart bump are all well-structured and heavily tested. One blocker, one dev artifact to clean up in this pass, and two lower-severity issues below.

Clean up in this pass (dev artifact, not a true blocker)

1. Production controller image points at an ephemeral ttl.sh dev registry

packages/system/backupstrategy-controller/values.yaml:2

image: "ttl.sh/f6960d6a-0c9d-4d93-808e-88660b3b0f02/backupstrategy-controller:latest@sha256:067cae6848f61f171ebd484ec47b761f50fd03480d99bd9c51fd5812d49ada53"

On main this value is ghcr.io/cozystack/cozystack/backupstrategy-controller:v1.4.0-rc.2@sha256:dd5533.... The branch swaps the production image for a ttl.sh anonymous build. ttl.sh images are anonymous and expire (default TTL is hours, max ~1 week), so any cluster that deploys this chart after the TTL lapses gets ImagePullBackOff on the backup controller and all backup/restore reconciliation stops. This is a dev artifact left in from local iteration.

This image: line is normally written by the make image-backupstrategy-controller target (see packages/system/backupstrategy-controller/Makefile — the target ends with yq -i '.backupStrategyController.image = strenv(IMAGE)'), so the ttl.sh value came from running that target against a personal registry.

This is not strictly a merge blocker — the release pipeline rewrites backupStrategyController.image with a real published digest, so the ttl.sh value won't reach a release artifact. But since this PR needs another revision pass anyway, the dev value should not be left in the branch.

Fix: revert backupStrategyController.image to a real published ghcr.io/cozystack/... digest produced by the project's image-build/release pipeline (do not hand-type a digest — let the build target/CI write it).

Test that proves it: a release/preflight check that asserts every */values.yaml controller image: references an allowed registry prefix (ghcr.io/cozystack/) and rejects ttl.sh/docker.io/<personal>. If such a guard does not exist, the right place is the existing release/manifest CI step, not a source-grepping unit test. At minimum, a reviewer-runnable check: grep -R "ttl.sh" packages/*/*/values.yaml must return nothing.

Blockers

2. Published CRD description advertises a "to-copy" restore the driver rejects

api/backups/strategy/v1alpha1/etcd_types.go:36-39 (propagates verbatim into the generated CRD packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml:29 as the user-facing openAPIV3Schema.description).

The type doc comment (and therefore the CRD a tenant sees via kubectl explain etcd.strategy.backups.cozystack.io) says restores happen:

"either by recreating the source EtcdCluster (in-place) or by patching a freshly provisioned target EtcdCluster before the operator marks it Ready (to-copy)."

But reconcileEtcdRestore (internal/backupcontroller/etcdstrategy_controller.go:573-592) explicitly and terminally rejects every to-copy RestoreJob ("Etcd driver does not support to-copy restore..."). The implementation supports in-place only; the to-copy branch described in the doc does not exist. This is a documentation-accuracy blocker per the review contract: the user-facing API description promises a capability the code refuses. A tenant reading the CRD will author a targetApplicationRef to-copy restore and get a hard failure that contradicts the documented contract.

Note the same misleading "to-copy" phrasing also appears in the etcdtypes.EtcdClusterSpec.Bootstrap doc comment at internal/backupcontroller/etcdtypes/types.go:96 ("to-copy: patch a freshly-rendered EtcdCluster before it goes Ready") — that one is internal-only so lower stakes, but it describes the same nonexistent path and should be corrected in the same pass for consistency.

Fix: rewrite the Etcd type doc comment so it describes only the implemented behavior (in-place: suspend HR, capture live spec, delete + recreate EtcdCluster with bootstrap.restore.source, resume HR), and state that to-copy is not supported and why (same-namespace TargetApplicationRef + chart-pinned release name etcd). Regenerate the CRD via the project codegen (make generate / hack/update-codegen.sh) rather than hand-editing the generated YAML, so the description in definitions/strategy.backups.cozystack.io_etcds.yaml is rederived from the corrected source.

Test that proves it: this is a doc/codegen contract, not a runtime path — the correct guard is the existing codegen drift check (CI runs make generate and fails on a dirty tree). After fixing the comment and regenerating, a clean git diff on the CRD confirms source and generated artifact agree. Do not add a source-grepping unit test for the string. The runtime rejection itself is already covered (TestReconcileEtcdRestore_RejectsToCopy, TestReconcileEtcdRestore_RejectsForeignTargetAPIGroup).

Lower severity (recommended, not blocking)

3. BackupJob can spin in Running forever if the EtcdCluster never becomes Ready

internal/backupcontroller/etcdstrategy_controller.go:211-221

j.Status.StartedAt is stamped on the first reconcile, and the etcdDefaultBackupDeadline (20m) gate is only consulted in the default branch of the EtcdBackup phase switch (line 286) — which is reached only after the EtcdCluster is Ready and an EtcdBackup exists. While the cluster is NotFound or not-Ready, the code returns via requeueBackupJobWithReason (lines 213, 219), which never checks the deadline. A BackupJob against a cluster that never becomes Ready (broken etcd, deleted app, stuck PVC) stays Running indefinitely and never terminates.

This is called out as recommended rather than blocking only because the merged FoundationDB driver has the identical behavior (internal/backupcontroller/foundationdbstrategy_controller.go:188-197 requeues with no deadline while waiting for the cluster to be available) — so the etcd driver is consistent with the established sibling pattern, not introducing a new divergence. If the maintainers want to fix it, fix both drivers together: have the wait-for-Ready requeue path consult etcdDefaultBackupDeadline (or a dedicated wait-for-ready deadline) and flip the BackupJob Failed once exceeded.

Test that would prove the gap (and the fix): a reconcileEtcd unit test that seeds a BackupJob with Status.StartedAt set to now - 21m and an EtcdCluster that is absent (or has Ready=False), then asserts the reconcile returns a terminal markBackupJobFailed instead of another RequeueAfter. Today that test would fail (it requeues forever); with the deadline wired into the wait path it passes.

4. forcePathStyle default semantics may surprise on the artifact-less restore path

internal/backupcontroller/etcdstrategy_controller.go:1065-1068

On the driverMetadata fallback, forcePathStyle is reconstructed only if the etcd.aenix.io/force-path-style key is present in DriverMetadata. At backup time the key is written only when s.ForcePathStyle != nil (lines 478-480). So a backup taken with forcePathStyle unset renders a restore destination with forcePathStyle nil — which is correct round-tripping. This is fine as-is; flagging only to confirm it was checked: the round-trip is faithful (unset stays unset). No change required, but worth a one-line test asserting the nil round-trips through resolveEtcdRestoreDestination so a future refactor that defaults it to true (matching the chart's backup.forcePathStyle: true legacy default) doesn't silently diverge.

Things verified clean (no action needed)

• Vendored etcd-operator chart bump (v0.4.3 → v0.4.4): Chart.yaml, crds/etcd-backup.yaml, and templates/rbac/clusterrole-manager-role.yml under packages/system/etcd-operator/charts/etcd-operator/ are byte-identical to a fresh helm pull oci://ghcr.io/aenix-io/charts/etcd-operator --version 0.4.4. The make update target would reproduce them exactly — no hand-edit drift.
• No leaked secrets. The examples/backups/etcd/ scripts derive S3 creds from a Bucket CR (secretS3.accessKey) and env vars, never hardcode them; generated .bucket-info.env / .sentinel.env / .backup-name.env are chmod 600 and added to .gitignore.
• PVC-gone gate label is correct. etcdClusterFullyGone selects PVCs by app.kubernetes.io/instance=etcd, which matches the operator labels the chart itself relies on (packages/extra/etcd/templates/etcd-cluster.yaml:273-276 WorkloadMonitor selector).
• README / values tables consistent. packages/extra/etcd/README.md Backup parameters table matches the DEPRECATED descriptions in values.yaml and values.schema.json; the etcd-rd/cozyrds/etcd.yaml embedded openAPISchema was regenerated to match.
• Crash-recovery restore state machine (spec-capture condition, TargetPurged, oversized/empty-spec fast-fail before destructive purge, HR resume on terminal failure) is thoroughly unit-tested (TestReconcileEtcdRestore_*).

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.

Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.

LGTM

Reviewed branch backup-etcd (PR #2641) against target main (merge-base ee9d1544e). Single feature commit adding an Etcd backup-strategy driver to the backupstrategy-controller, plus chart deprecation of the legacy backup.* flow, an etcd-operator bump v0.4.3 -> v0.4.5, and an end-to-end example. ~5000 lines, the bulk of it tests and a regenerated operator CRD.

What I verified

• go build ./... clean; go vet ./internal/backupcontroller/... clean.
• go test ./internal/backupcontroller/... passes, including with -race. Test coverage is genuinely thorough (1860 lines): validation, templating, snapshot round-trip + unrecognised-schema gate, S3 key reconstruction vs artifact-URI preference (including the dev7 -rev2 suffix bug), idempotent EtcdBackup creation, no-owner-ref contract, to-copy + foreign-APIGroup rejection, BackupJob/RestoreJob deadline paths, crash-recovery state-machine branches, oversized/empty captured-spec fail-fast-before-purge, and HR-resume-on-terminal-failure.
• HelmRelease naming: the driver uses hrName := target.AppName with no prefix. Confirmed correct against packages/system/etcd-rd/cozyrds/etcd.yaml which sets release.prefix: "" (unlike CNPG, which has a prefix and correctly adds one). The example also waits on hr "etcd" = app name.
• RBAC (packages/system/backupstrategy-controller/templates/rbac.yaml): adds etcd.aenix.io/etcdclusters get/list/watch/create/delete and etcdbackups get/list/watch/create. PVC list (used by etcdClusterFullyGone) and helmreleases get/update (suspend/resume) are already granted by the shared rules. The deliberate omission of cluster-scoped Secret read is consistent with the controller comment and avoids the stuck-informer failure mode.
• Operator bump is internally consistent: Chart.yaml appVersion/version both -> v0.4.5; deployment image tag defaults to .Chart.AppVersion; the chart is vendored via helm pull (Makefile), and the regenerated CRDs (controller-gen v0.16.5) add EtcdBackup.status.snapshot and EtcdCluster.spec.bootstrap.restore.source + security.tls.serverTrustedCASecret, all of which the driver depends on.
• Generated strategy CRD (packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml) matches the Go types: has(self.s3) XValidation, MinLength constraints, Cluster scope.
• No leaked secrets. The example reads S3 creds from the in-cluster BucketInfo Secret at runtime and writes them to .bucket-info.env, which .gitignore now covers (examples/backups/*/.bucket-info.env, .sentinel.env, .backup-name.env). No untracked secret files present.
• Documentation accuracy: packages/extra/etcd/README.md, values.yaml, values.schema.json, and the etcd-rd openAPISchema all consistently mark backup.* DEPRECATED with matching wording; the values table is the generated form (header widths regenerated). README's description of the in-place restore flow matches the controller implementation and the example.

Recommended to fix (cosmetic, non-blocking)

• internal/backupcontroller/etcdstrategy_controller.go:580-582 — a stray double blank line between createEtcdBackupArtifact and the RestoreJob-path comment banner. gofmt -d flags it (collapse to one blank line). The repo's pre-commit hook runs make generate rather than gofmt, so this is not a hard CI gate here, but it is non-canonical formatting. Fix: gofmt -w internal/backupcontroller/etcdstrategy_controller.go. (Test: not applicable — gofmt is the check; gofmt -l returning empty is the proof.)

Notes (not issues, for awareness)

• The post-restore behavior where helm-controller re-renders a bootstrap-less EtcdCluster and evicts the driver-stamped spec.bootstrap.restore block is intentional and safe: the operator only consults bootstrap on first reconcile, so eviction after the cluster is Ready causes no re-bootstrap. The example (05-restore-in-place.sh) explicitly waits for this eviction to avoid a Helm/operator diff war on a later unrelated sync. Worth keeping in mind operationally.
• TestEtcdDestinationTemplate_NoPVCField uses reflect on the struct type (not source-file grepping) to assert the PVC field stays absent — this is a legitimate compile-time/type contract test, not a brittle text-matching drift guard.
• The restore deadline (effectiveRestoreDeadline) is measured from RestoreJob.Status.StartedAt, which is stamped at the very start, so the clock includes the purge/PVC-drain window, not just the bootstrap window. This is documented and intentional (30m default, overridable via spec.options.restoreTimeoutSeconds).

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.

Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.