Add a workaround for using mount without privileges, re-enable rootless tests in CI#6933
Add a workaround for using mount without privileges, re-enable rootless tests in CI#6933nalind wants to merge 7 commits into
mount without privileges, re-enable rootless tests in CI#6933Conversation
nalind
changed the title
mount without privileges, re-enable rootless tests in CImount without privileges, re-enable rootless tests in CI
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
Thanks, I am a bit conflicted about this.
On the one hand I see how this helps us with testing but on the other side do we really want to ship and fully support these commands for general use?
I mean 900 lines is not that bad but it also is not exactly straight forward code and then well we take on a new sftp dep which is like 10k lines as well. That feels a bit much for just testing, though I guess the reason why documents them is because they can also be useful to some users so I can see the point.
Since you are the main maintainer here and likely already considered all this when you write the code I guess I am cool with it.
CI wise LGTM, I did not review the 900 lines of sftp.go so far though
|
LGTM |
|
Pinging Dave mostly to make sure this is tracked for RHEL if we move forward with this. |
|
I keep meaning to bring up the sshfs/sftp part of it at a meeting to get a better feel for whether or not we want to include that part of this, since it has limitations if you try to use that outside of test cases. |
|
Been kicking the idea around since https://redhat.atlassian.net/browse/RUN-1457, finally had a reason to look into it. |
|
I think it is certainly a nice use case for the tests. Maybe hiding the commands and label it unsupported on RHEL would help to reduce or maintenance load. |
|
It's a nice approach. Do I understand correctly that the pipeloop is self contained in the test directory, and that sshfs is already a part of RHEL? If not, I see no issue with installing it simply for the tests from EPEL. We pull BATS in to run tests. I just don't want any part of a new RHEL package, especially just to run tests. |
|
The pipeloop helper is standalone, but AFAIK sshfs is not a part of RHEL or EPEL. |
We'll need something that we can use to test the serve-sftp command without also testing our management of an sshfs mountpoint. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
This is how we run tests in CI, and we would like timing information there. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add an undocumented CLI command which serves the contents of a container or image over stdio using the SFTP protocol. The intent is to be able to slot this in where the sshfs(1) documentation suggests running the OpenSSH SFTP server subsystem binary to share a directory's contents with a remote shell session. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add an undocumented CLI command which pairs "buildah serve-sftp" with
the copy of "sshfs" that we launch, piping the output of each to the
input of the other, as suggested in sshfs(1).
Because "buildah mount-sshfs" doesn't set up a user namespace the way
that "buildah unshare" and most of our other CLI commands do, the copy
of "sshfs" that it runs will run outside of any user namespaces, while
the "buildah serve-sftp" bits will of course run inside of a user
namespace if one would normally be created for the invoking user.
Not being run inside of a user's user namespace means that neither
"buildah mount-sshfs", nor by extension, "buildah umount-sshfs", can be
guaranteed to be able to directly examine storage, so we also add an
extra, hidden, CLIs ("buildah find-data-dir" and "buildah
list-sshfs-mount-dirs") that can be called to locate where the userdata
directory and would-be mountpoints for a given container or image ID
are.
We have "buildah umount-sshfs" exec "buildah find-data-dir" to look up
the userdata directory for something which was previously mounted by
"buildah mount-sshfs" and reconstruct the location of the mountpoint so
that the experience of using "buildah mount-sshfs" and "buildah
umount-sshfs" will more closely match that of "buildah mount" and
"buildah umount".
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Previously, if the tests were run as a non-root user, we'd always run "buildah mount" under "buildah unshare" to avoid error messages about how it wasn't expected to work. This, however, only ever "worked" for the vfs storage driver, which didn't actually mount or unmount filesystems. Drop that and instead use a run_buildah_mount wrapper function that calls either `buildah mount` or `buildah mount-sshfs`, depending on the current UID. Add a corresponding run_buildah_umount wrapper function, and use it instead of `buildah unmount` or `buildah umount`. For the most part, this is merely mechanical replacement. Call run_buildah_umount() before `buildah rm` in a number of tests: the umount-with-detach that the container removal logic uses doesn't provide enough time for the mountpoint to become idle before it then attempts to remove it. The "copy-from-image" test was actually using `buildah add`, so move it from copy.bats to add.bats and add a workalike in its place. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
My mistake: I was looking for a package named "sshfs", not "fuse-sshfs". It's been noted elsewhere that fuse-sshfs is in EPEL 9 and 10. Changed the new commands to be undocumented. |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
buildah mount-sshfsworkaround for not being able tobuildah mountwhen not root.buildah mount-sshfswhen run by a UID other than 0, and working around the workaround's limitations (chiefly: avoidbuildah run/add/copywhile the filesystem is mounted).How to verify it
New and updated integration tests!
Which issue(s) this PR fixes:
This is mainly about re-enabling tests in CI. We weren't really testing with overlay in rootless scenarios before.
Special notes for your reviewer:
The new commands are marked as hidden.
This doesn't add new library APIs.
Does this PR introduce a user-facing change?