OCPBUGS-84842: Bootloader update attempt should be conditional

pkg/controller/build/buildrequest/buildrequestopts.go

@@ -55,7 +55,7 @@ func (b BuildRequestOpts) getKernelPackages() (string, map[string][]string, erro
 	}
 
 	// 64K memory pages kernel is only supported for aarch64
-	if newKtype == ctrlcommon.KernelType64kPages && goruntime.GOARCH != "arm64" {
+	if newKtype == ctrlcommon.KernelType64kPages && goruntime.GOARCH != ctrlcommon.GoArchARM64 {
 		return "", nil, fmt.Errorf("64k-pages is only supported for aarch64 architecture")
 	}
 

pkg/controller/common/constants.go

@@ -174,6 +174,10 @@ const (
 	// This matches etcd's default request size limit of 1.5MB (1572864 bytes).
 	// Reference: https://issues.redhat.com/browse/OCPBUGS-62619
 	MaxMachineConfigSize = 1572864
+
+	// Go GOARCH values for architecture matching
+	GoArchAMD64 = "amd64"
+	GoArchARM64 = "arm64"
 )
 
 // Commonly-used MCO ConfigMap names

pkg/daemon/bootupd.go

@@ -6,9 +6,12 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strconv"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/util/uuid"
+
+	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
 )
 
 // runGetOut executes a command on the host and returns its stdout output.
@@ -47,7 +50,7 @@ func (dn *Daemon) runBootupdViaContainer(imageURL string) error {
 	}
 	// For now, only attempt bootloader updates on x86_64 and aarch64 as they are the ones
 	// affected by the secure boot issue.
-	if runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64" {
+	if runtime.GOARCH != ctrlcommon.GoArchAMD64 && runtime.GOARCH != ctrlcommon.GoArchARM64 {
 		return nil
 	}
 	logSystem("runBootupdViaContainer: attempting bootloader update")
@@ -329,6 +332,91 @@ func findAnyESP(devices []lsblkDevice) string {
 	return ""
 }
 
+// shimIsSafe returns true if the installed shim package is at or above the
+// minimum version that contains safe Secure Boot signing keys (15.8-3.el9_2).
+// On non-UEFI arches where shim is absent this always returns true.
+func shimIsSafe() bool {
+	var pkgName string
+	switch runtime.GOARCH {
+	case ctrlcommon.GoArchAMD64:
+		pkgName = "shim-x64"
+	case ctrlcommon.GoArchARM64:
+		pkgName = "shim-aa64"
+	default:
+		return true
+	}
+
+	out, err := runGetOut("rpm", "-q", "--queryformat", "%{VERSION}-%{RELEASE}", pkgName)
+	if err != nil {
+		// Package not installed or rpm failed — fail open so bootupd runs.
+		logSystem("shimIsSafe: could not query %s, proceeding with bootloader update: %v", pkgName, err)
+		return false
+	}
+
+	installed := strings.TrimSpace(string(out))
+	const safeVersion = "15.8-3.el9_2"
+	safe := shimVersionAtLeast(installed, safeVersion)
+	if safe {
+		logSystem("shimIsSafe: %s %s >= %s, bootloader update not required", pkgName, installed, safeVersion)
+	} else {
+		logSystem("shimIsSafe: %s %s < %s, bootloader update required", pkgName, installed, safeVersion)
+	}
+	return safe
+}
+
+// shimVersionAtLeast reports whether the RPM "VERSION-RELEASE" string installed
+// is >= threshold by comparing their numeric segments in order.
+// Shim versions are always of the form "X.Y-N.elM_P", so comparing only the
+// numeric runs (ignoring distro markers like "el9") is sufficient and correct.
+func shimVersionAtLeast(installed, threshold string) bool {
+	instVer, instRel, ok1 := strings.Cut(installed, "-")
+	thrVer, thrRel, ok2 := strings.Cut(threshold, "-")
+	if !ok1 || !ok2 {
+		return false
+	}
+	if cmp := compareNumericSegments(instVer, thrVer); cmp != 0 {
+		return cmp > 0
+	}
+	return compareNumericSegments(instRel, thrRel) >= 0
+}
+
+// compareNumericSegments extracts all runs of digits from each string and
+// compares them pairwise as integers. Non-digit characters act as separators.
+// "15.8" → [15,8], "3.el9_2" → [3,9,2].
+func compareNumericSegments(a, b string) int {
+	aNums := extractInts(a)
+	bNums := extractInts(b)
+	for i := 0; i < len(aNums) && i < len(bNums); i++ {
+		if aNums[i] != bNums[i] {
+			if aNums[i] > bNums[i] {
+				return 1
+			}
+			return -1
+		}
+	}
+	return len(aNums) - len(bNums)
+}
+
+// extractInts returns all runs of digits in s parsed as integers.
+func extractInts(s string) []int {
+	var nums []int
+	i := 0
+	for i < len(s) {
+		if s[i] < '0' || s[i] > '9' {
+			i++
+			continue
+		}
+		j := i
+		for j < len(s) && s[j] >= '0' && s[j] <= '9' {
+			j++
+		}
+		n, _ := strconv.Atoi(s[i:j])
+		nums = append(nums, n)
+		i = j
+	}
+	return nums
+}
+
 // findRAIDESPs returns the ESP partition on every disk that is a member of the
 // RAID array containing raidDev. All replicas must be updated so the node can
 // boot from any surviving member after a disk failure.

