From 45a2d2d0065ea3ad6938df8025a98b5bf458505d Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 1 Oct 2024 10:48:48 -0400 Subject: [PATCH 1/3] feat: add computed uuid to model resource 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 faciliate differentiating models of the same name with different users. --- docs/resources/model.md | 3 +- internal/juju/client.go | 98 +++++++++++++++------- internal/juju/models.go | 46 +++++----- internal/provider/resource_access_model.go | 22 ++--- internal/provider/resource_model.go | 30 +++++-- internal/provider/resource_model_test.go | 4 + 6 files changed, 132 insertions(+), 71 deletions(-) diff --git a/docs/resources/model.md b/docs/resources/model.md index f37b0fe1..e7f593f3 100644 --- a/docs/resources/model.md +++ b/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 ### Nested Schema for `cloud` diff --git a/internal/juju/client.go b/internal/juju/client.go index e5351f23..a5763f96 100644 --- a/internal/juju/client.go +++ b/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 ( @@ -62,12 +63,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 { @@ -143,12 +144,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 } @@ -182,7 +185,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 a name +// is provided, first search the modelUUIDCache for the uuid. If it's +// not found, fill the model cache and try again. If the modelIdentifier +// is a uuid, return that without verification. +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{}) @@ -191,16 +203,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) } @@ -224,44 +240,64 @@ 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) { +func (sc *sharedClient) ModelName(modelIdentifier string) (string, error) { + if !names.IsValidModel(modelIdentifier) { + return modelIdentifier, nil + } sc.modelUUIDmu.Lock() defer sc.modelUUIDmu.Unlock() - if modelWithName, ok := sc.modelUUIDcache[modelName]; ok { - return modelWithName.modelType, nil + jModel, ok := sc.modelUUIDcache[modelIdentifier] + if ok { + return jModel.name, nil } - - return model.ModelType(""), errors.NotFoundf("type for model %q", modelName) + if err := sc.fillModelCache(); err != nil { + return "", err + } + jModel, ok = sc.modelUUIDcache[modelIdentifier] + var err error + if !ok { + err = fmt.Errorf("unable to find model name for %q", modelIdentifier) + } + return jModel.name, err } -func (sc *sharedClient) RemoveModel(modelUUID string) { +func (sc *sharedClient) ModelType(modelIdentifier string) (model.ModelType, error) { sc.modelUUIDmu.Lock() - var modelName string - for k, v := range sc.modelUUIDcache { - if v.uuid == modelUUID { - modelName = k - break + defer sc.modelUUIDmu.Unlock() + if names.IsValidModel(modelIdentifier) { + if modelWithUUID, ok := sc.modelUUIDcache[modelIdentifier]; ok { + return modelWithUUID.modelType, nil } } - if modelName != "" { - delete(sc.modelUUIDcache, modelName) + + for _, m := range sc.modelUUIDcache { + if m.name == modelIdentifier { + return m.modelType, nil + } } + + return model.ModelType(""), errors.NotFoundf("type for model %q", modelIdentifier) +} + +func (sc *sharedClient) RemoveModel(modelUUID string) { + sc.modelUUIDmu.Lock() + delete(sc.modelUUIDcache, modelUUID) sc.modelUUIDmu.Unlock() } func (sc *sharedClient) AddModel(modelName, modelUUID string, modelType model.ModelType) { sc.modelUUIDmu.Lock() - sc.modelUUIDcache[modelName] = jujuModel{ - uuid: modelUUID, + sc.modelUUIDcache[modelUUID] = jujuModel{ + name: modelName, modelType: modelType, } sc.modelUUIDmu.Unlock() diff --git a/internal/juju/models.go b/internal/juju/models.go index 33358114..bbe41dc4 100644 --- a/internal/juju/models.go +++ b/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,6 +60,7 @@ type ReadModelResponse struct { type UpdateModelInput struct { Name string + UUID string CloudName string Config map[string]string Unset []string @@ -72,22 +68,28 @@ type UpdateModelInput struct { 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 } diff --git a/internal/provider/resource_access_model.go b/internal/provider/resource_access_model.go index 7f78aef5..1fd8a05d 100644 --- a/internal/provider/resource_access_model.go +++ b/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)) diff --git a/internal/provider/resource_model.go b/internal/provider/resource_model.go index 442f8710..ea09f972 100644 --- a/internal/provider/resource_model.go +++ b/internal/provider/resource_model.go @@ -52,6 +52,7 @@ type modelResourceModel struct { Constraints types.String `tfsdk:"constraints"` Credential types.String `tfsdk:"credential"` Type types.String `tfsdk:"type"` + UUID types.String `tfsdk:"uuid"` // ID required by the testing framework ID types.String `tfsdk:"id"` } @@ -67,12 +68,20 @@ func (r *modelResource) Schema(_ context.Context, _ resource.SchemaRequest, resp Description: "A resource that represent a Juju Model.", Attributes: map[string]schema.Attribute{ "name": schema.StringAttribute{ - Description: "The name to be assigned to the model", - Required: true, + Description: "The name to be assigned to the model. Changing this value will" + + " require the model to be destroyed and recreated by terraform.", + Required: true, PlanModifiers: []planmodifier.String{ stringplanmodifier.RequiresReplace(), }, }, + "uuid": schema.StringAttribute{ + Description: "The uuid of the model", + Computed: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.UseStateForUnknown(), + }, + }, "config": schema.MapAttribute{ Description: "Override default model configuration", Optional: true, @@ -224,7 +233,7 @@ func (r *modelResource) Create(ctx context.Context, req resource.CreateRequest, Credential: credential, }) if err != nil { - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create model, got error: %s", err)) + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create model %q, got error: %s", plan.Name, err)) return } r.trace(fmt.Sprintf("model created : %q", modelName)) @@ -246,6 +255,7 @@ func (r *modelResource) Create(ctx context.Context, req resource.CreateRequest, plan.Credential = types.StringValue(response.CloudCredentialName) plan.Type = types.StringValue(response.Type) + plan.UUID = types.StringValue(response.UUID) plan.ID = types.StringValue(response.UUID) r.trace(fmt.Sprintf("model resource created: %q", modelName)) @@ -366,6 +376,7 @@ func (r *modelResource) Read(ctx context.Context, req resource.ReadRequest, resp state.Name = types.StringValue(modelName) state.Type = types.StringValue(response.ModelInfo.Type) state.Credential = types.StringValue(credential) + state.UUID = types.StringValue(response.ModelInfo.UUID) state.ID = types.StringValue(response.ModelInfo.UUID) r.trace(fmt.Sprintf("Read model resource for: %v", modelName)) @@ -455,7 +466,7 @@ func (r *modelResource) Update(ctx context.Context, req resource.UpdateRequest, } err = r.client.Models.UpdateModel(juju.UpdateModelInput{ - Name: plan.Name.ValueString(), + UUID: plan.UUID.ValueString(), CloudName: cloudNameInput, Config: configMap, Unset: unsetConfigKeys, @@ -486,9 +497,14 @@ func (r *modelResource) Delete(ctx context.Context, req resource.DeleteRequest, return } - err := r.client.Models.DestroyModel(juju.DestroyModelInput{ - UUID: state.ID.ValueString(), - }) + modelUUID := state.UUID.ValueString() + if modelUUID == "" { + modelUUID = state.ID.ValueString() + } + arg := juju.DestroyModelInput{ + UUID: modelUUID, + } + err := r.client.Models.DestroyModel(arg) if err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete model, got error: %s", err)) return diff --git a/internal/provider/resource_model_test.go b/internal/provider/resource_model_test.go index 20c38d01..7daa1475 100644 --- a/internal/provider/resource_model_test.go +++ b/internal/provider/resource_model_test.go @@ -5,6 +5,7 @@ package provider import ( "fmt" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -14,6 +15,8 @@ import ( "github.com/juju/juju/rpc/params" ) +var validUUID = regexp.MustCompile(`[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}`) + func TestAcc_ResourceModel(t *testing.T) { modelName := acctest.RandomWithPrefix("tf-test-model") logLevelInfo := "INFO" @@ -29,6 +32,7 @@ func TestAcc_ResourceModel(t *testing.T) { Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "name", modelName), resource.TestCheckResourceAttr(resourceName, "config.logging-config", fmt.Sprintf("=%s", logLevelInfo)), + resource.TestMatchResourceAttr(resourceName, "uuid", validUUID), ), }, { From 590826c85032b1b39a1649c9c69d3d7f01f8cb17 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 1 Oct 2024 12:50:59 -0400 Subject: [PATCH 2/3] fix: use model uuid not id Use juju_model.test_model.uuid in test plans now that uuid is an attribute of juju_model. Follows the examples for this resource. --- internal/provider/resource_access_jaas_model_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/provider/resource_access_jaas_model_test.go b/internal/provider/resource_access_jaas_model_test.go index 71458cf3..8a0e253a 100644 --- a/internal/provider/resource_access_jaas_model_test.go +++ b/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}}"] From 5b0eeb2bb0beccec1655f25bbe0b2cd9689f6c89 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 2 Oct 2024 15:35:01 -0400 Subject: [PATCH 3/3] fix: comments, error handling, remove unused method Added comment headers to all methods not having them for the cache of model data. Added error checking to AddModel, so that existing data is not overwritten, nor is incomplete data added. Removed the unused method ModelName. --- internal/juju/client.go | 52 ++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/internal/juju/client.go b/internal/juju/client.go index a5763f96..dd7f51f1 100644 --- a/internal/juju/client.go +++ b/internal/juju/client.go @@ -186,10 +186,10 @@ func (sc *sharedClient) GetConnection(modelIdentifier *string) (api.Connection, } // ModelUUID returns the model uuid for the provided modelIdentifier. -// The modelIdentifier can be a model name or model uuid. If a name -// is provided, first search the modelUUIDCache for the uuid. If it's -// not found, fill the model cache and try again. If the modelIdentifier -// is a uuid, return that without verification. +// 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 @@ -249,27 +249,8 @@ func (sc *sharedClient) fillModelCache() error { return nil } -func (sc *sharedClient) ModelName(modelIdentifier string) (string, error) { - if !names.IsValidModel(modelIdentifier) { - return modelIdentifier, nil - } - sc.modelUUIDmu.Lock() - defer sc.modelUUIDmu.Unlock() - jModel, ok := sc.modelUUIDcache[modelIdentifier] - if ok { - return jModel.name, nil - } - if err := sc.fillModelCache(); err != nil { - return "", err - } - jModel, ok = sc.modelUUIDcache[modelIdentifier] - var err error - if !ok { - err = fmt.Errorf("unable to find model name for %q", modelIdentifier) - } - return jModel.name, err -} - +// 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() @@ -285,22 +266,39 @@ func (sc *sharedClient) ModelType(modelIdentifier string) (model.ModelType, erro } } - return model.ModelType(""), errors.NotFoundf("type for model %q", modelIdentifier) + 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() 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() + 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