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

runtimetest: Make TAP output more granular #308

Merged
merged 4 commits into from
Apr 11, 2018

Conversation

wking
Copy link
Contributor

@wking wking commented Jan 17, 2017

Granular TAP output makes it easier to see what's being tested (and what's going wrong). You could feed this into any TAP harness you like, but I've chosen prove (it seems popular). You need a TAP harness looking for test failures, because runtimetest now returns zero when it successfully runs the tests (regardless of whether the tests all pass). To avoid confusing the TAP consumer, all of the non-TAP logging goes to stderr (and not stdout).

With this change, the verbose output looks like:

# prove -v ./test_runtime.sh :: -r ~/bin/runc
./test_runtime.sh ..
TAP version 13
ok 1 # SKIP root.readonly falsy
ok 2 - hostname matches expected value
ok 3 - mount "/proc" found
ok 4 - mount "/dev" found
ok 5 - mount "/dev/pts" found
ok 6 - mount "/dev/shm" found
ok 7 - mount "/dev/mqueue" found
ok 8 - mount "/sys" found
ok 9 - expected capability CAP_CHOWN set
ok 10 - expected capability CAP_DAC_OVERRIDE set
ok 11 - unexpected capability CAP_DAC_READ_SEARCH not set
ok 12 - expected capability CAP_FOWNER set
ok 13 - expected capability CAP_FSETID set
ok 14 - expected capability CAP_KILL set
ok 15 - expected capability CAP_SETGID set
ok 16 - expected capability CAP_SETUID set
ok 17 - expected capability CAP_SETPCAP set
ok 18 - unexpected capability CAP_LINUX_IMMUTABLE not set
ok 19 - expected capability CAP_NET_BIND_SERVICE set
ok 20 - unexpected capability CAP_NET_BROADCAST not set
ok 21 - unexpected capability CAP_NET_ADMIN not set
ok 22 - expected capability CAP_NET_RAW set
ok 23 - unexpected capability CAP_IPC_LOCK not set
ok 24 - unexpected capability CAP_IPC_OWNER not set
ok 25 - unexpected capability CAP_SYS_MODULE not set
ok 26 - unexpected capability CAP_SYS_RAWIO not set
ok 27 - expected capability CAP_SYS_CHROOT set
ok 28 - unexpected capability CAP_SYS_PTRACE not set
ok 29 - unexpected capability CAP_SYS_PACCT not set
ok 30 - unexpected capability CAP_SYS_ADMIN not set
ok 31 - unexpected capability CAP_SYS_BOOT not set
ok 32 - unexpected capability CAP_SYS_NICE not set
ok 33 - unexpected capability CAP_SYS_RESOURCE not set
ok 34 - unexpected capability CAP_SYS_TIME not set
ok 35 - unexpected capability CAP_SYS_TTY_CONFIG not set
ok 36 - expected capability CAP_MKNOD set
ok 37 - unexpected capability CAP_LEASE not set
ok 38 - expected capability CAP_AUDIT_WRITE set
ok 39 - unexpected capability CAP_AUDIT_CONTROL not set
ok 40 - expected capability CAP_SETFCAP set
ok 41 - unexpected capability CAP_MAC_OVERRIDE not set
ok 42 - unexpected capability CAP_MAC_ADMIN not set
ok 43 - unexpected capability CAP_SYSLOG not set
ok 44 - unexpected capability CAP_WAKE_ALARM not set
ok 45 - unexpected capability CAP_BLOCK_SUSPEND not set
ok 46 - unexpected capability CAP_AUDIT_READ not set
ok 47 - lstat default symlink /dev/fd
ok 48 - default symlink /dev/fd is a symlink
ok 49 - default symlink /dev/fd has expected target
ok 50 - lstat default symlink /dev/stdin
ok 51 - default symlink /dev/stdin is a symlink
ok 52 - default symlink /dev/stdin has expected target
ok 53 - lstat default symlink /dev/stdout
ok 54 - default symlink /dev/stdout is a symlink
ok 55 - default symlink /dev/stdout has expected target
ok 56 - lstat default symlink /dev/stderr
ok 57 - default symlink /dev/stderr is a symlink
ok 58 - default symlink /dev/stderr has expected target
ok 59 - mount /proc has expected type
ok 60 - mount /sys has expected type
ok 61 - mount /dev/pts has expected type
ok 62 - mount /dev/shm has expected type
ok 63 - stat default device /dev/null
ok 64 - default device /dev/null is a device
ok 65 - stat default device /dev/zero
ok 66 - default device /dev/zero is a device
ok 67 - stat default device /dev/full
ok 68 - default device /dev/full is a device
ok 69 - stat default device /dev/random
ok 70 - default device /dev/random is a device
ok 71 - stat default device /dev/urandom
ok 72 - default device /dev/urandom is a device
ok 73 - stat default device /dev/tty
ok 74 - default device /dev/tty is a device
ok 75 - stat default device /dev/ptmx
ok 76 - default device /dev/ptmx is a device
ok 77 # SKIP linux.devices (no devices configured)
ok 78 - has expected working directory
ok 79 - has expected environment variable PATH
ok 80 - has expected environment variable TERM
ok 81 - has expected user ID
ok 82 - has expected group ID
ok 83 - has expected number of process arguments
ok 84 - has expected process argument 0
ok 85 - has expected noNewPrivileges
ok 86 # SKIP linux.resources.oomScoreAdj falsy
ok 87 - has expected soft RLIMIT_NOFILE
ok 88 - has expected hard RLIMIT_NOFILE
ok 89 # SKIP linux.uidMappings checks (no mappings specified)
ok 90 # SKIP linux.gidMappings checks (no mappings specified)
1..90
ok
All tests successful.
Files=1, Tests=90,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.04 cusr  0.01 csys =  0.07 CPU)
Result: PASS

@wking wking force-pushed the granular-tap branch 2 times, most recently from c30a87d to 459c384 Compare January 17, 2017 04:59
@Mashimiao
Copy link

I think that's too detailed...

@wking
Copy link
Contributor Author

wking commented Jan 17, 2017 via email

@mrunalp
Copy link
Contributor

mrunalp commented Jan 17, 2017

My first reaction was that it was too granular but I see how it could be helpful.

@Mashimiao
Copy link

@wking can you rebase this?

@wking
Copy link
Contributor Author

wking commented Mar 8, 2017 via email

@Mashimiao
Copy link

@liangchenye How do you think about this PR?

@wking
Copy link
Contributor Author

wking commented Nov 24, 2017

If folks are considering v0.4.0 PRs in ascending-date order, I'd prefer they skip over this one and look at #439 first. That PR is current, and it's easier to rebase than this one. I'll try and get this rebased in the next week or so, but this PR will need another rebase if the older and also milestoned for v0.4.0 #93 lands first.

@Mashimiao
Copy link

#93 needs more work, I have put it into lower priority than this one.

@zhouhao3
Copy link

zhouhao3 commented Jan 11, 2018

@wking Others have been merged,I think it is time to update this pr.

wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 11, 2018
No semantic changes here, but the DRYer approach makes future pivots
easier (e.g. opencontainers#308).

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 11, 2018
No semantic changes here, but the DRYer approach makes future pivots
easier (e.g. opencontainers#308).

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented Jan 11, 2018 via email

@zhouhao3
Copy link

@wking Excuse me, how about this patch update? I'm ready release 0.4 version, I think this patch need to merge before the release.

@wking
Copy link
Contributor Author

wking commented Jan 19, 2018

Sorry, I'm only ~ half way through, and I still need to make adjustments to handle specerror. Maybe bump this to 0.5?

@zhouhao3
Copy link

Fine.

@zhouhao3 zhouhao3 modified the milestones: v0.4.0, v0.5.0 Jan 19, 2018
@zhouhao3
Copy link

zhouhao3 commented Mar 6, 2018

@wking Can you update this pr? So we can put it in 0.6.

@wking
Copy link
Contributor Author

wking commented Apr 6, 2018

Finally rebased onto master with 459c3847c384c1. The diff is huge, and I only just got through it, so give it a careful read before merging. On the other hand, don't wait too long or we'll have another big rebase to go through ;). I'll go back through in the next few days and try to double check things as well.

@wking wking force-pushed the granular-tap branch 2 times, most recently from 9ec0740 to d7e4d91 Compare April 6, 2018 13:34
Copy link
Contributor

@alban alban left a comment

Choose a reason for hiding this comment

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

I like it so far. I gave it a try and it seems to work. I am still reading the diff.

if err == nil {
return true, nil
} else if err == io.EOF {
return false, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this surprising to return "non-readable" when the file might well be readable but just empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find this surprising to return "non-readable" when the file might well be readable but just empty.

Yeah, but I'm not sure how to detect readable-but-empty (and what I have here is what we've been using). Do you have ideas? The lack of support for that case makes this brittle, but I'm ok with it if we update our tests (e.g. here) to use non-empty test files. runtimetest is for internal testing only (not part of our public API), so a few cludges to work around the runtimetest limitation are unfortunate but survivable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, our current tests use an empty file and treat an immediate EOF as success. So I am changing the logic. But the old logic doesn't make much sense to me. What if the runtime implemented linux.readonlyPaths by binding /dev/null over the listed paths? That would seem to satisfy the spec (which is pretty weak on this anyway), while still resulting in immediate EOFs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just add a comment to explain the situation:

	} else if err == io.EOF {
                // linux_readonly_paths.go only tests non-empty files. So if we find
                // an empty file, the runtime did not apply the ReadonlyPaths correctly
		return false, nil

I agree about updating the test you mention :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree about updating the test you mention :)

