storage.go: add filesystems validation warnings

Fixes coreos#1397, a common configuration error is to create a filesystem over a whole-disk device rather than a partition. Add a warning for a filesystem device matching the disk device. Additionally, another configuration error was identified for a way to recreate the filesystem on every provision, while the configuration to do so is valid, it might be done unitionally. Add a warning when wipeTable is true but wipeFilesystem is false.
prestist · Aug 17, 2022 · 9d24208 · 9d24208
1 parent d508eb2
commit 9d24208
Show file tree

Hide file tree

Showing 12 changed files with 595 additions and 5 deletions.
diff --git a/config/shared/errors/errors.go b/config/shared/errors/errors.go
@@ -70,6 +70,8 @@ var (
 	ErrNoPath                    = errors.New("path not specified")
 	ErrPathRelative              = errors.New("path not absolute")
 	ErrDirtyPath                 = errors.New("path is not fully simplified")
+	ErrPartitionsOverwritten     = errors.New("filesystem overwrites partitioned device")
+	ErrFilesystemImplicitWipe    = errors.New("device matches disk with wipeTable enabled; filesystem will be wiped")
 	ErrRaidLevelRequired         = errors.New("raid level is required")
 	ErrSparesUnsupportedForLevel = errors.New("spares unsupported for linear and raid0 arrays")
 	ErrUnrecognizedRaidLevel     = errors.New("unrecognized raid level")

diff --git a/config/v3_0/types/storage.go b/config/v3_0/types/storage.go
@@ -67,5 +67,20 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
 			}
 		}
 	}
+	disks := make(map[string]Disk)
+	for _, d := range s.Disks {
+		disks[d.Device] = d
+	}
+
+	for _, f := range s.Filesystems {
+		disk, exist := disks[f.Device]
+		if exist {
+			if len(disk.Partitions) > 0 {
+				r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrPartitionsOverwritten)
+			} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
+				r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrFilesystemImplicitWipe)
+			}
+		}
+	}
 	return
 }
diff --git a/config/v3_0/types/storage_test.go b/config/v3_0/types/storage_test.go
@@ -25,7 +25,7 @@ import (
 	"github.com/coreos/vcontext/report"
 )
 
-func TestStorageValidate(t *testing.T) {
+func TestStorageValidateErrors(t *testing.T) {
 	tests := []struct {
 		in  Storage
 		at  path.ContextPath
@@ -148,3 +148,104 @@ func TestStorageValidate(t *testing.T) {
 		}
 	}
 }
+
+func TestStorageValidateWarnings(t *testing.T) {
+	tests := []struct {
+		in  Storage
+		at  path.ContextPath
+		out error
+	}{
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sda",
+						Partitions: []Partition{
+							{}, {},
+						},
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: errors.ErrPartitionsOverwritten,
+			at:  path.New("", "filesystems", "/dev/sda"),
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device:    "/dev/sda",
+						WipeTable: util.BoolToPtr(true),
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device:         "/dev/sda",
+						WipeFilesystem: nil,
+					},
+				},
+			},
+			out: errors.ErrFilesystemImplicitWipe,
+			at:  path.New("", "filesystems", "/dev/sda"),
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device:    "/dev/sda",
+						WipeTable: util.BoolToPtr(false),
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: nil,
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+			},
+			out: nil,
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: nil,
+		},
+	}
+
+	for i, test := range tests {
+		r := test.in.Validate(path.ContextPath{})
+		expected := report.Report{}
+		expected.AddOnWarn(test.at, test.out)
+		if !reflect.DeepEqual(expected, r) {
+			t.Errorf("#%d: bad report: want %v, got %v", i, expected, r)
+		}
+	}
+}
diff --git a/config/v3_1/types/storage.go b/config/v3_1/types/storage.go
@@ -67,5 +67,22 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
 			}
 		}
 	}
+	disks := make(map[string]Disk)
+	for _, d := range s.Disks {
+		disks[d.Device] = d
+	}
+
+	for _, f := range s.Filesystems {
+		disk, exist := disks[f.Device]
+		if exist {
+			if exist {
+				if len(disk.Partitions) > 0 {
+					r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrPartitionsOverwritten)
+				} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
+					r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrFilesystemImplicitWipe)
+				}
+			}
+		}
+	}
 	return
 }
diff --git a/config/v3_1/types/storage_test.go b/config/v3_1/types/storage_test.go
@@ -25,7 +25,7 @@ import (
 	"github.com/coreos/vcontext/report"
 )
 
-func TestStorageValidate(t *testing.T) {
+func TestStorageValidateErrors(t *testing.T) {
 	tests := []struct {
 		in  Storage
 		at  path.ContextPath
@@ -148,3 +148,103 @@ func TestStorageValidate(t *testing.T) {
 		}
 	}
 }
+
+func TestStorageValidateWarnings(t *testing.T) {
+	tests := []struct {
+		in  Storage
+		at  path.ContextPath
+		out error
+	}{
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sda",
+						Partitions: []Partition{
+							{}, {},
+						},
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: errors.ErrPartitionsOverwritten,
+			at:  path.New("", "filesystems", "/dev/sda"),
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device:    "/dev/sda",
+						WipeTable: util.BoolToPtr(true),
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: errors.ErrFilesystemImplicitWipe,
+			at:  path.New("", "filesystems", "/dev/sda"),
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device:    "/dev/sda",
+						WipeTable: util.BoolToPtr(false),
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: nil,
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+			},
+			out: nil,
+		},
+		{
+			in: Storage{
+				Disks: []Disk{
+					{
+						Device: "/dev/sdb",
+					},
+				},
+				Filesystems: []Filesystem{
+					{
+						Device: "/dev/sda",
+					},
+				},
+			},
+			out: nil,
+		},
+	}
+
+	for i, test := range tests {
+		r := test.in.Validate(path.ContextPath{})
+		expected := report.Report{}
+		expected.AddOnWarn(test.at, test.out)
+		if !reflect.DeepEqual(expected, r) {
+			t.Errorf("#%d: bad report: want %v, got %v", i, expected, r)
+		}
+	}
+}
diff --git a/config/v3_2/types/storage.go b/config/v3_2/types/storage.go
@@ -67,5 +67,22 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
 			}
 		}
 	}
+	disks := make(map[string]Disk)
+	for _, d := range s.Disks {
+		disks[d.Device] = d
+	}
+
+	for _, f := range s.Filesystems {
+		disk, exist := disks[f.Device]
+		if exist {
+			if exist {
+				if len(disk.Partitions) > 0 {
+					r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrPartitionsOverwritten)
+				} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
+					r.AddOnWarn(c.Append("filesystems", f.Device), errors.ErrFilesystemImplicitWipe)
+				}
+			}
+		}
+	}
 	return
 }