Skip to content

Reject shrinking disk during YAML validation #3596

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

songponssw
Copy link
Contributor

I wish we would detect the attempt to downsize the disk size, and then reject the change as invalid, but that can be a separate PR.

As @jandubois suggested in #3533.

Currently, rejecting shrinking disk size happens during instance startup.
This PR adds validation earlier during YAML validation.
The function compares values between the new config and the latest config YAML files to reject disk size shrinking during validation.

@AkihiroSuda AkihiroSuda requested a review from Copilot May 31, 2025 11:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements early YAML validation to reject configurations that attempt to shrink the disk size rather than waiting until instance startup.

  • Added a ValidateYAMLAgainstLatest function that compares disk sizes from the new and latest YAML configurations.
  • Integrated the new validation into the limactl edit flow to save and report rejected configurations.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pkg/limayaml/validate.go Added a YAML validation function to prevent disk size shrinking.
cmd/limactl/edit.go Integrated validation into the edit command and added a function to handle rejected YAML.

func saveRejectedYAML(y []byte, origErr error) error {
rejectedYAML := "lima.REJECTED.yaml"
if writeErr := os.WriteFile(rejectedYAML, y, 0o644); writeErr != nil {
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr)
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

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

The error message wraps two errors using '%w: %w', which might be confusing. Consider simplifying the error wrapping to clearly associate the cause of failure with a single wrapped error.

Suggested change
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr)
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w", rejectedYAML, writeErr)

Copilot uses AI. Check for mistakes.

@songponssw songponssw force-pushed the reject-shrinking-disk branch from 8cb23be to 4532c86 Compare June 18, 2025 14:58
@songponssw songponssw force-pushed the reject-shrinking-disk branch from 4532c86 to 4f4a7c1 Compare June 18, 2025 16:55
@songponssw
Copy link
Contributor Author

The disk value can be nil when editing some YAML file.
For example, the editing kernel and initrd location in hack/inject-cmdline-to-template.sh.

@songponssw songponssw requested a review from AkihiroSuda June 19, 2025 00:06
@AkihiroSuda AkihiroSuda added this to the v1.1.2 milestone Jun 19, 2025
@songponssw songponssw marked this pull request as ready for review June 19, 2025 07:10
@AkihiroSuda AkihiroSuda requested a review from jandubois June 19, 2025 08:38
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but there is some nitpicking I think should be addressed.

func saveRejectedYAML(y []byte, origErr error) error {
rejectedYAML := "lima.REJECTED.yaml"
if writeErr := os.WriteFile(rejectedYAML, y, 0o644); writeErr != nil {
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr)
Copy link
Member

Choose a reason for hiding this comment

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

As Copilot suggested above, you cannot use 2 %w formatting codes in an error; only the last one will be wrapped; the other will be treated as %v.

So either explicitly choose which one should be wrapped, and use %v for the other one explicitly. Or combine both errors with errors.Join():

Suggested change
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr)
return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w", rejectedYAML, errors.Join(writeErr, origErr))

if err = Unmarshal(yLatest, &l, "Unmarshal latest YAML bytes"); err != nil {
return err
}
if err = Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err = Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {
if err = Unmarshal(yNew, &n, "Unmarshal new YAML bytes); err != nil {

}

// Handle editing the template without a disk value
if n.Disk == nil && l.Disk == nil {
Copy link
Member

Choose a reason for hiding this comment

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

You are dereferencing both, so you need to bail if either one is nil.

Suggested change
if n.Disk == nil && l.Disk == nil {
if n.Disk == nil || l.Disk == nil {

Comment on lines +602 to +606
var err error
if err = Unmarshal(yLatest, &l, "Unmarshal latest YAML bytes"); err != nil {
return err
}
if err = Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid err declaration:

Suggested change
var err error
if err = Unmarshal(yLatest, &l, "Unmarshal latest YAML bytes"); err != nil {
return err
}
if err = Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {
if err := Unmarshal(yLatest, &l, "Unmarshal latest YAML bytes"); err != nil {
return err
}
if err := Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {

@@ -595,3 +595,37 @@ func warnExperimental(y *LimaYAML) {
logrus.Warn("`mountInotify` is experimental")
}
}

// ValidateYAMLAgainstLatestConfig validates the values between the latest YAML and the updated(New) YAML.
func ValidateYAMLAgainstLatestConfig(yNew, yLatest []byte) error {
Copy link
Member

Choose a reason for hiding this comment

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

Let's cover this function with unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants