Skip to content

Add support for podman (Fedora 31+, RHEL8+)#128

Open
AaronDMarasco wants to merge 2 commits intopackpack:masterfrom
AaronDMarasco:master
Open

Add support for podman (Fedora 31+, RHEL8+)#128
AaronDMarasco wants to merge 2 commits intopackpack:masterfrom
AaronDMarasco:master

Conversation

@AaronDMarasco
Copy link
Copy Markdown
Contributor

Red Hat (the company) is moving towards root-less containers with podman. This fixes packpack to use the proper flags for the container to build RPMs as well as fixing the "Requires" of its own RPM when built.

$ for f in build/packpack*noarch.rpm; do echo -n "$f: "; rpm -qp --requires $f | grep docker; done
build/packpack-1.0.130-1.el7.noarch.rpm: docker >= 1.5
build/packpack-1.0.130-1.el8.noarch.rpm: podman-docker
build/packpack-1.0.130-1.fc30.noarch.rpm: docker >= 1.5
build/packpack-1.0.130-1.fc31.noarch.rpm: podman-docker
build/packpack-1.0.131-1.fc32.noarch.rpm: podman-docker

@Totktonada
Copy link
Copy Markdown
Member

Thank you!

That is new for me: I'll need some time to learn necessary context and to test the patch.

@AaronDMarasco
Copy link
Copy Markdown
Contributor Author

OK feel free to let me know if there are any questions. Basically, there is an RPM called podman-docker that provides a docker CLI wrapper to convert all the existing commands to call podman under the hood. So the build script checks for podman first, and if you have that, then assumes any executable docker in the path is the wrapper.

The other change forces mounts to map SELinux contexts to the running user; it's a lot more lenient when running as root. Without it, you cannot write to the build mounted volume.

@AaronDMarasco
Copy link
Copy Markdown
Contributor Author

AaronDMarasco commented Feb 15, 2021

It's been some time... do you need more info or anything?

(Resolved conflicts.)

@Totktonada
Copy link
Copy Markdown
Member

My apologizes for the delay!

I'll take a look at the week.

Comment thread packpack Outdated
Comment thread rpm/packpack.spec
Comment thread packpack
Comment on lines +25 to +26
# This might be needed for all SELinux installs
EXTRA_MOUNT=,Z
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it is safe default. At least source tree, cache and packpack directory directory may be intended to use from other containers and from the host system as well (it seems some services may lost access to those directories after labeling).

I read several related articles:

moby/moby#30934
https://www.projectatomic.io/blog/2015/06/using-volumes-with-docker-can-cause-problems-with-selinux/

And now I'm not ever sure whether we'll able to run packpack on the same source tree several times. Or docker/podman will relabel it each time (remove old label and add a new one)? So only parallel builds will fail?

What do you think? Should not we make this choice explicit rather than default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea; I am not an SELinux expert. I just know that without it, I couldn't read the files.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just obserted the following call in the packpack script:

#
# Fix security context for selinux
#
chcon -Rt svirt_sandbox_file_t ${PACKDIR} ${SOURCEDIR} ${BUILDDIR} \
    1> /dev/null 2> /dev/null || :

