Fix boot modes and ESP partitions for RHEL 8 and 9 (RHEL-69628)

pkg/distro/distro_test.go

@@ -13,14 +13,24 @@ import (
 	"github.com/osbuild/images/pkg/blueprint"
 	"github.com/osbuild/images/pkg/container"
 	"github.com/osbuild/images/pkg/distro"
-	"github.com/osbuild/images/pkg/distro/distro_test_common"
 	"github.com/osbuild/images/pkg/distrofactory"
 	"github.com/osbuild/images/pkg/ostree"
 	"github.com/osbuild/images/pkg/rpmmd"
+	testrepos "github.com/osbuild/images/test/data/repositories"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// listTestedDistros returns a list of distro names that are explicitly tested
+func listTestedDistros(t *testing.T) []string {
+	testRepoRegistry, err := testrepos.New()
+	require.Nil(t, err)
+	require.NotEmpty(t, testRepoRegistry)
+	distros := testRepoRegistry.ListDistros()
+	require.NotEmpty(t, distros)
+	return distros
+}
+
 // Ensure all image types report the correct names for their pipelines.
 // Each image type contains a list of build and payload pipelines. They are
 // needed for knowing the names of pipelines from the static object without
@@ -45,7 +55,7 @@ func TestImageTypePipelineNames(t *testing.T) {
 
 	assert := assert.New(t)
 	distroFactory := distrofactory.NewDefault()
-	distros := distro_test_common.ListTestedDistros(t)
+	distros := listTestedDistros(t)
 	for _, distroName := range distros {
 		d := distroFactory.GetDistro(distroName)
 		for _, archName := range d.ListArches() {
@@ -361,7 +371,7 @@ func TestPipelineRepositories(t *testing.T) {
 	}
 
 	distroFactory := distrofactory.NewDefault()
-	distros := distro_test_common.ListTestedDistros(t)
+	distros := listTestedDistros(t)
 	for tName, tCase := range testCases {
 		t.Run(tName, func(t *testing.T) {
 			for _, distroName := range distros {
@@ -529,7 +539,7 @@ func TestDistro_ManifestFIPSWarning(t *testing.T) {
 	}
 
 	distroFactory := distrofactory.NewDefault()
-	distros := distro_test_common.ListTestedDistros(t)
+	distros := listTestedDistros(t)
 	for _, distroName := range distros {
 		// FIPS blueprint customization is not supported for RHEL 7 images
 		if strings.HasPrefix(distroName, "rhel-7") {
@@ -578,7 +588,7 @@ func TestOSTreeOptionsErrorForNonOSTreeImgTypes(t *testing.T) {
 	distroFactory := distrofactory.NewDefault()
 	assert.NotNil(distroFactory)
 
-	distros := distro_test_common.ListTestedDistros(t)
+	distros := listTestedDistros(t)
 	assert.NotEmpty(distros)
 
 	for _, distroName := range distros {

pkg/distro/distro_test_common/distro_test_common.go

@@ -9,9 +9,10 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/osbuild/images/pkg/blueprint"
+	"github.com/osbuild/images/pkg/disk"
 	"github.com/osbuild/images/pkg/distro"
 	"github.com/osbuild/images/pkg/ostree"
-	testrepos "github.com/osbuild/images/test/data/repositories"
+	"github.com/osbuild/images/pkg/platform"
 )
 
 const RandomTestSeed = 0
@@ -476,12 +477,43 @@ func TestDistro_OSTreeOptions(t *testing.T, d distro.Distro) {
 	}
 }
 
-// ListTestedDistros returns a list of distro names that are explicitly tested
-func ListTestedDistros(t *testing.T) []string {
-	testRepoRegistry, err := testrepos.New()
-	require.Nil(t, err)
-	require.NotEmpty(t, testRepoRegistry)
-	distros := testRepoRegistry.ListDistros()
-	require.NotEmpty(t, distros)
-	return distros
+// TestESP checks whether all UEFI and hybrid images with a partition table have an ESP partition.
+// It also checks the opposite, i.e. that legacy images don't have an ESP. This test is only
+// performed on image types with a partition table, thus it doesn't run on e.g. installers and
+// ostree commits.
+// distros is a list of distros to test
+// ptFunc is a function that returns an uncustomized partition table for a given image type. This
+// proxy method is needed because the distro.ImageType interface doesn't provide a way to get a
+// partition table.
+func TestESP(t *testing.T, distros []distro.Distro, ptFunc func(i distro.ImageType) (*disk.PartitionTable, error)) {
+	for _, d := range distros {
+		for _, archName := range d.ListArches() {
+			a, err := d.GetArch(archName)
+			require.NoError(t, err)
+
+			for _, itName := range a.ListImageTypes() {
+				i, err := a.GetImageType(itName)
+				require.NoError(t, err)
+
+				// Skip image types that don't have a partition table.
+				if i.PartitionType() == disk.PT_NONE {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("%s/%s/%s", d.Name(), archName, itName), func(t *testing.T) {
+					pt, err := ptFunc(i)
+					require.NoError(t, err)
+
+					switch i.BootMode() {
+					case platform.BOOT_HYBRID, platform.BOOT_UEFI:
+						require.NotNil(t, pt.FindMountable("/boot/efi"))
+					default:
+						require.Nil(t, pt.FindMountable("/boot/efi"))
+					}
+
+				})
+			}
+		}
+
+	}
 }

pkg/distro/fedora/distro_internal_test.go

@@ -0,0 +1,28 @@
+package fedora
+
+import (
+	"math/rand"
+	"testing"
+
+	"github.com/osbuild/images/pkg/blueprint"
+	"github.com/osbuild/images/pkg/disk"
+	"github.com/osbuild/images/pkg/distro"
+	"github.com/osbuild/images/pkg/distro/distro_test_common"
+)
+
+// math/rand is good enough in this case
+/* #nosec G404 */
+var rng = rand.New(rand.NewSource(0))
+
+func TestESP(t *testing.T) {
+	var distros []distro.Distro
+	for _, distroName := range []string{"fedora-40", "fedora-41", "fedora-42"} {
+		d := DistroFactory(distroName)
+		distros = append(distros, d)
+	}
+
+	distro_test_common.TestESP(t, distros, func(i distro.ImageType) (*disk.PartitionTable, error) {
+		it := i.(*imageType)
+		return it.getPartitionTable(&blueprint.Customizations{}, distro.ImageOptions{}, rng)
+	})
+}

pkg/distro/rhel/distro_test.go

@@ -0,0 +1,77 @@
+package rhel_test
+
+import (
+	"fmt"
+	"math/rand"
+	"strings"
+	"testing"
+
+	"github.com/osbuild/images/pkg/blueprint"
+	"github.com/osbuild/images/pkg/disk"
+	"github.com/osbuild/images/pkg/distro"
+	"github.com/osbuild/images/pkg/distro/distro_test_common"
+	"github.com/osbuild/images/pkg/distro/rhel"
+	"github.com/osbuild/images/pkg/distrofactory"
+	"github.com/osbuild/images/pkg/platform"
+	"github.com/stretchr/testify/require"
+)
+
+// math/rand is good enough in this case
+/* #nosec G404 */
+var rng = rand.New(rand.NewSource(0))
+
+func TestESP(t *testing.T) {
+	var distros []distro.Distro
+	distroFactory := distrofactory.NewDefault()
+	for _, distroName := range []string{"rhel-7.9", "rhel-8.8", "rhel-8.9", "rhel-8.10", "centos-8", "rhel-9.0", "rhel-9.2", "rhel-9.4", "centos-9", "rhel-10.0", "centos-10"} {
+		distros = append(distros, distroFactory.GetDistro(distroName))
+	}
+
+	distro_test_common.TestESP(t, distros, func(i distro.ImageType) (*disk.PartitionTable, error) {
+		it := i.(*rhel.ImageType)
+		return it.GetPartitionTable([]blueprint.FilesystemCustomization{}, distro.ImageOptions{}, rng)
+	})
+}
+
+// TestAMIHybridBoot verifies that rhel 8.9 and 9.3 are the first RHEL versions
+// that implemented hybrid boot for the ami and ec2* image types
+func TestAMIHybridBoot(t *testing.T) {
+	testCases := []struct {
+		distro   string
+		bootMode platform.BootMode
+	}{
+		{"rhel-8.8", platform.BOOT_LEGACY},
+		{"rhel-8.9", platform.BOOT_HYBRID},
+		{"rhel-8.10", platform.BOOT_HYBRID},
+		{"centos-8", platform.BOOT_HYBRID},
+		{"rhel-9.0", platform.BOOT_LEGACY},
+		{"rhel-9.2", platform.BOOT_LEGACY},
+		{"rhel-9.3", platform.BOOT_HYBRID},
+		{"rhel-9.4", platform.BOOT_HYBRID},
+		{"centos-9", platform.BOOT_HYBRID},
+		{"rhel-10.0", platform.BOOT_HYBRID},
+		{"centos-10", platform.BOOT_HYBRID},
+	}
+
+	distroFactory := distrofactory.NewDefault()
+
+	for _, tc := range testCases {
+		// test only x86_64. ami for aarch64 has always UEFI, other arches are not defined.
+		a, err := distroFactory.GetDistro(tc.distro).GetArch("x86_64")
+		require.NoError(t, err)
+
+		for _, it := range a.ListImageTypes() {
+			// test only ami and ec2* image types
+			if it != "ami" && !strings.HasPrefix(it, "ec2") {
+				continue
+			}
+			t.Run(fmt.Sprintf("%s/%s", tc.distro, it), func(t *testing.T) {
+				it, err := a.GetImageType(it)
+				require.NoError(t, err)
+
+				require.Equal(t, tc.bootMode, it.BootMode())
+			})
+		}
+	}
+
+}

pkg/distro/rhel/rhel8/distro.go

@@ -115,6 +115,15 @@ func newDistro(name string, minor int) *rhel.Distribution {
 			ImageFormat: platform.FORMAT_RAW,
 		},
 	}
+
+	// Keep the RHEL EC2 x86_64 images before 8.9 BIOS-only for backward compatibility.
+	// RHEL-internal EC2 images and RHEL AMI images are kept intentionally in sync
+	// with regard to not supporting hybrid boot mode before RHEL version 8.9.
+	// The partitioning table for these reflects that and is also intentionally in sync.
+	if rd.IsRHEL() && common.VersionLessThan(rd.OsVersion(), "8.9") {
+		ec2X86Platform.UEFIVendor = ""
+	}
+
 	x86_64.AddImageTypes(
 		ec2X86Platform,
 		mkAmiImgTypeX86_64(),
@@ -342,16 +351,6 @@ func newDistro(name string, minor int) *rhel.Distribution {
 			mkAzureSapRhuiImgType(rd),
 		)
 
-		// keep the RHEL EC2 x86_64 images before 8.9 BIOS-only for backward compatibility
-		if common.VersionLessThan(rd.OsVersion(), "8.9") {
-			ec2X86Platform = &platform.X86{
-				BIOS: true,
-				BasePlatform: platform.BasePlatform{
-					ImageFormat: platform.FORMAT_RAW,
-				},
-			}
-		}
-
 		// add ec2 image types to RHEL distro only
 		x86_64.AddImageTypes(
 			ec2X86Platform,

pkg/distro/rhel/rhel9/distro.go

@@ -202,6 +202,15 @@ func newDistro(name string, major, minor int) *rhel.Distribution {
 			ImageFormat: platform.FORMAT_RAW,
 		},
 	}
+
+	// Keep the RHEL EC2 x86_64 images before 9.3 BIOS-only for backward compatibility.
+	// RHEL-internal EC2 images and RHEL AMI images are kept intentionally in sync
+	// with regard to not supporting hybrid boot mode before RHEL version 9.3.
+	// The partitioning table for these reflects that and is also intentionally in sync.
+	if rd.IsRHEL() && common.VersionLessThan(rd.OsVersion(), "9.3") {
+		ec2X86Platform.UEFIVendor = ""
+	}
+
 	x86_64.AddImageTypes(
 		ec2X86Platform,
 		mkAMIImgTypeX86_64(),
@@ -337,16 +346,6 @@ func newDistro(name string, major, minor int) *rhel.Distribution {
 
 		x86_64.AddImageTypes(azureX64Platform, mkAzureSapInternalImgType(rd))
 
-		// keep the RHEL EC2 x86_64 images before 9.3 BIOS-only for backward compatibility
-		if common.VersionLessThan(rd.OsVersion(), "9.3") {
-			ec2X86Platform = &platform.X86{
-				BIOS: true,
-				BasePlatform: platform.BasePlatform{
-					ImageFormat: platform.FORMAT_RAW,
-				},
-			}
-		}
-
 		// add ec2 image types to RHEL distro only
 		x86_64.AddImageTypes(ec2X86Platform, mkEc2ImgTypeX86_64(), mkEc2HaImgTypeX86_64(), mkEC2SapImgTypeX86_64(rd.OsVersion()))
 

pkg/distro/rhel/rhel9/partition_tables.go

@@ -1,6 +1,8 @@
 package rhel9
 
 import (
+	"strings"
+
 	"github.com/osbuild/images/internal/common"
 	"github.com/osbuild/images/pkg/arch"
 	"github.com/osbuild/images/pkg/datasizes"
@@ -24,6 +26,47 @@ func defaultBasePartitionTables(t *rhel.ImageType) (disk.PartitionTable, bool) {
 
 	switch t.Arch().Name() {
 	case arch.ARCH_X86_64.String():
+		// RHEL EC2 x86_64 images prior to 9.3 support only BIOS boot
+		if common.VersionLessThan(t.Arch().Distro().OsVersion(), "9.3") && t.IsRHEL() && (strings.HasPrefix(t.Name(), "ec2") || t.Name() == "ami") {
+			return disk.PartitionTable{
+				UUID: "D209C89E-EA5E-4FBD-B161-B461CCE297E0",
+				Type: disk.PT_GPT,
+				Partitions: []disk.Partition{
+					{
+						Size:     1 * datasizes.MebiByte,
+						Bootable: true,
+						Type:     disk.BIOSBootPartitionGUID,
+						UUID:     disk.BIOSBootPartitionUUID,
+					},
+					{
+						Size: bootSize,
+						Type: disk.XBootLDRPartitionGUID,
+						UUID: disk.FilesystemDataUUID,
+						Payload: &disk.Filesystem{
+							Type:         "xfs",
+							Mountpoint:   "/boot",
+							Label:        "boot",
+							FSTabOptions: "defaults",
+							FSTabFreq:    0,
+							FSTabPassNo:  0,
+						},
+					},
+					{
+						Size: 2 * datasizes.GibiByte,
+						Type: disk.FilesystemDataGUID,
+						UUID: disk.RootPartitionUUID,
+						Payload: &disk.Filesystem{
+							Type:         "xfs",
+							Label:        "root",
+							Mountpoint:   "/",
+							FSTabOptions: "defaults",
+							FSTabFreq:    0,
+							FSTabPassNo:  0,
+						},
+					},
+				},
+			}, true
+		}
 		return disk.PartitionTable{
 			UUID: "D209C89E-EA5E-4FBD-B161-B461CCE297E0",
 			Type: disk.PT_GPT,