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

Conversation

yasminvalim
Copy link
Contributor

@yasminvalim yasminvalim commented Jan 25, 2024

Issue Summary:

Ignition has a bug (tracked in GitHub issue #1457) where it lacks proper validation when a file conflicts with the parent directory of another file, link, or directory. This bug allows users to create configurations that lead to internal inconsistencies. Feel free to verify this in Jira as well.

Expected Behavior:

Ignition should reject configurations that result in internal inconsistencies. Specifically, it should fail validation when files, links, or directories conflict with implicit parent directories.

Actual Behavior:

Currently, users can specify a file that conflicts with the parent directory, leading to potential internal inconsistencies.

Reproduction Steps:

Write a configuration with a file at /foo/bar and a file/directory/link at /foo/bar/baz.
OR
Write a configuration with a file at /foo/bar and a directory at /foo/bar/baz.
OR
Write a configuration with a file at /foo/bar and a link at /foo/bar/baz.

@yasminvalim yasminvalim marked this pull request as draft January 25, 2024 01:19
@yasminvalim yasminvalim force-pushed the ignition-25-92 branch 3 times, most recently from da6f0d3 to 08131d1 Compare February 1, 2024 13:47
Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall these look like a great start to TTD! looks like there is an understanding of the issue. Just a small pick and make sure to have a positive case that allows a file to be named the same as a parent directory as long as the parent directory is configured.

config/v3_5_experimental/types/config_test.go Show resolved Hide resolved
config/v3_5_experimental/types/config_test.go Show resolved Hide resolved
config/shared/errors/errors.go Outdated Show resolved Hide resolved
@yasminvalim yasminvalim force-pushed the ignition-25-92 branch 2 times, most recently from 1d4720e to a32d127 Compare February 2, 2024 20:32
@yasminvalim
Copy link
Contributor Author

Overall these look like a great start to TTD! looks like there is an understanding of the issue. Just a small pick and make sure to have a positive case that allows a file to be named the same as a parent directory as long as the parent directory is configured.

Thanks for your review! Good idea, I added positive cases!

@yasminvalim

This comment was marked as resolved.

@prestist

This comment was marked as resolved.

@yasminvalim yasminvalim self-assigned this Apr 15, 2024
@yasminvalim

This comment was marked as resolved.

@yasminvalim
Copy link
Contributor Author

Some benchmarking to check performance:

func BenchmarkValidateParents(b *testing.B) {
    cfg := Config{
        Storage: Storage{
            Files: []File{
                {Node: Node{Path: "/foo/bar"}},
            },
            Directories: []Directory{
                {Node: Node{Path: "/foo/bar/baz"}},
            },
        },
    }

    for i := 0; i < b.N; i++ {
        _ = cfg.validateParents(path.New("json"))
    }
}

goos: linux
goarch: amd64
pkg: github.com/coreos/ignition/v2/config/v3_5_experimental/types
cpu: 12th Gen Intel(R) Core(TM) i7-1270P
BenchmarkValidateParents-16      2720104           417.5 ns/op       440 B/op          9 allocs/op
PASS
ok      github.com/coreos/ignition/v2/config/v3_5_experimental/types    1.588s

OTHER EXAMPLE:

[func BenchmarkValidateParents(b *testing.B) {
    cfg := Config{
        Storage: Storage{
            Files: []File{
                {Node: Node{Path: "/foo/bar"}},
            },
            Links: []Link{
                {Node: Node{Path: "/foo/bar/baz"}},
            },
        },
    }

    for i := 0; i < b.N; i++ {
        _ = cfg.validateParents(path.New("json"))
    }
}

goos: linux
goarch: amd64
pkg: github.com/coreos/ignition/v2/config/v3_5_experimental/types
cpu: 12th Gen Intel(R) Core(TM) i7-1270P
BenchmarkValidateParents-16      2664310           429.1 ns/op       440 B/op          9 allocs/op
PASS
ok      github.com/coreos/ignition/v2/config/v3_5_experimental/types    1.604s
](url)

OTHER EXAMPLE

func BenchmarkValidateParents(b *testing.B) {
    cfg := Config{
        Storage: Storage{
            Files: []File{
                {Node: Node{Path: "/foo/bar"}},
                {Node: Node{Path: "/foo/bar/baz"}},
            },
        },
    }

    for i := 0; i < b.N; i++ {
        _ = cfg.validateParents(path.New("json"))
    }
}

goos: linux
goarch: amd64
pkg: github.com/coreos/ignition/v2/config/v3_5_experimental/types
cpu: 12th Gen Intel(R) Core(TM) i7-1270P
BenchmarkValidateParents-16      2810596           423.6 ns/op       440 B/op          9 allocs/op
PASS
ok      github.com/coreos/ignition/v2/config/v3_5_experimental/types    1.630s

@yasminvalim yasminvalim force-pushed the ignition-25-92 branch 12 times, most recently from 0741e46 to 14501a9 Compare April 17, 2024 19:42
@yasminvalim yasminvalim force-pushed the ignition-25-92 branch 13 times, most recently from b5512fb to 831dd43 Compare April 22, 2024 16:16
in: Config{
Storage: Storage{
Files: []File{
{Node: Node{Path: "C:\\foo\\bar"}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks weird. Why are we validating this with Windows style paths here? This is only about the final system which Linux only AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out.
I was having CI failures on Windows, so I created tests to see if my validations applied to Windows as well. They're now working on Windows, but the unit tests should indeed be exclusive to Linux.

Copy link
Member

@ravanelli ravanelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't merge the main branch here. You need to rebase instead

@prestist
Copy link
Collaborator

Have you had any luck with building the kola tests locally to see what is going on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants