m/n/c/update: fix matching boot entries
The matching code accidentally worked as long as there was only one boot
entry for each loader path (boot-a.efi/boot-b.efi) as it type-asserted
a pointer which caused ok to always be false and thus all entries passed
through the UUID check.
This fixes the type assertion and following logic.
Change-Id: I83fdd2204028633dc274055f7d1ecb458747174e
Reviewed-on: https://review.monogon.dev/c/monogon/+/2031
Tested-by: Jenkins CI
Reviewed-by: Serge Bazanski <serge@monogon.tech>
diff --git a/metropolis/node/core/update/BUILD.bazel b/metropolis/node/core/update/BUILD.bazel
index e3cdcd1..e506984 100644
--- a/metropolis/node/core/update/BUILD.bazel
+++ b/metropolis/node/core/update/BUILD.bazel
@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
go_library(
name = "update",
@@ -18,3 +18,14 @@
"@org_golang_x_sys//unix",
],
)
+
+go_test(
+ name = "update_test",
+ srcs = ["update_test.go"],
+ embed = [":update"],
+ deps = [
+ "//metropolis/pkg/efivarfs",
+ "//metropolis/pkg/gpt",
+ "@com_github_google_uuid//:uuid",
+ ],
+)
diff --git a/metropolis/node/core/update/update.go b/metropolis/node/core/update/update.go
index 84a5db9..a357fa5 100644
--- a/metropolis/node/core/update/update.go
+++ b/metropolis/node/core/update/update.go
@@ -143,29 +143,9 @@
}
func (s *Service) getOrMakeBootEntry(existing map[int]*efivarfs.LoadOption, slot Slot) (int, error) {
- for idx, e := range existing {
- if len(e.FilePath) != 2 {
- // Not our entry
- continue
- }
- switch p := e.FilePath[0].(type) {
- case *efivarfs.HardDrivePath:
- gptMatch, ok := p.PartitionMatch.(*efivarfs.PartitionGPT)
- if ok && gptMatch.PartitionUUID != s.ESPPart.ID {
- // Not related to our ESP
- continue
- }
- default:
- continue
- }
- switch p := e.FilePath[1].(type) {
- case efivarfs.FilePath:
- if string(p) == slot.EFIBootPath() {
- return idx, nil
- }
- default:
- continue
- }
+ idx, ok := s.findBootEntry(existing, slot)
+ if ok {
+ return idx, nil
}
newEntry := &efivarfs.LoadOption{
Description: fmt.Sprintf("Metropolis Slot %s", slot),
@@ -189,6 +169,34 @@
return newIdx, err
}
+func (s *Service) findBootEntry(existing map[int]*efivarfs.LoadOption, slot Slot) (int, bool) {
+ for idx, e := range existing {
+ if len(e.FilePath) != 2 {
+ // Not our entry
+ continue
+ }
+ switch p := e.FilePath[0].(type) {
+ case *efivarfs.HardDrivePath:
+ gptMatch, ok := p.PartitionMatch.(efivarfs.PartitionGPT)
+ if !(ok && gptMatch.PartitionUUID == s.ESPPart.ID) {
+ // Not related to our ESP
+ continue
+ }
+ default:
+ continue
+ }
+ switch p := e.FilePath[1].(type) {
+ case efivarfs.FilePath:
+ if string(p) == slot.EFIBootPath() {
+ return idx, true
+ }
+ default:
+ continue
+ }
+ }
+ return 0, false
+}
+
// MarkBootSuccessful must be called after each boot if some implementation-
// defined criteria for a successful boot are met. If an update has been
// installed and booted and this function is called, the updated version is
diff --git a/metropolis/node/core/update/update_test.go b/metropolis/node/core/update/update_test.go
new file mode 100644
index 0000000..8206a22
--- /dev/null
+++ b/metropolis/node/core/update/update_test.go
@@ -0,0 +1,127 @@
+package update
+
+import (
+ "testing"
+
+ "github.com/google/uuid"
+
+ "source.monogon.dev/metropolis/pkg/efivarfs"
+ "source.monogon.dev/metropolis/pkg/gpt"
+)
+
+func TestFindBootEntry(t *testing.T) {
+ testUUID1 := uuid.MustParse("85cb7a0c-d31d-4b65-1111-919b069915f1")
+ testUUID2 := uuid.MustParse("d3086aa2-0327-4634-2222-5c6c8488cae3")
+ cases := []struct {
+ name string
+ slot Slot
+ espid uuid.UUID
+ entries map[int]*efivarfs.LoadOption
+ expectedOk bool
+ expectedIdx int
+ }{
+ {
+ name: "NoEntries",
+ slot: SlotA,
+ espid: testUUID1,
+ entries: make(map[int]*efivarfs.LoadOption),
+ expectedOk: false,
+ },
+ {
+ name: "FindSimple",
+ slot: SlotB,
+ espid: testUUID1,
+ entries: map[int]*efivarfs.LoadOption{
+ 5: &efivarfs.LoadOption{
+ Description: "Other Entry",
+ FilePath: efivarfs.DevicePath{
+ &efivarfs.HardDrivePath{
+ PartitionNumber: 1,
+ PartitionMatch: efivarfs.PartitionMBR{
+ DiskSignature: [4]byte{1, 2, 3, 4},
+ },
+ },
+ efivarfs.FilePath("EFI/something/else.efi"),
+ },
+ },
+ 6: &efivarfs.LoadOption{
+ Description: "Completely different entry",
+ FilePath: efivarfs.DevicePath{
+ &efivarfs.UnknownPath{
+ // Vendor-specific subtype
+ TypeVal: 1,
+ SubTypeVal: 4,
+ DataVal: []byte{1, 2, 3, 4},
+ },
+ efivarfs.FilePath("EFI/something"),
+ efivarfs.FilePath("else.efi"),
+ },
+ },
+ 16: &efivarfs.LoadOption{
+ Description: "Target Entry",
+ FilePath: efivarfs.DevicePath{
+ &efivarfs.HardDrivePath{
+ PartitionNumber: 2,
+ PartitionMatch: efivarfs.PartitionGPT{
+ PartitionUUID: testUUID1,
+ },
+ },
+ efivarfs.FilePath("/EFI/metropolis/boot-b.efi"),
+ },
+ },
+ },
+ expectedOk: true,
+ expectedIdx: 16,
+ },
+ {
+ name: "FindViaESPUUID",
+ slot: SlotA,
+ espid: testUUID1,
+ entries: map[int]*efivarfs.LoadOption{
+ 6: &efivarfs.LoadOption{
+ Description: "Other ESP UUID",
+ FilePath: efivarfs.DevicePath{
+ &efivarfs.HardDrivePath{
+ PartitionNumber: 2,
+ PartitionMatch: efivarfs.PartitionGPT{
+ PartitionUUID: testUUID2,
+ },
+ },
+ efivarfs.FilePath("/EFI/metropolis/boot-a.efi"),
+ },
+ },
+ 7: &efivarfs.LoadOption{
+ Description: "Target Entry",
+ FilePath: efivarfs.DevicePath{
+ &efivarfs.HardDrivePath{
+ PartitionNumber: 2,
+ PartitionMatch: efivarfs.PartitionGPT{
+ PartitionUUID: testUUID1,
+ },
+ },
+ efivarfs.FilePath("/EFI/metropolis/boot-a.efi"),
+ },
+ },
+ },
+ expectedOk: true,
+ expectedIdx: 7,
+ },
+ }
+
+ for _, c := range cases {
+ t.Run(c.name, func(t *testing.T) {
+ s := Service{
+ ESPPart: &gpt.Partition{
+ ID: c.espid,
+ },
+ }
+ idx, ok := s.findBootEntry(c.entries, c.slot)
+ if ok != c.expectedOk {
+ t.Fatalf("expected ok %t, got %t", c.expectedOk, ok)
+ }
+ if idx != c.expectedIdx {
+ t.Fatalf("expected idx %d, got %d", c.expectedIdx, idx)
+ }
+ })
+ }
+}