Skip to content
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

Ignition should fail validation if a file conflicts with the parent directory of another file/link/dir #1795

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/shared/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +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")
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")
Expand Down
96 changes: 95 additions & 1 deletion config/v3_5_experimental/types/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
package types

import (
"fmt"
"path/filepath"
"sort"
"strings"

"github.com/coreos/ignition/v2/config/shared/errors"
"github.com/coreos/ignition/v2/config/util"

Expand All @@ -31,6 +36,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{}{}
Expand Down Expand Up @@ -61,5 +68,92 @@ func (cfg Config) Validate(c path.ContextPath) (r report.Report) {
r.AddOnError(c.Append("storage", "links", i, "path"), errors.ErrPathConflictsSystemd)
}
}
return

r.Merge(cfg.validateParents(c))

return r
}

func (cfg Config) validateParents(c path.ContextPath) report.Report {
yasminvalim marked this conversation as resolved.
Show resolved Hide resolved
var entries []struct {
Path string
Field string
}
r := report.Report{}

for i, f := range cfg.Storage.Files {
fmt.Println("File variable value:", f) // Added print statement
r = handlePathConflict(f.Path, "files", i, c, r, errors.ErrPathAlreadyExists)
addPathAndEntry(f.Path, "files", &entries)
}

for i, d := range cfg.Storage.Directories {
fmt.Println("Directory variable value:", d) // Added print statement
r = handlePathConflict(d.Path, "directories", i, c, r, errors.ErrPathAlreadyExists)
addPathAndEntry(d.Path, "directories", &entries)
}

for i, l := range cfg.Storage.Links {
fmt.Println("Link variable value:", l) // Added print statement
r = handlePathConflict(l.Path, "links", i, c, r, errors.ErrPathAlreadyExists)
addPathAndEntry(l.Path, "links", &entries)
}

sort.Slice(entries, func(i, j int) bool {
return depth(entries[i].Path) < depth(entries[j].Path)
})

for i, entry := range entries {
fmt.Println("Entry variable value:", entry) // Added print statement
if i > 0 && isWithin(entry.Path, entries[i-1].Path) {
if entries[i-1].Field != "directories" {
errorMsg := fmt.Errorf("invalid entry at path %s: %v", entry.Path, errors.ErrMissLabeledDir)
r.AddOnError(c.Append("storage", entry.Field, i, "path"), errorMsg)
fmt.Println("Error message:", errorMsg) // Added print statement
return r
}
}
}

return r
}

func handlePathConflict(path, fieldName string, index int, c path.ContextPath, r report.Report, err error) report.Report {
fmt.Println("Path variable value:", path) // Added print statement
if _, exists := paths[path]; exists {
r.AddOnError(c.Append("storage", fieldName, index, "path"), err)
fmt.Println("Error:", err) // Added print statement
}
return r
}

func addPathAndEntry(path, fieldName string, entries *[]struct{ Path, Field string }) {
*entries = append(*entries, struct {
Path string
Field string
}{Path: path, Field: fieldName})
fmt.Println("Added entry:", path) // Added print statement
}

func depth(path string) uint {
var count uint
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
}

func isWithin(newPath, prevPath string) bool {
fmt.Println("New path variable value:", newPath) // Added print statement
fmt.Println("Previous path variable value:", prevPath) // Added print statement
return strings.HasPrefix(newPath, prevPath) && newPath != prevPath
}
78 changes: 76 additions & 2 deletions config/v3_5_experimental/types/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
package types

import (
"fmt"
"reflect"
"testing"

"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"
)
Expand Down Expand Up @@ -183,7 +183,53 @@ func TestConfigValidation(t *testing.T) {
out: errors.ErrPathConflictsSystemd,
at: path.New("json", "storage", "links", 0, "path"),
},
// test 6: non-conflicting scenarios

// test 6: file path conflicts with another file path, should error
yasminvalim marked this conversation as resolved.
Show resolved Hide resolved
{
in: Config{
Storage: Storage{
Files: []File{
{Node: Node{Path: "/foo/bar"}},
{Node: Node{Path: "/foo/bar/baz"}},
},
},
},
out: fmt.Errorf("invalid entry at path /foo/bar/baz: %w", errors.ErrMissLabeledDir),
at: path.New("json", "storage", "files", 1, "path"),
},
// test 7: file path conflicts with link path, should error
{
in: Config{
yasminvalim marked this conversation as resolved.
Show resolved Hide resolved
Storage: Storage{
Files: []File{
{Node: Node{Path: "/foo/bar"}},
},
Links: []Link{
{Node: Node{Path: "/foo/bar/baz"}},
},
},
},
out: fmt.Errorf("invalid entry at path /foo/bar/baz: %w", errors.ErrMissLabeledDir),
at: path.New("json", "storage", "links", 1, "path"),
},

// test 8: file path conflicts with directory path, should error
{
in: Config{
Storage: Storage{
Files: []File{
{Node: Node{Path: "/foo/bar"}},
},
Directories: []Directory{
{Node: Node{Path: "/foo/bar/baz"}},
},
},
},
out: fmt.Errorf("invalid entry at path /foo/bar/baz: %w", 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
{
in: Config{
Storage: Storage{
Expand Down Expand Up @@ -248,7 +294,35 @@ func TestConfigValidation(t *testing.T) {
},
},
},

// test 10: non-conflicting scenarios with same parent directory, should not error
{
in: Config{
Storage: Storage{
Files: []File{
{Node: Node{Path: "/foo/bar/baz"}},
},
Directories: []Directory{
{Node: Node{Path: "/foo/bar"}},
},
},
},
},
// test 11: non-conflicting scenarios with a link, should not error
{
in: Config{
Storage: Storage{
Files: []File{
{Node: Node{Path: "/foo/bar"}},
},
Links: []Link{
{Node: Node{Path: "/baz/qux"}},
},
},
},
},
}

for i, test := range tests {
r := test.in.Validate(path.New("json"))
expected := report.Report{}
Expand Down
1 change: 1 addition & 0 deletions docs/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading