Merge pull request #11821 from wallyworld/azure-bootstrap-fixes

#11821

## Description of change

There's some issues bootstrapping to Azure.
The first is that the "Juju CLI" app in the canonical.com AD appears to have been setup as single tenant only. This means that depending on your subscription tenant, you could not bootstrap since it was not possible to create a service principal off this app. In my case, I could bootstrap but autoload-credentials failed.

The fix is to create a new "Juju" app in the canonical.com AD and have it be multi-tenant. The Juju change is to update the app id which is used to create the user's service principal. A drive by fix autoload-credentials was done to replace " " in detected credential names with "_".

The second issue is that for some accounts, resource groups cannot be created by policy. So we add a new Azure specific model config "resource-group-name". This will use an existing resource group. To make it work, Juju cannot be allowed to create a "default" model at bootstrap or else the same resource group will be used for both. So a new --no-default-model arg was added to bootstrap. We want to get rid of the default model eventually anyway.

There's a problem though - since we cannot tag these "read only" resource groups, destroy-controller cannot see them, so any models which use a custom group name will not have resources cleaned up. This is not easily fixed. It will be a known issue if this type of deployment is used. The user will need to destroy all models and then the controller.

## QA steps

install the az cli tool (this will be there anyway if azure used previously)
az logout
az login
juju autoload-credentials
(choose interactive method)

There should be a credential created allowing you to juju bootstrap to azure.

To test the resource group, set up a resource group "juju-test" via the azure dashboard, then

juju bootstrap azure -config resource-group-name=juju-test --no-default-model

The juju-test group should have the controller machine etc in it.
Create a new group juju-test2. Add a model

juju add-model test --config resource-group-name=juju-test2
juju add-machine

The juju-test2 group should have machine and some network items.

juju destroy-model test

The juju-test2 group will have everything deleted but the group will remain behind

## Bug reference

https://bugs.launchpad.net/bugs/1885557
https://bugs.launchpad.net/bugs/1869939

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
 }
 

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 {

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),
 			},
 		},
 	}

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{

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")
 

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
 }

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)

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)