Skip to content

feat: Add Cocoon-compatible Debian 13 images - #195

Open
ppmzhang2 wants to merge 1 commit into
cocoonstack:masterfrom
ppmzhang2:feat/img-debian
Open

feat: Add Cocoon-compatible Debian 13 images#195
ppmzhang2 wants to merge 1 commit into
cocoonstack:masterfrom
ppmzhang2:feat/img-debian

Conversation

@ppmzhang2

Copy link
Copy Markdown

This is from "https://github.com/ppmzhang2/cocoon-os/" to update the upstream repo's package, including:

  • build amd64 and arm64 images with Cocoon boot and networking hooks
  • install the checksum-verified agent and development SSH access
  • validate image metadata, kernel, initramfs, and service contracts
  • document local builds, publication, and native smoke testing

- build amd64 and arm64 images with Cocoon boot and networking hooks
- install the checksum-verified agent and development SSH access
- validate image metadata, kernel, initramfs, and service contracts
- document local builds, publication, and native smoke testing

@CMGS CMGS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this is careful work and it found two real debian bugs. before we take on a new image family though, a few things need sorting out. the big one: i think two of your three behavior deltas have the same root cause and one dockerfile line fixes both (see the comment on network.sh). if that holds, most of this diff disappears. the rest is about keeping the vendored copies diffable, trimming the validator, and getting a real boot on record. details inline.

# shellcheck disable=SC2154
[ -n "$rootmnt" ] || exit 0

strip_colons() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this and the klibc mount fix in overlay.sh are the same root cause: there is no busybox in this initramfs, only klibc-utils. klibc has no tr, and its mount does not reorder options after the operands. busybox has both, which is why the ubuntu copy works as is.

debian's initramfs-tools only recommends busybox, and the dockerfile uses --no-install-recommends. ubuntu's depends on it, so it always lands there.

can you check with lsinitramfs /boot/initrd.img-* | grep -c busybox inside the built image? if it is 0, adding busybox to the apt list is a one line fix and both script deltas go away. that would leave the resolv.conf symlink as the only real difference from the ubuntu hooks, which is a much smaller thing to carry.

@@ -0,0 +1,143 @@
#!/bin/sh
# Vendored from cocoon/os-image/ubuntu/network.sh at Cocoon v0.5.9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the vendoring is fine, we already do it for android. but please vendor verbatim and apply only the real deltas on top.

right now the three scripts are reworked head to toe: echo to printf, [ -z x ] && continue to [ -n x ] || continue, renamed locals, rewritten comments, local dropped. all behavior preserving, and it costs us the ability to diff the fork against its source. android/overlay.sh differs from the ubuntu one by 23 lines and you can see the android specific parts at a glance. this one differs by ~150 lines for about 20 lines of real change.

same ask for overlay.sh and install-agent.sh.


[ -n "$dns_servers" ] || dns_servers="8.8.8.8 8.8.4.4"

# Debian's systemd-resolved package creates this relative symlink at install

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is a genuine find, thanks. worth checking whether ubuntu 24.04 needs it too since it also ships systemd-resolved. if it does, the fix belongs in os-image/ubuntu/network.sh and both families get it.

# Debian's initramfs uses klibc mount, which requires all options before
# the device and directory operands. rootmnt is supplied by initramfs-tools.
# shellcheck disable=SC2154
mount -t overlay -o "$OVL_OPTS" overlay "$rootmnt" || panic "overlay failed"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options before operands is correct everywhere, not just under klibc. util-linux and busybox both accept it. so this is a strict improvement and it should land in os-image/ubuntu/overlay.sh and os-image/android/overlay.sh too, rather than only in the debian fork. we do not want three drifting copies of one boot contract.

same goes for the shellcheck cleanups, the ubuntu hooks have warnings today and would happily take the fixes.

