-
Notifications
You must be signed in to change notification settings - Fork 168
build-sys: Rework sealing to be one build step #1898
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
base: main
Are you sure you want to change the base?
Conversation
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 is an excellent refactoring of the build system, unifying the standard and sealed image build processes into a single, multi-stage Docker build. This simplifies the build logic, improves maintainability, and likely enhances build caching. The changes are well-structured, breaking down the complex sealing process into smaller, dedicated scripts and Dockerfile stages. The removal of outdated files and the consolidation of logic into a single, more powerful Dockerfile is a significant cleanup. My review includes a few suggestions to improve the robustness of some of the new shell scripts to better handle cases where file globs might match more than one file.
| kver=$(cd /usr/lib/modules && echo *) | ||
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | ||
| echo "Error: No kernel found" >&2 | ||
| exit 1 | ||
| fi |
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 command kver=$(cd /usr/lib/modules && echo *) is not robust if multiple kernel version directories exist under /usr/lib/modules. In such a case, kver would contain a space-separated list of versions, which would break subsequent commands. It's safer to ensure exactly one kernel directory is found and fail otherwise.
| kver=$(cd /usr/lib/modules && echo *) | |
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | |
| echo "Error: No kernel found" >&2 | |
| exit 1 | |
| fi | |
| kver_paths=(/usr/lib/modules/*) | |
| if [ "${#kver_paths[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 kernel version directory, but found ${#kver_paths[@]}" >&2 | |
| ls -l /usr/lib/modules/ | |
| exit 1 | |
| fi | |
| kver=$(basename "${kver_paths[0]}") |
| kver=$(cd "${target}/usr/lib/modules" && echo *) | ||
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | ||
| echo "Error: No kernel found" >&2 | ||
| exit 1 | ||
| fi |
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 command kver=$(cd "${target}/usr/lib/modules" && echo *) is not robust if multiple kernel version directories exist. In such a case, kver would contain a space-separated list of versions, which would break subsequent commands. It's safer to ensure exactly one kernel directory is found and fail otherwise.
| kver=$(cd "${target}/usr/lib/modules" && echo *) | |
| if [ -z "$kver" ] || [ "$kver" = "*" ]; then | |
| echo "Error: No kernel found" >&2 | |
| exit 1 | |
| fi | |
| kver_paths=("${target}/usr/lib/modules"/*) | |
| if [ "${#kver_paths[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 kernel version directory, but found ${#kver_paths[@]}" >&2 | |
| ls -l "${target}/usr/lib/modules/" | |
| exit 1 | |
| fi | |
| kver=$(basename "${kver_paths[0]}") |
| sdboot_unsigned=$(ls ./usr/lib/systemd/boot/efi/systemd-boot*.efi) | ||
| sdboot_bn=$(basename ${sdboot_unsigned}) |
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 use of ls with a glob to find the systemd-boot EFI binary can be fragile. If the glob matches more than one file, the sdboot_unsigned variable will contain a multi-line string, which could cause sbsign to behave unexpectedly. It's safer to ensure exactly one file is found.
sdboot_unsigned_files=(./usr/lib/systemd/boot/efi/systemd-boot*.efi)
if [ ${#sdboot_unsigned_files[@]} -ne 1 ]; then
echo "Error: Expected 1 systemd-boot EFI file, but found ${#sdboot_unsigned_files[@]}" >&2
ls -l ./usr/lib/systemd/boot/efi/
exit 1
fi
sdboot_unsigned="${sdboot_unsigned_files[0]}"
sdboot_bn=$(basename "${sdboot_unsigned}")
| sdboot=$(ls /usr/lib/systemd/boot/efi/systemd-boot*.efi) | ||
| sdboot_bn=$(basename "${sdboot}") |
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 use of ls with a glob to find the systemd-boot EFI binary can be fragile. If the glob matches more than one file, the sdboot variable will contain a multi-line string. This would cause the install command on line 24 to fail because it would be interpreted as multiple destination arguments. It's safer to ensure exactly one file is found.
| sdboot=$(ls /usr/lib/systemd/boot/efi/systemd-boot*.efi) | |
| sdboot_bn=$(basename "${sdboot}") | |
| sdboot_files=(/usr/lib/systemd/boot/efi/systemd-boot*.efi) | |
| if [ "${#sdboot_files[@]}" -ne 1 ]; then | |
| echo "Error: Expected 1 systemd-boot EFI file, but found ${#sdboot_files[@]}" >&2 | |
| ls -l /usr/lib/systemd/boot/efi/ | |
| exit 1 | |
| fi | |
| sdboot="${sdboot_files[0]}" | |
| sdboot_bn=$(basename "${sdboot}") |
0ba5f48 to
4c7882c
Compare
OK this was working for me at one point but then broke and it took me quite a while to figure out why: containers/storage#743 (comment) |
|
OK this will depend on more composefs-rs work so we need to get back on git main, which is #1791 |
Merged now, so I think this is unblocked? |
|
I'm working on containers/composefs-rs#209 because I was hitting unexpected diffs - will flesh this out more |
182d8fe to
35cbf82
Compare
🎉 finally!!! |
|
This requires containers/composefs-rs#209 Though it'd also be cleaner if rebased on top of the buildsys changes from #1912 |
e35a329 to
182f6c2
Compare
OK it took me+agent quite a while to figure out this is caused by a combination of coreos/bootupd#995 plus us not having #1816 Effectively we had skew between the base image and the yum repos which is really https://gitlab.com/redhat/centos-stream/containers/bootc/-/issues/1174 |
The base image may be built from a compose that has newer packages than what's available on the public mirrors. This causes version skew where packages like bootupd have different versions between the base image and our built image. For example, bootupd 0.2.32 changed the EFI file layout from /usr/lib/bootupd/updates/EFI/ to /usr/lib/efi/, and if we build with an older bootupd from mirrors while the target image has the newer layout, bootloader installation fails. Enable the CentOS Stream compose repos with higher priority to ensure we get matching versions. xref https://gitlab.com/redhat/centos-stream/containers/bootc/-/issues/1174 Signed-off-by: Colin Walters <walters@verbum.org>
Now that we're doing a "from scratch" build we don't have the mtime issue, and so we can change our build system to do everything in a single step. Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
Add support for bind-mounting an extra source directory into container builds, primarily for developing against a local composefs-rs checkout. Usage: BOOTC_extra_src=$HOME/src/composefs-rs just build The directory is mounted at /run/extra-src inside the container. When using this, also patch Cargo.toml to use path dependencies pointing to /run/extra-src/crates/.... Signed-off-by: Colin Walters <walters@verbum.org> Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
The composefs-rs PR 209 has been merged to main. This updates bootc to use the containers/composefs-rs repository at the merge commit. Key API changes: - Directory::default() -> Directory::new(Stat::uninitialized()) - read_filesystem() no longer takes stat_root parameter - New read_container_root() for OCI containers (propagates /usr metadata to root) - stat_root CLI flag renamed to no_propagate_usr_to_root with inverted logic See containers/composefs-rs#209 Signed-off-by: Colin Walters <walters@verbum.org>
We changed how composefs digests are computed to ensure that mounted filesystem via --mount=type=image and install-time view (OCI tar layer processing from containers-storage) match. There were various problems like differing metadata for `/` among other things. Signed-off-by: Colin Walters <walters@verbum.org>
Add comprehensive documentation for building sealed bootc images, focusing on the core concepts and the key command: `bootc container compute-composefs-digest`. Key additions: - Document how sealed images work (UKI + composefs digest + Secure Boot) - Explain the build workflow abstractly without distribution-specific details - Document the compute-composefs-digest command and its options - Add section on generating/signing UKIs with ukify - Document developer testing commands (just variant=composefs-sealeduki-sdboot) - Add validation tooling documentation This provides the foundation for distribution-specific documentation to build upon with concrete Containerfile examples. Assisted-by: OpenCode (Claude Sonnet 4) Signed-off-by: Colin Walters <walters@verbum.org>
Justfile changes: - Organize targets into groups (core, testing, docs, debugging, maintenance) - Add `list-variants` target to show available build variants - Simplify comments to be concise single-line descriptions - Move composefs targets (build-sealed, test-composefs) into core group CONTRIBUTING.md changes: - Reference `just --list` and `just list-variants` instead of duplicating - Remove tables that duplicate Justfile information - Fix broken link to cli.rs The Justfile is now self-documenting via `just --list` (grouped targets) and `just list-variants` (build configuration options). Assisted-by: OpenCode (Claude Sonnet 4) Signed-off-by: Colin Walters <walters@verbum.org>
This is useful when debugging issues with stale cached layers, such as package version skew between base images and repos. Signed-off-by: Colin Walters <walters@verbum.org>
182f6c2 to
75b5312
Compare
Now that we're doing a "from scratch" build we don't
have the mtime issue, and so we can change our build system
to do everything in a single step.
Assisted-by: OpenCode (Opus 4.5)