snap: add validation of gadget.yaml #6634

bboozzoo · 2019-03-21T12:07:53Z

Add validation of gadget definition based on what ubuntu-image does. Try to match error message where possible.

The validation only considers the structure of gadget.yaml, internal references and sanity of defined entries.

Things like validation of structure overlap, offset sizes are best done in a separate step, when the structure is 'resolved' for writing. Meaning, that the information from gadget.yaml is conbined with the contents of the gadget snap (eg. when size is left unspecified) and platform requirements (eg. default sector size). The ubuntu-image tries to do that upfront, but makes some assumptions along the way.

The PR contains #6609

Add validation of gadget definition based on what ubuntu-image does. Try to match error message where possible. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ate-validation

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga · 2019-03-25T08:15:49Z

Related to this work: what is the future interaction with ubuntu-image? I would expect that there is only one implementation of certain things. Will ubuntu-image depend on "snap" to pack/validate gadgets? Will we have to live with two implementations?

bboozzoo · 2019-03-25T08:54:40Z

As discussed, we'd like to eventually grow the functionality to be able to assemble the image ourselves. At which time ubuntu-image could be updated to use snap.

zyga · 2019-03-25T08:58:23Z

@bboozzoo that is a good plan. Thanks

mvo5

Looks nice, some comments and nitpicks for consideration but no blockers, feel free to ignore most of them (or act, depending on how much you like them).

mvo5 · 2019-03-25T08:16:57Z

snap/gadget.go

@@ -223,6 +244,7 @@ func ReadGadgetInfo(info *Info, classic bool) (*GadgetInfo, error) {
 		default:
 			return nil, fmt.Errorf(errorFormat, "bootloader must be one of grub, u-boot or android-boot")
 		}
+


Extra whitespace? Or deliberate?

mvo5 · 2019-03-25T08:18:43Z

snap/gadget.go

@@ -234,6 +256,225 @@ func ReadGadgetInfo(info *Info, classic bool) (*GadgetInfo, error) {
 	return &gi, nil
 }

