From f0eb45667822b7942f4f9bc82ea29e97cbda9ea7 Mon Sep 17 00:00:00 2001 From: Ygal Blum Date: Tue, 26 Sep 2023 12:33:50 +0300 Subject: [PATCH] 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 --- pkg/specgenutil/volumes.go | 38 ++------------ pkg/specgenutilexternal/mount.go | 40 +++++++++++++++ pkg/systemd/quadlet/quadlet.go | 85 ++++++++++++++++++++++---------- test/e2e/quadlet/mount.container | 8 ++- test/e2e/quadlet_test.go | 32 +++++++++--- 5 files changed, 133 insertions(+), 70 deletions(-) create mode 100644 pkg/specgenutilexternal/mount.go diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 06ad6b4f17..b9a85f18b4 100644 --- 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=,[src=,]target=[,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 new file mode 100644 index 0000000000..0883b2a99a --- /dev/null +++ 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=,[src=,]target=[,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 index 37f8b07780..31e863baaa 100644 --- 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 index dae7be9f2c..aaf1498123 100644 --- 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 index ed39474939..45fcecf2d4 100644 --- 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