Merge pull request #599 from hmlanigan/model-uuid-not-name

#599

## Description

Add uuid computed attribute to the juju_model resource. It already exists in the juju_model datasource.

The sharedClient ModelUUID method will return the uuid if that's what provided as the ModelIdentifier, otherwise it will check the model cache to convert the model name to a model uuid.

The model cache has been updated to key off the model uuid rather than name to facilitate differentiating models of the same name with different users.

## Type of change

- Change existing resource
- Logic changes in resources (the API interaction with Juju has been changed)

## QA steps

The model and jaas access model acceptance tests have been updated to use and check for model UUID. Results can be seen in the GitHub Actions for this PR.

Run the following plan with `terraform init && terraform plan && terraform apply` against a running juju 3.x controller. Verify that the correct model uuid is listed in the output by comparing with the output of `juju show-model testme | grep model-uuid`
```tf
terraform {
 required_providers {
 juju = {
 version = ">= 0.14.0"
 source = "juju/juju"
 }
 }
}

provider "juju" {
}

resource "juju_model" "test" {
 name = "testme"
}

output {
 value = juju_model.test.uuid
}

```

docs/resources/model.md

@@ -34,7 +34,7 @@ resource "juju_model" "this" {
 
 ### Required
 
-- `name` (String) The name to be assigned to the model
+- `name` (String) The name to be assigned to the model. Changing this value will require the model to be destroyed and recreated by terraform.
 
 ### Optional
 
@@ -47,6 +47,7 @@ resource "juju_model" "this" {
 
 - `id` (String) The ID of this resource.
 - `type` (String) Type of the model. Set by the Juju's API server
+- `uuid` (String) The uuid of the model
 
 <a id="nestedblock--cloud"></a>
 ### Nested Schema for `cloud`

internal/juju/client.go

@@ -17,6 +17,7 @@ import (
 	"github.com/juju/juju/api/client/modelmanager"
 	"github.com/juju/juju/api/connector"
 	"github.com/juju/juju/core/model"
+	"github.com/juju/names/v5"
 )
 
 const (
@@ -63,12 +64,12 @@ func (c Client) IsJAAS() bool {
 }
 
 type jujuModel struct {
-	uuid      string
+	name      string
 	modelType model.ModelType
 }
 
 func (j jujuModel) String() string {
-	return fmt.Sprintf("uuid(%s) type(%s)", j.uuid, j.modelType.String())
+	return fmt.Sprintf("uuid(%s) type(%s)", j.name, j.modelType.String())
 }
 
 type sharedClient struct {
@@ -145,12 +146,14 @@ func (sc *sharedClient) IsJAAS(defaultVal bool) bool {
 }
 
 // GetConnection returns a juju connection for use creating juju
-// api clients given the provided model name.
-func (sc *sharedClient) GetConnection(modelName *string) (api.Connection, error) {
+// api clients given the provided model uuid, name, or neither.
+// Allowing a model name is a fallback behavior until the name
+// used by most resources has been removed in favor of the UUID.
+func (sc *sharedClient) GetConnection(modelIdentifier *string) (api.Connection, error) {
 	var modelUUID string
-	if modelName != nil {
+	if modelIdentifier != nil {
 		var err error
-		modelUUID, err = sc.ModelUUID(*modelName)
+		modelUUID, err = sc.ModelUUID(*modelIdentifier)
 		if err != nil {
 			return nil, err
 		}
@@ -184,7 +187,16 @@ func (sc *sharedClient) GetConnection(modelName *string) (api.Connection, error)
 	return conn, nil
 }
 
-func (sc *sharedClient) ModelUUID(modelName string) (string, error) {
+// ModelUUID returns the model uuid for the provided modelIdentifier.
+// The modelIdentifier can be a model name or model uuid. If the modelIdentifier
+// is a uuid, return that without verification. If a name is provided, first
+// search the modelUUIDCache for the uuid. If it's not found, fill the model
+// cache and try again.
+func (sc *sharedClient) ModelUUID(modelIdentifier string) (string, error) {
+	if names.IsValidModel(modelIdentifier) {
+		return modelIdentifier, nil
+	}
+	modelName := modelIdentifier
 	sc.modelUUIDmu.Lock()
 	defer sc.modelUUIDmu.Unlock()
 	dataMap := make(map[string]interface{})
@@ -193,16 +205,20 @@ func (sc *sharedClient) ModelUUID(modelName string) (string, error) {
 		dataMap[k] = v.String()
 	}
 	sc.Tracef(fmt.Sprintf("ModelUUID cache looking for %q", modelName), dataMap)
-	if modelWithName, ok := sc.modelUUIDcache[modelName]; ok {
-		sc.Tracef(fmt.Sprintf("Found uuid for %q in cache", modelName))
-		return modelWithName.uuid, nil
+	for uuid, m := range sc.modelUUIDcache {
+		if m.name == modelName {
+			sc.Tracef(fmt.Sprintf("Found uuid for %q in cache", modelName))
+			return uuid, nil
+		}
 	}
 	if err := sc.fillModelCache(); err != nil {
 		return "", err
 	}
-	if modelWithName, ok := sc.modelUUIDcache[modelName]; ok {
-		sc.Tracef(fmt.Sprintf("Found uuid for %q in cache on 2nd attempt", modelName))
-		return modelWithName.uuid, nil
+	for uuid, m := range sc.modelUUIDcache {
+		if m.name == modelName {
+			sc.Tracef(fmt.Sprintf("Found uuid for %q in cache on 2nd attempt", modelName))
+			return uuid, nil
+		}
 	}
 	return "", errors.NotFoundf("model %q", modelName)
 }
@@ -226,47 +242,65 @@ func (sc *sharedClient) fillModelCache() error {
 		return err
 	}
 	for _, modelSummary := range modelSummaries {
-		modelWithName := jujuModel{
-			uuid:      modelSummary.UUID,
+		modelWithUUID := jujuModel{
+			name:      modelSummary.Name,
 			modelType: modelSummary.Type,
 		}
-		sc.modelUUIDcache[modelSummary.Name] = modelWithName
+		sc.modelUUIDcache[modelSummary.UUID] = modelWithUUID
 	}
 	return nil
 }
 
-func (sc *sharedClient) ModelType(modelName string) (model.ModelType, error) {
+// ModelType returns the model type for the provided modelIdentifier from
+// the cache of model data. The modelIdentifier can be a name or UUID.
+func (sc *sharedClient) ModelType(modelIdentifier string) (model.ModelType, error) {
 	sc.modelUUIDmu.Lock()
 	defer sc.modelUUIDmu.Unlock()
-	if modelWithName, ok := sc.modelUUIDcache[modelName]; ok {
-		return modelWithName.modelType, nil
+	if names.IsValidModel(modelIdentifier) {
+		if modelWithUUID, ok := sc.modelUUIDcache[modelIdentifier]; ok {
+			return modelWithUUID.modelType, nil
+		}
 	}
 
-	return model.ModelType(""), errors.NotFoundf("type for model %q", modelName)
+	for _, m := range sc.modelUUIDcache {
+		if m.name == modelIdentifier {
+			return m.modelType, nil
+		}
+	}
+
+	return "", errors.NotFoundf("type for model %q", modelIdentifier)
 }
 
+// RemoveModel deletes the model with the given UUID from the cache of
+// model data.
 func (sc *sharedClient) RemoveModel(modelUUID string) {
 	sc.modelUUIDmu.Lock()
-	var modelName string
-	for k, v := range sc.modelUUIDcache {
-		if v.uuid == modelUUID {
-			modelName = k
-			break
-		}
-	}
-	if modelName != "" {
-		delete(sc.modelUUIDcache, modelName)
-	}
+	delete(sc.modelUUIDcache, modelUUID)
 	sc.modelUUIDmu.Unlock()
 }
 
+// AddModel adds a model to the cache of model data. If any of the three required
+// pieces of data are empty, nothing is added to the cache of model data. If the UUID
+// already exists in the cache, do nothing.
 func (sc *sharedClient) AddModel(modelName, modelUUID string, modelType model.ModelType) {
+	if modelName == "" || !names.IsValidModel(modelUUID) || modelType.String() == "" {
+		sc.Tracef("Missing data, failed to add to the cache.", map[string]interface{}{
+			"modelName": modelName, "modelUUID": modelUUID, "modelType": modelType.String()})
+		return
+	}
+
 	sc.modelUUIDmu.Lock()
-	sc.modelUUIDcache[modelName] = jujuModel{
-		uuid:      modelUUID,
+	defer sc.modelUUIDmu.Unlock()
+	if m, ok := sc.modelUUIDcache[modelUUID]; ok {
+		sc.Warnf("Attempting to add an existing model to the cache.", map[string]interface{}{
+			"existing model in cache": m, "new modelName": modelName, "new modelUUID": modelUUID,
+			"new modelType": modelType.String()})
+		return
+	}
+	sc.modelUUIDcache[modelUUID] = jujuModel{
+		name:      modelName,
 		modelType: modelType,
 	}
-	sc.modelUUIDmu.Unlock()
 }
 
 // module names for logging

internal/juju/models.go

@@ -11,6 +11,7 @@ import (
 	"github.com/juju/juju/api/client/modelconfig"
 	"github.com/juju/juju/api/client/modelmanager"
 	"github.com/juju/juju/core/constraints"
+	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/names/v5"
 )
@@ -34,12 +35,6 @@ type modelsClient struct {
 	SharedClient
 }
 
-type GrantModelInput struct {
-	User      string
-	Access    string
-	ModelName string
-}
-
 type CreateModelInput struct {
 	Name        string
 	CloudName   string
@@ -65,29 +60,36 @@ type ReadModelResponse struct {
 
 type UpdateModelInput struct {
 	Name        string
+	UUID        string
 	CloudName   string
 	Config      map[string]string
 	Unset       []string
 	Constraints *constraints.Value
 	Credential  string
 }
 
-type UpdateAccessModelInput struct {
-	ModelName string
-	OldAccess string
-	Grant     []string
-	Revoke    []string
-	Access    string
-}
-
 type DestroyModelInput struct {
 	UUID string
 }
 
+type GrantModelInput struct {
+	User            string
+	Access          string
+	ModelIdentifier string
+}
+
+type UpdateAccessModelInput struct {
+	ModelIdentifier string
+	OldAccess       string
+	Grant           []string
+	Revoke          []string
+	Access          string
+}
+
 type DestroyAccessModelInput struct {
-	ModelName string
-	Revoke    []string
-	Access    string
+	ModelIdentifier string
+	Revoke          []string
+	Access          string
 }
 
 func newModelsClient(sc SharedClient) *modelsClient {
@@ -124,6 +126,8 @@ func (c *modelsClient) GetModelByName(name string) (*params.ModelInfo, error) {
 
 	modelInfo := results[0].Result
 
+	c.AddModel(modelInfo.Name, modelUUID, model.ModelType(modelInfo.Type))
+
 	c.Tracef(fmt.Sprintf("Retrieved model info: %s, %+v", name, modelInfo))
 	return modelInfo, nil
 }
@@ -256,7 +260,7 @@ func (c *modelsClient) ReadModel(name string) (*ReadModelResponse, error) {
 }
 
 func (c *modelsClient) UpdateModel(input UpdateModelInput) error {
-	conn, err := c.GetConnection(&input.Name)
+	conn, err := c.GetConnection(&input.UUID)
 	if err != nil {
 		return err
 	}
@@ -350,7 +354,7 @@ func (c *modelsClient) GrantModel(input GrantModelInput) error {
 
 	client := modelmanager.NewClient(conn)
 
-	modelUUID, err := c.ModelUUID(input.ModelName)
+	modelUUID, err := c.ModelUUID(input.ModelIdentifier)
 	if err != nil {
 		return err
 	}
@@ -367,7 +371,7 @@ func (c *modelsClient) GrantModel(input GrantModelInput) error {
 // If a user has had `write`, then removing that access would decrease their
 // access to `read` and the user will remain part of the model access.
 func (c *modelsClient) UpdateAccessModel(input UpdateAccessModelInput) error {
-	model := input.ModelName
+	model := input.ModelIdentifier
 	access := input.OldAccess
 
 	uuid, err := c.ModelUUID(model)
@@ -412,7 +416,7 @@ func (c *modelsClient) DestroyAccessModel(input DestroyAccessModelInput) error {
 
 	client := modelmanager.NewClient(conn)
 
-	uuid, err := c.ModelUUID(input.ModelName)
+	uuid, err := c.ModelUUID(input.ModelIdentifier)
 	if err != nil {
 		return err
 	}

internal/provider/resource_access_jaas_model_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-testing/plancheck"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 	"github.com/juju/names/v5"
+
 	internaltesting "github.com/juju/terraform-provider-juju/internal/testing"
 )
 
@@ -349,7 +350,7 @@ resource "juju_model" "test-model" {
 }
 
 resource "juju_jaas_access_model" "test" {
-  model_uuid          = juju_model.test-model.id
+  model_uuid          = juju_model.test-model.uuid
   access              = "{{.Access}}"
   users               = ["{{.UserOne}}", "{{.UserTwo}}"]
 }
@@ -370,7 +371,7 @@ resource "juju_model" "test-model" {
 }
 
 resource "juju_jaas_access_model" "test" {
-  model_uuid          = juju_model.test-model.id
+  model_uuid          = juju_model.test-model.uuid
   access              = "{{.Access}}"
   users               = ["{{.User}}"]
 }
@@ -394,7 +395,7 @@ resource "juju_jaas_group" "test" {
 }
 
 resource "juju_jaas_access_model" "test" {
-  model_uuid          = juju_model.test-model.id
+  model_uuid          = juju_model.test-model.uuid
   access              = "{{.Access}}"
   users               = ["{{.User}}"]
   groups              = [juju_jaas_group.test.uuid]
@@ -418,7 +419,7 @@ resource "juju_model" "test-model" {
 }
 
 resource "juju_jaas_access_model" "test" {
-  model_uuid          = juju_model.test-model.id
+  model_uuid          = juju_model.test-model.uuid
   access              = "{{.Access}}"
   service_accounts    = ["{{.SvcAcc}}"]
 }
@@ -438,7 +439,7 @@ resource "juju_model" "test-model" {
 }
 
 resource "juju_jaas_access_model" "test" {
-  model_uuid          = juju_model.test-model.id
+  model_uuid          = juju_model.test-model.uuid
   access              = "{{.Access}}"
   users               = ["{{.User}}"]
   service_accounts    = ["{{.SvcAccOne}}", "{{.SvcAccTwo}}"]

internal/provider/resource_access_model.go

@@ -142,9 +142,9 @@ func (a *accessModelResource) Create(ctx context.Context, req resource.CreateReq
 	// Call Models.GrantModel
 	for _, user := range users {
 		err := a.client.Models.GrantModel(juju.GrantModelInput{
-			User:      user,
-			Access:    accessStr,
-			ModelName: modelNameStr,
+			User:            user,
+			Access:          accessStr,
+			ModelIdentifier: modelNameStr,
 		})
 		if err != nil {
 			resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create access model resource, got error: %s", err))
@@ -281,11 +281,11 @@ func (a *accessModelResource) Update(ctx context.Context, req resource.UpdateReq
 	}
 
 	err := a.client.Models.UpdateAccessModel(juju.UpdateAccessModelInput{
-		ModelName: modelName,
-		OldAccess: oldAccess,
-		Grant:     addedUserList,
-		Revoke:    missingUserList,
-		Access:    access,
+		ModelIdentifier: modelName,
+		OldAccess:       oldAccess,
+		Grant:           addedUserList,
+		Revoke:          missingUserList,
+		Access:          access,
 	})
 	if err != nil {
 		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to update access model resource, got error: %s", err))
@@ -321,9 +321,9 @@ func (a *accessModelResource) Delete(ctx context.Context, req resource.DeleteReq
 	}
 
 	err := a.client.Models.DestroyAccessModel(juju.DestroyAccessModelInput{
-		ModelName: plan.Model.ValueString(),
-		Revoke:    stateUsers,
-		Access:    plan.Access.ValueString(),
+		ModelIdentifier: plan.Model.ValueString(),
+		Revoke:          stateUsers,
+		Access:          plan.Access.ValueString(),
 	})
 	if err != nil {
 		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete access model resource, got error: %s", err))