boot,bootloader: add support for shim fallback and setting EFI boot variables on install by valentindavid · Pull Request #13511 · canonical/snapd

valentindavid · 2024-01-23T13:51:30Z

This is #13205 rebased onto #13412

codecov-commenter · 2024-01-26T16:50:09Z

Codecov Report

Attention: Patch coverage is 71.53558% with 76 lines in your changes are missing coverage. Please review.

Project coverage is 78.84%. Comparing base (953fad1) to head (4d43110).

Files	Patch %	Lines
boot/assets.go	31.70%	25 Missing and 3 partials ⚠️
boot/setefibootvars_linux.go	73.78%	20 Missing and 7 partials ⚠️
gadget/mountedfilesystem.go	80.00%	10 Missing and 5 partials ⚠️
bootloader/grub.go	80.64%	4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13511      +/-   ##
==========================================
- Coverage   78.85%   78.84%   -0.02%     
==========================================
  Files        1043     1044       +1     
  Lines      134592   134848     +256     
==========================================
+ Hits       106137   106321     +184     
- Misses      21839    21896      +57     
- Partials     6616     6631      +15

Flag	Coverage Δ
unittests	`78.84% <71.53%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ernestl · 2024-03-13T14:21:24Z

@valentindavid @pedronis changed this to target 2.63 as agreed.

valentindavid · 2024-04-12T13:49:46Z

@olivercalder Could you have a look at the commits that are on top of your two commits and see if that makes sense to you.

The main one is 7be3e78

The rest is just some refactoring. Of the tests specially. And I can squash it later.

ernestl · 2024-04-12T18:03:13Z

Not ready for 2.63, moving to 2.64

olivercalder

Looks really great, thanks for all your work on this! A few questions/comments, but really nothing major.

olivercalder · 2024-04-12T18:29:40Z

Potentially, should the check if assetPath != "" occur within SetEfiBootVariables, so that each caller doesn't need to make the check? I think SetEfiBootVariables currently assumes assetPath is non-nil, might be safer if it were the one to check.

olivercalder · 2024-04-12T18:36:02Z

2024 now 🙃

olivercalder · 2024-04-12T18:38:46Z

nitpick: 2024 here too

olivercalder · 2024-04-12T18:39:53Z

This file is no longer changed, so no need to update copyright date?

olivercalder · 2024-04-12T18:43:24Z

pedronis

thank you, did a pass, main remark is that new code needs more unit testing

pedronis · 2024-04-18T14:37:38Z

even if not exported it would be good when possible to start with the function name here

this comment still applies

pedronis · 2024-04-18T14:37:48Z

pedronis · 2024-04-18T14:39:21Z

this is exported mostly for testing via spread tests? maybe the doc should remark that is usually not meant to be called directly

pedronis · 2024-04-18T14:39:41Z

can the uni tests check something about this?

pedronis · 2024-04-18T14:42:20Z

changes in assets.go don't have matching unit tests afaict

There should be TestUpdateBootEntryOnInstall and TestUpdateBootEntryOnUpdate in boot/assets_test.go.

Ah sorry, that was an older comment.

pedronis · 2024-04-18T14:43:20Z

unit tests strangely don't seem to reach these bits

this seems still true

Added 3 tests (no error, error in constructLoadOption, error in setEfiBootOptionVariable)

The test checks that EFI boot variables exist for the following: 1. A Boot#### variable pointing to the shim file path. 2. A BootOrder variable with the #### from the above Boot#### as first. Since the layout of EFI assets is dependent on the gadget snap, the test downloads and unpacks the gadget, then modifies the contents so that one variant has the shim and grub binaries in `EFI/boot/` and another variant has the shim and grub binaries in `EFI/ubuntu/` and the fallback binary in `EFI/boot/`. After building a core image around that modified gadget, the VM is booted and the test checks that the EFI variables are set correctly. Then, the test modifies the gadget to match the other variant's initial layout, and then installs the newly modified gadget. This should trigger re-setting EFI boot variables as well. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fix problems in spread test for setting EFI boot variables Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: disabled TPM on EFI boot vars test and separated gadget script Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fixed EFI vars test to use correct toolbox and include all EFI assets Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: modify-gadget.sh re-use existing gadget so edition is incremented Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fix mangled EFI var search string and other improvements Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: polish tests for setting EFI boot variables Notably, allow tests/nested/core/core20-set-efi-boot-variables to run on arm64 as well as amd64, simplify setefivars.go to search for multiple assets on multiple architectures, and allow tests/nested/manual/core20-set-efi-boot-vars to run on any ubuntu-2*. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

…leted

…ests

…t deleted files

… a boot entry

…t have UC

…to libs

…bles ... so we can reuse the script in other tests

… with UC available

…e boot

…is called

ernestl · 2024-05-31T13:58:13Z

@alfonsosanchezbeato, please do a final pass

alfonsosanchezbeato

LGTM, thanks for the changes

github-actions Bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Jan 23, 2024

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch from a17eb93 to 32f0cfc Compare February 22, 2024 15:26

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch 4 times, most recently from bd393d0 to bfc91a6 Compare March 5, 2024 15:52

ernestl added this to the 2.62 milestone Mar 6, 2024

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch 2 times, most recently from a9b1628 to 9c899fb Compare March 12, 2024 15:33

ernestl mentioned this pull request Mar 13, 2024

boot,bootloader: add support for shim fallback and setting EFI boot variables on install #13205

Closed

ernestl modified the milestones: 2.62, 2.63 Mar 13, 2024

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch 2 times, most recently from 222b8e9 to 4a062d2 Compare March 20, 2024 09:38

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch from 9fb4819 to 54eefa6 Compare March 20, 2024 16:14

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch from 54eefa6 to 4d43110 Compare April 4, 2024 11:35

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch from 4d43110 to ac21dff Compare April 12, 2024 13:45

valentindavid requested a review from olivercalder April 12, 2024 13:46

valentindavid marked this pull request as ready for review April 12, 2024 13:46

ernestl modified the milestones: 2.63, 2.64 Apr 12, 2024

olivercalder reviewed Apr 12, 2024

View reviewed changes

Meulengracht requested review from alfonsosanchezbeato and pedronis April 17, 2024 07:48

pedronis reviewed Apr 18, 2024

View reviewed changes

pedronis requested a review from bboozzoo April 18, 2024 14:45

olivercalder and others added 25 commits May 30, 2024 16:01

bootloader/grub.go: only consider new shim asset in boot entry for now

ee71497

tests/nested/core/core20-set-efi-boot-variables: fix details

f99c500

boot: update uefi variables on gadget update

6a2007f

tests/nested/manual/core20-set-efi-boot-vars: work-around file not de…

3127018

…leted

tests/nested/manual/core20-set-efi-boot-vars: use fb.efi like other t…

e8e5589

…ests

tests/nested/manual/core20-set-efi-boot-vars: drop use of toolbox snap

31a6ef1

tests/nested/manual/core20-set-efi-boot-vars: drop work-around for no…

e2288c3

…t deleted files

tests/nested/manual/core20-set-efi-boot-vars: verify install does add…

0008158

… a boot entry

tests/nested/manual/core20-set-efi-boot-vars: run only on version tha…

e6b7f1e

…t have UC

tests/nested/manual/core20-set-efi-boot-vars: obey GADGET_CHANNEL

3d2e2ad

tests/nested/manual/core20-set-efi-boot-vars: move get_boot_entry.py …

575b29a

…to libs

tests/nested/manual/core20-set-efi-boot-vars: factorize copy of varia…

e519dc0

…bles ... so we can reuse the script in other tests

tests/nested/core/core20-set-efi-boot-variables: stop using toolbox snap

28b588d

tests/nested/core/core20-set-efi-boot-variables: only run on versions…

98c37c4

… with UC available

overlord/devicestate: test using EfiLoadOptionParameters

3f79aed

boot: test that variables are set

fada8f3

boot: test observers' UpdateBootEntry

9975c7d

tests/nested/manual/core20-set-efi-boot-vars: also test without secur…

75b25f4

…e boot

many: use trusted install observer when UEFI variables are supported

709ad43

boot/makebootable.go: rename sealer to observer

1c0a5f7

boot/grub.go: fix function name in doc

b52f2ff

cmd/snap-bootstrap: verify that ObserveExistingTrustedRecoveryAssets …

7389b08

…is called

boot: add tests for SetEfiBootVariables

2eb3ff3

many: comment on calls to ObserveExistingTrustedRecoveryAssets

e6c9868

valentindavid force-pushed the valentindavid/olivers-uefi-variable-branch-rebased branch from 9677738 to e6c9868 Compare May 30, 2024 14:09

valentindavid requested a review from alfonsosanchezbeato May 31, 2024 12:21

alfonsosanchezbeato approved these changes May 31, 2024

View reviewed changes

ernestl merged commit 2034c7e into canonical:master Jun 3, 2024

Conversation

valentindavid commented Jan 23, 2024

Uh oh!

codecov-commenter commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ernestl commented Mar 13, 2024

Uh oh!

valentindavid commented Apr 12, 2024

Uh oh!

ernestl commented Apr 12, 2024

Uh oh!

olivercalder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ernestl commented May 31, 2024

Uh oh!

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Jan 26, 2024 •

edited

Loading