podman generate systemd --new do not duplicate params

podman generate systemd --new inserts extra idfile arguments. The
generated unit can break when the user did provide their own idfile
arguments as they overwrite the arguments added by generate systemd.
This also happens when a user tries to generate the systemd unit on
a container already create with a --new unit. This should now
create a identical unit. The solution is to remove all user provided
idfile arguments.

This commit also ensures that we do not remove arguments that are part
off the containers entrypoint.

Fixes #9776

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>

pkg/systemd/generate/common.go

@@ -39,20 +39,46 @@ After=network-online.target
 RequiresMountsFor={{{{.GraphRoot}}}} {{{{.RunRoot}}}}
 `
 
-// filterPodFlags removes --pod and --pod-id-file from the specified command.
+// filterPodFlags removes --pod, --pod-id-file and --infra-conmon-pidfile from the specified command.
-func filterPodFlags(command []string) []string {
+// argCount is the number of last arguments which should not be filtered, e.g. the container entrypoint.
+func filterPodFlags(command []string, argCount int) []string {
 	processed := []string{}
-	for i := 0; i < len(command); i++ {
+	for i := 0; i < len(command)-argCount; i++ {
 		s := command[i]
-		if s == "--pod" || s == "--pod-id-file" {
+		if s == "--pod" || s == "--pod-id-file" || s == "--infra-conmon-pidfile" {
 			i++
 			continue
 		}
-		if strings.HasPrefix(s, "--pod=") || strings.HasPrefix(s, "--pod-id-file=") {
+		if strings.HasPrefix(s, "--pod=") ||
+			strings.HasPrefix(s, "--pod-id-file=") ||
+			strings.HasPrefix(s, "--infra-conmon-pidfile=") {
 			continue
 		}
 		processed = append(processed, s)
 	}
+	processed = append(processed, command[len(command)-argCount:]...)
+	return processed
+}
+
+// filterCommonContainerFlags removes --conmon-pidfile, --cidfile and --cgroups from the specified command.
+// argCount is the number of last arguments which should not be filtered, e.g. the container entrypoint.
+func filterCommonContainerFlags(command []string, argCount int) []string {
+	processed := []string{}
+	for i := 0; i < len(command)-argCount; i++ {
+		s := command[i]
+
+		switch {
+		case s == "--conmon-pidfile", s == "--cidfile", s == "--cgroups":
+			i++
+			continue
+		case strings.HasPrefix(s, "--conmon-pidfile="),
+			strings.HasPrefix(s, "--cidfile="),
+			strings.HasPrefix(s, "--cgroups="):
+			continue
+		}
+		processed = append(processed, s)
+	}
+	processed = append(processed, command[len(command)-argCount:]...)
 	return processed
 }
 

pkg/systemd/generate/common_test.go

@@ -1,7 +1,6 @@
 package generate
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -9,22 +8,144 @@ import (
 
 func TestFilterPodFlags(t *testing.T) {
 	tests := []struct {
-		input []string
+		input    []string
+		output   []string
+		argCount int
 	}{
-		{[]string{"podman", "pod", "create"}},
+		{
-		{[]string{"podman", "pod", "create", "--name", "foo"}},
+			[]string{"podman", "pod", "create"},
-		{[]string{"podman", "pod", "create", "--pod-id-file", "foo"}},
+			[]string{"podman", "pod", "create"},
-		{[]string{"podman", "pod", "create", "--pod-id-file=foo"}},
+			0,
-		{[]string{"podman", "run", "--pod", "foo"}},
+		},
-		{[]string{"podman", "run", "--pod=foo"}},
+		{
+			[]string{"podman", "pod", "create", "--name", "foo"},
+			[]string{"podman", "pod", "create", "--name", "foo"},
+			0,
+		},
+		{
+			[]string{"podman", "pod", "create", "--pod-id-file", "foo"},
+			[]string{"podman", "pod", "create"},
+			0,
+		},
+		{
+			[]string{"podman", "pod", "create", "--pod-id-file=foo"},
+			[]string{"podman", "pod", "create"},
+			0,
+		},
+		{
+			[]string{"podman", "pod", "create", "--pod-id-file", "foo", "--infra-conmon-pidfile", "foo"},
+			[]string{"podman", "pod", "create"},
+			0,
+		},
+		{
+			[]string{"podman", "pod", "create", "--pod-id-file", "foo", "--infra-conmon-pidfile=foo"},
+			[]string{"podman", "pod", "create"},
+			0,
+		},
+		{
+			[]string{"podman", "run", "--pod", "foo"},
+			[]string{"podman", "run"},
+			0,
+		},
+		{
+			[]string{"podman", "run", "--pod=foo"},
+			[]string{"podman", "run"},
+			0,
+		},
+		{
+			[]string{"podman", "run", "--pod=foo", "fedora", "podman", "run", "--pod=test", "alpine"},
+			[]string{"podman", "run", "fedora", "podman", "run", "--pod=test", "alpine"},
+			5,
+		},
+		{
+			[]string{"podman", "run", "--pod", "foo", "fedora", "podman", "run", "--pod", "test", "alpine"},
+			[]string{"podman", "run", "fedora", "podman", "run", "--pod", "test", "alpine"},
+			6,
+		},
+		{
+			[]string{"podman", "run", "--pod-id-file=foo", "fedora", "podman", "run", "--pod-id-file=test", "alpine"},
+			[]string{"podman", "run", "fedora", "podman", "run", "--pod-id-file=test", "alpine"},
+			5,
+		},
+		{
+			[]string{"podman", "run", "--pod-id-file", "foo", "fedora", "podman", "run", "--pod-id-file", "test", "alpine"},
+			[]string{"podman", "run", "fedora", "podman", "run", "--pod-id-file", "test", "alpine"},
+			6,
+		},
 	}
 
 	for _, test := range tests {
-		processed := filterPodFlags(test.input)
+		processed := filterPodFlags(test.input, test.argCount)
-		for _, s := range processed {
+		assert.Equal(t, test.output, processed)
-			assert.False(t, strings.HasPrefix(s, "--pod-id-file"))
+	}
-			assert.False(t, strings.HasPrefix(s, "--pod"))
+}
-		}
+
+func TestFilterCommonContainerFlags(t *testing.T) {
+	tests := []struct {
+		input    []string
+		output   []string
+		argCount int
+	}{
+		{
+			[]string{"podman", "run", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--conmon-pidfile", "foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--conmon-pidfile=foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cidfile", "foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cidfile=foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cgroups", "foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cgroups=foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine"},
+			[]string{"podman", "run", "alpine"},
+			1,
+		},
+		{
+			[]string{"podman", "run", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo", "alpine", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo"},
+			[]string{"podman", "run", "alpine", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo"},
+			7,
+		},
+		{
+			[]string{"podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo"},
+			[]string{"podman", "run", "alpine", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo"},
+			4,
+		},
+	}
+
+	for _, test := range tests {
+		processed := filterCommonContainerFlags(test.input, test.argCount)
+		assert.Equal(t, test.output, processed)
 	}
 }
 

pkg/systemd/generate/containers.go

@@ -238,13 +238,7 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 			"--cidfile", "{{{{.ContainerIDFile}}}}",
 			"--cgroups=no-conmon",
 		)
-		// If the container is in a pod, make sure that the
+		remainingCmd := info.CreateCommand[index:]
-		// --pod-id-file is set correctly.
-		if info.Pod != nil {
-			podFlags := []string{"--pod-id-file", "{{{{.Pod.PodIDFile}}}}"}
-			startCommand = append(startCommand, podFlags...)
-			info.CreateCommand = filterPodFlags(info.CreateCommand)
-		}
 
 		// Presence check for certain flags/options.
 		fs := pflag.NewFlagSet("args", pflag.ContinueOnError)
@@ -254,7 +248,16 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 		fs.BoolP("detach", "d", false, "")
 		fs.String("name", "", "")
 		fs.Bool("replace", false, "")
-		fs.Parse(info.CreateCommand[index:])
+		fs.Parse(remainingCmd)
+
+		remainingCmd = filterCommonContainerFlags(remainingCmd, fs.NArg())
+		// If the container is in a pod, make sure that the
+		// --pod-id-file is set correctly.
+		if info.Pod != nil {
+			podFlags := []string{"--pod-id-file", "{{{{.Pod.PodIDFile}}}}"}
+			startCommand = append(startCommand, podFlags...)
+			remainingCmd = filterPodFlags(remainingCmd, fs.NArg())
+		}
 
 		hasDetachParam, err := fs.GetBool("detach")
 		if err != nil {
@@ -266,8 +269,6 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 			return "", err
 		}
 
-		remainingCmd := info.CreateCommand[index:]
-
 		if !hasDetachParam {
 			// Enforce detaching
 			//

pkg/systemd/generate/containers_test.go

@@ -392,6 +392,56 @@ ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
 PIDFile=%t/jadda-jadda.pid
 Type=forking
 
+[Install]
+WantedBy=multi-user.target default.target
+`
+
+	goodNewWithIDFiles := `# jadda-jadda.service
+# autogenerated by Podman CI
+
+[Unit]
+Description=Podman jadda-jadda.service
+Documentation=man:podman-generate-systemd(1)
+Wants=network.target
+After=network-online.target
+RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage
+
+[Service]
+Environment=PODMAN_SYSTEMD_UNIT=%n
+Restart=always
+TimeoutStopSec=70
+ExecStartPre=/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon -d awesome-image:latest podman run --cgroups=foo --conmon-pidfile=foo --cidfile=foo alpine
+ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10
+ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
+PIDFile=%t/jadda-jadda.pid
+Type=forking
+
+[Install]
+WantedBy=multi-user.target default.target
+`
+
+	goodNewWithPodIDFiles := `# jadda-jadda.service
+# autogenerated by Podman CI
+
+[Unit]
+Description=Podman jadda-jadda.service
+Documentation=man:podman-generate-systemd(1)
+Wants=network.target
+After=network-online.target
+RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage
+
+[Service]
+Environment=PODMAN_SYSTEMD_UNIT=%n
+Restart=always
+TimeoutStopSec=70
+ExecStartPre=/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --pod-id-file %t/pod-foobar.pod-id-file -d awesome-image:latest podman run --cgroups=foo --conmon-pidfile=foo --cidfile=foo --pod-id-file /tmp/pod-foobar.pod-id-file alpine
+ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10
+ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
+PIDFile=%t/jadda-jadda.pid
+Type=forking
+
 [Install]
 WantedBy=multi-user.target default.target
 `
