Quadlet container mount - support non key=val options

Some keys, e.g. ro do not have values. The current implementation crashed looking for the = sign Externalize findMountType in a new package Parse mount command using FindMountType Rebuild parameter string using csv Add test case and adjust the test framework Signed-off-by: Ygal Blum <[email protected]>
containers · Sep 27, 2023 · f0eb456 · f0eb456
1 parent a018fe7
commit f0eb456
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 70 deletions.
diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go
@@ -1,7 +1,6 @@
 package specgenutil
 
 import (
-	"encoding/csv"
 	"errors"
 	"fmt"
 	"path"
@@ -13,14 +12,14 @@ import (
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/rootless"
 	"github.com/containers/podman/v4/pkg/specgen"
+	"github.com/containers/podman/v4/pkg/specgenutilexternal"
 	"github.com/containers/podman/v4/pkg/util"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 var (
-	errOptionArg     = errors.New("must provide an argument for option")
-	errNoDest        = errors.New("must set volume destination")
-	errInvalidSyntax = errors.New("incorrect mount format: should be --mount type=<bind|glob|tmpfs|volume>,[src=<host-dir|volume-name>,]target=<ctr-dir>[,options]")
+	errOptionArg = errors.New("must provide an argument for option")
+	errNoDest    = errors.New("must set volume destination")
 )
 
 // Parse all volume-related options in the create config into a set of mounts
@@ -140,35 +139,6 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string)
 	return finalMounts, finalVolumes, finalOverlayVolume, finalImageVolumes, nil
 }
 
-// findMountType parses the input and extracts the type of the mount type and
-// the remaining non-type tokens.
-func findMountType(input string) (mountType string, tokens []string, err error) {
-	// Split by comma, iterate over the slice and look for
-	// "type=$mountType". Everything else is appended to tokens.
-	found := false
-	csvReader := csv.NewReader(strings.NewReader(input))
-	records, err := csvReader.ReadAll()
-	if err != nil {
-		return "", nil, err
-	}
-	if len(records) != 1 {
-		return "", nil, errInvalidSyntax
-	}
-	for _, s := range records[0] {
-		kv := strings.Split(s, "=")
-		if found || !(len(kv) == 2 && kv[0] == "type") {
-			tokens = append(tokens, s)
-			continue
-		}
-		mountType = kv[1]
-		found = true
-	}
-	if !found {
-		err = errInvalidSyntax
-	}
-	return
-}
-
 // Mounts takes user-provided input from the --mount flag as well as Mounts
 // specified in containers.conf and creates OCI spec mounts and Libpod named volumes.
 // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ...
