runtime: Warn if `XDG_RUNTIME_DIR` is set but is not writable. #11327

flouthoc · 2021-08-26T10:53:30Z

If user is rootless and XDG_RUNTIME_DIR is found, podman will not proceed with /tmp directory it will try to use existing XDG_RUNTIME_DIR and if current user has no write access to XDG_RUNTIME_DIR podman session will fail later

flouthoc · 2021-08-26T10:54:15Z

We will have to see if tests are unhappy with this warning.

flouthoc · 2021-08-26T10:54:48Z

cc @giuseppe @mheon @rhatdan PTAL

libpod/runtime.go

Luap99 · 2021-08-26T11:57:13Z

libpod/runtime.go

+	// if current user has no write access to XDG_RUNTIME_DIR we will fail later
+	if runtimeDir != "" {
+		if unix.Access(runtimeDir, unix.W_OK) != nil {
+			logrus.Warnf("Required login session. Note: For su or sudo try `su -i | sudo -l")


su -i or sudo -l do not create a systemd session for the user. Only ssh or machinectl do.

Yes but they do create enough of a session that podman will work, I believe.

This PR adds a warning if XDG_RUNTIME_DIR is set but not writeable, telling the user to use su -i or sudo -l does not make XDG_RUNTIME_DIR writeable because systemd still doesn't create a session for the user.

@Luap99 I sort of agree i think user must do loginctl or by any means create a session before performing any su commands. I think we cannot make su work at all without patching su as it doesn't really cares about systemd session.

I am not really sure about this. Should we just change warning to Note: su or sudo with -c might not work or we can fall back to tmpdir but pretty sure it will fail later somewhere especially for cgroupv2 in case of falling back to tmpdir

I think it should just warn about XDG_RUNTIME_DIR not being writable.

The user could have a valid session but the XDG_RUNTIME_DIR has the wrong value.

rhatdan · 2021-08-26T12:27:40Z

@giuseppe @mheon WDYT?

This LGTM, but as @Luap99 are we just pushing the problem down the road?

giuseppe · 2021-08-26T13:03:40Z

This LGTM, but as @Luap99 are we just pushing the problem down the road?

I think we should not suggest what the fix is. It should be enough to say the directory is not writable

mheon · 2021-08-26T13:12:36Z

Check is in the wrong place, should be moved to after https://github.com/containers/podman/blob/52ceb273c04854efca34a90231a4db45deabc471/libpod/runtime.go#L340

Basically, at this point, we're determining what the runtime directory should be for a new installation. If a runtime directory has already been chosen, whatever we choose here will be overwritten by the DB at this point. So we could be warning that XDG_RUNTIME_DIR is not writable when the user has explicitly chosen a directory that is actually usable.

flouthoc · 2021-08-26T13:17:38Z

@mheon Would it be better if we check everytime a user namespace is created ?

mheon · 2021-08-26T13:20:07Z

We will not have the correct directories at that point. We need to check once we have the final rundir nailed down, and the line I linked is when that happens.

flouthoc · 2021-08-26T13:24:27Z

@giuseppe @mheon @Luap99 @rhatdan PTAL. I have changed warning message as well.

libpod/runtime.go

mheon · 2021-08-26T13:33:55Z

libpod/runtime.go

+	// it will try to use existing XDG_RUNTIME_DIR
+	// if current user has no write access to XDG_RUNTIME_DIR we will fail later
+	if unix.Access(runtime.storageConfig.RunRoot, unix.W_OK) != nil {
+		logrus.Warnf("XDG_RUNTIME_DIR is pointing to a path which is not writable. Most likely podman will fail.")


Suggest changing "XDG_RUNTIME_DIR" to "Temporary files directory"

I think only actionable from user's side which user can take if this warning shows up is to debug using XDG_RUNTIME_DIR , "Temporary files directory" might not pass enough context to user WDYT ?

Why, users will have no idea what that means? I think we should make this smarter to at lease check if XDG_RUNTIME_DIR is set and report it in that case.

Can we update the podman run and podman start man pages with information about loginctl, so users have a chance to figure this out.

This is too common an error for us to keep getting issues and bugzillas. Users need to have a clue on how to fix this without having to talk to Support.

Eventually this becomes a costly support issue for RHEL.

Printing the directory path seems reasonable to me.

@rhatdan On v1 systems it is perfectly possible to get Podman running without a login session, and a lot of people are doing it unintentionally by running for the first time under sudo or su without a proper login session. We can't start throwing warnings about perfectly-working configurations - we should only check XDG_RUNTIME_DIR if we are actually using it.

Does this mean a user with XDG_RUNTIME_DIR set to an unwritable directory, podman will work in V1? If yes, then we should add a check for cgroupv2.

How about we add something like this into man pages

Note: Podman expects a valid login session for `rootless` use-case, In most cases podman will figure out solution on its own but if `XDG_RUNTIME_DIR` is pointing to path which is not writable most likely execution will fail. In such cases `rootless` user needs a valid systemd session.

Since the three most common cases for this are
1 su
2 sudo
3 systemd unit files, we should document all three.

XDG_RUNTIME_DIR is just a side effect. The problem is people logging in as root and then lauching rootless containers by switching to a rootless user. In the case of XDG_RUNTIME_DIR being set incorrectly, the problem is that su (and maybe sudo) do not clean the environment when you change users, therefore rootless podman attempts to use roots XDG_RUNTIME_DIR.

Added a check for cgroupv2 and updated docs. Will add into start man page as well if note looks fine.

docs/source/markdown/podman-run.1.md

libpod/runtime.go

jwhonce · 2021-08-26T23:17:39Z

@baude @ashley-cui Is the use of golang.org/x/sys/unix here going to cause issues for remote? I know we usually use os.Stat() and test for os.IsExist()

Luap99 · 2021-08-27T07:59:50Z

@baude @ashley-cui Is the use of golang.org/x/sys/unix here going to cause issues for remote? I know we usually use os.Stat() and test for os.IsExist()

The libpod package should never be imported on the remote client so this is fine.

[NO TESTS NEEDED] Signed-off-by: Aditya Rajan <[email protected]>

flouthoc · 2021-08-30T11:19:08Z

@giuseppe @Luap99 @mheon @rhatdan @TomSweeneyRedHat Moved resolution guide to troubleshooting page, could you all please take a look.

giuseppe · 2021-08-30T12:02:06Z

libpod/runtime.go

+		// If user is rootless and XDG_RUNTIME_DIR is found, podman will not proceed with /tmp directory
+		// it will try to use existing XDG_RUNTIME_DIR
+		// if current user has no write access to XDG_RUNTIME_DIR we will fail later
+		if unix.Access(runtime.storageConfig.RunRoot, unix.W_OK) != nil {


I am not sure this is the right place for the check.

runtime.storageConfig.RunRoot could have been set for a different reason than XDG_RUNTIME_DIR being specified.

Would it work if we have it in SetXdgDirs for the case where os.Getenv("XDG_RUNTIME_DIR") != "" ?

@giuseppe first version of PR had check exactly where you suggested, moved it after @mheon suggestion reason is added in the comment itself #11327 (comment)

This seems more sane - there's no guarantee Podman is actually using XDG_RUNTIME_DIR (we can fall back to other directories). This guarantees that we are checking whatever the currently-configured temporary files directory is.

thanks for the clarification. It makes sense.

flouthoc · 2021-09-02T12:16:01Z

/assign @mheon

flouthoc · 2021-09-02T12:17:17Z

assigning @mheon for review since he is already writing stuff on this.

mheon · 2021-09-07T17:54:38Z

LGTM

rhatdan · 2021-09-08T09:19:32Z

/lgtm

rhatdan · 2021-09-08T17:09:19Z

/approve

openshift-ci · 2021-09-08T17:09:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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

flouthoc commented Aug 26, 2021

View reviewed changes

libpod/runtime.go Outdated Show resolved Hide resolved

giuseppe reviewed Aug 26, 2021

View reviewed changes

libpod/runtime.go Outdated Show resolved Hide resolved

flouthoc force-pushed the warn-non-writeable-xdg-runtime branch from 7ed6953 to a29273c Compare August 26, 2021 11:03

rhatdan reviewed Aug 26, 2021

View reviewed changes

libpod/runtime.go Show resolved Hide resolved

flouthoc force-pushed the warn-non-writeable-xdg-runtime branch from a29273c to 52ceb27 Compare August 26, 2021 11:35

flouthoc requested review from giuseppe and rhatdan August 26, 2021 11:37

Luap99 reviewed Aug 26, 2021

View reviewed changes

flouthoc requested a review from Luap99 August 26, 2021 12:43

flouthoc force-pushed the warn-non-writeable-xdg-runtime branch from 52ceb27 to ce0834d Compare August 26, 2021 13:23

mheon reviewed Aug 26, 2021

View reviewed changes

libpod/runtime.go Outdated Show resolved Hide resolved

flouthoc force-pushed the warn-non-writeable-xdg-runtime branch from ce0834d to 39cc5c8 Compare August 26, 2021 13:30

mheon reviewed Aug 26, 2021

View reviewed changes

flouthoc force-pushed the warn-non-writeable-xdg-runtime branch 2 times, most recently from 63c10b1 to 667e2cb Compare August 26, 2021 14:44