From 284a7bde02fc84caa033a35261b75f15ef1eaf58 Mon Sep 17 00:00:00 2001 From: Itxaka Date: Tue, 15 Apr 2025 13:37:04 +0200 Subject: [PATCH] Make Persistatant grub vars respect existing ones We were basically overwriting the file if it existed which is bad. Now it will read the file if it exists and ingest the existing ones, then in will override witht he given vars if they match, warn the user and then store the full processed vars. This means it will never overwrite any vars again Signed-off-by: Itxaka --- internal/agent/hooks/gruboptions.go | 2 +- pkg/action/bootentries.go | 2 +- pkg/action/bootentries_test.go | 2 +- pkg/elemental/elemental.go | 2 +- pkg/elemental/elemental_test.go | 6 ++--- pkg/utils/grub.go | 38 +++++++++++++++++++++-------- pkg/utils/utils_test.go | 6 ++--- 7 files changed, 38 insertions(+), 20 deletions(-) diff --git a/internal/agent/hooks/gruboptions.go b/internal/agent/hooks/gruboptions.go index 53be59d..785f124 100644 --- a/internal/agent/hooks/gruboptions.go +++ b/internal/agent/hooks/gruboptions.go @@ -56,7 +56,7 @@ func grubOptions(c config.Config, opts map[string]string) error { return err } } - err = utils.SetPersistentVariables(filepath.Join(runtime.OEM.MountPoint, "grubenv"), opts, c.Fs) + err = utils.SetPersistentVariables(filepath.Join(runtime.OEM.MountPoint, "grubenv"), opts, &c) if err != nil { c.Logger.Logger.Error().Err(err).Msg("Failed to set grub options") } diff --git a/pkg/action/bootentries.go b/pkg/action/bootentries.go index 0a8bc24..bcb0ca0 100644 --- a/pkg/action/bootentries.go +++ b/pkg/action/bootentries.go @@ -60,7 +60,7 @@ func selectBootEntryGrub(cfg *config.Config, entry string) error { vars := map[string]string{ "next_entry": entry, } - err = utils.SetPersistentVariables("/oem/grubenv", vars, cfg.Fs) + err = utils.SetPersistentVariables("/oem/grubenv", vars, cfg) if err != nil { cfg.Logger.Errorf("could not set default boot entry: %s\n", err) return err diff --git a/pkg/action/bootentries_test.go b/pkg/action/bootentries_test.go index ca8a8db..7a66cb5 100644 --- a/pkg/action/bootentries_test.go +++ b/pkg/action/bootentries_test.go @@ -694,7 +694,7 @@ var _ = Describe("Bootentries tests", Label("bootentry"), func() { err = SelectBootEntry(config, "kairos") Expect(err).ToNot(HaveOccurred()) Expect(memLog.String()).To(ContainSubstring("Default boot entry set to kairos")) - variables, err := utils.ReadPersistentVariables("/oem/grubenv", fs) + variables, err := utils.ReadPersistentVariables("/oem/grubenv", config) Expect(err).ToNot(HaveOccurred()) Expect(variables["next_entry"]).To(Equal("kairos")) }) diff --git a/pkg/elemental/elemental.go b/pkg/elemental/elemental.go index 6e7863e..cc2937a 100644 --- a/pkg/elemental/elemental.go +++ b/pkg/elemental/elemental.go @@ -584,7 +584,7 @@ func (e Elemental) SetDefaultGrubEntry(partMountPoint string, imgMountPoint stri return utils.SetPersistentVariables( filepath.Join(partMountPoint, cnst.GrubOEMEnv), map[string]string{"default_menu_entry": defaultEntry}, - e.config.Fs, + e.config, ) } diff --git a/pkg/elemental/elemental_test.go b/pkg/elemental/elemental_test.go index d553445..526bcfa 100644 --- a/pkg/elemental/elemental_test.go +++ b/pkg/elemental/elemental_test.go @@ -848,7 +848,7 @@ var _ = Describe("Elemental", Label("elemental"), func() { el := elemental.NewElemental(config) Expect(config.Fs.Mkdir("/tmp", cnst.DirPerm)).To(BeNil()) Expect(el.SetDefaultGrubEntry("/tmp", "/imgMountpoint", "dio")).To(BeNil()) - varsParsed, err := utils.ReadPersistentVariables(filepath.Join("/tmp", cnst.GrubOEMEnv), config.Fs) + varsParsed, err := utils.ReadPersistentVariables(filepath.Join("/tmp", cnst.GrubOEMEnv), config) Expect(err).To(BeNil()) Expect(varsParsed["default_menu_entry"]).To(Equal("dio")) }) @@ -856,7 +856,7 @@ var _ = Describe("Elemental", Label("elemental"), func() { el := elemental.NewElemental(config) Expect(config.Fs.Mkdir("/mountpoint", cnst.DirPerm)).To(BeNil()) Expect(el.SetDefaultGrubEntry("/mountpoint", "/imgMountPoint", "")).To(BeNil()) - _, err := utils.ReadPersistentVariables(filepath.Join("/tmp", cnst.GrubOEMEnv), config.Fs) + _, err := utils.ReadPersistentVariables(filepath.Join("/tmp", cnst.GrubOEMEnv), config) // Because it didnt do anything due to the entry being empty, the file should not be there Expect(err).ToNot(BeNil()) _, err = config.Fs.Stat(filepath.Join("/tmp", cnst.GrubOEMEnv)) @@ -871,7 +871,7 @@ var _ = Describe("Elemental", Label("elemental"), func() { el := elemental.NewElemental(config) Expect(el.SetDefaultGrubEntry("/mountpoint", "/imgMountPoint", "")).To(BeNil()) - varsParsed, err := utils.ReadPersistentVariables(filepath.Join("/mountpoint", cnst.GrubOEMEnv), config.Fs) + varsParsed, err := utils.ReadPersistentVariables(filepath.Join("/mountpoint", cnst.GrubOEMEnv), config) Expect(err).To(BeNil()) Expect(varsParsed["default_menu_entry"]).To(Equal("test")) diff --git a/pkg/utils/grub.go b/pkg/utils/grub.go index 09fabde..76db0cc 100644 --- a/pkg/utils/grub.go +++ b/pkg/utils/grub.go @@ -24,6 +24,7 @@ import ( "github.com/kairos-io/kairos-agent/v2/pkg/utils/fs" "github.com/kairos-io/kairos-sdk/utils" "io/fs" + "os" "path/filepath" "sort" "strings" @@ -298,21 +299,38 @@ func findGrubDir(vfs v1.FS, dir string) string { } -// SetPersistentVariables sets the given vars into the given grubEnvFile for grub to read them -// TODO: Ingest the existing values from the grubenv file and set them as well -func SetPersistentVariables(grubEnvFile string, vars map[string]string, fs v1.FS) error { +func SetPersistentVariables(grubEnvFile string, vars map[string]string, c *agentConfig.Config) error { var b bytes.Buffer + // Write header b.WriteString("# GRUB Environment Block\n") - keys := make([]string, 0, len(vars)) - for k := range vars { + // First we need to read the existing values from the grubenv file if they exist + finalVars, err := ReadPersistentVariables(grubEnvFile, c) + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("error reading existing grubenv file %s: %s", grubEnvFile, err) + } + } + + // Merge the existing vars with the new ones + // existing vars will be overridden by the new ones from vars if they match + for key, newValue := range vars { + if oldValue, exists := finalVars[key]; exists { + c.Logger.Logger.Warn().Str("key", key).Str("oldValue", oldValue).Str("newValue", newValue).Msg("Overriding existing grubenv variable") + } + finalVars[key] = newValue + } + + keys := make([]string, 0, len(finalVars)) + + for k := range finalVars { keys = append(keys, k) } sort.Strings(keys) for _, k := range keys { - if len(vars[k]) > 0 { - b.WriteString(fmt.Sprintf("%s=%s\n", k, vars[k])) + if len(finalVars[k]) > 0 { + b.WriteString(fmt.Sprintf("%s=%s\n", k, finalVars[k])) } } @@ -322,7 +340,7 @@ func SetPersistentVariables(grubEnvFile string, vars map[string]string, fs v1.FS for i := 0; i < toBeFilled; i++ { b.WriteByte('#') } - return fs.WriteFile(grubEnvFile, b.Bytes(), cnst.FilePerm) + return c.Fs.WriteFile(grubEnvFile, b.Bytes(), cnst.FilePerm) } // copyGrubFonts will try to finds and copy the needed grub fonts into the system @@ -439,9 +457,9 @@ func (g Grub) copyGrub() error { } // ReadPersistentVariables will read a grub env file and parse the values -func ReadPersistentVariables(grubEnvFile string, fs v1.FS) (map[string]string, error) { +func ReadPersistentVariables(grubEnvFile string, c *agentConfig.Config) (map[string]string, error) { vars := make(map[string]string) - f, err := fs.ReadFile(grubEnvFile) + f, err := c.Fs.ReadFile(grubEnvFile) if err != nil { return nil, err } diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index ef130e7..6a63a89 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -804,9 +804,9 @@ var _ = Describe("Utils", Label("utils"), func() { defer os.Remove(temp.Name()) Expect(utils.SetPersistentVariables( temp.Name(), map[string]string{"key1": "value1", "key2": "value2"}, - config.Fs, + config, )).To(BeNil()) - readVars, err := utils.ReadPersistentVariables(temp.Name(), config.Fs) + readVars, err := utils.ReadPersistentVariables(temp.Name(), config) Expect(err).To(BeNil()) Expect(readVars["key1"]).To(Equal("value1")) Expect(readVars["key2"]).To(Equal("value2")) @@ -814,7 +814,7 @@ var _ = Describe("Utils", Label("utils"), func() { It("Fails setting variables", func() { e := utils.SetPersistentVariables( "badfilenopath", map[string]string{"key1": "value1"}, - config.Fs, + config, ) Expect(e).NotTo(BeNil()) })