Various fixes, better validation, txn closures

domain/storage/errors/errors.go

@@ -9,7 +9,13 @@ import (
 
 const (
 	// MissingPoolTypeError is used when a provider type is empty.
-	MissingPoolTypeError = errors.ConstError("pool provider type is missing")
+	MissingPoolTypeError = errors.ConstError("pool provider type is empty")
 	// MissingPoolNameError is used when a name is empty.
-	MissingPoolNameError = errors.ConstError("pool name is missing")
+	MissingPoolNameError = errors.ConstError("pool name is empty")
+
+	InvalidPoolNameError = errors.ConstError("pool name is not valid")
+
+	PoolNotFoundError = errors.ConstError("storage pool is not found")
+
+	PoolAlreadyExists = errors.ConstError("storage pool already exists")
 )

domain/storage/service/service.go

@@ -51,13 +51,6 @@ type PoolAttrs map[string]any
 // CreateStoragePool creates a storage pool, returning an error satisfying [errors.AlreadyExists]
 // if a pool with the same name already exists.
 func (s *Service) CreateStoragePool(ctx context.Context, name string, providerType storage.ProviderType, attrs PoolAttrs) error {
-	if name == "" {
-		return storageerrors.MissingPoolNameError
-	}
-	if providerType == "" {
-		return storageerrors.MissingPoolTypeError
-	}
-
 	err := s.validateConfig(name, providerType, attrs)
 	if err != nil {
 		return errors.Trace(err)
@@ -74,9 +67,19 @@ func (s *Service) CreateStoragePool(ctx context.Context, name string, providerTy
 }
 
 func (s *Service) validateConfig(name string, providerType storage.ProviderType, attrs map[string]interface{}) error {
+	if name == "" {
+		return storageerrors.MissingPoolNameError
+	}
+	if !storage.IsValidPoolName(name) {
+		return fmt.Errorf("pool name %q not valid%w", name, errors.Hide(storageerrors.InvalidPoolNameError))
+	}
+	if providerType == "" {
+		return storageerrors.MissingPoolTypeError
+	}
 	if s.registry == nil {
-		return errors.New("cannot validate storage provider config without a registry")
+		return errors.Errorf("cannot validate storage provider config for %q without a registry", name)
 	}
+
 	cfg, err := storage.NewConfig(name, providerType, attrs)
 	if err != nil {
 		return errors.Trace(err)
@@ -126,7 +129,7 @@ func (s *Service) DeleteStoragePool(ctx context.Context, name string) error {
 }
 
 // ReplaceStoragePool replaces an existing storage pool, returning an error
-// satisfying [errors.NotFound] if a pool with the name does not exist.
+// satisfying [storageerrors.PoolNotFoundError] if a pool with the name does not exist.
 func (s *Service) ReplaceStoragePool(ctx context.Context, name string, providerType storage.ProviderType, attrs PoolAttrs) error {
 	// Use the existing provider type unless explicitly overwritten.
 	if providerType == "" {
@@ -185,7 +188,7 @@ func (a *Service) validatePoolListFilter(filter domainstorage.StoragePoolFilter)
 func (a *Service) validateNameCriteria(names []string) error {
 	for _, n := range names {
 		if !storage.IsValidPoolName(n) {
-			return errors.NotValidf("pool name %q", n)
+			return fmt.Errorf("pool name %q not valid%w", n, errors.Hide(storageerrors.InvalidPoolNameError))
 		}
 	}
 	return nil
@@ -205,8 +208,11 @@ func (s *Service) validateProviderCriteria(providers []string) error {
 }
 
 // GetStoragePoolByName returns the storage pool with the specified name, returning an error
-// satisfying [errors.NotFound] if it doesn't exist.
+// satisfying [storageerrors.PoolNotFoundError] if it doesn't exist.
 func (s *Service) GetStoragePoolByName(ctx context.Context, name string) (*storage.Config, error) {
+	if !storage.IsValidPoolName(name) {
+		return nil, fmt.Errorf("pool name %q not valid%w", name, errors.Hide(storageerrors.InvalidPoolNameError))
+	}
 	sp, err := s.st.GetStoragePoolByName(ctx, name)
 	if err != nil {
 		return nil, errors.Trace(err)

domain/storage/service/service_test.go

@@ -5,7 +5,6 @@ package service
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/juju/errors"
 	"github.com/juju/loggo/v2"
@@ -15,6 +14,7 @@ import (
 	gc "gopkg.in/check.v1"
 
 	domainstorage "github.com/juju/juju/domain/storage"
+	storageerrors "github.com/juju/juju/domain/storage/errors"
 	"github.com/juju/juju/internal/storage"
 	dummystorage "github.com/juju/juju/internal/storage/provider/dummy"
 )
@@ -28,6 +28,8 @@ type serviceSuite struct {
 
 var _ = gc.Suite(&serviceSuite{})
 
+const validationError = errors.ConstError("missing attribute foo")
+
 func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller {
 	ctrl := gomock.NewController(c)
 
@@ -38,7 +40,7 @@ func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller {
 			"ebs": &dummystorage.StorageProvider{
 				ValidateConfigFunc: func(sp *storage.Config) error {
 					if _, ok := sp.Attrs()["foo"]; !ok {
-						return fmt.Errorf("missing attribute foo")
+						return validationError
 					}
 					return nil
 				},
@@ -69,10 +71,32 @@ func (s *serviceSuite) TestCreateStoragePool(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *serviceSuite) TestCreateStoragePoolInvalidName(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	err := s.service().CreateStoragePool(context.Background(), "66invalid", "ebs", PoolAttrs{"foo": "foo val"})
+	c.Assert(err, jc.ErrorIs, storageerrors.InvalidPoolNameError)
+}
+
+func (s *serviceSuite) TestCreateStoragePoolMissingName(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	err := s.service().CreateStoragePool(context.Background(), "", "ebs", PoolAttrs{"foo": "foo val"})
+	c.Assert(err, jc.ErrorIs, storageerrors.MissingPoolNameError)
+}
+
+func (s *serviceSuite) TestCreateStoragePoolMissingType(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	err := s.service().CreateStoragePool(context.Background(), "ebs-fast", "", PoolAttrs{"foo": "foo val"})
+	c.Assert(err, jc.ErrorIs, storageerrors.MissingPoolTypeError)
+}
+
 func (s *serviceSuite) TestCreateStoragePoolValidates(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
 	err := s.service().CreateStoragePool(context.Background(), "ebs-fast", "ebs", PoolAttrs{"bar": "bar val"})
+	c.Assert(err, jc.ErrorIs, validationError)
 	c.Assert(err, gc.ErrorMatches, `.* missing attribute foo`)
 }
 
@@ -101,6 +125,20 @@ func (s *serviceSuite) TestReplaceStoragePool(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *serviceSuite) TestReplaceStoragePoolInvalidName(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	err := s.service().ReplaceStoragePool(context.Background(), "66invalid", "ebs", PoolAttrs{"foo": "foo val"})
+	c.Assert(err, jc.ErrorIs, storageerrors.InvalidPoolNameError)
+}
+
+func (s *serviceSuite) TestReplaceStoragePoolMissingName(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	err := s.service().ReplaceStoragePool(context.Background(), "", "ebs", PoolAttrs{"foo": "foo val"})
+	c.Assert(err, jc.ErrorIs, storageerrors.MissingPoolNameError)
+}
+
 func (s *serviceSuite) TestReplaceStoragePoolExistingProvider(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
@@ -122,6 +160,7 @@ func (s *serviceSuite) TestReplaceStoragePoolValidates(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
 	err := s.service().ReplaceStoragePool(context.Background(), "ebs-fast", "ebs", PoolAttrs{"bar": "bar val"})
+	c.Assert(err, jc.ErrorIs, validationError)
 	c.Assert(err, gc.ErrorMatches, `.* missing attribute foo`)
 }
 
@@ -175,7 +214,7 @@ func (s *serviceSuite) TestListStoragePoolsInvalidFilterName(c *gc.C) {
 	_, err := s.service().ListStoragePools(context.Background(), domainstorage.StoragePoolFilter{
 		Names: []string{"666invalid"},
 	})
-	c.Assert(err, jc.ErrorIs, errors.NotValid)
+	c.Assert(err, jc.ErrorIs, storageerrors.InvalidPoolNameError)
 	c.Assert(err, gc.ErrorMatches, `pool name "666invalid" not valid`)
 }