Eval symlinks on XDG_RUNTIME_DIR #15918

rhatdan · 2022-09-23T18:10:49Z

Partial Fix for #14606

[NO NEW TESTS NEEDED]

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

Does this PR introduce a user-facing change?

None

openshift-ci · 2022-09-23T18:10:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

mheon · 2022-09-23T19:24:16Z

pkg/rootless/rootless_linux.c

@@ -359,7 +359,8 @@ static void __attribute__((constructor)) init()

  /* Shortcut.  If we are able to join the pause pid file, do it now so we don't
     need to re-exec.  */
-  xdg_runtime_dir = getenv ("XDG_RUNTIME_DIR");
+    char path[PATH_MAX];


Whitespace here looks a bit wierd

mheon · 2022-10-13T14:26:49Z

Code LGTM, I'm good to merge whenever the tests start passing

rhatdan · 2022-10-19T11:45:36Z

@edsantiago Any idea what is breaking the Windows man page generator? I don't see how this PR is causing this error?

edsantiago · 2022-10-19T11:56:30Z

The error I see is:

docs: no doc file nor link docs/build/man/podman-.1 for 'podman '

The space looks suspicious but I don't know this code and have no idea what it means. Am looking into it now.

The other weird thing I see is:

lstat podman_tmp_5mgn/podman-run--1/bus: no such file or directory

I think that's related, because the string lstat does not appear on a successful CI run of this step. Continuing to investigate, am posting this in case it triggers a memory in anyone.

edsantiago · 2022-10-19T11:58:49Z

Okay..... there's a weird error involving "bus", and your PR adds the string "bus". I think there's a connection. Maybe XDG_WHATEVER is unset on Macs?

edsantiago · 2022-10-19T13:26:42Z

I see your new push but I don't think that's going to fix it: XDG_RUNTIME_DIR gets set complicatedly in one of the config.go files. I'm looking right now at

podman/pkg/util/utils_supported.go

