Merge pull request #2051 from cclerget/delete-image-errors

Fix ImageDelete API not returning error when checking if image is used by nodes/profiles

CHANGELOG.md

@@ -24,6 +24,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 - Fix "address already in use" in `wwclient` when `secure: true`. #2009
 - Use device names in netplan bonds. #2013
+- Fix ImageDelete API not returning error when checking if image is used by nodes/profiles
 
 ### Dependencies
 

internal/pkg/api/image/image.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -58,37 +59,38 @@ func ImageDelete(cdp *wwapiv1.ImageDeleteParameter) (err error) {
 		return fmt.Errorf("could not open nodeDB: %s", err)
 	}
 
-ARG_LOOP:
+	// validate image names
-	for i := 0; i < len(cdp.ImageNames); i++ {
+	for _, imageName := range cdp.ImageNames {
-		//_, arg := range args {
-		imageName := cdp.ImageNames[i]
-		for _, n := range nodeDB.Nodes {
-			if n.ImageName == imageName {
-				wwlog.Error("image %s is in use by node %s, skipping", imageName, n.Id())
-				continue ARG_LOOP
-			}
-		}
-		for _, p := range nodeDB.NodeProfiles {
-			if p.ImageName == imageName {
-				wwlog.Error("image %s is in use by profile %s, skipping", imageName, p.Id())
-				continue ARG_LOOP
-			}
-		}
-
 		if !image.ValidSource(imageName) {
-			wwlog.Error("image name is not a valid source: %s", imageName)
+			return fmt.Errorf("image name is not valid source: %s", imageName)
-			continue
 		}
+	}
+
+	// check if the deleted images are not used by nodes
+	for nodeName, node := range nodeDB.Nodes {
+		if slices.Contains(cdp.ImageNames, node.ImageName) {
+			return fmt.Errorf("image %s is in use by node %s, cannot delete", node.ImageName, nodeName)
+		}
+	}
+
+	// check if the deleted images are not used by profiles
+	for profileName, profile := range nodeDB.NodeProfiles {
+		if slices.Contains(cdp.ImageNames, profile.ImageName) {
+			return fmt.Errorf("image %s is in use by profile %s, cannot delete", profile.ImageName, profileName)
+		}
+	}
+
+	// delete images
+	for _, imageName := range cdp.ImageNames {
 		err := image.DeleteSource(imageName)
 		if err != nil {
-			wwlog.Error("could not remove source: %s", imageName)
+			return fmt.Errorf("could not remove source image %s: %w", imageName, err)
 		}
 		err = image.DeleteImage(imageName)
 		if err != nil {
-			wwlog.Error("could not remove image files %s", imageName)
+			return fmt.Errorf("could not remove image file %s: %w", imageName, err)
 		}
-
+		wwlog.Info("Image %q has been deleted", imageName)
-		fmt.Printf("Image has been deleted: %s\n", imageName)
 	}
 
 	return

internal/pkg/warewulfd/api/image_test.go

@@ -16,7 +16,8 @@ import (
 	"github.com/warewulf/warewulf/internal/pkg/warewulfd"
 )
 
-var imageTests = map[string]struct {
+var imageTests = []struct {
+	name              string
 	initFiles         []string
 	request           func(serverURL string) (*http.Request, error)
 	response          string
@@ -25,7 +26,8 @@ var imageTests = map[string]struct {
 	resultAbsentFiles []string
 	authenticate      bool
 }{
-	"test no authentication": {
+	{
+		name: "test no authentication",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/test-image/rootfs/file",
 		},
@@ -36,8 +38,8 @@ var imageTests = map[string]struct {
 		status:       http.StatusUnauthorized,
 		authenticate: false,
 	},
-
+	{
-	"test get all images": {
+		name: "test get all images",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/test-image/rootfs/file",
 		},
@@ -47,8 +49,8 @@ var imageTests = map[string]struct {
 		response:     `{"test-image": {"kernels":[], "size":0, "buildtime":0, "writable":true}}`,
 		authenticate: true,
 	},
-
+	{
-	"test get single image": {
+		name: "test get single image",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/test-image/rootfs/file",
 		},
@@ -58,8 +60,8 @@ var imageTests = map[string]struct {
 		response:     `{"kernels":[], "size":0, "buildtime":0, "writable":true}`,
 		authenticate: true,
 	},
-
+	{
-	"test build image": {
+		name: "test build image",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/test-image/rootfs/file",
 		},
@@ -73,8 +75,8 @@ var imageTests = map[string]struct {
 		},
 		authenticate: true,
 	},
-
+	{
-	"test rename image": {
+		name: "test rename image",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/test-image/rootfs/file",
 		},
@@ -84,15 +86,15 @@ var imageTests = map[string]struct {
 		response:     `{"kernels":[], "size":512, "buildtime":"<<PRESENCE>>", "writable":true}`,
 		authenticate: true,
 	},
-
+	{
-	"test delete image": {
+		name: "test delete image",
 		initFiles: []string{
 			"/var/lib/warewulf/chroots/new-image/rootfs/file",
 		},
 		request: func(serverURL string) (*http.Request, error) {
 			return http.NewRequest(http.MethodDelete, serverURL+"/api/images/new-image", nil)
 		},
-		response: `{"kernels":[], "size":0, "buildtime":"<<PRESENCE>>", "writable":true}`,
+		response: `{"kernels":[], "size":512, "buildtime":"<<PRESENCE>>", "writable":true}`,
 		resultAbsentFiles: []string{
 			"/var/lib/warewulf/chroots/new-image",
 			"/srv/warewulf/images/new-image.img",
@@ -108,13 +110,13 @@ users:
 - name: admin
   password hash: $2b$05$5QVWDpiWE7L4SDL9CYdi3O/l6HnbNOLoXgY2sa1bQQ7aSBKdSqvsC
 `
+	env := testenv.New(t)
+	defer env.RemoveAll()
 
-	for name, tt := range imageTests {
+	warewulfd.SetNoDaemon()
-		t.Run(name, func(t *testing.T) {
-			warewulfd.SetNoDaemon()
-			env := testenv.New(t)
-			defer env.RemoveAll()
 
+	for _, tt := range imageTests {
+		t.Run(tt.name, func(t *testing.T) {
 			// Create test files
 			for _, fileName := range tt.initFiles {
 				env.CreateFile(fileName)