# alive by moving them below the new overlay root before switch-root.
for mnt in $LAYER_MOUNTS; do
mkdir -p "${rootmnt}${mnt}"
mount -n -o move "$mnt" "${rootmnt}${mnt}" || panic "move ${mnt} into rootfs failed"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one changes the guest contract and needs more than a code comment.

the overlay is already mounted here, so mkdir -p "${rootmnt}${mnt}" writes into the cow upper layer. that means every vm now carries /.cocoon/layers/* and /.cocoon/cow in its cow disk, and those ride into snapshots and clones. it also exposes the overlay's own upper/ and work/ dirs to the guest, and writing there directly is undefined behavior for overlayfs.

can you describe the exact failure this fixes? if run-init really returns ENOTEMPTY on debian, ubuntu likely wants the same treatment, and then it is a shared change with a note in docs/os-image.md, which is where the boot contract is documented. if there is a way to keep the mounts alive without publishing them inside the guest root, that would be better.

# shellcheck disable=SC2016
assert_contains_regex \
"$rootfs/etc/initramfs-tools/scripts/cocoon-overlay" \
'^[[:space:]]*mount -t overlay -o "\$OVL_OPTS" overlay "\$rootmnt" \|\| panic "overlay failed"$' \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these source text assertions do not test the image, they freeze the script text.

you already cmp the installed hook against the vendored source a few lines up, so the file is proven identical. asserting that the same file contains a specific line can only fail when somebody edits the script, and then it fails by design and forces a matching edit here. renaming a variable would break the build. same pattern at 398, 403, 435, 439.

please drop the source greps. the behavior level checks around them are the valuable part: kernel CONFIG, lsinitramfs proving the hooks are in the initrd, enabled and masked units, sshd -T. keep those.


[[ -r "$SCRIPT_DIR/platforms" ]] || fatal "missing platform declaration: $SCRIPT_DIR/platforms"
[[ "$(<"$SCRIPT_DIR/platforms")" == "linux/amd64,linux/arm64" ]] \
|| fatal "platforms must declare exactly linux/amd64,linux/arm64"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this asserts the content of a file the pr itself ships, and the value is the same as the ci default when platforms is absent. it cannot fail. drop the check and the platforms file with it.

# (144927060c3e90dbe2f3e1a15143572c402958de), with pinned agent archives.
set -eu

AGENT_VERSION="0.2.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep ${COCOON_AGENT_VERSION:-0.2.0}. the per arch sha256 is the real gate, bumping the version without updating both checksums already fails loudly at sha256sum -c. dropping the override loses the local build knob and buys nothing.

while here, the ubuntu copy carries a few why comments you removed: why the per arch shas exist, and why ExecStartPre has the leading dash. those are the ones worth keeping.

esac

mkdir -p /run/sshd /etc/ssh/sshd_config.d
cat > /etc/ssh/sshd_config.d/00-cocoon-development.conf <<'EOF'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the drop-in file is cleaner than the sed in the ubuntu copy, no objection there. but this publishes an image to ghcr with root login and password auth explicitly on, labelled insecure in the image itself. our ubuntu images already allow root login so it is not new in kind, but the explicit password auth line is a policy call for @CMGS, not something i can wave through in review.

Comment thread os-image/debian/README.md
cloud kernel and checksum-verified Cocoon agent, plus the shared initramfs,
EROFS/overlay boot hooks, systemd networking, and SSH configuration.

## Build and Validate Locally

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static validation is a good start but it is not our acceptance bar for an os image. the bar is a full boot. the pr description mentions native smoke testing, and i do not see it here.

can you add the run you did, or do one: cocoon vm run the built image, show it getting an ip, cocoon vm exec over vsock, and a snapshot plus clone. amd64 is enough if arm64 is awkward. debian's cloud kernel is trimmed, and checking CONFIG symbols cannot prove the initramfs assembles and switch_root succeeds.

also please add the family to the catalog table in docs/os-image.md, that is the published image list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants