[CI:DOCS] Handle DOCKER_HOST environment for podman-docker package #21532

rhatdan · 2024-02-06T14:13:19Z

Rootless users should be defaulted to point DOCKER_HOST at $XDG_RUNTIME_DIR/podman/podman.sock

When podman-docker package is installed.

Does this PR introduce a user-facing change?

DOCKER_HOST will be set by default for rootless users when podman-docker is installed.

[NO NEW TESTS NEEDED]

rhatdan · 2024-02-06T14:14:11Z

@lsm5 I think this needs changes to rpm spec file, but I am not sure where the changes have to happen.

@nalind @Luap99 @vrothberg @mheon @baude @umohnani8 @mtrmac PTAL

packit-as-a-service · 2024-02-06T14:22:43Z

Ephemeral COPR build failed. @containers/packit-build please check.

mheon · 2024-02-06T14:23:16Z

What if the user has the Moby package also installed on Fedora? I don't think that sets DOCKER_HOST by default so we'll hijack the other runtime.

rhatdan · 2024-02-06T14:26:08Z

podman-docker package conflicts against the moby or docker package, since it installs /usr/bin/podman as well as all of the man pages.

lsm5 · 2024-02-06T16:47:46Z

@lsm5 I think this needs changes to rpm spec file, but I am not sure where the changes have to happen.

guess we could run the script in %post docker. I can test it out and report back.

Luap99 · 2024-02-06T17:01:46Z

I see no special case for root. I assume this should not be set for root users given XDG_RUNTIME_DIR may not be set so you might generate an invalid DOCKER_HOST for root which could break their scripts, so I think we need some $(id -u) -ne 0 check in there.

rhatdan · 2024-02-06T21:13:21Z

I was thinking we should check if XDG_RUNTIME_DIR is set or not, only do it if it is set, would that fix the root issue?

Does Podman ignore XDG_RUNTIME_DIR if set for root?

# XDG_RUNTIME_DIR=/tmp/dan podman info | grep -i dan
      rundir: /tmp/dan/crun

So if XDG_RUNTIME_DIR is not set then don't set DOCKER_HOST.

lstolcman · 2024-02-06T23:05:34Z

Shouldn't it be

DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock

?

According to the source, if protocol in Docker_host is not set, it defaults to tcp://: https://stackoverflow.com/a/62757128

The tutorial about socket has unix:// prefix as well: https://github.com/containers/podman/blob/main/docs/tutorials/socket_activation.md

Luap99 · 2024-02-07T10:58:01Z

Yes podman as root ingores XDG_RUNTIME_DIR for the most part,
XDG_RUNTIME_DIR as root is /run/user/0 when you login via ssh while when you login differently it may not be set at all so we use /run. And /run/user/0/podman/podman.sock is not our socket location as root so you would point it at the wrong thing.

rhatdan · 2024-02-07T11:30:16Z

@Luap99 I showed above the podman does not ignore XDG_RUNTIME_DIR when run as root. it follows it. At least for runtimedir.

rhatdan · 2024-02-07T12:00:29Z

@lsm5 could you see what I am doing wrong, when I test locally I see this works correctly. But in CI/CD it is not prepending the DESTDIR?

$ make install.docker DESTDIR=/tmp/dan
install -Z -d -m 755 /tmp/dan/usr/local/bin
env BINDIR=/usr/local/bin ETCDIR=/etc envsubst < docker/docker.in > /tmp/tmp.Glg0K67X9Y
install -Z -m 755 /tmp/tmp.Glg0K67X9Y /tmp/dan/usr/local/bin/docker
rm /tmp/tmp.Glg0K67X9Y
install -Z -m 755 -d /tmp/dan/usr/local/lib/systemd/system  /tmp/dan/usr/local/lib/systemd/user /tmp/dan/usr/local/lib/tmpfiles.d /tmp/dan/usr/local/share/user-tmpfiles.d
install -Z -d -m 755 /tmp/dan/etc/profile.d
install -Z -m 644 docker/podman-docker.sh /tmp/dan/etc/profile.d/podman-docker.sh
install -Z -m 644 docker/podman-docker.csh /tmp/dan/etc/profile.d/podman-docker.csh
install -Z -m 644 contrib/systemd/system/podman-docker.conf -t /tmp/dan/usr/local/lib/tmpfiles.d
install -Z -m 644 contrib/systemd/system/podman-docker.conf -t /tmp/dan/usr/local/share/user-tmpfiles.d

Makefile

Luap99 · 2024-02-07T13:11:58Z

@Luap99 I showed above the podman does not ignore XDG_RUNTIME_DIR when run as root. it follows it. At least for runtimedir.

That is a crun dir not podman, you should check the runroot. But really this does not matter here, what matters is were the podman socket is located and our systemd unit will always use /run/podman/podman.sock as root (systemd system manager)).

lsm5 · 2024-02-07T13:54:37Z

commit 96d055533d31ba471c2bd451624f8d799c276a87 (HEAD -> refs/heads/docker)
Author: Lokesh Mandvekar <[email protected]>
Date:   Wed Feb 7 18:47:05 2024

    fix rpm spec for pr#21532
    
    Signed-off-by: Lokesh Mandvekar <[email protected]>

diff --git a/rpm/podman.spec b/rpm/podman.spec
index d305f8df4..58edf9095 100644
--- a/rpm/podman.spec
+++ b/rpm/podman.spec
@@ -284,7 +284,7 @@ cd ..

 %install
 install -dp %{buildroot}%{_unitdir}
-PODMAN_VERSION=%{version} %{__make} PREFIX=%{buildroot}%{_prefix} ETCDIR=%{_sysconfdir} \
+PODMAN_VERSION=%{version} %{__make} DESTDIR=%{buildroot} PREFIX=%{_prefix} ETCDIR=%{_sysconfdir} \
        install.bin \
        install.man \
        install.systemd \
@@ -341,6 +341,7 @@ cp -pav test/system %{buildroot}/%{_datadir}/%{name}/test/
 %files docker
 %{_bindir}/docker
 %{_mandir}/man1/docker*.1*
+%{_sysconfdir}/profile.d/%{name}-docker.*
 %{_tmpfilesdir}/%{name}-docker.conf
 %{_user_tmpfilesdir}/%{name}-docker.conf

@rhatdan you'll need this spec file patch. Previously we were setting PREFIX=%{buildroot}%{_prefix} and not setting DESTDIR in spec which caused the issue you noticed.

packit-as-a-service · 2024-02-07T21:01:32Z

Cockpit tests failed for commit 5fca7acf35a567746c11d190ee6a566b9bb2a1d4. @martinpitt, @jelly, @mvollmer please check.

nalind · 2024-02-07T21:50:42Z

docker/podman-docker.csh

+# DOCKER_HOST initialization
+
+if ($?DOCKER_HOST) exit
+if ( $uid == 0 ) then


The sh equivalent is checking the euid. Should this be looking at $euid instead, assuming csh is always tcsh?

I think we go with euid, and then if these are not set because of a shell that does not support it, we will set the DOCKER_HOST iff the XDG_RUNTIME_DIR is also set.

nalind · 2024-02-07T21:50:44Z

docker/podman-docker.sh

+# DOCKER_HOST initialization
+
+[ -z "$DOCKER_HOST" ] || return
+if [ "$EUID" -eq 0 ]; then


This variable doesn't seem to be standardized (dash doesn't define $EUID, if anything other than bash matters, so it errors out if $DOCKER_HOST isn't set).

Same as above, this is only trying to prevent the case where XDG_RUNTIME_DIR is set for a root user.

leonpano2006 · 2024-02-09T08:47:19Z

Are you sure this pull request can fix my issue ticket?

Since I started wings with sudo/as root

rhatdan · 2024-02-09T15:39:25Z

@leonpano2006 if you ran into this problem as root via sudo, then most likely you had configuration issues. With podman-docker installed it should have /run/docker/docker.sock as a symbolic link to /run/podman/podman.sock. (At least after reboot). If you do not see the link then we have a different issue.

Anyways this should fix the case where users want to run docker-compose or other docker-py based tools without the docker daemon running in rootless mode.

nalind · 2024-02-09T16:05:21Z

Scriptlets LGTM.

leonpano2006 · 2024-02-09T19:32:29Z

@leonpano2006 if you ran into this problem as root via sudo, then most likely you had configuration issues. With podman-docker installed it should have /run/docker/docker.sock as a symbolic link to /run/podman/podman.sock. (At least after reboot). If you do not see the link then we have a different issue.

Anyways this should fix the case where users want to run docker-compose or other docker-py based tools without the docker daemon running in rootless mode.

can this fix pterodactyl/panel#4928 ?
my issue is based on this issue

leonpano2006 · 2024-02-12T10:22:52Z

this is without sudo on fresh install

this is with sudo on fresh install

rhatdan · 2024-02-12T13:23:28Z

You need to enable the service.

$ systemctl --user enable --now podman.socket

rhatdan · 2024-02-12T13:24:05Z

@lsm5 @Luap99 @mheon PTAL.

Luap99

@edsantiago PTAL

Luap99 · 2024-02-12T13:31:17Z

docker/podman-docker.sh