pkg/daemon/bootupd_test.go

@@ -137,6 +137,67 @@ func TestFindBootPartition(t *testing.T) {
 	}
 }
 
+func TestShimVersionAtLeast(t *testing.T) {
+	tests := []struct {
+		installed string
+		threshold string
+		want      bool
+	}{
+		// Exactly at the threshold — safe.
+		{"15.8-3.el9_2", "15.8-3.el9_2", true},
+		// Older minor version — not safe.
+		{"15.6-1.el9", "15.8-3.el9_2", false},
+		{"15.7-2.el9", "15.8-3.el9_2", false},
+		// Same version, older release — not safe.
+		{"15.8-1.el9", "15.8-3.el9_2", false},
+		{"15.8-2.el9_2", "15.8-3.el9_2", false},
+		// Same version, newer release — safe.
+		{"15.8-4.el9_2", "15.8-3.el9_2", true},
+		// Newer major/minor — safe.
+		{"15.9-1.el9", "15.8-3.el9_2", true},
+		{"16.0-1.el9", "15.8-3.el9_2", true},
+		// Malformed input (missing release) — not safe.
+		{"15.8", "15.8-3.el9_2", false},
+		{"15.8-3.el9_2", "15.8", false},
+	}
+
+	for _, tt := range tests {
+		got := shimVersionAtLeast(tt.installed, tt.threshold)
+		if got != tt.want {
+			t.Errorf("shimVersionAtLeast(%q, %q) = %v, want %v", tt.installed, tt.threshold, got, tt.want)
+		}
+	}
+}
+
+func TestCompareNumericSegments(t *testing.T) {
+	tests := []struct {
+		a, b string
+		want int
+	}{
+		{"15.8", "15.8", 0},
+		{"15.9", "15.8", 1},
+		{"15.8", "15.9", -1},
+		{"15.10", "15.9", 1}, // numeric: 10 > 9
+		{"3.el9_2", "3.el9_2", 0},
+		{"3.el9_2", "2.el9_2", 1},
+		{"3.el9_2", "4.el9_2", -1},
+		{"3.el9_3", "3.el9_2", 1},
+		{"3.el9_2", "3.el9_3", -1},
+	}
+
+	for _, tt := range tests {
+		got := compareNumericSegments(tt.a, tt.b)
+		if got < 0 {
+			got = -1
+		} else if got > 0 {
+			got = 1
+		}
+		if got != tt.want {
+			t.Errorf("compareNumericSegments(%q, %q) = %d, want %d", tt.a, tt.b, got, tt.want)
+		}
+	}
+}
+
 func TestFindRAIDESPs(t *testing.T) {
 	esps := findRAIDESPs(raidDiskTree, "/dev/md0")
 	if len(esps) != 2 {

pkg/daemon/update.go

@@ -1864,7 +1864,7 @@ func (dn *CoreOSDaemon) switchKernel(oldConfig, newConfig *mcfgv1.MachineConfig)
 	}
 
 	// 64K memory pages kernel is only supported for aarch64
-	if newKtype == ctrlcommon.KernelType64kPages && goruntime.GOARCH != "arm64" {
+	if newKtype == ctrlcommon.KernelType64kPages && goruntime.GOARCH != ctrlcommon.GoArchARM64 {
 		return fmt.Errorf("64k-pages is only supported for aarch64 architecture")
 	}
 
@@ -2758,8 +2758,10 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error {
 	newURL := config.Spec.OSImageURL
 	klog.Infof("Updating OS to layered image %q", newURL)
 
-	if err := dn.runBootupdViaContainer(newURL); err != nil {
-		klog.Warningf("bootloader update failed: %s", err)
+	if !shimIsSafe() {
+		if err := dn.runBootupdViaContainer(newURL); err != nil {
+			klog.Warningf("bootloader update failed: %s", err)
+		}
 	}
 
 	newEnough, err := dn.NodeUpdaterClient.IsNewEnoughForLayering()