Merge pull request #27426 from Honny1/local-api-artifact-add

Artifact add optimization on macOS and Windows

internal/localapi/types.go

@@ -1,7 +1,11 @@
 package localapi
 
+import "errors"
+
 // LocalAPIMap is a map of local paths to their target paths in the VM
 type LocalAPIMap struct {
 	ClientPath string `json:"ClientPath,omitempty"`
 	RemotePath string `json:"RemotePath,omitempty"`
 }
+
+var ErrPathNotAbsolute = errors.New("path is not absolute")

internal/localapi/utils.go

@@ -237,29 +237,41 @@ func CheckIfImageBuildPathsOnRunningMachine(ctx context.Context, containerFiles
 	return translatedContainerFiles, options, true
 }
 
-// IsHyperVProvider checks if the current machine provider is Hyper-V.
-// It returns true if the provider is Hyper-V, false otherwise, or an error if the check fails.
-func IsHyperVProvider(ctx context.Context) (bool, error) {
+func getVmProviderType(ctx context.Context) (define.VMType, error) {
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
 		logrus.Debugf("Failed to get client connection: %v", err)
-		return false, err
+		return define.UnknownVirt, err
 	}
 
 	_, vmProvider, err := FindMachineByPort(conn.URI.String(), conn.URI)
 	if err != nil {
 		logrus.Debugf("Failed to get machine hypervisor type: %v", err)
-		return false, err
+		return define.UnknownVirt, err
 	}
 
-	return vmProvider.VMType() == define.HyperVVirt, nil
+	return vmProvider.VMType(), nil
+}
+
+// IsHyperVProvider checks if the current machine provider is Hyper-V.
+// It returns true if the provider is Hyper-V, false otherwise, or an error if the check fails.
+func IsHyperVProvider(ctx context.Context) (bool, error) {
+	providerType, err := getVmProviderType(ctx)
+	return providerType == define.HyperVVirt, err
+}
+
+// IsWSLProvider checks if the current machine provider is WSL.
+// It returns true if the provider is WSL, false otherwise, or an error if the check fails.
+func IsWSLProvider(ctx context.Context) (bool, error) {
+	providerType, err := getVmProviderType(ctx)
+	return providerType == define.WSLVirt, err
 }
 
 // ValidatePathForLocalAPI checks if the provided path satisfies requirements for local API usage.
 // It returns an error if the path is not absolute or does not exist on the filesystem.
 func ValidatePathForLocalAPI(path string) error {
 	if !filepath.IsAbs(path) {
-		return fmt.Errorf("path %q is not absolute", path)
+		return fmt.Errorf("%w: %q", ErrPathNotAbsolute, path)
 	}
 
 	if err := fileutils.Exists(path); err != nil {

internal/localapi/utils_unsupported.go

@@ -24,6 +24,11 @@ func IsHyperVProvider(ctx context.Context) (bool, error) {
 	return false, nil
 }
 
+func IsWSLProvider(ctx context.Context) (bool, error) {
+	logrus.Debug("IsWSLProvider is not supported")
+	return false, nil
+}
+
 func ValidatePathForLocalAPI(path string) error {
 	logrus.Debug("ValidatePathForLocalAPI is not supported")
 	return nil

pkg/api/handlers/libpod/artifacts.go

@@ -5,8 +5,11 @@ package libpod
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"net/http"
+	"path/filepath"
 
+	"github.com/containers/podman/v6/internal/localapi"
 	"github.com/containers/podman/v6/libpod"
 	"github.com/containers/podman/v6/pkg/api/handlers/utils"
 	api "github.com/containers/podman/v6/pkg/api/types"
@@ -212,19 +215,21 @@ func BatchRemoveArtifact(w http.ResponseWriter, r *http.Request) {
 	utils.WriteResponse(w, http.StatusOK, artifacts)
 }
 
+type artifactAddRequestQuery struct {
+	Name             string   `schema:"name"`
+	FileName         string   `schema:"fileName"`
+	FileMIMEType     string   `schema:"fileMIMEType"`
+	Annotations      []string `schema:"annotations"`
+	ArtifactMIMEType string   `schema:"artifactMIMEType"`
+	Append           bool     `schema:"append"`
+	Replace          bool     `schema:"replace"`
+	Path             string   `schema:"path"`
+}
+
 func AddArtifact(w http.ResponseWriter, r *http.Request) {
-	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
 	decoder := r.Context().Value(api.DecoderKey).(*schema.Decoder)
 
-	query := struct {
-		Name             string   `schema:"name"`
-		FileName         string   `schema:"fileName"`
-		FileMIMEType     string   `schema:"fileMIMEType"`
-		Annotations      []string `schema:"annotations"`
-		ArtifactMIMEType string   `schema:"artifactMIMEType"`
-		Append           bool     `schema:"append"`
-		Replace          bool     `schema:"replace"`
-	}{}
+	query := artifactAddRequestQuery{}
 
 	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
 		utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to parse parameters for %s: %w", r.URL.String(), err))
@@ -236,6 +241,56 @@ func AddArtifact(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	artifactBlobs := []entities.ArtifactBlob{{
+		BlobReader: r.Body,
+		FileName:   query.FileName,
+	}}
+
+	addArtifactHelper(query, artifactBlobs, w, r)
+}
+
+func AddLocalArtifact(w http.ResponseWriter, r *http.Request) {
+	decoder := r.Context().Value(api.DecoderKey).(*schema.Decoder)
+
+	query := artifactAddRequestQuery{}
+
+	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
+		utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to parse parameters for %s: %w", r.URL.String(), err))
+		return
+	}
+
+	if query.Name == "" || query.FileName == "" {
+		utils.Error(w, http.StatusBadRequest, errors.New("name and file parameters are required"))
+		return
+	}
+
+	cleanPath := filepath.Clean(query.Path)
+	// Check if the path exists on server side.
+	// Note: localapi.ValidatePathForLocalAPI returns nil if the file exists and path is absolute, not an error.
+	switch err := localapi.ValidatePathForLocalAPI(cleanPath); {
+	case err == nil:
+		// no error -> continue
+	case errors.Is(err, localapi.ErrPathNotAbsolute):
+		utils.Error(w, http.StatusBadRequest, err)
+		return
+	case errors.Is(err, fs.ErrNotExist):
+		utils.Error(w, http.StatusNotFound, fmt.Errorf("file does not exist: %q", cleanPath))
+		return
+	default:
+		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("failed to access file: %w", err))
+		return
+	}
+
+	artifactBlobs := []entities.ArtifactBlob{{
+		BlobFilePath: cleanPath,
+		FileName:     query.FileName,
+	}}
+
+	addArtifactHelper(query, artifactBlobs, w, r)
+}
+
+func addArtifactHelper(query artifactAddRequestQuery, artifactBlobs []entities.ArtifactBlob, w http.ResponseWriter, r *http.Request) {
+	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
 	annotations, err := domain_utils.ParseAnnotations(query.Annotations)
 	if err != nil {
 		utils.Error(w, http.StatusBadRequest, err)
@@ -250,13 +305,7 @@ func AddArtifact(w http.ResponseWriter, r *http.Request) {
 		Replace:          query.Replace,
 	}
 
-	artifactBlobs := []entities.ArtifactBlob{{
-		BlobReader: r.Body,
-		FileName:   query.FileName,
-	}}
-
 	imageEngine := abi.ImageEngine{Libpod: runtime}
-
 	artifacts, err := imageEngine.ArtifactAdd(r.Context(), query.Name, artifactBlobs, artifactAddOptions)
 	if err != nil {
 		if errors.Is(err, libartifact_types.ErrArtifactNotExist) {

pkg/api/handlers/libpod/images.go

@@ -16,6 +16,7 @@ import (
 	"strings"
 
 	"github.com/containers/buildah"
+	"github.com/containers/podman/v6/internal/localapi"
 	"github.com/containers/podman/v6/libpod"
 	"github.com/containers/podman/v6/libpod/define"
 	"github.com/containers/podman/v6/pkg/api/handlers"
@@ -41,7 +42,6 @@ import (
 	"go.podman.io/storage"
 	"go.podman.io/storage/pkg/archive"
 	"go.podman.io/storage/pkg/chrootarchive"
-	"go.podman.io/storage/pkg/fileutils"
 	"go.podman.io/storage/pkg/idtools"
 )
 
@@ -396,10 +396,13 @@ func ImagesLocalLoad(w http.ResponseWriter, r *http.Request) {
 
 	cleanPath := filepath.Clean(query.Path)
 	// Check if the path exists on server side.
-	// Note: fileutils.Exists returns nil if the file exists, not an error.
-	switch err := fileutils.Exists(cleanPath); {
+	// Note: localapi.ValidatePathForLocalAPI returns nil if the file exists and path is absolute, not an error.
+	switch err := localapi.ValidatePathForLocalAPI(cleanPath); {
 	case err == nil:
 		// no error -> continue
+	case errors.Is(err, localapi.ErrPathNotAbsolute):
+		utils.Error(w, http.StatusBadRequest, err)
+		return
 	case errors.Is(err, fs.ErrNotExist):
 		utils.Error(w, http.StatusNotFound, fmt.Errorf("file does not exist: %q", cleanPath))
 		return

pkg/api/server/register_artifacts.go

@@ -212,6 +212,65 @@ func (s *APIServer) registerArtifactHandlers(r *mux.Router) error {
 	//   500:
 	//     $ref: "#/responses/internalError"
 	r.Handle(VersionedPath("/libpod/artifacts/add"), s.APIHandler(libpod.AddArtifact)).Methods(http.MethodPost)
+	// swagger:operation POST /libpod/artifacts/local/add libpod ArtifactLocalLibpod
+	// ---
+	// tags:
+	//  - artifacts
+	// summary: Add a local file as an artifact
+	// description: |
+	//   Add a file from the local filesystem as a new OCI artifact, or append to an existing artifact if 'append' is true.
+	// produces:
+	//   - application/json
+	// parameters:
+	//   - name: name
+	//     in: query
+	//     description: Mandatory reference to the artifact (e.g., quay.io/image/artifact:tag)
+	//     required: true
+	//     type: string
+	//   - name: path
+	//     in: query
+	//     description: Absolute path to the local file on the server filesystem to be added
+	//     required: true
+	//     type: string
+	//   - name: fileName
+	//     in: query
+	//     description: Name/title of the file within the artifact
+	//     required: true
+	//     type: string
+	//   - name: fileMIMEType
+	//     in: query
+	//     description: Optionally set the MIME type of the file
+	//     type: string
+	//   - name: annotations
+	//     in: query
+	//     description: Array of annotation strings e.g "test=true"
+	//     type: array
+	//     items:
+	//       type: string
+	//   - name: artifactMIMEType
+	//     in: query
+	//     description: Use type to describe an artifact
+	//     type: string
+	//   - name: append
+	//     in: query
+	//     description: Append files to an existing artifact
+	//     type: boolean
+	//     default: false
+	//   - name: replace
+	//     in: query
+	//     description: Replace an existing artifact with the same name
+	//     type: boolean
+	//     default: false
+	// responses:
+	//   201:
+	//     $ref: "#/responses/artifactAddResponse"
+	//   400:
+	//     $ref: "#/responses/badParamError"
+	//   404:
+	//     $ref: "#/responses/artifactNotFound"
+	//   500:
+	//     $ref: "#/responses/internalError"
+	r.Handle(VersionedPath("/libpod/artifacts/local/add"), s.APIHandler(libpod.AddLocalArtifact)).Methods(http.MethodPost)
 	// swagger:operation POST /libpod/artifacts/{name}/push libpod ArtifactPushLibpod
 	// ---
 	// tags:

pkg/api/server/register_images.go

@@ -952,7 +952,7 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
 	//     name: path
 	//     type: string
 	//     required: true
-	//     description: Path to the image archive file on the server filesystem
+	//     description: Absolute path to the image archive file on the server filesystem
 	// produces:
 	// - application/json
 	// responses:

pkg/bindings/artifacts/add.go

@@ -4,17 +4,31 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/url"
 
 	"github.com/containers/podman/v6/pkg/bindings"
+	"github.com/containers/podman/v6/pkg/domain/entities"
 	entitiesTypes "github.com/containers/podman/v6/pkg/domain/entities/types"
 )
 
 func Add(ctx context.Context, artifactName string, blobName string, artifactBlob io.Reader, options *AddOptions) (*entitiesTypes.ArtifactAddReport, error) {
-	conn, err := bindings.GetClient(ctx)
+	params, err := prepareParams(artifactName, blobName, options)
 	if err != nil {
 		return nil, err
 	}
+	return helperAdd(ctx, "/artifacts/add", params, artifactBlob)
+}
 
+func AddLocal(ctx context.Context, artifactName string, blobName string, blobPath string, options *AddOptions) (*entitiesTypes.ArtifactAddReport, error) {
+	params, err := prepareParams(artifactName, blobName, options)
+	if err != nil {
+		return nil, err
+	}
+	params.Set("path", blobPath)
+	return helperAdd(ctx, "/artifacts/local/add", params, nil)
+}
+
+func prepareParams(name string, fileName string, options *AddOptions) (url.Values, error) {
 	if options == nil {
 		options = new(AddOptions)
 	}
@@ -24,16 +38,25 @@ func Add(ctx context.Context, artifactName string, blobName string, artifactBlob
 		return nil, err
 	}
 
-	params.Set("name", artifactName)
-	params.Set("fileName", blobName)
+	params.Set("name", name)
+	params.Set("fileName", fileName)
 
-	response, err := conn.DoRequest(ctx, artifactBlob, http.MethodPost, "/artifacts/add", params, nil)
+	return params, nil
+}
+
+func helperAdd(ctx context.Context, endpoint string, params url.Values, artifactBlob io.Reader) (*entities.ArtifactAddReport, error) {
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	response, err := conn.DoRequest(ctx, artifactBlob, http.MethodPost, endpoint, params, nil)
 	if err != nil {
 		return nil, err
 	}
 	defer response.Body.Close()
 
-	var artifactAddReport entitiesTypes.ArtifactAddReport
+	var artifactAddReport entities.ArtifactAddReport
 	if err := response.Process(&artifactAddReport); err != nil {
 		return nil, err
 	}

pkg/domain/infra/tunnel/artifact.go

@@ -5,10 +5,14 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 
+	"github.com/containers/podman/v6/internal/localapi"
 	"github.com/containers/podman/v6/pkg/bindings/artifacts"
 	"github.com/containers/podman/v6/pkg/domain/entities"
+	"github.com/containers/podman/v6/pkg/errorhandling"
+	"github.com/sirupsen/logrus"
 	"go.podman.io/image/v5/types"
 )
 
@@ -103,26 +107,62 @@ func (ir *ImageEngine) ArtifactAdd(_ context.Context, name string, artifactBlob
 			// When adding more than 1 blob, set append true after the first
 			options.WithAppend(true)
 		}
-		f, err := os.Open(blob.BlobFilePath)
-		if err != nil {
-			return nil, err
-		}
-		defer f.Close()
 
-		artifactAddReport, err = artifacts.Add(ir.ClientCtx, name, blob.FileName, f, &options)
-		if err != nil && i > 0 {
-			removeOptions := artifacts.RemoveOptions{
-				Artifacts: []string{name},
-			}
-			_, recoverErr := artifacts.Remove(ir.ClientCtx, "", &removeOptions)
-			if recoverErr != nil {
-				return nil, fmt.Errorf("failed to cleanup unfinished artifact add: %w", errors.Join(err, recoverErr))
-			}
-			return nil, err
+		isWSL, err := localapi.IsWSLProvider(ir.ClientCtx)
+		if err != nil {
+			logrus.Debugf("IsWSLProvider check failed: %v", err)
 		}
+		if !isWSL {
+			if localMap, ok := localapi.CheckPathOnRunningMachine(ir.ClientCtx, blob.BlobFilePath); ok {
+				artifactAddReport, err = artifacts.AddLocal(ir.ClientCtx, name, blob.FileName, localMap.RemotePath, &options)
+				if err == nil {
+					continue
+				}
+				var errModel *errorhandling.ErrorModel
+				if errors.As(err, &errModel) {
+					switch errModel.ResponseCode {
+					case http.StatusNotFound, http.StatusMethodNotAllowed:
+					default:
+						return nil, artifactAddErrorCleanup(ir.ClientCtx, i, name, err)
+					}
+				} else {
+					return nil, artifactAddErrorCleanup(ir.ClientCtx, i, name, err)
+				}
+			}
+		}
+
+		artifactAddReport, err = addArtifact(ir.ClientCtx, name, i, blob, &options)
 		if err != nil {
 			return nil, err
 		}
 	}
 	return artifactAddReport, nil
 }
+
+func artifactAddErrorCleanup(ctx context.Context, index int, name string, err error) error {
+	if index == 0 {
+		return err
+	}
+	removeOptions := artifacts.RemoveOptions{
+		Artifacts: []string{name},
+	}
+	_, recoverErr := artifacts.Remove(ctx, "", &removeOptions)
+	if recoverErr != nil {
+		return fmt.Errorf("failed to cleanup unfinished artifact add: %w", errors.Join(err, recoverErr))
+	}
+	return err
+}
+
+func addArtifact(ctx context.Context, name string, index int, blob entities.ArtifactBlob, options *artifacts.AddOptions) (*entities.ArtifactAddReport, error) {
+	f, err := os.Open(blob.BlobFilePath)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	artifactAddReport, err := artifacts.Add(ctx, name, blob.FileName, f, options)
+	if err != nil {
+		return nil, artifactAddErrorCleanup(ctx, index, name, err)
+	}
+	return artifactAddReport, nil
+}

test/apiv2/10-images.at

@@ -502,7 +502,7 @@ t GET libpod/images/quay.io/libpod/busybox:latest/exists  204
 
 # Test with directory instead of file
 mkdir -p ${TMPD}/testdir
-t POST libpod/local/images/load?path="${TMPD}/testdir" 500
+t POST libpod/local/images/load?path="${TMPD}/testdir" 400
 
 cleanLoad
 
@@ -512,6 +512,6 @@ t POST libpod/local/images/load?invalid=arg 400
 
 t POST libpod/local/images/load?path="" 400
 
-t POST libpod/local/images/load?path="../../../etc/passwd" 404
+t POST libpod/local/images/load?path="../../../etc/passwd" 400
 
 # vim: filetype=sh

test/apiv2/python/rest_api/fixtures/api_testcase.py

@@ -84,6 +84,19 @@ class Artifact:
         os.remove(self.file.name)
         return r
 
+    def add_local(self) -> requests.Response:
+        try:
+            r = requests.post(
+                self.uri + "/artifacts/local/add",
+                params=self.parameters,
+            )
+        except Exception:
+            pass
+
+        if self.file is not None and os.path.exists(self.file.name):
+            os.remove(self.file.name)
+        return r
+
     def do_artifact_inspect_request(self) -> requests.Response:
         r = requests.get(
             self.uri + "/artifacts/" + self.name + "/json",

test/apiv2/python/rest_api/test_v2_0_0_artifact.py

@@ -2,6 +2,7 @@ import os
 import tarfile
 import unittest
 from typing import cast
+from pathlib import Path
 
 import requests
 
@@ -43,6 +44,40 @@ class ArtifactTestCase(APITestCase):
         # Assert blob media type fallback detection is working
         self.assertEqual(artifact_layer["mediaType"], "application/octet-stream")
 
+    def test_add_local(self):
+        ARTIFACT_NAME = "quay.io/myimage/mylocalartifact:latest"
+        file = ArtifactFile()
+        parameters: dict[str, str | list[str]] = {
+            "name": ARTIFACT_NAME,
+            "fileName": file.name,
+            "path": Path(file.name).absolute(),
+        }
+
+        artifact = Artifact(self.uri(""), ARTIFACT_NAME, parameters, file)
+
+        add_response = artifact.add_local()
+
+        # Assert correct response code
+        self.assertEqual(add_response.status_code, 201, add_response.text)
+
+        # Assert return response is json and contains digest
+        add_response_json = add_response.json()
+        self.assertIn("sha256:", cast(str, add_response_json["ArtifactDigest"]))
+
+        inspect_response_json = artifact.do_artifact_inspect_request().json()
+        artifact_layer = inspect_response_json["Manifest"]["layers"][0]
+
+        # Assert uploaded artifact blob is expected size
+        self.assertEqual(artifact_layer["size"], file.size)
+
+        # Assert uploaded artifact blob has expected title annotation
+        self.assertEqual(
+            artifact_layer["annotations"]["org.opencontainers.image.title"], file.name
+        )
+
+        # Assert blob media type fallback detection is working
+        self.assertEqual(artifact_layer["mediaType"], "application/octet-stream")
+
     def test_add_with_replace(self):
         ARTIFACT_NAME = "quay.io/myimage/newartifact:latest"
 
@@ -317,6 +352,52 @@ class ArtifactTestCase(APITestCase):
             "name and file parameters are required",
         )
 
+    def test_add_local_with_not_existing_file(self):
+        ARTIFACT_NAME = "quay.io/myimage/myartifact:latest"
+        parameters: dict[str, str | list[str]] = {
+            "name": ARTIFACT_NAME,
+            "fileName": "notexistsfile",
+            "path": "/home/notexistsfile",
+        }
+
+        artifact = Artifact(self.uri(""), ARTIFACT_NAME, parameters, None)
+
+        r = artifact.add_local()
+
+        rjson = r.json()
+
+        # Assert correct response code
+        self.assertEqual(r.status_code, 404, r.text)
+
+        # Assert return error response is json and contains correct message
+        self.assertEqual(
+            rjson["cause"],
+            'file does not exist: "/home/notexistsfile"',
+        )
+
+    def test_add_local_with_not_absolute_path(self):
+        ARTIFACT_NAME = "quay.io/myimage/myartifact:latest"
+        parameters: dict[str, str | list[str]] = {
+            "name": ARTIFACT_NAME,
+            "fileName": "notexistsfile",
+            "path": "../../etc/passwd",
+        }
+
+        artifact = Artifact(self.uri(""), ARTIFACT_NAME, parameters, None)
+
+        r = artifact.add_local()
+
+        rjson = r.json()
+
+        # Assert correct response code
+        self.assertEqual(r.status_code, 400, r.text)
+
+        # Assert return error response is json and contains correct message
+        self.assertEqual(
+            rjson["cause"],
+            'path is not absolute',
+        )
+
     def test_inspect(self):
         ARTIFACT_NAME = "quay.io/myimage/myartifact_mime_type:latest"
 
@@ -545,7 +626,6 @@ class ArtifactTestCase(APITestCase):
             "artifact does not exist",
         )
 
-
     def test_remove_all(self):
         # Create some artifacts to remove
         artifact_names = [
@@ -595,7 +675,6 @@ class ArtifactTestCase(APITestCase):
 
         url = self.uri("/artifacts/remove")
         r = requests.delete(url, params=removeparameters)
-        rjson = r.json()
 
         # Assert correct response code
         self.assertEqual(r.status_code, 200, r.text)