runtimetest: correctly check for a readable directory

[dongsupark]

So far tests linux_masked_paths.t & linux_readonly_paths.t have failed with error messages like cannot test read access for "/dirname". That's because testReadAccess() only checked if the given path is a regular file, returning an error for every other case. testReadAccess() should actually take care of another case of a given
path being a directory, to be able to correctly check for its readability.

Also run testFileReadAccess() or testFileWriteAccess() for all file types, not only a normal file, because the runtime spec does not mandate the type of masked files. It could be actually any file, including a character device like /dev/null, especially in case of runc.

Found by @alban.

Signed-off-by: Dongsu Park [email protected]

[alban]

With this branch, I get correctly formatted TAP output for both linux_masked_paths.t and linux_readonly_paths.t.

But I get this test failure with runc:

$ sudo validation/linux_masked_paths.t
...
not ok 275 - cannot read masked path "/masked-dir"

Is it a bug in the test or in runc?

[alban]

I think this needs 2 changes:

• in validation/linux_masked_paths.go: create a file in testDir, so that the directory is not empty
• in cmd/runtimetest/main.go::testDirectoryReadAccess(): don't ignore the return value of ioutil.ReadDir() with _ and return false when ioutil.ReadDir() gives an empty array.

The rationale is the same as explained in the comment of testFileReadAccess() (see also this discussion).

[dongsupark]

@alban Done.
Created a file under testDir, and added a check for an empty array.

[alban]

@dongsupark linux_masked_paths.t now works fine with runc but the test for linux_masked_paths.t became "skipped":

$ sudo validation/linux_masked_paths.t
...
ok 284 # SKIP linux.readonlyPaths not set

This is because testDir does not contain any files. Could you create a file in linux_readonly_paths.go in the same way you did in linux_masked_paths.go?

[alban]

I didn't test correctly before... It works for me now:

$ sudo validation/linux_masked_paths.t|grep -E 'mask|read'
ok 275 - cannot read masked path "/masked-dir"
ok 276 - cannot read masked path "/masked-file"
ok 284 # SKIP linux.readonlyPaths not set
$ sudo validation/linux_readonly_paths.t|grep -E 'mask|read'
ok 275 # SKIP linux.maskedPaths not set
ok 283 - "/readonly-dir" (linux.readonlyPaths[0]) is not writable
ok 284 - "/readonly-file" (linux.readonlyPaths[1]) is not writable

[wking]

I'd rather continue to test the mode. For example, the runtime may use a permission change to make a symlink read-only (or not read/writable at all), and we don't want to use testFileReadAccess to test symlink readability. I'd rather support /dev/null and other character devices with logic like:

switch fi.Mode&os.ModeType {
case 0, os.ModeDevice|os.ModeCharDevice:
    return testFileReadAccess(path)
case os.ModeDir:
    return restDirectoryReadAccess(path)
}
return false, fmt.Errorf("cannot test read access for %q (mode %d)", path, fi.Mode())

[wking]

I don't think we want to mask all errors like this. For example, we expect readonlyPaths entries to be readable, and I expect the tests should exit nonzero if, for example, this call returns an IsNotExist error. Can we follow testFileReadAccess and whitelist some errors as acceptable blockers (e.g. EACCES)? And can we also follow testFileReadAccess by only using non-empty test directories and treating successful-but-empty reads as “not readable”?

[wking]

I prefer keeping the explicit mode checks to avoid calling testFileWriteAccess on types where its logic doesn't make sense. And if runtimes seek to block write access by binding /dev/null over the path in question, we'll need a more sophisticated check than what testFileWriteAccess does now, because you can certainly write a byte to /dev/null.

[dongsupark]

@wking I'm not sure I understand. Can you please elaborate how we can do a more sophisticated check for writeability of /dev/null?
I don't see a general way to check if a character device can be written. AFAIK the read/write behavior varies a lot for each character device. The best we can do is to check for the file mode, isn't it?

[dongsupark]

@wking I pushed a commit to address your review comments as much as possible. I hope it looks ok to you.

[zhouhao3]

I didn't understand why do that. I tested it as if it had no effect on the results.

[dongsupark]

@q384566678 You're right.
What I wanted to do was defer os.Remove(tmpfile.Name()). Fixed it.

[zhouhao3]

LGTM

@dongsupark Merge it first, and then you can rebase #644. Maybe we can also change masked-path test.

[zhouhao3]

Maybe we can also change masked-path test.

Sorry, I didn't notice that you had already done it. Well done!

[dongsupark]

@q384566678 Done, rebased #643 as well as #644. :-)