I did that and added a comment similar to the one you suggested in 857249d06482da. How does that look? I think I'll have to suck it up and a testDirectoryReadAccess as well, but I haven't had time to work that up yet.

tmpfile.Close()
os.RemoveAll(filepath.Join(path, tmpfile.Name()))
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

this branch cannot be taken because it was tested a few lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this branch cannot be taken because it was tested a few lines above.

Thanks :). I've dropped it with d7e4d91857249d.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can it use WriteFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can it use WriteFile?

Good idea. I've updated to use WriteFile for the whole PR with 7ecd67992de75c.

Copy link
Contributor

@alban alban left a comment

Choose a reason for hiding this comment

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

More comments. Reviewed up to line 920 (validateOOMScoreAdj)

}
if spec.Linux.RootfsPropagation == "shared" {
return nil
c.harness.Ok(exposed, fmt.Sprintf("shared root propogation exposes %q", targetFile))
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the spec quite weak about this RootfsPropagation:

  • a mount could be both shared and slave
  • rootfsPropagation=slave is only meaningful if we say what is the master of that enslaved rootfs. Should it necessarily be wrt to a mount in the host namespace?

But for the purpose of this PR, your change is fine.

Copy link
Contributor Author

@wking wking Apr 6, 2018

Choose a reason for hiding this comment

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

Should it necessarily be wrt to a mount in the host namespace?

This is another case where we can work around runtimetest limitations by limiting the configs we create in validation/ to things runtimetest can check for ;).

But for the purpose of this PR, your change is fine.

Yeah, I'm fine making not growing this PR to fix everything ;). And if folks want me to spin off anything I am doing (like adjusting the readonly tests), let me know and I'll try to spin those out into their own PRs.

} else {
exists = true
}
rfcError, err := c.Ok(exists, specerror.DevicesAvailable, version, fmt.Sprintf("has a file at %s", description))
Copy link
Contributor

Choose a reason for hiding this comment

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

specerror.DevicesAvailable -> condition

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto in the 6 other occurrences of specerror.DevicesAvailable below in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

specerror.DevicesAvailable -> condition

Thanks. I've fixed all of those with 06482da031e4c8.

})
}

