launchpad-reviewers team mailing list archive

[Colin Watson <mp+436285@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:snap-lxd-simplify into launchpad-buildd:master.

Commit message:
Don't require the lxd snap to be installed at postinst time

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/436285

In commit 679f3234d4c21d12ca5469cbda9242d9fb27c647, I switched from depending on the lxd .deb to requiring the snap to be installed, with the aim of getting launchpad-buildd working on jammy.  Unfortunately, this turned out to break upgrades of the riscv64 builders, where we currently handle upgrading the base VM images using a chroot.

However, the only reason the postinst needed to care about this was so that it could add the `buildd` user to the `lxd` group, and now that we run launchpad-buildd from a systemd service we can safely just rely on `SupplementaryGroups=lxd` giving the service that group membership.  Not actually adding that user to that group avoids problems with the group membership leaking through `sbuild`, allowing us to drop a chunk of quite obscure code.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:snap-lxd-simplify into launchpad-buildd:master.

diff --git a/bin/sbuild-package b/bin/sbuild-package
index 5e3e9f0..b5fc862 100755
--- a/bin/sbuild-package
+++ b/bin/sbuild-package
@@ -51,25 +51,6 @@ getent group sbuild | sudo tee -a chroot-autobuild/etc/group > /dev/null || exit
 getent passwd sbuild | sudo tee -a chroot-autobuild/etc/passwd > /dev/null || exit 2
 sudo chown sbuild:sbuild chroot-autobuild/build || exit 2
 
-# schroot calls initgroups(3) when entering a session, which sets
-# supplementary groups to the target user's groups in the host system's
-# /etc/group.  As a result, additional group memberships of the buildd user
-# (currently just lxd) outside the chroot are also visible inside the
-# chroot, and must exist in /etc/group there as well or else a few package
-# builds will fail, so we temporarily remove the lxd group membership for
-# the duration of this build.  This is all very unfortunate and not very
-# robust; perhaps eventually we should fix this by doing package builds in
-# LXD containers, although that would have its own problems, particularly
-# when bootstrapping new architectures.
-cleanup () {
-    sudo adduser --quiet buildd lxd
-}
-trap cleanup EXIT
-# According to deluser(8):
-#   6   The user does not belong to the specified group.  No action was
-#       performed.
-sudo deluser --quiet buildd lxd || [ $? = 6 ]
-
 hostarch=$(dpkg --print-architecture)
 
 UNAME26=""
diff --git a/debian/changelog b/debian/changelog
index 867c9f5..8eb2462 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,6 +2,8 @@ launchpad-buildd (227) UNRELEASED; urgency=medium
 
   * Tolerate receiving "builder_constraints": None.
   * Check the appropriate server.key path for the LXD snap.
+  * Restructure lxd group membership handling to avoid requiring the lxd
+    snap to be installed at postinst time.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Tue, 24 Jan 2023 13:13:27 +0000
 
diff --git a/debian/launchpad-buildd@.service b/debian/launchpad-buildd@.service
index 9d4b4b2..cdc7a99 100644
--- a/debian/launchpad-buildd@.service
+++ b/debian/launchpad-buildd@.service
@@ -13,10 +13,6 @@ Type=simple
 RuntimeDirectory=launchpad-buildd
 LogsDirectory=launchpad-buildd
 User=buildd
-# The buildd user should normally already be a member of this group, but due
-# to the deluser hacks in sbuild-package it's possible for the group
-# membership to be missing if a non-virtualized builder crashes in the
-# middle of an sbuild job.  Make sure of it here.
 SupplementaryGroups=lxd
 EnvironmentFile=-/etc/default/launchpad-buildd
 Environment=BUILDD_CONFIG=/etc/launchpad-buildd/%i
diff --git a/debian/postinst b/debian/postinst
index 84f9a40..7eb86cb 100644
--- a/debian/postinst
+++ b/debian/postinst
@@ -21,11 +21,6 @@ make_buildd()
 
 case "$1" in
     configure)
-	if [ ! -d /var/snap/lxd ]; then
-	    echo "LXD is not installed.  Run 'snap install lxd' and retry." >&2
-	    exit 1
-	fi
-
 	getent group buildd >/dev/null 2>&1 ||
                 addgroup --gid $BUILDDGID buildd
 
@@ -33,9 +28,26 @@ case "$1" in
         adduser --ingroup buildd --disabled-login --gecos 'Buildd user' \
                 --uid $BUILDDUID ${USER}
         adduser --quiet buildd sbuild
-        # Note that any additional group memberships here must currently be
-        # reflected in the deluser hacks in sbuild-package.
-        adduser --quiet buildd lxd
+
+	if dpkg --compare-versions "$2" lt-nl 227~; then
+	    # We used to add the buildd user to the lxd group.  This had
+	    # problems with leaking through sbuild, and it required lxd to
+	    # be installed at postinst time, which is problematic now that
+	    # lxd is typically installed as a snap, so we now rely entirely
+	    # on SupplementaryGroups=lxd in the systemd service.  Clean up
+	    # the old group membership.
+	    code=0
+	    sudo deluser --quiet buildd lxd || code=$?
+	    # According to deluser(8):
+	    #   0   Success: The action was successfully executed.
+	    #   3   There is no such group. No action was performed.
+	    #   6   The user does not belong to the specified group.  No
+	    #       action was performed.
+	    case $code in
+		0|3|6) ;;
+		*) exit "$code" ;;
+	    esac
+	fi
 
 	SUDO_VERSION=$(sudo -V | sed -n '/^Sudo version/s/.* //p')
 	if dpkg --compare-versions $SUDO_VERSION lt 1.7 ||