Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add validation for nestedVirtualization, rosetta, and mountType #3127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions pkg/limayaml/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,37 @@ func Validate(y *LimaYAML, warn bool) error {
}
}

if y.Rosetta.Enabled != nil && *y.Rosetta.Enabled && *y.VMType != VZ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check should also make sure the arch is aarch64. Theoretically it should also check that the OS is darwin, but you can set the vmType to VZ unless you are on macOS, so that should already be covered.

return fmt.Errorf("field `rosetta.enabled` can only be enabled for VMType %q; got %q", VZ, *y.VMType)
}

if y.NestedVirtualization != nil && *y.NestedVirtualization && *y.VMType != VZ {
return fmt.Errorf("field `nestedVirtualization` can only be enabled for VMType %q; got %q", VZ, *y.VMType)
}

if err := validateMountType(y); err != nil {
return err
}

return nil
}

func validateMountType(y *LimaYAML) error {
validMountTypes := map[string]bool{
REVSSHFS: true,
NINEP: true,
VIRTIOFS: true,
WSLMount: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have a test that on Windows the only valid mountType is WSLMount.

}

if !validMountTypes[*y.MountType] {
return fmt.Errorf("field `mountType` must be: %q, %q, %q, %q; got %q", REVSSHFS, NINEP, VIRTIOFS, WSLMount, *y.MountType)
}

if *y.MountType == VIRTIOFS && runtime.GOOS == "darwin" && *y.VMType != VZ {
return fmt.Errorf("field `mountType` %q on macOS requires vmType %q; got %q", *y.MountType, VZ, *y.VMType)
}

return nil
}

Expand Down Expand Up @@ -432,6 +463,9 @@ func validateNetwork(y *LimaYAML) error {
return fmt.Errorf("field `%s.macAddress` must be a 48 bit (6 bytes) MAC address; actual length of %q is %d bytes", field, nw.MACAddress, len(hw))
}
}
if nw.VZNAT != nil && *nw.VZNAT && *y.VMType != VZ {
return fmt.Errorf("field `%s.vzNAT` needs vmType set to %q; got %q", field, VZ, *y.VMType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar errors should use similar wording. So always use requires or always needs (check what existing errors already use), and don't mix and match.

Suggested change
return fmt.Errorf("field `%s.vzNAT` needs vmType set to %q; got %q", field, VZ, *y.VMType)
return fmt.Errorf("field `%s.vzNAT` requires vmType %q; got %q", field, VZ, *y.VMType)

}
// FillDefault() will make sure that nw.Interface is not the empty string
if len(nw.Interface) >= 16 {
return fmt.Errorf("field `%s.interface` must be less than 16 bytes, but is %d bytes: %q", field, len(nw.Interface), nw.Interface)
Expand Down
131 changes: 131 additions & 0 deletions pkg/limayaml/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,134 @@ func TestValidateParamIsUsed(t *testing.T) {
assert.Error(t, err, "field `param` key \"rootFul\" is not used in any provision, probe, copyToHost, or portForward")
}
}

func TestValidateRosetta(t *testing.T) {
images := `images: [{"location": "/"}]`

nilData := ``
y, err := Load([]byte(nilData+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.NilError(t, err)

invalidRosetta := `
rosetta:
enabled: true
vmType: "qemu"
`
y, err = Load([]byte(invalidRosetta+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
if runtime.GOOS == "darwin" && IsNativeArch(AARCH64) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows this should return a validation error that vm type QEMU is not supported.

assert.Error(t, err, "field `rosetta.enabled` can only be enabled for VMType \"vz\"; got \"qemu\"")
} else {
assert.NilError(t, err)
}

validRosetta := `
rosetta:
enabled: true
vmType: "vz"
`
y, err = Load([]byte(validRosetta+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.NilError(t, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected this to fail on Linux and Windows, which don't support the VZ vm type. If it doesn't fail validation on Linux, then that needs to be added.


rosettaDisabled := `
rosetta:
enabled: false
vmType: "qemu"
`
y, err = Load([]byte(rosettaDisabled+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, expected to fail validation on Windows.

I'm stopping the review here; this continues to be an issue for the rest of the tests.

Instead of testing for the specific failure on e.g. Windows, I think you should probably skip the whole tests on platforms where they are expected to fail for different reasons than the one you are testing.

assert.NilError(t, err)
}

func TestValidateNestedVirtualization(t *testing.T) {
images := `images: [{"location": "/"}]`

validYAML := `
nestedVirtualization: true
vmType: vz
` + images

y, err := Load([]byte(validYAML), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.NilError(t, err)

invalidYAML := `
nestedVirtualization: true
vmType: qemu
` + images

y, err = Load([]byte(invalidYAML), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.Error(t, err, "field `nestedVirtualization` can only be enabled for VMType \"vz\"; got \"qemu\"")
}

func TestValidateMountTypeOS(t *testing.T) {
images := `images: [{"location": "/"}]`

nilMountConf := ``
y, err := Load([]byte(nilMountConf+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.NilError(t, err)

inValidMountTypeLinux := `
mountType: "random"
`
y, err = Load([]byte(inValidMountTypeLinux+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, true)
assert.Error(t, err, "field `mountType` must be \"reverse-sshfs\" or \"9p\" or \"virtiofs\", or \"wsl2\", got \"random\"")

validMountTypeLinux := `
mountType: "virtiofs"
`
y, err = Load([]byte(validMountTypeLinux+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, true)
if runtime.GOOS == "darwin" && IsNativeArch(AARCH64) {
assert.Error(t, err, "field `mountType` \"virtiofs\" on macOS requires vmType \"vz\"; got \"qemu\"")
} else {
assert.NilError(t, err)
}

validMountTypeMac := `
mountType: "virtiofs"
vmType: "vz"
`
y, err = Load([]byte(validMountTypeMac+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
assert.NilError(t, err)

invalidMountTypeMac := `
mountType: "virtiofs"
vmType: "qemu"
`
y, err = Load([]byte(invalidMountTypeMac+"\n"+images), "lima.yaml")
assert.NilError(t, err)

err = Validate(y, false)
if runtime.GOOS == "darwin" {
assert.Error(t, err, "field `mountType` \"virtiofs\" on macOS requires vmType \"vz\"; got \"qemu\"")
} else {
assert.NilError(t, err)
}
}
Loading