Running Moby Tests as Unprivileged User

[mysteriouspants]

I work at a Linux distribution in software packaging and I've recently undertaken to package docker-24.0.5. In brief, we build moby and docker/cli from source using the golang provided from our own package repositories. We then execute the test suite from both moby and docker to validate that the build was successful.

However, there are a large number of moby tests which expect to be run with root privileges. I've fixed this by using a patch to inject skip.If(t, os.Getuid() != 0, "requires root") in these tests, 101 in all according to grep '\+\s*skip.If | wc -l. I'd like to contribute this upstream in some way. If this were baked into moby, we can stop carrying this as a vendor-specific patch - and it will hopefully make it easier for other distributions to package moby as well.

I'd like some guidance on what, if anything, the project would like to see in these patches. I can think of three ways to accomplish this, though presumably there are more.

1. We can use the existing "am I root?" check. I'm not sure if this is good, since I'm not sure how moby developers execute these tests themselves - do they do so as root? Turning off the tests for everyone is a no-go.
2. We can turn them off with an environment variable. Eg. I can set UNPRIVILEGED=true in my RPM spec file, then we can skip if that environment variable is set. It should be transparent to moby developers, who presumably do not have that variable set.
3. Nothing. The project isn't ready to consider this kind of a change. Packaging concerns are a downstream issue, after all, and we can certainly keep carrying the patch files.

Let me know what you think!

[thaJeztah]

We can use the existing "am I root?" check. I'm not sure if this is good, since I'm not sure how moby developers execute these tests themselves - do they do so as root? Turning off the tests for everyone is a no-go.

I expect most developers will run the test-suites docker-in-docker, in which case the tests run as root inside the container (which could be either "actual" root, or a "remapped" root (with user-namespaces enabled)). Some users may be running tests directly on "bare-metal" (not docker-in-docker), so for those, the skips may be relevant.

Given that the full test-suite takes a long time to run, I expect most people working on the code to only be running unit-tests, or (if integration tests are needed to verify changes) to run a limited set of integration tests, using the TEST_FILTER make var, for example (picking from my own bash history);

make BIND_DIR=. DOCKER_GRAPHDRIVER=vfs TEST_FILTER=TestInfoAPI test-integration

w.r.t. the Skips themselves; because of all of the above, it's not unlikely that there are t.Skip() in tests that are incorrect (or not needed at all). Such skips may have been added just through "copy/paste" from other tests in the same file, or possibly the assumption was incorrect. We'd unlikely notice such "wrong" skips because in CI, the test-suites run containerised (as (remapped) root).

Unfortunately there are many of such t.Skip in place where no (code) comment was added to describe the "why". Time doesn't always permit us to dig up history for them, but when we run into such cases, we try to retrofit the skips with a proper comment to document why it's skipped (which can help find which skips are "permanent", and which could be removed, given conditions decribed in the comment).

(contributions in that area are very much appreciated as well, but I realize those can be a bit of a chore to do ❤️)

If this were baked into moby, we can stop carrying this as a vendor-specific patch - and it will hopefully make it easier for other distributions to package moby as well.

Contributions are always welcome! If they are generic patches, and benefit the project as a whole, we would definitely accept them.

• We can use the existing "am I root?" check.
• We can turn them off with an environment variable. Eg. I can set UNPRIVILEGED=true in my RPM spec file

Perhaps you can enlighten me a bit on this part. Is there a specific reason why os.GetUid() is causing issues?

To give a bit of context: we do have environment variables in various places to control things. Honestly, we have "too many" of them. Many environment variables were added in the past, and either are no longer really relevant, or we lost track "why" they were, and as a result it's easy to continue "preserving them" (they must be there for a reason... right?), which can result in things becoming more complicated than they need to be (lots of bash.. lots of bash to set up things).

So where possible we want to (try to) make these tests "just work", and detect whether they should be skipped (or not) using test-utilities, instead of having to be run with "magic" environment variables.

That's not to say we don't allow new environment-variables to be introduced, but just that we need to check the alternatives to decide what makes most sense (which will differ for each case).

If (with the above in mind) there's still reasons why an environment variable would be beneficial, then we could

The most basic (if we expected "privileged" to be the default, could be to check for non-empty values (any value set to be considered true);

func IsUnprivileged() bool {
	if val := os.GetEnv("UNPRIVILEGED"); val != "" {
		return true
    }
	return os.GetUID() != 0
}

If we need for UNPRIVILEGED to be either "enabled" or "disabled", we can add a ParseBool (if the value is set), something like this;

func IsUnprivileged(t testing.T) bool {
	t.Helper()
	if val := os.GetEnv("UNPRIVILEGED"); val != "" {
		ok, err := strconv.ParseBool(val)
		if err != nil {
			t.Log("failed to parse $UNPRIVILEGED (%s): %v", val, err)
        }
		return ok
    }
	return os.GetUID() != 0
}

If course, the reverse could also be true (assume "unprivileged"), I guess that part is to be discussed 😅

[mysteriouspants]

Thanks for your time in response!

It's good to hear that contributions are welcome! I find it helpful to talk about what I want to do first, before submitting a PR, since it can shape the contribution to something the project is happy with.

There isn't anything incorrect about using os.Getuid as a privilege check - it does work. I'm primarily worried about the assumption that the integration tests being run as root by default causing pain should I be incorrect about that assumption. I think, based on what you've said about Docker In Docker being the primary way of building the project when developing it, that starting off with a simple os.Getuid() != 0 check is adequate. If this is not the case I'm happy to iterate further in the future.

I don't expect anybody to audit their tests to make sure they don't require privileges to run. As package maintainers take new versions of moby we can add those conditional skips in our vendor patches, and contribute them back upstream as they're found.