diff --git a/.github/scripts/tests/test_command_descriptions.py b/.github/scripts/tests/test_command_descriptions.py index ec35c3b2..7633c782 100644 --- a/.github/scripts/tests/test_command_descriptions.py +++ b/.github/scripts/tests/test_command_descriptions.py @@ -200,6 +200,10 @@ class CommandDescriptionsTests(unittest.TestCase): if lang != "en": messages = json.loads(path.read_text()) english = catalog("en") + # Storage-retention labels await the translation bot. Seed + # only those new leaves in this steady-state fixture. + for key, value in english["backup"]["storageRetention"].items(): + messages["backup"].setdefault("storageRetention", {}).setdefault(key, value) for section, key in (("encryption", "pbsHelp"), ("keyfileActions", "pveKeyDescription")): messages["backup"][section].setdefault(key, english["backup"][section][key]) diff --git a/.github/scripts/tests/test_vm_backup_jobs_wiring.py b/.github/scripts/tests/test_vm_backup_jobs_wiring.py index a8ecf4f5..930c82e3 100644 --- a/.github/scripts/tests/test_vm_backup_jobs_wiring.py +++ b/.github/scripts/tests/test_vm_backup_jobs_wiring.py @@ -15,7 +15,7 @@ class VmBackupJobsWiring(TestCase): source = ROUTES.read_text() routes = re.findall(r'@vm_backup_bp\.route\("([^"]+)", methods=\["(\w+)"\]\)\n@(\w+)\ndef (\w+)', source) self.assertEqual(len(routes), source.count('@vm_backup_bp.route(')) - self.assertEqual(len(routes), 8) + self.assertEqual(len(routes), 10) for path, method, guard, name in routes: expected = 'require_auth' if method == 'GET' else 'require_admin_scope' self.assertEqual(guard, expected, f'{method} {path} ({name})') diff --git a/.github/workflows/test-backup-storage-retention.yml b/.github/workflows/test-backup-storage-retention.yml new file mode 100644 index 00000000..321a227e --- /dev/null +++ b/.github/workflows/test-backup-storage-retention.yml @@ -0,0 +1,50 @@ +name: Backup storage retention + +on: + pull_request: + paths: + - '.github/workflows/test-backup-storage-retention.yml' + - 'AppImage/scripts/flask_vm_backup_routes.py' + - 'AppImage/components/backup-storage-retention.tsx' + - 'AppImage/components/backup-tab.tsx' + - 'AppImage/components/vm-backup-jobs.tsx' + - 'AppImage/lib/backup-retention.ts' + - 'tests/test_vm_backup_jobs_api.py' + - 'tests/test_backup_storage_retention.cjs' + push: + branches: [main, develop] + paths: + - 'AppImage/scripts/flask_vm_backup_routes.py' + - 'AppImage/components/backup-storage-retention.tsx' + - 'AppImage/lib/backup-retention.ts' + - 'tests/test_vm_backup_jobs_api.py' + - 'tests/test_backup_storage_retention.cjs' + workflow_dispatch: + +permissions: + contents: read + +jobs: + storage-retention: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + - uses: actions/setup-node@v6 + with: + node-version: '22.14.0' + - name: Install fixture dependencies + run: | + python -m pip install 'Flask==3.1.2' + npm ci --prefix .github/ci/offline-node --ignore-scripts --no-audit --no-fund + - name: Test native job and storage configuration APIs + run: python -B -m unittest discover -s tests -p test_vm_backup_jobs_api.py -v + - name: Test shared retention properties + env: + NODE_PATH: ${{ github.workspace }}/.github/ci/offline-node/node_modules + run: node tests/test_backup_storage_retention.cjs diff --git a/AppImage/components/backup-storage-retention.tsx b/AppImage/components/backup-storage-retention.tsx new file mode 100644 index 00000000..f27ae046 --- /dev/null +++ b/AppImage/components/backup-storage-retention.tsx @@ -0,0 +1,99 @@ +"use client" + +import { useState } from "react" +import useSWR from "swr" +import { HardDrive, Loader2, Pencil, RefreshCw } from "lucide-react" +import { Card, CardContent, CardHeader, CardTitle } from "./ui/card" +import { Badge } from "./ui/badge" +import { Button } from "./ui/button" +import { Checkbox } from "./ui/checkbox" +import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from "./ui/dialog" +import { Input } from "./ui/input" +import { Label } from "./ui/label" +import { fetchApi } from "../lib/api-config" +import { buildRetention, parseRetention, RETENTION } from "../lib/backup-retention" +import { useT } from "../lib/i18n/provider" + +interface StorageRetention { id: string; type: string; prune_backups: string; digest: string } +const fetcher = (url: string) => fetchApi<{ storages: StorageRetention[] }>(url) + +export function BackupStorageRetention() { + const t = useT() + const { data, error, mutate, isValidating } = useSWR("/api/vm-backup-storages", fetcher) + const [editing, setEditing] = useState(null) + const [fields, setFields] = useState>({}) + const [keepAll, setKeepAll] = useState(false) + const [busy, setBusy] = useState(false) + const [message, setMessage] = useState("") + const [saveError, setSaveError] = useState("") + + function edit(storage: StorageRetention) { + const policy = parseRetention(storage.prune_backups) + setFields(policy.fields) + setKeepAll(policy.keepAll || !storage.prune_backups) + setSaveError("") + setMessage("") + setEditing(storage) + } + + async function save() { + if (!editing || busy) return + const prune = buildRetention(fields, keepAll) + if (!prune) return setSaveError(t("backup.storageRetention.choosePolicy")) + setBusy(true) + setSaveError("") + try { + await fetchApi(`/api/vm-backup-storages/${encodeURIComponent(editing.id)}`, { + method: "PUT", body: JSON.stringify({ digest: editing.digest, prune_backups: prune }), + }) + setMessage(t("backup.storageRetention.saved")) + setEditing(null) + await mutate() + } catch (error) { + setSaveError(`${t("backup.storageRetention.saveFailed")} ${error instanceof Error ? error.message : ""}`) + } finally { setBusy(false) } + } + + return + + {t("backup.storageRetention.title")} + + + +

