Merge pull request canonical#217 from canonical/fix-no-volume-in-gadg…

…et-yaml

Improve validation and robustness around volumes management declared in gadget.yaml

README.md

@@ -1,7 +1,7 @@
 # ubuntu-image: build Ubuntu images
 
 [![ubuntu-image](https://snapcraft.io/ubuntu-image/badge.svg)](https://snapcraft.io/ubuntu-image)
-![Build](https://github.com/canonical/ubuntu-image/actions/workflows/build-and-test.yml/badge.svg)
+![Build](https://github.com/canonical/ubuntu-image/actions/workflows/build-and-test.yml/badge.svg?branch=main)
 [![codecov](https://codecov.io/gh/canonical/ubuntu-image/branch/main/graph/badge.svg?token=F9jE9HKo1a)](https://codecov.io/gh/canonical/ubuntu-image)
 [![Go Report Card](https://goreportcard.com/badge/github.com/canonical/ubuntu-image)](https://goreportcard.com/report/github.com/canonical/ubuntu-image)
 

debian/changelog

@@ -3,6 +3,8 @@ ubuntu-image (3.5) UNRELEASED; urgency=medium
   [ Paul Mars ]
   * Update snapd dependency due to a breaking change
   * Fix deb822 format ubuntu.sources security entry
+  * Improve validation and overall handling of volumes from the
+    gadget.yaml. 
 
  -- Paul Mars <[email protected]>  Fri, 19 Apr 2024 15:53:14 +0200
 

internal/statemachine/classic_states.go

@@ -457,30 +457,31 @@ func (stateMachine *StateMachine) prepareImgArtifactsMultipleVolumes(artifacts *
 // The names of these images are placed in the VolumeNames map, which is used
 // as an input file for an eventual `qemu-convert` operation.
 func (stateMachine *StateMachine) prepareQcow2ArtifactsMultipleVolumes(artifacts *imagedefinition.Artifact) error {
-	if artifacts.Qcow2 != nil {
-		for _, qcow2 := range *artifacts.Qcow2 {
-			if qcow2.Qcow2Volume == "" {
-				return fmt.Errorf("Volume names must be specified for each image when using a gadget with more than one volume")
-			}
-			// We can save a whole lot of disk I/O here if the volume is
-			// already specified as a .img file
-			if artifacts.Img != nil {
-				found := false
-				for _, img := range *artifacts.Img {
-					if img.ImgVolume == qcow2.Qcow2Volume {
-						found = true
-					}
-				}
-				if !found {
-					// if a .img artifact for this volume isn't explicitly stated in
-					// the image definition, then create one
-					stateMachine.VolumeNames[qcow2.Qcow2Volume] = fmt.Sprintf("%s.img", qcow2.Qcow2Name)
+	if artifacts.Qcow2 == nil {
+		return nil
+	}
+	for _, qcow2 := range *artifacts.Qcow2 {
+		if qcow2.Qcow2Volume == "" {
+			return fmt.Errorf("Volume names must be specified for each image when using a gadget with more than one volume")
+		}
+		// We can save a whole lot of disk I/O here if the volume is
+		// already specified as a .img file
+		if artifacts.Img != nil {
+			found := false
+			for _, img := range *artifacts.Img {
+				if img.ImgVolume == qcow2.Qcow2Volume {
+					found = true
 				}
-			} else {
-				// no .img artifacts exist in the image definition,
-				// but we still need to create one to convert to qcow2
+			}
+			if !found {
+				// if a .img artifact for this volume isn't explicitly stated in
+				// the image definition, then create one
 				stateMachine.VolumeNames[qcow2.Qcow2Volume] = fmt.Sprintf("%s.img", qcow2.Qcow2Name)
 			}
+		} else {
+			// no .img artifacts exist in the image definition,
+			// but we still need to create one to convert to qcow2
+			stateMachine.VolumeNames[qcow2.Qcow2Volume] = fmt.Sprintf("%s.img", qcow2.Qcow2Name)
 		}
 	}
 	return nil

internal/statemachine/common_states.go

@@ -51,16 +51,26 @@ The gadget.yaml file is expected to be located in a "meta" subdirectory of the p
 		return fmt.Errorf("Error running InfoFromGadgetYaml: %s", err.Error())
 	}
 
+	err = gadget.Validate(stateMachine.GadgetInfo, nil, nil)
+	if err != nil {
+		return fmt.Errorf("invalid gadget: %s", err.Error())
+	}
+
 	// check if the unpack dir should be preserved
 	err = preserveUnpack(stateMachine.tempDirs.unpack)
 	if err != nil {
 		return err
 	}
 
+	err = stateMachine.validateVolumes()
+	if err != nil {
+		return err
+	}
+
 	// for the --image-size argument, the order of the volumes specified in gadget.yaml
 	// must be preserved. However, since gadget.Info stores the volumes as a map, the
 	// order is not preserved. We use the already read-in gadget.yaml file to store the
-	// order of the volumes as an array in the StateMachine struct
+	// order of the volumes in a slice in the StateMachine struct
 	stateMachine.saveVolumeOrder(string(gadgetYamlBytes))
 
 	if err := stateMachine.postProcessGadgetYaml(); err != nil {
@@ -97,6 +107,14 @@ func preserveUnpack(unpackDir string) error {
 	return nil
 }
 
+// validateVolumes checks there is at least one volume in the gadget
+func (stateMachine *StateMachine) validateVolumes() error {
+	if len(stateMachine.GadgetInfo.Volumes) < 1 {
+		return fmt.Errorf("no volume in the gadget.yaml. Specify at least one volume.")
+	}
+	return nil
+}
+
 var generateDiskInfoState = stateFunc{"generate_disk_info", (*StateMachine).generateDiskInfo}
 
 // If --disk-info was used, copy the provided file to the correct location

internal/statemachine/common_test.go

@@ -89,6 +89,14 @@ func TestFailedLoadGadgetYaml(t *testing.T) {
 	err = stateMachine.loadGadgetYaml()
 	asserter.AssertErrContains(err, "Error running InfoFromGadgetYaml")
 
+	stateMachine.YamlFilePath = filepath.Join("testdata", "gadget_no_volumes.yaml")
+	err = stateMachine.loadGadgetYaml()
+	asserter.AssertErrContains(err, "Specify at least one volume.")
+
+	stateMachine.YamlFilePath = filepath.Join("testdata", "gadget_two_seeded_volumes.yaml")
+	err = stateMachine.loadGadgetYaml()
+	asserter.AssertErrContains(err, "invalid gadget:")
+
 	// set a valid yaml file and preserveDir
 	stateMachine.YamlFilePath = filepath.Join("testdata",
 		"gadget_tree", "meta", "gadget.yaml")

internal/statemachine/state_machine.go

@@ -129,6 +129,9 @@ type StateMachine struct {
 	// names of images for each volume
 	VolumeNames map[string]string
 
+	// name of the "main volume"
+	MainVolumeName string
+
 	Packages []string
 	Snaps    []string
 }
@@ -268,21 +271,24 @@ func (stateMachine *StateMachine) saveVolumeOrder(gadgetYamlContents string) {
 	stateMachine.VolumeOrder = sortedVolumes
 }
 
-// postProcessGadgetYaml adds the rootfs to the partitions list if needed
+// postProcessGadgetYaml gathers several addition information from the volumes and
+// operates several fixes on the volumes in the GadgetInfo.
+// - Adds the rootfs to the partitions list if needed
+// - Adds missing content
 func (stateMachine *StateMachine) postProcessGadgetYaml() error {
 	var rootfsSeen bool = false
 	var farthestOffset quantity.Offset
 	var lastOffset quantity.Offset
-	farthestOffsetUnknown := false
-	var volume *gadget.Volume
+	var farthestOffsetUnknown bool = false
+	lastVolumeName := ""
 
 	for _, volumeName := range stateMachine.VolumeOrder {
-		volume = stateMachine.GadgetInfo.Volumes[volumeName]
+		lastVolumeName = volumeName
+		volume := stateMachine.GadgetInfo.Volumes[volumeName]
 		volumeBaseDir := filepath.Join(stateMachine.tempDirs.volumes, volumeName)
 		if err := osMkdirAll(volumeBaseDir, 0755); err != nil {
 			return fmt.Errorf("Error creating volume dir: %s", err.Error())
 		}
-		// look for the rootfs and check if the image is seeded
 		for i := range volume.Structure {
 			structure := &volume.Structure[i]
 			stateMachine.warnUsageOfSystemLabel(volumeName, structure, i)
@@ -291,7 +297,7 @@ func (stateMachine *StateMachine) postProcessGadgetYaml() error {
 				rootfsSeen = true
 			}
 
-			stateMachine.checkSystemSeed(volume, structure, i)
+			stateMachine.handleSystemSeed(volume, structure, i)
 
 			err := checkStructureContent(structure)
 			if err != nil {
@@ -316,7 +322,7 @@ func (stateMachine *StateMachine) postProcessGadgetYaml() error {
 		}
 	}
 
-	fixMissingSystemData(volume, farthestOffset, farthestOffsetUnknown, rootfsSeen, stateMachine.GadgetInfo.Volumes)
+	stateMachine.fixMissingSystemData(lastVolumeName, farthestOffset, farthestOffsetUnknown, rootfsSeen)
 
 	return nil
 }
@@ -329,14 +335,18 @@ func (stateMachine *StateMachine) warnUsageOfSystemLabel(volumeName string, stru
 	}
 }
 
-// checkSystemSeed checks if the struture is a system-seed one and fixes the Label if needed
-func (stateMachine *StateMachine) checkSystemSeed(volume *gadget.Volume, structure *gadget.VolumeStructure, structIndex int) {
-	if structure.Role == gadget.SystemSeed {
-		stateMachine.IsSeeded = true
-		if structure.Label == "" {
-			structure.Label = structure.Name
-			volume.Structure[structIndex] = *structure
-		}
+// handleSystemSeed checks if the struture is a system-seed one and fixes the Label if needed
+func (stateMachine *StateMachine) handleSystemSeed(volume *gadget.Volume, structure *gadget.VolumeStructure, structIndex int) {
+	if structure.Role != gadget.SystemSeed {
+		return
+	}
+	stateMachine.IsSeeded = true
+	// The "main" volume is the one with a system-seed structure
+	stateMachine.MainVolumeName = structure.VolumeName
+
+	if structure.Label == "" {
+		structure.Label = structure.Name
+		volume.Structure[structIndex] = *structure
 	}
 }
 
@@ -387,32 +397,38 @@ func fixMissingContent(volume *gadget.Volume, structure *gadget.VolumeStructure,
 	volume.Structure[structIndex] = *structure
 }
 
-// fixMissingSystemData handles the case of unspecified system-data
-// partition where we simply attach the rootfs at the end of the
-// partition list.
-// Since so far we have no knowledge of the rootfs contents, the
-// size is set to 0, and will be calculated later
-// Note that there is only one volume, so "volume" points to it
-func fixMissingSystemData(volume *gadget.Volume, farthestOffset quantity.Offset, farthestOffsetUnknown bool, rootfsSeen bool, volumes map[string]*gadget.Volume) {
-	if !farthestOffsetUnknown && !rootfsSeen && len(volumes) == 1 {
-		rootfsStructure := gadget.VolumeStructure{
-			Name:       "",
-			Label:      "writable",
-			Offset:     &farthestOffset,
-			Size:       quantity.Size(0),
-			Type:       "83,0FC63DAF-8483-4772-8E79-3D69D8477DE4",
-			Role:       gadget.SystemData,
-			ID:         "",
-			Filesystem: "ext4",
-			Content:    []gadget.VolumeContent{},
-			Update:     gadget.VolumeUpdate{},
-			// "virtual" yaml index for the new structure (it would
-			// be the last one in gadget.yaml)
-			YamlIndex: len(volume.Structure),
-		}
-
-		volume.Structure = append(volume.Structure, rootfsStructure)
+// fixMissingSystemData handles the case of unspecified system-data partition
+// where we simply attach the rootfs at the end of the partition list.
+// Since so far we have no knowledge of the rootfs contents, the size is set
+// to 0, and will be calculated later
+func (stateMachine *StateMachine) fixMissingSystemData(lastVolumeName string, farthestOffset quantity.Offset, farthestOffsetUnknown bool, rootfsSeen bool) {
+	// We only add the structure if there is a single volume because we cannot
+	// be sure which volume is considered the "main one" by the user, even though
+	// we have a way to find it (see comment about the MainVolume field). We
+	// should revisit this if we want to be stricter in the future about that.
+	if farthestOffsetUnknown || rootfsSeen || len(stateMachine.GadgetInfo.Volumes) != 1 {
+		return
 	}
+	// So for now we consider the main volume to be the last one
+	volume := stateMachine.GadgetInfo.Volumes[lastVolumeName]
+
+	rootfsStructure := gadget.VolumeStructure{
+		Name:       "",
+		Label:      "writable",
+		Offset:     &farthestOffset,
+		Size:       quantity.Size(0),
+		Type:       "83,0FC63DAF-8483-4772-8E79-3D69D8477DE4",
+		Role:       gadget.SystemData,
+		ID:         "",
+		Filesystem: "ext4",
+		Content:    []gadget.VolumeContent{},
+		Update:     gadget.VolumeUpdate{},
+		// "virtual" yaml index for the new structure (it would
+		// be the last one in gadget.yaml)
+		YamlIndex: len(volume.Structure),
+	}
+
+	volume.Structure = append(volume.Structure, rootfsStructure)
 }
 
 // readMetadata reads info about a partial state machine encoded as JSON from disk
@@ -465,6 +481,7 @@ func (stateMachine *StateMachine) loadState(partialStateMachine *StateMachine) e
 	stateMachine.ImageSizes = partialStateMachine.ImageSizes
 	stateMachine.VolumeOrder = partialStateMachine.VolumeOrder
 	stateMachine.VolumeNames = partialStateMachine.VolumeNames
+	stateMachine.MainVolumeName = partialStateMachine.MainVolumeName
 
 	stateMachine.Packages = partialStateMachine.Packages
 	stateMachine.Snaps = partialStateMachine.Snaps