AFAIU (I'm not an expert too), it doing the same (but like ,z, not ,Z) for all host directories that will be used as volumes except ${CACHE_DIR}.

Can you share precise error? Maybe we should just add ${CACHE_DIR} here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, when I comment out my EXTRA_MOUNT, this is the result:

+ docker run --volume /home/user/git_stuff/packpack/pack:/pack:ro --volume /home/user/git_stuff/packpack:/source:ro --volume /home/user/git_stuff/packpack/build:/build:rw --env-file /home/user/git_stuff/packpack/build/env --workdir /source --rm=true --tty=true --entrypoint=/build/userwrapper.sh -e XDG_CACHE_HOME=/cache -e CCACHE_DIR=/cache/ccache -e TMPDIR=/tmp -e CI= --volume /home/user/.cache/packpack:/cache:rw packpack/packpack:fedora-33 make -f /pack/Makefile -C /source BUILDDIR=/build -j
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
/bin/sh: /build/userwrapper.sh: Permission denied

Copy link
Copy Markdown
Contributor Author

@AaronDMarasco AaronDMarasco Feb 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that SELinux line is already in there for me as well and doesn't seem to be doing anything. I took out the dev/null redirects to ensure no error messages were being ignored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried only applying the ",Z" to the rw volume, and it still didn't work.

+ make -f /pack/Makefile -C /source BUILDDIR=/build -j
make: *** /source: Permission denied.  Stop.

Let me know if there's anything else you want me to test, otherwise, this is what needs to be done to work on Fedora hosts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks unexpected, because we set the label for the build directory. Is it on a new project clone or after previous run with ,Z?

@Totktonada
Copy link
Copy Markdown
Member

Totktonada commented Feb 21, 2021

@AaronDMarasco Thanks for patience! Please, look at several questions above.

BTW, I suggest to rebase on top of master (git rebase origin/master) rather than merge master into the branch (git merge origin/master).

Also just built for F33 without a problem
@ligurio
Copy link
Copy Markdown

ligurio commented Jun 21, 2021

(I have no rights to add a review notes to pull request, so I'll add them as a separate comment)

First of all, @AaronDMarasco, thanks for your pull request!

I've checked build of packpack for CentOS target oS using podman on CentOS 8 host OS and docker on Ubuntu 20.04 host OS. Content of binary and src packages are the same, also compared using command line diff <(rpm -q --dump --scripts -p ./build/packpack-1.0.137-1.el8.noarch.rpm) <(rpm -q --dump --scripts -p ./build/packpack-1.0.137-1.fc24.noarch.rpm) and pkgdiff.

[s.bronnikov@tarantool-core-dev-mcs1 packpack]$ rpm -ql ./build/packpack-1.0.137-1.el8.noarch.rpm
/usr/bin/packpack
/usr/share/doc/packpack
/usr/share/doc/packpack/README.md
/usr/share/licenses/packpack
/usr/share/licenses/packpack/LICENSE
/usr/share/packpack/Makefile
/usr/share/packpack/apk.mk
/usr/share/packpack/config.mk
/usr/share/packpack/deb.mk
/usr/share/packpack/rpm.mk
/usr/share/packpack/tarball.mk
[s.bronnikov@tarantool-core-dev-mcs1 packpack]$ rpm -ql ./build/packpack-1.0.137-1.el8.src.rpm
packpack-1.0.137.tar.xz
packpack.spec
sergeyb@pony:~/sources/MRG/packpack$ rpm -ql ./build/packpack-1.0.137-1.fc24.noarch.rpm
/usr/bin/packpack
/usr/share/doc/packpack
/usr/share/doc/packpack/README.md
/usr/share/licenses/packpack
/usr/share/licenses/packpack/LICENSE
/usr/share/packpack/Makefile
/usr/share/packpack/apk.mk
/usr/share/packpack/config.mk
/usr/share/packpack/deb.mk
/usr/share/packpack/rpm.mk
/usr/share/packpack/tarball.mk
sergeyb@pony:~/sources/MRG/packpack$ #rpm -ql ./build/packpack-
sergeyb@pony:~/sources/MRG/packpack$ ls ./build/
build.log                           packpack-1.0.137-1.fc24.noarch.rpm  userwrapper.sh
env                                 packpack-1.0.137-1.fc24.src.rpm     
sergeyb@pony:~/sources/MRG/packpack$ rpm -ql ./build/packpack-1.0.137-1.fc24.src.rpm 
packpack-1.0.137.tar.xz
packpack.spec

Aaron, @AaronDMarasco, changes in commit "Add support for podman (Fedora 31+, RHEL8+)" look trivial, but could you add a couple of lines about changes? Not everyone knows about 'z' option in Docker, it's worth to add a link to documentation (https://docs.docker.com/storage/bind-mounts/#configure-the-selinux-label).
The same about the second commit "Made some tweaks based on Github feedback re: su-exec calls". It is not clear what problem has been solved.

We have a PackPack CI (.travis.yml) and I think that we need to update OS matrix there too.

@AaronDMarasco
Copy link
Copy Markdown
Contributor Author

Aaron, @AaronDMarasco, changes in commit "Add support for podman (Fedora 31+, RHEL8+)" look trivial, but could you add a couple of lines about changes? Not everyone knows about 'z' option in Docker, it's worth to add a link to documentation (https://docs.docker.com/storage/bind-mounts/#configure-the-selinux-label).
The same about the second commit "Made some tweaks based on Github feedback re: su-exec calls". It is not clear what problem has been solved.

Sorry, but no. I made an attempt nearly a year ago to help out on a project that I don't even personally use, but just happen to know a decent amount about its subject (RPMs). I rebased it a few months ago as requested after it was already "approved" in Sept and it still wasn't merged in. If you have explicit specific questions, feel free to ask. But I think every line at this point has been covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants