From 57b022782bba8cd48865f9dd84e9fea8a1588e4c Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Thu, 17 Oct 2024 14:40:18 +0200
Subject: [PATCH] quadlet: ensure user units wait for the network

As documented in the issue there is no way to wait for system units from
the user session[1]. This causes problems for rootless quadlet units as
they might be started before the network is fully up. TWhile this was
always the case and thus was never really noticed the main thing that
trigger a bunch of errors was the switch to pasta.

Pasta requires the network to be fully up in order to correctly select
the right "template" interface based on the routes. If it cannot find a
suitable interface it just fails and we cannot start the container
understandingly leading to a lot of frustration from users.

As there is no sign of any movement on the systemd issue we work around
here by using our own user unit that check if the system session
network-online.target it ready.

Now for testing it is a bit complicated. While we do now correctly test
the root and rootless generator since commit ada75c0bb8 the resulting
Wants/After= lines differ between them and there is no logic in the
testfiles themself to say if root/rootless to match specifics. One idea
was to use `assert-key-is-rootless/root` but that seemed like more
duplication for little reason so use a regex and allow both to make it
pass always. To still have some test coverage add a check in the system
test to ask systemd if we did indeed have the right depdendencies where
we can check for exact root/rootless name match.

[1] https://github.com/systemd/systemd/issues/3312

Fixes #22197

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 cmd/quadlet/main.go                           |  4 +-
 docs/source/markdown/podman-systemd.unit.5.md | 22 +++++----
 pkg/systemd/quadlet/quadlet.go                | 47 ++++++++++---------
 test/e2e/quadlet/basic.build                  |  4 +-
 test/e2e/quadlet/basic.container              |  4 +-
 test/e2e/quadlet/basic.image                  |  4 +-
 test/e2e/quadlet/mount.container              |  2 +-
 test/e2e/quadlet/mount.servicename.container  |  2 +-
 test/e2e/quadlet/network.quadlet.build        |  2 +-
 test/e2e/quadlet/network.quadlet.container    |  2 +-
 .../quadlet/network.quadlet.servicename.build |  2 +-
 .../network.quadlet.servicename.container     |  2 +-
 test/e2e/quadlet/network.reuse.container      |  2 +-
 test/e2e/quadlet/network.reuse.name.container |  2 +-
 test/e2e/quadlet/volume.quadlet.build         |  2 +-
 .../quadlet/volume.quadlet.servicename.build  |  2 +-
 test/e2e/quadlet/volume.servicename.container |  2 +-
 test/system/252-quadlet.bats                  |  9 ++++
 18 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/cmd/quadlet/main.go b/cmd/quadlet/main.go
