mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-03 18:55:33 +00:00
fix: reap the whole generated-ISO family without over-matching, and stop building python from the filename
Two filed issues in one rewrite of preflight-cleanup.sh's ISO block, because they are the same twenty lines. #111 — ISO_FILENAME was interpolated into python3 -c PROGRAM TEXT inside a single-quoted literal, so a quote in the value escaped it and executed arbitrary Python in a container holding PVE_API_TOKEN, PVE_PASSWORD, the Terraform state and the storage VM's SSH key. It now arrives through the environment and is read with os.environ. The volid is passed to urllib's quote() via argv for the same reason, and an empty encode result now skips the volume instead of issuing a DELETE against the bare collection URL. #105 — generated ISOs embed a hash of first-boot.sh, so every change to that script mints a new filename. Deleting only the exact current name orphaned each earlier ISO on the storage permanently, because force-cleanup wipes the Terraform state that could otherwise reclaim it. The family is now swept by rebuilding the full generated shape: the captured prefix plus twelve hex characters plus .iso. A prefix test alone would also have matched a longer FQDN's family and any hand-uploaded "-manual-backup.iso" sibling, which in a script whose job is deletion is worse than the leak it fixes. Multi-delete applies only to that family. A name that is not generated — the storage VM's cloud image — keeps the original one-shot behaviour, since a basename can repeat across content namespaces and a plain name carries nothing that identifies a family. Adds preflight-cleanup.test.sh, wired into shell-selfchecks. The script had no coverage at all. It stubs curl and sleep, then asserts on the DELETEs issued: the family goes, the pinned base ISO and unrelated uploads stay, a non-hash sibling stays, the cloud image takes only itself, a quoted payload is data rather than code, and unset storage skips only the ISO branch. Every case also asserts the script ran to completion and removed the Terraform state, so a path that dies early cannot pass by having issued the right DELETEs first.
This commit is contained in:
goodolclint-claude[bot]
@@ -63,25 +63,84 @@ elif [ -z "$ISO_STORAGE" ]; then
|
||||
fi
|
||||
echo "WARNING: TF_VAR_iso_storage is unset — skipping ISO cleanup rather than guessing a storage pool"
|
||||
else
|
||||
ISO_EXISTS=$(curl -sk -H "Authorization: PVEAPIToken=${API_TOKEN}" \
|
||||
# Generated auto-install ISOs carry a hash of first-boot.sh in the name, so each
|
||||
# change to that script mints a new filename. Deleting only the current name
|
||||
# strands every earlier one on the storage, and force-cleanup wipes the
|
||||
# Terraform state that could otherwise reclaim them. Match the whole family.
|
||||
ISO_MATCHES=$(curl -sk -H "Authorization: PVEAPIToken=${API_TOKEN}" \
|
||||
"${API_BASE}/nodes/${NODE}/storage/${ISO_STORAGE}/content" 2>/dev/null \
|
||||
| python3 -c "
|
||||
import json, sys
|
||||
data = json.load(sys.stdin).get('data', [])
|
||||
for item in data:
|
||||
if item.get('volid', '').endswith('/${ISO_FILENAME}'):
|
||||
print(item['volid'])
|
||||
break
|
||||
" 2>/dev/null || true)
|
||||
| ISO_FILENAME="$ISO_FILENAME" ISO_STORAGE="$ISO_STORAGE" python3 -c '
|
||||
import json, os, re, sys
|
||||
|
||||
if [ -n "$ISO_EXISTS" ]; then
|
||||
echo "Found orphaned ISO: ${ISO_EXISTS}"
|
||||
echo " Deleting..."
|
||||
ENCODED=$(python3 -c "import urllib.parse; print(urllib.parse.quote('${ISO_EXISTS}', safe=''))")
|
||||
curl -sk -X DELETE -H "Authorization: PVEAPIToken=${API_TOKEN}" \
|
||||
"${API_BASE}/nodes/${NODE}/storage/${ISO_STORAGE}/content/${ENCODED}" >/dev/null 2>&1
|
||||
sleep 2
|
||||
echo " ISO cleanup done"
|
||||
name = os.environ["ISO_FILENAME"]
|
||||
storage = os.environ["ISO_STORAGE"]
|
||||
# Generated names are <base>-auto-<storage-vm-fqdn-dashed>-<12 hex of first-boot.sh>.iso.
|
||||
# Siblings differ only in the hash, so sweep the family by rebuilding the full
|
||||
# shape — a prefix test alone would also match a longer FQDN or a hand-uploaded
|
||||
# "-manual-backup.iso", and this script deletes what it matches.
|
||||
family = re.match(r"^(.+-auto-.+-)[0-9a-f]{12}\.iso$", name)
|
||||
|
||||
try:
|
||||
data = json.load(sys.stdin).get("data", [])
|
||||
except Exception:
|
||||
sys.exit(0)
|
||||
|
||||
def candidates():
|
||||
for item in data:
|
||||
volid = item.get("volid", "")
|
||||
# The channel to the shell is newline-delimited, so a volid carrying a
|
||||
# newline would arrive as two lines and the tail would be deleted without
|
||||
# ever having matched. The anchored sibling pattern below already
|
||||
# excludes such a volid, so this is unreachable today and no test can
|
||||
# cover it — it is here so loosening that pattern cannot silently
|
||||
# reintroduce the split.
|
||||
if any(c in volid for c in "\r\n\0"):
|
||||
continue
|
||||
# A volid names its own storage. Deleting one through a different
|
||||
# storage endpoint is never right.
|
||||
if not volid.startswith(storage + ":"):
|
||||
continue
|
||||
yield volid, volid.rsplit("/", 1)[-1]
|
||||
|
||||
if family:
|
||||
sibling = re.compile(r"^" + re.escape(family.group(1)) + r"[0-9a-f]{12}\.iso$")
|
||||
for volid, base in candidates():
|
||||
if sibling.match(base):
|
||||
print(volid)
|
||||
else:
|
||||
# Anything else (the storage VM cloud image) keeps the original one-shot
|
||||
# behaviour: a basename can repeat across content namespaces, and a
|
||||
# non-generated name carries nothing that identifies a family.
|
||||
for volid, base in candidates():
|
||||
if base == name:
|
||||
print(volid)
|
||||
break
|
||||
' 2>/dev/null || true)
|
||||
|
||||
if [ -n "$ISO_MATCHES" ]; then
|
||||
while IFS= read -r volid; do
|
||||
[ -n "$volid" ] || continue
|
||||
echo "Found orphaned ISO: ${volid}"
|
||||
echo " Deleting..."
|
||||
ENCODED=$(python3 -c 'import sys, urllib.parse; print(urllib.parse.quote(sys.argv[1], safe=""))' "$volid")
|
||||
if [ -z "$ENCODED" ]; then
|
||||
echo " WARNING: could not encode ${volid} — skipping rather than issuing a bare DELETE" >&2
|
||||
continue
|
||||
fi
|
||||
code=$(curl -sk -o /dev/null -w '%{http_code}' -X DELETE \
|
||||
-H "Authorization: PVEAPIToken=${API_TOKEN}" \
|
||||
"${API_BASE}/nodes/${NODE}/storage/${ISO_STORAGE}/content/${ENCODED}" 2>/dev/null || echo 000)
|
||||
sleep 2
|
||||
case "$code" in
|
||||
2*) echo " ISO cleanup done" ;;
|
||||
*) echo " WARNING: DELETE of ${volid} returned ${code} — it is still on ${ISO_STORAGE}" >&2
|
||||
if [ "${GITHUB_ACTIONS:-}" = "true" ]; then
|
||||
echo "::warning::ISO ${volid} was not deleted (HTTP ${code}); it will accumulate on ${ISO_STORAGE}"
|
||||
fi ;;
|
||||
esac
|
||||
done <<EOF
|
||||
$ISO_MATCHES
|
||||
EOF
|
||||
else
|
||||
echo "No orphaned ISO found"
|
||||
fi
|
||||
|
||||
+147
@@ -0,0 +1,147 @@
|
||||
#!/usr/bin/env bash
|
||||
# Self-check for preflight-cleanup.sh's ISO cleanup branch.
|
||||
#
|
||||
# Stubs curl and sleep on PATH so every path runs offline in ~0s, then asserts
|
||||
# on the DELETEs the script actually issued.
|
||||
#
|
||||
# Run: bash tests/infrastructure/scripts/preflight-cleanup.test.sh
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
TARGET="$SCRIPT_DIR/preflight-cleanup.sh"
|
||||
|
||||
TMP="$(mktemp -d)"
|
||||
trap 'rm -rf "$TMP"' EXIT
|
||||
mkdir -p "$TMP/bin" "$TMP/tf"
|
||||
|
||||
# Storage listing the stub serves. Two generated ISOs of the same family (the
|
||||
# current first-boot.sh hash and a stale one), plus two volumes that must never
|
||||
# be touched: the pinned base ISO and an unrelated upload.
|
||||
cat > "$TMP/content.json" <<'JSON'
|
||||
{"data":[
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-158bb4537f15.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-0badc0ffee12.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1.iso"},
|
||||
{"volid":"ci-isos:iso/someone-elses.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-manual-backup.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-extra-0badc0ffee12.iso"},
|
||||
{"volid":"ci-isos:import/noble-server-cloudimg-amd64.qcow2"},
|
||||
{"volid":"ci-isos:iso/noble-server-cloudimg-amd64.qcow2"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-aaaaaaaaaaaa.qcow2"},
|
||||
{"volid":"ci-isos:iso/evil-proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-bbbbbbbbbbbb.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-OTHERHOST-cccccccccccc.iso"},
|
||||
{"volid":"other-storage:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-dddddddddddd.iso"},
|
||||
{"volid":"ci-isos:iso/proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-eeeeeeeeeeee\n.iso"},
|
||||
{"volid":"ci-isos:iso/plain-image-0123456789ab.iso"},
|
||||
{"volid":"ci-isos:iso/plain-image-ffffffffffff.iso"}
|
||||
]}
|
||||
JSON
|
||||
|
||||
cat > "$TMP/bin/curl" <<'STUB'
|
||||
#!/usr/bin/env bash
|
||||
args="$*"
|
||||
if [[ "$args" == *"-X DELETE"* ]]; then
|
||||
for a in "$@"; do [[ "$a" == http* ]] && echo "DELETE $a" >> "$STUB_LOG"; done
|
||||
# The script asks for the status with -w; echo what it expects to read.
|
||||
[[ "$args" == *"%{http_code}"* ]] && echo "${STUB_DELETE_CODE:-200}"
|
||||
exit 0
|
||||
fi
|
||||
if [[ "$args" == *"/storage/"*"/content"* ]]; then
|
||||
cat "$STUB_CONTENT"; exit 0
|
||||
fi
|
||||
if [[ "$args" == *"status/current"* ]]; then
|
||||
echo '{"data":{}}'; exit 0
|
||||
fi
|
||||
echo '{"data":[]}'
|
||||
STUB
|
||||
printf '#!/usr/bin/env bash\nexit 0\n' > "$TMP/bin/sleep"
|
||||
chmod +x "$TMP/bin/curl" "$TMP/bin/sleep"
|
||||
|
||||
export PATH="$TMP/bin:$PATH"
|
||||
export STUB_CONTENT="$TMP/content.json"
|
||||
export PVE_TARGET_NODE=pve-test
|
||||
export TF_VAR_iso_storage=ci-isos
|
||||
|
||||
# Records the exit status rather than discarding it: the script is best-effort by
|
||||
# design (set -uo pipefail, no -e), so a path that dies early would otherwise
|
||||
# still issue the expected DELETEs and pass.
|
||||
run_case() {
|
||||
STUB_LOG="$TMP/log.$1"; export STUB_LOG; : > "$STUB_LOG"
|
||||
: > "$TMP/tf/terraform.tfstate"
|
||||
set +e
|
||||
bash "$TARGET" https://pve.example.com:8006 token@pam!t=x 5091 "$2" "$TMP/tf" >"$TMP/out.$1" 2>&1
|
||||
echo $? > "$TMP/rc.$1"
|
||||
set -e
|
||||
}
|
||||
|
||||
assert_completed() {
|
||||
[ "$(cat "$TMP/rc.$1")" = "0" ] || fail "$1: script exited $(cat "$TMP/rc.$1")" "$TMP/out.$1"
|
||||
grep -q "Pre-flight cleanup complete" "$TMP/out.$1" || fail "$1: did not reach the end" "$TMP/out.$1"
|
||||
[ -f "$TMP/tf/terraform.tfstate" ] && fail "$1: stale Terraform state survived" "$TMP/out.$1"
|
||||
return 0
|
||||
}
|
||||
|
||||
fail() { echo "FAIL: $1"; echo "--- delete log ---"; cat "$2" 2>/dev/null; exit 1; }
|
||||
|
||||
# 1. The whole generated family goes, and nothing else does.
|
||||
run_case family "proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-158bb4537f15.iso"
|
||||
assert_completed family
|
||||
grep -q "158bb4537f15" "$TMP/log.family" || fail "current-hash ISO was not deleted" "$TMP/log.family"
|
||||
grep -q "0badc0ffee12" "$TMP/log.family" || fail "stale-hash ISO was not deleted (#105)" "$TMP/log.family"
|
||||
grep -q "proxmox-ve_9.2-1.iso" "$TMP/log.family" && fail "deleted the pinned base ISO" "$TMP/log.family"
|
||||
grep -q "someone-elses" "$TMP/log.family" && fail "deleted an unrelated volume" "$TMP/log.family"
|
||||
|
||||
# 2. The storage VM's cloud image — the real non-family argument, and not an
|
||||
# .iso — matches only itself and drags nothing else with it.
|
||||
run_case cloudimage "noble-server-cloudimg-amd64.qcow2"
|
||||
assert_completed cloudimage
|
||||
grep -q "noble-server-cloudimg" "$TMP/log.cloudimage" || fail "cloud image was not deleted" "$TMP/log.cloudimage"
|
||||
[ "$(grep -c DELETE "$TMP/log.cloudimage")" = "1" ] || fail "cloud image cleanup deleted more than itself" "$TMP/log.cloudimage"
|
||||
|
||||
# 2b. A hand-uploaded sibling sharing the family prefix but NOT the hash shape
|
||||
# must survive: a prefix-only test would delete it.
|
||||
run_case notfamily "proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-158bb4537f15.iso"
|
||||
grep -q "manual-backup" "$TMP/log.notfamily" && fail "deleted a non-hash sibling" "$TMP/log.notfamily"
|
||||
grep -q "ci-example-com-extra-0badc0ffee12" "$TMP/log.notfamily" && fail "deleted a longer-FQDN family" "$TMP/log.notfamily"
|
||||
grep -q "aaaaaaaaaaaa.qcow2" "$TMP/log.notfamily" && fail "deleted a non-.iso under the family prefix" "$TMP/log.notfamily"
|
||||
grep -q "evil-" "$TMP/log.notfamily" && fail "matched the prefix mid-string instead of anchoring" "$TMP/log.notfamily"
|
||||
grep -q "OTHERHOST" "$TMP/log.notfamily" && fail "deleted another host family (prefix truncated?)" "$TMP/log.notfamily"
|
||||
grep -q "other-storage" "$TMP/log.notfamily" && fail "deleted a volume on a different storage" "$TMP/log.notfamily"
|
||||
# A volid carrying a newline splits the line-delimited channel: the tail arrives
|
||||
# as a DELETE the matcher never approved.
|
||||
grep -qE "content/\.iso$" "$TMP/log.notfamily" && fail "a newline in a volid produced an unapproved DELETE" "$TMP/log.notfamily"
|
||||
grep -q "eeeeeeeeeeee" "$TMP/log.notfamily" && fail "deleted a volid containing a control character" "$TMP/log.notfamily"
|
||||
|
||||
# The DELETE must go to the percent-encoded volid, not a raw one: a bare volid
|
||||
# would be read by PVE as extra path segments.
|
||||
grep -q "content/ci-isos%3Aiso%2Fproxmox" "$TMP/log.family" || fail "volid was not URL-encoded in the DELETE" "$TMP/log.family"
|
||||
|
||||
# 3. A quote in the filename is data, not Python source. Before the fix this
|
||||
# interpolated into the program text and could execute (#111).
|
||||
run_case inject "x') or __import__('os').system('touch $TMP/PWNED') or ''.endswith('y"
|
||||
[ -e "$TMP/PWNED" ] && fail "filename was executed as Python (#111)" "$TMP/log.inject"
|
||||
[ "$(grep -c DELETE "$TMP/log.inject" || true)" = "0" ] || fail "injection payload matched a volume" "$TMP/log.inject"
|
||||
grep -q "No orphaned ISO found" "$TMP/out.inject" || fail "inject case did not reach the ISO branch" "$TMP/out.inject"
|
||||
assert_completed inject
|
||||
|
||||
# 4. Unset storage skips only the ISO branch; VM destroy and state cleanup still run.
|
||||
TF_VAR_iso_storage="" run_case nostorage "proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-158bb4537f15.iso"
|
||||
grep -q "skipping ISO cleanup" "$TMP/out.nostorage" || fail "expected the skip warning" "$TMP/out.nostorage"
|
||||
[ "$(grep -c DELETE "$TMP/log.nostorage" || true)" = "0" ] || fail "deleted an ISO with no storage configured" "$TMP/log.nostorage"
|
||||
assert_completed nostorage
|
||||
|
||||
# 4b. A hash-shaped name that is NOT a generated auto-install ISO is not a
|
||||
# family: without the -auto- anchor it would sweep unrelated siblings.
|
||||
run_case plain "plain-image-0123456789ab.iso"
|
||||
assert_completed plain
|
||||
grep -q "plain-image-0123456789ab" "$TMP/log.plain" || fail "plain image was not deleted" "$TMP/log.plain"
|
||||
[ "$(grep -c DELETE "$TMP/log.plain")" = "1" ] || fail "a non-auto name swept siblings" "$TMP/log.plain"
|
||||
|
||||
# 5. A rejected DELETE is reported, not logged as done.
|
||||
STUB_DELETE_CODE=403 run_case rejected "proxmox-ve_9.2-1-auto-pvetest-storage-ci-example-com-158bb4537f15.iso"
|
||||
grep -q "ISO cleanup done" "$TMP/out.rejected" && fail "a 403 DELETE was reported as done" "$TMP/out.rejected"
|
||||
grep -q "returned 403" "$TMP/out.rejected" || fail "a rejected DELETE was not surfaced" "$TMP/out.rejected"
|
||||
assert_completed rejected
|
||||
|
||||
echo "PASS: preflight-cleanup.sh ISO cleanup"
|
||||
Reference in New Issue
Block a user