Skip to content

Commit

Permalink
storage.go: add filesystems validation warnings
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@prestist
prestist committed Oct 25, 2022
1 parent 58bf6aa commit f2f249d
Show file tree
Hide file tree
Showing 12 changed files with 574 additions and 5 deletions.
2 changes: 2 additions & 0 deletions config/shared/errors/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,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")
Expand Down
15 changes: 15 additions & 0 deletions config/v3_0/types/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,20 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
ownerCheck(l1.Group.ID == nil, c.Append("links", i, "group", "id"))
ownerCheck(l1.Group.Name == nil, c.Append("links", i, "group", "name"))
}
disks := make(map[string]Disk)
for _, d := range s.Disks {
disks[d.Device] = d
}

for i, f := range s.Filesystems {
disk, exist := disks[f.Device]
if exist {
if len(disk.Partitions) > 0 {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrPartitionsOverwritten)
} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrFilesystemImplicitWipe)
}
}
}
return
}
100 changes: 99 additions & 1 deletion config/v3_0/types/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,18 @@ import (
"github.com/coreos/vcontext/report"
)

func TestStorageValidate(t *testing.T) {
func TestStorageValidateErrors(t *testing.T) {
tests := []struct {
in Storage
at path.ContextPath
err error
warn error
}{
// test empty storage config returns nil
{
in: Storage{},
},
// test a storage config with no conflicting paths returns nil
{
in: Storage{
Links: []Link{
Expand All @@ -57,6 +59,7 @@ func TestStorageValidate(t *testing.T) {
},
},
},
// test when a file uses a configured symlink path returns ErrFileUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -73,6 +76,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrFileUsedSymlink,
at: path.New("", "files", 0),
},
// test when a directory uses a configured symlink path returns ErrDirectoryUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -89,6 +93,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrDirectoryUsedSymlink,
at: path.New("", "directories", 0),
},
// test the same path listed for two separate symlinks returns ErrLinkUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -103,6 +108,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrLinkUsedSymlink,
at: path.New("", "links", 1),
},
// test that two symlinks can be configured at a time
{
in: Storage{
Links: []Link{
Expand All @@ -115,6 +121,7 @@ func TestStorageValidate(t *testing.T) {
},
},
},
// test when a directory uses a configured symlink with the 'Hard:= true' returns ErrHardLinkToDirectory
{
in: Storage{
Links: []Link{
Expand Down Expand Up @@ -227,3 +234,94 @@ func TestStorageValidate(t *testing.T) {
}
}
}

func TestStorageValidateWarnings(t *testing.T) {
tests := []struct {
in Storage
at path.ContextPath
out error
}{
// test a disk with partitions with the same 'device' as a filesystem returns ErrPartitionsOverwritten
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sda",
Partitions: []Partition{
{}, {},
},
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sda",
},
},
},
out: errors.ErrPartitionsOverwritten,
at: path.New("", "filesystems", 0, "device"),
},
// test a disk with the same 'device' and 'WipeTable:=true' as a configured filesystem returns ErrFilesystemImplicitWipe
{
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", 0, "device"),
},
// test a disk with the same 'device' and 'WipeTable:=false' as a configured filesystem returns nil
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sda",
WipeTable: util.BoolToPtr(false),
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sda",
WipeFilesystem: util.BoolToPtr(false),
},
},
},
out: nil,
},
// test a disk with no partitions with the same 'device' as a filesystem returns nil
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sdb",
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sdb",
},
},
},
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)
}
}
}
15 changes: 15 additions & 0 deletions config/v3_1/types/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,20 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
ownerCheck(l1.Group.ID == nil, c.Append("links", i, "group", "id"))
ownerCheck(l1.Group.Name == nil, c.Append("links", i, "group", "name"))
}
disks := make(map[string]Disk)
for _, d := range s.Disks {
disks[d.Device] = d
}

