runtimetest: fix root readonly check

[alban]

Instead of trying to write a file on the rootfs, check the "ro"
per-mount option on the root mountpoint. The rootfs might not be
readable despite spec.Root.Readonly being false. Example: the rootfs
belong to an unmapped uid.

Signed-off-by: Alban Crequy alban@kinvolk.io

[wking]

The rootfs might not be readable despite spec.Root.Readonly being false. Example: the rootfs belong to an unmapped uid.

Checking the spec again, it doesn't actually say anything about how the runtime should handle the readonly false and unset cases. So I'm in favor of something like:

if spec.Root.Readonly {
  err := testWriteAccess("/")
  if err == nil {
    return specerror.NewError(specerror.RootReadonlyImplement, fmt.Errorf("rootfs must be readonly"), rspec.Version)
} else {
  err := testWriteAccess("/")
  if err != nil {
    // print a Skip-level warning, which may depend on #594 or other *tap.T passing
  }
}

That way we don't have to have an opinion on unspecified implementation details (perhaps the runtime doesn't bother with a ro mount if it knows the ID mapping will cover read-only-ness?).

[alban]

@wking what about this updated patch? I just skip the test on userns if spec.Root.Readonly == false, but still execute the test for validation/root_readonly_{false,true}.go.

I don't think I need to use *tap.T from #594 right now, it will just t.Pass() when err == nil. Do we actually have a "Skip-level warning"? Do you mean using "OPTIONAL"?

[wking]

I just skip the test on userns if spec.Root.Readonly == false...

I agree that the unmapped user-namespace situation should not be a spec violation. But I don't see a need for special handling there, because I don't see spec grounds for this error at all. Do you?

I don't think I need to use *tap.T from #594 right now, it will just t.Pass() when err == nil. Do we actually have a "Skip-level warning"? Do you mean using "OPTIONAL"?

I don't think we can use OPTIONAL, or even MAY, unless we're quoting spec wording, and the spec has nothing in that line for unset/false readonly. So I'd be ok with just:

if spec.Root.Readonly {
   // what's in master now
} // no need to check the else case

But I also think that users may find this surprising, and a TAP skip or stderr log are places where we can warn the user "hey, you might have expected setting readonly false would give you a writeable root, but yours isn't writeable. This isn't a spec volation, but we wanted to let you know anyway.".

[alban]

I agree that the unmapped user-namespace situation should not be a spec violation. But I don't see a need for special handling there, because I don't see spec grounds for this error at all. Do you?

No, I don't.

I don't think we can use OPTIONAL, or even MAY, unless we're quoting spec wording, and the spec has nothing in that line for unset/false readonly. So I'd be ok with just:

Ok, I updated the patch that way. I also deleted validation/root_readonly_false.go since that would not test anything.

But I also think that users may find this surprising, and a TAP skip or stderr log are places where we can warn the user "hey, you might have expected setting readonly false would give you a writeable root, but yours isn't writeable. This isn't a spec volation, but we wanted to let you know anyway.".

That would be good indeed. I didn't do that in this PR because that would need more changes for the TAP skip.

[liangchenye]

LGTM