-
Notifications
You must be signed in to change notification settings - Fork 339
ci: Add bootc-ubuntu-setup action for ci-bootc workflow #3558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The previous approach of pulling podman/crun/skopeo from Debian testing was causing dependency issues on the Ubuntu 24.04 runners. Add a local copy of the bootc-ubuntu-setup action from bootc-dev/infra which uses Ubuntu plucky packages instead, and also handles disk space cleanup and KVM access setup. I'm going to investigate fetching this more directly in the future. Assisted-by: OpenCode (Sonnet 4) Signed-off-by: Colin Walters <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new composite GitHub Action, bootc-ubuntu-setup, to standardize the setup of Ubuntu runners for CI workflows. The changes address dependency issues on Ubuntu 24.04 by using packages from the 'plucky' release, and also include steps for disk cleanup and KVM configuration. My review focuses on improving the efficiency, robustness, and security of the shell scripts within the action. I've identified a few areas for improvement, including optimizing package removal, ensuring consistent script error handling, and hardening security configurations for KVM access and downloaded artifacts.
| shell: bash | ||
| run: | | ||
| set -xeuo pipefail | ||
| echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting MODE="0666" for /dev/kvm grants read/write access to all users on the runner, which is overly permissive and poses a security risk. The standard and more secure practice on Ubuntu is to use MODE="0660", restricting access to the root user and members of the kvm group. You should also ensure the runner user is added to the kvm group (e.g., with sudo usermod -aG kvm "$USER") for this to work.
echo 'KERNEL=="kvm", GROUP="kvm", MODE="0660", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules| cd $td | ||
| # Install bcvk | ||
| target=bcvk-$(arch)-unknown-linux-gnu | ||
| /bin/time -f '%E %C' curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action downloads a binary artifact from GitHub releases but doesn't verify its integrity using a checksum. This is a significant security vulnerability, as it opens the door to supply chain attacks if the release asset is compromised. It is highly recommended to verify the checksum of the downloaded file.
The bcvk project does not seem to publish checksums for its releases currently. The best long-term solution is to add checksum generation to the bcvk release process. Once checksums are available (e.g., in a sha256sums.txt file), you could verify the download like this:
# Download the artifact and its checksums
curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz
curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/sha256sums.txt
# Verify the checksum
sha256sum -c --ignore-missing sha256sums.txtEven without checksums being published, this is a risk that should be tracked and addressed.
| for x in ${unwanted_pkgs[@]}; do | ||
| /bin/time -f '%E %C' sudo apt-get remove -y $x | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling apt-get remove in a loop is inefficient. Each call starts a new process and interacts with the package manager. You can achieve the same result more efficiently by passing all packages to a single apt-get remove command. This also makes the timing with /bin/time more meaningful as it will measure the total time for removing all packages.
/bin/time -f '%E %C' sudo apt-get remove -y "${unwanted_pkgs[@]}"| - name: Update podman and install just | ||
| shell: bash | ||
| run: | | ||
| set -eux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other steps in this action and for improved robustness, it's best to include -o pipefail in your set command. This ensures that a pipeline will exit with a non-zero status if any command in the pipeline fails, which is often the desired behavior in CI scripts.
set -euxo pipefail|
Ah, this was a dup of #3556 |
The previous approach of pulling podman/crun/skopeo from Debian testing was causing dependency issues on the Ubuntu 24.04 runners. Add a local copy of the bootc-ubuntu-setup action from bootc-dev/infra which uses Ubuntu plucky packages instead, and also handles disk space cleanup and KVM access setup.
I'm going to investigate fetching this more directly in the future.
Assisted-by: OpenCode (Sonnet 4)