Merge pull request #10523 from anastasiamac/addk8s-input-validation

#10523

## Description of change

'add-k8s' tries to detect cloud/region. However, for the cases when the command cannot detect the cloud, it relies on user input.

Previously, if the user supplied cloud/region but the command successfully detected what cloud/region to use, user input was ignored and not even validated. 

This PR checks user input when cloud/region is detected and if there is a mismatch, the user is prompted.

As a drive-by:
* Ensure that the command no longer ignores --local
* When cloud region validation fails, we list available regions. However, some clouds may not have regions (see lp#1819409) and displaying empty list is not helpful. This PR displays a better message. 

## Bug reference

https://bugs.launchpad.net/juju/+bug/1830949
https://bugs.launchpad.net/juju/+bug/1839898

cloud/clouds.go

@@ -302,9 +302,13 @@ func RegionByName(regions []Region, name string) (*Region, error) {
 		}
 		return &region, nil
 	}
+	availableRegions := "cloud has no regions"
+	if len(regions) > 0 {
+		availableRegions = fmt.Sprintf("expected one of %q", RegionNames(regions))
+	}
 	return nil, errors.NewNotFound(nil, fmt.Sprintf(
-		"region %q not found (expected one of %q)",
-		name, RegionNames(regions),
+		"region %q not found (%v)",
+		name, availableRegions,
 	))
 }
 

cloud/clouds_test.go

@@ -6,7 +6,9 @@ package cloud_test
 import (
 	"io/ioutil"
 	"path/filepath"
+	"regexp"
 
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -329,3 +331,35 @@ ca-certificates: [fakecacert]
 		CACertificates: []string{"fakecacert"},
 	})
 }
+
+func (s *cloudSuite) TestRegionByNameNoRegions(c *gc.C) {
+	r, err := cloud.RegionByName([]cloud.Region{}, "star")
+	c.Assert(r, gc.IsNil)
+	c.Assert(err, gc.ErrorMatches, regexp.QuoteMeta(`region "star" not found (cloud has no regions)`))
+}
+
+func (s *cloudSuite) TestRegionByName(c *gc.C) {
+	regions := []cloud.Region{
+		{Name: "mars"},
+		{Name: "earth"},
+		{Name: "jupiter"},
+	}
+
+	r, err := cloud.RegionByName(regions, "mars")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(r, gc.Not(gc.IsNil))
+	c.Assert(r, gc.DeepEquals, &cloud.Region{Name: "mars"})
+}
+
+func (s *cloudSuite) TestRegionByNameNotFound(c *gc.C) {
+	regions := []cloud.Region{
+		{Name: "mars"},
+		{Name: "earth"},
+		{Name: "jupiter"},
+	}
+
+	r, err := cloud.RegionByName(regions, "star")
+	c.Assert(err, gc.ErrorMatches, regexp.QuoteMeta(`region "star" not found (expected one of ["earth" "jupiter" "mars"])`))
+	c.Assert(err, jc.Satisfies, errors.IsNotFound)
+	c.Assert(r, gc.IsNil)
+}

cmd/juju/caas/add.go

