-
Notifications
You must be signed in to change notification settings - Fork 361
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
Add support for mariner rootfs build scenarios #12900
Conversation
@am11 I'd appreciate your eyes on this too (I can't add you directly as a reviewer). |
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.
Overall LGTM, thanks!
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.
nit: if we could avoid adding new files and instead burn keys lines exactly like the official script does: https://github.com/alpinelinux/alpine-chroot-install/blob/6d08f12a8a70dd9b9dc7d997c88aa7789cc03c42/alpine-chroot-install#L85-L133, that would be cleaner. (we already have too many small fragmented files than necessary in this part of the infra).
eng/common/cross/build-rootfs.sh
Outdated
if command -v qemu-debootstrap > /dev/null; then | ||
__Debootstrap="qemu-debootstrap" | ||
else | ||
__Debootstrap="debootstrap" |
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 main consumer of this script is https://github.com/dotnet/dotnet-buildtools-prereqs-docker repo where it will always use the if
branch. We should:
- invert this condition because
qemu-debootstrap
gives deprecation warning and tells us to usedebootstrap
- move --keyring option in the conditions above which are missing it (riscv64 and armv6 already define --keyring and expect it to install it on the host)
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.
I am planning to add new build images in dotnet-buildtools-prereqs-docker based on CBL-mariner, which will use the fall-back. I wanted to minimize changes to the existing build images (which are used for servicing for example), but I will give it a try.
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.
I have switched this to always use debootstrap with the minbase package set. I locally built all of the dotnet-buildtools-prereqs-docker images which build a rootfs and it seemed to work with the changes in dotnet/dotnet-buildtools-prereqs-docker#829.
For the mariner builds I will do something similar and place the ubuntu keyring at the location that debootstrap expects by default, so no changes to the other cases were needed.
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.
I think we must not check in the full gpg key to git repo. As mentioned above, these keys are installed on the intended host system (docker builder layer primarily). The keys in the docker layer https://github.com/dotnet/dotnet-buildtools-prereqs-docker images are installed from Debian/Ubuntu servers, for which -- I believe -- the supply chain has a blanket trust.
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 mariner docker images which I plan to add don't have the ubuntu keyring available. Any suggestions?
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.
If we are going to acquire https://src.fedoraproject.org/rpms/debootstrap in mariner's base image, we can also acquire https://src.fedoraproject.org/rpms/debian-keyring and use the keys this package provides. (Debian keys usually work for Ubuntu and vice-versa)
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.
Changed this to require the debootstrap keyring to be available on the host. It will make this script less convenient to use in a vanilla mariner environment which doesn't have the signing keys available (they are not available in a mariner package either, whereas debootstrap is). To make local development less painful I've added an argument to allow skipping signature checks (which was the default before this change).
Also fix keyring arg quoting (should not be quoted in case empty)
This reverts commit a47931d.
@janvorli PTAL |
Co-authored-by: Adeel Mujahid <[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.
LGTM 👍
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.
LGTM, thank you!
This makes a number of fixes to the rootfs build logic that enables running from a mariner host:
Fall back on[Use] debootstrap since qemu-debootstrap isn't available in mariner (and is deprecated upstream).I didn't force signature checking for qemu-debootstrap to avoid breaking existing scenarios.Contributes to dotnet/runtime#83428