if description == "/dev/console (default device)" {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it be better to test

if device.Path == "/dev/console" {

Copy link
Contributor Author

@wking wking Apr 6, 2018

Choose a reason for hiding this comment

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

wouldn't it be better to test

if device.Path == "/dev/console" {

I'm fine with that. My current check ensures that we do perform the rest of these checks if the config JSON explicitly included /dev/console in linux.devices. But we don't do that in validation/, and I don't expect us to start, so it probably doesn't matter either way. Do any maintainers have a preference?

continue
}

isSymlink := fi.Mode()&os.ModeSymlink == os.ModeSymlink
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it more clear written this way:

isSymlink := fi.Mode()&os.ModeType == os.ModeSymlink

but that's equivalent in Golang because os.ModeType is a bitfield.

It confused me at first because in C, stat.st_mode is not a bitfield (man 7 inode):
S_IFLNK = S_IFREG | S_IFCHR

       The following mask values are defined for the file type:

           S_IFMT     0170000   bit mask for the file type bit field

           S_IFSOCK   0140000   socket
           S_IFLNK    0120000   symbolic link
           S_IFREG    0100000   regular file
           S_IFBLK    0060000   block device
           S_IFDIR    0040000   directory
           S_IFCHR    0020000   character device
           S_IFIFO    0010000   FIFO

(Things I learnt today)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it was already written like that before. Feel free to ignore my comment ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it more clear written this way…

I like that argument. I've pushed the change as a new commit with 7ecd679, in case the maintainers want me to spin it out into a separate PR.


for i, path := range spec.Linux.ReadonlyPaths {
readable, err := testReadAccess(path)
if err != nil && !os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ignoring the os.IsNotExist error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ignoring the os.IsNotExist error?

Because if the path is not there at all, it's certainly not readable ;). But while that makes some sense for maskedPaths, runtimes should probably not go that far with readonlyPaths. I've dropped it from the readonlyPaths checker in 031e4c85e395e8.

if err != nil && !os.IsNotExist(err) {
return err
}
if !writable {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the if? The condition is already in the c.harness.Ok() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why the if? The condition is already in the c.harness.Ok() below.

A sloppy copy/paste from the non-fatal readable test a few lines up. Fixed by dropping this if in 5e395e81e67d33.

@wking wking force-pushed the granular-tap branch 3 times, most recently from 5e395e8 to 1e67d33 Compare April 6, 2018 17:08
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
We've been using ModeSymlink as the mask since the check landed in
da25004 (runtimetest: add linux default symbolic link validation,
2016-11-30, opencontainers#284).  POSIX provides S_IS*(m) macros to portably
interpret the mode type, but does not define values for each type [2].
Alban pointed out that st_mode is not a bitfield on Linux [1].  For
example, Linux defines [3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/fileutils that referenced this pull request Apr 6, 2018
POSIX provides S_IS*(m) macros to portably interpret the mode type,
but does not define values for each type [2].  Alban pointed out that
st_mode is not a bitfield on Linux [1].  For example, Linux defines
[3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers/runtime-tools#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45
wking added a commit to wking/fileutils that referenced this pull request Apr 6, 2018
POSIX provides S_IS*(m) macros to portably interpret the mode type,
but does not define values for each type [1].  Alban pointed out that
st_mode is not a bitfield on Linux [2].  For example, Linux defines
[3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[2]: opencontainers/runtime-tools#308 (comment)
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45
wking added a commit to wking/fileutils that referenced this pull request Apr 6, 2018
POSIX provides S_IS*(m) macros to portably interpret the mode type,
but does not define values for each type [1].  Alban pointed out that
st_mode is not a bitfield on Linux [2].  For example, Linux defines
[3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

I've also dropped the 07000 portion of the previous 07777 mask in
favor of the cross-platform ModePerm mask.  This avoids an internal
magic number, and the sticky, suid, and sgid bits don't make sense for
device nodes.  And I'm using some contants from os instead of their
syscall analogs.  We can't drop the syscall dependency, because we're
still using syscall to construct the Mknod arguments, but with this
commit we're no longer using it to inspect the source file type.

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[2]: opencontainers/runtime-tools#308 (comment)
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
We've been using ModeSymlink as the mask since the check landed in
da25004 (runtimetest: add linux default symbolic link validation,
2016-11-30, opencontainers#284).  POSIX provides S_IS*(m) macros to portably
interpret the mode type, but does not define values for each type [2].
Alban pointed out that st_mode is not a bitfield on Linux [1].  For
example, Linux defines [3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
We've been using ModeSymlink as the mask since the check landed in
da25004 (runtimetest: add linux default symbolic link validation,
2016-11-30, opencontainers#284).  POSIX provides S_IS*(m) macros to portably
interpret the mode type, but does not define values for each type [2].
Alban pointed out that st_mode is not a bitfield on Linux [1].  For
example, Linux defines [3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
We've been using ModeSymlink as the mask since the check landed in
da25004 (runtimetest: add linux default symbolic link validation,
2016-11-30, opencontainers#284).  POSIX provides S_IS*(m) macros to portably
interpret the mode type, but does not define values for each type [2].
Alban pointed out that st_mode is not a bitfield on Linux [1].  For
example, Linux defines [3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45

Signed-off-by: W. Trevor King <[email protected]>
wking added 3 commits April 6, 2018 11:51
Granular TAP output makes it easier to see what's being tested (and
what's going wrong).  You'll want a TAP harness looking for test
failures, because runtimetest now returns a lot of information, most
of which is (hopefully ;) about passing tests.  In order to accomplish
this change, I've setup a complianceTester structure to make it easy
to pass the TAP harness down into the helper functions.
complianceTester also holds the target compliance level, so it can
decide whether a spec violation is sufficiently serious to be a failed
test violoation or is minor enough to be a warning.  I've used skips
for the warnings, since they seemed like the closest fit from the TAP
spec.

And I've made a number of other improvements while I was working
through this, but haven't written them all up in this commit message.

One change is the new support for testing file write access;
previously we only tested directory write access.  Its possible that
some WriteFile errors should kill the test suite, but for now I'm
counting all of them as successful "cannot write" determinations.

Signed-off-by: W. Trevor King <[email protected]>
Previously we were only looking at directories.  But the spec does not
say these are directory-only options, so test files too.  Put some
content in the files, because runtimetest currently has no way to
check the readability of empty files.

Signed-off-by: W. Trevor King <[email protected]>
We've been using ModeSymlink as the mask since the check landed in
da25004 (runtimetest: add linux default symbolic link validation,
2016-11-30, opencontainers#284).  POSIX provides S_IS*(m) macros to portably
interpret the mode type, but does not define values for each type [2].
Alban pointed out that st_mode is not a bitfield on Linux [1].  For
example, Linux defines [3]:

  S_IFBLK 060000
  S_IFDIR 040000
  S_IFCHR 020000

So 'm&S_IFCHR == S_IFCHR', for example, would succeed for both
character and block devices.  Go translates the system values to a
platform-agnostic bitfield [4], so the previous approach works on Go.
But it may be confusing for people used to the native non-bitfield
mode, so this commit moves us to an approach that does not rely on
Go's using a bitfield.

[1]: opencontainers#308 (comment)
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stat.h?h=v4.16#n9
[4]: https://github.com/golang/go/blob/b0d437f866eb8987cde7e6550cacd77876f36d4b/src/os/types.go#L45

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented Apr 6, 2018

I've fixed a few typos that had snuck in with my recent rerolls, and Travis is happy again. Also, in case folks are interested, here's an example of how node-tap renders the runtimetest output if I launch it with a random config.json from my host as an unprivileged user (without bothering to actually use a runtime or launch a container ;):

$ tap ./runtimetest
./runtimetest ..................................... 199/275
  not ok has expected working directory
    actual: /…/src/github.com/opencontainers/runtime-tools
    expected: /
  
  not ok mounts[1] (/tmp) found
  not ok mounts[2] (/dev) found in order
    config:
      destination: /dev
      type: devtmpfs
      source: devtmpfs
    earlier:
      config:
        destination: /tmp
        type: tmpfs
        source: none
      indexConfig: 0
      indexSystem: 18
    indexConfig: 2
    indexSystem: 0
    level: MUST
    reference: 'https://github.com/opencontainers/runtime-spec/blob/v1.1.0/config.md#mounts'
  
  not ok has expected user ID
    actual: 1000
    expected: 1
  
  not ok has expected group ID
    actual: 1000
    expected: 1
  
  not ok unexpected bounding capability CAP_CHOWN not set
  not ok unexpected bounding capability CAP_DAC_OVERRIDE not set
  not ok unexpected bounding capability CAP_DAC_READ_SEARCH not set
  not ok unexpected bounding capability CAP_FOWNER not set
  not ok unexpected bounding capability CAP_FSETID not set
  not ok unexpected bounding capability CAP_KILL not set
  not ok unexpected bounding capability CAP_SETGID not set
  not ok unexpected bounding capability CAP_SETUID not set
  not ok unexpected bounding capability CAP_SETPCAP not set
  not ok unexpected bounding capability CAP_LINUX_IMMUTABLE not set
  not ok unexpected bounding capability CAP_NET_BIND_SERVICE not set
  not ok unexpected bounding capability CAP_NET_BROADCAST not set
  not ok unexpected bounding capability CAP_NET_ADMIN not set
  not ok unexpected bounding capability CAP_NET_RAW not set
  not ok unexpected bounding capability CAP_IPC_LOCK not set
  not ok unexpected bounding capability CAP_IPC_OWNER not set
  not ok unexpected bounding capability CAP_SYS_MODULE not set
  not ok unexpected bounding capability CAP_SYS_RAWIO not set
  not ok unexpected bounding capability CAP_SYS_CHROOT not set
  not ok unexpected bounding capability CAP_SYS_PTRACE not set
  not ok unexpected bounding capability CAP_SYS_PACCT not set
  not ok unexpected bounding capability CAP_SYS_ADMIN not set
  not ok unexpected bounding capability CAP_SYS_BOOT not set
  not ok unexpected bounding capability CAP_SYS_NICE not set
  not ok unexpected bounding capability CAP_SYS_RESOURCE not set
  not ok unexpected bounding capability CAP_SYS_TIME not set
  not ok unexpected bounding capability CAP_SYS_TTY_CONFIG not set
  not ok unexpected bounding capability CAP_MKNOD not set
  not ok unexpected bounding capability CAP_LEASE not set
  not ok unexpected bounding capability CAP_AUDIT_WRITE not set
  not ok unexpected bounding capability CAP_AUDIT_CONTROL not set
  not ok unexpected bounding capability CAP_SETFCAP not set
  not ok unexpected bounding capability CAP_MAC_OVERRIDE not set
  not ok unexpected bounding capability CAP_MAC_ADMIN not set
  not ok unexpected bounding capability CAP_SYSLOG not set
  not ok unexpected bounding capability CAP_WAKE_ALARM not set
  not ok unexpected bounding capability CAP_BLOCK_SUSPEND not set
  not ok unexpected bounding capability CAP_AUDIT_READ not set
  not ok has expected process argument 0
    actual: ./runtimetest
    expected: sh
    index: 0

  Skipped: 32
     root.readonly is false but the root filesystem is still not writable
     hostname not set
     /dev/null (default device) has unconfigured permissions
     /dev/null (default device) has an unconfigured user ID
     /dev/null (default device) has an unconfigured group ID
     /dev/zero (default device) has unconfigured permissions
     /dev/zero (default device) has an unconfigured user ID
     /dev/zero (default device) has an unconfigured group ID
     /dev/full (default device) has unconfigured permissions
     /dev/full (default device) has an unconfigured user ID
     /dev/full (default device) has an unconfigured group ID
     /dev/random (default device) has unconfigured permissions
     /dev/random (default device) has an unconfigured user ID
     /dev/random (default device) has an unconfigured group ID
     /dev/urandom (default device) has unconfigured permissions
     /dev/urandom (default device) has an unconfigured user ID
     /dev/urandom (default device) has an unconfigured group ID
     /dev/tty (default device) has unconfigured permissions
     /dev/tty (default device) has an unconfigured user ID
     /dev/tty (default device) has an unconfigured group ID
     /dev/ptmx (default device) has unconfigured permissions
     /dev/ptmx (default device) has an unconfigured user ID
     /dev/ptmx (default device) has an unconfigured group ID
     linux.devices is not set
     linux.maskedPaths not set
     process.oomScoreAdj not set
     linux.seccomp not set
     linux.readonlyPaths not set
     linux.rootfsPropagation not set
     linux.sysctl not set
     linux.uidMappings not set
     linux.gidMappings not set

total ............................................. 199/275
  

  199 passing (102.442ms)
  32 pending
  44 failing

I ran these examples again with:

  $ runc --version
  runc version 1.0.0-rc5+dev
  commit: cc4307ab6643668ce5abc6b524e1764a54c32550
  spec: 1.0.0
  $ uname -r
  4.15.0

I don't know what that hugetlb permission error is about, but that's
what I see ;).  It could be an issue with my local system.

Signed-off-by: W. Trevor King <[email protected]>
@zhouhao3
Copy link

zhouhao3 commented Apr 10, 2018

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 8b973a6 into opencontainers:master Apr 11, 2018
@wking wking deleted the granular-tap branch April 11, 2018 05:56
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