@@ -782,6 +832,47 @@ WantedBy=multi-user.target default.target
 			false,
 			false,
 		},
+		{"good with ID files",
+			containerInfo{
+				Executable:        "/usr/bin/podman",
+				ServiceName:       "jadda-jadda",
+				ContainerNameOrID: "jadda-jadda",
+				RestartPolicy:     "always",
+				PIDFile:           "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:       10,
+				PodmanVersion:     "CI",
+				GraphRoot:         "/var/lib/containers/storage",
+				RunRoot:           "/var/run/containers/storage",
+				CreateCommand:     []string{"I'll get stripped", "create", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "awesome-image:latest", "podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine"},
+				EnvVariable:       define.EnvVariable,
+			},
+			goodNewWithIDFiles,
+			true,
+			false,
+			false,
+		},
+		{"good with pod ID files",
+			containerInfo{
+				Executable:        "/usr/bin/podman",
+				ServiceName:       "jadda-jadda",
+				ContainerNameOrID: "jadda-jadda",
+				RestartPolicy:     "always",
+				PIDFile:           "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:       10,
+				PodmanVersion:     "CI",
+				GraphRoot:         "/var/lib/containers/storage",
+				RunRoot:           "/var/run/containers/storage",
+				CreateCommand:     []string{"I'll get stripped", "create", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "--pod", "test", "awesome-image:latest", "podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "--pod-id-file", "/tmp/pod-foobar.pod-id-file", "alpine"},
+				EnvVariable:       define.EnvVariable,
+				Pod: &podInfo{
+					PodIDFile: "%t/pod-foobar.pod-id-file",
+				},
+			},
+			goodNewWithPodIDFiles,
+			true,
+			false,
+			false,
+		},
 	}
 	for _, tt := range tests {
 		test := tt

pkg/systemd/generate/pods.go

@@ -279,16 +279,16 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions)
 			}
 			podRootArgs = info.CreateCommand[1 : podCreateIndex-1]
 			info.RootFlags = strings.Join(escapeSystemdArguments(podRootArgs), " ")
