Repository navigation
Send the firewall export to the account that exists, and bound what it keeps (#92) - #250
Merged
Merged
Conversation
… bound what it keeps (#92) The off-host copy shipped defaulting to `robo@10.0.99.30`. The login on oracle is `atropos`, and there is no /etc/default/homelab-timers on this host to override it, so every nightly run would have failed on `Permission denied` — local export written and verified, copy step dead, job correctly reporting a failure nobody would have guessed the cause of from the message. It was invisible for a day only because the checkout the timer runs from was behind the commit that added the copy. A default nobody has executed is a guess. Retention comes with the fix rather than after it, because until now both sides kept every export ever taken, nightly, forever. FW_KEEP (default 30 — a month at one a night, long enough to reach past a bad rule nobody noticed for a fortnight) bounds each side. It never evicts the newest, never touches a file this script did not write, and clears the .part fragments a died copy leaves on oracle — which list_remote's filter had made invisible. FW_KEEP and not KEEP: all four units read the same EnvironmentFile and backup-volumes.sh already owns KEEP there, where it governs gigabyte volume sets. Order is prune_local, copy, prune_offhost, and both boundaries are forced. Pruning the far side first would have the same run re-upload what it deleted, nightly, forever. Copying before the local prune would transfer and verify bytes the far-side prune drops seconds later. Local retention hangs off the local write, not the job, so an unreachable oracle cannot quietly restore the unbounded growth this fixes. oracle's login shell is zsh, where an unmatched glob is fatal rather than the literal bash passes through: `rm -f "$dir"/*.part` on an empty directory exits non-zero, and a non-zero remote command fails the job — a clean night reported as ScheduledJobFailed. So nothing sent over the wire contains a glob at all. The prune cds in and deletes explicit basenames, each one a string the far side just reported and that was re-validated here against a character class with no quote, no metacharacter and no leading dash. FW_OFFHOST's directory is now validated too, because it is read from an operator-edited file and this is the first code path that hands it to rm. Also: list_backups sorts by name, not mtime. The stamp is UTC ISO-8601, so the order is identical in the normal case and correct when an mtime has been touched — and it makes both sides the same function of the same strings, which is what stops them disagreeing about which are the newest FW_KEEP. Exercised against a zsh shim with fixtures for each case — stray files, live and dead .part fragments, an empty far side, an unreachable one, malformed FW_OFFHOST and FW_KEEP — then end to end against the real morpheus and oracle, including the remote prune on a scratch directory there, and through the unit's own ExecStart: exit 0, homelab_job_last_exit_code 0. Three prose claims said this script was "non-destructive throughout" (Makefile, restore-volumes.sh, and its own header). Retention makes that false, so they now say what still holds: it never writes to the thing it backs up and never deletes outside its own retention window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 tasks
There was a problem hiding this comment.
🟡 Changes recommended
The new retention logic has a confirmed prune/dry-run inconsistency for stray filenames and the off-host dir validation permits leading - which can break remote coreutils calls via option parsing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the firewall backup workflow to copy exports to the correct off-host account and adds bounded retention so firewall exports don’t grow unbounded on either host, with documentation updates to match the new behavior.
Changes:
- Fix default off-host destination to use
atropos@10.0.99.30and addFW_KEEPretention (default 30) for local + off-host exports. - Implement retention pruning (including stale
.partfragments) and add--list(dry-run style) and--prunemodes. - Update Makefile help text and runbooks/roadmap documentation to reflect the corrected destination and retention behavior.
File summaries
| File | Description |
|---|---|
| scripts/restore-volumes.sh | Updates commentary to reflect backup-firewall’s “non-destructive to source + bounded retention” model. |
| scripts/backup-firewall.sh | Fixes default off-host user, adds FW_KEEP validation, implements local+remote retention pruning and .part cleanup, adds --prune. |
| Makefile | Documents firewall backup retention knobs/behavior and aligns restore comments with the updated model. |
| docs/runbooks/schedule-maintenance.md | Notes that firewall backups both fail loudly on copy failure and now have bounded retention (FW_KEEP). |
| docs/runbooks/restore-the-firewall.md | Updates off-host user, documents FW_KEEP retention and the new list/prune behaviors. |
| docs/roadmap.md | Records the default-user copy bug and documents the retention rationale and zsh/glob constraints. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+399
to
+406
| local i=0 | ||
| for name in "${have[@]}"; do | ||
| [[ -n $name ]] || continue | ||
| i=$((i + 1)) | ||
| ((i > FW_KEEP)) || continue | ||
| [[ $name == "$newest_name" ]] && continue | ||
| if is_backup_name "$name"; then victims+=("$name"); else strays+=("$name"); fi | ||
| done |
Comment on lines
+203
to
+206
| [[ $OFFHOST_DIR =~ ^[A-Za-z0-9._/-]+$ ]] \ | ||
| || { red "FW_OFFHOST directory may only contain letters, digits, . _ - and /, got '$OFFHOST_DIR'"; exit 1; } | ||
| [[ $OFFHOST_DIR != "/" && $OFFHOST_DIR != *..* ]] \ | ||
| || { red "FW_OFFHOST directory may not be / or contain '..', got '$OFFHOST_DIR'"; exit 1; } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The off-host copy added yesterday shipped defaulting to
robo@10.0.99.30. The login onoracleisatropos:There is no
/etc/default/homelab-timerson this host to override it, so every nightly run would have failed on its copy step — local export written and verified, copy dead,ScheduledJobFailedwithin the hour for a reason the message does not name. It stayed invisible for a day only because the checkout the timer runs from was behind the commit that added the copy.Retention lands with the fix rather than after it, because until now both sides kept every export ever taken, nightly, forever.
Why these choices
FW_KEEP, default 30, notKEEP. All four units read the sameEnvironmentFile, andbackup-volumes.shalready ownsKEEPthere — one line meant for nightly exports would otherwise reset retention on gigabyte volume sets. 30 is a month at one a night:backup-volumes.sh's 7 is a longer window because that job is weekly.Order is
prune_local→ copy →prune_offhost, and both boundaries are forced. Pruning the far side first would have the same run re-upload what it deleted, nightly, forever. Copying before the local prune would transfer and verify bytes the far-side prune drops seconds later. Local retention hangs off the local write, not the job, so an unreachableoraclecannot quietly restore the unbounded growth this fixes.No glob is sent to
oracle. Its login shell is zsh, where an unmatched glob is fatal rather than the literal bash passes through —rm -f "$dir"/*.parton an empty directory exits non-zero, and a non-zero remote command fails the job, reporting a clean night asScheduledJobFailed. The prunecds in and deletes explicit basenames, each a string the far side just reported and re-validated here.FW_OFFHOST's directory is validated too: it comes from an operator-edited file and this is the first path that hands it torm.list_backupssorts by name, not mtime. Same order normally, correct when an mtime has been touched — and it makes both sides the same function of the same strings, which is what stops them disagreeing about which are the newestFW_KEEP.Stale
.partfragments were invisible to every path (list_remote's filter requires a.sops.yamlending) and accumulated forever. They are now cleared on a state predicate rather than an age guard, which cannot race a live transfer.Blast radius
oraclestill holds ciphertext only; the age key stays onprometheus. The ssh key that could already overwrite there can now also delete there — the same trust in the same key, now stated in the header.This adds deletion to a backup script. Mitigations: never the newest, never a file the script did not write, never an unvalidated
rmtarget, andARGS=--listis a dry run that marks exactly what the next run removes, computed from the same partition the prune uses.Verification
make validateand./scripts/lint.shpass.Exercised against a zsh shim with fixtures for each case — stray files, live and dead
.partfragments, an empty far side, an unreachable one, malformedFW_OFFHOSTandFW_KEEP, idempotency, and one-run convergence from full divergence.Then against the real hosts:
make backup-firewall— export frommorpheusverified (config 24.6, 88 rules), copied tooracle, byte-identical, exit 0make backup-firewall ARGS=--verify-only— decrypts here, identical thereoracle: newest three kept, both dead fragments cleared, a strayREADMEuntouched, rerun a clean no-opExecStartthroughrun-scheduled.sh: exit 0,homelab_job_last_exit_code{homelab_job="backup-firewall"} 0sudo systemctl start homelab-backup-firewall.serviceneeds a password this session could not supply, so the unit was exercised via its exactExecStartrather than through systemd.Still open on #92
The spare ProDesk and the bench rehearsal — both blocked on the purchase. Offsite is still unowned:
prometheusandoracleshare a shelf and a roof.🤖 Generated with Claude Code