v3_5_experimental: add validation, unit tests and error

Author: yasminvalim

config/shared/errors/errors.go

@@ -85,7 +85,8 @@ var (
 	ErrInvalidProxy              = errors.New("proxies must be http(s)")
 	ErrInsecureProxy             = errors.New("insecure plaintext HTTP proxy specified for HTTPS resources")
 	ErrPathConflictsSystemd      = errors.New("path conflicts with systemd unit or dropin")
-	ErrPathConflictsParentDir    = errors.New("path conflicts with parent directory of another file, link, or directory")
+	ErrPathAlreadyExists         = errors.New("path already exists")
+	ErrMissLabeledDir            = errors.New("parent directory path matches configured file, check path, and ensure parent directory is configured")
 
 	// Systemd section errors
 	ErrInvalidSystemdExt       = errors.New("invalid systemd unit extension")

config/v3_5_experimental/types/config.go

@@ -35,6 +35,8 @@ var (
 	}
 )
 
+var paths = map[string]struct{}{}
+
 func (cfg Config) Validate(c path.ContextPath) (r report.Report) {
 	systemdPath := "/etc/systemd/system/"
 	unitPaths := map[string]struct{}{}
@@ -76,43 +78,21 @@ func (cfg Config) validateParents(c path.ContextPath) report.Report {
 		Path  string
 		Field string
 	}
-	paths := map[string]struct{}{}
 	r := report.Report{}
 
 	for i, f := range cfg.Storage.Files {
-		if _, exists := paths[f.Path]; exists {
-			r.AddOnError(c.Append("storage", "files", i, "path"), errors.ErrPathConflictsParentDir) //TODO:  should add different error?
-			return r
-		}
-		paths[f.Path] = struct{}{}
-		entries = append(entries, struct {
-			Path  string
-			Field string
-		}{Path: f.Path, Field: "files"})
+		r = handlePathConflict(f.Path, "files", i, c, r, errors.ErrPathAlreadyExists)
+		addPathAndEntry(f.Path, "files", &entries)
 	}
 
 	for i, d := range cfg.Storage.Directories {
-		if _, exists := paths[d.Path]; exists {
-			r.AddOnError(c.Append("storage", "directories", i, "path"), errors.ErrPathConflictsParentDir) //TODO: should add different error?
-			return r
-		}
-		paths[d.Path] = struct{}{}
-		entries = append(entries, struct {
-			Path  string
-			Field string
-		}{Path: d.Path, Field: "directories"})
+		r = handlePathConflict(d.Path, "directories", i, c, r, errors.ErrPathAlreadyExists)
+		addPathAndEntry(d.Path, "directories", &entries)
 	}
 
 	for i, l := range cfg.Storage.Links {
-		if _, exists := paths[l.Path]; exists {
-			r.AddOnError(c.Append("storage", "links", i, "path"), errors.ErrPathConflictsParentDir) //TODO:  error to already exist path
-			return r
-		}
-		paths[l.Path] = struct{}{}
-		entries = append(entries, struct {
-			Path  string
-			Field string
-		}{Path: l.Path, Field: "links"})
+		r = handlePathConflict(l.Path, "links", i, c, r, errors.ErrPathAlreadyExists)
+		addPathAndEntry(l.Path, "links", &entries)
 	}
 
 	sort.Slice(entries, func(i, j int) bool {
@@ -122,7 +102,7 @@ func (cfg Config) validateParents(c path.ContextPath) report.Report {
 	for i, entry := range entries {
 		if i > 0 && isWithin(entry.Path, entries[i-1].Path) {
 			if entries[i-1].Field != "directories" {
-				r.AddOnError(c.Append("storage", entry.Field, i, "path"), errors.ErrPathConflictsParentDir) //TODO: conflict parent directories error
+				r.AddOnError(c.Append("storage", entry.Field, i, "path"), errors.ErrMissLabeledDir)
 				return r
 			}
 		}
@@ -131,16 +111,37 @@ func (cfg Config) validateParents(c path.ContextPath) report.Report {
 	return r
 }
 
-// check the depth
+func handlePathConflict(path, fieldName string, index int, c path.ContextPath, r report.Report, err error) report.Report {
+	if _, exists := paths[path]; exists {
+		r.AddOnError(c.Append("storage", fieldName, index, "path"), err)
+	}
+	return r
+}
+
+func addPathAndEntry(path, fieldName string, entries *[]struct{ Path, Field string }) {
+	*entries = append(*entries, struct {
+		Path  string
+		Field string
+	}{Path: path, Field: fieldName})
+}
+
 func depth(path string) uint {
 	var count uint
-	for p := filepath.Clean(path); p != "/" && p != "."; count++ {
-		p = filepath.Dir(p)
+	cleanedPath := filepath.FromSlash(filepath.Clean(path))
+	sep := string(filepath.Separator)
+
+	volume := filepath.VolumeName(cleanedPath)
+	if volume != "" {
+		cleanedPath = cleanedPath[len(volume):]
+	}
+
+	for cleanedPath != sep && cleanedPath != "." {
+		cleanedPath = filepath.Dir(cleanedPath)
+		count++
 	}
 	return count
 }
 
-// isWithin checks if newPath is within prevPath.
 func isWithin(newPath, prevPath string) bool {
 	return strings.HasPrefix(newPath, prevPath) && newPath != prevPath
 }

config/v3_5_experimental/types/config_test.go

@@ -20,7 +20,6 @@ import (
 
 	"github.com/coreos/ignition/v2/config/shared/errors"
 	"github.com/coreos/ignition/v2/config/util"
-
 	"github.com/coreos/vcontext/path"
 	"github.com/coreos/vcontext/report"
 )
@@ -194,7 +193,7 @@ func TestConfigValidation(t *testing.T) {
 					},
 				},
 			},
-			out: errors.ErrPathConflictsParentDir,
+			out: errors.ErrMissLabeledDir,
 			at:  path.New("json", "storage", "files", 1, "path"),
 		},
 
@@ -210,8 +209,8 @@ func TestConfigValidation(t *testing.T) {
 					},
 				},
 			},
-			out: errors.ErrPathConflictsParentDir,
-			at:  path.New("json", "storage", "links", 0, "path"),
+			out: errors.ErrMissLabeledDir,
+			at:  path.New("json", "storage", "links", 1, "path"),
 		},
 
 		// test 8: file path conflicts with directory path, should error
