From 6e7de438cc0b8f707050d0866a785f201c7aa87d Mon Sep 17 00:00:00 2001 From: Lewis Roy Date: Wed, 23 Apr 2025 00:37:14 +1000 Subject: [PATCH] bug: Correct Docker compat REST API image delete endpoint The Docker `-XDELETE image/$name?force=true` endpoint only removes containers using an image if they are in a non running state. In Podman, when forcefully removing images we also forcefully delete containers using the image including running containers. This patch changes the Docker image force delete compat API to act like the Docker API while maintaining commands like `podman rmi -f $imagename` It also corrects the API return code returned when an image is requested to be deleted with running containers using it. Fixes: https://github.com/containers/podman/issues/25871 Signed-off-by: Lewis Roy --- libpod/reset.go | 2 +- libpod/runtime_img.go | 11 ++--- pkg/api/handlers/compat/images_remove.go | 12 ++++-- pkg/api/server/register_images.go | 2 +- pkg/domain/entities/images.go | 15 ++++--- pkg/domain/infra/abi/images.go | 4 +- test/apiv2/10-images.at | 41 +++++++++++++++++++ .../python/rest_api/test_v2_0_0_image.py | 5 +-- 8 files changed, 72 insertions(+), 20 deletions(-) diff --git a/libpod/reset.go b/libpod/reset.go index c223aeb5df..00ae3bd42c 100644 --- a/libpod/reset.go +++ b/libpod/reset.go @@ -178,7 +178,7 @@ func (r *Runtime) Reset(ctx context.Context) error { rmiOptions := &libimage.RemoveImagesOptions{ Force: true, Ignore: true, - RemoveContainerFunc: r.RemoveContainersForImageCallback(ctx), + RemoveContainerFunc: r.RemoveContainersForImageCallback(ctx, true), Filters: []string{"readonly=false"}, } if _, rmiErrors := r.LibimageRuntime().RemoveImages(ctx, nil, rmiOptions); rmiErrors != nil { diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index bac8ec31f2..cf32d953e8 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -23,9 +23,10 @@ import ( // RemoveContainersForImageCallback returns a callback that can be used in // `libimage`. When forcefully removing images, containers using the image -// should be removed as well. The callback allows for more graceful removal as -// we can use the libpod-internal removal logic. -func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage.RemoveContainerFunc { +// should be removed as well unless the request comes from the Docker compat API. +// The callback allows for more graceful removal as we can use the libpod-internal +// removal logic. +func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context, force bool) libimage.RemoveContainerFunc { return func(imageID string) error { if !r.valid { return define.ErrRuntimeStopped @@ -44,12 +45,12 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage if err != nil { return fmt.Errorf("container %s is in pod %s, but pod cannot be retrieved: %w", ctr.ID(), ctr.config.Pod, err) } - if _, err := r.removePod(ctx, pod, true, true, timeout); err != nil { + if _, err := r.removePod(ctx, pod, true, force, timeout); err != nil { return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } else { opts := ctrRmOpts{ - Force: true, + Force: force, Timeout: timeout, } if _, _, err := r.removeContainer(ctx, ctr, opts); err != nil { diff --git a/pkg/api/handlers/compat/images_remove.go b/pkg/api/handlers/compat/images_remove.go index 8379da99b1..9382ebb86e 100644 --- a/pkg/api/handlers/compat/images_remove.go +++ b/pkg/api/handlers/compat/images_remove.go @@ -8,6 +8,7 @@ import ( "net/http" "github.com/containers/podman/v5/libpod" + "github.com/containers/podman/v5/libpod/define" "github.com/containers/podman/v5/pkg/api/handlers/utils" api "github.com/containers/podman/v5/pkg/api/types" "github.com/containers/podman/v5/pkg/domain/entities" @@ -41,9 +42,10 @@ func RemoveImage(w http.ResponseWriter, r *http.Request) { imageEngine := abi.ImageEngine{Libpod: runtime} options := entities.ImageRemoveOptions{ - Force: query.Force, - NoPrune: query.NoPrune, - Ignore: query.Ignore, + Force: query.Force, + NoPrune: query.NoPrune, + Ignore: query.Ignore, + DisableForceRemoveContainers: true, } report, rmerrors := imageEngine.Remove(r.Context(), []string{possiblyNormalizedName}, options) if len(rmerrors) > 0 && rmerrors[0] != nil { @@ -56,6 +58,10 @@ func RemoveImage(w http.ResponseWriter, r *http.Request) { utils.Error(w, http.StatusConflict, fmt.Errorf("image %s is in use: %w", name, err)) return } + if errors.Is(err, define.ErrCtrStateInvalid) { + utils.Error(w, http.StatusConflict, fmt.Errorf("image %s is in an invalid state: %w", name, err)) + return + } utils.Error(w, http.StatusInternalServerError, err) return } diff --git a/pkg/api/server/register_images.go b/pkg/api/server/register_images.go index 9350abbe35..ca565ffda1 100644 --- a/pkg/api/server/register_images.go +++ b/pkg/api/server/register_images.go @@ -227,7 +227,7 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error { // - in: query // name: force // type: boolean - // description: remove the image even if used by containers or has other tags + // description: Remove the image even if it is being used by stopped containers or has other tags // - in: query // name: noprune // type: boolean diff --git a/pkg/domain/entities/images.go b/pkg/domain/entities/images.go index b9100af8ff..395335d9a3 100644 --- a/pkg/domain/entities/images.go +++ b/pkg/domain/entities/images.go @@ -64,7 +64,8 @@ type ImageRemoveOptions struct { // Confirms if given name is a manifest list and removes it, otherwise returns error. LookupManifest bool // NoPrune will not remove dangling images - NoPrune bool + NoPrune bool + DisableForceRemoveContainers bool } // ImageRemoveReport is the response for removing one or more image(s) from storage @@ -73,8 +74,10 @@ type ImageRemoveReport = entitiesTypes.ImageRemoveReport type ImageHistoryOptions struct{} -type ImageHistoryLayer = entitiesTypes.ImageHistoryLayer -type ImageHistoryReport = entitiesTypes.ImageHistoryReport +type ( + ImageHistoryLayer = entitiesTypes.ImageHistoryLayer + ImageHistoryReport = entitiesTypes.ImageHistoryReport +) // ImagePullOptions are the arguments for pulling images. type ImagePullOptions struct { @@ -255,8 +258,10 @@ type ImagePruneOptions struct { Filter []string `json:"filter" schema:"filter"` } -type ImageTagOptions struct{} -type ImageUntagOptions struct{} +type ( + ImageTagOptions struct{} + ImageUntagOptions struct{} +) // ImageInspectReport is the data when inspecting an image. type ImageInspectReport = entitiesTypes.ImageInspectReport diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go index 53b0cee197..0f38f63c6d 100644 --- a/pkg/domain/infra/abi/images.go +++ b/pkg/domain/infra/abi/images.go @@ -58,7 +58,7 @@ func (ir *ImageEngine) Exists(_ context.Context, nameOrID string) (*entities.Boo func (ir *ImageEngine) Prune(ctx context.Context, opts entities.ImagePruneOptions) ([]*reports.PruneReport, error) { pruneOptions := &libimage.RemoveImagesOptions{ - RemoveContainerFunc: ir.Libpod.RemoveContainersForImageCallback(ctx), + RemoveContainerFunc: ir.Libpod.RemoveContainersForImageCallback(ctx, true), IsExternalContainerFunc: ir.Libpod.IsExternalContainerCallback(ctx), ExternalContainers: opts.External, Filters: append(opts.Filter, "readonly=false"), @@ -666,7 +666,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie if !opts.All { libimageOptions.Filters = append(libimageOptions.Filters, "intermediate=false") } - libimageOptions.RemoveContainerFunc = ir.Libpod.RemoveContainersForImageCallback(ctx) + libimageOptions.RemoveContainerFunc = ir.Libpod.RemoveContainersForImageCallback(ctx, !opts.DisableForceRemoveContainers) libimageReport, libimageErrors := ir.Libpod.LibimageRuntime().RemoveImages(ctx, images, libimageOptions) diff --git a/test/apiv2/10-images.at b/test/apiv2/10-images.at index eb434ba4fc..93492d1f14 100644 --- a/test/apiv2/10-images.at +++ b/test/apiv2/10-images.at @@ -231,6 +231,47 @@ t DELETE images/test1:latest 200 t GET "images/get?names=alpine" 200 '[POSIX tar archive]' +# START: Testing variance between Docker API and Podman API +# regarding force deleting images. +# Podman: Force deleting an image will force remove any +# container using the image. +# Docker: Force deleting an image will only remove non +# running containers using the image. + +# Create new image +podman image build -t docker.io/library/test1:latest - <file4 +EOF + +# Create running container +podman run --rm -d --name test_container docker.io/library/test1:latest top + +# When using the Docker Compat API, force deleting an image +# shouldn't force delete any container using the image, only +# containers in a non running state should be removed. +# https://github.com/containers/podman/issues/25871 +t DELETE images/test1:latest?force=true 409 + +# When using the Podman Libpod API, deleting an image +# with a running container will fail. +t DELETE libpod/images/test1:latest 409 + +# When using the Podman Libpod API, force deleting an +# image will also force delete all containers using the image. + +# Verify container exists. +t GET libpod/containers/test_container/exists 204 + +# Delete image with force. +t DELETE libpod/images/test1:latest?force=true 200 + +# Verify container also removed. +t GET libpod/containers/test_container/exists 404 + +# END: Testing variance between Docker API and Podman API +# regarding force deleting images. + podman pull busybox t GET "images/get?names=alpine&names=busybox" 200 '[POSIX tar archive]' img_cnt=$(tar xf "$WORKDIR/curl.result.out" manifest.json -O | jq "length") diff --git a/test/apiv2/python/rest_api/test_v2_0_0_image.py b/test/apiv2/python/rest_api/test_v2_0_0_image.py index 58d03b1493..7882be2059 100644 --- a/test/apiv2/python/rest_api/test_v2_0_0_image.py +++ b/test/apiv2/python/rest_api/test_v2_0_0_image.py @@ -66,9 +66,8 @@ class ImageTestCase(APITestCase): self.assertIn("sha256:",image['Id']) def test_delete(self): - r = requests.delete(self.podman_url + "/v1.40/images/alpine?force=true") - self.assertEqual(r.status_code, 200, r.text) - self.assertIsInstance(r.json(), list) + r = requests.delete(self.compat_uri("images/alpine?force=true")) + self.assertEqual(r.status_code, 409, r.text) def test_pull(self): r = requests.post(self.uri("/images/pull?reference=alpine"), timeout=15)