Merge pull request #13403 from wallyworld/better-k8s-model-message

Better error message adding a k8s model and namespace exists

apiserver/facades/client/modelmanager/modelmanager.go

@@ -613,7 +613,7 @@ func (m *ModelManagerAPI) newCAASModel(
 	cloudRegionName string,
 	cloudCredentialTag names.CloudCredentialTag,
 	ownerTag names.UserTag,
-) (common.Model, error) {
+) (_ common.Model, err error) {
 	newConfig, err := m.newModelConfig(cloudSpec, createArgs, controllerModel)
 	if err != nil {
 		return nil, errors.Annotate(err, "failed to create config")
@@ -622,6 +622,20 @@ func (m *ModelManagerAPI) newCAASModel(
 	if err != nil {
 		return nil, errors.Annotate(err, "getting controller config")
 	}
+
+	defer func() {
+		// Retain the error stack but with a better message.
+		if errors.IsAlreadyExists(err) {
+			err = errors.Wrap(err, errors.NewAlreadyExists(nil,
+				`
+the model cannot be created because a namespace with the proposed
+model name already exists in the k8s cluster.
+Please choose a different model name.
+`[1:],
+			))
+		}
+	}()
+
 	broker, err := m.getBroker(stdcontext.TODO(), environs.OpenParams{
 		ControllerUUID: controllerConfig.ControllerUUID(),
 		Cloud:          cloudSpec,
@@ -635,16 +649,6 @@ func (m *ModelManagerAPI) newCAASModel(
 		m.callContext,
 		environs.CreateParams{ControllerUUID: controllerConfig.ControllerUUID()},
 	); err != nil {
-		if errors.IsAlreadyExists(err) {
-			// Retain the error stack but with a better message.
-			return nil, errors.Wrap(err, errors.NewAlreadyExists(nil,
-				`
-the model cannot be created because a namespace with the proposed
-model name already exists in the k8s cluster.
-Please choose a different model name.
-`[1:],
-			))
-		}
 		return nil, errors.Annotatef(err, "creating namespace %q", createArgs.Name)
 	}
 

caas/kubernetes/provider/base_test.go

@@ -220,14 +220,15 @@ func (s *BaseSuite) setupController(c *gc.C) *gomock.Controller {
 	s.mockNamespaces.EXPECT().Get(gomock.Any(), s.getNamespace(), v1.GetOptions{}).Times(2).
 		Return(nil, s.k8sNotFoundError())
 
-	return s.setupBroker(c, ctrl, coretesting.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc)
+	return s.setupBroker(c, ctrl, coretesting.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc, "")
 }
 
 func (s *BaseSuite) setupBroker(
 	c *gc.C, ctrl *gomock.Controller, controllerUUID string,
 	newK8sClientFunc provider.NewK8sClientFunc,
 	newK8sRestFunc k8sspecs.NewK8sRestClientFunc,
 	randomPrefixFunc utils.RandomPrefixFunc,
+	expectErr string,
 ) *gomock.Controller {
 	s.clock = testclock.NewClock(time.Time{})
 
@@ -254,7 +255,11 @@ func (s *BaseSuite) setupBroker(
 	var err error
 	s.broker, err = provider.NewK8sBroker(controllerUUID, s.k8sRestConfig, s.cfg, s.getNamespace(), newK8sClientFunc, newK8sRestFunc,
 		watcherFn, stringsWatcherFn, randomPrefixFunc, s.clock)
-	c.Assert(err, jc.ErrorIsNil)
+	if expectErr == "" {
+		c.Assert(err, jc.ErrorIsNil)
+	} else {
+		c.Assert(err, gc.ErrorMatches, expectErr)
+	}
 	return ctrl
 }
 

caas/kubernetes/provider/k8s.go

@@ -215,6 +215,9 @@ func newK8sBroker(
 		isLegacyLabels: isLegacy,
 	}
 	if err := client.ensureNamespaceAnnotationForControllerUUID(controllerUUID, isLegacy); err != nil {
+		if errors.IsNotFound(err) {
+			return nil, errors.NewAlreadyExists(nil, fmt.Sprintf("namespace %q may already be in use", cfg.Name()))
+		}
 		return nil, errors.Trace(err)
 	}
 	return client, nil

caas/kubernetes/provider/k8s_test.go

@@ -820,7 +820,7 @@ func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDMigrated(
 		s.mockNamespaces.EXPECT().Update(gomock.Any(), &nsAfter, v1.UpdateOptions{}).Times(1).
 			Return(&nsAfter, nil),
 	)
-	s.setupBroker(c, ctrl, newControllerUUID, newK8sClientFunc, newK8sRestFunc, randomPrefixFunc).Finish()
+	s.setupBroker(c, ctrl, newControllerUUID, newK8sClientFunc, newK8sRestFunc, randomPrefixFunc, "").Finish()
 }
 
 func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDNotMigrated(c *gc.C) {
@@ -841,7 +841,7 @@ func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDNotMigrat
 		s.mockNamespaces.EXPECT().Get(gomock.Any(), s.getNamespace(), v1.GetOptions{}).Times(2).
 			Return(ns, nil),
 	)
-	s.setupBroker(c, ctrl, testing.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc).Finish()
+	s.setupBroker(c, ctrl, testing.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc, "").Finish()
 }
 
 func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDNameSpaceNotCreatedYet(c *gc.C) {
@@ -856,7 +856,26 @@ func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDNameSpace
 		s.mockNamespaces.EXPECT().Get(gomock.Any(), s.getNamespace(), v1.GetOptions{}).Times(2).
 			Return(nil, s.k8sNotFoundError()),
 	)
-	s.setupBroker(c, ctrl, testing.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc).Finish()
+	s.setupBroker(c, ctrl, testing.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc, "").Finish()
+}
+
+func (s *K8sBrokerSuite) TestEnsureNamespaceAnnotationForControllerUUIDNameSpaceExists(c *gc.C) {
+	ctrl := gomock.NewController(c)
+
+	newK8sClientFunc, newK8sRestFunc := s.setupK8sRestClient(c, ctrl, s.getNamespace())
+	randomPrefixFunc := func() (string, error) {
+		return "appuuid", nil
+	}
+
+	gomock.InOrder(
+		s.mockNamespaces.EXPECT().Get(gomock.Any(), s.getNamespace(), v1.GetOptions{}).Times(2).
+			Return(&core.Namespace{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "test",
+				},
+			}, nil),
+	)
+	s.setupBroker(c, ctrl, testing.ControllerTag.Id(), newK8sClientFunc, newK8sRestFunc, randomPrefixFunc, `namespace "test" may already be in use`).Finish()
 }
 
 func (s *K8sBrokerSuite) TestFileSetToVolumeFiles(c *gc.C) {

caas/kubernetes/provider/namespaces.go

@@ -5,6 +5,7 @@ package provider
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/juju/errors"
 	core "k8s.io/api/core/v1"
@@ -124,7 +125,7 @@ func (k *kubernetesClient) createNamespace(name string) error {
 
 	if err := k.ensureNamespaceAnnotations(ns); err != nil {
 		if errors.IsNotFound(err) {
-			return errors.Annotatef(err, "namespace may already be in use")
+			return errors.NewAlreadyExists(nil, fmt.Sprintf("namespace %q may already be in use", name))
 		}
 		return errors.Trace(err)
 	}