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 09a004b84a..56fa1888fb 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 6c473df253..e78f604719 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 a1e70e0dbf..0983923d01 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 e78f604719..611e495085 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 6c6de8abb5..88df7344a7 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 c7349a8ce0..8b15632e1d 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 5265d30d18..ef491eec6c 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]