Address review comments

Instead of using files in the ContainerEngine API for volume
import/export, expose readers/writers, so we can use os.Stdin and
os.Stdout - meaning that things will work on Windows, where
/dev/stdin and /dev/stdout don't exist.

Avoid the use of a temporary file on import by reading straight
from the request body.

Remove a utility function that doesn't actually do anything.

Signed-off-by: Matt Heon <[email protected]>

Author: mheon

cmd/podman/volumes/export.go

@@ -3,6 +3,8 @@ package volumes
 import (
 	"context"
 	"errors"
+	"fmt"
+	"os"
 
 	"github.com/containers/common/pkg/completion"
 	"github.com/containers/podman/v5/cmd/podman/common"
@@ -27,7 +29,7 @@ Allow content of volume to be exported into external tar.`
 )
 
 var (
-	cliExportOpts entities.VolumeExportOptions
+	targetPath string
 )
 
 func init() {
@@ -38,19 +40,32 @@ func init() {
 	flags := exportCommand.Flags()
 
 	outputFlagName := "output"
-	flags.StringVarP(&cliExportOpts.OutputPath, outputFlagName, "o", "/dev/stdout", "Write to a specified file (default: stdout, which must be redirected)")
+	flags.StringVarP(&targetPath, outputFlagName, "o", "", "Write to a specified file (default: stdout, which must be redirected)")
 	_ = exportCommand.RegisterFlagCompletionFunc(outputFlagName, completion.AutocompleteDefault)
 }
 
 func export(cmd *cobra.Command, args []string) error {
 	containerEngine := registry.ContainerEngine()
 	ctx := context.Background()
 
-	if cliExportOpts.OutputPath == "" {
-		return errors.New("expects output path, use --output=[path]")
+	if targetPath == "" && cmd.Flag("output").Changed {
+		return errors.New("must provide valid path for file to write to")
 	}
 
-	if err := containerEngine.VolumeExport(ctx, args[0], cliExportOpts); err != nil {
+	exportOpts := entities.VolumeExportOptions{}
+
+	if targetPath != "" {
+		targetFile, err := os.Create(targetPath)
+		if err != nil {
+			return fmt.Errorf("unable to create target file path %q: %w", targetPath, err)
+		}
+		defer targetFile.Close()
+		exportOpts.Output = targetFile
+	} else {
+		exportOpts.Output = os.Stdout
+	}
+
+	if err := containerEngine.VolumeExport(ctx, args[0], exportOpts); err != nil {
 		return err
 	}
 	return nil

cmd/podman/volumes/import.go

@@ -2,8 +2,11 @@ package volumes
 
 import (
 	"context"
+	"fmt"
+	"os"
 
 	"github.com/containers/podman/v5/cmd/podman/common"
+	"github.com/containers/podman/v5/cmd/podman/parse"
 	"github.com/containers/podman/v5/cmd/podman/registry"
 	"github.com/containers/podman/v5/pkg/domain/entities"
 	"github.com/spf13/cobra"
@@ -31,18 +34,27 @@ func init() {
 }
 
 func importVol(cmd *cobra.Command, args []string) error {
-	filePath := args[1]
-	if filePath == "-" {
-		filePath = "/dev/stdin"
+	opts := entities.VolumeImportOptions{}
+
+	filepath := args[1]
+	if filepath == "-" {
+		opts.Input = os.Stdin
+	} else {
+		if err := parse.ValidateFileName(filepath); err != nil {
+			return err
+		}
+
+		targetFile, err := os.Open(filepath)
+		if err != nil {
+			return fmt.Errorf("unable to create target file path %q: %w", filepath, err)
+		}
+		defer targetFile.Close()
+		opts.Input = targetFile
 	}
 
 	containerEngine := registry.ContainerEngine()
 	ctx := context.Background()
 
-	opts := entities.VolumeImportOptions{
-		InputPath: filePath,
-	}
-
 	if err := containerEngine.VolumeImport(ctx, args[0], opts); err != nil {
 		return err
 	}

libpod/volume.go

@@ -11,6 +11,7 @@ import (
 	"github.com/containers/podman/v5/libpod/lock"
 	"github.com/containers/podman/v5/libpod/plugin"
 	"github.com/containers/podman/v5/utils"
+	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/sirupsen/logrus"
 )
@@ -344,8 +345,8 @@ func (v *Volume) Import(r io.Reader) error {
 		}
 	}()
 
-	if err := utils.UntarToFileSystem(mountPoint, r, nil); err != nil {
-		return err
+	if err := archive.Untar(r, mountPoint, nil); err != nil {
+		return fmt.Errorf("extracting into volume %s: %w", v.Name(), err)
 	}
 
 	return nil

pkg/api/handlers/libpod/volumes.go

@@ -4,13 +4,10 @@ package libpod
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/url"
-	"os"
-
-	"errors"
 
 	"github.com/containers/podman/v5/libpod"
 	"github.com/containers/podman/v5/libpod/define"
@@ -248,22 +245,13 @@ func ImportVolume(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	tmpfile, err := os.CreateTemp("", "podman-volume-import-")
-	if err != nil {
-		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("creating temporary file: %w", err))
-		return
-	}
-	defer func() {
-		tmpfile.Close()
-		os.Remove(tmpfile.Name())
-	}()
-
-	if _, err := io.Copy(tmpfile, r.Body); err != nil {
-		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("copying archive to temporary file: %w", err))
+	if r.Body == nil {
+		utils.Error(w, http.StatusInternalServerError, errors.New("must provide tar file to import in request body"))
 		return
 	}
+	defer r.Body.Close()
 
-	if err := vol.Import(tmpfile); err != nil {
+	if err := vol.Import(r.Body); err != nil {
 		utils.Error(w, http.StatusInternalServerError, err)
 		return
 	}

pkg/api/server/register_volumes.go

@@ -161,19 +161,19 @@ func (s *APIServer) registerVolumeHandlers(r *mux.Router) error {
 	//    description: the name or ID of the volume
 	// produces:
 	// - application/x-tar
-        // responses:
-        //   200:
+	// responses:
+	//   200:
 	//     description: no error
 	//     schema:
 	//      type: string
-        //      format: binary
+	//      format: binary
 	//   404:
 	//     $ref: "#/responses/volumeNotFound"
 	//   500:
 	//     $ref: "#/responses/internalError"
 	r.Handle(VersionedPath("/libpod/volumes/{name}/export"), s.APIHandler(libpod.ExportVolume)).Methods(http.MethodGet)
 
-	// swagger:operation PUT /libpod/volumes/{name}/import libpod VolumeImportLibpod
+	// swagger:operation POST /libpod/volumes/{name}/import libpod VolumeImportLibpod
 	// ---
 	// tags:
 	//  - volumes
@@ -200,7 +200,7 @@ func (s *APIServer) registerVolumeHandlers(r *mux.Router) error {
 	//     $ref: "#/responses/volumeNotFound"
 	//   500:
 	//     $ref: "#/responses/internalError"
-	r.Handle(VersionedPath("/libpod/volumes/{name}/import"), s.APIHandler(libpod.ImportVolume)).Methods(http.MethodPut)
+	r.Handle(VersionedPath("/libpod/volumes/{name}/import"), s.APIHandler(libpod.ImportVolume)).Methods(http.MethodPost)
 
 	/*
 	 * Docker compatibility endpoints

pkg/bindings/volumes/volumes.go

@@ -2,15 +2,12 @@ package volumes
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 	"strings"
 
 	"github.com/containers/podman/v5/pkg/bindings"
-	"github.com/containers/podman/v5/pkg/domain/entities"
 	"github.com/containers/podman/v5/pkg/domain/entities/reports"
 	entitiesTypes "github.com/containers/podman/v5/pkg/domain/entities/types"
 	jsoniter "github.com/json-iterator/go"
@@ -146,17 +143,7 @@ func Exists(ctx context.Context, nameOrID string, options *ExistsOptions) (bool,
 }
 
 // Export exports a volume to the given path
-func Export(ctx context.Context, nameOrID string, options entities.VolumeExportOptions) error {
-	if options.OutputPath == "" {
-		return errors.New("must provide valid path for file to write to")
-	}
-
-	targetFile, err := os.Create(options.OutputPath)
-	if err != nil {
-		return fmt.Errorf("unable to create target file path %q: %w", options.OutputPath, err)
-	}
-	defer targetFile.Close()
-
+func Export(ctx context.Context, nameOrID string, exportTo io.Writer) error {
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
 		return err
@@ -168,31 +155,21 @@ func Export(ctx context.Context, nameOrID string, options entities.VolumeExportO
 	defer response.Body.Close()
 
 	if response.IsSuccess() || response.IsRedirection() {
-		if _, err := io.Copy(targetFile, response.Body); err != nil {
+		if _, err := io.Copy(exportTo, response.Body); err != nil {
 			return fmt.Errorf("writing volume %s contents to file: %w", nameOrID, err)
 		}
 	}
 	return response.Process(nil)
 }
 
 // Import imports the given tar into the given volume
-func Import(ctx context.Context, nameOrID string, options entities.VolumeImportOptions) error {
-	if options.InputPath == "" {
-		return errors.New("must provide valid path for file to write to")
-	}
-
-	targetFile, err := os.Create(options.InputPath)
-	if err != nil {
-		return fmt.Errorf("unable to create target file path %q: %w", options.InputPath, err)
-	}
-	defer targetFile.Close()
-
+func Import(ctx context.Context, nameOrID string, importFrom io.Reader) error {
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
 		return err
 	}
 
-	response, err := conn.DoRequest(ctx, targetFile, http.MethodPut, "/volumes/%s/import", nil, nil, nameOrID)
+	response, err := conn.DoRequest(ctx, importFrom, http.MethodPost, "/volumes/%s/import", nil, nil, nameOrID)
 	if err != nil {
 		return err
 	}

pkg/domain/entities/volumes.go

@@ -1,6 +1,7 @@
 package entities
 
 import (
+	"io"
 	"net/url"
 
 	"github.com/containers/podman/v5/pkg/domain/entities/types"
@@ -45,10 +46,11 @@ type VolumeUnmountReport = types.VolumeUnmountReport
 
 // VolumeExportOptions describes the options required to export a volume.
 type VolumeExportOptions struct {
-	OutputPath string
+	Output io.Writer
 }
 
 // VolumeImportOptions describes the options required to import a volume
 type VolumeImportOptions struct {
-	InputPath string
+	// Input will be closed upon being fully consumed
+	Input io.Reader
 }

pkg/domain/infra/abi/play.go

@@ -38,7 +38,7 @@ import (
 	"github.com/containers/podman/v5/pkg/specgenutil"
 	"github.com/containers/podman/v5/pkg/systemd/notifyproxy"
 	"github.com/containers/podman/v5/pkg/util"
-	"github.com/containers/podman/v5/utils"
+	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/fileutils"
 	"github.com/coreos/go-systemd/v22/daemon"
 	"github.com/opencontainers/go-digest"
@@ -1504,7 +1504,7 @@ func (ic *ContainerEngine) importVolume(ctx context.Context, vol *libpod.Volume,
 	}
 
 	// dont care if volume is mounted or not we are gonna import everything to mountPoint
-	return utils.UntarToFileSystem(mountPoint, tarFile, nil)
+	return archive.Untar(tarFile, mountPoint, nil)
 }
 
 // readConfigMapFromFile returns a kubernetes configMap obtained from --configmap flag

pkg/domain/infra/abi/volumes.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 
 	"github.com/containers/podman/v5/libpod"
 	"github.com/containers/podman/v5/libpod/define"
@@ -243,16 +242,6 @@ func (ic *ContainerEngine) VolumeReload(ctx context.Context) (*entities.VolumeRe
 }
 
 func (ic *ContainerEngine) VolumeExport(ctx context.Context, nameOrID string, options entities.VolumeExportOptions) error {
-	if options.OutputPath == "" {
-		return errors.New("must provide valid path for file to write to")
-	}
-
-	targetFile, err := os.Create(options.OutputPath)
-	if err != nil {
-		return fmt.Errorf("unable to create target file path %q: %w", options.OutputPath, err)
-	}
-	defer targetFile.Close()
-
 	vol, err := ic.Libpod.GetVolume(nameOrID)
 	if err != nil {
 		return err
@@ -264,30 +253,20 @@ func (ic *ContainerEngine) VolumeExport(ctx context.Context, nameOrID string, op
 	}
 	defer contents.Close()
 
-	if _, err := io.Copy(targetFile, contents); err != nil {
-		return fmt.Errorf("writing volume %s to file: %w", vol.Name(), err)
+	if _, err := io.Copy(options.Output, contents); err != nil {
+		return fmt.Errorf("writing volume %s contents: %w", vol.Name(), err)
 	}
 
 	return nil
 }
 
 func (ic *ContainerEngine) VolumeImport(ctx context.Context, nameOrID string, options entities.VolumeImportOptions) error {
-	if options.InputPath == "" {
-		return errors.New("must provide valid path to import from")
-	}
-
-	inputFile, err := os.Open(options.InputPath)
-	if err != nil {
-		return fmt.Errorf("opening import file %q: %w", options.InputPath, err)
-	}
-	defer inputFile.Close()
-
 	vol, err := ic.Libpod.GetVolume(nameOrID)
 	if err != nil {
 		return err
 	}
 
-	if err := vol.Import(inputFile); err != nil {
+	if err := vol.Import(options.Input); err != nil {
 		return err
 	}
 

pkg/domain/infra/tunnel/volumes.go

@@ -115,9 +115,9 @@ func (ic *ContainerEngine) VolumeReload(ctx context.Context) (*entities.VolumeRe
 }
 
 func (ic *ContainerEngine) VolumeExport(ctx context.Context, nameOrID string, options entities.VolumeExportOptions) error {
-	return volumes.Export(ic.ClientCtx, nameOrID, options)
+	return volumes.Export(ic.ClientCtx, nameOrID, options.Output)
 }
 
 func (ic *ContainerEngine) VolumeImport(ctx context.Context, nameOrID string, options entities.VolumeImportOptions) error {
-	return volumes.Import(ic.ClientCtx, nameOrID, options)
+	return volumes.Import(ic.ClientCtx, nameOrID, options.Input)
 }