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

test_runtime.sh: Add a user namespace #114

Closed

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 18, 2016

You shouldn't need to be root to test a runtime. The id calls use the
POSIX command to find the current user's user and group IDs.

Builds on #113, so review that first.

@wking
Copy link
Contributor Author

wking commented Jun 18, 2016

An alternative to picking one way or the other is to use a test framework with both and skip the entries which require root when the caller isn't. For examples in the ccon test suite, see:

@wking wking force-pushed the test-runtime-user-namespace branch from 01c92d6 to fdbcc47 Compare June 21, 2016 22:36
You shouldn't need to be root to test a runtime.  The id calls use the
POSIX command [1] to find the current user's user and group IDs.

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/id.html

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Jun 21, 2016

Rebased onto master with 01c92d6fdbcc47 now that #113 has landed (so GitHub will recognize this as a single-commit PR).

@liangchenye
Copy link
Member

Hi @wking , how does it work? It just adds maps to config.json, but a runtime (runc) still need 'root' to create/start the container.

@wking
Copy link
Contributor Author

wking commented Jul 4, 2016

On Mon, Jul 04, 2016 at 03:56:22AM -0700, 梁辰晔 (Liang Chenye) wrote:

Hi @wking , how does it work? It just adds maps to config.json, but
a runtime (runc) still need 'root' to create/start the container.

If runC still needs root, you can use ‘-r 'sudo runc'’ or call the
script with ‘sudo test_runtime.sh’. But ccon-oci 1 does not need root,
so it's nicer for me to not elevate privileges before running a test
suite that I haven't audited.

And my preferred approach is to have the runtime tests cover both
privileged and unprivileged callers 2, which would give me the
unprivileged tests I'm looking for (and skips for the privileged
tests) while keeping privileged tests for runC (with skips for the
unprivileged tests). Folks that want to test for complete coverage
would want to run both the privileged and unprivileged tests. When
testing ccon, I do this with two passes 3:

$ make

ok 4 # skip Test privileged process.user.uid (missing ROOT of ECHO,ID,ROOT)

fixed 0
success 49
failed 0
broken 0
total 58

$ sudo make

ok 7 # skip Test unprivileged process.user.uid (missing !ROOT of ECHO,GREP,ID,!ROOT)

fixed 0
success 53
failed 0
broken 0
total 58

@mrunalp
Copy link
Contributor

mrunalp commented Jul 5, 2016

This did not work with runc last time I tried. I'll try again tomorrow (still on PTO).

@wking
Copy link
Contributor Author

wking commented Jul 5, 2016

On Tue, Jul 05, 2016 at 08:29:08AM -0700, Mrunal Patel wrote:

This did not work with runc last time I tried. I'll try again
tomorrow (still on PTO).

Searching runC for open user namespace issues turned up
opencontainers/runc#252 and the setgroups call in
opencontainers/runc#774 and opencontainers/runc#814. There's also the
gid=5 option in the default template's /dev/pts mount.

But stepping back, this is a good reason to have a test suite that
iterates over a number of configurations 1, so we can fail runC when
it doesn't support a particular use-case and pass it when it does, but
don't have to tailor the test suite to target what runC currently
supports.

@wking
Copy link
Contributor Author

wking commented Jul 5, 2016

On Tue, Jul 05, 2016 at 09:20:01AM -0700, W. Trevor King wrote:

Tue, Jul 05, 2016 at 08:29:08AM -0700, Mrunal Patel:

This did not work with runc last time I tried. I'll try again
tomorrow (still on PTO).

Searching runC for open user namespace issues…

Also opencontainers/runc#799.

@cyphar
Copy link
Member

cyphar commented Jul 22, 2016

Yes, there are a couple of user namespace issues I've found in runC as a result of opencontainers/runc#774. They include:

  • Everything to do with consoles.
  • Namespace joining and creation issues (these are solved by nsenter: major cleanups runc#950).
  • setgroups doesn't like me at all, it seems.
  • The default config (which is understandable) has gid=5.

I mentioned in the rootless containers PR that we really need to make our test suite do checks for user namespaces because quite a few things break that shouldn't. Problem is that you can't even run a single integration test case with the current console setup code (which is something that I mentioned in the last weekly call is something I'm working on fixing).

@wking
Copy link
Contributor Author

wking commented Jul 22, 2016

On Fri, Jul 22, 2016 at 11:30:56AM -0700, Aleksa Sarai wrote:

Yes, there are a couple of user namespace issues I've found in runC…

And while fixing runC to work with user namespaces would be nice, I
think this PR is really a band-aid for testing without root. I'd much
rather have a validation test suite that covered both root and
non-root invocations and user-namespace and non configs 1. Do we
expect #61 to get picked up again any time soon? Can I PR some bats
stuff cribbed from runC? Or some sharness stuff cribbed from ccon?

@mrunalp
Copy link
Contributor

mrunalp commented Jul 22, 2016

Yeah, I think we should make progress on #61. We can review it and request a rebase.

@Mashimiao
Copy link

@opencontainers/runtime-tools-maintainers should we close this PR?

@wking
Copy link
Contributor Author

wking commented Nov 16, 2016

On Wed, Nov 16, 2016 at 01:13:39AM -0800, Ma Shimiao wrote:

should we close this PR?

I'm fine closing this PR, but would like more clarity on the path
forward. #61 has not seen progress since May, and I haven't seen a
reason to expect development on it to pick back up in the near future.
I've offered to submit a bats or sharness runtime test suite 1 and
asked for clarification on resitance to those approaches 2. But I
think we should have progress in one of those directions (#61 or a
bats/sharness runtime test suite) before we close this pain-point PR.

@cyphar
Copy link
Member

cyphar commented Nov 16, 2016

For an update, since the last time I posted here my rootless containers PR (opencontainers/runc#774) now passes all of the runC tests. So runC no longer has the same issue it did before (though you need to create a different config using runc spec --rootless for the same reasons described above -- configurations become invalid very easily with user namespaced containers where you only map a single user).

The only real issue I have with this PR is that it uses user namespaces for all tests. While this sounds like a no-op in the root-to-root case it actually isn't (you are messing with a lot of different aspects of the final environment). So there should be a flag to enable user namespace testing.

Also, as an aside runC's test suite also now works with regular user namespaces (which wasn't possible before without --console-socket) because of opencontainers/runc#1018 and opencontainers/runc#774.

@wking
Copy link
Contributor Author

wking commented Nov 16, 2016

On Wed, Nov 16, 2016 at 09:48:55AM -0800, Aleksa Sarai wrote:

The only real issue I have with this PR is that it uses user
namespaces for all tests…

There's no way to get this right when you only run one test container
;), which is why I'd like an actual test harness running multiple
tests (#61, or sharness like ccon, or bats like runC, …).

@Mashimiao
Copy link

As test_runtime.sh is removing as planed, this PR will not be needed.

@Mashimiao Mashimiao closed this Mar 6, 2017
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