{t("backup.storageRetention.description")}

+ {message &&

{message}

} + {error ?

{t("backup.storageRetention.loadFailed")}

: !data ? + : !data.storages.length ? +

{t("backup.storageRetention.empty")}

: + data.storages.map(storage =>
+ {storage.id}{storage.type} + {storage.prune_backups || t("backup.storageRetention.default")} + +
)} + + { if (!open && !busy) setEditing(null) }}> + + {t("backup.storageRetention.editTitle", { storage: editing?.id ?? "" })} + {t("backup.storageRetention.effect")} + +
+ {RETENTION.map(key =>
+ + setFields(previous => ({ ...previous, [key]: event.target.value }))} /> +
)} +
+ {saveError &&

{saveError}

} + + + + + +} diff --git a/AppImage/components/backup-tab.tsx b/AppImage/components/backup-tab.tsx index ae62e19c..ffa5f82d 100644 --- a/AppImage/components/backup-tab.tsx +++ b/AppImage/components/backup-tab.tsx @@ -7,6 +7,7 @@ import { useState } from "react" import { DatabaseBackup } from "lucide-react" import { HostBackup } from "./host-backup" import { VmBackupJobs } from "./vm-backup-jobs" +import { BackupStorageRetention } from "./backup-storage-retention" import { useT } from "../lib/i18n/provider" type BackupView = "host" | "guests" @@ -51,7 +52,7 @@ export function BackupTab() {
- {view === "guests" && } + {view === "guests" && <>} ) } diff --git a/AppImage/components/vm-backup-jobs.tsx b/AppImage/components/vm-backup-jobs.tsx index b4054300..972c187d 100644 --- a/AppImage/components/vm-backup-jobs.tsx +++ b/AppImage/components/vm-backup-jobs.tsx @@ -21,6 +21,7 @@ import { import { fetchApi } from "../lib/api-config" import { cancelButtonClass } from "../lib/utils" import { useT } from "../lib/i18n/provider" +import { RETENTION, parseRetention, buildRetention } from "../lib/backup-retention" interface VmBackupJob { id: string @@ -70,24 +71,6 @@ const EMPTY_FORM: FormState = { mode: "snapshot", compress: "zstd", retention: {}, keepAll: false, notes: "{{guestname}}", attached: [], } -// The limits a job can keep, in the order Proxmox lists them. -const RETENTION = ["last", "hourly", "daily", "weekly", "monthly", "yearly"] as const - -const parseRetention = (prune: string) => { - const fields: Record = {} - let keepAll = false - for (const part of prune.split(",")) { - const [key, value] = part.trim().split("=") - if (key === "keep-all") keepAll = value === "1" - else if (key?.startsWith("keep-") && Number(value) > 0) fields[key.slice(5)] = String(Number(value)) - } - return { fields, keepAll } -} - -const buildRetention = (fields: Record, keepAll: boolean) => { - const parts = RETENTION.filter((key) => Number(fields[key]) > 0).map((key) => `keep-${key}=${Number(fields[key])}`) - return parts.length ? parts.join(",") : keepAll ? "keep-all=1" : "" -} const isOn = (value: unknown) => value === 1 || value === true || value === "1" const jobEnabled = (job: VmBackupJob) => job.enabled === undefined || isOn(job.enabled) diff --git a/AppImage/lib/backup-retention.ts b/AppImage/lib/backup-retention.ts new file mode 100644 index 00000000..7caa7686 --- /dev/null +++ b/AppImage/lib/backup-retention.ts @@ -0,0 +1,18 @@ +// Native Proxmox retention properties, shared by storage and job editors. +export const RETENTION = ["last", "hourly", "daily", "weekly", "monthly", "yearly"] as const + +export function parseRetention(prune: string) { + const fields: Record = {} + let keepAll = false + for (const part of prune.split(",")) { + const [key, value] = part.trim().split("=") + if (key === "keep-all") keepAll = value === "1" + else if (key?.startsWith("keep-") && Number(value) > 0) fields[key.slice(5)] = String(Number(value)) + } + return { fields, keepAll } +} + +export function buildRetention(fields: Record, keepAll: boolean) { + const parts = RETENTION.filter(key => Number(fields[key]) > 0).map(key => `keep-${key}=${Number(fields[key])}`) + return parts.length ? parts.join(",") : keepAll ? "keep-all=1" : "" +} diff --git a/AppImage/messages/en/common.json b/AppImage/messages/en/common.json index ca7e536d..f0d208f8 100644 --- a/AppImage/messages/en/common.json +++ b/AppImage/messages/en/common.json @@ -3938,6 +3938,23 @@ } }, "backup": { + "storageRetention": { + "title": "Storage retention", + "keepAll": "Keep all backups", + "description": "Default backup retention stored in Proxmox. Backup jobs with an explicit policy override this default.", + "refresh": "Refresh", + "saved": "Storage retention saved. No pruning was run.", + "choosePolicy": "Choose Keep all or at least one positive retention count.", + "saveFailed": "Could not save storage retention. Refresh the list if the configuration changed.", + "loadFailed": "Could not load storage retention.", + "empty": "No backup storages found.", + "default": "Proxmox default", + "edit": "Edit", + "editTitle": "Retention for {storage}", + "effect": "Applies to future backup runs that inherit this storage policy. Saving does not delete existing backups. Protected backups remain protected.", + "cancel": "Cancel", + "save": "Save retention" + }, "actions": { "addAnother": "Add another", "addPath": "Add path", diff --git a/AppImage/scripts/flask_vm_backup_routes.py b/AppImage/scripts/flask_vm_backup_routes.py index be83d8f9..e1259a2c 100644 --- a/AppImage/scripts/flask_vm_backup_routes.py +++ b/AppImage/scripts/flask_vm_backup_routes.py @@ -16,6 +16,9 @@ Endpoints, all under /api/vm-backup-jobs: DELETE / delete a job (its backups are kept) POST //toggle enable / disable POST //run run it now, as a Proxmox task on each node + +Storage retention is read/edited under /api/vm-backup-storages. Saving a +policy only changes the native storage configuration; it never runs pruning. """ import json @@ -229,6 +232,56 @@ def options(): }) +@vm_backup_bp.route("/api/vm-backup-storages", methods=["GET"]) +@require_auth +def backup_storage_retention(): + ok, storages = _pvesh_json(["get", "/storage"]) + if not ok or not isinstance(storages, list): + return _err(storages if not ok else "Invalid storage inventory", 502) + return jsonify({"storages": [ + {"id": storage["storage"], "type": storage.get("type", ""), + "prune_backups": _flat(storage.get("prune-backups") or ""), + "digest": storage.get("digest", "")} + for storage in storages + if isinstance(storage, dict) and storage.get("storage") + and "backup" in (storage.get("content") or "").split(",") + ]}) + + +@vm_backup_bp.route("/api/vm-backup-storages/", methods=["PUT"]) +@require_admin_scope +def update_backup_storage_retention(storage_id): + if not STORAGE_RE.fullmatch(storage_id): + return _err("Invalid storage") + data = request.get_json(silent=True) + if not isinstance(data, dict): + return _err("A retention policy and current configuration digest are required") + digest, prune = data.get("digest"), data.get("prune_backups") + if not isinstance(digest, str) or not re.fullmatch(r"[a-fA-F0-9]{40}", digest): + return _err("Refresh the storage list before editing its retention") + if not isinstance(prune, str) or not PRUNE_RE.fullmatch(prune): + return _err("Invalid retention policy") + pairs = [part.split("=", 1) for part in prune.split(",")] + keys = [key for key, _ in pairs] + if (len(keys) != len(set(keys)) or + ("keep-all" in keys and (len(keys) != 1 or pairs[0][1] != "1")) or + not any(int(value) > 0 for _, value in pairs)): + return _err("Choose keep-all=1 or one or more positive retention counts") + # Check that this is still a backup storage, then let Proxmox check the + # digest under its storage lock. Other storage settings are never passed. + ok, storage = _pvesh_json(["get", f"/storage/{storage_id}"]) + if not ok: + return _err(storage, 502) + if not isinstance(storage, dict) or "backup" not in (storage.get("content") or "").split(","): + return _err("This storage does not accept backups", 409) + if storage.get("digest") != digest: + return _err("Storage configuration changed; refresh before saving", 409) + ok, out = _pvesh(["set", f"/storage/{storage_id}", "--digest", digest, "--prune-backups", prune]) + if not ok: + return _err(out, 409) + return jsonify({"success": True}) + + @vm_backup_bp.route("/api/vm-backup-jobs", methods=["POST"]) @require_admin_scope def create_job(): diff --git a/tests/test_backup_storage_retention.cjs b/tests/test_backup_storage_retention.cjs new file mode 100644 index 00000000..218be9fd --- /dev/null +++ b/tests/test_backup_storage_retention.cjs @@ -0,0 +1,27 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const vm = require('node:vm'); +const ts = require('typescript'); +const source = fs.readFileSync('AppImage/lib/backup-retention.ts', 'utf8'); +const context = { exports: {} }; +vm.runInNewContext(ts.transpileModule(source, { compilerOptions: { module: ts.ModuleKind.CommonJS } }).outputText, context); +const { parseRetention, buildRetention } = context.exports; +for (const policy of ['keep-all=1', 'keep-last=3', 'keep-last=3,keep-weekly=2', 'keep-hourly=24,keep-daily=7,keep-monthly=12,keep-yearly=2']) { + const { fields, keepAll } = parseRetention(policy); + assert.equal(buildRetention(fields, keepAll), policy); +} +assert.equal(buildRetention({}, false), '', 'empty policy remains explicit'); +assert.equal(buildRetention({}, true), 'keep-all=1'); +assert.equal(buildRetention({ daily: '3', hourly: '0', last: '' }, false), 'keep-daily=3'); +assert.equal(buildRetention({ last: '3' }, true), 'keep-last=3', 'existing job editor precedence preserved'); +const component = fs.readFileSync('AppImage/components/backup-storage-retention.tsx', 'utf8'); +const english = JSON.parse(fs.readFileSync('AppImage/messages/en/common.json')).backup.storageRetention; +for (const [, key] of component.matchAll(/t\("backup\.storageRetention\.([A-Za-z]+)"/g)) { + assert.ok(typeof english[key] === 'string' && english[key], `translation: ${key}`); +} +assert.ok(component.includes('digest: editing.digest')); +assert.ok(component.includes('method: "PUT"')); +assert.ok(!component.includes('/prune')); +assert.ok(fs.readFileSync('AppImage/components/vm-backup-jobs.tsx', 'utf8').includes('from "../lib/backup-retention"')); +assert.ok(fs.readFileSync('AppImage/components/backup-tab.tsx', 'utf8').includes('')); +console.log('PASS: shared native retention properties, keep-all/count round trips, explicit empty policy, translations and digest wiring'); diff --git a/tests/test_vm_backup_jobs_api.py b/tests/test_vm_backup_jobs_api.py index cd7a2fda..c9f7b12b 100644 --- a/tests/test_vm_backup_jobs_api.py +++ b/tests/test_vm_backup_jobs_api.py @@ -1,5 +1,6 @@ """VM/CT backup jobs API: what reaches pvesh, with pvesh itself replaced.""" import importlib +import importlib.util import json import sys import types @@ -23,6 +24,34 @@ GUESTS = [{"vmid": 100, "name": "web", "type": "qemu", "node": "pve1"}, {"vmid": 200, "name": "other", "type": "qemu", "node": "pve2"}] +@unittest.skipIf(Flask is None, "Flask runtime required") +class StorageRetentionAuthorizationTests(unittest.TestCase): + def test_real_authentication_guard_blocks_read_only_writes_before_pvesh(self): + scripts = Path(__file__).resolve().parents[1] / "AppImage/scripts" + auth = types.ModuleType("auth_manager") + auth.load_auth_config = lambda: {"enabled": True} + auth.verify_token = lambda token: "tester" if token in ("admin", "reader") else None + auth.verify_token_full = lambda token: ("tester", "full_admin" if token == "admin" else "read_only") + spec = importlib.util.spec_from_file_location("storage_retention_middleware", scripts / "jwt_middleware.py") + middleware = importlib.util.module_from_spec(spec) + with patch.dict(sys.modules, auth_manager=auth): + spec.loader.exec_module(middleware) + spec = importlib.util.spec_from_file_location("storage_retention_auth_routes", scripts / "flask_vm_backup_routes.py") + routes = importlib.util.module_from_spec(spec) + with patch.dict(sys.modules, jwt_middleware=middleware): + spec.loader.exec_module(routes) + app = Flask(__name__) + app.register_blueprint(routes.vm_backup_bp) + client = app.test_client() + data = {"digest": "a" * 40, "prune_backups": "keep-last=3"} + with patch.object(routes, "_pvesh") as run: + self.assertEqual(client.get("/api/vm-backup-storages").status_code, 401) + self.assertEqual(client.put("/api/vm-backup-storages/local", json=data).status_code, 401) + self.assertEqual(client.put("/api/vm-backup-storages/local", json=data, + headers={"Authorization": "Bearer reader"}).status_code, 403) + run.assert_not_called() + + @unittest.skipIf(Flask is None, "Flask runtime required") class VmBackupJobsApiTests(unittest.TestCase): def setUp(self): @@ -73,9 +102,59 @@ class VmBackupJobsApiTests(unittest.TestCase): def test_reading_needs_a_session_and_changing_needs_an_administrator(self): guards = dict((name, kind) for kind, name in self.guards) - self.assertEqual({name for name, kind in guards.items() if kind == "auth"}, {"list_jobs", "options", "get_job"}) + self.assertEqual({name for name, kind in guards.items() if kind == "auth"}, {"list_jobs", "options", "get_job", "backup_storage_retention"}) self.assertEqual({name for name, kind in guards.items() if kind == "admin"}, - {"create_job", "update_job", "delete_job", "toggle_job", "run_job"}) + {"create_job", "update_job", "delete_job", "toggle_job", "run_job", "update_backup_storage_retention"}) + + def test_storage_retention_lists_only_backup_storages_and_no_secrets(self): + with patch.object(self.routes, "_pvesh_json", return_value=(True, [ + {"storage": "local", "type": "dir", "content": "backup,iso", "digest": "a" * 40, + "prune-backups": {"keep-last": 3}, "password": "private"}, + {"storage": "images", "type": "dir", "content": "images", "digest": "a" * 40}, + ])): + answer = self.client.get("/api/vm-backup-storages").get_json() + self.assertEqual(answer, {"storages": [{"id": "local", "type": "dir", "digest": "a" * 40, "prune_backups": "keep-last=3"}]}) + + def storage_policy(self, prune="keep-last=3,keep-weekly=2", digest="a" * 40, current=None): + if current is None: + current = {"storage": "local", "content": "backup,iso", "digest": "a" * 40} + with patch.object(self.routes, "_pvesh_json", return_value=(True, current)), self.pvesh(): + return self.client.put("/api/vm-backup-storages/local", json={"digest": digest, "prune_backups": prune}) + + def test_saving_native_storage_retention_never_runs_pruning_or_changes_other_properties(self): + response = self.storage_policy() + self.assertEqual(response.status_code, 200) + self.assertEqual(self.changes(), [["set", "/storage/local", "--digest", "a" * 40, + "--prune-backups", "keep-last=3,keep-weekly=2"]]) + self.calls.clear() + self.assertEqual(self.storage_policy("keep-all=1").status_code, 200) + self.assertEqual(self.changes()[0][-1], "keep-all=1") + + def test_invalid_storage_retention_is_rejected_before_commands(self): + invalid = [None, [], {}, {"digest": "a" * 40, "prune_backups": []}] + for prune in ("", "keep-last=-1", "keep-all=0", "keep-all=2", "keep-last=0", "keep-last=1,keep-last=2", + "keep-all=1,keep-last=2", "keep-last=2 --delete path", "keep-last=1\n", "keep-other=1"): + invalid.append({"digest": "a" * 40, "prune_backups": prune}) + invalid += [{"digest": "invalid", "prune_backups": "keep-last=1"}] + with patch.object(self.routes, "_pvesh") as run: + for data in invalid: + with self.subTest(data=data): + self.assertEqual(self.client.put("/api/vm-backup-storages/local", json=data).status_code, 400) + self.assertEqual(self.client.put("/api/vm-backup-storages/-bad", json=invalid[-1]).status_code, 400) + run.assert_not_called() + + def test_stale_storage_configuration_or_non_backup_storage_is_not_overwritten(self): + for current in ({"content": "backup", "digest": "b" * 40}, + {"content": "images", "digest": "a" * 40}): + self.assertEqual(self.storage_policy(current=current).status_code, 409) + self.assertEqual(self.changes(), []) + + def test_native_storage_update_failure_is_visible(self): + with patch.object(self.routes, "_pvesh_json", return_value=(True, {"content": "backup", "digest": "a" * 40})), \ + patch.object(self.routes, "_pvesh", return_value=(False, "configuration changed under lock")): + response = self.client.put("/api/vm-backup-storages/local", json={"digest": "a" * 40, "prune_backups": "keep-last=3"}) + self.assertEqual(response.status_code, 409) + self.assertIn("changed", response.get_json()["error"]) def test_the_list_gives_text_properties_and_the_host_backups_that_hang_from_a_job(self): with self.pvesh(), patch.object(self.routes, "_host_backups_attached",