@@ -141,6 +141,9 @@ type AddCAASCommand struct {
 	// cloud is an alias of the hostCloudRegion.
 	cloud string
 
+	// givenHostCloudRegion holds a copy of cloud name/type and/or region as supplied by the user via options.
+	givenHostCloudRegion string
+
 	// workloadStorage is a storage class specified by the user.
 	workloadStorage string
 
@@ -228,6 +231,8 @@ func (c *AddCAASCommand) Init(args []string) (err error) {
 		if err != nil {
 			return errors.Trace(err)
 		}
+		// Keep a copy of the original user supplied value for comparison and validation later.
+		c.givenHostCloudRegion = c.hostCloudRegion
 	}
 
 	if c.gke {
@@ -322,6 +327,7 @@ func (c *AddCAASCommand) getGKEKubeConfig(ctx *cmd.Context) (io.Reader, string,
 func (c *AddCAASCommand) getAKSKubeConfig(ctx *cmd.Context) (io.Reader, string, error) {
 	p := &clusterParams{
 		name:          c.clusterName,
+		region:        c.hostCloudRegion,
 		resourceGroup: c.resourceGroup,
 	}
 
@@ -421,6 +427,11 @@ func (c *AddCAASCommand) Run(ctx *cmd.Context) (err error) {
 	if err != nil {
 		return errors.Trace(err)
 	}
+	// By this stage, we know if cloud name/type and/or region input is needed from the user.
+	// If we could not detect it, check what was provided.
+	if err := checkCloudRegion(c.givenHostCloudRegion, newCloud.HostCloudRegion); err != nil {
+		return errors.Trace(err)
+	}
 
 	if err := addCloudToLocal(c.cloudMetadataStore, newCloud); err != nil {
 		return errors.Trace(err)
@@ -433,7 +444,7 @@ func (c *AddCAASCommand) Run(ctx *cmd.Context) (err error) {
 	if clusterName == "" {
 		clusterName = newCloud.HostCloudRegion
 	}
-	if c.controllerName == "" {
+	if c.Local {
 		successMsg := fmt.Sprintf("k8s substrate %q added as cloud %q%s", clusterName, c.caasName, storageMsg)
 		successMsg += fmt.Sprintf("\nYou can now bootstrap to this cloud by running 'juju bootstrap %s'.", c.caasName)
 		fmt.Fprintln(ctx.Stdout, successMsg)
@@ -458,12 +469,32 @@ func (c *AddCAASCommand) Run(ctx *cmd.Context) (err error) {
 	return nil
 }
 
+func checkCloudRegion(given, detected string) error {
+	if given == "" {
+		// User provided no host cloud/region information.
+		return nil
+	}
+	givenCloud, givenRegion, _ := jujucloud.SplitHostCloudRegion(given)
+	detectedCloud, detectedRegion, _ := jujucloud.SplitHostCloudRegion(detected)
+	if givenCloud != "" && givenCloud != detectedCloud {
+		if givenRegion != "" || givenCloud != detectedRegion {
+			// If givenRegion is empty, then givenCloud may be a region.
+			// Check that it is not a region.
+			return errors.Errorf("specified cloud %q was different to the detected cloud %q: re-run the command without specifying the cloud", givenCloud, detectedCloud)
+		}
+	}
+	if givenRegion != "" && givenRegion != detectedRegion {
+		return errors.Errorf("specified region %q was different to the detected region %q: re-run the command without specifying the region", givenRegion, detectedRegion)
+	}
+	return nil
+}
+
 func (c *AddCAASCommand) newK8sClusterBroker(cloud jujucloud.Cloud, credential jujucloud.Credential) (caas.ClusterMetadataChecker, error) {
 	openParams, err := provider.BaseKubeCloudOpenParams(cloud, credential)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	if c.controllerName != "" {
+	if !c.Local {
 		ctrlUUID, err := c.ControllerUUID(c.store, c.controllerName)
 		if err != nil {
 			return nil, errors.Trace(err)

cmd/juju/caas/add_test.go

@@ -813,6 +813,32 @@ func (s *addCAASSuite) TestAddGkeCluster(c *gc.C) {
 	s.assertStoreClouds(c, "gce/us-east1")
 }
 
+func (s *addCAASSuite) TestGivenCloudMatch(c *gc.C) {
+	err := caas.CheckCloudRegion("gce", "gce/us-east1")
+	c.Assert(err, jc.ErrorIsNil)
+}
+
+func (s *addCAASSuite) TestGivenCloudMismatch(c *gc.C) {
+	err := caas.CheckCloudRegion("maas", "gce")
+	c.Assert(err, gc.ErrorMatches, `specified cloud "maas" was different to the detected cloud "gce": re-run the command without specifying the cloud`)
+
+	err = caas.CheckCloudRegion("maas", "gce/us-east1")
+	c.Assert(err, gc.ErrorMatches, `specified cloud "maas" was different to the detected cloud "gce": re-run the command without specifying the cloud`)
+}
+
+func (s *addCAASSuite) TestGivenRegionMatch(c *gc.C) {
+	err := caas.CheckCloudRegion("/us-east1", "gce/us-east1")
+	c.Assert(err, jc.ErrorIsNil)
+
+	err = caas.CheckCloudRegion("us-east1", "gce/us-east1")
+	c.Assert(err, jc.ErrorIsNil)
+}
+
+func (s *addCAASSuite) TestGivenRegionMismatch(c *gc.C) {
+	err := caas.CheckCloudRegion("gce/us-east1", "gce/us-east10")
+	c.Assert(err, gc.ErrorMatches, `specified region "us-east1" was different to the detected region "us-east10": re-run the command without specifying the region`)
+}
+
 func (s *addCAASSuite) assertStoreClouds(c *gc.C, hostCloud string) {
 	s.cloudMetadataStore.CheckCall(c, 2, "WritePersonalCloudMetadata",
 		map[string]cloud.Cloud{

cmd/juju/caas/export_test.go

@@ -85,3 +85,7 @@ func (f *fakeCluster) ensureExecutable() error {
 func FakeCluster(config string) k8sCluster {
 	return &fakeCluster{config: config, cloudType: "gce"}
 }
+
+var (
+	CheckCloudRegion = checkCloudRegion
+)

cmd/juju/controller/addmodel.go

@@ -94,10 +94,7 @@ are the ones used to create any future resources within the model.
 If no cloud/region is specified, then the model will be deployed to
 the same cloud/region as the controller model. If a region is specified
 without a cloud qualifier, then it is assumed to be in the same cloud
-as the controller model. It is not currently possible for a controller
-to manage multiple clouds, so the only valid cloud is the same cloud
-as the controller model is deployed to. This may change in a future
-release.
+as the controller model. 
 
 Examples: