Merge pull request #9848 from Luap99/fix-9776

podman generate systemd --new do not duplicate params

6 files changed, 290 insertions, 32 deletions

diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go
index 19d468403..eafd45528 100644
--- a/pkg/systemd/generate/common.go
+++ b/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.
-func filterPodFlags(command []string) []string {
+// filterPodFlags removes --pod, --pod-id-file and --infra-conmon-pidfile from the specified command.
+// 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
 }
 
diff --git a/pkg/systemd/generate/common_test.go b/pkg/systemd/generate/common_test.go
index 3787e461e..30e758127 100644
--- a/pkg/systemd/generate/common_test.go
+++ b/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", "--pod-id-file", "foo"}},
-		{[]string{"podman", "pod", "create", "--pod-id-file=foo"}},
-		{[]string{"podman", "run", "--pod", "foo"}},
-		{[]string{"podman", "run", "--pod=foo"}},
+		{
+			[]string{"podman", "pod", "create"},
+			[]string{"podman", "pod", "create"},
+			0,
+		},
+		{
+			[]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, test.argCount)
+		assert.Equal(t, test.output, processed)
+	}
+}
+
+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 := filterPodFlags(test.input)
-		for _, s := range processed {
-			assert.False(t, strings.HasPrefix(s, "--pod-id-file"))
-			assert.False(t, strings.HasPrefix(s, "--pod"))
-		}
+		processed := filterCommonContainerFlags(test.input, test.argCount)
+		assert.Equal(t, test.output, processed)
 	}
 }
 
diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go
index bc13a6116..e06655a8d 100644
--- a/pkg/systemd/generate/containers.go
+++ b/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
-		// --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)
-		}
+		remainingCmd := info.CreateCommand[index:]
 
 		// 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
 			//
diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go
index 1359c1a37..899ba6bfa 100644
--- a/pkg/systemd/generate/containers_test.go
+++ b/pkg/systemd/generate/containers_test.go
@@ -395,6 +395,56 @@ 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
+`
 	tests := []struct {
 		name     string
 		info     containerInfo
@@ -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
diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go
index a76979ecf..1b92649e8 100644
--- a/pkg/systemd/generate/pods.go
+++ b/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",
-				"--infra-conmon-pidfile", "{{{{.PIDFile}}}}",
-				"--pod-id-file", "{{{{.PodIDFile}}}}"}...)
+			"pod", "create",
+			"--infra-conmon-pidfile", "{{{{.PIDFile}}}}",
+			"--pod-id-file", "{{{{.PodIDFile}}}}")
 
 		// Presence check for certain flags/options.
 		fs := pflag.NewFlagSet("args", pflag.ContinueOnError)
diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go
index 559f7365f..0e4d92c50 100644
--- a/pkg/systemd/generate/pods_test.go
+++ b/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 {