+if [ "$EUID" -eq 0 ]; then
+       DOCKER_HOST=unix:///run/podman/podman.sock
+else
+       [ ! -z "$XDG_RUNTIME_DIR" ] && DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock


use -n over ! -z

Also doesn't one have to export these vars to actually make then environment variables?

lsm5

RPM and makefile changes LGTM. haven't actually tried the scriptlets.

edsantiago

/hold

edsantiago

I really can't review this. The parentheses-to-curlybraces changes are very noisy.

If these are absolutely necessary, could you explain why? I will then suck it up and review.
If they are not absolutely necessary, could you please revert them?

rhatdan · 2024-02-12T14:10:43Z

I will switch the $() to ${} in a separate PR.

rhatdan · 2024-02-12T14:28:27Z

@edsantiago PTANL

edsantiago

Some concerns about sh rc file and about packaging.

I'm puzzled about the choice to use curlybraces (I tend to think of parens as more conventional), but I'll let it be. Thank you for splitting your commits.

edsantiago · 2024-02-12T14:40:00Z

rpm/podman.spec

@@ -248,7 +248,7 @@ LDFLAGS=''

 %install
 install -dp %{buildroot}%{_unitdir}
-PODMAN_VERSION=%{version} %{__make} PREFIX=%{buildroot}%{_prefix} ETCDIR=%{_sysconfdir} \
+PODMAN_VERSION=%{version} %{__make} DESTDIR=%{buildroot} PREFIX=%{_prefix} ETCDIR=%{_sysconfdir} \


Oh, thank goodness. I remember griping about this years ago.

edsantiago · 2024-02-12T15:05:42Z

docker/podman-docker.sh

@@ -0,0 +1,8 @@
+# DOCKER_HOST initialization
+
+[ -z "$DOCKER_HOST" ] || return


I don't like this. Try sourcing the file twice (in bash): it will leave you with exit status 1.

Also, what Nalin said. id -u might be safer.

