From 9d242081e0e897e76788145e40a0648de82248ea Mon Sep 17 00:00:00 2001 From: Steven Presti Date: Wed, 29 Jun 2022 15:25:35 -0400 Subject: [PATCH] storage.go: add filesystems validation warnings Fixes #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. --- config/shared/errors/errors.go | 2 + config/v3_0/types/storage.go | 15 +++ config/v3_0/types/storage_test.go | 103 ++++++++++++++++- config/v3_1/types/storage.go | 17 +++ config/v3_1/types/storage_test.go | 102 ++++++++++++++++- config/v3_2/types/storage.go | 17 +++ config/v3_2/types/storage_test.go | 104 +++++++++++++++++- config/v3_3/types/storage.go | 17 +++ config/v3_3/types/storage_test.go | 102 ++++++++++++++++- config/v3_4_experimental/types/storage.go | 17 +++ .../v3_4_experimental/types/storage_test.go | 102 ++++++++++++++++- docs/release-notes.md | 2 + 12 files changed, 595 insertions(+), 5 deletions(-) diff --git a/config/shared/errors/errors.go b/config/shared/errors/errors.go index 44852cb902..b0a52f8dcd 100644 --- 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 index eac98e74b5..abffe345f9 100644 --- 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 index 00c5861af1..72c57c873e 100644 --- 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 index eac98e74b5..5429d75463 100644 --- 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 index 00c5861af1..b7ccd05e0f 100644 --- 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 index fd1b8cecf6..ea67b5dd10 100644 --- 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 } diff --git a/config/v3_2/types/storage_test.go b/config/v3_2/types/storage_test.go index 7d27bd550f..3d0f7683b3 100644 --- a/config/v3_2/types/storage_test.go +++ b/config/v3_2/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,105 @@ 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: util.BoolToPtr(false), + }, + }, + }, + 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", + WipeFilesystem: util.BoolToPtr(false), + }, + }, + }, + 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_3/types/storage.go b/config/v3_3/types/storage.go index 0210861312..c0b987663a 100644 --- a/config/v3_3/types/storage.go +++ b/config/v3_3/types/storage.go @@ -71,5 +71,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_3/types/storage_test.go b/config/v3_3/types/storage_test.go index cb9256cf16..2cd434a6a7 100644 --- a/config/v3_3/types/storage_test.go +++ b/config/v3_3/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 @@ -180,3 +180,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_4_experimental/types/storage.go b/config/v3_4_experimental/types/storage.go index 0210861312..c0b987663a 100644 --- a/config/v3_4_experimental/types/storage.go +++ b/config/v3_4_experimental/types/storage.go @@ -71,5 +71,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_4_experimental/types/storage_test.go b/config/v3_4_experimental/types/storage_test.go index cb9256cf16..2cd434a6a7 100644 --- a/config/v3_4_experimental/types/storage_test.go +++ b/config/v3_4_experimental/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 @@ -180,3 +180,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/docs/release-notes.md b/docs/release-notes.md index 122edc67c0..38a5db46dc 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -16,6 +16,8 @@ nav_order: 9 ### Changes - Warn if template for enabled systemd instance unit has no `Install` section +- Warn if filesystem overwrites partitioned disk +- Warn if `wipeTable` overwrites a filesystem that would otherwise be reused - Install ignition-apply in `/usr/libexec` - Convert `NEWS` to Markdown and move to docs site