Use different registered app for azure credentials; allow use of cust…

…om resource groups
orbital-transfer-survey · Jul 10, 2020 · 625bd31 · 625bd31
1 parent e0dc334
commit 625bd31
Show file tree

Hide file tree

Showing 12 changed files with 271 additions and 61 deletions.
diff --git a/cloudconfig/instancecfg/instancecfg.go b/cloudconfig/instancecfg/instancecfg.go
@@ -749,9 +749,6 @@ func (cfg *BootstrapConfig) VerifyConfig() (err error) {
 	if cfg.BootstrapMachineInstanceId == "" {
 		return errors.New("missing bootstrap machine instance ID")
 	}
-	if len(cfg.HostedModelConfig) == 0 {
-		return errors.New("missing hosted model config")
-	}
 	return nil
 }
 

diff --git a/cmd/juju/cloud/detectcredentials.go b/cmd/juju/cloud/detectcredentials.go
@@ -148,6 +148,7 @@ type discoveredCredential struct {
 	credentialName   string
 	credential       jujucloud.Credential
 	isNew            bool
+	isDefault        bool
 }
 
 func (c *detectCredentialsCommand) credentialsAPI() (CredentialAPI, error) {
@@ -259,9 +260,14 @@ func (c *detectCredentialsCommand) Run(ctxt *cmd.Context) error {
 			if errors.IsNotFound(err) || len(detected.AuthCredentials) == 0 {
 				continue
 			}
+			// Some providers, eg Azure can have spaces in cred names and we don't want that.
+			detected.DefaultCredential = strings.ReplaceAll(detected.DefaultCredential, " ", "_")
 			sortedName := []string{}
-			for credName := range detected.AuthCredentials {
-				sortedName = append(sortedName, credName)
+			for credName, cred := range detected.AuthCredentials {
+				formattedCredName := strings.ReplaceAll(credName, " ", "_")
+				sortedName = append(sortedName, formattedCredName)
+				delete(detected.AuthCredentials, credName)
+				detected.AuthCredentials[formattedCredName] = cred
 			}
 			naturalsort.Sort(sortedName)
 
@@ -286,6 +292,7 @@ func (c *detectCredentialsCommand) Run(ctxt *cmd.Context) error {
 					cloudType:      providerName,
 					credentialName: credName,
 					credential:     newCred,
+					isDefault:      detected.DefaultCredential == credName,
 				}
 
 				// Fill in the default cloud and other meta information.
@@ -425,6 +432,9 @@ func (c *detectCredentialsCommand) interactiveCredentialsUpdate(ctxt *cmd.Contex
 		if cred.region != "" {
 			existing.DefaultRegion = cred.region
 		}
+		if existing.DefaultCredential == "" && cred.isDefault {
+			existing.DefaultCredential = cred.credentialName
+		}
 		existing.AuthCredentials[cred.credentialName] = cred.credential
 		addLoadedCredential(loaded, cloudName, cred)
 		if c.Client {

diff --git a/cmd/juju/cloud/detectcredentials_test.go b/cmd/juju/cloud/detectcredentials_test.go
@@ -159,7 +159,7 @@ func (s *detectCredentialsSuite) assertDetectCredential(c *gc.C, t detectCredent
 	s.aCredential = jujucloud.CloudCredential{
 		DefaultRegion: "default region",
 		AuthCredentials: map[string]jujucloud.Credential{
-			"test": s.credentialWithLabel(jujucloud.AccessKeyAuthType, "credential")},
+			"test cred": s.credentialWithLabel(jujucloud.AccessKeyAuthType, "credential")},
 	}
 	clouds := map[string]jujucloud.Cloud{
 		"test-cloud": {
@@ -194,7 +194,7 @@ func (s *detectCredentialsSuite) TestDetectCredentialOverwrites(c *gc.C) {
 	s.store.Credentials = map[string]jujucloud.CloudCredential{
 		"test-cloud": {
 			AuthCredentials: map[string]jujucloud.Credential{
-				"test": jujucloud.NewCredential(jujucloud.AccessKeyAuthType, nil),
+				"test_cred": jujucloud.NewCredential(jujucloud.AccessKeyAuthType, nil),
 			},
 		},
 	}
@@ -206,7 +206,7 @@ func (s *detectCredentialsSuite) TestDetectCredentialKeepsExistingRegion(c *gc.C
 		"test-cloud": {
 			DefaultRegion: "west",
 			AuthCredentials: map[string]jujucloud.Credential{
-				"test": jujucloud.NewCredential(jujucloud.AccessKeyAuthType, nil),
+				"test_cred": jujucloud.NewCredential(jujucloud.AccessKeyAuthType, nil),
 			},
 		},
 	}

diff --git a/cmd/juju/commands/bootstrap.go b/cmd/juju/commands/bootstrap.go
@@ -195,14 +195,16 @@ type bootstrapCommand struct {
 	showClouds          bool
 	showRegionsForCloud string
 	controllerName      string
-	hostedModelName     string
 	CredentialName      string
 	Cloud               string
 	Region              string
 	noGUI               bool
 	noSwitch            bool
 	interactive         bool
 
+	hostedModelName string
+	noHostedModel   bool
+
 	// Force is used to allow a bootstrap to be run on unsupported series.
 	Force bool
 }
@@ -276,6 +278,7 @@ func (c *bootstrapCommand) SetFlags(f *gnuflag.FlagSet) {
 	f.BoolVar(&c.noGUI, "no-gui", false, "Do not install the Juju GUI in the controller when bootstrapping")
 	f.BoolVar(&c.noSwitch, "no-switch", false, "Do not switch to the newly created controller")
 	f.BoolVar(&c.Force, "force", false, "Allow the bypassing of checks such as supported series")
+	f.BoolVar(&c.noHostedModel, "no-default-model", false, "Do not create a default model")
 }
 
 func (c *bootstrapCommand) Init(args []string) (err error) {
@@ -468,6 +471,9 @@ func (c *bootstrapCommand) initializeHostedModel(
 	environ environs.BootstrapEnviron,
 	bootstrapParams *bootstrap.BootstrapParams,
 ) (*jujuclient.ModelDetails, error) {
+	if c.noHostedModel {
+		return nil, nil
+	}
 	if isCAASController && c.hostedModelName == defaultHostedModelName {
 		// k8s controller does NOT have "default" hosted model
 		// if the user didn't specify a preferred hosted model name.
@@ -518,16 +524,22 @@ func (c *bootstrapCommand) initializeHostedModel(
 // the user is informed how to create one.
 func (c *bootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 	var hostedModel *jujuclient.ModelDetails
+	var isCAASController bool
 	defer func() {
 		resultErr = handleChooseCloudRegionError(ctx, resultErr)
 		if !c.showClouds && resultErr == nil {
 			var msg string
 			if hostedModel == nil {
-				msg = `
+				workloadType := ""
+				if isCAASController {
+					workloadType = "k8s "
+				}
+				msg = fmt.Sprintf(`
 Now you can run
 	juju add-model <model-name>
-to create a new model to deploy k8s workloads.
-`
+to create a new model to deploy %sworkloads.
+`, workloadType)
+
 			} else {
 				msg = fmt.Sprintf("Initial model %q added", c.hostedModelName)
 			}
@@ -581,7 +593,7 @@ to create a new model to deploy k8s workloads.
 		}
 	}
 
-	isCAASController := jujucloud.CloudIsCAAS(cloud)
+	isCAASController = jujucloud.CloudIsCAAS(cloud)
 
 	// Custom clouds may not have explicitly declared support for any auth-
 	// types, in which case we'll assume that they support everything that
@@ -1337,6 +1349,17 @@ func (c *bootstrapCommand) bootstrapConfigs(
 	if err := common.FinalizeAuthorizedKeys(ctx, bootstrapModelConfig); err != nil {
 		return bootstrapConfigs{}, errors.Annotate(err, "finalizing authorized-keys")
 	}
+
+	// We need to do an Azure specific check here.
+	// This won't be needed once the "default" model is banished.
+	// Until it is, we need to ensure that if a resource-group-name is specified,
+	// the user has also disabled the default model, otherwise we end up with 2
+	// models with the same resource group name.
+	resourceGroupName, ok := bootstrapModelConfig["resource-group-name"]
+	if ok && resourceGroupName != "" && !c.noHostedModel {
+		return bootstrapConfigs{}, errors.Errorf("if using resource-group-name %q then --no-default-model is required as well", resourceGroupName)
+	}
+
 	logger.Debugf("preparing controller with config: %v", bootstrapModelConfig)
 
 	configs := bootstrapConfigs{

diff --git a/cmd/juju/commands/bootstrap_test.go b/cmd/juju/commands/bootstrap_test.go
@@ -464,6 +464,10 @@ var bootstrapTests = []bootstrapTest{{
 	info: "controller name cannot be set via config",
 	args: []string{"--config", "controller-name=test"},
 	err:  `controller name cannot be set via config, please use cmd args`,
+}, {
+	info: "resource-group-name needs --no-default-model",
+	args: []string{"--config", "resource-group-name=foo"},
+	err:  `if using resource-group-name "foo" then --no-default-model is required as well`,
 }}
 
 func (s *BootstrapSuite) TestRunCloudNameUnknown(c *gc.C) {
@@ -614,6 +618,25 @@ func (s *BootstrapSuite) TestBootstrapDefaultModel(c *gc.C) {
 	c.Assert(bootstrapFuncs.args.HostedModelConfig["foo"], gc.Equals, "bar")
 }
 
+func (s *BootstrapSuite) TestBootstrapNoDefaultModel(c *gc.C) {
+	s.patchVersionAndSeries(c, "raring")
+
+	var bootstrapFuncs fakeBootstrapFuncs
+	s.PatchValue(&getBootstrapFuncs, func() BootstrapInterface {
+		return &bootstrapFuncs
+	})
+
+	cmdtesting.RunCommand(
+		c, s.newBootstrapCommand(),
+		"dummy", "devcontroller",
+		"--auto-upgrade",
+		"--no-default-model",
+		"--config", "foo=bar",
+	)
+	c.Assert(utils.IsValidUUIDString(bootstrapFuncs.args.ControllerConfig.ControllerUUID()), jc.IsTrue)
+	c.Assert(bootstrapFuncs.args.HostedModelConfig, gc.HasLen, 0)
+}
+
 func (s *BootstrapSuite) TestBootstrapTimeout(c *gc.C) {
 	s.patchVersionAndSeries(c, "raring")
 

diff --git a/provider/azure/config.go b/provider/azure/config.go
@@ -25,6 +25,10 @@ const (
 	// configAttrLoadBalancerSkuName mirrors the LoadBalancerSkuName type in the Azure SDK
 	configAttrLoadBalancerSkuName = "load-balancer-sku-name"
 
+	// configResourceGroupName specifies an existing resource group to use
+	// rather than Juju creating it.
+	configResourceGroupName = "resource-group-name"
+
 	// The below bits are internal book-keeping things, rather than
 	// configuration. Config is just what we have to work with.
 
@@ -36,21 +40,25 @@ const (
 var configFields = schema.Fields{
 	configAttrStorageAccountType:  schema.String(),
 	configAttrLoadBalancerSkuName: schema.String(),
+	configResourceGroupName:       schema.String(),
 }
 
 var configDefaults = schema.Defaults{
 	configAttrStorageAccountType:  string(storage.StandardLRS),
 	configAttrLoadBalancerSkuName: string(network.LoadBalancerSkuNameStandard),
+	configResourceGroupName:       schema.Omit,
 }
 
 var immutableConfigAttributes = []string{
 	configAttrStorageAccountType,
+	configResourceGroupName,
 }
 
 type azureModelConfig struct {
 	*config.Config
 	storageAccountType  string
 	loadBalancerSkuName string
+	resourceGroupName   string
 }
 
 // knownStorageAccountTypes returns a list of valid storage SKU names.
@@ -119,16 +127,30 @@ func validateConfig(newCfg, oldCfg *config.Config) (*azureModelConfig, error) {
 	// Resource group names must not exceed 80 characters. Resource group
 	// names are based on the model UUID and model name, the latter of
 	// which the model creator controls.
-	modelTag := names.NewModelTag(newCfg.UUID())
-	resourceGroup := resourceGroupName(modelTag, newCfg.Name())
-	if n := len(resourceGroup); n > resourceNameLengthMax {
-		smallestResourceGroup := resourceGroupName(modelTag, "")
-		return nil, errors.Errorf(`resource group name %q is too long
+	var userSpecifiedResourceGroup string
+	resourceGroup, ok := validated[configResourceGroupName].(string)
+	if ok {
+		userSpecifiedResourceGroup = resourceGroup
+		if len(resourceGroup) > resourceNameLengthMax {
+			return nil, errors.Errorf(`resource group name %q is too long
+
+Please choose a name of no more than %d characters.`,
+				resourceGroup,
+				resourceNameLengthMax,
+			)
+		}
+	} else {
+		modelTag := names.NewModelTag(newCfg.UUID())
+		resourceGroup = resourceGroupName(modelTag, newCfg.Name())
+		if n := len(resourceGroup); n > resourceNameLengthMax {
+			smallestResourceGroup := resourceGroupName(modelTag, "")
+			return nil, errors.Errorf(`resource group name %q is too long
 
 Please choose a model name of no more than %d characters.`,
-			resourceGroup,
-			resourceNameLengthMax-len(smallestResourceGroup),
-		)
+				resourceGroup,
+				resourceNameLengthMax-len(smallestResourceGroup),
+			)
+		}
 	}
 
 	if newCfg.FirewallMode() == config.FwGlobal {
@@ -164,6 +186,7 @@ Please choose a model name of no more than %d characters.`,
 		newCfg,
 		storageAccountType,
 		loadBalancerSkuName,
+		userSpecifiedResourceGroup,
 	}
 	return azureConfig, nil
 }

diff --git a/provider/azure/config_test.go b/provider/azure/config_test.go
@@ -68,6 +68,14 @@ func (s *configSuite) TestValidateModelNameLength(c *gc.C) {
 Please choose a model name of no more than 32 characters.`)
 }
 
+func (s *configSuite) TestValidateResourceGroupNameLength(c *gc.C) {
+	s.assertConfigInvalid(
+		c, testing.Attrs{"resource-group-name": "someextremelyoverlylongishresourcegroupname-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"},
+		`resource group name "someextremelyoverlylongishresourcegroupname-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" is too long
+
+Please choose a name of no more than 80 characters.`)
+}
+
 func (s *configSuite) TestValidateStorageAccountTypeCantChange(c *gc.C) {
 	cfgOld := makeTestModelConfig(c, testing.Attrs{"storage-account-type": "Standard_LRS"})
 	_, err := s.provider.Validate(cfgOld, cfgOld)
@@ -91,6 +99,16 @@ func (s *configSuite) TestValidateLoadBalancerSkuNameCanChange(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *configSuite) TestValidateResourceGroupNameCantChange(c *gc.C) {
+	cfgOld := makeTestModelConfig(c, testing.Attrs{"resource-group-name": "foo"})
+	_, err := s.provider.Validate(cfgOld, cfgOld)
+	c.Assert(err, jc.ErrorIsNil)
+
+	cfgNew := makeTestModelConfig(c, testing.Attrs{"resource-group-name": "bar"})
+	_, err = s.provider.Validate(cfgNew, cfgOld)
+	c.Assert(err, gc.ErrorMatches, `cannot change immutable "resource-group-name" config \(foo -> bar\)`)
+}
+
 func (s *configSuite) assertConfigValid(c *gc.C, attrs testing.Attrs) {
 	cfg := makeTestModelConfig(c, attrs)
 	_, err := s.provider.Validate(cfg, nil)

diff --git a/provider/azure/credentials.go b/provider/azure/credentials.go
@@ -21,7 +21,6 @@ import (
 const (
 	credAttrAppId          = "application-id"
 	credAttrSubscriptionId = "subscription-id"
-	credAttrTenantId       = "tenant-id"
 	credAttrAppPassword    = "application-password"
 
 	// clientCredentialsAuthType is the auth-type for the
@@ -116,21 +115,14 @@ func (c environProviderCredentials) DetectCredentials() (*cloud.CloudCredential,
 	}
 	var defaultCredential string
 	authCredentials := make(map[string]cloud.Credential)
-	for i, acc := range accounts {
+	for _, acc := range accounts {
 		cloudInfo, ok := cloudMap[acc.CloudName]
 		if !ok {
 			continue
 		}
 		cred, err := c.accountCredential(acc, cloudInfo)
 		if err != nil {
 			logger.Debugf("cannot get credential for %s: %s", acc.Name, err)
-			if i == 0 {
-				// Assume that if this fails the first
-				// time then it will always fail and
-				// don't attempt to create any further
-				// credentials.
-				return nil, errors.NotFoundf("credentials")
-			}
 			continue
 		}
 		cred.Label = fmt.Sprintf("%s subscription %s", cloudInfo.Name, acc.Name)