@@ -181,7 +151,7 @@ func Mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, m
 	parseMounts := func(mounts []string, ignoreDup bool) error {
 		for _, mount := range mounts {
 			// TODO: Docker defaults to "volume" if no mount type is specified.
-			mountType, tokens, err := findMountType(mount)
+			mountType, tokens, err := specgenutilexternal.FindMountType(mount)
 			if err != nil {
 				return err
 			}

diff --git a/pkg/specgenutilexternal/mount.go b/pkg/specgenutilexternal/mount.go
@@ -0,0 +1,40 @@
+package specgenutilexternal
+
+import (
+	"encoding/csv"
+	"errors"
+	"strings"
+)
+
+var (
+	errInvalidSyntax = errors.New("incorrect mount format: should be --mount type=<bind|glob|tmpfs|volume>,[src=<host-dir|volume-name>,]target=<ctr-dir>[,options]")
+)
+
+// FindMountType parses the input and extracts the type of the mount type and
+// the remaining non-type tokens.
+func FindMountType(input string) (mountType string, tokens []string, err error) {
+	// Split by comma, iterate over the slice and look for
+	// "type=$mountType". Everything else is appended to tokens.
+	found := false
+	csvReader := csv.NewReader(strings.NewReader(input))
+	records, err := csvReader.ReadAll()
+	if err != nil {
+		return "", nil, err
+	}
+	if len(records) != 1 {
+		return "", nil, errInvalidSyntax
+	}
+	for _, s := range records[0] {
+		kv := strings.Split(s, "=")
+		if found || !(len(kv) == 2 && kv[0] == "type") {
+			tokens = append(tokens, s)
+			continue
+		}
+		mountType = kv[1]
+		found = true
+	}
+	if !found {
+		err = errInvalidSyntax
+	}
+	return
+}
diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go
@@ -1,12 +1,15 @@
 package quadlet
 
 import (
+	"bytes"
+	"encoding/csv"
 	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 
+	"github.com/containers/podman/v4/pkg/specgenutilexternal"
 	"github.com/containers/podman/v4/pkg/systemd/parser"
 	"github.com/containers/storage/pkg/regexp"
 )
@@ -729,33 +732,10 @@ func ConvertContainer(container *parser.UnitFile, names map[string]string, isUse
 
 	mounts := container.LookupAllArgs(ContainerGroup, KeyMount)
 	for _, mount := range mounts {
-		params := strings.Split(mount, ",")
-		paramsMap := make(map[string]string, len(params))
-		for _, param := range params {
-			kv := strings.Split(param, "=")
-			paramsMap[kv[0]] = kv[1]
-		}
-		if paramType, ok := paramsMap["type"]; ok {
-			if paramType == "volume" || paramType == "bind" || paramType == "glob" {
-				var err error
-				if paramSource, ok := paramsMap["source"]; ok {
-					paramsMap["source"], err = handleStorageSource(container, service, paramSource, names)
-				} else if paramSource, ok = paramsMap["src"]; ok {
-					paramsMap["src"], err = handleStorageSource(container, service, paramSource, names)
-				}
-				if err != nil {
-					return nil, err
-				}
-			}
-		}
-		paramsArray := make([]string, 0, len(params))
-		paramsArray = append(paramsArray, fmt.Sprintf("%s=%s", "type", paramsMap["type"]))
-		for k, v := range paramsMap {
-			if k != "type" {
-				paramsArray = append(paramsArray, fmt.Sprintf("%s=%s", k, v))
-			}
+		mountStr, err := resolveContainerMountParams(container, service, mount, names)
+		if err != nil {
+			return nil, err
 		}
-		mountStr := strings.Join(paramsArray, ",")
 		podman.add("--mount", mountStr)
 	}
 
@@ -1538,3 +1518,56 @@ func handleImageSource(quadletImageName string, serviceUnitFile *parser.UnitFile
 
 	return quadletImageName, nil
 }
+
+func resolveContainerMountParams(containerUnitFile, serviceUnitFile *parser.UnitFile, mount string, names map[string]string) (string, error) {
+	mountType, tokens, err := specgenutilexternal.FindMountType(mount)
+	if err != nil {
+		return "", err
+	}
+
+	// Source resolution is required only for these types of mounts
+	if !(mountType == "volume" || mountType == "bind" || mountType == "glob") {
+		return mount, nil
+	}
+
+	sourceIndex := -1
+	originalSource := ""
+	for i, token := range tokens {
+		kv := strings.SplitN(token, "=", 2)
+		if kv[0] == "source" || kv[0] == "src" {
+			if len(kv) < 2 {
+				return "", fmt.Errorf("source parameter does not include a value")
+			}
+			sourceIndex = i
+			originalSource = kv[1]
+		}
+	}
+
+	resolvedSource, err := handleStorageSource(containerUnitFile, serviceUnitFile, originalSource, names)
+	if err != nil {
+		return "", err
+	}
+	tokens[sourceIndex] = fmt.Sprintf("source=%s", resolvedSource)
+
+	tokens = append([]string{fmt.Sprintf("type=%s", mountType)}, tokens...)
+
+	return convertToCSV(tokens)
+}
+
+func convertToCSV(s []string) (string, error) {
+	var buf bytes.Buffer
+	writer := csv.NewWriter(&buf)
+
+	err := writer.Write(s)
+	if err != nil {
+		return "", err
+	}
+	writer.Flush()
+
+	ret := buf.String()
+	if ret[len(ret)-1] == '\n' {
+		ret = ret[:len(ret)-1]
+	}
+
+	return ret, nil
+}
diff --git a/test/e2e/quadlet/mount.container b/test/e2e/quadlet/mount.container
@@ -2,9 +2,9 @@
 Image=localhost/imagename
 ## assert-podman-args-key-val "--mount" "," "type=bind,source=/path/on/host,destination=/path/in/container"
 Mount=type=bind,source=/path/on/host,destination=/path/in/container
-## assert-podman-args-key-val "--mount" "," "type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared"
+## assert-podman-args-key-val "--mount" "," "type=bind,source=/path/on/host,dst=/path/in/container,relabel=shared"
 Mount=type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared
-## assert-podman-args-key-val "--mount" "," "type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared,U=true"
+## assert-podman-args-key-val "--mount" "," "type=bind,source=/path/on/host,dst=/path/in/container,relabel=shared,U=true"
 Mount=type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared,U=true
 ## assert-podman-args-key-val "--mount" "," "type=volume,source=vol1,destination=/path/in/container,ro=true"
 Mount=type=volume,source=vol1,destination=/path/in/container,ro=true
@@ -20,3 +20,7 @@ Mount=type=image,source=fedora,destination=/fedora-image,rw=true
 Mount=type=devpts,destination=/dev/pts
 ## assert-podman-args-key-val-regex "--mount" "," "type=bind,source=.*/podman_test.*/quadlet/path/on/host,destination=/path/in/container"
 Mount=type=bind,source=./path/on/host,destination=/path/in/container
+## assert-podman-args-key-val "--mount" "," "type=volume,source=vol1,destination=/path/in/container,ro"
+Mount=type=volume,source=vol1,destination=/path/in/container,ro
+## assert-podman-args-key-val "--mount" "," "type=bind,source=/tmp,\"dst=/path,1\""
+Mount=type=bind,src=/tmp,\"dst=/path,1\"
diff --git a/test/e2e/quadlet_test.go b/test/e2e/quadlet_test.go
@@ -1,6 +1,7 @@
 package integration
 
 import (
+	"encoding/csv"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -177,15 +178,24 @@ func (t *quadletTestcase) assertPodmanArgsRegex(args []string, unit *parser.Unit
 	return findSublistRegex(podmanArgs, args) != -1
 }
 
-func keyValueStringToMap(keyValueString, separator string) map[string]string {
+func keyValueStringToMap(keyValueString, separator string) (map[string]string, error) {
 	keyValMap := make(map[string]string)
-	keyVarList := strings.Split(keyValueString, separator)
-	for _, param := range keyVarList {
-		kv := strings.Split(param, "=")
-		keyValMap[kv[0]] = kv[1]
+	csvReader := csv.NewReader(strings.NewReader(keyValueString))
+	csvReader.Comma = []rune(separator)[0]
+	keyVarList, err := csvReader.ReadAll()
+	if err != nil {
+		return nil, err
+	}
+	for _, param := range keyVarList[0] {
+		val := ""
+		kv := strings.SplitN(param, "=", 2)
+		if len(kv) == 2 {
+			val = kv[1]
+		}
+		keyValMap[kv[0]] = val
 	}
 
-	return keyValMap
+	return keyValMap, nil
 }
 
 func keyValMapEqualRegex(expectedKeyValMap, actualKeyValMap map[string]string) bool {
@@ -208,7 +218,10 @@ func keyValMapEqualRegex(expectedKeyValMap, actualKeyValMap map[string]string) b
 func (t *quadletTestcase) assertPodmanArgsKeyVal(args []string, unit *parser.UnitFile, key string, allowRegex bool) bool {
 	podmanArgs, _ := unit.LookupLastArgs("Service", key)
 
-	expectedKeyValMap := keyValueStringToMap(args[2], args[1])
+	expectedKeyValMap, err := keyValueStringToMap(args[2], args[1])
+	if err != nil {
+		return false
+	}
 	argKeyLocation := 0
 	for {
 		subListLocation := findSublist(podmanArgs[argKeyLocation:], []string{args[0]})
@@ -217,7 +230,10 @@ func (t *quadletTestcase) assertPodmanArgsKeyVal(args []string, unit *parser.Uni
 		}
 
 		argKeyLocation += subListLocation
-		actualKeyValMap := keyValueStringToMap(podmanArgs[argKeyLocation+1], args[1])
+		actualKeyValMap, err := keyValueStringToMap(podmanArgs[argKeyLocation+1], args[1])
+		if err != nil {
+			break
+		}
 		if allowRegex {
 			if keyValMapEqualRegex(expectedKeyValMap, actualKeyValMap) {
 				return true