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

validation: add tests for NSNewNSWithoutPath & NSInheritWithoutType #620

Merged

Conversation

dongsupark
Copy link
Contributor

@dongsupark dongsupark commented Apr 12, 2018

This PR is to check for tests NSNewNSWithoutPath as well as NSInheritWithoutType.

  • NSNewNSWithoutPath: "If path is not specified, the runtime MUST create a new container namespace of the given type."
  • NSInheritWithoutType: "If a namespace type is not specified in the namespaces array, the container MUST inherit the runtime namespace of that type."

See also #572

Signed-off-by: Dongsu Park dongsu@kinvolk.io

/cc @alban

@dongsupark dongsupark force-pushed the dongsu/test-ns-without-path branch 2 times, most recently from 40f829c to efd51d0 Compare April 12, 2018 15:52
@dongsupark dongsupark changed the title validation: add a new test for NSNewNSWithoutPath validation: add tests for NSNewNSWithoutPath & NSInheritWithoutType Apr 13, 2018
@dongsupark
Copy link
Contributor Author

Pushed a new commit for adding a validation test for NSInheritWithoutType.

@dongsupark dongsupark force-pushed the dongsu/test-ns-without-path branch 2 times, most recently from 2c2cd61 to d7985e3 Compare April 17, 2018 10:14
Dongsu Park added 2 commits April 17, 2018 12:15
This test is to check for NSNewNSWithoutPath, i.e. "If path is not
specified, the runtime MUST create a new container namespace of the
given type"

See also opencontainers#572

Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
This test is to check for NSInheritWithoutType, i.e. "If a namespace
type is not specified in the namespaces array, the container MUST
inherit the runtime namespace of that type".

See also opencontainers#572

Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
@alban
Copy link
Contributor

alban commented Apr 17, 2018

Testing with runc:

$ sudo validation/linux_ns_itype.t 
TAP version 13
ok 1 - inherit namespace cgroup without type
ok 2 - inherit namespace ipc without type
ok 3 - inherit namespace mnt without type
ok 4 - inherit namespace net without type
ok 5 - inherit namespace pid without type
ok 6 - inherit namespace user without type
ok 7 - inherit namespace uts without type
1..7

This one works fine for me, but I have SELinux warnings in the journal:

Apr 17 14:50:38 neptune python3[23713]: SELinux is preventing linux_ns_itype. from write access on the directory task.

  *****  Plugin catchall (100. confidence) suggests   **************************

  If you believe that linux_ns_itype. should be allowed write access on the task directory by default.
  Then you should report this as a bug.
  You can generate a local policy module to allow this access.
  Do allow this access for now by executing:
  # ausearch -c 'linux_ns_itype.' --raw | audit2allow -M my-linuxnsitype
  # semodule -X 300 -i my-linuxnsitype.pp

I guess we can ignore that.

The other one fails with runc because of opencontainers/runc#1184:

$ sudo validation/linux_ns_nopath.t 
TAP version 13
failed to create the container
namespace {"cgroup" ""} does not exist
exit status 1

The output is not valid TAP output.

@wking Should we generate a TAP "not ok" output for this error, or should we continue to use util.Fatal() here? I tried to clarify that in #621

@dongsupark Except those remarks, LGTM (but I'm not a maintainer)

@wking
Copy link
Contributor

wking commented Apr 17, 2018

@wking Should we generate a TAP "not ok" output for this error, or should we continue to use util.Fatal() here?

Your wording on this in #621 looks good to me. However the spec currently allows create to generate an error basically whenever it wants; there's no requirement for runtimes to support cgroup namespaces or anything else (opencontainers/runtime-spec#813). I think that blows a pretty big hole in cross-runtime portability, but as long as the spec is so loose there, I'm fine with either "not ok" or exit 1 for create errors (there are likely other places where we should replace util.Fatal with a "not ok" and exit 0 though).

@zhouhao3
Copy link

@dongsupark Changes LGTM overall, As they said, maybe you need to change the TAP output.

To be able to deal with diagnostics in a general way, split out the diagnostics
handling code into a new function `pringDiag()`, which will be normally called
from the defer function's context. This way we can simplify the error handling
routines.

Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
@dongsupark
Copy link
Contributor Author

dongsupark commented Apr 23, 2018

@q384566678 As you noted, I made these two tests print out diagnostics for every possible case. I hope it could address your review.

BTW travis builds suddenly started getting broken, because of this error:

# cd .; git clone https://go.googlesource.com/lint /home/travis/gopath/src/golang.org/x/lint
Cloning into '/home/travis/gopath/src/golang.org/x/lint'...
fatal: remote error: Git repository not found
package golang.org/x/lint: exit status 128

That looks like a totally unrelated issue.

EDIT: the golint issue is now fixed. Travis builds work fine.

@dongsupark dongsupark force-pushed the dongsu/test-ns-without-path branch from 763c8a8 to e830fa3 Compare April 23, 2018 13:20
To be able to deal with diagnostics in a general way, split out the diagnostics
handling code into a new function `pringDiag()`, which will be normally called
from the defer function's context. This way we can simplify the error handling
routines.

Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
@zhouhao3
Copy link

zhouhao3 commented Apr 24, 2018

LGTM
@liangchenye PTAL

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 1f9c0f1 into opencontainers:master May 3, 2018
@zhouhao3 zhouhao3 mentioned this pull request May 3, 2018
44 tasks
@dongsupark dongsupark deleted the dongsu/test-ns-without-path branch May 3, 2018 06:13
@dongsupark
Copy link
Contributor Author

@q384566678 Thanks for merging!

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.

4 participants