Upgrade OS images: Proxmox VE, Ubuntu, OpenWRT, Debian - #177
Conversation
WalkthroughUpdated the Debian base image from bookworm to trixie across devcontainer configs and CI; added Debian 13 handling in Ansible bootstrap; added Proxmox v9 upgrade notes; and bumped/fortified OpenWRT and Ubuntu VM creation scripts with checksum verification and argument-parsing refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant Remote
User->>Script: request download (ISO/image)
Script->>Remote: wget (file)
Remote-->>Script: file stream
Script->>Remote: wget (SHA256SUMS)
Remote-->>Script: SHA256SUMS
Script->>Script: extract expected checksum
Script->>Script: echo "expected file" | sha256sum -c -
alt checksum matches
Script->>User: proceed (create VM)
else checksum mismatch or missing
Script->>User: exit with error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used📓 Path-based instructions (1)**/*.{sh,Dockerfile,yml,yaml,tf}📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (7)
proxmox/create-openwrt-vm.sh (2)
20-30: Add checksum verification for downloaded imageValidate integrity of the OpenWRT image after download to avoid corrupted/incomplete media.
download_installer() { - readonly IMAGE_PATH="openwrt-${OPENWRT_VERSION}.img.gz" - readonly DOWNLOAD_URL="https://downloads.openwrt.org/releases/${OPENWRT_VERSION}/targets/x86/64/openwrt-${OPENWRT_VERSION}-x86-64-generic-ext4-combined.img.gz" + local -r IMAGE_PATH="openwrt-${OPENWRT_VERSION}.img.gz" + local -r DOWNLOAD_URL="https://downloads.openwrt.org/releases/${OPENWRT_VERSION}/targets/x86/64/openwrt-${OPENWRT_VERSION}-x86-64-generic-ext4-combined.img.gz" + local -r SHA256SUMS_URL="https://downloads.openwrt.org/releases/${OPENWRT_VERSION}/targets/x86/64/sha256sums" if [ -e "${IMAGE_PATH}" ]; then echo "OpenWRT disk image ${IMAGE_PATH} already downloaded" else echo "Downloading OpenWRT disk image ${IMAGE_PATH}" - wget -O "${IMAGE_PATH}" "${DOWNLOAD_URL}" + wget -q --show-progress -O "${IMAGE_PATH}" "${DOWNLOAD_URL}" + echo "Verifying SHA256 checksum..." + EXPECTED_SHA256="$(wget -qO- "${SHA256SUMS_URL}" | awk '/generic-ext4-combined\.img\.gz$/ {print $1; exit}')" + if [ -z "$EXPECTED_SHA256" ]; then + echo "Failed to retrieve expected SHA256 from ${SHA256SUMS_URL}" >&2 + exit 1 + fi + echo "${EXPECTED_SHA256} ${IMAGE_PATH}" | sha256sum -c - fi }
61-73: Fix argument parsing: shift inside for-in is incorrectUsing shift inside a for arg in "$@" loop is a common pitfall and can lead to skipped or misparsed args. Use a while loop over "$1".
-for arg in "$@"; do - case $arg in - --download-only) - DOWNLOAD_ONLY=true - shift - ;; - *) - echo "Unknown argument: ${arg}" - exit 1 - ;; - esac -done +while [[ $# -gt 0 ]]; do + case "$1" in + --download-only) + DOWNLOAD_ONLY=true + shift + ;; + *) + echo "Unknown argument: $1" + exit 1 + ;; + esac +doneproxmox/create-ubuntu-server-vm.sh (3)
78-86: Add checksum verification for the ISO (optionally verify signature)Validate the ISO using official SHA256SUMS (and GPG signature if desired).
download_installer() { readonly INSTALL_ISO_PATH="/var/lib/vz/template/iso/${INSTALL_ISO}" if [ -e "${INSTALL_ISO_PATH}" ]; then echo "Installer ISO ${INSTALL_ISO} already downloaded" else echo "Downloading installer ISO ${INSTALL_ISO}" - wget -O ${INSTALL_ISO_PATH} https://releases.ubuntu.com/${UBUNTU_VERSION}/${INSTALL_ISO} + local -r BASE_URL="https://releases.ubuntu.com/${UBUNTU_VERSION}" + wget -q --show-progress -O "${INSTALL_ISO_PATH}" "${BASE_URL}/${INSTALL_ISO}" + echo "Verifying SHA256 checksum..." + wget -q -O /tmp/SHA256SUMS "${BASE_URL}/SHA256SUMS" + if ! sha256sum -c /tmp/SHA256SUMS --ignore-missing | grep -q "${INSTALL_ISO}: OK"; then + echo "Checksum verification failed for ${INSTALL_ISO}" >&2 + exit 1 + fi + # Optional: also verify GPG signature with SHA256SUMS.gpg and Ubuntu keys fi }
18-21: Hash method mismatch: comment says SHA-512 but value is SHA-256The hash starts with
$5$ (SHA-256). Either regenerate with SHA-512 ($6$ ) as the comment states, or update the comment to SHA-256 for accuracy.Options:
- Regenerate with SHA-512:
docker run -it --rm alpine mkpasswd --method=SHA-512- Or update the comment to: --method=SHA-256
112-124: Fix argument parsing: shift inside for-in is incorrectSame issue as the OpenWRT script. Use a while loop over "$1".
-for arg in "$@"; do - case $arg in - --download-only) - DOWNLOAD_ONLY=true - shift - ;; - *) - echo "Unknown argument: ${arg}" - exit 1 - ;; - esac -done +while [[ $# -gt 0 ]]; do + case "$1" in + --download-only) + DOWNLOAD_ONLY=true + shift + ;; + *) + echo "Unknown argument: $1" + exit 1 + ;; + esac +doneansible/bootstrap-ansible.sh (2)
33-33: Fix package name: use gnupg instead of non-existent pgp.Debian/Ubuntu provide gpg via gnupg. The "pgp" package does not exist and will cause apt failures.
- apt-get install --yes wget pgp + apt-get install --yes wget gnupg
57-57: Correct gpg option: use --dearmor (current --dearmour will fail).gpg uses the American spelling --dearmor. The current flag breaks keyring creation.
- wget -O- "https://keyserver.ubuntu.com/pks/lookup?fingerprint=on&op=get&search=0x6125E2A8C77F2818FB7BD15B93C4A3FD7BB9C367" | gpg --dearmour -o /usr/share/keyrings/ansible-archive-keyring.gpg + wget -O- "https://keyserver.ubuntu.com/pks/lookup?fingerprint=on&op=get&search=0x6125E2A8C77F2818FB7BD15B93C4A3FD7BB9C367" | gpg --dearmor -o /usr/share/keyrings/ansible-archive-keyring.gpg
🧹 Nitpick comments (5)
proxmox/create-openwrt-vm.sh (1)
32-37: Avoid global readonly in functions; use local -rreadonly inside functions creates global, immutable variables. Use local -r to limit scope.
create_vm() { # extract disk image - readonly VM_IMG="openwrt-${OPENWRT_VERSION}-vm-${VMID}.img" + local -r VM_IMG="openwrt-${OPENWRT_VERSION}-vm-${VMID}.img"proxmox/create-ubuntu-server-vm.sh (2)
33-39: Scope and quoting improvements for authorized_keysUse local for AUTHORIZED_KEYS_FILE and quote variables. Minor robustness improvement.
get_authorized_keys_config() { - AUTHORIZED_KEYS_FILE=~/.ssh/authorized_keys - if [ -s "$AUTHORIZED_KEYS_FILE" ]; then + local AUTHORIZED_KEYS_FILE="$HOME/.ssh/authorized_keys" + if [ -s "$AUTHORIZED_KEYS_FILE" ]; then echo " authorized-keys:" - sed 's/^/\ - "/; s/$/"/' "$AUTHORIZED_KEYS_FILE" + sed 's/^/\ - "/; s/$/"/' "$AUTHORIZED_KEYS_FILE" fi }
102-106: Consistent boolean check for AUTOINSTALLPrefer test expression over executing a command named “true/false”.
- if $AUTOINSTALL; then + if [ "$AUTOINSTALL" = true ]; then create_autoinstall_config qm set ${VMID} --ide0 local-lvm:cloudinit qm set ${VMID} --cicustom "vendor=local:snippets/${AUTOINSTALL_CONFIG_FILE}" fiproxmox/README.md (1)
17-23: Good addition: v9 upgrade pointersNice concise guidance. Consider adding two bullets:
- Take a backup/snapshot and ensure cluster is healthy before upgrade.
- Verify repository configuration (enterprise vs no-subscription) matches environment.
ansible/bootstrap-ansible.sh (1)
58-58: Use HTTPS for the APT source line.You already pin the keyring with signed-by (good). Use https to reduce exposure to MITM even further.
- echo "deb [signed-by=/usr/share/keyrings/ansible-archive-keyring.gpg] http://ppa.launchpad.net/ansible/ansible/ubuntu $UBUNTU_CODENAME main" | tee /etc/apt/sources.list.d/ansible.list + echo "deb [signed-by=/usr/share/keyrings/ansible-archive-keyring.gpg] https://ppa.launchpadcontent.net/ansible/ansible/ubuntu $UBUNTU_CODENAME main" | tee /etc/apt/sources.list.d/ansible.listNote: launchpad supports HTTPS via ppa.launchpadcontent.net for content delivery.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.devcontainer/Dockerfile(1 hunks).devcontainer/Taskfile.dev.yaml(1 hunks).devcontainer/devcontainer.json(1 hunks).github/workflows/devcontainer.yml(1 hunks)ansible/bootstrap-ansible.sh(1 hunks)proxmox/README.md(1 hunks)proxmox/create-openwrt-vm.sh(1 hunks)proxmox/create-ubuntu-server-vm.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,Dockerfile,yml,yaml,tf}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Files:
proxmox/create-openwrt-vm.shproxmox/create-ubuntu-server-vm.shansible/bootstrap-ansible.sh
🧠 Learnings (1)
📚 Learning: 2025-01-25T11:16:21.899Z
Learnt from: bubacoder
PR: bubacoder/infra#89
File: ansible/bootstrap-ansible.sh:23-24
Timestamp: 2025-01-25T11:16:21.899Z
Learning: In the bootstrap-ansible.sh script, the `--ignore-installed` flag is only needed for pip installations on Ubuntu to fix PyYAML conflicts. The Debian installation works fine without this flag.
Applied to files:
ansible/bootstrap-ansible.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check
- GitHub Check: build-test-push (debian, debian:trixie-slim)
- GitHub Check: build
- GitHub Check: build-test-push (ubuntu, ubuntu:24.04)
🔇 Additional comments (10)
proxmox/create-openwrt-vm.sh (3)
8-8: Version bump looks goodOPENWRT_VERSION updated to 24.10.2. No issues spotted with the change itself.
41-49: Network bridges: verify intended LAN/WAN mappingBoth net0 and net1 attach to vmbr0. If vmbr1 is intended for WAN, adjust accordingly.
38-40: Resizing raw disk doesn’t expand filesystemqemu-img resize grows the block device but not the filesystem. Confirm OpenWRT auto-expands the ext4 root or add resize logic.
proxmox/create-ubuntu-server-vm.sh (2)
8-10: Ubuntu version bump looks correctUpdated to 24.04.3. Filename/URL pattern matches Ubuntu release naming.
41-76: Ensure storage ‘local’ supports snippetscicustom uses local:snippets. Confirm storage ‘local’ has content type snippets enabled, or switch to a storage that does.
ansible/bootstrap-ansible.sh (1)
61-61: Good: Debian pip flags align with our prior guidance.Debian path omits --ignore-installed while keeping --break-system-packages. This matches the prior learning about avoiding --ignore-installed on Debian.
.devcontainer/Dockerfile (1)
3-3: LGTM: Base image upgraded to debian:trixie-slim.Consistent with other files. ARG is correctly in scope for the first stage and overridden where needed.
.devcontainer/devcontainer.json (1)
11-11: LGTM: devcontainer build-arg updated to trixie.Matches Dockerfile and CI. No further changes needed.
.github/workflows/devcontainer.yml (1)
35-35: LGTM: CI matrix updated to debian:trixie-slim.Workflow remains pinned to SHAs and includes a sanity test; alignment with devcontainer changes is good.
.devcontainer/Taskfile.dev.yaml (1)
15-15: LGTM: Task updated for Debian trixie.Description and BASE_IMAGE are consistent with Dockerfile/CI.
Also applies to: 19-20
3bd2270 to
30ed948
Compare
30ed948 to
65fd227
Compare
Summary by CodeRabbit