From 6a63dea92339cb24bc15c370a2de3d20553bd2cb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 18:53:30 +0000 Subject: [PATCH] fix: apply review feedback on Docker storage and security documentation - Fix storage_ip derivation to use default route first, not Swarm RemoteManagers - Make iSCSI container entrypoint idempotent (check before create for target/LUN/bind) - Pin erichough/nfs-server from 'latest' to '2.2.1' for reproducibility - Replace curl|sh Docker install with apt repo in Dockerfile.test - Add docker.sock security warnings in docker-compose.test.yml and CI workflow - Update stale 'storage VM' references in comments and .env.test.example" Co-authored-by: GoodOlClint <151449+GoodOlClint@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoodOlClint/PSProxmoxVE/sessions/534c0a63-2988-496c-b0ae-e291fcfa050b --- .github/workflows/integration-tests.yml | 8 ++++++++ tests/.env.test.example | 4 ++-- tests/Dockerfile.test | 9 ++++++++- tests/docker-compose.test.yml | 2 ++ tests/infrastructure/docker-compose.storage.yml | 17 +++++++++++++---- tests/infrastructure/scripts/run-integration.sh | 14 ++++++++------ 6 files changed, 41 insertions(+), 13 deletions(-) diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 370293e..8fce156 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -115,6 +115,10 @@ jobs: password: ${{ secrets.GITHUB_TOKEN }} volumes: - /opt/pve-isos:/opt/pve-isos + # WARNING: The Docker socket is mounted to allow storage containers to be + # managed from within the job container. This grants root-equivalent access + # to the runner's Docker daemon. Only use on dedicated, isolated self-hosted + # runners — never on shared runners. - /var/run/docker.sock:/var/run/docker.sock steps: @@ -186,6 +190,10 @@ jobs: password: ${{ secrets.GITHUB_TOKEN }} volumes: - /opt/pve-isos:/opt/pve-isos + # WARNING: The Docker socket is mounted to allow storage containers to be + # stopped from within the job container. This grants root-equivalent access + # to the runner's Docker daemon. Only use on dedicated, isolated self-hosted + # runners — never on shared runners. - /var/run/docker.sock:/var/run/docker.sock steps: diff --git a/tests/.env.test.example b/tests/.env.test.example index 824fcc2..0f6e083 100644 --- a/tests/.env.test.example +++ b/tests/.env.test.example @@ -5,10 +5,10 @@ # will be provisioned. Integration tests run against the nested VMs, # not against this host directly. # -# Provisioned VMs: +# Provisioned resources: # - 2x PVE 9 nodes (pve9a, pve9b) for cluster testing # - 2x PVE 8 nodes (pve8a, pve8b) for cluster testing -# - 1x Ubuntu storage VM (iSCSI target + NFS server) +# - Shared storage provided by Docker containers on the runner host (iSCSI + NFS) # ── Required: parent PVE for provisioning ───────────────────────────── PVE_ENDPOINT=https://pve.example.com:8006 diff --git a/tests/Dockerfile.test b/tests/Dockerfile.test index 4df8846..9261022 100644 --- a/tests/Dockerfile.test +++ b/tests/Dockerfile.test @@ -65,5 +65,12 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ && rm -rf /var/lib/apt/lists/* # Install Docker CLI (for managing storage containers on the host via mounted socket) -RUN curl -fsSL https://get.docker.com | sh 2>/dev/null \ +RUN install -m 0755 -d /etc/apt/keyrings \ + && curl -fsSL https://download.docker.com/linux/ubuntu/gpg \ + | gpg --dearmor -o /etc/apt/keyrings/docker.gpg \ + && chmod a+r /etc/apt/keyrings/docker.gpg \ + && echo "deb [arch=amd64 signed-by=/etc/apt/keyrings/docker.gpg] https://download.docker.com/linux/ubuntu noble stable" \ + > /etc/apt/sources.list.d/docker.list \ + && apt-get update \ + && apt-get install -y --no-install-recommends docker-ce-cli docker-compose-plugin \ && rm -rf /var/lib/apt/lists/* diff --git a/tests/docker-compose.test.yml b/tests/docker-compose.test.yml index 7399848..196780c 100644 --- a/tests/docker-compose.test.yml +++ b/tests/docker-compose.test.yml @@ -38,6 +38,8 @@ services: volumes: - ..:/repo - /opt/pve-isos:/opt/pve-isos + # WARNING: Mounting the Docker socket grants this container root-equivalent + # access to the host's Docker daemon. Only use on trusted, isolated machines. - /var/run/docker.sock:/var/run/docker.sock working_dir: /repo stdin_open: true diff --git a/tests/infrastructure/docker-compose.storage.yml b/tests/infrastructure/docker-compose.storage.yml index cff3268..3993428 100644 --- a/tests/infrastructure/docker-compose.storage.yml +++ b/tests/infrastructure/docker-compose.storage.yml @@ -30,15 +30,24 @@ services: fi tgtd --foreground & sleep 2 - tgtadm --lld iscsi --op new --mode target --tid 1 -T $$ISCSI_IQN - tgtadm --lld iscsi --op new --mode logicalunit --tid 1 --lun 1 --backing-store /srv/iscsi/lun0.img - tgtadm --lld iscsi --op bind --mode target --tid 1 -I ALL + # Create target if it does not already exist + if ! tgtadm --lld iscsi --op show --mode target | grep -q "Target 1: $$ISCSI_IQN"; then + tgtadm --lld iscsi --op new --mode target --tid 1 -T $$ISCSI_IQN + fi + # Create logical unit (LUN 1) if it does not already exist + if ! tgtadm --lld iscsi --op show --mode logicalunit --tid 1 2>/dev/null | grep -qE "LUN:[[:space:]]*1($|[^0-9])"; then + tgtadm --lld iscsi --op new --mode logicalunit --tid 1 --lun 1 --backing-store /srv/iscsi/lun0.img + fi + # Bind target to all initiators if not already bound + if ! tgtadm --lld iscsi --op show --mode target --tid 1 2>/dev/null | grep -q "Initiator-address: ALL"; then + tgtadm --lld iscsi --op bind --mode target --tid 1 -I ALL + fi echo "iSCSI target ready: $$ISCSI_IQN (port 3260)" wait restart: unless-stopped nfs-server: - image: erichough/nfs-server:latest + image: erichough/nfs-server:2.2.1 container_name: pvetest-nfs network_mode: host privileged: true diff --git a/tests/infrastructure/scripts/run-integration.sh b/tests/infrastructure/scripts/run-integration.sh index 8266a87..6a152c4 100755 --- a/tests/infrastructure/scripts/run-integration.sh +++ b/tests/infrastructure/scripts/run-integration.sh @@ -5,10 +5,10 @@ # Called by both the GitHub Actions workflow and the local dev container. # # Provisions two PVE nodes per version (a/b) for cluster testing, plus -# an Ubuntu storage VM for iSCSI/NFS shared storage testing. +# Docker containers on the runner host for iSCSI/NFS shared storage. # # Usage: -# run-integration.sh provision Provision nested PVE VMs + storage VM +# run-integration.sh provision Provision nested PVE VMs + start storage containers # run-integration.sh test [8|9|all] Run integration tests (default: all) # run-integration.sh cleanup Destroy provisioned VMs # run-integration.sh all [8|9|all] Full lifecycle: provision → test → cleanup @@ -204,10 +204,12 @@ cmd_provision() { # Get the Docker host's real IP (not the container's). The storage containers # use host networking, so PVE nodes reach them via the host's IP. local storage_ip - storage_ip=$(docker info --format '{{range .Swarm.RemoteManagers}}{{.Addr}}{{end}}' 2>/dev/null | cut -d: -f1) + # Prefer the IP of the default route's interface on the Docker host. + storage_ip=$(docker run --rm --net=host alpine ip route get 1.1.1.1 2>/dev/null | awk '{for(i=1;i<=NF;i++) if($i=="src") print $(i+1)}') if [ -z "$storage_ip" ]; then - # Fallback: get IP of the default route's interface on the Docker host - storage_ip=$(docker run --rm --net=host alpine ip route get 1.1.1.1 2>/dev/null | awk '{for(i=1;i<=NF;i++) if($i=="src") print $(i+1)}') + # Fallback: use the local node address from Docker info (not RemoteManagers, + # which can return addresses of remote Swarm managers). + storage_ip=$(docker info --format '{{.Swarm.NodeAddr}}' 2>/dev/null | cut -d: -f1) fi if [ -z "$storage_ip" ]; then ci_error "Could not determine Docker host IP for storage services" @@ -453,7 +455,7 @@ main() { echo "Usage: $(basename "$0") {provision|test|cleanup|all} [8|9|all]" echo "" echo "Subcommands:" - echo " provision Provision nested PVE VMs + storage VM" + echo " provision Provision nested PVE VMs + start storage containers" echo " test [8|9|all] Run integration tests (default: all versions)" echo " cleanup Destroy all provisioned VMs" echo " all [8|9|all] Full lifecycle: provision → test → cleanup"