index 4433fb9a15..49efe103f2 100644
--- a/cmd/quadlet/main.go
+++ b/cmd/quadlet/main.go
@@ -739,9 +739,9 @@ func process() error {
 			service, err = quadlet.ConvertNetwork(unit, unit.Filename, unitsInfoMap)
 		case strings.HasSuffix(unit.Filename, ".image"):
 			warnIfAmbiguousName(unit, quadlet.ImageGroup)
-			service, err = quadlet.ConvertImage(unit, unitsInfoMap)
+			service, err = quadlet.ConvertImage(unit, unitsInfoMap, isUserFlag)
 		case strings.HasSuffix(unit.Filename, ".build"):
-			service, err = quadlet.ConvertBuild(unit, unitsInfoMap)
+			service, err = quadlet.ConvertBuild(unit, unitsInfoMap, isUserFlag)
 		case strings.HasSuffix(unit.Filename, ".pod"):
 			service, err = quadlet.ConvertPod(unit, unit.Filename, unitsInfoMap, isUserFlag)
 		default:
diff --git a/docs/source/markdown/podman-systemd.unit.5.md b/docs/source/markdown/podman-systemd.unit.5.md
index a204d62004..47d7dda91a 100644
--- a/docs/source/markdown/podman-systemd.unit.5.md
+++ b/docs/source/markdown/podman-systemd.unit.5.md
@@ -238,9 +238,14 @@ that limit the output to only the units you are debugging.
 
 ### Implicit network dependencies
 
-In the case of Container, Image and Build units, Quadlet will add dependencies on the `network-online.target`
-by adding `After=` and `Wants=` properties to the unit. This is to ensure that the network is reachable if
-an image needs to be pulled.
+In the case of Container, Image and Build units, Quadlet will add dependencies on the `network-online.target` (as root)
+or `podman-user-wait-network-online.service` (as user) by adding `After=` and `Wants=` properties to the unit.
+This is to ensure that the network is reachable if an image needs to be pulled and by the time the container is started.
+
+The special case `podman-user-wait-network-online.service` unit is needed as user because user units are unable to wait
+for system (root) units so `network-online.target` doesn't do anything there and is instead ignored. As this caused
+a significant amount of issues we decided to work around this with our own special purpose unit that simply checks if
+the `network-online.target` unit is active with `systemctl is-active network-online.target`.
 
 This behavior can be disabled by adding `DefaultDependencies=false` in the `Quadlet` section.
 
@@ -1791,10 +1796,10 @@ exists on the host, pulling it if needed.
 Using image units allows containers and volumes to depend on images being automatically pulled. This is
 particularly interesting when using special options to control image pulls.
 
-Note: The generated service have a dependency on `network-online.target` assuring the network is reachable if
-an image needs to be pulled.
-If the image service needs to run without available network (e.g. early in boot), the requirement can be
-overridden simply by adding an empty `After=` in the unit file. This will unset all previously set After's.
+Note: The generated service have a dependency on `network-online.target` or
+`podman-user-wait-network-online.service` assuring the network is reachable if an image needs to be pulled.
+If the image service needs to run without available network (e.g. early in boot), this behavior
+can be disabled by adding `DefaultDependencies=false` in the `Quadlet` section.
 
 Valid options for `[Image]` are listed below:
 
@@ -1936,7 +1941,8 @@ Valid options for `[Quadlet]` are listed below:
 
 Add Quadlet's default network dependencies to the unit (default is `true`).
 
-When set to false, Quadlet will **not** add a dependency (After=, Wants=) to `network-online.target` to the generated unit.
+When set to false, Quadlet will **not** add a dependency (After=, Wants=) to
+`network-online.target`/`podman-user-wait-network-online.service` to the generated unit.
 
 ## EXAMPLES
 
diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go
index a369482fdf..611b510143 100644
--- a/pkg/systemd/quadlet/quadlet.go
+++ b/pkg/systemd/quadlet/quadlet.go
@@ -524,13 +524,7 @@ func ConvertContainer(container *parser.UnitFile, isUser bool, unitsInfoMap map[
 	service := container.Dup()
 	service.Filename = unitInfo.ServiceFileName()
 
-	// Add a dependency on network-online.target so the image pull does not happen
-	// before network is ready
-	// https://github.com/containers/podman/issues/21873
-	if service.LookupBooleanWithDefault(QuadletGroup, KeyDefaultDependencies, true) {
-		service.PrependUnitLine(UnitGroup, "After", "network-online.target")
-		service.PrependUnitLine(UnitGroup, "Wants", "network-online.target")
-	}
+	addDefaultDependencies(service, isUser)
 
 	if container.Path != "" {
 		service.Add(UnitGroup, "SourcePath", container.Path)
@@ -1282,7 +1276,7 @@ func ConvertKube(kube *parser.UnitFile, unitsInfoMap map[string]*UnitInfo, isUse
 	return service, nil
 }
 
-func ConvertImage(image *parser.UnitFile, unitsInfoMap map[string]*UnitInfo) (*parser.UnitFile, error) {
+func ConvertImage(image *parser.UnitFile, unitsInfoMap map[string]*UnitInfo, isUser bool) (*parser.UnitFile, error) {
 	unitInfo, ok := unitsInfoMap[image.Filename]
 	if !ok {
 		return nil, fmt.Errorf("internal error while processing network %s", image.Filename)
@@ -1291,13 +1285,7 @@ func ConvertImage(image *parser.UnitFile, unitsInfoMap map[string]*UnitInfo) (*p
 	service := image.Dup()
 	service.Filename = unitInfo.ServiceFileName()
 
-	// Add a dependency on network-online.target so the image pull does not happen
-	// before network is ready
-	// https://github.com/containers/podman/issues/21873
-	if service.LookupBooleanWithDefault(QuadletGroup, KeyDefaultDependencies, true) {
-		service.PrependUnitLine(UnitGroup, "After", "network-online.target")
-		service.PrependUnitLine(UnitGroup, "Wants", "network-online.target")
-	}
+	addDefaultDependencies(service, isUser)
 
 	if image.Path != "" {
 		service.Add(UnitGroup, "SourcePath", image.Path)
@@ -1365,7 +1353,7 @@ func ConvertImage(image *parser.UnitFile, unitsInfoMap map[string]*UnitInfo) (*p
 	return service, nil
 }
 
-func ConvertBuild(build *parser.UnitFile, unitsInfoMap map[string]*UnitInfo) (*parser.UnitFile, error) {
+func ConvertBuild(build *parser.UnitFile, unitsInfoMap map[string]*UnitInfo, isUser bool) (*parser.UnitFile, error) {
 	unitInfo, ok := unitsInfoMap[build.Filename]
 	if !ok {
 		return nil, fmt.Errorf("internal error while processing network %s", build.Filename)
@@ -1379,13 +1367,7 @@ func ConvertBuild(build *parser.UnitFile, unitsInfoMap map[string]*UnitInfo) (*p
 	service := build.Dup()
 	service.Filename = unitInfo.ServiceFileName()
 
-	// Add a dependency on network-online.target so the image pull does not happen
-	// before network is ready
-	// https://github.com/containers/podman/issues/21873
-	if service.LookupBooleanWithDefault(QuadletGroup, KeyDefaultDependencies, true) {
-		service.PrependUnitLine(UnitGroup, "After", "network-online.target")
-		service.PrependUnitLine(UnitGroup, "Wants", "network-online.target")
-	}
+	addDefaultDependencies(service, isUser)
 
 	/* Rename old Build group to X-Build so that systemd ignores it */
 	service.RenameGroup(BuildGroup, XBuildGroup)
@@ -2184,3 +2166,22 @@ func addVolumes(quadletUnitFile, serviceUnitFile *parser.UnitFile, groupName str
 
 	return nil
 }
+
+func addDefaultDependencies(service *parser.UnitFile, isUser bool) {
+	// Add a dependency on network-online.target so the image pull container startup
+	// does not happen before network is ready.
+	// https://github.com/containers/podman/issues/21873
+	if service.LookupBooleanWithDefault(QuadletGroup, KeyDefaultDependencies, true) {
+		networkUnit := "network-online.target"
+		// network-online.target only exists as root and user session cannot wait for it
+		// https://github.com/systemd/systemd/issues/3312
+		// Given this is a bad problem with pasta which can fail to start or use the
+		// wrong interface if the network is not fully set up we need to work around
+		// that: https://github.com/containers/podman/issues/22197.
+		if isUser {
+			networkUnit = "podman-user-wait-network-online.service"
+		}
+		service.PrependUnitLine(UnitGroup, "After", networkUnit)
+		service.PrependUnitLine(UnitGroup, "Wants", networkUnit)
+	}
+}
diff --git a/test/e2e/quadlet/basic.build b/test/e2e/quadlet/basic.build
index b8d5d08c4c..ebc7cbc791 100644
--- a/test/e2e/quadlet/basic.build
+++ b/test/e2e/quadlet/basic.build
@@ -1,7 +1,7 @@
 ## assert-podman-final-args-regex /.*/podman-e2e-.*/subtest-.*/quadlet
 ## assert-podman-args "--tag" "localhost/imagename"
-## assert-key-is "Unit" "After" "network-online.target"
-## assert-key-is "Unit" "Wants" "network-online.target"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service"
+## assert-key-is-regex "Unit" "Wants" "network-online.target|podman-user-wait-network-online.service"
 ## assert-key-is "Unit" "RequiresMountsFor" "%t/containers"
 ## assert-key-is-regex "Service" "WorkingDirectory" "/.*/podman-e2e-.*/subtest-.*/quadlet"
 ## assert-key-is "Service" "Type" "oneshot"
diff --git a/test/e2e/quadlet/basic.container b/test/e2e/quadlet/basic.container
index cbceda4e4c..5f7417dbf7 100644
--- a/test/e2e/quadlet/basic.container
+++ b/test/e2e/quadlet/basic.container
@@ -15,8 +15,8 @@
 ## assert-key-is-regex "Service" "ExecStopPost" "-[/S].*/podman rm -v -f -i --cidfile=%t/%N.cid"
 ## assert-key-is-regex "Service" "ExecStop" ".*/podman rm -v -f -i --cidfile=%t/%N.cid"
 ## assert-key-is "Service" "Environment" "PODMAN_SYSTEMD_UNIT=%n"
-## assert-key-is "Unit" "After" "network-online.target"
-## assert-key-is "Unit" "Wants" "network-online.target"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service"
+## assert-key-is-regex "Unit" "Wants" "network-online.target|podman-user-wait-network-online.service"
 
 [Container]
 Image=localhost/imagename
diff --git a/test/e2e/quadlet/basic.image b/test/e2e/quadlet/basic.image
index 9a30765e52..cb1bcf34ca 100644
--- a/test/e2e/quadlet/basic.image
+++ b/test/e2e/quadlet/basic.image
@@ -1,6 +1,6 @@
 ## assert-podman-final-args localhost/imagename
-## assert-key-is "Unit" "After" "network-online.target"
-## assert-key-is "Unit" "Wants" "network-online.target"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service"
+## assert-key-is-regex "Unit" "Wants" "network-online.target|podman-user-wait-network-online.service"
 ## assert-key-is "Unit" "RequiresMountsFor" "%t/containers"
 ## assert-key-is "Service" "Type" "oneshot"
 ## assert-key-is "Service" "RemainAfterExit" "yes"
diff --git a/test/e2e/quadlet/mount.container b/test/e2e/quadlet/mount.container
index bd156dc72b..615fe58631 100644
--- a/test/e2e/quadlet/mount.container
+++ b/test/e2e/quadlet/mount.container
@@ -10,7 +10,7 @@ Mount=type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared,U=true
 Mount=type=volume,source=vol1,destination=/path/in/container,ro=true
 ## assert-podman-args-key-val "--mount" "," "type=volume,source=systemd-basic,destination=/path/in/container,ro=true"
 ## assert-key-is "Unit" "Requires" "basic-volume.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic-volume.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic-volume.service"
 Mount=type=volume,source=basic.volume,destination=/path/in/container,ro=true
 ## assert-podman-args-key-val "--mount" "," "type=tmpfs,tmpfs-size=512M,destination=/path/in/container"
 Mount=type=tmpfs,tmpfs-size=512M,destination=/path/in/container
diff --git a/test/e2e/quadlet/mount.servicename.container b/test/e2e/quadlet/mount.servicename.container
index 95be8f82af..00a80a5af8 100644
--- a/test/e2e/quadlet/mount.servicename.container
+++ b/test/e2e/quadlet/mount.servicename.container
@@ -2,5 +2,5 @@
 Image=localhost/imagename
 ## assert-podman-args-key-val "--mount" "," "type=volume,source=test-volume,destination=/path/in/container,ro=true"
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 Mount=type=volume,source=service-name.volume,destination=/path/in/container,ro=true
diff --git a/test/e2e/quadlet/network.quadlet.build b/test/e2e/quadlet/network.quadlet.build
index eb755dcedd..f38c820d21 100644
--- a/test/e2e/quadlet/network.quadlet.build
+++ b/test/e2e/quadlet/network.quadlet.build
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "systemd-basic"
 ## assert-key-is "Unit" "Requires" "basic-network.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic-network.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic-network.service"
 
 [Build]
 ImageTag=localhost/imagename
diff --git a/test/e2e/quadlet/network.quadlet.container b/test/e2e/quadlet/network.quadlet.container
index 7811cad10c..ba805e3273 100644
--- a/test/e2e/quadlet/network.quadlet.container
+++ b/test/e2e/quadlet/network.quadlet.container
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "systemd-basic"
 ## assert-key-is "Unit" "Requires" "basic-network.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic-network.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic-network.service"
 
 [Container]
 Image=localhost/imagename
diff --git a/test/e2e/quadlet/network.quadlet.servicename.build b/test/e2e/quadlet/network.quadlet.servicename.build
index a06c030ebd..95137f673d 100644
--- a/test/e2e/quadlet/network.quadlet.servicename.build
+++ b/test/e2e/quadlet/network.quadlet.servicename.build
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "test-network"
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 
 [Build]
 ImageTag=localhost/imagename
diff --git a/test/e2e/quadlet/network.quadlet.servicename.container b/test/e2e/quadlet/network.quadlet.servicename.container
index 54c74fd1ea..d28791b463 100644
--- a/test/e2e/quadlet/network.quadlet.servicename.container
+++ b/test/e2e/quadlet/network.quadlet.servicename.container
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "test-network"
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 
 [Container]
 Image=localhost/imagename
diff --git a/test/e2e/quadlet/network.reuse.container b/test/e2e/quadlet/network.reuse.container
index e26546ba44..f9a6f6b142 100644
--- a/test/e2e/quadlet/network.reuse.container
+++ b/test/e2e/quadlet/network.reuse.container
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "container:systemd-basic"
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 
 [Container]
 Image=localhost/imagename
diff --git a/test/e2e/quadlet/network.reuse.name.container b/test/e2e/quadlet/network.reuse.name.container
index 49fef56267..2927e6fea0 100644
--- a/test/e2e/quadlet/network.reuse.name.container
+++ b/test/e2e/quadlet/network.reuse.name.container
@@ -1,6 +1,6 @@
 ## assert-podman-args "--network" "container:foobar"
 ## assert-key-is "Unit" "Requires" "name.service"
-## assert-key-is "Unit" "After" "network-online.target" "name.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "name.service"
 
 [Container]
 Image=localhost/imagename
diff --git a/test/e2e/quadlet/volume.quadlet.build b/test/e2e/quadlet/volume.quadlet.build
index 6cb57bda13..08ede62f3e 100644
--- a/test/e2e/quadlet/volume.quadlet.build
+++ b/test/e2e/quadlet/volume.quadlet.build
@@ -1,6 +1,6 @@
 ## assert-podman-args "-v" "systemd-basic:/volume/basic"
 ## assert-key-is "Unit" "Requires" "basic-volume.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic-volume.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic-volume.service"
 
 [Build]
 ImageTag=localhost/imagename
diff --git a/test/e2e/quadlet/volume.quadlet.servicename.build b/test/e2e/quadlet/volume.quadlet.servicename.build
index d33df93335..4034d2b4c1 100644
--- a/test/e2e/quadlet/volume.quadlet.servicename.build
+++ b/test/e2e/quadlet/volume.quadlet.servicename.build
@@ -1,6 +1,6 @@
 ## assert-podman-args "-v" "test-volume:/volume/basic"
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 
 [Build]
 ImageTag=localhost/imagename
diff --git a/test/e2e/quadlet/volume.servicename.container b/test/e2e/quadlet/volume.servicename.container
index dece6a4af3..aeb7bb7644 100644
--- a/test/e2e/quadlet/volume.servicename.container
+++ b/test/e2e/quadlet/volume.servicename.container
@@ -1,6 +1,6 @@
 [Container]
 Image=localhost/imagename
 ## assert-key-is "Unit" "Requires" "basic.service"
-## assert-key-is "Unit" "After" "network-online.target" "basic.service"
+## assert-key-is-regex "Unit" "After" "network-online.target|podman-user-wait-network-online.service" "basic.service"
 ## assert-podman-args -v test-volume:/container/quadlet
 Volume=service-name.volume:/container/quadlet
diff --git a/test/system/252-quadlet.bats b/test/system/252-quadlet.bats
index c7a80277e1..b91030e111 100644
--- a/test/system/252-quadlet.bats
+++ b/test/system/252-quadlet.bats
@@ -201,6 +201,15 @@ EOF
     run_quadlet "$quadlet_file"
     service_setup $QUADLET_SERVICE_NAME
 
+    run -0 systemctl show --property=Wants --property=After "$QUADLET_SERVICE_NAME"
+    service="network-online.target"
+    if is_rootless; then
+        service="podman-user-wait-network-online.service"
+    fi
+    assert "${lines[0]}" == "Wants=$service" "quadlet unit Wants network dependency"
+    # Note systemd adds some other default services to After= so no exact match possible
+    assert "${lines[1]}" =~ "After=.*$service.*" "quadlet unit After network dependency"
+
     # Check that we can read the logs from the container with podman logs even
     # with the `passthrough` driver.  The log may need a short period of time
     # to bubble up into the journal logs, so wait for it.