Lines 42 to 43 in f6053ce

    
           tmpDir := filepath.Join(os.TempDir(), fmt.Sprintf("podman-run-%s", uid)) 
        
           if err := os.MkdirAll(tmpDir, 0700); err != nil {

...which has the string "podman-run-%s" which closely matches the "podman-run--1" in the lstat error (except, wtf, is "-1" a valid UID in Mac-land???). The other part, podman_tmp_5mgn, is set in Makefile: that's $TMPDIR. So XDG is getting set to a default value in TMPDIR, and something is then failing later. But I can't reproduce it on my end no matter what I use for TMPDIR and XDG. Still looking.

edsantiago · 2022-10-19T13:44:10Z

AHA! Reproduced it via hackery:

diff --git a/pkg/util/utils_supported.go b/pkg/util/utils_supported.go
index 90a2ecf86..c5877ea9f 100644
--- a/pkg/util/utils_supported.go
+++ b/pkg/util/utils_supported.go
@@ -34,7 +34,7 @@ func GetRuntimeDir() (string, error) {
                }
 
                uid := fmt.Sprintf("%d", rootless.GetRootlessUID())
-               if runtimeDir == "" {
+               if runtimeDir == "xxxx" {
                        tmpDir := filepath.Join("/run", "user", uid)
                        if err := os.MkdirAll(tmpDir, 0700); err != nil {
                                logrus.Debug(err)

With that, compile, and run:

$ mkdir foo123
$ env -i TMPDIR=foo123 bin/podman-remote help
lstat foo123/podman-run-14904/bus: no such file or directory
$ echo $?
1

edsantiago · 2022-10-19T13:51:17Z

This patch (against main, not against your PR) fixes it. Obviously there may be a better solution:

diff --git a/cmd/podman/registry/config.go b/cmd/podman/registry/config.go
index a118fdc4d..0f6028888 100644
--- a/cmd/podman/registry/config.go
+++ b/cmd/podman/registry/config.go
@@ -105,7 +105,12 @@ func setXdgDirs() error {
        if _, found := os.LookupEnv("DBUS_SESSION_BUS_ADDRESS"); !found {
                sessionAddr := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "bus")
                if _, err := os.Stat(sessionAddr); err == nil {
-                       os.Setenv("DBUS_SESSION_BUS_ADDRESS", "unix:path="+sessionAddr)
+                       dereferenced, err := filepath.EvalSymlinks(sessionAddr)
+                       if err != nil {
+                               return err
+                       }
+
+                       os.Setenv("DBUS_SESSION_BUS_ADDRESS", "unix:path="+dereferenced)
                }
        }

edsantiago · 2022-10-19T15:47:29Z

cmd/podman/registry/config.go

+		runtimeDir := os.Getenv("XDG_RUNTIME_DIR")
+		if runtimeDir != "" {


This isn't quite the same as mine. Even if you get it to compile, the logic is different: in my approach, I first assemble XDG/bus, then check if that exists, then deref links.

XDG will always be set. That's some early config hackery that @giuseppe added years ago. What does not exist on macs is XDG + bus.

edsantiago · 2022-10-19T15:48:34Z

libpod/reset.go

@@ -157,7 +157,13 @@ func (r *Runtime) reset(ctx context.Context) error {
 		}
 	}

-	xdgRuntimeDir := filepath.Clean(os.Getenv("XDG_RUNTIME_DIR"))
+	xdgRuntimeDir := os.Getenv("XDG_RUNTIME_DIR")
+	if xdgRuntimeDir != "" {


Again, I think this is misleading because XDG is always, always going to be set very early on by config code.

There is no change here if XDG_RUNTIME_DIR is not set or is set to "", Current code would not fail if XDG_RUNTIME_DIR=="" new code would, so I add his check.

edsantiago · 2022-10-19T15:49:01Z

pkg/systemd/dbus.go

@@ -129,7 +129,11 @@ func dbusAuthRootlessConnection(createBus func(opts ...godbus.ConnOption) (*godb
 func newRootlessConnection() (*dbus.Conn, error) {
 	return dbus.NewConnection(func() (*godbus.Conn, error) {
 		return dbusAuthRootlessConnection(func(opts ...godbus.ConnOption) (*godbus.Conn, error) {
-			path := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "systemd/private")
+			path := filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "systemd", "private")
+			path, err := filepath.EvalSymlinks(path)


And, like above, I think you need an existence check before you EvalSymlinks.

What would the check do if it does not exists?

Still call
return godbus.Dial(fmt.Sprintf("unix:path=%s", path))
?

If the file does not exists EvalSymlinks will return that it does not exists.

I don't know enough about this systemd/private path to comment. But:

old code: just returns a string with the path, whether the path exists or not (leave it to caller to deal with problems)

new code: barfs if path does not exist. This is new behavior. I don't know if it's good new behavior or bad new behavior... but it's a change.

I think the old code will barf when godbus.Dial attempts to connect to a non existing socket
The new code will report the non existing socket earlier, as I read it.

edsantiago · 2022-10-19T20:44:10Z

pkg/util/utils_supported.go

+
+		if runtimeDir != "" {
+			rootlessRuntimeDir, rootlessRuntimeDirError = filepath.EvalSymlinks(runtimeDir)
+			return


Kind of made me nervous to return early, but AFAICT it's perfectly fine, no code paths are affected, and this actually seems cleaner. PR LGTM but this has too many ramifications for me.

rhatdan · 2022-10-19T21:28:45Z

@containers/podman-maintainers PTAL

Luap99

Please add the test from my PR #14668 to make sure it actually fixes the issue.

edsantiago · 2022-10-20T11:55:56Z

Thanks for the test, @Luap99. Here's a version that doesn't depend on /var/run existing and being a symlink.

@test "podman rootless netns works when XDG_RUNTIME_DIR includes symlinks" {
    # regression test for https://github.com/containers/podman/issues/14606
    is_rootless || skip "only meaningful for rootless"

    # Create a tmpdir symlink pointing to /run, and use it briefly
    ln -s /run $PODMAN_TMPDIR/run
    local tmp_run=$PODMAN_TMPDIR/run/user/$(id -u)
    test -d $tmp_run || skip "/run/user/MYUID unavailable"

    # This 'run' would previously fail with:
    #    IPAM error: failed to open database ....
    XDG_RUNTIME_DIR=$tmp_run run_podman run --network bridge --rm $IMAGE ip a
    assert "$output" =~ "eth0"
}

(warning: if you run this test with podman @ main, or podman without this patch, you destroy everything and need to run podman system reset before you can run podman again. No idea why, don't really care).

Partial Fix for containers#14606 [NO NEW TESTS NEEDED] Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2022-10-29T15:20:55Z

@Luap99 @edsantiago PTAL

edsantiago · 2022-10-31T11:41:23Z

/lgtm

openshift-ci bot added the release-note-none label Sep 23, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2022

mheon reviewed Sep 23, 2022

View reviewed changes

rhatdan force-pushed the XDG_RUNTIME_DIR branch 2 times, most recently from 936263c to ae19075 Compare September 27, 2022 18:05

flouthoc mentioned this pull request Sep 29, 2022

Eval symlinks on XDG_RUNTIME_DIR containers/common#1162

Merged

rhatdan force-pushed the XDG_RUNTIME_DIR branch from ae19075 to 42f4f93 Compare October 12, 2022 21:17

rhatdan force-pushed the XDG_RUNTIME_DIR branch 2 times, most recently from fcef9f7 to d9c00f6 Compare October 17, 2022 14:08

rhatdan force-pushed the XDG_RUNTIME_DIR branch from d9c00f6 to 7c6bf96 Compare October 19, 2022 12:52

rhatdan force-pushed the XDG_RUNTIME_DIR branch from 7c6bf96 to a2b935f Compare October 19, 2022 15:11

edsantiago reviewed Oct 19, 2022

View reviewed changes

rhatdan force-pushed the XDG_RUNTIME_DIR branch from a2b935f to ee9aeae Compare October 19, 2022 18:34

edsantiago reviewed Oct 19, 2022

View reviewed changes

Luap99 requested changes Oct 20, 2022

View reviewed changes

rhatdan force-pushed the XDG_RUNTIME_DIR branch 2 times, most recently from 2b42abf to ba5e896 Compare October 28, 2022 09:47

Eval symlinks on XDG_RUNTIME_DIR

71f0c9f

Partial Fix for containers#14606 [NO NEW TESTS NEEDED] Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the XDG_RUNTIME_DIR branch from ba5e896 to 71f0c9f Compare October 28, 2022 18:32

openshift-ci bot assigned edsantiago Oct 31, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022

openshift-merge-robot merged commit ee896b2 into containers:main Oct 31, 2022

This was referenced Nov 1, 2022

IPAM error: failed to open database /run/user/1000/containers/networks/ipam.db #14606

Closed

rootless netns: eval symlink for XDG_RUNTIME_DIR #14668

Closed

edsantiago mentioned this pull request Nov 14, 2022

sdnotify play kube policies: podman container wait, hangs #16076

Closed

rhatdan deleted the XDG_RUNTIME_DIR branch December 1, 2022 22:00

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eval symlinks on XDG_RUNTIME_DIR #15918

Eval symlinks on XDG_RUNTIME_DIR #15918

rhatdan commented Sep 23, 2022

openshift-ci bot commented Sep 23, 2022

mheon Sep 23, 2022

mheon commented Oct 13, 2022

rhatdan commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago Oct 19, 2022

edsantiago Oct 19, 2022

rhatdan Oct 19, 2022

edsantiago Oct 19, 2022

rhatdan Oct 19, 2022

edsantiago Oct 19, 2022

rhatdan Oct 19, 2022

edsantiago Oct 19, 2022

rhatdan commented Oct 19, 2022

Luap99 left a comment

edsantiago commented Oct 20, 2022

rhatdan commented Oct 29, 2022

edsantiago commented Oct 31, 2022

		runtimeDir := os.Getenv("XDG_RUNTIME_DIR")
		if runtimeDir != "" {

Eval symlinks on XDG_RUNTIME_DIR #15918

Eval symlinks on XDG_RUNTIME_DIR #15918

Conversation

rhatdan commented Sep 23, 2022

Does this PR introduce a user-facing change?

openshift-ci bot commented Sep 23, 2022

Choose a reason for hiding this comment

mheon commented Oct 13, 2022

rhatdan commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

edsantiago commented Oct 19, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Oct 19, 2022

Luap99 left a comment

Choose a reason for hiding this comment

edsantiago commented Oct 20, 2022

rhatdan commented Oct 29, 2022

edsantiago commented Oct 31, 2022