Also, export foo=bar is pretty universal, but once upon a time there were shells that did not allow it. grep export /etc/profile.d/*.sh suggests that if those shells still exist, nobody cares about them, and neither do I, but I want to be very explicit about that here.

Also, why the seven-space indentation?

Therefore, suggestion:

if [ -z "$DOCKER_HOST" ]; then if [ `id -u` -eq 0 ]; then export DOCKER_HOST=unix:///run/podman/podman.sock else if [ -n "$XDG_RUNTIME_DIR" ]; then export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock else FIXME: do we want a fallback? fi fi fi

I think we just do nothing if XDG_RUNTIME_DIR is not set.

edsantiago · 2024-02-12T15:08:12Z

rpm/podman.spec

@@ -300,6 +300,7 @@ cp -pav test/system %{buildroot}/%{_datadir}/%{name}/test/
 %files docker
 %{_bindir}/docker
 %{_mandir}/man1/docker*.1*
+%{_sysconfdir}/profile.d/%{name}-docker.*


Shouldn't this be under %files remote?

No these are only installed if the user installs podman-docker package.

Signed-off-by: Daniel J Walsh <[email protected]>

Rootless users should be defaulted to point DOCKER_HOST at $XDG_RUNTIME_DIR/podman/podman.sock When podman-docker package is installed. [NO NEW TESTS NEEDED] Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2024-02-12T21:08:56Z

I'm puzzled about the choice to use curlybraces (I tend to think of parens as more conventional), but I'll let it be. Thank you for splitting your commits.

I switched to default for DESTDIR to $(), all I wanted was consistency. There are other uses of ${} in the code, but I don't feel like screwing around with them now.

BTW I thought this would a PR that would take an hour to get in. :^(

openshift-ci · 2024-02-12T21:41:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, lsm5, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [edsantiago,lsm5,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago

LGTM. The curly-brace-paren thing is an unmitigated disaster IMO, I really would've preferred a separate slower cleanup PR because even now we still have a mix of curlies and parens, probably dating back to whoever made which edit and had whichever preference. Changes like this make spelunking harder. But again, I won't block.

This is a big-scary change that I think requires more eyeballs. @containers/podman-maintainers PTAL.

edsantiago · 2024-02-12T21:43:48Z

docker/podman-docker.csh

@@ -0,0 +1,10 @@
+# DOCKER_HOST initialization
+
+if ($?DOCKER_HOST) exit


Late followup: I was concerned about this exit, wondering if it would exit the actual shell. It does not.

# cat foo.csh echo aaa source podman-docker.csh echo bbb # source foo.csh aaa bbb

edsantiago · 2024-02-12T21:47:29Z

Sigh. @containers/podman-maintainers PTAL anyway.

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 6, 2024

rhatdan force-pushed the docker branch 4 times, most recently from 454ffe1 to e5b111f Compare February 6, 2024 14:17

rhatdan force-pushed the docker branch from e5b111f to f9c75d5 Compare February 6, 2024 21:20

rhatdan changed the title ~~Handle DOCKER_HOST environment for podman-docker package~~ [CI:DOCS] Handle DOCKER_HOST environment for podman-docker package Feb 6, 2024

rhatdan force-pushed the docker branch from f9c75d5 to 47fa20f Compare February 6, 2024 21:20

rhatdan force-pushed the docker branch from 47fa20f to 84bca34 Compare February 7, 2024 01:40

rhatdan force-pushed the docker branch from 84bca34 to 7752bb2 Compare February 7, 2024 11:34

lsm5 reviewed Feb 7, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

rhatdan force-pushed the docker branch from 7752bb2 to 5fca7ac Compare February 7, 2024 20:24

rhatdan added the 5.0 label Feb 7, 2024

nalind reviewed Feb 7, 2024

View reviewed changes

rhatdan force-pushed the docker branch from bdee211 to 79731c5 Compare February 8, 2024 04:14

Luap99 reviewed Feb 12, 2024

View reviewed changes

lsm5 approved these changes Feb 12, 2024

View reviewed changes

edsantiago suggested changes Feb 12, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2024

edsantiago reviewed Feb 12, 2024

View reviewed changes

rhatdan force-pushed the docker branch from 79731c5 to 9366cb7 Compare February 12, 2024 14:26

edsantiago reviewed Feb 12, 2024

View reviewed changes

rhatdan added 2 commits February 12, 2024 16:03

Consistant handling DESTDIR variable expansion

36853c0

Signed-off-by: Daniel J Walsh <[email protected]>

Handle DOCKER_HOST environment for podman-docker package

ad55ba6

Rootless users should be defaulted to point DOCKER_HOST at $XDG_RUNTIME_DIR/podman/podman.sock When podman-docker package is installed. [NO NEW TESTS NEEDED] Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the docker branch from 9366cb7 to ad55ba6 Compare February 12, 2024 21:06

edsantiago approved these changes Feb 12, 2024

View reviewed changes

rhatdan added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 12, 2024

openshift-merge-bot bot merged commit 3691f84 into containers:main Feb 12, 2024
37 checks passed

edsantiago reviewed Feb 12, 2024

View reviewed changes

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 13, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators May 13, 2024

		@@ -0,0 +1,8 @@
		# DOCKER_HOST initialization

		[ -z "$DOCKER_HOST" ] \|\| return

		@@ -0,0 +1,10 @@
		# DOCKER_HOST initialization

		if ($?DOCKER_HOST) exit

[CI:DOCS] Handle DOCKER_HOST environment for podman-docker package #21532

[CI:DOCS] Handle DOCKER_HOST environment for podman-docker package #21532

Conversation

rhatdan commented Feb 6, 2024 • edited Loading

Does this PR introduce a user-facing change?

rhatdan commented Feb 6, 2024

packit-as-a-service bot commented Feb 6, 2024

mheon commented Feb 6, 2024

rhatdan commented Feb 6, 2024

lsm5 commented Feb 6, 2024

Luap99 commented Feb 6, 2024

rhatdan commented Feb 6, 2024 • edited Loading

lstolcman commented Feb 6, 2024 • edited Loading

Luap99 commented Feb 7, 2024

rhatdan commented Feb 7, 2024

rhatdan commented Feb 7, 2024

Luap99 commented Feb 7, 2024

lsm5 commented Feb 7, 2024

packit-as-a-service bot commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonpano2006 commented Feb 9, 2024

rhatdan commented Feb 9, 2024

nalind commented Feb 9, 2024

leonpano2006 commented Feb 9, 2024 • edited Loading

leonpano2006 commented Feb 12, 2024 • edited Loading

rhatdan commented Feb 12, 2024

rhatdan commented Feb 12, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsm5 left a comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

rhatdan commented Feb 12, 2024

rhatdan commented Feb 12, 2024

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

rhatdan commented Feb 12, 2024

openshift-ci bot commented Feb 12, 2024

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Feb 12, 2024

rhatdan commented Feb 6, 2024 •

edited

Loading

rhatdan commented Feb 6, 2024 •

edited

Loading

lstolcman commented Feb 6, 2024 •

edited

Loading

nalind Feb 7, 2024 •

edited

Loading

leonpano2006 commented Feb 9, 2024 •

edited

Loading

leonpano2006 commented Feb 12, 2024 •

edited

Loading

rhatdan Feb 12, 2024 •

edited

Loading