fix: keep container uid the same as host to allow ops on workdir root

[carlosrodfern]

Volume mounts on the container are by default read only for non-root users.
This prevents the necessary access for rootless container. The change in this
commit makes the container uid be the same as the host uid so that the user
under which podman runs, which owns the workdir root, is the same user uid on
the container, so the permissions also match.

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• adjust plugin docstring
• modify the json schema
• mention the version
• include a release note

Related: #3252

[happz]

Wouldn't this also change read-only files to writable? Is that an expected, or even desired course of action?

[carlosrodfern]

That's a really good point. Because the directory is mounted in the host, it will make /var/tmp/tmt on the host world writable.

The other solution I can think of is a deeper change, basically, making the logic around TEST_WRAPPER_TEMPLATE different for container, leveraging the fact that the container directory is mounted.

[happz]

The other solution I can think of is a deeper change, basically, making the logic around TEST_WRAPPER_TEMPLATE different for container, leveraging the fact that the container directory is mounted.

That we can do as well, the wrapper does not have to be the same for everything. It's a Jinja template, two shell files, we can do whatever we need to do to make all things work - permissions, TTY, duration, signals, and whatever else is waiting there to bite us. I tried to document and improve wrappers a bit, if you wish to give this avenue a try, please, start on top of this PR.

[carlosrodfern]

So the logic creating the pid files would need to not be executed on the container, but on the host, assuming it would be available to the container, and assuming flock works on the mount that podman does. That is necessary so it can be placed in the /var/tmp/tmt directory as the other PR is trying to do.

Basically, it is a change not just to the TEST_WRAPPER_TEMPLATE but to the code using it as well, adding different execution paths for containers. I can look into it.

[happz]

Pid file exists so tmt-reboot, called by a different tmt instance, could safely notify about upcoming reboot, and kill the test process our tmt manages. This presumes both the test and tmt-reboot run on the same guest, sharing it, like a baremetal machine or a VM from a cloud - thinking about it now, it does make much less sense in the context of containers.

Seems like a topic for tomorrow's tmt hacking meeting, make we could introduce some feature flag signaling to tmt when keeping pidfile makes sense, maybe provided by the guest class implementation.

[thrix]

added to hacking

[carlosrodfern]

@happz , did you have a chance to look into it?

[happz]

I didn't, let's give it a try now.

There are several provision plugins, what does the existence of the PID file and the out-of-session tmt-reboot mean for them? The PID file exists so tmt-reboot can update the running test with reboot command and timeout, and then kill the test process spawned by a tmt managing the test. tmt-reboot runs on the guest, not on the runner, and the killed process was running on the guest, not on the runner. And the more I think about it, the more I'm getting lost in various combinations :/

Let's focus on the workdir permissions and the original issue, "Containers that mount the workdir but run with non-root users are not able to operate on the workdir because the mounted volume appears as read only." Isn't it possible to fix this by some careful podman run setup? Like mapping ownership, permissions, etc?

SSH-capable guests do have something similar, but they seem to get away with just read+executable permissions:

    def setup(self) -> None:
        if self.is_dry_run:
            return
        if not self.facts.is_superuser and self.become:
            self.package_manager.install(FileSystemPath('/usr/bin/setfacl'))
            workdir_root = effective_workdir_root()
            self.execute(
                ShellScript(
                    f"""
                    mkdir -p {workdir_root};
                    setfacl -d -m o:rX {workdir_root}
                    """
                )
            )

[carlosrodfern]

I did try to devise a solution using uidmaps but no luck. I'll give --userns a try.

[carlosrodfern]

@happz I just pushed a solution using --userns. Let me know your thoughts. I tested it on @mcasquer 's branch and it passed the provision/become tests.

[thrix]

moved the milestone, seems this is not making the release today

[happz]

/packit build

[happz]

Why the change from self.parent.plan.workdir to self.workdir_root? I don't believe the yare the same thing, workdir_root will contain files completely unrelated to the current run.

[carlosrodfern]

Mainly because the new pid code is trying to create the pid in the /var/tmp/tmt dir, which doesn't match any of the permission if the mount is /var/tmp/tmt/run-001/etc....

[carlosrodfern]

Hmmm, I believe I can still do it with the workdir if I bring the setup(..) method back without the recursive chmod change. It would be a little bit more over the place since it is modifying the parent dir /var/tmp/tmt created by the mount with some permissions but with a different set of permission for the subdirectories as inherited by the userns and the mount itself, but it might work.

[carlosrodfern]

OK, I added a new commit bringing setup(..) back, but without the recursive chmod. It works as well on the pid change branch. This approach may feel a little bit less consistent in the sense that the workdir_root has now permissions to allow modifications in it outside the plan execution, but the plan execution itself (a subdir) is mounted with a different set of permissions as set by volume mount with the keep-id userns option.

@happz, Let me know, I can revert that last commit.

[happz]

Sure. BTW is there a test that would demonstrate the problem, a test we could watch turn green once the fix is correct? :)

[carlosrodfern]

provision/facts/guest is failing because the preparation step to install setfacl is doing a sudo but the user is nobody, so it is asking for the sudo password in that step.

            provision_options="--image fedora:39"
            bfu_provision_options="$provision_options --user=nobody"

Should the test be changed to use a similar unprivileged container image with a passwordless sudo fedora user? I can make that change if that's OK.

The provision/user one is failing because the workdir is actually not a subdirectory of self.workdir_root, so I pushed a change to add mkdir -p {self.workdir_root}, and it is going to be needed for the pid change as well.

[happz]

/packit build