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: #25871

Signed-off-by: Lewis Roy <[email protected]>

Author: ninja-quokka

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 {

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 {

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
 	}

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

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

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)
 

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 - <<EOF
+from alpine
+RUN >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")

test/apiv2/python/rest_api/test_v2_0_0_image.py

@@ -66,9 +66,8 @@ def test_inspect(self):
         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)