cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. #11212

flouthoc · 2021-08-12T13:25:10Z

If podman is invoked with cgroup manager as systemd and user is rootless , podman should fail early if no valid session is present.

Problem:
Currently following scenario is handled by runtimes. Idea is to make experience more seamless for users with appropriate failures at manager level.

PS: We can also just spit a warning instead of strict failure.

Closes: #11197

flouthoc · 2021-08-12T13:26:53Z

libpod/container_internal_linux.go

+		// If user is rootless and no valid systemd user session is present then break early.
+		if rootless.IsRootless() {
+			if !utils.CheckifSystemdSessionisValid(rootless.GetRootlessUID()) {
+				return "", errors.Wrapf(define.ErrInternal, "invalid systemd user session")


Can we have a warning instead of hard failure and let runtime handle the failures ?

Warning is probably better, the configuration could work.

flouthoc · 2021-08-12T13:31:03Z

@mheon Could you please take a look ? We can also spit a warning instead of explicit failure.

mheon · 2021-08-12T13:32:07Z

I'm unsure as to whether this is a good idea. We have a lot of people running rootless Podman in cron jobs, systemd units, and other environments that don't have a login session, generally with success. Warning line is a lot better, and we might even want to drop it down to Info level so it's not printed by default, but people will still see it with --log-level=debug.

Also, this shouldn't be for all rootless systems - I think this is Rootless CGv2 + Systemd cgroups driver only.

flouthoc · 2021-08-12T13:34:59Z

@mheon Agreed we can suppress this to debug log level as well.

giuseppe · 2021-08-12T14:11:45Z

utils/utils_supported.go

@@ -101,6 +101,20 @@ func GetCgroupProcess(pid int) (string, error) {
 	return getCgroupProcess(fmt.Sprintf("/proc/%d/cgroup", pid))
 }

+// CheckifSystemdSessionisValid checks if valid user session ever existsted or not.
+func CheckifSystemdSessionisValid(uid int) bool {
+	slicePath := fmt.Sprintf("/sys/fs/cgroup/user.slice/user-%d.slice/user@%d.service", uid, uid)


does systemd document that it uses always this path and it won't be changed in future?

Should we define this in containers.conf just in case?

@giuseppe @TomSweeneyRedHat I checked so systemd doesn't guarantee about this path, although its accepted as a norm in most of cases but in some cases it could also be /sys/fs/cgroup/user.slice/user-1000.slice/session-11.scope. I'll refactor the code and get session details from Dbus instead.

@giuseppe @TomSweeneyRedHat Made changes instead of doing a stat on a static path we are now verifying valid systemd session for rootless user using d-bus interface of systemd-logind following method is guaranteed by systemd.

libpod/container_internal_linux.go

giuseppe

LGTM

flouthoc · 2021-08-16T12:03:15Z

@TomSweeneyRedHat @rhatdan PTAL

rhatdan · 2021-08-16T12:59:03Z

libpod/runtime.go

+			if runtime.config.Engine.CgroupManager == config.SystemdCgroupsManager {
+				unified, _ := cgroups.IsCgroup2UnifiedMode()
+				if unified && rootless.IsRootless() && !utils.CheckifSystemdSessionisValid(rootless.GetRootlessUID()) {
+					logrus.Infof("Invalid systemd user session for current user")


This should be higher the Infof, most likely Warning.
Info does not show by default, so likely no one will see this.

@rhatdan changed to warning instead :)

rhatdan · 2021-08-16T13:01:55Z

utils/utils_supported.go

@@ -101,6 +101,72 @@ func GetCgroupProcess(pid int) (string, error) {
 	return getCgroupProcess(fmt.Sprintf("/proc/%d/cgroup", pid))
 }

+// CheckifSystemdSessionisValid checks if sessions is valid for provided rootless uid.
+func CheckifSystemdSessionisValid(uid int) bool {


You must like to type. How about
IsSystemdSessionValid()
ValidSystemdSession()

There is a pkg/systemd, would this make more sense under there. Then you could have
systemd.IsSessionValid()

Sure I'll do that.

@rhatdan this is implemented in latest commit.

pkg/systemd/dbus.go

openshift-ci · 2021-08-16T18:42:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

flouthoc · 2021-08-17T05:37:42Z

@rhatdan added logrus.Debugf for internal errors instead of logrus.Warnf as few of tests are invoking top level commands like info with cgroup manager=systemd such tests will get warning for "invalid systemd sessions" but they have no intentions of running a container so I think we can use logrus.Debugf instead of a warning otherwise we need to fix downstream tests so that they expect a warning.

…on is not present. [NO TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>

rhatdan · 2021-08-17T21:06:20Z

/lgtm

flouthoc commented Aug 12, 2021

View reviewed changes

flouthoc force-pushed the check-valid-systemd-session branch 2 times, most recently from 940eeb6 to a68560e Compare August 12, 2021 13:55

flouthoc requested a review from mheon August 12, 2021 13:56

giuseppe reviewed Aug 12, 2021

View reviewed changes

flouthoc force-pushed the check-valid-systemd-session branch 2 times, most recently from 33557c4 to 91ba60a Compare August 15, 2021 11:26

flouthoc changed the title ~~cgroup-manager-systemd: Fail early if user is rootless and no relevent user session is present.~~ cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. Aug 15, 2021

giuseppe reviewed Aug 15, 2021

View reviewed changes

libpod/container_internal_linux.go Outdated Show resolved Hide resolved

flouthoc force-pushed the check-valid-systemd-session branch from 91ba60a to faded89 Compare August 16, 2021 08:09

flouthoc requested a review from giuseppe August 16, 2021 08:10

giuseppe approved these changes Aug 16, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2021

flouthoc force-pushed the check-valid-systemd-session branch from faded89 to b5faabd Compare August 16, 2021 08:17

flouthoc requested a review from TomSweeneyRedHat August 16, 2021 08:50

rhatdan reviewed Aug 16, 2021

View reviewed changes

flouthoc force-pushed the check-valid-systemd-session branch 2 times, most recently from 86add8d to 1a698bf Compare August 16, 2021 13:32

flouthoc requested a review from rhatdan August 16, 2021 13:32

rhatdan approved these changes Aug 16, 2021

View reviewed changes

flouthoc force-pushed the check-valid-systemd-session branch 2 times, most recently from 76c9c58 to 37597a3 Compare August 16, 2021 18:28

flouthoc requested a review from rhatdan August 16, 2021 18:29

rhatdan approved these changes Aug 16, 2021

View reviewed changes

pkg/systemd/dbus.go Show resolved Hide resolved

pkg/systemd/dbus.go Show resolved Hide resolved

pkg/systemd/dbus.go Show resolved Hide resolved

pkg/systemd/dbus.go Show resolved Hide resolved

pkg/systemd/dbus.go Outdated Show resolved Hide resolved

flouthoc force-pushed the check-valid-systemd-session branch from 37597a3 to 6b0d131 Compare August 17, 2021 05:34

flouthoc force-pushed the check-valid-systemd-session branch from 6b0d131 to f3b58bc Compare August 17, 2021 05:35

flouthoc force-pushed the check-valid-systemd-session branch from f3b58bc to 9d8fac6 Compare August 17, 2021 09:21

cgroup-manager-systemd:Fail early if user:rootless and relevent sessi…

e7ee15f

…on is not present. [NO TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the check-valid-systemd-session branch from 9d8fac6 to e7ee15f Compare August 17, 2021 10:49

openshift-ci bot assigned rhatdan Aug 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2021

openshift-ci bot merged commit a3d8b48 into containers:main Aug 17, 2021

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 22, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. #11212

cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. #11212

flouthoc commented Aug 12, 2021 •

edited

Loading

flouthoc Aug 12, 2021

mheon Aug 12, 2021

flouthoc commented Aug 12, 2021

mheon commented Aug 12, 2021

flouthoc commented Aug 12, 2021

giuseppe Aug 12, 2021

TomSweeneyRedHat Aug 13, 2021

flouthoc Aug 14, 2021

flouthoc Aug 15, 2021

giuseppe left a comment

flouthoc commented Aug 16, 2021

rhatdan Aug 16, 2021

flouthoc Aug 16, 2021

rhatdan Aug 16, 2021

flouthoc Aug 16, 2021

flouthoc Aug 16, 2021

openshift-ci bot commented Aug 16, 2021

flouthoc commented Aug 17, 2021 •

edited

Loading

rhatdan commented Aug 17, 2021

cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. #11212

cgroup-manager-systemd: Warn early if user is rootless and no relevent user session is present. #11212

Conversation

flouthoc commented Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Aug 12, 2021

mheon commented Aug 12, 2021

flouthoc commented Aug 12, 2021

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

giuseppe left a comment

Choose a reason for hiding this comment

flouthoc commented Aug 16, 2021

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

openshift-ci bot commented Aug 16, 2021

flouthoc commented Aug 17, 2021 • edited Loading

rhatdan commented Aug 17, 2021

flouthoc commented Aug 12, 2021 •

edited

Loading

flouthoc commented Aug 17, 2021 •

edited

Loading