[lxc-devel] [lxd/master] lxd: Improve error messages

monstermunchkin on Github lxc-bot at linuxcontainers.org
Tue Jun 19 12:42:39 UTC 2018


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 373 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20180619/1a2c6e71/attachment.bin>
-------------- next part --------------
From 14e3cb460aa31e43c17b14578a7aa081827eccba Mon Sep 17 00:00:00 2001
From: Thomas Hipp <thomas.hipp at canonical.com>
Date: Tue, 19 Jun 2018 12:55:26 +0200
Subject: [PATCH] lxd: Improve error messages

Resolves #4453

Signed-off-by: Thomas Hipp <thomas.hipp at canonical.com>
---
 lxd/api.go                |  2 +-
 lxd/api_cluster.go        |  2 +-
 lxd/certificates.go       |  4 ++--
 lxd/container_file.go     |  2 +-
 lxd/container_metadata.go |  4 ++--
 lxd/container_patch.go    |  2 +-
 lxd/container_post.go     |  2 +-
 lxd/container_put.go      |  2 +-
 lxd/container_snapshot.go |  4 ++--
 lxd/containers_post.go    |  2 +-
 lxd/daemon.go             |  6 +++---
 lxd/images.go             | 10 +++++-----
 lxd/networks.go           | 11 ++++++-----
 lxd/operations.go         |  4 ++--
 lxd/profiles.go           |  2 +-
 lxd/response.go           | 48 ++++++++++++++++++++++++++++++++++++-----------
 lxd/storage_pools.go      |  2 +-
 lxd/storage_volumes.go    | 10 ++++++----
 18 files changed, 74 insertions(+), 45 deletions(-)

diff --git a/lxd/api.go b/lxd/api.go
index 825874f1b..f6509de1c 100644
--- a/lxd/api.go
+++ b/lxd/api.go
@@ -39,7 +39,7 @@ func RestServer(d *Daemon) *http.Server {
 	mux.NotFoundHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		logger.Info("Sending top level 404", log.Ctx{"url": r.URL})
 		w.Header().Set("Content-Type", "application/json")
-		NotFound.Render(w)
+		NotFound(nil).Render(w)
 	})
 
 	return &http.Server{Handler: &lxdHttpServer{r: mux, d: d}}
diff --git a/lxd/api_cluster.go b/lxd/api_cluster.go
index a8cc8f978..1aa7b6011 100644
--- a/lxd/api_cluster.go
+++ b/lxd/api_cluster.go
@@ -408,7 +408,7 @@ func clusterNodeGet(d *Daemon, r *http.Request) Response {
 		}
 	}
 
