From dd428af89837c813d504b62b8f44b4251ffd1956 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 20 Dec 2022 10:35:13 +0100 Subject: [PATCH 1/2] quadlet: Rename parser.LookupBoolean to LookupBooleanWithDefault We add a regular LookupBoolean that can fail lookups, which is used by the WithDefault version. We want to use this directly later in some places. It is fina to change API here because this has not been in a release yet. Signed-off-by: Alexander Larsson --- pkg/systemd/parser/unitfile.go | 16 +++++++++++++--- pkg/systemd/quadlet/quadlet.go | 16 ++++++++-------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/pkg/systemd/parser/unitfile.go b/pkg/systemd/parser/unitfile.go index 09a004b84aa7..56fa1888fb43 100644 --- a/pkg/systemd/parser/unitfile.go +++ b/pkg/systemd/parser/unitfile.go @@ -615,16 +615,26 @@ func (f *UnitFile) Lookup(groupName string, key string) (string, bool) { } // Lookup the last instance of a key and convert the value to a bool -func (f *UnitFile) LookupBoolean(groupName string, key string, defaultValue bool) bool { +func (f *UnitFile) LookupBoolean(groupName string, key string) (bool, bool) { v, ok := f.Lookup(groupName, key) if !ok { - return defaultValue + return false, false } return strings.EqualFold(v, "1") || strings.EqualFold(v, "yes") || strings.EqualFold(v, "true") || - strings.EqualFold(v, "on") + strings.EqualFold(v, "on"), true +} + +// Lookup the last instance of a key and convert the value to a bool +func (f *UnitFile) LookupBooleanWithDefault(groupName string, key string, defaultValue bool) bool { + v, ok := f.LookupBoolean(groupName, key) + if !ok { + return defaultValue + } + + return v } /* Mimics strol, which is what systemd uses */ diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go index 6c473df25333..e78f604719c7 100644 --- a/pkg/systemd/quadlet/quadlet.go +++ b/pkg/systemd/quadlet/quadlet.go @@ -295,13 +295,13 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile addNetworks(container, ContainerGroup, service, podman) // Run with a pid1 init to reap zombies by default (as most apps don't do that) - runInit := container.LookupBoolean(ContainerGroup, KeyRunInit, false) + runInit := container.LookupBooleanWithDefault(ContainerGroup, KeyRunInit, false) if runInit { podman.add("--init") } // By default we handle startup notification with conmon, but allow passing it to the container with Notify=yes - notify := container.LookupBoolean(ContainerGroup, KeyNotify, false) + notify := container.LookupBooleanWithDefault(ContainerGroup, KeyNotify, false) if notify { podman.add("--sdnotify=container") } else { @@ -316,7 +316,7 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile } // Default to no higher level privileges or caps - noNewPrivileges := container.LookupBoolean(ContainerGroup, KeyNoNewPrivileges, false) + noNewPrivileges := container.LookupBooleanWithDefault(ContainerGroup, KeyNoNewPrivileges, false) if noNewPrivileges { podman.add("--security-opt=no-new-privileges") } @@ -345,12 +345,12 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile podman.addf("--cap-add=%s", strings.ToLower(caps)) } - readOnly := container.LookupBoolean(ContainerGroup, KeyReadOnly, false) + readOnly := container.LookupBooleanWithDefault(ContainerGroup, KeyReadOnly, false) if readOnly { podman.add("--read-only") } - volatileTmp := container.LookupBoolean(ContainerGroup, KeyVolatileTmp, false) + volatileTmp := container.LookupBooleanWithDefault(ContainerGroup, KeyVolatileTmp, false) if volatileTmp { /* Read only mode already has a tmpfs by default */ if !readOnly { @@ -537,7 +537,7 @@ func ConvertNetwork(network *parser.UnitFile, name string) (*parser.UnitFile, er podman := NewPodmanCmdline("network", "create", "--ignore") - if disableDNS := network.LookupBoolean(NetworkGroup, KeyNetworkDisableDNS, false); disableDNS { + if disableDNS := network.LookupBooleanWithDefault(NetworkGroup, KeyNetworkDisableDNS, false); disableDNS { podman.add("--disable-dns") } @@ -569,7 +569,7 @@ func ConvertNetwork(network *parser.UnitFile, name string) (*parser.UnitFile, er return nil, fmt.Errorf("cannot set gateway or range without subnet") } - if internal := network.LookupBoolean(NetworkGroup, KeyNetworkInternal, false); internal { + if internal := network.LookupBooleanWithDefault(NetworkGroup, KeyNetworkInternal, false); internal { podman.add("--internal") } @@ -577,7 +577,7 @@ func ConvertNetwork(network *parser.UnitFile, name string) (*parser.UnitFile, er podman.addf("--ipam-driver=%s", ipamDriver) } - if ipv6 := network.LookupBoolean(NetworkGroup, KeyNetworkIPv6, false); ipv6 { + if ipv6 := network.LookupBooleanWithDefault(NetworkGroup, KeyNetworkIPv6, false); ipv6 { podman.add("--ipv6") } From 0cf36684c67e97da6ebe6ca57f56ab0b909ae976 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 20 Dec 2022 10:56:48 +0100 Subject: [PATCH 2/2] quadlet: Handle booleans that have defaults better The ReadOnly and the RunInit keys affect options that have a variable default (configurable in containers.conf). This means we need to handle them a bit differently in quadlet to allow overriding the default. For example, we can't assume ReadOnly=false doesn't need to add any argument because no argument may mean readonly=true if the default is changed. We now don't add any argument (leaving the default) if the key is not specified, or we always add an argument (--foo or --foo=false) if the key is specified (overriding whatever the default is). Signed-off-by: Alexander Larsson --- pkg/systemd/quadlet/podmancmdline.go | 8 ++++++++ pkg/systemd/quadlet/quadlet.go | 12 ++++++------ test/e2e/quadlet/basepodman.container | 2 -- test/e2e/quadlet/readwrite-notmpfs.container | 1 + test/e2e/quadlet/readwrite.container | 1 + 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/systemd/quadlet/podmancmdline.go b/pkg/systemd/quadlet/podmancmdline.go index a1e70e0dbf40..0983923d0136 100644 --- a/pkg/systemd/quadlet/podmancmdline.go +++ b/pkg/systemd/quadlet/podmancmdline.go @@ -57,6 +57,14 @@ func (c *PodmanCmdline) addAnnotations(annotations map[string]string) { c.addKeys("--annotation", annotations) } +func (c *PodmanCmdline) addBool(arg string, val bool) { + if val { + c.add(arg) + } else { + c.addf("%s=false", arg) + } +} + func NewPodmanCmdline(args ...string) *PodmanCmdline { c := &PodmanCmdline{ Args: make([]string, 0), diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go index e78f604719c7..611e495085f8 100644 --- a/pkg/systemd/quadlet/quadlet.go +++ b/pkg/systemd/quadlet/quadlet.go @@ -295,9 +295,9 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile addNetworks(container, ContainerGroup, service, podman) // Run with a pid1 init to reap zombies by default (as most apps don't do that) - runInit := container.LookupBooleanWithDefault(ContainerGroup, KeyRunInit, false) - if runInit { - podman.add("--init") + runInit, ok := container.LookupBoolean(ContainerGroup, KeyRunInit) + if ok { + podman.addBool("--init", runInit) } // By default we handle startup notification with conmon, but allow passing it to the container with Notify=yes @@ -345,9 +345,9 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile podman.addf("--cap-add=%s", strings.ToLower(caps)) } - readOnly := container.LookupBooleanWithDefault(ContainerGroup, KeyReadOnly, false) - if readOnly { - podman.add("--read-only") + readOnly, ok := container.LookupBoolean(ContainerGroup, KeyReadOnly) + if ok { + podman.addBool("--read-only", readOnly) } volatileTmp := container.LookupBooleanWithDefault(ContainerGroup, KeyVolatileTmp, false) diff --git a/test/e2e/quadlet/basepodman.container b/test/e2e/quadlet/basepodman.container index 6c6de8abb51a..88df7344a792 100644 --- a/test/e2e/quadlet/basepodman.container +++ b/test/e2e/quadlet/basepodman.container @@ -4,9 +4,7 @@ Image=localhost/imagename # Disable all default features to get as empty podman run command as we can -ReadOnly=no NoNewPrivileges=no DropCapability= -RunInit=no VolatileTmp=no Timezone= diff --git a/test/e2e/quadlet/readwrite-notmpfs.container b/test/e2e/quadlet/readwrite-notmpfs.container index c7349a8ce0e2..8b15632e1d23 100644 --- a/test/e2e/quadlet/readwrite-notmpfs.container +++ b/test/e2e/quadlet/readwrite-notmpfs.container @@ -1,3 +1,4 @@ +## assert-podman-args "--read-only=false" ## !assert-podman-args "--read-only" ## !assert-podman-args "--tmpfs" "/tmp:rw,size=512M,mode=1777" diff --git a/test/e2e/quadlet/readwrite.container b/test/e2e/quadlet/readwrite.container index 5265d30d182c..ef491eec6cac 100644 --- a/test/e2e/quadlet/readwrite.container +++ b/test/e2e/quadlet/readwrite.container @@ -1,4 +1,5 @@ ## !assert-podman-args "--read-only" +## assert-podman-args "--read-only=false" ## assert-podman-args "--tmpfs" "/tmp:rw,size=512M,mode=1777" [Container]