for i, f := range s.Filesystems {
disk, exist := disks[f.Device]
if exist {
if len(disk.Partitions) > 0 {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrPartitionsOverwritten)
} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrFilesystemImplicitWipe)
}
}
}
return
}
100 changes: 99 additions & 1 deletion config/v3_1/types/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,18 @@ import (
"github.com/coreos/vcontext/report"
)

func TestStorageValidate(t *testing.T) {
func TestStorageValidateErrors(t *testing.T) {
tests := []struct {
in Storage
at path.ContextPath
err error
warn error
}{
// test empty storage config returns nil
{
in: Storage{},
},
// test a storage config with no conflicting paths returns nil
{
in: Storage{
Links: []Link{
Expand All @@ -57,6 +59,7 @@ func TestStorageValidate(t *testing.T) {
},
},
},
// test when a file uses a configured symlink path returns ErrFileUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -73,6 +76,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrFileUsedSymlink,
at: path.New("", "files", 0),
},
// test when a directory uses a configured symlink path returns ErrDirectoryUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -89,6 +93,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrDirectoryUsedSymlink,
at: path.New("", "directories", 0),
},
// test the same path listed for two separate symlinks returns ErrLinkUsedSymlink
{
in: Storage{
Links: []Link{
Expand All @@ -103,6 +108,7 @@ func TestStorageValidate(t *testing.T) {
err: errors.ErrLinkUsedSymlink,
at: path.New("", "links", 1),
},
// test that two symlinks can be configured at a time
{
in: Storage{
Links: []Link{
Expand All @@ -115,6 +121,7 @@ func TestStorageValidate(t *testing.T) {
},
},
},
// test when a directory uses a configured symlink with the 'Hard:= true' returns ErrHardLinkToDirectory
{
in: Storage{
Links: []Link{
Expand Down Expand Up @@ -227,3 +234,94 @@ func TestStorageValidate(t *testing.T) {
}
}
}

func TestStorageValidateWarnings(t *testing.T) {
tests := []struct {
in Storage
at path.ContextPath
out error
}{
// test a disk with partitions with the same 'device' as a filesystem returns ErrPartitionsOverwritten
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sda",
Partitions: []Partition{
{}, {},
},
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sda",
},
},
},
out: errors.ErrPartitionsOverwritten,
at: path.New("", "filesystems", 0, "device"),
},
// test a disk with the same 'device' and 'WipeTable:=true' as a configured filesystem returns ErrFilesystemImplicitWipe
{
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", 0, "device"),
},
// test a disk with the same 'device' and 'WipeTable:=false' as a configured filesystem returns nil
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sda",
WipeTable: util.BoolToPtr(false),
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sda",
WipeFilesystem: util.BoolToPtr(false),
},
},
},
out: nil,
},
// test a disk with no partitions with the same 'device' as a filesystem returns nil
{
in: Storage{
Disks: []Disk{
{
Device: "/dev/sdb",
},
},
Filesystems: []Filesystem{
{
Device: "/dev/sdb",
},
},
},
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)
}
}
}
15 changes: 15 additions & 0 deletions config/v3_2/types/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,20 @@ func (s Storage) Validate(c vpath.ContextPath) (r report.Report) {
ownerCheck(l1.Group.ID == nil, c.Append("links", i, "group", "id"))
ownerCheck(l1.Group.Name == nil, c.Append("links", i, "group", "name"))
}
disks := make(map[string]Disk)
for _, d := range s.Disks {
disks[d.Device] = d
}

for i, f := range s.Filesystems {
disk, exist := disks[f.Device]
if exist {
if len(disk.Partitions) > 0 {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrPartitionsOverwritten)
} else if !util.IsTrue(f.WipeFilesystem) && util.IsTrue(disk.WipeTable) {
r.AddOnWarn(c.Append("filesystems", i, "device"), errors.ErrFilesystemImplicitWipe)
}
}
}
return
}
Loading

0 comments on commit f2f249d

Please sign in to comment.