@@ -220,15 +219,14 @@ func TestConfigValidation(t *testing.T) {
 				Storage: Storage{
 					Files: []File{
 						{Node: Node{Path: "/foo/bar"}},
-						{Node: Node{Path: "/foo/bar"}},
 					},
 					Directories: []Directory{
 						{Node: Node{Path: "/foo/bar/baz"}},
 					},
 				},
 			},
-			out: errors.ErrPathConflictsParentDir,
-			at:  path.New("json", "storage", "directories", 0, "path"),
+			out: errors.ErrMissLabeledDir,
+			at:  path.New("json", "storage", "directories", 1, "path"),
 		},
 
 		// test 9: non-conflicting scenarios with systemd unit and systemd dropin file, should not error
@@ -302,7 +300,6 @@ func TestConfigValidation(t *testing.T) {
 			in: Config{
 				Storage: Storage{
 					Files: []File{
-						{Node: Node{Path: "/foo/bar"}},
 						{Node: Node{Path: "/foo/bar/baz"}},
 					},
 					Directories: []Directory{

docs/release-notes.md

@@ -24,6 +24,7 @@ nav_order: 9
 
 - Fix failure when config only disables units already disabled
 - Retry HTTP requests on Azure on status codes 404, 410, and 429
+- Fix validation to catch conflicts with the parent directory of another file, link or directories
 
 
 ## Ignition 2.17.0 (2023-11-20)