Merge pull request #143 from canonical/invalid-fstab

FR-5404 - The 'fstab' of 'customization' destroy original fstab content

.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

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()

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.

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" {

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),
 			},

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

internal/statemachine/classic_states.go

@@ -27,8 +27,10 @@ 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].*`)
+	localePresentRegex = regexp.MustCompile(`(?m)^LANG=|LC_[A-Z_]+=`)
+)
 
 // parseImageDefinition parses the provided yaml file and ensures it is valid
 func (stateMachine *StateMachine) parseImageDefinition() error {
@@ -1014,9 +1016,9 @@ func (stateMachine *StateMachine) customizeCloudInit() error {
 func (stateMachine *StateMachine) customizeFstab() error {
 	classicStateMachine := stateMachine.parent.(*ClassicStateMachine)
 
-	// open /etc/fstab for writing
-	fstabIO, err := osOpenFile(filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab"),
-		os.O_CREATE|os.O_WRONLY, 0644)
+	fstabPath := filepath.Join(stateMachine.tempDirs.chroot, "etc", "fstab")
+
+	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())
 	}
@@ -1040,6 +1042,7 @@ func (stateMachine *StateMachine) customizeFstab() error {
 		)
 		fstabEntries = append(fstabEntries, fstabEntry)
 	}
+
 	_, err = fstabIO.Write([]byte(strings.Join(fstabEntries, "\n") + "\n"))
 
 	return err
@@ -1289,24 +1292,68 @@ 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
+	}
+
+	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
+	}
+
+	fstabPath := filepath.Join(classicStateMachine.tempDirs.rootfs, "etc", "fstab")
+	fstabBytes, err := osReadFile(fstabPath)
+	if err != nil {
+		return fmt.Errorf("Error reading fstab: %s", err.Error())
+	}
+
+	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"))
+	}
+
+	if !rootMountFound {
+		newLines = append(newLines, fmt.Sprintf("LABEL=%s	/	ext4	%s	0	%s", rootFSLabel, rootFSOptions, fsckOrder))
+	}
+
+	err = osWriteFile(fstabPath, []byte(strings.Join(newLines, "\n")+"\n"), 0644)
+	if err != nil {
+		return fmt.Errorf("Error writing to fstab: %s", err.Error())
 	}
 	return nil
 }