From 2c4c135bcc366b9f3157d9268cc69e59e941e77a Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:20:37 +0100 Subject: [PATCH 1/7] lxd/db: Remove `ErrAlreadyDefined` sentinel error. This error is imported by the `lxd/request` package so that it can convert it to an `http.StatusConflict` on a call to `response.SmartError`. Other applications are using the `lxd/request` package. Removing this error prevents the import of `lxd/db` into `lxd/request` and therefore prevents the import of `lxd/db` into downstream repositories. Signed-off-by: Mark Laing --- lxd/db/errors.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lxd/db/errors.go b/lxd/db/errors.go index e42641a469e0..2a6fad4f9eb6 100644 --- a/lxd/db/errors.go +++ b/lxd/db/errors.go @@ -5,10 +5,6 @@ import ( ) var ( - // ErrAlreadyDefined hapens when the given entry already exists, - // for example a container. - ErrAlreadyDefined = fmt.Errorf("The record already exists") - // ErrNoClusterMember is used to indicate no cluster member has been found for a resource. ErrNoClusterMember = fmt.Errorf("No cluster member found") ) From 11be4eac1653bf8884d60543f4ed360fae9fe599 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:24:56 +0100 Subject: [PATCH 2/7] lxd/db: Replace `ErrAlreadyDefined` with an `api.StatusError`. This will be caught by `response.SmartError` equivalently to return a 409 to the caller. Additionally, error messages are updated to be more descriptive. Signed-off-by: Mark Laing --- lxd/db/backups.go | 4 ++-- lxd/db/networks.go | 2 +- lxd/db/node.go | 2 +- lxd/db/storage_pools.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lxd/db/backups.go b/lxd/db/backups.go index e669994100c4..1beb7b6cfdfb 100644 --- a/lxd/db/backups.go +++ b/lxd/db/backups.go @@ -161,7 +161,7 @@ WHERE projects.name=? AND instances.name=?` func (c *ClusterTx) CreateInstanceBackup(ctx context.Context, args InstanceBackup) error { _, err := c.getInstanceBackupID(ctx, args.Name) if err == nil { - return ErrAlreadyDefined + return api.StatusErrorf(http.StatusConflict, "Backup for instance %q already exists", args.Name) } instanceOnlyInt := 0 @@ -388,7 +388,7 @@ ORDER BY storage_volumes_backups.id` func (c *ClusterTx) CreateStoragePoolVolumeBackup(ctx context.Context, args StoragePoolVolumeBackup) error { _, err := c.getStoragePoolVolumeBackupID(ctx, args.Name) if err == nil { - return ErrAlreadyDefined + return api.StatusErrorf(http.StatusConflict, "Backup for storage volume %q already exists", args.Name) } volumeOnlyInt := 0 diff --git a/lxd/db/networks.go b/lxd/db/networks.go index 98ac311eb656..f5f66a67f746 100644 --- a/lxd/db/networks.go +++ b/lxd/db/networks.go @@ -352,7 +352,7 @@ func (c *ClusterTx) CreatePendingNetwork(ctx context.Context, node string, proje } if count != 0 { - return ErrAlreadyDefined + return api.StatusErrorf(http.StatusConflict, "Network %q already exists", name) } // Insert the node-specific configuration with state networkPending. diff --git a/lxd/db/node.go b/lxd/db/node.go index 955610ef59f9..a0e4fc69dec8 100644 --- a/lxd/db/node.go +++ b/lxd/db/node.go @@ -381,7 +381,7 @@ func (c *ClusterTx) RenameNode(ctx context.Context, old string, new string) erro } if count != 0 { - return ErrAlreadyDefined + return api.StatusErrorf(http.StatusConflict, "A cluster member already exists with name %q", new) } stmt := `UPDATE nodes SET name=? WHERE name=?` diff --git a/lxd/db/storage_pools.go b/lxd/db/storage_pools.go index 3107a85a98d5..a73626ea0551 100644 --- a/lxd/db/storage_pools.go +++ b/lxd/db/storage_pools.go @@ -320,7 +320,7 @@ func (c *ClusterTx) CreatePendingStoragePool(ctx context.Context, node string, n } if count != 0 { - return ErrAlreadyDefined + return api.StatusErrorf(http.StatusConflict, "A storage pool already exists with name %q", name) } // Insert a node-specific entry pointing to ourselves with state storagePoolPending. From e337cfd8f08bc5bcbca1b25acfa9916fdc559b42 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:27:58 +0100 Subject: [PATCH 3/7] lxd: Replace `db.ErrAlreadyDefined` with an `api.StatusError`. Signed-off-by: Mark Laing --- lxd/storage_pools_utils.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lxd/storage_pools_utils.go b/lxd/storage_pools_utils.go index 453123f23c14..441e1269d872 100644 --- a/lxd/storage_pools_utils.go +++ b/lxd/storage_pools_utils.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "net/http" "github.com/canonical/lxd/lxd/cluster/request" "github.com/canonical/lxd/lxd/db" @@ -22,7 +23,7 @@ func storagePoolDBCreate(s *state.State, poolName string, poolDescription string return err }) if err == nil { - return -1, fmt.Errorf("The storage pool already exists: %w", db.ErrAlreadyDefined) + return -1, api.StatusErrorf(http.StatusConflict, "Storage pool %q already exists", poolName) } // Make sure that we don't pass a nil to the next function. From 4dcc17202abf2ee10cdcaff52698083096cdc5f4 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:29:38 +0100 Subject: [PATCH 4/7] lxd: Check for database conflicts using `api.StatusErrorCheck`. Signed-off-by: Mark Laing --- lxd/backup.go | 12 ++---------- lxd/networks.go | 5 ++--- lxd/storage_pools.go | 2 +- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/lxd/backup.go b/lxd/backup.go index a8d626ba968c..3dcd7ca67fc9 100644 --- a/lxd/backup.go +++ b/lxd/backup.go @@ -57,11 +57,7 @@ func backupCreate(s *state.State, args db.InstanceBackup, sourceInst instance.In return tx.CreateInstanceBackup(ctx, args) }) if err != nil { - if err == db.ErrAlreadyDefined { - return fmt.Errorf("Backup %q already exists", args.Name) - } - - return fmt.Errorf("Insert backup info into database: %w", err) + return fmt.Errorf("Failed creating instance backup record: %w", err) } revert.Add(func() { @@ -410,11 +406,7 @@ func volumeBackupCreate(s *state.State, args db.StoragePoolVolumeBackup, project return tx.CreateStoragePoolVolumeBackup(ctx, args) }) if err != nil { - if err == db.ErrAlreadyDefined { - return fmt.Errorf("Backup %q already exists", args.Name) - } - - return fmt.Errorf("Failed creating backup record: %w", err) + return fmt.Errorf("Failed creating storage volume backup record: %w", err) } revert.Add(func() { diff --git a/lxd/networks.go b/lxd/networks.go index 9f97a7e25b49..c747ed57ffde 100644 --- a/lxd/networks.go +++ b/lxd/networks.go @@ -3,7 +3,6 @@ package main import ( "context" "encoding/json" - "errors" "fmt" "net" "net/http" @@ -403,7 +402,7 @@ func networksPost(d *Daemon, r *http.Request) response.Response { return tx.CreatePendingNetwork(ctx, targetNode, projectName, req.Name, netType.DBType(), req.Config) }) if err != nil { - if err == db.ErrAlreadyDefined { + if api.StatusErrorCheck(err, http.StatusConflict) { return response.BadRequest(fmt.Errorf("The network is already defined on member %q", targetNode)) } @@ -447,7 +446,7 @@ func networksPost(d *Daemon, r *http.Request) response.Response { // Don't pass in any config, as these nodes don't have any node-specific // config and we don't want to create duplicate global config. err = tx.CreatePendingNetwork(ctx, member.Name, projectName, req.Name, netType.DBType(), nil) - if err != nil && !errors.Is(err, db.ErrAlreadyDefined) { + if err != nil && !api.StatusErrorCheck(err, http.StatusConflict) { return fmt.Errorf("Failed creating pending network for member %q: %w", member.Name, err) } } diff --git a/lxd/storage_pools.go b/lxd/storage_pools.go index 9f7148dd5c07..283836a999bb 100644 --- a/lxd/storage_pools.go +++ b/lxd/storage_pools.go @@ -340,7 +340,7 @@ func storagePoolsPost(d *Daemon, r *http.Request) response.Response { return tx.CreatePendingStoragePool(ctx, targetNode, req.Name, req.Driver, req.Config) }) if err != nil { - if err == db.ErrAlreadyDefined { + if api.StatusErrorCheck(err, http.StatusConflict) { return response.BadRequest(fmt.Errorf("The storage pool already defined on member %q", targetNode)) } From 0196303d53c79047893fb328c0b8e5180bbaf196 Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:30:57 +0100 Subject: [PATCH 5/7] lxd/instance: Check for database conflicts using `api.StatusErrorCheck`. Signed-off-by: Mark Laing --- lxd/instance/instance_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lxd/instance/instance_utils.go b/lxd/instance/instance_utils.go index 6079481688b7..7a02ec9016a4 100644 --- a/lxd/instance/instance_utils.go +++ b/lxd/instance/instance_utils.go @@ -967,7 +967,7 @@ func CreateInternal(s *state.State, args db.InstanceArgs, clearLogDir bool) (Ins return nil }) if err != nil { - if err == db.ErrAlreadyDefined { + if api.StatusErrorCheck(err, http.StatusConflict) { thing := "Instance" if shared.IsSnapshot(args.Name) { thing = "Snapshot" From df393a88b5d58109490d21b597e36c7c246a5b1c Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 10:31:21 +0100 Subject: [PATCH 6/7] lxd/response: Remove dependency on `lxd/db` from `lxd/response`. Signed-off-by: Mark Laing --- lxd/response/smart.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/lxd/response/smart.go b/lxd/response/smart.go index 554d08bd45de..3e6cbd0c2f14 100644 --- a/lxd/response/smart.go +++ b/lxd/response/smart.go @@ -6,14 +6,12 @@ import ( "net/http" "os" - "github.com/canonical/lxd/lxd/db" "github.com/canonical/lxd/shared/api" ) var httpResponseErrors = map[int][]error{ http.StatusNotFound: {os.ErrNotExist, sql.ErrNoRows}, http.StatusForbidden: {os.ErrPermission}, - http.StatusConflict: {db.ErrAlreadyDefined}, } // SmartError returns the right error message based on err. From 02b950f70d679a3fbe2d20e8abbe60303e3d5cea Mon Sep 17 00:00:00 2001 From: Mark Laing Date: Wed, 3 Apr 2024 11:25:47 +0100 Subject: [PATCH 7/7] lxd/db: Update unit tests to check for 409 Conflict. Signed-off-by: Mark Laing --- lxd/db/networks_test.go | 3 ++- lxd/db/node_test.go | 4 +++- lxd/db/storage_pools_test.go | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lxd/db/networks_test.go b/lxd/db/networks_test.go index 4a7633e745cb..34cb58378a24 100644 --- a/lxd/db/networks_test.go +++ b/lxd/db/networks_test.go @@ -4,6 +4,7 @@ package db_test import ( "context" + "net/http" "testing" "github.com/stretchr/testify/assert" @@ -94,7 +95,7 @@ func TestNetworksCreatePending_AlreadyDefined(t *testing.T) { require.NoError(t, err) err = tx.CreatePendingNetwork(context.Background(), "buzz", api.ProjectDefaultName, "network1", db.NetworkTypeBridge, map[string]string{}) - require.Equal(t, db.ErrAlreadyDefined, err) + assert.True(t, api.StatusErrorCheck(err, http.StatusConflict)) } // If no node with the given name is found, an error is returned. diff --git a/lxd/db/node_test.go b/lxd/db/node_test.go index 5c59b5503d67..4153d332ff18 100644 --- a/lxd/db/node_test.go +++ b/lxd/db/node_test.go @@ -4,6 +4,7 @@ package db_test import ( "context" + "net/http" "testing" "time" @@ -14,6 +15,7 @@ import ( "github.com/canonical/lxd/lxd/db/cluster" "github.com/canonical/lxd/lxd/db/operationtype" "github.com/canonical/lxd/lxd/response" + "github.com/canonical/lxd/shared/api" "github.com/canonical/lxd/shared/osarch" "github.com/canonical/lxd/shared/version" ) @@ -145,7 +147,7 @@ func TestRenameNode(t *testing.T) { _, err = tx.CreateNode("buzz", "5.6.7.8:666") require.NoError(t, err) err = tx.RenameNode(context.Background(), "rusp", "buzz") - assert.Equal(t, db.ErrAlreadyDefined, err) + assert.True(t, api.StatusErrorCheck(err, http.StatusConflict)) } // Remove a new raft node. diff --git a/lxd/db/storage_pools_test.go b/lxd/db/storage_pools_test.go index 9b762e218bd9..3940bd14821b 100644 --- a/lxd/db/storage_pools_test.go +++ b/lxd/db/storage_pools_test.go @@ -4,6 +4,7 @@ package db_test import ( "context" + "net/http" "testing" "time" @@ -13,6 +14,7 @@ import ( "github.com/canonical/lxd/lxd/db" "github.com/canonical/lxd/lxd/db/cluster" "github.com/canonical/lxd/lxd/response" + "github.com/canonical/lxd/shared/api" ) func init() { @@ -151,7 +153,7 @@ func TestStoragePoolsCreatePending_AlreadyDefined(t *testing.T) { require.NoError(t, err) err = tx.CreatePendingStoragePool(context.Background(), "buzz", "pool1", "dir", map[string]string{}) - require.Equal(t, db.ErrAlreadyDefined, err) + assert.True(t, api.StatusErrorCheck(err, http.StatusConflict)) } // If no node with the given name is found, an error is returned.