-	return NotFound
+	return NotFound(fmt.Errorf("node '%s' not found", name))
 }
 
 func clusterNodePost(d *Daemon, r *http.Request) Response {
diff --git a/lxd/certificates.go b/lxd/certificates.go
index 7c2a5a0c7..09416a192 100644
--- a/lxd/certificates.go
+++ b/lxd/certificates.go
@@ -106,7 +106,7 @@ func certificatesPost(d *Daemon, r *http.Request) Response {
 		return SmartError(err)
 	}
 	if d.checkTrustedClient(r) != nil && util.PasswordCheck(secret, req.Password) != nil {
-		return Forbidden
+		return Forbidden(nil)
 	}
 
 	if req.Type != "client" {
@@ -295,7 +295,7 @@ func certificateFingerprintDelete(d *Daemon, r *http.Request) Response {
 
 	certInfo, err := d.cluster.CertificateGet(fingerprint)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	err = d.cluster.CertDelete(certInfo.Fingerprint)
diff --git a/lxd/container_file.go b/lxd/container_file.go
index c9ffbd9e6..e179fc79e 100644
--- a/lxd/container_file.go
+++ b/lxd/container_file.go
@@ -42,7 +42,7 @@ func containerFileHandler(d *Daemon, r *http.Request) Response {
 	case "DELETE":
 		return containerFileDelete(c, path, r)
 	default:
-		return NotFound
+		return NotFound(fmt.Errorf("method '%s' not found", r.Method))
 	}
 }
 
diff --git a/lxd/container_metadata.go b/lxd/container_metadata.go
index 2f3a790b4..e1400f663 100644
--- a/lxd/container_metadata.go
+++ b/lxd/container_metadata.go
@@ -170,7 +170,7 @@ func containerMetadataTemplatesGet(d *Daemon, r *http.Request) Response {
 	}
 
 	if !shared.PathExists(templatePath) {
-		return NotFound
+		return NotFound(fmt.Errorf("path '%s' not found", templatePath))
 	}
 
 	// Create a temporary file with the template content (since the container
@@ -298,7 +298,7 @@ func containerMetadataTemplatesDelete(d *Daemon, r *http.Request) Response {
 	}
 
 	if !shared.PathExists(templatePath) {
-		return NotFound
+		return NotFound(fmt.Errorf("path '%s' not found", templatePath))
 	}
 
 	// Delete the template
diff --git a/lxd/container_patch.go b/lxd/container_patch.go
index 2b5a386db..e39ee6083 100644
--- a/lxd/container_patch.go
+++ b/lxd/container_patch.go
@@ -31,7 +31,7 @@ func containerPatch(d *Daemon, r *http.Request) Response {
 
 	c, err := containerLoadByName(d.State(), name)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	// Validate the ETag
diff --git a/lxd/container_post.go b/lxd/container_post.go
index a3ae76620..41f3353de 100644
--- a/lxd/container_post.go
+++ b/lxd/container_post.go
@@ -180,7 +180,7 @@ func containerPost(d *Daemon, r *http.Request) Response {
 	// Check that the name isn't already in use
 	id, _ := d.cluster.ContainerID(req.Name)
 	if id > 0 {
-		return Conflict
+		return Conflict(fmt.Errorf("name '%s' already in use", req.Name))
 	}
 
 	run := func(*operation) error {
diff --git a/lxd/container_put.go b/lxd/container_put.go
index 7c18fa287..b738b1937 100644
--- a/lxd/container_put.go
+++ b/lxd/container_put.go
@@ -35,7 +35,7 @@ func containerPut(d *Daemon, r *http.Request) Response {
 
 	c, err := containerLoadByName(d.State(), name)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	// Validate the ETag
diff --git a/lxd/container_snapshot.go b/lxd/container_snapshot.go
index fc39d03b8..5f885a508 100644
--- a/lxd/container_snapshot.go
+++ b/lxd/container_snapshot.go
@@ -183,7 +183,7 @@ func snapshotHandler(d *Daemon, r *http.Request) Response {
 	case "DELETE":
 		return snapshotDelete(sc, snapshotName)
 	default:
-		return NotFound
+		return NotFound(fmt.Errorf("method '%s' not found", r.Method))
 	}
 }
 
@@ -288,7 +288,7 @@ func snapshotPost(d *Daemon, r *http.Request, sc container, containerName string
 	// Check that the name isn't already in use
 	id, _ := d.cluster.ContainerID(fullName)
 	if id > 0 {
-		return Conflict
+		return Conflict(fmt.Errorf("name '%s' already in use", fullName))
 	}
 
 	rename := func(op *operation) error {
diff --git a/lxd/containers_post.go b/lxd/containers_post.go
index 4ea62a10b..300698117 100644
--- a/lxd/containers_post.go
+++ b/lxd/containers_post.go
@@ -177,7 +177,7 @@ func createFromNone(d *Daemon, req *api.ContainersPost) Response {
 func createFromMigration(d *Daemon, req *api.ContainersPost) Response {
 	// Validate migration mode
 	if req.Source.Mode != "pull" && req.Source.Mode != "push" {
-		return NotImplemented
+		return NotImplemented(fmt.Errorf("mode '%s' not implemented", req.Source.Mode))
 	}
 
 	var c container
diff --git a/lxd/daemon.go b/lxd/daemon.go
index fef03f24f..b37ff701c 100644
--- a/lxd/daemon.go
+++ b/lxd/daemon.go
@@ -266,7 +266,7 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 			logger.Warn(
 				"rejecting request from untrusted client",
 				log.Ctx{"ip": r.RemoteAddr})
-			Forbidden.Render(w)
+			Forbidden(nil).Render(w)
 			return
 		}
 
@@ -284,7 +284,7 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 		}
 
 		var resp Response
-		resp = NotImplemented
+		resp = NotImplemented(nil)
 
 		switch r.Method {
 		case "GET":
@@ -308,7 +308,7 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 				resp = c.patch(d, r)
 			}
 		default:
-			resp = NotFound
+			resp = NotFound(fmt.Errorf("method '%s' not found", r.Method))
 		}
 
 		if err := resp.Render(w); err != nil {
diff --git a/lxd/images.go b/lxd/images.go
index ac4128d58..371a1ec56 100644
--- a/lxd/images.go
+++ b/lxd/images.go
@@ -1208,7 +1208,7 @@ func imageGet(d *Daemon, r *http.Request) Response {
 	}
 
 	if !info.Public && public && !imageValidSecret(info.Fingerprint, secret) {
-		return NotFound
+		return NotFound(fmt.Errorf("image '%s' not found", info.Fingerprint))
 	}
 
 	etag := []interface{}{info.Public, info.AutoUpdate, info.Properties}
@@ -1324,7 +1324,7 @@ func aliasesPost(d *Daemon, r *http.Request) Response {
 	// This is just to see if the alias name already exists.
 	_, _, err := d.cluster.ImageAliasGet(req.Name, true)
 	if err == nil {
-		return Conflict
+		return Conflict(fmt.Errorf("alias '%s' already exists", req.Name))
 	}
 
 	id, _, err := d.cluster.ImageGet(req.Target, false, false)
@@ -1495,7 +1495,7 @@ func aliasPost(d *Daemon, r *http.Request) Response {
 	// Check that the name isn't already in use
 	id, _, _ := d.cluster.ImageAliasGet(req.Name, true)
 	if id > 0 {
-		return Conflict
+		return Conflict(fmt.Errorf("alias '%s' already in use", req.Name))
 	}
 
 	id, _, err := d.cluster.ImageAliasGet(name, true)
@@ -1527,7 +1527,7 @@ func imageExport(d *Daemon, r *http.Request) Response {
 		}
 
 		if !imgInfo.Public && !imgInfo.Cached {
-			return NotFound
+			return NotFound(fmt.Errorf("image '%s' not found", fingerprint))
 		}
 	} else {
 		_, imgInfo, err = d.cluster.ImageGet(fingerprint, false, false)
@@ -1536,7 +1536,7 @@ func imageExport(d *Daemon, r *http.Request) Response {
 		}
 
 		if !imgInfo.Public && public && !imageValidSecret(imgInfo.Fingerprint, secret) {
-			return NotFound
+			return NotFound(fmt.Errorf("image '%s' not found", imgInfo.Fingerprint))
 		}
 	}
 
diff --git a/lxd/networks.go b/lxd/networks.go
index 23d4eaf5c..5b486fd64 100644
--- a/lxd/networks.go
+++ b/lxd/networks.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/gorilla/mux"
 	log "github.com/lxc/lxd/shared/log15"
+	"github.com/pkg/errors"
 
 	lxd "github.com/lxc/lxd/client"
 	"github.com/lxc/lxd/lxd/cluster"
@@ -435,7 +436,7 @@ func networkDelete(d *Daemon, r *http.Request) Response {
 	// Get the existing network
 	n, err := networkLoadByName(state, name)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	withDatabase := true
@@ -503,7 +504,7 @@ func networkPost(d *Daemon, r *http.Request) Response {
 	// Get the existing network
 	n, err := networkLoadByName(state, name)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	// Sanity checks
@@ -523,7 +524,7 @@ func networkPost(d *Daemon, r *http.Request) Response {
 	}
 
 	if shared.StringInSlice(req.Name, networks) {
-		return Conflict
+		return Conflict(fmt.Errorf("network '%s' already exists", req.Name))
 	}
 
 	// Rename it
@@ -614,7 +615,7 @@ func doNetworkUpdate(d *Daemon, name string, oldConfig map[string]string, req ap
 	// Load the network
 	n, err := networkLoadByName(d.State(), name)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	err = n.Update(req)
@@ -639,7 +640,7 @@ func networkLeasesGet(d *Daemon, r *http.Request) Response {
 
 	// Validate that we do have leases for it
 	if !n.Managed || n.Type != "bridge" {
-		return NotFound
+		return NotFound(errors.New("leases not found"))
 	}
 
 	if !shared.PathExists(leaseFile) {
diff --git a/lxd/operations.go b/lxd/operations.go
index 849256ed2..299ab8107 100644
--- a/lxd/operations.go
+++ b/lxd/operations.go
@@ -492,7 +492,7 @@ func operationAPIDelete(d *Daemon, r *http.Request) Response {
 
 	op, err := operationGet(id)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	_, err = op.Cancel()
@@ -554,7 +554,7 @@ func operationAPIWaitGet(d *Daemon, r *http.Request) Response {
 	id := mux.Vars(r)["id"]
 	op, err := operationGet(id)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	_, err = op.WaitFinal(timeout)
diff --git a/lxd/profiles.go b/lxd/profiles.go
index fee793eb2..c5c8b134b 100644
--- a/lxd/profiles.go
+++ b/lxd/profiles.go
@@ -294,7 +294,7 @@ func profilePost(d *Daemon, r *http.Request) Response {
 	// Check that the name isn't already in use
 	id, _, _ := d.cluster.ProfileGet(req.Name)
 	if id > 0 {
-		return Conflict
+		return Conflict(fmt.Errorf("name '%s' already in use", req.Name))
 	}
 
 	if strings.Contains(req.Name, "/") {
diff --git a/lxd/response.go b/lxd/response.go
index 85ac1fd92..64321987d 100644
--- a/lxd/response.go
+++ b/lxd/response.go
@@ -492,11 +492,37 @@ func (r *errorResponse) Render(w http.ResponseWriter) error {
 	return nil
 }
 
-/* Some standard responses */
-var NotImplemented = &errorResponse{http.StatusNotImplemented, "not implemented"}
-var NotFound = &errorResponse{http.StatusNotFound, "not found"}
-var Forbidden = &errorResponse{http.StatusForbidden, "not authorized"}
-var Conflict = &errorResponse{http.StatusConflict, "already exists"}
+func NotImplemented(err error) Response {
+	message := "not implemented"
+	if err != nil {
+		message = err.Error()
+	}
+	return &errorResponse{http.StatusNotImplemented, message}
+}
+
+func NotFound(err error) Response {
+	message := "not found"
+	if err != nil {
+		message = err.Error()
+	}
+	return &errorResponse{http.StatusNotFound, message}
+}
+
+func Forbidden(err error) Response {
+	message := "not authorized"
+	if err != nil {
+		message = err.Error()
+	}
+	return &errorResponse{http.StatusForbidden, message}
+}
+
+func Conflict(err error) Response {
+	message := "already exists"
+	if err != nil {
+		message = err.Error()
+	}
+	return &errorResponse{http.StatusConflict, message}
+}
 
 func Unavailable(err error) Response {
 	message := "unavailable"
@@ -526,17 +552,17 @@ func SmartError(err error) Response {
 	case nil:
 		return EmptySyncResponse
 	case os.ErrNotExist:
-		return NotFound
+		return NotFound(err)
 	case sql.ErrNoRows:
-		return NotFound
+		return NotFound(err)
 	case db.ErrNoSuchObject:
-		return NotFound
+		return NotFound(err)
 	case os.ErrPermission:
-		return Forbidden
+		return Forbidden(err)
 	case db.ErrAlreadyDefined:
-		return Conflict
+		return Conflict(err)
 	case sqlite3.ErrConstraintUnique:
-		return Conflict
+		return Conflict(err)
 	default:
 		return InternalError(err)
 	}
diff --git a/lxd/storage_pools.go b/lxd/storage_pools.go
index 17bff4bee..3c0568774 100644
--- a/lxd/storage_pools.go
+++ b/lxd/storage_pools.go
@@ -499,7 +499,7 @@ func storagePoolDelete(d *Daemon, r *http.Request) Response {
 
 	poolID, err := d.cluster.StoragePoolGetID(poolName)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	// If this is not an internal cluster request, check if the storage
diff --git a/lxd/storage_volumes.go b/lxd/storage_volumes.go
index 12a46a149..ede82ff1f 100644
--- a/lxd/storage_volumes.go
+++ b/lxd/storage_volumes.go
@@ -223,7 +223,7 @@ func doVolumeCreateOrCopy(d *Daemon, poolName string, req *api.StorageVolumesPos
 func doVolumeMigration(d *Daemon, poolName string, req *api.StorageVolumesPost) Response {
 	// Validate migration mode
 	if req.Source.Mode != "pull" && req.Source.Mode != "push" {
-		return NotImplemented
+		return NotImplemented(fmt.Errorf("mode '%s' not implemented", req.Source.Mode))
 	}
 
 	storage, err := storagePoolVolumeDBCreateInternal(d.State(), poolName, req)
@@ -418,8 +418,10 @@ func storagePoolVolumeTypePost(d *Daemon, r *http.Request) Response {
 	// Check that the name isn't already in use.
 	_, err = d.cluster.StoragePoolNodeVolumeGetTypeID(req.Name,
 		storagePoolVolumeTypeCustom, poolID)
-	if err == nil || err != nil && err != db.ErrNoSuchObject {
-		return Conflict
+	if err == nil {
+		return Conflict(fmt.Errorf("name '%s' already in use", req.Name))
+	} else if err != nil && err != db.ErrNoSuchObject {
+		return Conflict(err)
 	}
 
 	doWork := func() error {
@@ -758,7 +760,7 @@ func storagePoolVolumeTypeDelete(d *Daemon, r *http.Request) Response {
 
 	s, err := storagePoolVolumeInit(d.State(), poolName, volumeName, volumeType)
 	if err != nil {
-		return NotFound
+		return NotFound(err)
 	}
 
 	switch volumeType {


More information about the lxc-devel mailing list