+func indexAndName(idx int, name string) string {


(nitpick²) maybe fmtIndexAndName()?

mvo5 · 2019-03-25T08:21:41Z

snap/gadget.go

+		if err := validateVolumeStructure(&s, vol); err != nil {
+			maybeName := ""
+			if s.Name != "" {
+				maybeName = fmt.Sprintf("(%q)", s.Name)


Don't we need a leading space here: " (%q)" ? The error will otherwise concat idx and name to something like 1("foo"). Or am I misreading (or is it intentional)?

mvo5 · 2019-03-25T08:24:23Z

snap/gadget.go

+
+func validateVolume(name string, vol *GadgetVolume) error {
+	if !validVolumeName.MatchString(name) {
+		return errors.New("invalid volume name")


~~Do we need to include what volume name string is invalid here (i.e. fmt.Error("invalid volume name %q"), only ? Or will this error be wrapped somewhere that includes this info?~~ - I see that this is wrapped.

do we need to mention "volume" in this error?

Updated. The error just below needs the same treatment.

mvo5 · 2019-03-25T10:10:11Z

snap/gadget.go

+				indexAndName(idx, s.Name), s.OffsetWrite.RelativeTo)
+		}
+
+		if s.Filesystem != "" && s.Filesystem != "none" {


We have a "IsBare" helper, could that be used here?

mvo5 · 2019-03-25T10:34:01Z

snap/gadget.go

+func ParseGadgetRelativeOffset(grs string) (*GadgetRelativeOffset, error) {
+	toWhat := ""
+	sizeSpec := grs
+	idx := strings.IndexRune(grs, '+')


Would strings.Split() be slightly shorter here?

Did the change locally, looks very much the same.

mvo5 · 2019-03-25T10:35:33Z

snap/gadget.go

+	if err != nil {
+		return nil, fmt.Errorf("cannot parse offset %q: %v", sizeSpec, err)
+	}
+	if size > 4*2<<30 {


(nitpick) maybe var limit4G = 4*2<<30 \n if size > limit4G ... ?

mvo5 · 2019-03-25T10:44:39Z

snap/gadget_test.go

+            target: /
+`)
+
+func mustParseGadgetSize(c *C, s string) snap.GadgetSize {


(nitpick²) all(?) of the other "mustFoo()" do not take a "check.C" and just panic. Probably not worth changing though.

mvo5 · 2019-03-25T10:50:18Z

snap/gadget.go

+
+// ParseGadgetSize parses a string expressing size in gadget declaration. The
+// accepted format is one of: <bytes> | <bytes/2^20>M | <bytes/2^30>G.
+func ParseGadgetSize(gs string) (GadgetSize, error) {


We have similar code in strutil.ParseByteSize I was wondering if we could unify those, but the trouble is of course is that that strutil.ParseByteSize uses MB MegaByte (10^6) vs this code here which uses MiB (^20) but uses "M" as the ending which is not the correct extentision, it should be "MiB". So probably easiest to just keep it as is.

Yeah, I wanted to use the helper we have, but the formats accepted in gadget.yaml are only these 3:

<number> (bytes)

<number>M

<number>G

mvo5 · 2019-03-25T10:58:26Z

snap/gadget_test.go

+		err string
+	}{
+		{"1234", 1234, ""},
+		{"1234M", 1234 * 2 << 20, ""},


Given how many 2 << 20 and 2 << 30 I'm seeing I wonder if we should have a const in snapd for this somewhere? Like "2units.Mebibyte" and "2units.Gigibyte" etc ?

zyga

Old partial review, starting afresh.

zyga · 2019-03-25T09:00:55Z

snap/gadget.go

+)
+
+var (
+	validVolumeName = regexp.MustCompile("^[a-z-]+$")


This allows a volume to start with -, is that desirable? Do volume names have to be lower case? I would suspect at least some volumes wanting to have all-caps name for some legacy reason.

Looks like ubuntu-image is bad too, this is regexp they use: ^[-a-zA-Z0-9]+$, and this is what the docs page states: [a-z-]+

Updated to match what ubuntu-image does.

zyga · 2019-03-25T09:01:22Z

snap/gadget.go

+var (
+	validVolumeName = regexp.MustCompile("^[a-z-]+$")
+	validTypeID     = regexp.MustCompile("^[0-9A-F]{2}$")
+	validGUUID      = regexp.MustCompile("^[0-9A-F]{8}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{12}$")


snap/gadget.go

zyga · 2019-03-25T09:03:35Z

snap/gadget.go

-	for _, v := range gi.Volumes {
+	for name, v := range gi.Volumes {
+		if err := validateVolume(name, &v); err != nil {
+			return nil, fmt.Errorf("invalid volume %q: %v", name, err)


Would it be sensible that the error you create here was actually returned by validateVolume and that this place simply returned it verbatim?

Ha! I was thinking exactly the same and then wondered if we have a pattern and then wondered if go has a pattern and then got lost in the weeds. But I still wonder if there is a pattern for this in the go community in cases like this where it seems like both choices make sense. Maybe @pedronis has an opinion/thoughts?

There's this https://github.com/pkg/errors that has very nicely defined Wrap function: https://github.com/pkg/errors/blob/master/errors.go#L181 for the case of taking one error and wrapping it with some more messages.

You can have multiple volumes in gadget.yaml. Returning error as is would imply the validate* has to include all necessary information to be able to locate the invalid entity.

FWIW we have a similar problem with code in snap/validate.go.

I think this if fine, we just need to be careful not to use "volume" in the errors below too unnecessarily

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ate-validation

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

thank you, did a first pass

pedronis · 2019-03-26T16:24:43Z

snap/gadget.go

-	for _, v := range gi.Volumes {
+	for name, v := range gi.Volumes {
+		if err := validateVolume(name, &v); err != nil {
+			return nil, fmt.Errorf("invalid volume %q: %v", name, err)


I think this if fine, we just need to be careful not to use "volume" in the errors below too unnecessarily

pedronis · 2019-03-26T16:26:32Z

snap/gadget.go

+
+func validateVolume(name string, vol *GadgetVolume) error {
+	if !validVolumeName.MatchString(name) {
+		return errors.New("invalid volume name")


do we need to mention "volume" in this error?

pedronis · 2019-03-26T16:27:04Z

snap/gadget.go

+	if name != "" {
+		return fmt.Sprintf("#%v (%q)", idx, name)
+	}
+	return fmt.Sprintf("#%v", idx)


this path seems not to be exercised in the tests

pedronis · 2019-03-26T16:30:59Z

snap/gadget.go

+
+// GadgetSize describes the size of a structure item or an offset within the
+// structure.
+type GadgetSize uint64


it's fine (in that world)

pedronis · 2019-03-26T16:33:37Z

snap/gadget.go

+	names := make(map[string]bool, len(vs.Update.Preserve))
+	for _, n := range vs.Update.Preserve {
+		if names[n] {
+			return fmt.Errorf("duplicate preserve entry %q", n)


this reads strangely, maybe quotes help:

duplicate "preserve" entry %q

pedronis · 2019-03-26T16:46:48Z

snap/gadget.go

+	bare := vs.IsBare()
+	for i, c := range vs.Content {
+		var err error
+		if bare {


but we could simply set a checker outside and not have the if here:

var contentChecker func(*VolumeContent) error

pedronis · 2019-03-26T16:48:01Z

snap/gadget.go

+	structureNames := make(map[string]bool, len(vol.Structure))
+	for idx, s := range vol.Structure {
+		if err := validateVolumeStructure(&s, vol); err != nil {
+			return fmt.Errorf("invalid structure %s: %v", fmtIndexAndName(idx, s.Name), err)


seems tests never get to this error

pedronis · 2019-03-26T16:48:19Z

snap/gadget.go

+	}
+
+	if s == "" {
+		return errors.New(`type is not specified`)


this error is not exercised

pedronis · 2019-03-26T16:48:43Z

snap/gadget.go

+
+func validateVolumeStructure(vs *VolumeStructure, vol *GadgetVolume) error {
+	if err := validateStructureType(vs.Type, vol); err != nil {
+		return fmt.Errorf("invalid type %q: %v", vs.Type, err)


tests don't get to this error

pedronis · 2019-03-26T16:48:51Z

snap/gadget.go

+		return fmt.Errorf("invalid type %q: %v", vs.Type, err)
+	}
+	if err := validateRole(vs, vol); err != nil {
+		return fmt.Errorf("invalid role %q: %v", vs.Role, err)


tests don't get to this error

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

thank you

zyga · 2019-03-29T08:55:31Z

snap/gadget.go

-	Filesystem  string          `yaml:"filesystem"`
-	Content     []VolumeContent `yaml:"content"`
-	Update      VolumeUpdate    `yaml:"update"`
+	Name        string               `yaml:"name"`


What is the difference between the label and the name?

Label is just a filesystem label. Name written as partition name if GPT schema is in use, otherwise it's informative.

Ah, can you document that in the structure please?

zyga

LGTM because it's green and it is Friday but please look at my comments for several simple follow-ups.

snap/gadget.go

zyga · 2019-03-29T09:04:59Z

snap/gadget.go

+	return fmt.Sprintf("#%v", idx)
+}
+
+func validateVolume(name string, vol *GadgetVolume) error {


zyga · 2019-03-29T09:06:52Z

snap/gadget.go

+	return nil
+}
+
+func validateVolumeStructure(vs *VolumeStructure, vol *GadgetVolume) error {


zyga · 2019-03-29T09:07:55Z

snap/gadget.go

+	// - "bare"
+	// - [0-9A-Z]{2} - MBR type
+	// - GPT UUID
+	// - hybrid ID


Perhaps add a wikipedia reference to explain what hybrid IDs are for.

zyga · 2019-03-29T09:09:16Z

snap/gadget.go

+	return nil
+}
+
+func validateStructureType(s string, vol *GadgetVolume) error {


zyga · 2019-03-29T09:20:18Z

snap/gadget.go

+	return nil
+}
+
+func validateStructureUpdate(up *VolumeUpdate, vs *VolumeStructure) error {


snap/gadget.go

zyga · 2019-03-29T09:22:07Z

snap/gadget.go

+
+// ParseGadgetSize parses a string expressing size in gadget declaration. The
+// accepted format is one of: <bytes> | <bytes/2^20>M | <bytes/2^30>G.
+func ParseGadgetSize(gs string) (GadgetSize, error) {


zyga · 2019-03-29T09:23:14Z

snap/gadget.go

+	if err != nil {
+		return nil, fmt.Errorf("cannot parse offset %q: %v", sizeSpec, err)
+	}
+	if size > 4*SizeGiB {


The comment below is redundant. Can you please add a reference to the origin of the limit?

snap/gadget.go

…ate-validation

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2019-03-29T14:05:01Z

Pushed a patch adding comments to volume, structure and content, based on what's there in https://docs.snapcraft.io/the-gadget-snap/696.

…ate-validation

bboozzoo added 4 commits March 21, 2019 12:52

Merge branch 'bboozzoo/gadget-yaml-update' into bboozzoo/gadget-update

94049c9

snap: add basic validation of gadget definition

6c621ef

Add validation of gadget definition based on what ubuntu-image does. Try to match error message where possible. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/gadget-upd…

2cd6139

…ate-validation

snap: fix composite literal with unkeyed fields

84f6b93

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis self-requested a review March 22, 2019 14:32

mvo5 approved these changes Mar 25, 2019

View reviewed changes

zyga reviewed Mar 26, 2019

View reviewed changes

bboozzoo added 3 commits March 26, 2019 13:03

snap: gadget validation tweaks

f24c704

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/gadget-upd…

0279a24

…ate-validation

snap: fix gofmt 1.9

08289ed

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis reviewed Mar 26, 2019

View reviewed changes

bboozzoo mentioned this pull request Mar 27, 2019

ubuntu_image: parser improvements canonical/ubuntu-image-legacy#168

Merged

bboozzoo added 3 commits March 27, 2019 14:26

snap: tweak gadget validation, cleanup error wrapping, improve tests

14970af

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

snap: gadget structure names must be unique within a volume

b80cdbf

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

snap: simplify structure content checking code

ebd0325

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo requested review from pedronis and zyga March 28, 2019 11:42

pedronis approved these changes Mar 28, 2019

View reviewed changes

zyga reviewed Mar 29, 2019

View reviewed changes

zyga approved these changes Mar 29, 2019

View reviewed changes

bboozzoo added 2 commits March 29, 2019 14:01

Merge remote-tracking branch 'origin/master' into bboozzoo/gadget-upd…

efeefb0

…ate-validation

snap: add comments about gadget structures and volumes

17da12d

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/gadget-upd…

77405b0

…ate-validation

bboozzoo merged commit db6412d into snapcore:master Apr 1, 2019

snap: add validation of gadget.yaml #6634

snap: add validation of gadget.yaml #6634

Conversation

bboozzoo commented Mar 21, 2019

zyga commented Mar 25, 2019

bboozzoo commented Mar 25, 2019

zyga commented Mar 25, 2019

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Mar 29, 2019

pedronis Mar 26, 2019 •

edited

pedronis Mar 26, 2019 •

edited