From 02f41052778b44946bc535ed09bd9ff86dacdd91 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 15:54:00 +0200 Subject: [PATCH 01/16] Add test case to reproduce LP: #2031889 --- internal/statemachine/classic_test.go | 47 ++++++++++++++++++++------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 076c1a96..94fe8bcb 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -3602,10 +3602,11 @@ func TestCustomizeFstab(t *testing.T) { name string fstab []*imagedefinition.Fstab expectedFstab string + existingFstab string }{ { - "one_entry", - []*imagedefinition.Fstab{ + name: "one_entry to an empty fstab", + fstab: []*imagedefinition.Fstab{ { Label: "writable", Mountpoint: "/", @@ -3615,12 +3616,28 @@ func TestCustomizeFstab(t *testing.T) { FsckOrder: 1, }, }, - `LABEL=writable / ext4 defaults 1 1 + expectedFstab: `LABEL=writable / ext4 defaults 1 1 `, }, { - "two_entries", - []*imagedefinition.Fstab{ + name: "one_entry to a non-empty fstab", + fstab: []*imagedefinition.Fstab{ + { + Label: "writable", + Mountpoint: "/", + FSType: "ext4", + MountOptions: "defaults", + Dump: true, + FsckOrder: 1, + }, + }, + expectedFstab: `LABEL=writable / ext4 defaults 1 1 +`, + existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, + }, + { + name: "two_entries", + fstab: []*imagedefinition.Fstab{ { Label: "writable", Mountpoint: "/", @@ -3638,13 +3655,13 @@ func TestCustomizeFstab(t *testing.T) { FsckOrder: 1, }, }, - `LABEL=writable / ext4 defaults 0 1 + expectedFstab: `LABEL=writable / ext4 defaults 0 1 LABEL=system-boot /boot/firmware vfat defaults 0 1 `, }, { - "defaults_assumed", - []*imagedefinition.Fstab{ + name: "defaults_assumed", + fstab: []*imagedefinition.Fstab{ { Label: "writable", Mountpoint: "/", @@ -3652,7 +3669,7 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 FsckOrder: 1, }, }, - `LABEL=writable / ext4 defaults 0 1 + expectedFstab: `LABEL=writable / ext4 defaults 0 1 `, }, } @@ -3687,13 +3704,19 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 err = os.MkdirAll(filepath.Join(stateMachine.tempDirs.chroot, "etc"), 0644) asserter.AssertErrNil(err, true) + fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab") + + // simulate an already existing fstab file + if len(tc.existingFstab) != 0 { + err = osWriteFile(fstabPath, []byte(tc.existingFstab), 0644) + asserter.AssertErrNil(err, true) + } + // customize the fstab, ensure no errors, and check the contents err = stateMachine.customizeFstab() asserter.AssertErrNil(err, true) - fstabBytes, err := os.ReadFile( - filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab"), - ) + fstabBytes, err := os.ReadFile(fstabPath) asserter.AssertErrNil(err, true) if string(fstabBytes) != tc.expectedFstab { From e5c46ba8b00a7537ead1e1c0da482406207cc0bc Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 15:54:38 +0200 Subject: [PATCH 02/16] Refactor populateClassicRootfsContents to make easily readable --- internal/statemachine/classic_states.go | 49 +++++++++++++++---------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index 6eaa65d0..ea7bc5a7 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -27,8 +27,11 @@ import ( "github.com/canonical/ubuntu-image/internal/imagedefinition" ) -var seedVersionRegex = regexp.MustCompile(`^[a-z0-9].*`) -var localePresentRegex = regexp.MustCompile(`(?m)^LANG=|LC_[A-Z_]+=`) +var ( + seedVersionRegex = regexp.MustCompile(`^[a-z0-9].*`) + fstabRegex = regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`) + localePresentRegex = regexp.MustCompile(`(?m)^LANG=|LC_[A-Z_]+=`) +) // parseImageDefinition parses the provided yaml file and ensures it is valid func (stateMachine *StateMachine) parseImageDefinition() error { @@ -1289,24 +1292,30 @@ func (stateMachine *StateMachine) populateClassicRootfsContents() error { } } - if classicStateMachine.ImageDef.Customization != nil { - if len(classicStateMachine.ImageDef.Customization.Fstab) == 0 { - fstabPath := filepath.Join(classicStateMachine.tempDirs.rootfs, "etc", "fstab") - fstabBytes, err := osReadFile(fstabPath) - if err == nil { - if !strings.Contains(string(fstabBytes), "LABEL=writable") { - re := regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`) - newContents := re.ReplaceAll(fstabBytes, []byte("LABEL=writable\t/\t$1")) - if !strings.Contains(string(newContents), "LABEL=writable") { - newContents = []byte("LABEL=writable / ext4 defaults 0 0\n") - } - err := osWriteFile(fstabPath, newContents, 0644) - if err != nil { - return fmt.Errorf("Error writing to fstab: %s", err.Error()) - } - } - } - } + if classicStateMachine.ImageDef.Customization == nil { + return nil + } + + if len(classicStateMachine.ImageDef.Customization.Fstab) != 0 { + return nil + } + + fstabPath := filepath.Join(classicStateMachine.tempDirs.rootfs, "etc", "fstab") + fstabBytes, err := osReadFile(fstabPath) + if err != nil { + return fmt.Errorf("Error reading fstab: %s", err.Error()) + } + + if strings.Contains(string(fstabBytes), "LABEL=writable") { + return nil + } + newContents := fstabRegex.ReplaceAll(fstabBytes, []byte("LABEL=writable\t/\t$1")) + if !strings.Contains(string(newContents), "LABEL=writable") { + newContents = []byte("LABEL=writable / ext4 defaults 0 0\n") + } + err = osWriteFile(fstabPath, newContents, 0644) + if err != nil { + return fmt.Errorf("Error writing to fstab: %s", err.Error()) } return nil } From c0efc2c3f028ab77f9c2d92ec3abc0b683150a71 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 16:29:27 +0200 Subject: [PATCH 03/16] Add a FstabTruncate options to handle existing fstab --- internal/imagedefinition/README.rst | 4 +++ internal/imagedefinition/image_definition.go | 1 + internal/statemachine/classic_states.go | 28 ++++++++++++++++++-- internal/statemachine/classic_test.go | 3 ++- 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/internal/imagedefinition/README.rst b/internal/imagedefinition/README.rst index db35ed91..6edb1e02 100644 --- a/internal/imagedefinition/README.rst +++ b/internal/imagedefinition/README.rst @@ -256,6 +256,10 @@ The following specification defines what is supported in the YAML: dump: (optional) # the order to fsck the filesystem fsck-order: + # Define what to do with the existing (if any) fstab file. + # Default to "false", which will keep existing entries and append new + # ones (if any) + fstab_truncate: (optional) artifacts: # Used to specify that ubuntu-image should create a .img file. img: (optional) diff --git a/internal/imagedefinition/image_definition.go b/internal/imagedefinition/image_definition.go index 70337121..50040cdb 100644 --- a/internal/imagedefinition/image_definition.go +++ b/internal/imagedefinition/image_definition.go @@ -76,6 +76,7 @@ type Customization struct { ExtraPackages []*Package `yaml:"extra-packages" json:"ExtraPackages,omitempty" extra_step_prebuilt_rootfs:"install_extra_packages"` ExtraSnaps []*Snap `yaml:"extra-snaps" json:"ExtraSnaps,omitempty" extra_step_prebuilt_rootfs:"install_extra_snaps"` Fstab []*Fstab `yaml:"fstab" json:"Fstab,omitempty"` + FstabTruncate bool `yaml:"fstab_truncate" json:"FstabTruncate,omitempty" default:"false"` Manual *Manual `yaml:"manual" json:"Manual,omitempty"` } diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index ea7bc5a7..275ec3e5 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -1017,9 +1017,25 @@ func (stateMachine *StateMachine) customizeCloudInit() error { func (stateMachine *StateMachine) customizeFstab() error { classicStateMachine := stateMachine.parent.(*ClassicStateMachine) + fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab") + // open /etc/fstab for writing - fstabIO, err := osOpenFile(filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab"), - os.O_CREATE|os.O_WRONLY, 0644) + flags := os.O_CREATE | os.O_RDWR + if classicStateMachine.ImageDef.Customization.FstabTruncate { + flags = flags | os.O_TRUNC + } else { + flags = flags | os.O_APPEND + } + + // check if fstab is empty + info, err := os.Stat(fstabPath) // an error means the file do not exist, which is fine + emptyFstab := true + + if err == nil && info.Size() != 0 { + emptyFstab = false + } + + fstabIO, err := osOpenFile(fstabPath, flags, 0644) if err != nil { return fmt.Errorf("Error opening fstab: %s", err.Error()) } @@ -1043,6 +1059,14 @@ func (stateMachine *StateMachine) customizeFstab() error { ) fstabEntries = append(fstabEntries, fstabEntry) } + + if !classicStateMachine.ImageDef.Customization.FstabTruncate && !emptyFstab { + _, err = fstabIO.Write([]byte("\n")) + if err != nil { + return err + } + } + _, err = fstabIO.Write([]byte(strings.Join(fstabEntries, "\n") + "\n")) return err diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 94fe8bcb..337fb9b8 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -3631,7 +3631,8 @@ func TestCustomizeFstab(t *testing.T) { FsckOrder: 1, }, }, - expectedFstab: `LABEL=writable / ext4 defaults 1 1 + expectedFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1 +LABEL=writable / ext4 defaults 1 1 `, existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, }, From 38d8d9669e79e4fd0027bb5dea7434c44b97d6f0 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 16:38:02 +0200 Subject: [PATCH 04/16] Update changelog --- debian/changelog | 1 + 1 file changed, 1 insertion(+) diff --git a/debian/changelog b/debian/changelog index 07add5f8..922d981d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -33,6 +33,7 @@ ubuntu-image (3.0+23.04ubuntu1) UNRELEASED; urgency=medium * Add experimental ubuntu-image 'pack' support * Support the keep-enabled parameter in ubuntu-image extra-ppas * Clean image build leftovers + * Make sure the fstab file is cleanly overridden (LP: #2031889) [ Alfonso Sanchez-Beato ] * Update to latest snapd, adapting to changes in layouts code. From 1442496db396f7ecfd494206b3fd547f2aa1e85f Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 16:38:20 +0200 Subject: [PATCH 05/16] Fix documentation-check github action --- .github/workflows/documentation.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/documentation.yml b/.github/workflows/documentation.yml index 4b37efa4..fb992d7f 100644 --- a/.github/workflows/documentation.yml +++ b/.github/workflows/documentation.yml @@ -7,20 +7,20 @@ jobs: runs-on: ubuntu-latest name: documentation-check steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0 - name: Get info on if image definition has changed id: changed-files-image-def - uses: tj-actions/changed-files@v32 + uses: tj-actions/changed-files@v39 with: files: | internal/imagedefinition/image_definition.go - name: Get info on if image definition README was updated id: changed-files-image-def-readme - uses: tj-actions/changed-files@v32 + uses: tj-actions/changed-files@v39 with: files: | internal/imagedefinition/README.rst @@ -30,24 +30,24 @@ jobs: run: | echo "Struct has changed" - name: test struct README - if: steps.changed-files-image-def-reamde.outputs.any_changed != 'true' + if: steps.changed-files-image-def-readme.outputs.any_changed != 'true' run: | echo "README has not changed" - name: Fail if image definition README has not been updated - if: steps.changed-files-image-def.outputs.any_changed == 'true' && steps.changed-files-image-def-readme.outputs.any_changed != true + if: steps.changed-files-image-def.outputs.any_changed == 'true' && steps.changed-files-image-def-readme.outputs.any_changed != 'true' run: | echo "Image Definition struct has changed but README was not updated" exit 1 - name: Get info on if command options or flags have changed id: changed-files-flags - uses: tj-actions/changed-files@v32 + uses: tj-actions/changed-files@v39 with: files: | internal/commands/* - name: Fail if command line args have changed but manpage has not been updated - if: steps.changed-files-flags.outputs.any_changed == 'true' && contains(steps.changed-files-flags.outputs.changed_files, 'ubuntu-image.rst') != true + if: steps.changed-files-flags.outputs.any_changed == 'true' && contains(steps.changed-files-flags.outputs.changed_files, 'ubuntu-image.rst') != 'true' run: | echo "Command line flags have been updated but the manpage has not" exit 1 From 3b0e75bc29596f68b9958625b698815292b7c1a6 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Mon, 25 Sep 2023 17:02:14 +0200 Subject: [PATCH 06/16] Exclude new Customization option from states generation --- internal/statemachine/helper.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/statemachine/helper.go b/internal/statemachine/helper.go index 282af199..3e9d53b8 100644 --- a/internal/statemachine/helper.go +++ b/internal/statemachine/helper.go @@ -900,6 +900,11 @@ func checkCustomizationSteps(searchStruct interface{}, tag string) (extraStates elem := value.Elem() for i := 0; i < elem.NumField(); i++ { field := elem.Field(i) + + if field.Kind() == reflect.Bool { + continue + } + if !field.IsNil() { tags := elem.Type().Field(i).Tag tagValue, hasTag := tags.Lookup(tag) From c7898e5d8ca41585e296a121f51b0a42b7cb6918 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Tue, 26 Sep 2023 09:07:43 +0200 Subject: [PATCH 07/16] Cover new cases for customizeFstab and populateClassicRootfsContents --- internal/statemachine/classic_test.go | 63 ++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 337fb9b8..905e5411 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -1855,9 +1855,9 @@ func TestFailedPrepareClassicImage(t *testing.T) { }) } -// TestPopulateClassicRootfsContents runs the state machine through populate_rootfs_contents and examines +// TestStateMachine_PopulateClassicRootfsContents runs the state machine through populate_rootfs_contents and examines // the rootfs to ensure at least some of the correct file are in place -func TestPopulateClassicRootfsContents(t *testing.T) { +func TestStateMachine_PopulateClassicRootfsContents(t *testing.T) { t.Run("test_populate_classic_rootfs_contents", func(t *testing.T) { if runtime.GOARCH != "amd64" { t.Skip("Test for amd64 only") @@ -1875,6 +1875,7 @@ func TestPopulateClassicRootfsContents(t *testing.T) { Rootfs: &imagedefinition.Rootfs{ Archive: "ubuntu", }, + Customization: &imagedefinition.Customization{}, } // need workdir set up for this @@ -1901,13 +1902,28 @@ func TestPopulateClassicRootfsContents(t *testing.T) { } } + // return when Customization.Fstab is not empty + stateMachine.ImageDef.Customization.Fstab = []*imagedefinition.Fstab{ + { + Label: "writable", + Mountpoint: "/", + FSType: "ext4", + MountOptions: "defaults", + Dump: true, + FsckOrder: 1, + }, + } + + err = stateMachine.populateClassicRootfsContents() + asserter.AssertErrNil(err, true) + os.RemoveAll(stateMachine.stateMachineFlags.WorkDir) }) } -// TestFailedPopulateClassicRootfsContents tests failed scenarios in populateClassicRootfsContents +// TestStateMachine_FailedPopulateClassicRootfsContents tests failed scenarios in populateClassicRootfsContents // this is accomplished by mocking functions -func TestFailedPopulateClassicRootfsContents(t *testing.T) { +func TestStateMachine_FailedPopulateClassicRootfsContents(t *testing.T) { t.Run("test_failed_populate_classic_rootfs_contents", func(t *testing.T) { asserter := helper.Asserter{T: t} var stateMachine ClassicStateMachine @@ -1959,6 +1975,24 @@ func TestFailedPopulateClassicRootfsContents(t *testing.T) { asserter.AssertErrContains(err, "Error writing to fstab") osWriteFile = os.WriteFile + // mock os.ReadFile + osReadFile = mockReadFile + defer func() { + osReadFile = os.ReadFile + }() + err = stateMachine.populateClassicRootfsContents() + asserter.AssertErrContains(err, "Error reading fstab") + osReadFile = os.ReadFile + + // return when existing fstab contains LABEL=writable + //nolint:gosec,G306 + err = os.WriteFile(filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab"), + []byte("LABEL=writable\n"), + 0644) + asserter.AssertErrNil(err, true) + err = stateMachine.populateClassicRootfsContents() + asserter.AssertErrNil(err, true) + // create an /etc/resolv.conf.tmp in the chroot err = os.MkdirAll(filepath.Join(stateMachine.tempDirs.chroot, "etc"), 0755) asserter.AssertErrNil(err, true) @@ -3601,6 +3635,7 @@ func TestCustomizeFstab(t *testing.T) { testCases := []struct { name string fstab []*imagedefinition.Fstab + fstabTruncate bool expectedFstab string existingFstab string }{ @@ -3633,6 +3668,23 @@ func TestCustomizeFstab(t *testing.T) { }, expectedFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1 LABEL=writable / ext4 defaults 1 1 +`, + existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, + }, + { + name: "one_entry to a non-empty fstab to be truncated", + fstab: []*imagedefinition.Fstab{ + { + Label: "writable", + Mountpoint: "/", + FSType: "ext4", + MountOptions: "defaults", + Dump: true, + FsckOrder: 1, + }, + }, + fstabTruncate: true, + expectedFstab: `LABEL=writable / ext4 defaults 1 1 `, existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, }, @@ -3689,7 +3741,8 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 Series: getHostSuite(), Rootfs: &imagedefinition.Rootfs{}, Customization: &imagedefinition.Customization{ - Fstab: tc.fstab, + Fstab: tc.fstab, + FstabTruncate: tc.fstabTruncate, }, } From 38c033ed5e33f005f5042f85950bf47151fd5923 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Tue, 17 Oct 2023 09:38:13 +0200 Subject: [PATCH 08/16] Test empty Customization case in populateClassicRootfsContents --- internal/statemachine/classic_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 905e5411..bc09f80f 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -1917,6 +1917,12 @@ func TestStateMachine_PopulateClassicRootfsContents(t *testing.T) { err = stateMachine.populateClassicRootfsContents() asserter.AssertErrNil(err, true) + // return when no Customization + stateMachine.ImageDef.Customization = nil + + err = stateMachine.populateClassicRootfsContents() + asserter.AssertErrNil(err, true) + os.RemoveAll(stateMachine.stateMachineFlags.WorkDir) }) } From 8adbe30d34354f4003c8a5d87e8faffdb3356cca Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Tue, 17 Oct 2023 09:38:18 +0200 Subject: [PATCH 09/16] Lint --- internal/statemachine/classic_states.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index 275ec3e5..1f30ad41 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -28,8 +28,8 @@ import ( ) var ( - seedVersionRegex = regexp.MustCompile(`^[a-z0-9].*`) - fstabRegex = regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`) + seedVersionRegex = regexp.MustCompile(`^[a-z0-9].*`) + fstabRegex = regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`) localePresentRegex = regexp.MustCompile(`(?m)^LANG=|LC_[A-Z_]+=`) ) From 9ac37a5564ae961b8d3f34cf710f0300e4fd5e3b Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Wed, 18 Oct 2023 18:23:53 +0200 Subject: [PATCH 10/16] Properly fix fstab --- internal/statemachine/classic_states.go | 51 ++++++++++-- internal/statemachine/classic_test.go | 100 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 7 deletions(-) diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index 1f30ad41..40a0c76d 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -29,7 +29,6 @@ import ( var ( seedVersionRegex = regexp.MustCompile(`^[a-z0-9].*`) - fstabRegex = regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`) localePresentRegex = regexp.MustCompile(`(?m)^LANG=|LC_[A-Z_]+=`) ) @@ -1320,6 +1319,13 @@ func (stateMachine *StateMachine) populateClassicRootfsContents() error { return nil } + return classicStateMachine.fixFstab() +} + +// fixFstab makes sure the fstab contains a valid entry for the root mount point +func (stateMachine *StateMachine) fixFstab() error { + classicStateMachine := stateMachine.parent.(*ClassicStateMachine) + if len(classicStateMachine.ImageDef.Customization.Fstab) != 0 { return nil } @@ -1330,14 +1336,45 @@ func (stateMachine *StateMachine) populateClassicRootfsContents() error { return fmt.Errorf("Error reading fstab: %s", err.Error()) } - if strings.Contains(string(fstabBytes), "LABEL=writable") { - return nil + rootMountFound := false + newLines := make([]string, 0) + rootFSLabel := "writable" + rootFSOptions := "discard,errors=remount-ro" + fsckOrder := "1" + + lines := strings.Split(string(fstabBytes), "\n") + for _, l := range lines { + if l == "# UNCONFIGURED FSTAB" { + // omit this line if still present + continue + } + + if strings.HasPrefix(l, "#") { + newLines = append(newLines, l) + continue + } + + entry := strings.Fields(l) + if len(entry) < 6 { + // ignore invalid fstab entry + continue + } + + if entry[1] == "/" && !rootMountFound { + entry[0] = "LABEL=" + rootFSLabel + entry[3] = rootFSOptions + entry[5] = fsckOrder + + rootMountFound = true + } + newLines = append(newLines, strings.Join(entry, "\t")) } - newContents := fstabRegex.ReplaceAll(fstabBytes, []byte("LABEL=writable\t/\t$1")) - if !strings.Contains(string(newContents), "LABEL=writable") { - newContents = []byte("LABEL=writable / ext4 defaults 0 0\n") + + if !rootMountFound { + newLines = append(newLines, fmt.Sprintf("LABEL=%s / ext4 %s 0 %s", rootFSLabel, rootFSOptions, fsckOrder)) } - err = osWriteFile(fstabPath, newContents, 0644) + + err = osWriteFile(fstabPath, []byte(strings.Join(newLines, "\n")+"\n"), 0644) if err != nil { return fmt.Errorf("Error writing to fstab: %s", err.Error()) } diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index bc09f80f..8c2af7f5 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -2016,6 +2016,106 @@ func TestStateMachine_FailedPopulateClassicRootfsContents(t *testing.T) { }) } +// TestSateMachine_fixFstab tests functionality of the fixFstab function +func TestSateMachine_fixFstab(t *testing.T) { + testCases := []struct { + name string + existingFstab string + expectedFstab string + }{ + { + name: "add entry to an existing but empty fstab", + existingFstab: "# UNCONFIGURED FSTAB", + expectedFstab: `LABEL=writable / ext4 discard,errors=remount-ro 0 1 +`, + }, + { + name: "fix existing entry amongst several others", + existingFstab: `# /etc/fstab: static file system information. +UUID=1565-1398 / ext4 defaults 0 0 +#Here is another comment that should be left in place +/dev/mapper/vgubuntu-swap_1 none swap sw 0 0 +`, + expectedFstab: `# /etc/fstab: static file system information. +LABEL=writable / ext4 discard,errors=remount-ro 0 1 +#Here is another comment that should be left in place +/dev/mapper/vgubuntu-swap_1 none swap sw 0 0 +`, + }, + { + name: "fix existing entry amongst several others (with spaces)", + existingFstab: `# /etc/fstab: static file system information. +UUID=1565-1398 / ext4 defaults 0 0 +/dev/mapper/vgubuntu-swap_1 none swap sw 0 0 +`, + expectedFstab: `# /etc/fstab: static file system information. +LABEL=writable / ext4 discard,errors=remount-ro 0 1 +/dev/mapper/vgubuntu-swap_1 none swap sw 0 0 +`, + }, + { + name: "fix only one root mount point", + existingFstab: `# /etc/fstab: static file system information. +UUID=1565-1398 / ext4 defaults 0 0 +UUID=1234-5678 / ext4 defaults 0 0 +`, + expectedFstab: `# /etc/fstab: static file system information. +LABEL=writable / ext4 discard,errors=remount-ro 0 1 +UUID=1234-5678 / ext4 defaults 0 0 +`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + asserter := helper.Asserter{T: t} + saveCWD := helper.SaveCWD() + defer saveCWD() + + var stateMachine ClassicStateMachine + stateMachine.commonFlags, stateMachine.stateMachineFlags = helper.InitCommonOpts() + stateMachine.parent = &stateMachine + stateMachine.ImageDef = imagedefinition.ImageDefinition{ + Architecture: getHostArch(), + Series: getHostSuite(), + Rootfs: &imagedefinition.Rootfs{}, + Customization: &imagedefinition.Customization{}, + } + + // set the defaults for the imageDef + err := helper.SetDefaults(&stateMachine.ImageDef) + asserter.AssertErrNil(err, true) + + // need workdir set up for this + err = stateMachine.makeTemporaryDirectories() + asserter.AssertErrNil(err, true) + + // create the /etc directory + err = os.MkdirAll(filepath.Join(stateMachine.tempDirs.rootfs, "etc"), 0644) + asserter.AssertErrNil(err, true) + + fstabPath := filepath.Join(stateMachine.tempDirs.rootfs, "etc", "fstab") + + // simulate an already existing fstab file + if len(tc.existingFstab) != 0 { + err = osWriteFile(fstabPath, []byte(tc.existingFstab), 0644) + asserter.AssertErrNil(err, true) + } + + err = stateMachine.fixFstab() + asserter.AssertErrNil(err, true) + + fstabBytes, err := os.ReadFile(fstabPath) + asserter.AssertErrNil(err, true) + + if string(fstabBytes) != tc.expectedFstab { + t.Errorf("Expected fstab content \"%s\", but got \"%s\"", + tc.expectedFstab, string(fstabBytes)) + } + }) + } +} + // TestGeneratePackageManifest tests if classic image manifest generation works func TestGeneratePackageManifest(t *testing.T) { t.Run("test_generate_package_manifest", func(t *testing.T) { From c26154508631227faed3283f07ccff7aee7e2daf Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Thu, 19 Oct 2023 09:03:12 +0200 Subject: [PATCH 11/16] Convert FstabTruncate to a pointer to properly handle default value --- internal/imagedefinition/error.go | 3 ++- internal/imagedefinition/image_definition.go | 2 +- internal/statemachine/classic_states.go | 8 ++++++-- internal/statemachine/classic_test.go | 18 +++++++++++------- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/internal/imagedefinition/error.go b/internal/imagedefinition/error.go index 19331d2d..17a7385c 100644 --- a/internal/imagedefinition/error.go +++ b/internal/imagedefinition/error.go @@ -3,5 +3,6 @@ package imagedefinition import "errors" var ( - ErrKeepEnabledNil = errors.New("KeepEnabled is nil. Thi value cannot be properly used.") + ErrKeepEnabledNil = errors.New("KeepEnabled is nil. Thi value cannot be properly used.") + ErrFstabTruncateNil = errors.New("FstabTruncate is nil. Thi value cannot be properly used.") ) diff --git a/internal/imagedefinition/image_definition.go b/internal/imagedefinition/image_definition.go index 50040cdb..e3d42f93 100644 --- a/internal/imagedefinition/image_definition.go +++ b/internal/imagedefinition/image_definition.go @@ -76,7 +76,7 @@ type Customization struct { ExtraPackages []*Package `yaml:"extra-packages" json:"ExtraPackages,omitempty" extra_step_prebuilt_rootfs:"install_extra_packages"` ExtraSnaps []*Snap `yaml:"extra-snaps" json:"ExtraSnaps,omitempty" extra_step_prebuilt_rootfs:"install_extra_snaps"` Fstab []*Fstab `yaml:"fstab" json:"Fstab,omitempty"` - FstabTruncate bool `yaml:"fstab_truncate" json:"FstabTruncate,omitempty" default:"false"` + FstabTruncate *bool `yaml:"fstab_truncate" json:"FstabTruncate,omitempty" default:"false"` Manual *Manual `yaml:"manual" json:"Manual,omitempty"` } diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index 40a0c76d..5fee454e 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -1018,9 +1018,13 @@ func (stateMachine *StateMachine) customizeFstab() error { fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab") + if classicStateMachine.ImageDef.Customization.FstabTruncate == nil { + return imagedefinition.ErrFstabTruncateNil + } + // open /etc/fstab for writing flags := os.O_CREATE | os.O_RDWR - if classicStateMachine.ImageDef.Customization.FstabTruncate { + if *classicStateMachine.ImageDef.Customization.FstabTruncate { flags = flags | os.O_TRUNC } else { flags = flags | os.O_APPEND @@ -1059,7 +1063,7 @@ func (stateMachine *StateMachine) customizeFstab() error { fstabEntries = append(fstabEntries, fstabEntry) } - if !classicStateMachine.ImageDef.Customization.FstabTruncate && !emptyFstab { + if !*classicStateMachine.ImageDef.Customization.FstabTruncate && !emptyFstab { _, err = fstabIO.Write([]byte("\n")) if err != nil { return err diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 8c2af7f5..4f5acf42 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -3792,7 +3792,7 @@ LABEL=writable / ext4 defaults 1 1 fstabTruncate: true, expectedFstab: `LABEL=writable / ext4 defaults 1 1 `, - existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, + existingFstab: `LABEL=xxx /test ext4 discard,errors=remount-ro 0 1`, }, { name: "two_entries", @@ -3848,7 +3848,7 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 Rootfs: &imagedefinition.Rootfs{}, Customization: &imagedefinition.Customization{ Fstab: tc.fstab, - FstabTruncate: tc.fstabTruncate, + FstabTruncate: helper.BoolPtr(tc.fstabTruncate), }, } @@ -3887,12 +3887,12 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 } } -// TestFailedCustomizeFstab tests failures in the customizeFstab function -func TestFailedCustomizeFstab(t *testing.T) { +// TestStateMachine_customizeFstab_fail tests failures in the customizeFstab function +func TestStateMachine_customizeFstab_fail(t *testing.T) { t.Run("test_failed_customize_fstab", func(t *testing.T) { asserter := helper.Asserter{T: t} - saveCWD := helper.SaveCWD() - defer saveCWD() + restoreCWD := helper.SaveCWD() + defer restoreCWD() var stateMachine ClassicStateMachine stateMachine.commonFlags, stateMachine.stateMachineFlags = helper.InitCommonOpts() @@ -3912,10 +3912,10 @@ func TestFailedCustomizeFstab(t *testing.T) { FsckOrder: 1, }, }, + FstabTruncate: helper.BoolPtr(true), }, } - // mock os.OpenFile osOpenFile = mockOpenFile defer func() { osOpenFile = os.OpenFile @@ -3923,6 +3923,10 @@ func TestFailedCustomizeFstab(t *testing.T) { err := stateMachine.customizeFstab() asserter.AssertErrContains(err, "Error opening fstab") osOpenFile = os.OpenFile + + stateMachine.ImageDef.Customization.FstabTruncate = nil + err = stateMachine.customizeFstab() + asserter.AssertErrContains(err, imagedefinition.ErrFstabTruncateNil.Error()) }) } From 62928ee12f6fe2ba9ac1724b00db361ca4eadd92 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Fri, 20 Oct 2023 09:19:55 +0200 Subject: [PATCH 12/16] Set osMock in a single dedicated place --- internal/statemachine/classic_test.go | 54 ------------------ internal/statemachine/pack_test.go | 45 --------------- internal/statemachine/tests_helper_test.go | 65 ++++++++++++++++++++++ 3 files changed, 65 insertions(+), 99 deletions(-) create mode 100644 internal/statemachine/tests_helper_test.go diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 4f5acf42..3ebf9434 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "io/fs" "os" "os/exec" "path" @@ -4635,59 +4634,6 @@ func TestClassicStateMachine_cleanRootfs_real_rootfs(t *testing.T) { }) } -type osMockConf struct { - osutilCopySpecialFileThreshold uint - ReadDirThreshold uint - RemoveThreshold uint - TruncateThreshold uint -} - -type osMock struct { - conf *osMockConf - beforeOsutilCopySpecialFileFail uint - beforeReadDirFail uint - beforeRemoveFail uint - beforeTruncateFail uint -} - -func (o *osMock) CopySpecialFile(path, dest string) error { - if o.beforeOsutilCopySpecialFileFail >= o.conf.osutilCopySpecialFileThreshold { - return fmt.Errorf("CopySpecialFile fail") - } - o.beforeOsutilCopySpecialFileFail++ - return nil -} - -func (o *osMock) ReadDir(name string) ([]fs.DirEntry, error) { - if o.beforeReadDirFail >= o.conf.ReadDirThreshold { - return nil, fmt.Errorf("ReadDir fail") - } - o.beforeReadDirFail++ - return []fs.DirEntry{}, nil -} - -func (o *osMock) Remove(name string) error { - if o.beforeRemoveFail >= o.conf.RemoveThreshold { - return fmt.Errorf("Remove fail") - } - o.beforeRemoveFail++ - - return nil -} - -func (o *osMock) Truncate(name string, size int64) error { - if o.beforeTruncateFail >= o.conf.TruncateThreshold { - return fmt.Errorf("Truncate fail") - } - o.beforeTruncateFail++ - - return nil -} - -func NewOSMock(conf *osMockConf) *osMock { - return &osMock{conf: conf} -} - func TestClassicStateMachine_cleanRootfs(t *testing.T) { sampleContent := "test" sampleSize := int64(len(sampleContent)) diff --git a/internal/statemachine/pack_test.go b/internal/statemachine/pack_test.go index 095d7570..bf5be955 100644 --- a/internal/statemachine/pack_test.go +++ b/internal/statemachine/pack_test.go @@ -2,7 +2,6 @@ package statemachine import ( "fmt" - "io/fs" "os" "os/exec" "path/filepath" @@ -15,50 +14,6 @@ import ( "github.com/canonical/ubuntu-image/internal/helper" ) -type osMockConf struct { - osutilCopySpecialFileThreshold uint - ReadDirThreshold uint - RemoveThreshold uint -} - -type osMock struct { - conf *osMockConf - beforeOsutilCopySpecialFileFail uint - beforeReadDirFail uint - beforeRemoveFail uint -} - -func (o *osMock) CopySpecialFile(path, dest string) error { - if o.beforeOsutilCopySpecialFileFail >= o.conf.osutilCopySpecialFileThreshold { - return fmt.Errorf("CopySpecialFile fail") - } - o.beforeOsutilCopySpecialFileFail++ - - return nil -} - -func (o *osMock) ReadDir(name string) ([]fs.DirEntry, error) { - if o.beforeReadDirFail >= o.conf.ReadDirThreshold { - return nil, fmt.Errorf("ReadDir fail") - } - o.beforeReadDirFail++ - - return []fs.DirEntry{}, nil -} - -func (o *osMock) Remove(name string) error { - if o.beforeRemoveFail >= o.conf.RemoveThreshold { - return fmt.Errorf("Remove fail") - } - o.beforeRemoveFail++ - - return nil -} - -func NewOSMock(conf *osMockConf) *osMock { - return &osMock{conf: conf} -} - func TestPack_Setup(t *testing.T) { t.Run("test_classic_setup", func(t *testing.T) { asserter := helper.Asserter{T: t} diff --git a/internal/statemachine/tests_helper_test.go b/internal/statemachine/tests_helper_test.go new file mode 100644 index 00000000..7bfdd568 --- /dev/null +++ b/internal/statemachine/tests_helper_test.go @@ -0,0 +1,65 @@ +package statemachine + +import ( + "fmt" + "io/fs" +) + +type osMockConf struct { + osutilCopySpecialFileThreshold uint + ReadDirThreshold uint + RemoveThreshold uint + TruncateThreshold uint +} + +// osMock holds methods to easily mock functions from os and snapd/osutil packages +// Each method can be configured to fail after a given number of calls +// This could be improved by letting the mock functions calls the real +// functions before failing. +type osMock struct { + conf *osMockConf + beforeOsutilCopySpecialFileFail uint + beforeReadDirFail uint + beforeRemoveFail uint + beforeTruncateFail uint +} + +func (o *osMock) CopySpecialFile(path, dest string) error { + if o.beforeOsutilCopySpecialFileFail >= o.conf.osutilCopySpecialFileThreshold { + return fmt.Errorf("CopySpecialFile fail") + } + o.beforeOsutilCopySpecialFileFail++ + + return nil +} + +func (o *osMock) ReadDir(name string) ([]fs.DirEntry, error) { + if o.beforeReadDirFail >= o.conf.ReadDirThreshold { + return nil, fmt.Errorf("ReadDir fail") + } + o.beforeReadDirFail++ + + return []fs.DirEntry{}, nil +} + +func (o *osMock) Remove(name string) error { + if o.beforeRemoveFail >= o.conf.RemoveThreshold { + return fmt.Errorf("Remove fail") + } + o.beforeRemoveFail++ + + return nil +} + +func (o *osMock) Truncate(name string, size int64) error { + if o.beforeTruncateFail >= o.conf.TruncateThreshold { + return fmt.Errorf("Truncate fail") + } + o.beforeTruncateFail++ + + return nil +} + +func NewOSMock(conf *osMockConf) *osMock { + return &osMock{conf: conf} +} From 6d72feb0eecfb67257146e9dba0332447c32a95e Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Fri, 20 Oct 2023 14:32:51 +0200 Subject: [PATCH 13/16] Fix SetDefaults for pointer to boolean with true as default value Tests were inaccurate. The default value was always set for pointers to bool --- internal/helper/helper.go | 10 ++++++---- internal/helper/helper_test.go | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/helper/helper.go b/internal/helper/helper.go index aed21afa..e44ee41c 100644 --- a/internal/helper/helper.go +++ b/internal/helper/helper.go @@ -135,14 +135,16 @@ func SetDefaults(needsDefaults interface{}) error { } // special case for pointer to bools } else if field.Type().Elem() == reflect.TypeOf(true) { - // make sure the pointer is never nil in case no value - // was set - if field.IsNil() { - field.Set(reflect.ValueOf(BoolPtr(false))) + // if a value is set, do nothing + if !field.IsNil() { + continue } tags := elem.Type().Field(i).Tag defaultValue, hasDefault := tags.Lookup("default") if !hasDefault { + // If no default and no value is set, make sure we have a valid + // value consistent with the "zero" value for a bool (false) + field.Set(reflect.ValueOf(BoolPtr(false))) continue } if defaultValue == "true" { diff --git a/internal/helper/helper_test.go b/internal/helper/helper_test.go index 58532fa9..6dc6e721 100644 --- a/internal/helper/helper_test.go +++ b/internal/helper/helper_test.go @@ -230,7 +230,7 @@ func TestSetDefaults(t *testing.T) { }, want: &S2{ A: "test", - B: BoolPtr(true), + B: BoolPtr(false), C: BoolPtr(false), D: BoolPtr(true), }, From 96f2ac5575826020d1c327ed0b80e29eb76b1f44 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Fri, 20 Oct 2023 14:47:20 +0200 Subject: [PATCH 14/16] Prevent from displaying help twice --- cmd/ubuntu-image/main.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/ubuntu-image/main.go b/cmd/ubuntu-image/main.go index b77005a0..05afa50a 100644 --- a/cmd/ubuntu-image/main.go +++ b/cmd/ubuntu-image/main.go @@ -71,6 +71,11 @@ func executeStateMachine(sm statemachine.SmInterface) error { // unhidePackOpts make pack options visible in help if the pack command is used // This should be removed when the pack command is made visible to everyone func unhidePackOpts(parser *flags.Parser) { + // Save given options before removing them temporarily + // otherwise the help will be displayed twice + opts := parser.Options + parser.Options = 0 + defer func() { parser.Options = opts }() // parse once to determine the active command // we do not care about error here since we will reparse again _, _ = parser.Parse() From 381615bef2a1c5b445df9f40d6df9c77881f9d11 Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Wed, 25 Oct 2023 11:54:20 +0200 Subject: [PATCH 15/16] Remove FstabTruncate option --- internal/imagedefinition/README.rst | 5 +- internal/imagedefinition/error.go | 3 +- internal/imagedefinition/image_definition.go | 1 - internal/statemachine/classic_states.go | 29 +----------- internal/statemachine/classic_test.go | 49 ++------------------ internal/statemachine/helper.go | 4 -- 6 files changed, 8 insertions(+), 83 deletions(-) diff --git a/internal/imagedefinition/README.rst b/internal/imagedefinition/README.rst index 6edb1e02..44da978d 100644 --- a/internal/imagedefinition/README.rst +++ b/internal/imagedefinition/README.rst @@ -242,6 +242,7 @@ The following specification defines what is supported in the YAML: # ubuntu-image will support creating many different types of # artifacts, including the actual images, manifest files, # changelogs, and a list of files in the rootfs. + # Set a custom fstab. The existing one (if any) will be truncated. fstab: (optional) - # the value of LABEL= for the fstab entry @@ -256,10 +257,6 @@ The following specification defines what is supported in the YAML: dump: (optional) # the order to fsck the filesystem fsck-order: - # Define what to do with the existing (if any) fstab file. - # Default to "false", which will keep existing entries and append new - # ones (if any) - fstab_truncate: (optional) artifacts: # Used to specify that ubuntu-image should create a .img file. img: (optional) diff --git a/internal/imagedefinition/error.go b/internal/imagedefinition/error.go index 17a7385c..19331d2d 100644 --- a/internal/imagedefinition/error.go +++ b/internal/imagedefinition/error.go @@ -3,6 +3,5 @@ package imagedefinition import "errors" var ( - ErrKeepEnabledNil = errors.New("KeepEnabled is nil. Thi value cannot be properly used.") - ErrFstabTruncateNil = errors.New("FstabTruncate is nil. Thi value cannot be properly used.") + ErrKeepEnabledNil = errors.New("KeepEnabled is nil. Thi value cannot be properly used.") ) diff --git a/internal/imagedefinition/image_definition.go b/internal/imagedefinition/image_definition.go index e3d42f93..70337121 100644 --- a/internal/imagedefinition/image_definition.go +++ b/internal/imagedefinition/image_definition.go @@ -76,7 +76,6 @@ type Customization struct { ExtraPackages []*Package `yaml:"extra-packages" json:"ExtraPackages,omitempty" extra_step_prebuilt_rootfs:"install_extra_packages"` ExtraSnaps []*Snap `yaml:"extra-snaps" json:"ExtraSnaps,omitempty" extra_step_prebuilt_rootfs:"install_extra_snaps"` Fstab []*Fstab `yaml:"fstab" json:"Fstab,omitempty"` - FstabTruncate *bool `yaml:"fstab_truncate" json:"FstabTruncate,omitempty" default:"false"` Manual *Manual `yaml:"manual" json:"Manual,omitempty"` } diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index 5fee454e..efabc9d7 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -1018,27 +1018,7 @@ func (stateMachine *StateMachine) customizeFstab() error { fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab") - if classicStateMachine.ImageDef.Customization.FstabTruncate == nil { - return imagedefinition.ErrFstabTruncateNil - } - - // open /etc/fstab for writing - flags := os.O_CREATE | os.O_RDWR - if *classicStateMachine.ImageDef.Customization.FstabTruncate { - flags = flags | os.O_TRUNC - } else { - flags = flags | os.O_APPEND - } - - // check if fstab is empty - info, err := os.Stat(fstabPath) // an error means the file do not exist, which is fine - emptyFstab := true - - if err == nil && info.Size() != 0 { - emptyFstab = false - } - - fstabIO, err := osOpenFile(fstabPath, flags, 0644) + fstabIO, err := osOpenFile(fstabPath, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0644) if err != nil { return fmt.Errorf("Error opening fstab: %s", err.Error()) } @@ -1063,13 +1043,6 @@ func (stateMachine *StateMachine) customizeFstab() error { fstabEntries = append(fstabEntries, fstabEntry) } - if !*classicStateMachine.ImageDef.Customization.FstabTruncate && !emptyFstab { - _, err = fstabIO.Write([]byte("\n")) - if err != nil { - return err - } - } - _, err = fstabIO.Write([]byte(strings.Join(fstabEntries, "\n") + "\n")) return err diff --git a/internal/statemachine/classic_test.go b/internal/statemachine/classic_test.go index 3ebf9434..cffe4da0 100644 --- a/internal/statemachine/classic_test.go +++ b/internal/statemachine/classic_test.go @@ -3740,12 +3740,11 @@ func TestCustomizeFstab(t *testing.T) { testCases := []struct { name string fstab []*imagedefinition.Fstab - fstabTruncate bool expectedFstab string existingFstab string }{ { - name: "one_entry to an empty fstab", + name: "one entry to an empty fstab", fstab: []*imagedefinition.Fstab{ { Label: "writable", @@ -3760,7 +3759,7 @@ func TestCustomizeFstab(t *testing.T) { `, }, { - name: "one_entry to a non-empty fstab", + name: "one entry to a non-empty fstab", fstab: []*imagedefinition.Fstab{ { Label: "writable", @@ -3771,30 +3770,12 @@ func TestCustomizeFstab(t *testing.T) { FsckOrder: 1, }, }, - expectedFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1 -LABEL=writable / ext4 defaults 1 1 -`, - existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, - }, - { - name: "one_entry to a non-empty fstab to be truncated", - fstab: []*imagedefinition.Fstab{ - { - Label: "writable", - Mountpoint: "/", - FSType: "ext4", - MountOptions: "defaults", - Dump: true, - FsckOrder: 1, - }, - }, - fstabTruncate: true, expectedFstab: `LABEL=writable / ext4 defaults 1 1 `, - existingFstab: `LABEL=xxx /test ext4 discard,errors=remount-ro 0 1`, + existingFstab: `LABEL=xxx / ext4 discard,errors=remount-ro 0 1`, }, { - name: "two_entries", + name: "two entries", fstab: []*imagedefinition.Fstab{ { Label: "writable", @@ -3815,19 +3796,6 @@ LABEL=writable / ext4 defaults 1 1 }, expectedFstab: `LABEL=writable / ext4 defaults 0 1 LABEL=system-boot /boot/firmware vfat defaults 0 1 -`, - }, - { - name: "defaults_assumed", - fstab: []*imagedefinition.Fstab{ - { - Label: "writable", - Mountpoint: "/", - FSType: "ext4", - FsckOrder: 1, - }, - }, - expectedFstab: `LABEL=writable / ext4 defaults 0 1 `, }, } @@ -3846,8 +3814,7 @@ LABEL=system-boot /boot/firmware vfat defaults 0 1 Series: getHostSuite(), Rootfs: &imagedefinition.Rootfs{}, Customization: &imagedefinition.Customization{ - Fstab: tc.fstab, - FstabTruncate: helper.BoolPtr(tc.fstabTruncate), + Fstab: tc.fstab, }, } @@ -3911,7 +3878,6 @@ func TestStateMachine_customizeFstab_fail(t *testing.T) { FsckOrder: 1, }, }, - FstabTruncate: helper.BoolPtr(true), }, } @@ -3921,11 +3887,6 @@ func TestStateMachine_customizeFstab_fail(t *testing.T) { }() err := stateMachine.customizeFstab() asserter.AssertErrContains(err, "Error opening fstab") - osOpenFile = os.OpenFile - - stateMachine.ImageDef.Customization.FstabTruncate = nil - err = stateMachine.customizeFstab() - asserter.AssertErrContains(err, imagedefinition.ErrFstabTruncateNil.Error()) }) } diff --git a/internal/statemachine/helper.go b/internal/statemachine/helper.go index 3e9d53b8..2d11936f 100644 --- a/internal/statemachine/helper.go +++ b/internal/statemachine/helper.go @@ -901,10 +901,6 @@ func checkCustomizationSteps(searchStruct interface{}, tag string) (extraStates for i := 0; i < elem.NumField(); i++ { field := elem.Field(i) - if field.Kind() == reflect.Bool { - continue - } - if !field.IsNil() { tags := elem.Type().Field(i).Tag tagValue, hasTag := tags.Lookup(tag) From 9a382e9de60169c0254555a842052fed9105247f Mon Sep 17 00:00:00 2001 From: Paul Mars Date: Thu, 26 Oct 2023 08:53:00 +0200 Subject: [PATCH 16/16] Use correct flags to open fstab file --- internal/statemachine/classic_states.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/statemachine/classic_states.go b/internal/statemachine/classic_states.go index efabc9d7..e0ae9719 100644 --- a/internal/statemachine/classic_states.go +++ b/internal/statemachine/classic_states.go @@ -1018,7 +1018,7 @@ func (stateMachine *StateMachine) customizeFstab() error { fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab") - fstabIO, err := osOpenFile(fstabPath, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0644) + fstabIO, err := osOpenFile(fstabPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) if err != nil { return fmt.Errorf("Error opening fstab: %s", err.Error()) }