-			podCreateArgs = filterPodFlags(info.CreateCommand[podCreateIndex+1:])
+			podCreateArgs = filterPodFlags(info.CreateCommand[podCreateIndex+1:], 0)
 		}
 		// We're hard-coding the first five arguments and append the
 		// CreateCommand with a stripped command and subcommand.
 		startCommand := []string{info.Executable}
 		startCommand = append(startCommand, podRootArgs...)
 		startCommand = append(startCommand,
-			[]string{"pod", "create",
+			"pod", "create",
-				"--infra-conmon-pidfile", "{{{{.PIDFile}}}}",
+			"--infra-conmon-pidfile", "{{{{.PIDFile}}}}",
-				"--pod-id-file", "{{{{.PodIDFile}}}}"}...)
+			"--pod-id-file", "{{{{.PodIDFile}}}}")
 
 		// Presence check for certain flags/options.
 		fs := pflag.NewFlagSet("args", pflag.ContinueOnError)

pkg/systemd/generate/pods_test.go

@@ -320,6 +320,25 @@ WantedBy=multi-user.target default.target
 			false,
 			false,
 		},
+		{"pod --new with ID files",
+			podInfo{
+				Executable:       "/usr/bin/podman",
+				ServiceName:      "pod-123abc",
+				InfraNameOrID:    "jadda-jadda-infra",
+				RestartPolicy:    "on-failure",
+				PIDFile:          "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:      10,
+				PodmanVersion:    "CI",
+				GraphRoot:        "/var/lib/containers/storage",
+				RunRoot:          "/var/run/containers/storage",
+				RequiredServices: []string{"container-1", "container-2"},
+				CreateCommand:    []string{"podman", "pod", "create", "--infra-conmon-pidfile", "/tmp/pod-123abc.pid", "--pod-id-file", "/tmp/pod-123abc.pod-id", "--name", "foo", "bar=arg with space"},
+			},
+			podGoodNamedNew,
+			true,
+			false,
+			false,
+		},
 	}
 
 	for _, tt := range tests {