Skip to content

Commit

Permalink
podman generate systemd --new do not duplicate params
Browse files Browse the repository at this point in the history 
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 containers#9776

Signed-off-by: Paul Holzinger <[email protected]>
  • Loading branch information
@mheon
Paul Holzinger authored and mheon committed Mar 29, 2021
1 parent 1089f83 commit 1b349d7
Show file tree
Hide file tree
Showing 6 changed files with 290 additions and 32 deletions.
36 changes: 31 additions & 5 deletions pkg/systemd/generate/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
147 changes: 134 additions & 13 deletions pkg/systemd/generate/common_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,151 @@
package generate

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
)

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)
}
}

Expand Down
21 changes: 11 additions & 10 deletions pkg/systemd/generate/containers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -266,8 +269,6 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
return "", err
}

remainingCmd := info.CreateCommand[index:]

if !hasDetachParam {
// Enforce detaching
//
Expand Down
91 changes: 91 additions & 0 deletions pkg/systemd/generate/containers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
`
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions pkg/systemd/generate/pods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 1b349d7

Please sign in to comment.